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

Catch SyntaxError and return 400 as well #82

Closed
dominique-pfister opened this issue Oct 29, 2021 · 3 comments
Closed

Catch SyntaxError and return 400 as well #82

dominique-pfister opened this issue Oct 29, 2021 · 3 comments
Labels
enhancement New feature or request wontfix This will not be worked on

Comments

@dominique-pfister
Copy link
Contributor

dominique-pfister commented Oct 29, 2021

In:

if (e instanceof TypeError && e.code === 'ERR_INVALID_CHAR') {
// eslint-disable-next-line no-console
console.error('invalid request header', e.message);
return {
statusCode: 400,
headers: {
'content-type': 'text/plain',
'x-invocation-id': context.awsRequestId,
},
body: e.message,
};
}

we could additionally check whether e is of type SyntaxError, which would indicate that JSON parsing of the request body failed in body-data-wrapper.js (see adobe/helix-shared#593)

Potential downside: some genuine parsing problem elsewhere in the code might be treated as bad request.

@dominique-pfister
Copy link
Contributor Author

Note that this would fix adobe/helix-shared#593

@tripodsan
Copy link
Contributor

I don't think this should be handled here.... if the SyntaxError is thrown where no input-data is involved, i.e. reading from a backend, a 500 would be more appropriate.

@dominique-pfister
Copy link
Contributor Author

Closing as it is now fixed in adobe/helix-shared#593

@dominique-pfister dominique-pfister added the wontfix This will not be worked on label Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request wontfix This will not be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants