Updating the lambda handler to handle invalid JSON body #3108
Conversation
|
@danperkins: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/ |
danperkins
force-pushed
the
lambda-safe-json-parsing
branch
from
9bffdda to
babc688
Compare
danperkins
force-pushed
the
lambda-safe-json-parsing
branch
from
babc688 to
47dfb1d
Compare
| @@ -25,9 +25,12 @@ const createAzureFunction = (options: CreateAppOptions = {}) => { | |||
| req.on('data', chunk => (body += chunk)); | |||
| req.on('end', () => { | |||
| const urlObject = url.parse(req.url, true); | |||
| try { | |||
| body = JSON.parse(body); | |||
| } catch (e) {} | |||
There was a problem hiding this comment.
Should something be logged here for debugging purposes? Maybe at a low log level?
There was a problem hiding this comment.
I only saw one other place in the tests where console logs are being used and it seemed to be more for debugging a failed 'expect' validation. I don't think it makes sense to add logging for a case that will always be hit for a specific test case
There was a problem hiding this comment.
Oh, I misunderstood what was happening. I did not realize this was a testcase. 🤦♂
|
@danperkins, @mdlavin, how will this interact with #1739? I don't think that's close to being merged (it's stalled out), but there's a comment (https://github.com/apollographql/apollo-server/pull/1739/files#r267636555) that mentions that for file uploads to be implemented for Could this be tweaked to only try parsing the body as JSON if the |
|
@dsanders11 should be pretty easy for that branch to pull in this change - they would just wrap the JSON.parse call in a try/catch like I did here. They even already have the code pulled out of the runHttpQuery call just like in this branch |
|
Is there any update on this issue? I would love that to be merged! |
|
The Lambda handler was completely rewritten in AS3 last year (it no longer parses its own JSON), and support for Lambda will be maintained by the community rather than in core starting with AS4 (currently Release Candidate), so this no longer applies. Sorry for the delay! |
We hit this recently after posting our service endpoints for external security testing. Instead of the lambda throwing exceptions and those getting wrapped in 502 - Internal Server Error responses I updated the handler to return a more helpful 400 response.
***Related to #727
Not every handler seems to deal with this the same way, so I just added a unit test that some error code in the 400-500 range should be returned instead of getting an unhandled exception. This seems to work in every scenario except that in the azure test I had to specifically handle parsing the body - I'm guessing that azure is going to take care of this on their side since this code was written specifically for the unit test