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

Warn about deprecations only if token decoding succeeds #600

Merged
merged 2 commits into from
Jun 17, 2024

Conversation

anakinj
Copy link
Member

@anakinj anakinj commented Jun 16, 2024

Description

This should fix the spammy deprecation warnings reported in #595

Checklist

Before the PR can be merged be sure the following are checked:

  • There are tests for the fix or feature added/changed
  • A description of the changes and a reference to the PR has been added to CHANGELOG.md. More details in the CONTRIBUTING.md

@anakinj anakinj force-pushed the context-specific-deprecation-warnings branch from 3fa1014 to 13d0f0a Compare June 16, 2024 19:43
@anakinj anakinj changed the title Warn about deprecations only if no exceptions happen Warn about deprecations only if token decoding succeeds Jun 16, 2024
@anakinj anakinj force-pushed the context-specific-deprecation-warnings branch from 13d0f0a to af64e35 Compare June 16, 2024 19:46
@anakinj
Copy link
Member Author

anakinj commented Jun 16, 2024

@jeremyevans If you have some spare time could you check out this attempt to warn about the upcoming stricter base64 decoding only if the token validation is a success.

@anakinj anakinj force-pushed the context-specific-deprecation-warnings branch from af64e35 to d5dd4e5 Compare June 16, 2024 19:49
Copy link

@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

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

I didn't test this code, but reading it, it looks like if you rescue the exception, the warning won't be emitted then, but it will be emitted later, after the next attempt that does not raise in the same thread.

I'll submit a pull request for an alternative approach and get your thoughts about it.

@anakinj
Copy link
Member Author

anakinj commented Jun 16, 2024

You were right, the warnings leaked over to the next call of decode. Did a little adjustment to prevent that.

@jeremyevans
Copy link

You were right, the warnings leaked over to the next call of decode. Did a little adjustment to prevent that.

My approach in #601 is simpler, does not mess with thread-local state, and uses uplevel to make it easier to find the underlying cause of the problem. It does require base64 decoding segments twice, but as most JWTs are small, I don't expect that should be a significant performance issue.

However, either #601 or this PR will fix the problem, so I'm fine with either.

@anakinj
Copy link
Member Author

anakinj commented Jun 17, 2024

@jeremyevans Thanks for your interest and insight on this one. I decided to go with this more complex and hacky version. Just to avoid doing something for every token decode that goes via that method.

@anakinj anakinj merged commit 1d2b671 into jwt:main Jun 17, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants