Crash on node 12 with certain Http Error Responses

[griffinmyers]

Confirm by changing [ ] to [x] below to ensure that it's a bug:

• I've gone though Developer Guide and API reference
• I've checked AWS Forums and StackOverflow for answers
• I've searched for previous similar issues and didn't find any solution

Describe the bug
Modern versions of node (at least >10, confirmed on v12.13.0) do not handle Error objects where the name property is a non-string properly. As described in nodejs/node#30572, the following simple program will crash when running on node 12.13.0:

const err = new Error('bloop');
err.name = 404;
console.log(err);

with the message:

internal/util/inspect.js:880
      name.endsWith('Error') &&
           ^

TypeError: name.endsWith is not a function
    at formatError (internal/util/inspect.js:880:12)
    at formatRaw (internal/util/inspect.js:681:14)
    at formatValue (internal/util/inspect.js:569:10)
    at inspect (internal/util/inspect.js:223:10)
    at formatWithOptions (internal/util/inspect.js:1651:40)
    at Object.Console.<computed> (internal/console/constructor.js:272:10)
    at Object.log (internal/console/constructor.js:282:61)
    at Object.<anonymous> (/Users/williammyers/projects/js/error-12.js:3:9)
    at Module._compile (internal/modules/cjs/loader.js:774:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:785:10)

It turns out, the AWS SDK will set the name property on certain errors to be the numeric HTTP status code of a failed HTTP request/response. See lib/protocol/query.js#L55-L57 and lib/util.js#L592-L597.

I can simulate this edge case with the following test program:

aws-test.js

const AWS = require('aws-sdk/lib/core');
const { extractError } = require('aws-sdk/lib/protocol/query');

const request = null;

const httpResponse = new AWS.HttpResponse();
httpResponse.statusCode = 404;
httpResponse.body = '';

const response = new AWS.Response(request);
response.httpResponse = httpResponse;
extractError(response)

console.log(response.error)

When run on node v10.15.1:

node aws-test.js
{ 404
    at extractError (/Users/williammyers/projects/js/node_modules/aws-sdk/lib/protocol/query.js:50:29)
    at Object.<anonymous> (/Users/williammyers/projects/js/aws-test.js:12:1)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
    at startup (internal/bootstrap/node.js:283:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:743:3) message: undefined, code: 404, time: 2019-11-21T06:34:18.129Z }

When run on node v12.13.0:

node aws-test.js
internal/util/inspect.js:914
      (name.endsWith('Error') &&
            ^

TypeError: name.endsWith is not a function
    at formatError (internal/util/inspect.js:914:13)
    at formatRaw (internal/util/inspect.js:703:14)
    at formatValue (internal/util/inspect.js:591:10)
    at inspect (internal/util/inspect.js:221:10)
    at formatWithOptions (internal/util/inspect.js:1693:40)
    at Object.Console.<computed> (internal/console/constructor.js:272:10)
    at Object.log (internal/console/constructor.js:282:61)
    at Object.<anonymous> (/Users/williammyers/projects/js/aws-test.js:14:9)
    at Module._compile (internal/modules/cjs/loader.js:956:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:973:10)

Is the issue in the browser/Node.js?
Node.js

If on Node.js, are you running this on AWS Lambda?
No

Details of the browser/Node.js version
node v12.13.0

SDK version number
v2.576.0

To Reproduce (observed behavior)
See description

Expected behavior
See description

Additional context
It looks like node is interested in patching this behavior, and I realize the core team here may not view this as something they're responsible for. Regardless, there are now published node versions that will exhibit some nasty behavior, and I think it's worth considering a fix here as well.

I also appreciate that fixing this, likely by coercing the name property to a string, is technically a breaking API change.

[griffinmyers]

I should add, I'm happy to submit a PR for this if triage suggests that's the approach folks would like to take.

[ajredniwja]

@griffinmyers Thank-you for reaching out to us with your issue.

Team would be happy to review the PR if it is not a breaking change.

[griffinmyers]

Thanks @ajredniwja, happy to submit a PR.

To fix this, we'll need to change the value set on the name property of certain errors raised by the sdk. For instance, nominally, err.name changes from 500 the Number to "500" the string. Depending on your view of things, that could be classified as a breaking API change (userland code could be doing something like:

// ...

try {
  await sqs.sendMessage({ MessageBody: messageBody, QueueUrl: queueUrl }).promise()
} catch (e) {
  if (e.name === 500) {
    // log somewhere special? retry logic?
  }
}

From a more pragmatic view, it looks fairly unlikely that people would observe or depend on this behavior--either AWS's XML parser would need to reject, or the "Code" field would need to be omitted from error responses (from a brief survey, it looks like it's consistently returned by many (all?) of AWS's api).

What do you think?

[SimonWoolf]

Team would be happy to review the PR if it is not a breaking change.

@ajredniwja Could you perhaps review the PR I submitted to fix this bug some two weeks ago, before this issue was filed? #2963

[ajredniwja]

@SimonWoolf I will have the team review the PR.

[BridgeAR]

It is highly unusual that an error name is set to something else than a string. This is just possible by actively manipulating the errors name in an unusual way. I suggest to consider to change the errors to a more "traditional" name and to add an extra property to the errors that contain the status code as number (e.g., statusCode).

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.