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

Show better error message for 413 from nginx #610

Closed
matthew-white opened this issue Mar 13, 2024 · 2 comments · Fixed by getodk/central-frontend#977
Closed

Show better error message for 413 from nginx #610

matthew-white opened this issue Mar 13, 2024 · 2 comments · Fixed by getodk/central-frontend#977
Assignees
Labels
frontend Requires a change to the UI

Comments

@matthew-white
Copy link
Member

matthew-white commented Mar 13, 2024

Before sending a file, Frontend checks that the file doesn't exceed the nginx limit of 100 MB. However, Frontend only checks that for files, not other request bodies, so it's possible for other requests to result in a 413 error from nginx. (Backend itself never seems to return a 413, though I think it should when the bodyParser limit of 250 kB is exceeded: #788.) I don't think I've previously seen a 413 before, but with getodk/central-backend#589 on the way, it now seems very possible.

Currently, if Frontend receives a 413 from nginx, it shows an error message of "Something went wrong: error code 413." It'd be better if it showed a more informative message. In other words, unlike files, we'll allow other large request bodies to be sent to nginx, but then we'll have Frontend show an informative error message for the resulting 413. We thought about having Frontend just not send the request in the first place, but we discussed the possibility that a user might want to increase the limit. Due to getodk/central-backend#609, if we had Frontend enforce the limit, then the user would have to modify Frontend code, which we don't want. We can have Frontend enforce the limit in the future, once getodk/central-backend#609 is resolved: see #781.

@matthew-white
Copy link
Member Author

This will probably involve adding a new if between these two lines: https://github.com/getodk/central-frontend/blob/82438aa97dcd305aac7973736016163a963b95c2/src/util/request.js#L228-L229. The if should check for a request to Backend where the response is a 413 error that is not a Problem.

@matthew-white
Copy link
Member Author

matthew-white commented Apr 23, 2024

We think that a Frontend request that receives a 413 error from nginx will involve sending a file (or a JSON representation of a file, as with the entities CSV file). Given that, let's use a message similar to the existing error message that mentions files (mixin.request.alert.fileSize).

check for a request to Backend

I think we should show this message for any 413 error response that is not a Backend Problem, including for requests that are not to Backend. In other words, the code change should happen within the last if, not before it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Requires a change to the UI
Projects
Status: ✅ done
Development

Successfully merging a pull request may close this issue.

2 participants