feat: add oidc strict state/nonce mode#4191
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughImplements end-to-end OIDC nonce handling: generator, session stash with eviction, redirect helper injecting nonce, and a callback adapter that enforces nonce equality. Integrates the redirect into SSO routing, adds logging redaction for sensitive query params, and updates one Czech localization entry. ChangesOIDC Nonce/State Validation
SSO Provider Routing
Logging Redaction
Czech Localization
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginView as Login View
participant OidcRedirect as oidc_redirect
participant Session
participant OIDCProvider as OIDC Provider
participant CallbackAdapter as NonceValidatingOpenIDConnectAdapter
User->>LoginView: initiate OIDC login
LoginView->>OidcRedirect: call oidc_redirect(provider, process, next_url)
OidcRedirect->>OidcRedirect: generate state_id and nonce
OidcRedirect->>Session: _stash_oidc_nonce(state_id, nonce)
OidcRedirect->>OIDCProvider: redirect with state and nonce in auth params
OIDCProvider->>User: user authenticates and returns id_token + state
User->>CallbackAdapter: callback with id_token and state
CallbackAdapter->>Session: pop expected nonce by state_id
CallbackAdapter->>CallbackAdapter: decode id_token and compare nonce claim
alt nonce matches
CallbackAdapter->>User: complete_login (success)
else nonce missing or mismatched
CallbackAdapter->>User: raise OAuth2Error (failure)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/iam/sso/oidc/views.py`:
- Around line 66-79: The code only performs nonce validation when id_token_str
is present, allowing strict-mode logins to bypass nonce checks if id_token is
missing; update the OIDC callback flow in the view handling id_token_str to
enforce strict-mode failure when no id_token is provided by checking for the
expected nonce even if id_token_str is falsy: retrieve state via
get_request_param(request, "state"), pop the expected_nonce from session using
f"{_STRICT_NONCE_SESSION_PREFIX}{state_id}", and if expected_nonce is not None
then either decode and validate nonce via self._decode_id_token(...) and compare
decoded.get("nonce") to expected_nonce (raising OAuth2Error("OIDC nonce
mismatch") and logging with provider=self.provider_id on mismatch) or, if
id_token_str is missing, immediately log and raise OAuth2Error to fail
strict-mode rather than continuing and setting data["id_token"].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 70d4f4de-061c-48b0-971a-df7289068cce
📒 Files selected for processing (7)
backend/iam/sso/oidc/views.pybackend/iam/sso/serializers.pybackend/iam/sso/views.pyfrontend/messages/en.jsonfrontend/messages/fr.jsonfrontend/src/lib/components/Forms/ModelForm/SsoSettingForm.sveltefrontend/src/lib/utils/schemas.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/messages/cs.json`:
- Line 1875: The translation string oidcStrictStateNonceHelpText currently lists
allowed URL-safe characters as "- . _ ~ ," including an erroneous comma; update
the message in frontend/messages/cs.json to remove the comma so the
allowed-symbols list reads "- . _ ~" (preserving spacing/quoting and Czech
wording), ensuring the key oidcStrictStateNonceHelpText is the only changed
text.
In `@frontend/messages/da.json`:
- Line 1887: The translation for key "oidcStrictStateNonceHelpText" lists an
incorrect comma in the allowed symbol set; update the string to remove the comma
so the symbol sequence matches the actual allowed characters (e.g. "_.-~") and
ensure the rest of the sentence remains unchanged.
In `@frontend/messages/el.json`:
- Line 2283: Update the Greek translation string for the OIDC help text key
"oidcStrictStateNonceHelpText" to remove the incorrect comma from the
allowed-symbols list so it describes only the symbols _. - ~ (e.g., change "A-Z,
a-z, 0-9 και τα σύμβολα - . _ ~ ," to "A-Z, a-z, 0-9 και τα σύμβολα _. - ~" or
similar grammatical order) ensuring the rest of the sentence and punctuation
remain unchanged.
In `@frontend/messages/et.json`:
- Line 2726: The translation for the key oidcStrictStateNonceHelpText includes
an incorrect comma in the allowed-symbol list; update that value so the allowed
symbols are reported as "_.-~" (or listed in the order used in docs) instead of
including a comma, ensuring the Estonian help text reflects the strict-mode
character set correctly for state and nonce.
In `@frontend/messages/hr.json`:
- Line 1795: Update the "oidcStrictStateNonceHelpText" string to remove the
comma from the documented allowed symbol set so it reads the allowed symbols as
"- . _ ~" (i.e., change "- . _ ~ ," to "- . _ ~"); ensure no other punctuation
is added or removed and keep the rest of the wording unchanged so the key
"oidcStrictStateNonceHelpText" accurately reflects the strict charset.
In `@frontend/messages/hu.json`:
- Line 1585: The help text for oidcStrictStateNonceHelpText incorrectly lists a
comma as an allowed symbol; update the Hungarian string to remove the comma from
the allowed-symbol list so it matches the strict charset (e.g., list only A-Z,
a-z, 0-9 and the symbols - . _ ~), preserving existing punctuation and spacing
in the JSON value for oidcStrictStateNonceHelpText.
In `@frontend/messages/ko.json`:
- Line 2377: The Korean translation for the key "oidcStrictStateNonceHelpText"
incorrectly lists a comma as an allowed URL-safe symbol; update the string to
remove the comma and list only the URL-safe symbols underscore, period, hyphen,
and tilde (e.g., "기호 _ . - ~") so it matches the English source and the PR
description.
In `@frontend/messages/lt.json`:
- Line 2569: The help text for the localization key oidcStrictStateNonceHelpText
incorrectly lists allowed symbols with spaces and a trailing comma (which may
suggest a comma is allowed); update the string to explicitly list the intended
symbol set as "_.-~" (no spaces or trailing comma) and adjust the surrounding
wording so it reads that allowed characters are A-Z, a-z, 0-9 and symbols
"_.-~", preserving the rest of the sentence and meaning.
In `@frontend/messages/nl.json`:
- Line 2164: The help text for oidcStrictStateNonceHelpText incorrectly lists a
comma as an allowed character; update the string to remove the comma from the
allowed-character list so it reads "A-Z, a-z, 0-9 and the symbols - . _ ~"
(i.e., remove the "," character from the symbols portion) in the
frontend/messages/nl.json entry for oidcStrictStateNonceHelpText.
In `@frontend/messages/pl.json`:
- Line 1760: The message value for "oidcStrictStateNonceHelpText" includes a
comma in the allowed-symbols list which contradicts strict mode; update the
string for the oidcStrictStateNonceHelpText key to list only the strict-mode
symbols (_.-~) instead of including a comma so the help text matches the
enforced character set.
In `@frontend/messages/tr.json`:
- Around line 1754-1755: The help text for oidcStrictStateNonceHelpText
currently lists allowed characters and mistakenly includes a comma in the symbol
set; update the string so the allowed symbols are shown accurately (A-Z, a-z,
0-9 and - . _ ~) without a trailing comma or wording that implies comma is
allowed, e.g., rephrase to explicitly state "URL-safe 40-character state and
nonce using characters A–Z, a–z, 0–9 and the symbols - . _ ~" and ensure the key
oidcStrictStateNonceHelpText is the one being updated.
In `@frontend/messages/uk.json`:
- Around line 2102-2103: The help text for oidcStrictStateNonceHelpText is
ambiguous about allowed characters; update the string so the allowed charset is
unambiguous by replacing the ",)" fragment that implies a comma is allowed with
the explicit symbol set "_.-~" (so the help text clearly lists A-Z, a-z, 0-9 and
the symbols _ . - ~) and keep the rest of the sentence intact; modify the value
for the key oidcStrictStateNonceHelpText accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e002445-be84-445d-826b-b0760ccb7be0
📒 Files selected for processing (23)
frontend/messages/ar.jsonfrontend/messages/cs.jsonfrontend/messages/da.jsonfrontend/messages/de.jsonfrontend/messages/el.jsonfrontend/messages/es.jsonfrontend/messages/et.jsonfrontend/messages/hi.jsonfrontend/messages/hr.jsonfrontend/messages/hu.jsonfrontend/messages/id.jsonfrontend/messages/it.jsonfrontend/messages/ko.jsonfrontend/messages/lt.jsonfrontend/messages/nl.jsonfrontend/messages/pl.jsonfrontend/messages/pt.jsonfrontend/messages/ro.jsonfrontend/messages/sv.jsonfrontend/messages/tr.jsonfrontend/messages/uk.jsonfrontend/messages/ur.jsonfrontend/messages/zh.json
✅ Files skipped from review due to trivial changes (11)
- frontend/messages/pt.json
- frontend/messages/zh.json
- frontend/messages/ur.json
- frontend/messages/de.json
- frontend/messages/es.json
- frontend/messages/it.json
- frontend/messages/sv.json
- frontend/messages/ro.json
- frontend/messages/ar.json
- frontend/messages/hi.json
- frontend/messages/id.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/iam/sso/oidc/views.py (1)
56-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire a matching nonce whenever one was stashed.
oidc_redirect()always stores an expected nonce, but this callback still succeeds if the provider returns noid_token, and it also only warns when the decodedid_tokenhas nononceclaim. Both paths let the login complete without binding the callback to the original authorization request, which defeats the PR's strict nonce guarantee.🔧 Minimal fix
- data = {} - if fetch_userinfo or (not id_token_str): - data["userinfo"] = self._fetch_user_info(token.token) - if id_token_str: - decoded = self._decode_id_token(app, id_token_str) - state_id = get_request_param(request, "state") - expected_nonce = request.session.pop( - f"{_OIDC_NONCE_SESSION_PREFIX}{state_id}", None - ) - if expected_nonce is not None: - returned_nonce = decoded.get("nonce") - if returned_nonce is None: - logger.warning( - "OIDC id_token has no nonce claim; skipping nonce validation", - provider=self.provider_id, - ) - elif returned_nonce != expected_nonce: - logger.error( - "OIDC nonce mismatch", - provider=self.provider_id, - ) - raise OAuth2Error("OIDC nonce mismatch") + data = {} + state_id = get_request_param(request, "state") + expected_nonce = request.session.pop( + f"{_OIDC_NONCE_SESSION_PREFIX}{state_id}", None + ) + if expected_nonce is not None and not id_token_str: + logger.error( + "OIDC id_token missing in strict nonce mode", + provider=self.provider_id, + ) + raise OAuth2Error("OIDC id_token missing in strict nonce mode") + if fetch_userinfo or (not id_token_str): + data["userinfo"] = self._fetch_user_info(token.token) + if id_token_str: + decoded = self._decode_id_token(app, id_token_str) + if expected_nonce is not None and decoded.get("nonce") != expected_nonce: + logger.error( + "OIDC nonce mismatch", + provider=self.provider_id, + ) + raise OAuth2Error("OIDC nonce mismatch")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/iam/sso/oidc/views.py` around lines 56 - 76, The flow must fail if a nonce was stashed but the provider did not return a matching nonce: when you pop expected_nonce from request.session (using _OIDC_NONCE_SESSION_PREFIX + state_id) and expected_nonce is not None, require an id_token_str and a decoded nonce; if id_token_str is missing throw OAuth2Error (not succeed), if decoded.get("nonce") is None throw OAuth2Error (replace the current warning), and keep the existing error+raise when returned_nonce != expected_nonce; use the existing helpers _decode_id_token, get_request_param and keep provider=self.provider_id in logs to locate the logic.
🧹 Nitpick comments (1)
frontend/messages/cs.json (1)
1874-1875: ⚡ Quick winKeep locale key order stable; avoid reordering during text-only edits.
Line 1875 was moved next to the edited key. Please keep
requirementWeightin its original position and limit this hunk to thebuilderChangedOntext update.💡 Minimal diff
- "builderChangedOn": "změněno v", - "requirementWeight": "Váha" + "requirementWeight": "Váha", + "builderChangedOn": "změněno v"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/messages/cs.json` around lines 1874 - 1875, The edit accidentally reordered locale keys: keep the original key order by reverting the movement of "requirementWeight" and only change the value of "builderChangedOn" to the updated string; locate the JSON object containing "builderChangedOn" and "requirementWeight", restore "requirementWeight" to its previous position, and limit the hunk to just the text update for "builderChangedOn" so no other keys are moved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@backend/iam/sso/oidc/views.py`:
- Around line 56-76: The flow must fail if a nonce was stashed but the provider
did not return a matching nonce: when you pop expected_nonce from
request.session (using _OIDC_NONCE_SESSION_PREFIX + state_id) and expected_nonce
is not None, require an id_token_str and a decoded nonce; if id_token_str is
missing throw OAuth2Error (not succeed), if decoded.get("nonce") is None throw
OAuth2Error (replace the current warning), and keep the existing error+raise
when returned_nonce != expected_nonce; use the existing helpers
_decode_id_token, get_request_param and keep provider=self.provider_id in logs
to locate the logic.
---
Nitpick comments:
In `@frontend/messages/cs.json`:
- Around line 1874-1875: The edit accidentally reordered locale keys: keep the
original key order by reverting the movement of "requirementWeight" and only
change the value of "builderChangedOn" to the updated string; locate the JSON
object containing "builderChangedOn" and "requirementWeight", restore
"requirementWeight" to its previous position, and limit the hunk to just the
text update for "builderChangedOn" so no other keys are moved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: beb4629d-5393-4d5e-bb37-fe4655e22a5b
📒 Files selected for processing (3)
backend/iam/sso/oidc/views.pybackend/iam/sso/views.pyfrontend/messages/cs.json
- restore HEADLESS_ONLY guard - strict nonce validation - redact OAuth2 codes from logs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/ciso_assistant/settings.py`:
- Line 47: The _SENSITIVE_QUERY_PARAMS frozenset in settings.py is missing
"refresh_token"; update the _SENSITIVE_QUERY_PARAMS definition (the variable
named _SENSITIVE_QUERY_PARAMS) to include "refresh_token" alongside "code",
"token", "id_token", and "access_token" so refresh tokens are treated as
sensitive and redacted from logs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68a29751-3f59-4ee8-bb4b-6ea5a445eca7
📒 Files selected for processing (4)
backend/ciso_assistant/settings.pybackend/iam/sso/oidc/views.pybackend/iam/sso/views.pyfrontend/messages/cs.json
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/iam/sso/views.py
- frontend/messages/cs.json
- backend/iam/sso/oidc/views.py
Contributor
Author
|
@eric-intuitem LGTM |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Security & Stability
Localization