architect / functions

AWS Lambda Node runtime helpers for Architect apps
https://arc.codes
163 stars 38 forks source link

Fix body parser to handle borked content type #562

Open mmrwoods opened 3 months ago

mmrwoods commented 3 months ago

I recently upgraded an app using @architect/functions from ^3.14.1 to ^8.1.6 and ran into an error parsing the body from some client requests.

It turned out that there was some client code sending an invalid content type header, formatting it the same as http accept, and including two comma-separated mime types, application/json and text/plain, which I know is just wrong, but the way the body parser handled this was to consider the request to be both json and plain text and then blow up when parsing the body as plain text, with the error below...

{
    "errorType": "TypeError",
    "errorMessage": "The first argument must be of type string or an instance of Buffer, ArrayBuffer, or Array or an Array-like Object. Received an instance of Object",
    "code": "ERR_INVALID_ARG_TYPE",
    "stack": [
        "TypeError [ERR_INVALID_ARG_TYPE]: The first argument must be of type string or an instance of Buffer, ArrayBuffer, or Array or an Array-like Object. Received an instance of Object",
        "    at new from (node:buffer:319:9)",
        "    at parseBody (/var/task/node_modules/@architect/functions/src/http/helpers/body-parser.js:44:22)",
        "    at Runtime.lambda [as handler] (/var/task/node_modules/@architect/functions/src/http/index.js:33:22)"
    ]
}

This commit fixes that by changing the conditions to only parse the body once, even if the content type is malformed and includes multiple types.

This seems to do the job ok, but it might better to actually validate the content type header and blow up with an informative error message.

CLAassistant commented 3 months ago

CLA assistant check
All committers have signed the CLA.

mmrwoods commented 3 months ago

P.S. I am not expecting this to be merged as is, consider it a bug report with test coverage ;-)