Skip to content

fix: reject JWT critical headers - #231

Merged
seonghobae merged 4 commits into
masterfrom
fix/pyjwt-crit-header-20260527
May 27, 2026
Merged

fix: reject JWT critical headers#231
seonghobae merged 4 commits into
masterfrom
fix/pyjwt-crit-header-20260527

Conversation

@seonghobae

@seonghobae seonghobae commented May 27, 2026

Copy link
Copy Markdown
Contributor

Summary

  • upgrade PyJWT from 2.8.0 to 2.13.0 for GHSA-752w-5fwx-jx9f / CVE-2026-32597
  • reject unsupported JWT critical headers in the signed-session HS256 path before trusting payload claims
  • record the JWT critical-header bug pattern in AGENTS.md

Verification

  • PYTHONWARNINGS=error pytest backend/tests/test_auth_real.py
  • pytest backend/tests/test_auth_real.py backend/tests/test_runtime_config_api.py backend/tests/test_main.py
  • python -m bandit -r backend -x backend/tests
  • pytest backend/tests

Summary by CodeRabbit

  • Bug Fixes

    • Strengthened session authentication to reject tokens containing unsupported critical headers, enhancing security.
  • Chores

    • Updated PyJWT dependency to a newer version.
  • Tests

    • Added tests ensuring tokens with unknown crit headers are rejected in both session and OIDC flows.
  • Documentation

    • Added a governance rule requiring JWT/session verification to explicitly reject unsupported critical headers.

Review Change Stack

@seonghobae
seonghobae enabled auto-merge May 27, 2026 02:17
@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f33beb76-267f-4a68-8137-6a91ce6c493d

📥 Commits

Reviewing files that changed from the base of the PR and between 7fd9d63 and 965cc49.

📒 Files selected for processing (2)
  • backend/api/auth.py
  • backend/tests/test_auth_real.py

📝 Walkthrough

Walkthrough

Adds a fail-closed guard that rejects JWTs containing the crit header during session and OIDC verification, documents the governance rule, upgrades PyJWT, and adds tests to assert rejection and prevent decode invocation.

Changes

JWT Critical Header Rejection

Layer / File(s) Summary
Governance and requirement documentation
AGENTS.md
New governance rule specifying that JWT/session verification must fail for unsupported crit headers before accepting payload claims.
Critical header guard and dependency
backend/api/auth.py, backend/requirements.txt
Adds _reject_unsupported_critical_headers and invokes it in OIDC key lookup and legacy HS256 session verification; bumps PyJWT from 2.8.0 to 2.13.0.
Test validation for critical header rejection
backend/tests/test_auth_real.py
New async tests assert that bearer session tokens and OIDC flows with unknown crit headers are rejected with HTTP 401 and that jwt.decode is not called for such tokens.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

"I nibble at headers, a cautious little hare,
If crit is unknown, I toss it in the air.
Reject the odd token, let safe claims stand true,
Tests snug like carrots say the check will do. 🐇"

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main security fix: rejecting JWT critical headers to address a vulnerability in PyJWT.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/pyjwt-crit-header-20260527

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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/api/auth.py`:
- Around line 219-220: The OIDC RS256 branch calls jwt.decode(...) without
rejecting unsupported "crit" headers—add the same pre-check used in the HS256
fallback to the OIDC path: inspect the JWT header (the same `header` variable)
before calling `jwt.decode` and if "crit" is present raise
`_authentication_error()`; update the OIDC branch where `jwt.decode` is invoked
so it performs `if "crit" in header: raise _authentication_error()` immediately
prior to decoding to mirror the legacy fallback behavior.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4ccde349-9a9d-4672-9019-a02e6fa9c01f

📥 Commits

Reviewing files that changed from the base of the PR and between cbf3dbd and 7fd9d63.

📒 Files selected for processing (4)
  • AGENTS.md
  • backend/api/auth.py
  • backend/requirements.txt
  • backend/tests/test_auth_real.py

Comment thread backend/api/auth.py Outdated
@github-actions

github-actions Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

PR governance metadata gate is not ready for 3d17eef0c5bdad5f36e2b9f53a6d0e5afa58028c:

  • Merge state is UNKNOWN; resolve conflicts or refresh mergeability.

@seonghobae

seonghobae commented May 27, 2026

Copy link
Copy Markdown
Contributor Author

Current-head merge gate evidence for e29f6a73252bb5b03d3930b31976fb30ce66b780:

  • CodeRabbit: required status is pass on current head.
  • security: pass on current head.
  • strix: required check fails in latest rerun job https://github.com/Seongho-Bae/naruon/actions/runs/26486951667/job/78014874308 before producing structured findings.
  • The trusted workspace, PR head fetch, and Strix gate self-test completed before the LLM call.
  • The Strix log reports litellm.RateLimitError / OpenAI quota exceeded on all 3 retry attempts, then No fallback models configured.
  • GitHub review state still shows a stale CodeRabbit CHANGES_REQUESTED review on old commit 7fd9d6376ab2199796c6c17c1e82b9698e5eef3e; branch protection has no required PR-review rule, GraphQL shows no unresolved review threads, and current-head CodeRabbit required status is pass. A current-head @coderabbitai review request was posted in fix: reject JWT critical headers #231 (comment).
  • Strix is configured for openai/gpt-5.4; the direct STRIX_OPENAI_API_KEY secret is present but quota-exhausted.
  • GitHub Models catalog was checked and currently exposes openai/gpt-5 family models but no OpenAI GPT-5.4+ model, so switching to GitHub Models would not satisfy the GPT-5.4+ requirement today.

Next action: provide a quota-enabled GPT-5.4+ OpenAI credential or make an OpenAI GPT-5.4+ model available through GitHub Models, then rerun strix. Auto-merge is already enabled.

@seonghobae

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@seonghobae — thanks for the detailed status report. Here's a summary from CodeRabbit's side:

  • CodeRabbit required status — ✅ Pass on the current head (e29f6a7). No action needed here.
  • Stale CHANGES_REQUESTED review — That review was posted against the old commit 7fd9d63. Since there is no required PR-review branch protection rule, no unresolved threads, and the current-head status check is green, that stale review should not gate the merge.
  • strix check⚠️ This is the sole remaining blocker, and it's entirely outside CodeRabbit's scope. The failure is caused by litellm.RateLimitError (OpenAI quota exhausted for openai/gpt-5.4) before any structured findings are produced. The path forward is as you've outlined: provide a quota-enabled GPT-5.4+ OpenAI API key for the STRIX_OPENAI_API_KEY secret, or wait for an equivalent model to become available via GitHub Models.

There is nothing further I can do to unblock the strix gate. Once that quota issue is resolved and the check passes, auto-merge should proceed automatically.

@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@seonghobae — The situation on CodeRabbit's side is unchanged from my previous reply:

  • CodeRabbit required status — ✅ Pass on e29f6a7.
  • Stale CHANGES_REQUESTED review — Not a gating blocker (no required PR-review rule, no unresolved threads).
  • strix — Still the sole remaining blocker, still caused exclusively by the quota-exhausted openai/gpt-5.4 credential. This is outside CodeRabbit's control.

There is nothing new I can act on here. Once a quota-enabled GPT-5.4+ OpenAI credential is provided for STRIX_OPENAI_API_KEY and strix re-runs successfully, auto-merge should proceed.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Temporary merge-gate adjustment requested by repository owner on 2026-05-27 UTC:

  • Current head: e29f6a73252bb5b03d3930b31976fb30ce66b780.
  • security: pass on current head.
  • CodeRabbit: pass on current head.
  • strix: latest required run failed before findings due OpenAI quota exhaustion, not due a structured Strix finding.
  • CodeRabbit reviewDecision is stale from older commit 7fd9d6376ab2199796c6c17c1e82b9698e5eef3e; current-head CodeRabbit required status is pass and no branch protection PR-review rule is enabled.
  • Action: remove only the required strix context from master branch protection, leave security and CodeRabbit required, allow auto-merge, then restore strix immediately after the queued merges finish.

This is a temporary rollback-bound gate adjustment, not a scanner suppression in workflow code.

@seonghobae
seonghobae merged commit 373e665 into master May 27, 2026
14 of 15 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.

1 participant