Handles ValueErrors with invalid hex values in query strings (#954) #963
Conversation
oauth2_provider/backends.py
Outdated
| except ValueError as error: | ||
| raise SuspiciousOperation(error) |
There was a problem hiding this comment.
As raised in #954 (comment) is catching all ValueErrors too broad? I think it should be fine, but it might end up exposing error messages users were never meant to see, possibly resulting in security flaws?
There was a problem hiding this comment.
I'm also a bit worried about this catching "real issues" and causing them to end up not being logged (for example in one Django setup I know SuspiciousOperation won't get logged into Sentry but a raw ValueError would). So I could imagine some time lost as this absorbs a real error.
The minimalist approach here would be to check if error.message == "invalid hex string" (or whatever) and only transform the error then (otherwise just reraising the ValueError). But to be honest this might not be a major thing.
There was a problem hiding this comment.
I've updated it now to check the message of the error, otherwise re-raises the exception as-is.
Codecov Report
@@ Coverage Diff @@
## master #963 +/- ##
==========================================
+ Coverage 96.56% 96.58% +0.02%
==========================================
Files 31 31
Lines 1718 1729 +11
==========================================
+ Hits 1659 1670 +11
Misses 59 59
Continue to review full report at Codecov.
|
|
can you plz fix he flake8 errors in py3.8? |
|
I made some updates @auvipy , but I’m flying a bit blind, as flake8 didn’t report these issues locally. I read the errors from the CI run, and did my best to fix them. |
|
ok wait for the CI |
|
thanks a lot everyone involved |
Fixes #954
Description of the Change
Invalid hex values result in HTTP 500 errors, the purpose of this PR is to convert them to HTTP 400 ones.
Checklist
CHANGELOG.mdupdated (only for user relevant changes)AUTHORS