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

[File Upload] - flawed error handling for bad JSON #1703

Closed
juona opened this issue Sep 21, 2018 · 6 comments · Fixed by #3926
Closed

[File Upload] - flawed error handling for bad JSON #1703

juona opened this issue Sep 21, 2018 · 6 comments · Fixed by #3926

Comments

@juona
Copy link

juona commented Sep 21, 2018

With "apollo-server-express": "^2.1.0".

I am testing my file upload API using a REST client (Insomnia). The Apollo Express Server delegates file upload handling to the Apollo Upload Server by Jayden Seric. This middleware handles multipart requests and requires a few HTTP headers to be set to work correctly. These headers ("operations" and "map") require JSON data. If I pass invalid JSON data to one of these headers, an unhandled exception is thrown asynchronously in the Upload Server thus killing the whole server!

I believe this issue to have been fixed in later releases of the Upload Server. The latest Apollo Express uses apollo-upload-server v5. The apollo-upload-server code has been migrated to graphql-upload and the current version is 8. There is a small change in the API, which I believe to be non-essential. It would be nice to have the dependency updated! : ]

Working on Windows I find forking the apollo-server repo and testing this change a bit of a nuisance but if required I could try to do it.

@AlpacaGoesCrazy
Copy link

AlpacaGoesCrazy commented Sep 21, 2018

could it be similar to #1419 ?

@juona
Copy link
Author

juona commented Sep 21, 2018

Don't think so. #1419 seems specific to the lambda server.

There seems to be a bunch of other problems in the validation/error handling area of the old upload server version. E.g. sending JSON with invalid data can easily hang the request and at a glance the new version appears to address these issues.

@jaydenseric
Copy link

See also #1509 (comment)

@edevil
Copy link

edevil commented Oct 24, 2018

Is this security issue reported somewhere? I think users should be aware of this issue when they are using apollo-server with file uploads. Maybe attach a CVE to @apollographql/apollo-upload-server so that "npm install" reports it?

@abernix
Copy link
Member

abernix commented Dec 5, 2018

This should be addressed by #2054. If you'd be willing to try that alpha release, as shared in #2054 (comment), I'd be happy to know that you'd had success with the upgrade.

I suspect it will fix this problem, so I'll go ahead and close this. Happy to reopen if you find that apollo-server-express@2.3.0-alpha.0 doesn't fix this problem for you!

Thanks for reporting this originally!

@abernix abernix closed this as completed Dec 5, 2018
@abernix
Copy link
Member

abernix commented Dec 13, 2018

The alpha releases didn't identify any problems so I've graduated this to the official apollo-server-*@2.3.0 releases.

abernix added a commit that referenced this issue Apr 10, 2020
Aim to provide file-upload parity support — as already supported within
the other Apollo Server integration packages — via the third-party
`graphql-upload` package.

Co-authored-by: charleswong28 <chung.triniti@gmail.com>
Co-authored-by: Steve Babigian <steve@noisykid.com>
Co-authored-by: Jesse Rosenberger <git@jro.cc>
Closes: #1419
Closes: #1703
Supersedes: #1739
...and therefore...
Closes: #1739
Supersedes: #3676
...and therefore...
Closes: #3676
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants