feat: add pkce support to oidc server#766
Merged
steveiliop56 merged 7 commits intomainfrom Apr 7, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPKCE support was added to the OIDC flow: frontend parsing now includes code_challenge/code_challenge_method; DB schema and queries store code_challenge; repository, service, and controller layers store and validate PKCE (including S256 hashing); token exchange rejects invalid PKCE; tests and test logger utilities updated. Changes
Sequence DiagramsequenceDiagram
participant Client
participant AuthServer as Authorization Server
participant TokenServer as Token Endpoint
participant DB as Database
Client->>AuthServer: GET /authorize (code_challenge, code_challenge_method)
AuthServer->>AuthServer: ValidateAuthorizeParams()
AuthServer->>DB: StoreCode(code, code_challenge, method)
DB-->>AuthServer: stored
AuthServer-->>Client: Redirect with code
Client->>TokenServer: POST /token (code, code_verifier)
TokenServer->>DB: GetOidcCode(code)
DB-->>TokenServer: OidcCode{code_challenge, method}
TokenServer->>TokenServer: ValidatePKCE(stored_challenge, code_verifier)
alt PKCE valid
TokenServer-->>Client: 200 + access_token
else PKCE invalid
TokenServer-->>Client: 400 invalid_grant
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #766 +/- ##
==========================================
- Coverage 19.13% 19.12% -0.02%
==========================================
Files 50 50
Lines 3877 3917 +40
==========================================
+ Hits 742 749 +7
- Misses 3062 3095 +33
Partials 73 73 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dosubot
Bot
added
the
size:L
![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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/oidc_controller_test.go`:
- Around line 499-502: The test builds a PKCE S256 code challenge using
base64.URLEncoding which emits padding; update the encoding to use
base64.RawURLEncoding so the code_challenge is BASE64URL(SHA256(code_verifier))
without '=' padding. Locate the snippet that creates hasher, computes
codeChallenge and assigns codeChallengeEncoded (variables: hasher,
codeChallenge, codeChallengeEncoded) and replace the encoder to
base64.RawURLEncoding while keeping the SHA256 flow unchanged.
- Around line 553-611: The test "Ensure request with invalid PKCE fails" is
asserting a 200 on the token exchange despite sending a mismatched CodeVerifier;
update the assertions in the run closure for the TokenRequest
(controller.TokenRequest) against POST "/api/oidc/token" to expect HTTP 400 and
that the JSON response contains error="invalid_grant" (instead of asserting
recorder.Code == 200); keep the rest of the request construction (code,
CodeVerifier "some-challenge-1", basic auth) unchanged so the test validates
PKCE rejection.
In `@internal/controller/oidc_controller.go`:
- Around line 312-320: ValidatePKCE in oidc_service.go currently compares the
stored S256 code_challenge directly to the raw code_verifier, which inverts
RFC7636 behavior and breaks S256 flows; update the S256 branch in ValidatePKCE
to hash-and-base64url-encode the incoming codeVerifier using the existing
hashAndEncodePKCE function (i.e., compare codeChallenge ==
service.hashAndEncodePKCE(codeVerifier)) so the server computes
BASE64URL(SHA256(code_verifier)) before comparison; keep the plain ("plain")
branch as-is and ensure the function name ValidatePKCE and helper
hashAndEncodePKCE are used for locating the change.
In `@internal/service/oidc_service.go`:
- Around line 765-769: The PKCE code-challenge uses base64.URLEncoding which
includes padding; update the hashAndEncodePKCE function to use
base64.RawURLEncoding so the SHA-256 digest is encoded without '=' padding per
RFC 7636 (i.e., replace base64.URLEncoding.EncodeToString(...) with
base64.RawURLEncoding.EncodeToString(...) in OIDCService.hashAndEncodePKCE).
- Around line 754-763: The PKCE validation in ValidatePKCE is inverted: it
currently hashes the verifier for the "plain" branch and compares raw verifier
for S256; instead, when codeChallengeMethod == "S256" (or any non-"plain"),
compute the BASE64URL(SHA256(codeVerifier>) via
service.hashAndEncodePKCE(codeVerifier) and compare that to the stored
codeChallenge, and for "plain" compare codeChallenge == codeVerifier directly;
update ValidatePKCE to use service.hashAndEncodePKCE(codeVerifier) in the
S256/non-plain path and keep simple equality for the "plain" path so hashed
challenges match correctly.
- Around line 298-303: The PKCE validation currently checks req.CodeChallenge ==
"plain" by mistake and only runs when both fields are non-empty; update the
logic in the OIDC request validation (referencing req.CodeChallenge and
req.CodeChallengeMethod in oidc_service.go) so you reject the plain method and
also reject a non-empty CodeChallenge with an empty CodeChallengeMethod (since
empty means plain in StoreCode). Concretely: when req.CodeChallenge != ""
validate that req.CodeChallengeMethod is present and equal to "S256" (reject if
empty, "plain", or any value other than "S256"); return the same error path used
currently for invalid_request.
🪄 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: d8cb2bca-b6a0-4247-a26e-a82890a42dde
📒 Files selected for processing (11)
frontend/src/lib/hooks/oidc.tsinternal/assets/migrations/000007_oidc_pkce.down.sqlinternal/assets/migrations/000007_oidc_pkce.up.sqlinternal/controller/oidc_controller.gointernal/controller/oidc_controller_test.gointernal/repository/models.gointernal/repository/oidc_queries.sql.gointernal/service/oidc_service.gosql/oidc_queries.sqlsql/oidc_schemas.sqlsqlc.yml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/service/oidc_service.go (1)
752-763: Consider enforcing RFC 7636 verifier shape before hashing.
ValidatePKCEwill currently hash any string, so very short or non-compliant verifiers still work if the stored challenge matches. Tightening this to the RFC 7636 character set and 43–128 length range would prevent weak PKCE configurations from slipping through.🔐 Minimal hardening direction
func (service *OIDCService) ValidatePKCE(codeChallenge string, codeVerifier string) bool { if codeChallenge == "" { return true } + if !isValidPKCEVerifier(codeVerifier) { + return false + } return codeChallenge == service.hashAndEncodePKCE(codeVerifier) }var pkceVerifierRE = regexp.MustCompile(`^[A-Za-z0-9\-._~]{43,128}$`) func isValidPKCEVerifier(value string) bool { return pkceVerifierRE.MatchString(value) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/oidc_service.go` around lines 752 - 763, ValidatePKCE currently hashes any codeVerifier; add RFC 7636 shape checks before hashing: introduce a pkceVerifierRE (^[A-Za-z0-9\-._~]{43,128}$) and an isValidPKCEVerifier helper, call it at the start of ValidatePKCE and return false for invalid verifiers, then only call hashAndEncodePKCE when the verifier passes the regex; keep hashAndEncodePKCE unchanged and reference both ValidatePKCE and hashAndEncodePKCE when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/service/oidc_service.go`:
- Around line 298-303: The current validation only checks CodeChallengeMethod
when CodeChallenge is present, allowing requests that set
req.CodeChallengeMethod (e.g., "S256") but omit req.CodeChallenge to slip
through and persist a code without a challenge; update the validation in the
function handling the auth request (the block that reads req.CodeChallenge and
req.CodeChallengeMethod and the similar block later) to reject the request with
errors.New("invalid_request") whenever req.CodeChallengeMethod is non-empty but
req.CodeChallenge is empty, and keep the existing check that rejects unknown
methods (req.CodeChallengeMethod != "S256" && req.CodeChallengeMethod !=
"plain"); ensure the same logic is applied in both validation sites so StoreCode
never receives a code with a method but no challenge.
---
Nitpick comments:
In `@internal/service/oidc_service.go`:
- Around line 752-763: ValidatePKCE currently hashes any codeVerifier; add RFC
7636 shape checks before hashing: introduce a pkceVerifierRE
(^[A-Za-z0-9\-._~]{43,128}$) and an isValidPKCEVerifier helper, call it at the
start of ValidatePKCE and return false for invalid verifiers, then only call
hashAndEncodePKCE when the verifier passes the regex; keep hashAndEncodePKCE
unchanged and reference both ValidatePKCE and hashAndEncodePKCE when applying
the change.
🪄 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: 90372d13-cf7d-4000-9d37-80558337f9b6
📒 Files selected for processing (17)
internal/assets/migrations/000007_oidc_pkce.down.sqlinternal/assets/migrations/000007_oidc_pkce.up.sqlinternal/controller/context_controller_test.gointernal/controller/health_controller_test.gointernal/controller/oidc_controller.gointernal/controller/oidc_controller_test.gointernal/controller/proxy_controller_test.gointernal/controller/resources_controller_test.gointernal/controller/user_controller_test.gointernal/controller/well_known_controller_test.gointernal/repository/models.gointernal/repository/oidc_queries.sql.gointernal/service/oidc_service.gointernal/utils/tlog/log_wrapper.gosql/oidc_queries.sqlsql/oidc_schemas.sqlsqlc.yml
✅ Files skipped from review due to trivial changes (5)
- internal/controller/resources_controller_test.go
- internal/controller/context_controller_test.go
- sqlc.yml
- internal/controller/health_controller_test.go
- internal/controller/well_known_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/assets/migrations/000007_oidc_pkce.down.sql
- internal/assets/migrations/000007_oidc_pkce.up.sql
- sql/oidc_schemas.sql
- internal/controller/oidc_controller_test.go
Rycochet
previously approved these changes
Apr 7, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/controller/oidc_controller_test.go (1)
654-656: Minor: Misleading variable name.The variable
codeholds the error query parameter, not an authorization code. Consider renaming for clarity:- code := queryParams.Get("error") - assert.NotEmpty(t, code) + errorParam := queryParams.Get("error") + assert.NotEmpty(t, errorParam)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/oidc_controller_test.go` around lines 654 - 656, The test reads the "error" query param into a misleadingly named variable `code`; rename that variable (e.g., to `errParam` or `errorParam`) wherever it's assigned from url.Query().Get("error") and update subsequent references such as the assertion `assert.NotEmpty(t, ...)` to use the new name so the intent (it's an error string, not an auth code) is clear; ensure changes are applied in the same test function in oidc_controller_test.go where url.Query() is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/controller/oidc_controller_test.go`:
- Around line 654-656: The test reads the "error" query param into a
misleadingly named variable `code`; rename that variable (e.g., to `errParam` or
`errorParam`) wherever it's assigned from url.Query().Get("error") and update
subsequent references such as the assertion `assert.NotEmpty(t, ...)` to use the
new name so the intent (it's an error string, not an auth code) is clear; ensure
changes are applied in the same test function in oidc_controller_test.go where
url.Query() is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 75f439ee-49dc-452b-a2ce-10629008e226
📒 Files selected for processing (1)
internal/controller/oidc_controller_test.go
|
Documentation Updates 1 document(s) were updated by changes in this PR: oidcView Changes@@ -51,6 +51,11 @@
- `client_secret_basic`
- `client_secret_post`
+
+Supported PKCE code challenge methods:
+
+- `plain`
+- `S256`
Due to the *mostly* stateless nature of Tinyauth, the user `sub` is based on the client ID and the username. This means that if the username or client ID changes, the `sub` will also change. This can cause issues with some OIDC clients that rely on the `sub` claim to identify the user consistently.
How did I do? Any feedback? |
This was referenced Apr 11, 2026
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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