fix(api): enable PKCE for Unraid OIDC - #2061
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe OIDC flow supports optional S256 PKCE. It generates and caches a verifier, adds the challenge to authorization requests, extracts the verifier during callbacks, and passes it to token exchange. The built-in Unraid.net provider enables PKCE by default. Tests cover discovery, custom endpoints, state, and token exchange. ChangesOIDC PKCE flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant OidcService
participant OidcStateService
participant OpenIDProvider
participant OidcTokenExchangeService
OidcService->>OidcStateService: Store codeVerifier in secure state
OidcService->>OpenIDProvider: Send code_challenge with S256
OpenIDProvider-->>OidcService: Return authorization code and state
OidcService->>OidcStateService: Validate state and extract codeVerifier
OidcService->>OidcTokenExchangeService: Exchange code with verifier
OidcTokenExchangeService->>OpenIDProvider: Submit pkceCodeVerifier
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/src/unraid-api/graph/resolvers/sso/auth/oidc-token-exchange.service.test.ts (1)
206-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse precise types for the grant response and grant call.
Line 206 adds
as any. Lines 219-220 add casts that can hide an incorrect grant call shape. Define a typed token response or typed test factory. AssertauthorizationCodeGrantwith URL and checks matchers instead of casting indexed mock-call values.As per coding guidelines, “Never use the
anytype; prefer precise typing” and “Avoid type casting where possible; prefer establishing correct types at the source.”🤖 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 `@api/src/unraid-api/graph/resolvers/sso/auth/oidc-token-exchange.service.test.ts` around lines 206 - 224, Update the OIDC token exchange test around exchangeCodeForTokens to replace the mockResolvedValue any cast with a precisely typed token response or typed test factory, and avoid casting indexed authorizationCodeGrant mock arguments. Assert the grant call using URL and authorization-check matchers that validate the expected redirect URI, state, and PKCE verifier without hiding an incorrect call shape.Source: Coding guidelines
🤖 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.
Nitpick comments:
In
`@api/src/unraid-api/graph/resolvers/sso/auth/oidc-token-exchange.service.test.ts`:
- Around line 206-224: Update the OIDC token exchange test around
exchangeCodeForTokens to replace the mockResolvedValue any cast with a precisely
typed token response or typed test factory, and avoid casting indexed
authorizationCodeGrant mock arguments. Assert the grant call using URL and
authorization-check matchers that validate the expected redirect URI, state, and
PKCE verifier without hiding an incorrect call shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f5591a3-f055-4d6c-8873-b1f2169009aa
📒 Files selected for processing (11)
.limetech/ai-review-markers/codex-api-oidc-code-binding-pkce-6db58e97e7b8.jsonapi/src/unraid-api/graph/resolvers/sso/auth/oidc-token-exchange.service.test.tsapi/src/unraid-api/graph/resolvers/sso/auth/oidc-token-exchange.service.tsapi/src/unraid-api/graph/resolvers/sso/core/oidc-config.service.tsapi/src/unraid-api/graph/resolvers/sso/core/oidc.service.test.tsapi/src/unraid-api/graph/resolvers/sso/core/oidc.service.tsapi/src/unraid-api/graph/resolvers/sso/models/oidc-provider.model.tsapi/src/unraid-api/graph/resolvers/sso/session/oidc-state-extractor.util.spec.tsapi/src/unraid-api/graph/resolvers/sso/session/oidc-state-extractor.util.tsapi/src/unraid-api/graph/resolvers/sso/session/oidc-state.service.spec.tsapi/src/unraid-api/graph/resolvers/sso/session/oidc-state.service.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2061 +/- ##
==========================================
+ Coverage 53.13% 53.14% +0.01%
==========================================
Files 1041 1041
Lines 72425 72462 +37
Branches 8354 8360 +6
==========================================
+ Hits 38482 38513 +31
- Misses 33816 33822 +6
Partials 127 127 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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: a147b7c0d7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| id_token: 'test-id-token', | ||
| }; | ||
|
|
||
| vi.mocked(client.authorizationCodeGrant).mockResolvedValue(mockTokens as any); |
There was a problem hiding this comment.
In this new PKCE test, mockTokens as any removes type checking from the mocked token response, so the test can keep passing even if the fixture stops satisfying client.TokenEndpointResponse. Please type the fixture/mock instead of casting away checking to match the repo rule against any and avoidable casts.
Useful? React with 👍 / 👎.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
🔄 PR Merged - Plugin Redirected to StagingThis PR has been merged and the preview plugin has been updated to redirect to the staging version. For users testing this PR:
Staging URL: Thank you for testing! 🚀 |
🤖 I have created a release *beep* *boop* --- ## [4.37.1](v4.37.0...v4.37.1) (2026-08-12) ### Bug Fixes * **api:** enable PKCE for Unraid OIDC ([#2061](#2061)) ([5f87782](5f87782)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
1 participant
Summary
The API now generates and preserves an S256 PKCE verifier for the built-in Unraid.net OIDC flow so Account can bind code redemption to the initiating client transaction.
Why This Exists
The Account issuer can enforce S256 PKCE after the client rollout. Without the verifier, a party that obtains an authorization code cannot redeem it at the token endpoint.
Resolution
Generate a verifier per authorization request, send its challenge, carry the verifier through the existing signed transient state, and supply it to
openid-clientduring code exchange. Custom providers remain unchanged unless their internal PKCE capability is enabled.Reviewer Considerations
Behavior Changes
unraid.netprovider opts into S256 PKCE by default.Implementation Summary
Verification
Risk
The accepted residual risk is that concurrent callback state redemption can race before cache deletion is observed. This PR does not claim strict distributed exactly-once state consumption. CLD-868 tracks the stronger atomic consume guarantee in the Account project.
Linear
Review Status
Limetech AI review was run with delegated security, architecture, checklist, type/modeling, tests, documentation, maintenance, and data-safety lenses. The atomic state-consumption limitation is documented, risk-accepted, and tracked in CLD-868. The final AI review marker records that disposition.
Summary by CodeRabbit
New Features
Bug Fixes
Tests