Throw helpful error messages

[kirillgroshkov]

What problem are you trying to solve?

I'm using Github API. On error (non-200 http status) got throws an HTTPError which is by default printed like this:

HTTPError: Response code 422 (Unprocessable Entity)
    at EventEmitter.<anonymous> (.../node_modules/got/dist/source/as-promise.js:118:31)
    at processTicksAndRejections (internal/process/task_queues.js:97:5) {
  name: 'HTTPError'
}

It gives me no information from the response body to be able to debug the issue (e.g on the local machine, or from the server logs in production).

Describe the feature

What I'd like to see is that it'd print the response.body, e.g by including it in Error.message (maybe limiting the maximum length, for sanity). Now I need to add this code to achieve the needed result:

await got(...)
  .catch(err => {
    console.log((err as HTTPError).response.body)
  })

Then it prints like this:

{"message":"Reference already exists","documentation_url":"https://developer.github.com/v3/git/refs/#create-a-reference"}

And suddenly the error message becomes very useful.

I'm aware that I can use Hooks, extend my got instance with some error-handling hook to achieve that (that what I'm going to try now). But wouldn't everyone benefit from such feature enabled by default in got? Or behind a non-default configuration flag? ...

Checklist

• [x] I have read the documentation and made sure this feature doesn't already exist.

[szmarczak]

https://github.com/sindresorhus/got/blob/master/readme.md#errors It's your responsibility to catch the error and read the useful information. Options aren't logged on purpose to prevent the leak of tokens.

@sindresorhus I think each error message should contain the request method and its URL.

[sindresorhus]

Agreed

[yeo-yeo]

FWIW, I had the same problem as @kirillgroshkov today and would have found the proposed functionality useful and intuitive

[kyler-hyuna]

Encountered this today. Error is thrown and no useful information can be derived from the error thrown. Not even the body returned from the request.

[LeonardoRick]

I agree too. I'm starting to use the lib and that was my first unconfortable usecase. Would be awesome to receive formated erros by default.

[Hazmi35]

The problem is we can't identify what kind of actual errors are really sent from the API, for example, 400 Bad Request could mean an invalid token but it could be mean something else too.

[felixfbecker]

got.HTTPError has a lot of useful properties within request and response, but they are non-enumerable for some reason. This means that they don't get printed by console.error() (which uses util.inspect() under the hood). The only property that is enumerable is timings, which is probably the least useful property out of all of them. This makes the log output of Got errors very verbose, with providing almost no useful information in that output (very bad signal/noise ratio):

HTTPError: Response code 403 (Forbidden)
    at Request.<anonymous> (node_modules/got/dist/source/as-promise/index.js:117:42)
    at processTicksAndRejections (internal/process/task_queues.js:93:5) {
  code: undefined,
  timings: {
    start: 1613304139851,
    socket: 1613304139851,
    lookup: 1613304139851,
    connect: 1613304139917,
    secureConnect: 1613304139986,
    upload: 1613304139986,
    response: 1613304140680,
    end: 1613304140681,
    error: undefined,
    abort: undefined,
    phases: {
      wait: 0,
      dns: 0,
      tcp: 66,
      tls: 69,
      request: 0,
      firstByte: 694,
      download: 1,
      total: 830
    }
  }
}

[szmarczak]

In Got v12, error.options is enumerable so it looks like this:

click here

HTTPError: Response code 404 (NOT FOUND)
    at Request.<anonymous> (file:///home/szm/Desktop/got/dist/source/as-promise/index.js:89:42)
    at Object.onceWrapper (node:events:485:26)
    at Request.emit (node:events:390:22)
    at Request.EventEmitter.emit (node:domain:532:15)
    at Request._onResponseBase (file:///home/szm/Desktop/got/dist/source/core/index.js:716:22)
    at processTicksAndRejections (node:internal/process/task_queues:94:5)
    at async Request._onResponse (file:///home/szm/Desktop/got/dist/source/core/index.js:755:13) {
  input: undefined,
  code: 'ERR_NON_2XX_3XX_RESPONSE',
  timings: {
    start: 1626356829477,
    socket: 1626356829477,
    lookup: 1626356829494,
    connect: 1626356829627,
    secureConnect: 1626356829763,
    upload: 1626356829764,
    response: 1626356829902,
    end: 1626356829904,
    error: undefined,
    abort: undefined,
    phases: {
      wait: 0,
      dns: 17,
      tcp: 133,
      tls: 136,
      request: 1,
      firstByte: 138,
      download: 2,
      total: 427
    }
  },
  options: {
    request: undefined,
    agent: { http: undefined, https: undefined, http2: undefined },
    h2session: undefined,
    decompress: true,
    timeout: {
      connect: undefined,
      lookup: undefined,
      read: undefined,
      request: undefined,
      response: undefined,
      secureConnect: undefined,
      send: undefined,
      socket: undefined
    },
    prefixUrl: '',
    body: undefined,
    form: undefined,
    json: undefined,
    cookieJar: undefined,
    ignoreInvalidCookies: false,
    searchParams: undefined,
    dnsLookup: undefined,
    dnsCache: undefined,
    context: {},
    hooks: {
      init: [],
      beforeRequest: [],
      beforeError: [],
      beforeRedirect: [],
      beforeRetry: [],
      afterResponse: []
    },
    followRedirect: true,
    maxRedirects: 10,
    cache: undefined,
    throwHttpErrors: true,
    username: '',
    password: '',
    http2: false,
    allowGetBody: false,
    headers: {
      'user-agent': 'got (https://github.com/sindresorhus/got)',
      accept: 'application/json',
      'accept-encoding': 'gzip, deflate, br'
    },
    methodRewriting: false,
    dnsLookupIpVersion: undefined,
    parseJson: [Function: parse],
    stringifyJson: [Function: stringify],
    retry: {
      limit: 2,
      methods: [ 'GET', 'PUT', 'HEAD', 'DELETE', 'OPTIONS', 'TRACE' ],
      statusCodes: [
        408, 413, 429, 500,
        502, 503, 504, 521,
        522, 524
      ],
      errorCodes: [
        'ETIMEDOUT',
        'ECONNRESET',
        'EADDRINUSE',
        'ECONNREFUSED',
        'EPIPE',
        'ENOTFOUND',
        'ENETUNREACH',
        'EAI_AGAIN'
      ],
      maxRetryAfter: undefined,
      calculateDelay: [Function: calculateDelay],
      backoffLimit: Infinity,
      noise: 100
    },
    localAddress: undefined,
    method: 'GET',
    createConnection: undefined,
    cacheOptions: {
      shared: undefined,
      cacheHeuristic: undefined,
      immutableMinTimeToLive: undefined,
      ignoreCargoCult: undefined
    },
    httpsOptions: {
      alpnProtocols: undefined,
      rejectUnauthorized: undefined,
      checkServerIdentity: undefined,
      certificateAuthority: undefined,
      key: undefined,
      certificate: undefined,
      passphrase: undefined,
      pfx: undefined,
      ciphers: undefined,
      honorCipherOrder: undefined,
      minVersion: undefined,
      maxVersion: undefined,
      signatureAlgorithms: undefined,
      tlsSessionLifetime: undefined,
      dhparam: undefined,
      ecdhCurve: undefined,
      certificateRevocationLists: undefined
    },
    encoding: undefined,
    resolveBodyOnly: false,
    isStream: false,
    responseType: 'text',
    url: URL {
      href: 'https://httpbin.org/https://httpbin.org/status/404',
      origin: 'https://httpbin.org',
      protocol: 'https:',
      username: '',
      password: '',
      host: 'httpbin.org',
      hostname: 'httpbin.org',
      port: '',
      pathname: '/https://httpbin.org/status/404',
      search: '',
      searchParams: URLSearchParams {},
      hash: ''
    },
    pagination: {
      transform: [Function: transform],
      paginate: [Function: paginate],
      filter: [Function: filter],
      shouldContinue: [Function: shouldContinue],
      countLimit: Infinity,
      backoff: 0,
      requestLimit: 10000,
      stackAllItems: false
    },
    setHost: true,
    maxHeaderSize: undefined,
    searchParameters: undefined
  }
}

We could attach error.body but trimmed down to 100 characters.

@sindresorhus Any thoughts?

[sindresorhus]

No one has commented whether it's safe to expose the body on the error though. I'm not against doing it, but I am concerned it will later turn into a security vulnerability of some kind. Someone will have to do some research about it.

One way to figure this out would be to look at other popular request libraries. For example, the Python one doesn't seem to expose the body directly on the error: https://stackoverflow.com/a/58085336/64949

There are multiple ways we could solve this:

• Only do it when process.env.NODE_ENV === 'DEBUG'.
• Make it opt-in via an option.
• Encourage using diagnostic channel for debugging: https://github.com/sindresorhus/got/issues/1792

[szmarczak]

Make it opt-in via an option.

It's already possible via the beforeError hook.

[jgehrcke]

We could attach error.body but trimmed down to 100 characters.

I think this is a sane choice. I would not worry so much about HTTP-client-specific security concerns here because the security problem clearly is in the HTTP server if that decides to write sensitive information into an HTTP response body.

And then of course this should be treated as "bytes", not "characters". In the sense that it should totally be prepared for gibberish from the point of view of a certain text encoding -- it should be able to show arbitrary byte sequences. A good way to approach this would be to do utf-8 decoding with a fallback to showing non-decodable fragments, too. Something like

>>> b'\x80abc'.decode("utf-8", "backslashreplace")
'\\x80abc'

for Python.

[szmarczak]

the security problem clearly is in the HTTP server

I disagree with that. The server may output sensitive info which is useful for debugging. E.g. GitHub hides secrets when running GitHub Actions. Got 12 exposes the entire Options object which should be enough to reproduce the issue and see for yourself.

[jgehrcke]

The server may output sensitive info which is useful for debugging.

But only in development mode :) That's how the web application frameworks work that I used so far. In production mode, no sensitive data (such as error detail) is supposed to leak into the response body.

But after all of course it's about the complete system, and the developer(s) responsible for both.

I am just saying that I think it's fair to -- by default -- log parts of the response body upon error, I think it's of more use than harm; based on many years of experience. Often times, it's exactly the response body that's lacking when people run into an error for the first time and it may contain the critical detail to really understand what happened.

[szmarczak]

In production mode, no sensitive data (such as error detail) is supposed to leak into the response body.

Unfortunately the world isn't perfect :P

Often times, it's exactly the response body that's lacking when people run into an error for the first time and it may contain the critical detail to really understand what happened.

I agree. But still the security concern doesn't go away. We need more feedback.

[erickhavel]

Hmm. What status code is my API supposed to return when the client hasn't specified enough parameters (in a POST request)? I've been using 400, which might arguably be too generic, but Got trips on both 412 (Precondition Failed, which seems like the most correct code) and 422 (Unprocessable Entity).

I don't see a fitting 2xx/3xx code for this scenario. And to be clear I don't mind the lack of descriptive error messages, I'm more worried about the code execution stopping and the user not receiving my helpful/descriptive error message asking to add the specific missing parameters.

Some discussions here:

https://stackoverflow.com/questions/3050518/what-http-status-response-code-should-i-use-if-the-request-is-missing-a-required

https://stackoverflow.com/questions/16133923/400-vs-422-response-to-post-of-data

[erickhavel]

Is my comment irrelevant? Or am I stupid for thinking that it's fine for API's to return 4xx's intentionally, that client's shouldn't crash due to it? Or is it just due to the thread hijacking that my comment got market off-topic?

If someone else stumbles upon this same issue: Needle is the only Node request library I found that handles 4xx like you'd (or let's say I would) expect and outputs the response body instead of throwing an error.

[szmarczak]

@erickhavel This issue is about making errors more helpful. You seem to be asking something different, directly related to your work. Please read the actual docs, Got can return the response body on error status code as well.

[aalexgabi]

TL;DR The default behavior of got library should balance security and pragmatism and should be configurable by the user

@szmarczak @sindresorhus I'm very happy to hear that error.options will become enumerable out of the box in got@12. I reported this a very long time ago here https://github.com/sindresorhus/got/issues/1067 I'm curious what changed in the meantime.

I'm afraid that making whole error.options enumerable it's going to be too verbose unless irrelevant/null/undefined properties are filtered out but it's definitely better than nothing.

Regarding error.body in most cases servers responding with 4xx or 5xx do not include sensitive data but information about why the error occurred. For example Google Storage API returns information about missing permissions which is very helpful for debugging (service account myapp does not have permission storage.objects.write for bucket mybucket).

On one side there is a security concern if

• a program blindly forwards and error to a client: this is no different than forwarding SQL errors to clients and should not be got's concern
• a program blindly logs or prints error objects: it's a reasonable issue and got should have sane defaults that allow for configurable behavior (beforeError handles probably all cases)

On the other side debugging every HTTP issue is taking minutes or hours instead of seconds OR you end up with a lot of boilerplate code OR you have to use a beforeError hook. None of those is preferable.

I propose that at the very least a set of properties considered safe are enumerable by default:

• method
• HTTP status code
• initial URL and final URL because URLs should never contain sensitive information given that it's logged by servers and if it contains sensitive information is not got's concern
• all headers keys but with some header values such as Authorization header replaced with '** sensitive information removed **'

Currently I end up with code like this because got errors are useless in an application that calls multiple endpoints. I can't even know which API call failed unless I use a beforeError hook or this:

  async setMediaStatus(mediaId, status, message) {
      const url = `/media/${mediaId}/events`;
      const body = {
        key: 'status',
        meta: {
          type: status,
          message,
        },
      };

      try {
        await this.api.post(url, {
          json: body,
        }).json();
      } catch (err) {
        err.method = 'POST';
        err.url = url;
        err.body = body;
        throw err;
      }
  }

I would like to see a sane default behavior in a future release of got. I would prefer having sensitive information in logs over writing code like this or spending time trying to reproduce or guess an error to see which API call failed every time it happens.

[kirillgroshkov]

For the record, I (the topic starter) "solved" it for myself by wrapping got with some function that adds beforeError hook by default (and alters some other defaults, unrelated to this issue), and it lives here: https://github.com/NaturalCycles/nodejs-lib/blob/master/src/got/getGot.ts#L68

@aalexgabi, your last message very well described the experience that I had when opening this issue.

[szmarczak]

I'm afraid that making whole error.options enumerable it's going to be too verbose unless irrelevant/null/undefined properties are filtered out but it's definitely better than nothing.

This is on purpose. Copy n' paste those to reproduce the error on any Got instance.

[throrin19]

I agreed, Got Error are useless : we have not the response in the error message and the error is really big to log with useless informations. It's a pain to simplify and know what the error is

[ariv803]

I may have missed it in this thread, but is it still not possible to get the body of the response when there is an error (in the "catch" block)? For example:

Got.get(reqUrl)
      .then((response: Response<string>) => 
        JSON.parse(response.body)
      )
      .catch((error: any) => {
        logger.info("I would like to output the response body here");
        logger.error(error);
      });

thank you!

[throrin19]

I made this to change Got Error and get response error (I use got-cjs, not directly got) :

import type { OptionsOfUnknownResponseBody, RequestError } from 'got-cjs';
import { got as originGot } from 'got-cjs';

interface GotErrorResponse {
    statusCode: number,
    body: unknown,
}

export class GotError extends Error {
    response: GotErrorResponse;

    constructor(err: RequestError) {
        const statusCode        = err.response?.statusCode || 0;
        const { method, url }   = err.options;
        const body              = err.response?.body || err.message;
        const message           = [[statusCode, method, url]
            .filter(Boolean)
            .join(' '), JSON.stringify(body)]
            .filter(Boolean)
            .join('\n');


        super(message);

        this.response = {
            statusCode,
            body,
        };
    }
}

/**
 * Wrapper to got library
 *
 * @param uri - URI to request
 * @param options - Got options
 * @returns Request's response
 */
export const got = async (uri: string | URL, options?: OptionsOfUnknownResponseBody) => {
    try {
        return await originGot(uri, options);
    } catch (err) {
        throw new GotError(err);
    }
};

[monolithed]

This behavior is rather peculiar. The semantics of HTTP responses do not always align with the business logic of applications, and in some cases, a 404 status is a valid response. In the current implementation, it seems that all concise examples from the documentation need to be wrapped in a try/catch block, which is presented as a downside when comparing it to the standard fetch approach.

const GET = async (request: NextRequest): Promise<NextResponse> => {
    try {
        const body = await got.get('...').json();

        return NextResponse.json({
            status: StatusCodes.OK,
            body
        });
    }
    catch (error: HTTPError) {
        let status = error.response?.statusCode;

        switch (status) { 
             case StatusCodes.NOT_FOUND: 
                 let body = {};
             
                 try {
                       const body = JSON.parse(error.response?.body);

                       return NextResponse.json({status, body)});
                 }
                 catch (error) {
                        return NextResponse.json({status)});
                 }
             default: 
                 return NextResponse.json({status: StatusCodes.INTERNAL_SERVER_ERROR});
        }
    }
};

When I used the fetch method, I didn't encounter such issues.