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 internal server errors on requests with unexpected parameters #1721

Merged
merged 4 commits into from
Apr 11, 2022

Conversation

richardebeling
Copy link
Member

@richardebeling richardebeling commented Mar 14, 2022

We had some views where users could trigger server errors if they wanted. Fix them so they result in 400 - Bad Requests, as they should.

For the views that can only be accessed by trusted users (managers, reviewers), it makes sense to keep the logging, as we would assume that the request was triggered by our code and we might want to investigate. A SuspiciousOperation will be logged at Error level, which we should be informed about.

evap/staff/views.py Outdated Show resolved Hide resolved
evap/staff/views.py Outdated Show resolved Hide resolved
@richardebeling
Copy link
Member Author

richardebeling commented Mar 15, 2022

Refactored a bit:

  • Django introduced a BadRequest exception in 3.2. We can use that to return a 400 that shouldn't be logged at error level.
  • I noticed we already use SuspiciousOperations at a few places where requests have missing/wrong arguments. I think it's a fair solution, as it will also result in 400 and the request will be logged.

I'll add tests later.

Regarding #1721 (comment) and #1721 (comment), I'd say we should stick to the intended meanings of 400 and 404, as it will make debugging easier (so no 404 for a parameter that's missing or of the wrong type)

@richardebeling richardebeling force-pushed the fuzzing-adaptions branch 2 times, most recently from 56ba5c0 to 42d2657 Compare March 19, 2022 00:44
@richardebeling
Copy link
Member Author

richardebeling commented Mar 19, 2022

I added tests in a new commit, so you may want to review by commit. It's a rather big pile of code for the tests, partly because most of the views were completely untested before. On the bright side, these tests cover 8 views from #1630. Coverage increases by 0.5%.

Copy link
Member

@niklasmohrin niklasmohrin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't looked at tests yet, rest looks good

evap/evaluation/tools.py Outdated Show resolved Hide resolved
@richardebeling richardebeling force-pushed the fuzzing-adaptions branch 2 times, most recently from 1a65179 to 3582101 Compare March 26, 2022 15:52
@richardebeling richardebeling force-pushed the fuzzing-adaptions branch 2 times, most recently from b776466 to feea880 Compare April 4, 2022 19:17
evap/staff/tests/test_views.py Outdated Show resolved Hide resolved
evap/staff/tests/test_views.py Outdated Show resolved Hide resolved
evap/staff/tests/test_views.py Outdated Show resolved Hide resolved
@richardebeling richardebeling merged commit f09b510 into e-valuation:main Apr 11, 2022
@richardebeling richardebeling deleted the fuzzing-adaptions branch April 11, 2022 16:19
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.

4 participants