Skip to content

Fix/exchange auth code sanitize redirect uri#230

Open
terfex wants to merge 8 commits into
kinde-oss:mainfrom
Simulnetic:fix/exchange-auth-code-sanitize-redirect-uri
Open

Fix/exchange auth code sanitize redirect uri#230
terfex wants to merge 8 commits into
kinde-oss:mainfrom
Simulnetic:fix/exchange-auth-code-sanitize-redirect-uri

Conversation

@terfex
Copy link
Copy Markdown

@terfex terfex commented May 11, 2026

Closes #228

Summary

redirect_uri is sanitized when it's sent to /oauth2/authorize
(via mapLoginMethodParamsForUrlsanitizeUrl) but not when it's sent
to /oauth2/token (in exchangeAuthCode). Because sanitizeUrl strips
trailing slashes, any consumer whose configured redirectURL ends in /
hits invalid_grant: "redirect_uri … does not match the one from the authorize request" on token exchange. This applies the same
normalization on both sides so the two requests always agree.

Changes

  • lib/utils/exchangeAuthCode.ts: wrap the outgoing redirect_uri in
    sanitizeUrl(...) — matches the existing behavior of
    mapLoginMethodParamsForUrl.ts.
  • lib/utils/exchangeAuthCode.test.ts: cover the trailing-slash case.

Notes

  • Behavior unchanged for consumers whose redirectURL is already
    trailing-slash-free (which is what authorize was effectively seeing
    before this fix anyway).
  • Considered the alternative — adding a disableUrlSanitization flag to
    exchangeAuthCode to mirror mapLoginMethodParamsForUrl — but
    asymmetric defaults are the bug; symmetric defaults are the fix. Happy
    to add the opt-out flag if maintainers want it for parity with the
    authorize side.

Test plan

  • npm run lint
  • npm test
  • New test covers the regression.

Checklist

🛟 If you need help, consider asking for advice over in the Kinde community.

terfex and others added 6 commits May 12, 2026 01:27
Add test to ensure sanitized redirect_uri matches expected value.
The new test was missing storage setup (state + codeVerifier) and a
fetchMock response, causing exchangeAuthCode to bail out early before
reaching fetch. Also fixes prettier formatting in types.ts and the test
file.

https://claude.ai/code/session_01MfwF59xe6Rs5kyxnQuPRAR
This file was never tracked in the project and is generated locally by npm install.

https://claude.ai/code/session_01MfwF59xe6Rs5kyxnQuPRAR
When callers pass disableUrlSanitization: true to generateAuthUrl the
authorize request sends the raw redirectURL. The token exchange must use
the same value; unconditionally calling sanitizeUrl() would produce a
mismatched redirect_uri and cause the provider to reject the exchange.

Adds the disableUrlSanitization option (default false) to
ExchangeAuthCodeParams and mirrors the same conditional used in
mapLoginMethodParamsForUrl. Also adds a test covering the raw-URI path.

https://claude.ai/code/session_01MfwF59xe6Rs5kyxnQuPRAR
fix: repair sanitized redirect_uri test and prettier formatting
@terfex terfex requested a review from a team as a code owner May 11, 2026 16:52
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 11, 2026

Review Change Stack

Warning

Rate limit exceeded

@terfex has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 52 minutes and 2 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 27dcbcbc-7fae-48a7-9eb1-c59bea9381ab

📥 Commits

Reviewing files that changed from the base of the PR and between 2682983 and 1a77b55.

📒 Files selected for processing (1)
  • .gitignore

Walkthrough

The PR adds conditional URL sanitization to exchangeAuthCode to align redirect URI handling with the authorize endpoint. It introduces an optional disableUrlSanitization parameter (default false) that controls whether the redirect URL is stripped of trailing slashes before token exchange. Tests verify both sanitized and raw redirect URI behaviors.

Changes

Token Exchange Redirect URI Sanitization

Layer / File(s) Summary
Parameter Type Definition
lib/utils/exchangeAuthCode.ts
Imports sanitizeUrl and adds optional disableUrlSanitization?: boolean field to ExchangeAuthCodeParams.
Function Implementation
lib/utils/exchangeAuthCode.ts
Destructures disableUrlSanitization parameter and conditionally applies sanitizeUrl() to redirectURL in the token request body based on the flag.
Test Coverage
lib/utils/exchangeAuthCode.test.ts
Two new tests verify that redirect_uri is sanitized (trailing slash removed) by default and preserved when disableUrlSanitization: true.

Unrelated Changes

  • .gitignore: Added package-lock.json to ignore list.
  • lib/sessionManager/types.ts: Reformatted SessionBase class declaration generic parameter list (no functional change).

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title directly addresses the main change: sanitizing redirect_uri in exchangeAuthCode to fix the redirect_uri mismatch issue.
Description check ✅ Passed The description clearly relates to the changeset and explains the bug, proposed fix, changes made, and testing performed.
Linked Issues check ✅ Passed The PR fully implements the fix for issue #228 by wrapping redirect_uri in sanitizeUrl() in exchangeAuthCode, adding tests for both sanitized and raw cases, and includes a flag to optionally disable sanitization.
Out of Scope Changes check ✅ Passed The .gitignore update is a minor housekeeping change; all other changes directly address issue #228. The multi-line formatting in SessionBase is unrelated but cosmetic-only.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

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.

Bug: exchangeAuthCode sends un-sanitized redirect_uri, causing invalid_grant when configured URL has a trailing slash

2 participants