Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make some normalized options enumerable #1067

Closed
aalexgabi opened this issue Feb 14, 2020 · 4 comments
Closed

Make some normalized options enumerable #1067

aalexgabi opened this issue Feb 14, 2020 · 4 comments
Labels
enhancement This change will extend Got features ✭ help wanted ✭

Comments

@aalexgabi
Copy link

Got errors have an options property which is not enumerable:

enumerable: false,

This was introduced because of a security issue: #754

But the chosen solution is not good for all cases. This means that in a service that does many requests it's impossible to know where the error comes from unless remembering to do manual exception wrapping in all places http requests are initiated:

  try {
        await got.patch(url, {
          json: body,
          headers: {
            Authorization: `Bearer ${configuration.get('api.token')}`,
          },
        });
      } catch (err) {
        err.context = _.omit(err.options, ['options.headers.Authorization'])
        throw err;
      }

Without it I get errors like this in logs which are useless:

    HTTPError: Response code 500 (Internal Server Error)
        at EventEmitter.<anonymous> (.../node_modules/got/dist/source/as-promise.js:118:31)
        at runMicrotasks (<anonymous>)
        at processTicksAndRejections (internal/process/task_queues.js:97:5)

I think this needs to be improved. For example one solution would be to have a default list of properties paths that should not be enumerable (ex. headers.Authorization) that can be overwritten by the user.

The very least would be to make the url, method and status code enumerable.

@sindresorhus
Copy link
Owner

The very least would be to make the url, method and status code enumerable.

I agree. We should whitelist some known-to-be-safe properties.

@szmarczak szmarczak added enhancement This change will extend Got features ✭ help wanted ✭ labels Feb 14, 2020
@szmarczak szmarczak changed the title Got errors are useless or require too much work because err.options is not enumerable Make some normalized options enumerable Feb 14, 2020
@szmarczak szmarczak mentioned this issue Apr 1, 2020
1 task
@szmarczak
Copy link
Collaborator

This also applies to error.response.

@robcresswell
Copy link

robcresswell commented Oct 22, 2020

@szmarczak Do you have any thoughts on what a solution might look like here? The quirk from my own usage with got is that logging the error thrown is not sufficient; you have to know to log the response body to get usable error messages usually, which isn't ideal for new consumers of the library

I recognise that this is documented right at the top of the README under Usage, but it'd be nice if the default behaviour would log usable error content.

@szmarczak
Copy link
Collaborator

Closing in favor of #1353

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This change will extend Got features ✭ help wanted ✭
Projects
None yet
Development

No branches or pull requests

4 participants