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

Fix body parser to handle borked content type #562

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mmrwoods
Copy link

@mmrwoods mmrwoods commented Jul 8, 2024

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.

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
Copy link

CLAassistant commented Jul 8, 2024

CLA assistant check
All committers have signed the CLA.

@mmrwoods
Copy link
Author

mmrwoods commented Jul 8, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants