-
Notifications
You must be signed in to change notification settings - Fork 17
chore: JWT warnings on failed JWT ops #124
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #124 +/- ##
==========================================
+ Coverage 77.26% 77.41% +0.14%
==========================================
Files 41 42 +1
Lines 4324 4361 +37
Branches 836 843 +7
==========================================
+ Hits 3341 3376 +35
Misses 718 718
- Partials 265 267 +2
Continue to review full report at Codecov.
|
did it, thank you |
@jschlyter finally got tests working well but py36 seems stucked :) |
I think it is time to say goodbye to 3.6, see #128 - it was EOL over 6 months ago. |
ok, so this is ready to be merged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it is a good idea to log the entire JWT on signature failures. I'd prefer if we the caller decides to do that. If an attacker submits a huge JWT for processing, ending up with it in log files (as a warning) is suboptimal.
One could include the JWT in the exception if that helps the caller. In that case we can defined a BadJwtSignature
based on BadSignature
that has a jwt attribute.
Good comment! |
Ok, this PR will take more time and design don't block the next release for this, we can leave as it is with the pending work to be completed and have the next release with the other PRs that are good right now |
I'd like to have a warning meggase on failing JWT validations, or may use a debug for that and not a warning?
The messages must be improved.
actually with this PR we have