Skip to content

fix: use up-to-date kid in JWT header when refreshing #3973

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tilgovi
Copy link

@tilgovi tilgovi commented Apr 5, 2025

This PR is a variation on #3942 that attempts to solve the problem by letting fosite set the kid header of tokens and removing all of the code to explicitly set this header in JWTs.

@tilgovi tilgovi requested review from aeneasr and a team as code owners April 5, 2025 19:27
@tilgovi tilgovi force-pushed the refresh-kid branch 4 times, most recently from 576225f to ed11711 Compare April 5, 2025 21:09
@tilgovi
Copy link
Author

tilgovi commented Apr 7, 2025

Not sure if there's anything I should do about the CodeQL scanning results task failing. It seems spurious.

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Thank you and good approach, some ideas to improve it further. I accepted the CodeQL issues

"extra": {
}
},
"headers": null,
Copy link
Member

Choose a reason for hiding this comment

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

Please keep the headers.extra keys, as changing them to null will potentially break webhook receivers.

rotateJwks("hydra.jwt.access-token")
rotateJwks("hydra.openid.id-token")

cy.refreshTokenBrowser(client, tokensBefore.refresh_token).then(
Copy link
Member

Choose a reason for hiding this comment

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

This test isn't actually checking that the kid is set correctly, it only validates its non-equality. Can you please make sure that the kid is set correctly - either with a regex (expect a non-nil uuid) or some other way?

From the snapshots it looks like it was public:hydra.jwt.access-token before. What is the value now?

@tilgovi
Copy link
Author

tilgovi commented Apr 17, 2025

I pushed up three commits to restore the extra key, validate the UUID format, and remove some of the extraneous diff.

@tilgovi
Copy link
Author

tilgovi commented Apr 17, 2025

Also, please let me know how you'd like me to resolve conflicts, if at all. I can merge or rebase or squash at your preference.

@tilgovi
Copy link
Author

tilgovi commented Apr 17, 2025

I realized the checks wouldn't even run due to the conflict, so I rebased and squashed.

@tilgovi
Copy link
Author

tilgovi commented Apr 18, 2025

And, I think the CI jobs flaked, but I can't re-run them. 😞

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.

3 participants