fix(oidc): validate ID tokens on refresh - #1898
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1898 +/- ##
==========================================
+ Coverage 87.09% 87.17% +0.08%
==========================================
Files 352 352
Lines 13404 13624 +220
Branches 640 671 +31
==========================================
+ Hits 11674 11877 +203
- Misses 1493 1504 +11
- Partials 237 243 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR strengthens the OpenID Connect refresh flow by validating any id_token returned from a refresh response and enforcing identity continuity before updated tokens/claims are persisted to extra_data. This addresses the gap described in Issue #589, where refresh responses could previously overwrite a trusted ID token without verification.
Changes:
- Add refresh-response ID token validation (
process_refresh_token_response) and introduce persisted identity-continuity context (_oidc_id_token_context) inOpenIdConnectAuth. - Enforce continuity checks across refreshes for issuer/subject/audience set, plus optional claims like
azp,nonce, andauth_timewhen present. - Add comprehensive backend tests covering successful refresh, missing/invalid tokens, and continuity violations; document the security fix in the changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| social_core/backends/open_id_connect.py | Validates refreshed ID tokens and persists/validates identity continuity context across refreshes. |
| social_core/tests/backends/test_open_id_connect.py | Adds end-to-end refresh tests ensuring refreshed ID tokens are validated and identity continuity is enforced. |
| social_core/tests/backends/open_id_connect.py | Extends test token-response builder to support refresh scenarios and additional ID token claims (e.g., azp, auth_time). |
| CHANGELOG.md | Adds an Unreleased security entry describing the new refresh ID token validation behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5155b8a1c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78a6ceedf6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
social_core/backends/open_id_connect.py:488
azpis optional for single-audience ID tokens (andvalidate_authorized_party()already allows it to be absent unless multiple audiences are present). The current continuity check rejects a refreshed ID token that omitsazpeven if all other identity claims are unchanged, which can cause refresh failures when providers includeazpinconsistently.
if previous.get("azp") != current.get("azp"):
raise AuthTokenError(self, "Incorrect refreshed id_token: azp")
Refresh responses could replace a trusted ID token without validating its signature or identity continuity. Validate refreshed tokens and bind their subject, audiences, authorized party, nonce, and authentication time before persistence. Closes python-social-auth#589
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a4de81707
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@nijel I'm not sure how well you have reviewed this, but I would be very skeptical that this actually does the correct things. LLMs seem to overfit these kind of scenarios and since I opened the original issue and have patched this in a code base to work, I'm not sure this is what we would want. I would personally revert this, but feel free to move forward with this change if you really think it's good. It just changes quite a few unrelated things, does add a lot of potentially unnecessary new code and just feels like slop. But I might be wrong. This is not to say that LLMs cannot be helpful in some cases... cc @alakae You might want to know about this and maybe pin previous versions or review this, just to let you know. |
|
@davidhalter the original patch was pretty limited, but perhaps I shouldn't have followed all the review feedback. Will get back to this tomorrow... |
Why was it limited? I understand that it might not be optimal, but I think this OpenID Connect is a very security sensitive topic and I'm not sure I would want any change that is not clearly explainable. |
|
I started with simple ID token validation + checking that claims match the original token. Additional code is from added claims validation. It probably should have been done in a separate pull request, but I folded it here because unvalidated original initial claims make validating refreshed claims harder (and this is part that was not covered in #589 at all). Further additional complexity comes from This also shows why maintaining this library is tricky. There are dozens of backends that do not always follow the same conventions, and most of these were one-off contributions years back with no ongoing maintenance from people that actually use them. LLM is great at pointing out such compatibility issues, but the decision on whether to try to be compatible at the cost of additional complexity is on me. |
|
#1905 is the cleanup that removes some of the additional logic. |
Refresh responses could replace a trusted ID token without validating its signature or identity continuity. Validate refreshed tokens and bind their subject, audiences, authorized party, nonce, and authentication time before persistence.
Closes #589