feat(core): explicit double-submit validation via doubleSubmitToken#233
Open
halvaradop wants to merge 2 commits into
Open
feat(core): explicit double-submit validation via doubleSubmitToken#233halvaradop wants to merge 2 commits into
halvaradop wants to merge 2 commits into
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR adds ChangesCSRF and session validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant createAuthAPI
participant assertDoubleSubmitToken
participant AuthEndpoint
participant SessionStrategy
createAuthAPI->>assertDoubleSubmitToken: normalize doubleSubmitToken and CSRF headers
assertDoubleSubmitToken-->>createAuthAPI: return updated options
createAuthAPI->>AuthEndpoint: delegate auth request
AuthEndpoint->>SessionStrategy: validate or update session with combined CSRF condition
SessionStrategy-->>AuthEndpoint: return session result
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/core/test/api/stateless/signOut.test.ts (1)
121-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImplement or remove this placeholder test.
test("")adds no regression coverage for sign-out’s double-submit-token behavior. Add valid and invalid token cases instead.🤖 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 `@packages/core/test/api/stateless/signOut.test.ts` around lines 121 - 122, Replace the empty test placeholder in the sign-out test suite with regression coverage for double-submit-token behavior, including both valid and invalid token cases. Use the existing sign-out test setup and assertions to verify the valid token succeeds and the invalid token is rejected.
🤖 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 `@packages/core/src/api/createApi.ts`:
- Around line 101-102: Update assertDoubleSubmitToken and all its call sites in
packages/core/src/api/createApi.ts at lines 101-102, 126-127, 150-151, 211-212,
231-236, 254-255, and 295-296 so header initialization uses options.headers when
provided, otherwise options.request?.headers, before setting x-csrf-token;
preserve request Cookie headers for sign-in, sign-up, session updates, user-info
refresh, token revocation, provider disconnect, and sign-out.
In `@packages/core/src/api/signInCredentials.ts`:
- Around line 16-21: Stop using doubleSubmitToken as a CSRF-skip flag: in
packages/core/src/api/signInCredentials.ts:16-21 and
packages/core/src/api/revokeToken.ts:62-79, pass only skipCSRFCheck to
verifyCSRFToken; in packages/core/src/api/updateSession.ts:15-22, pass
skipCSRFCheck to refreshSession. Preserve the wrapper’s behavior of setting
skipCSRFCheck to false when injecting the token so double-submit validation
remains enforced.
In `@packages/core/src/api/signUp.ts`:
- Around line 16-21: Use the normalized skipCSRFCheck value directly instead of
combining it with Boolean(doubleSubmitToken), so supplying a double-submit token
still performs CSRF validation. Update signUp.ts at lines 16-21 and signOut.ts
at lines 12-16 to pass skipCSRFCheck directly, and update refreshUserInfo.ts at
lines 19-36 to do the same for verifyCSRFToken and log the actual normalized
skip state.
---
Nitpick comments:
In `@packages/core/test/api/stateless/signOut.test.ts`:
- Around line 121-122: Replace the empty test placeholder in the sign-out test
suite with regression coverage for double-submit-token behavior, including both
valid and invalid token cases. Use the existing sign-out test setup and
assertions to verify the valid token succeeds and the invalid token is rejected.
🪄 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 Plus
Run ID: 7723c559-67b0-4aa4-9023-dac12db416ac
📒 Files selected for processing (21)
packages/core/CHANGELOG.mdpackages/core/src/@types/api.tspackages/core/src/@types/config.tspackages/core/src/actions/providers/tokens/tokens.tspackages/core/src/api/createApi.tspackages/core/src/api/getProviderTokens.tspackages/core/src/api/refreshUserInfo.tspackages/core/src/api/revokeToken.tspackages/core/src/api/signInCredentials.tspackages/core/src/api/signOut.tspackages/core/src/api/signUp.tspackages/core/src/api/updateSession.tspackages/core/src/router/context.tspackages/core/src/shared/utils.tspackages/core/src/shared/utils/api.tspackages/core/test/actions/providers/tokens/tokens/stateful.test.tspackages/core/test/actions/providers/tokens/tokens/stateless.test.tspackages/core/test/api/stateful/getProviderTokens.test.tspackages/core/test/api/stateless/getAccessToken.test.tspackages/core/test/api/stateless/getProviderTokens.test.tspackages/core/test/api/stateless/signOut.test.ts
💤 Files with no reviewable changes (4)
- packages/core/src/actions/providers/tokens/tokens.ts
- packages/core/test/api/stateless/getProviderTokens.test.ts
- packages/core/test/api/stateless/getAccessToken.test.ts
- packages/core/src/shared/utils.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/core/src/api/revokeToken.ts (2)
62-79: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate
doubleSubmitTokeninto the validation headers.
doubleSubmitTokenis only used as a boolean here. ThecreateAuthAPIwrapper callsrevokeTokendirectly, withoutassertDoubleSubmitToken, so noX-CSRF-Tokenheader is injected and callers cannot successfully opt into double-submit validation.🤖 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 `@packages/core/src/api/revokeToken.ts` around lines 62 - 79, Update the validation setup in revokeToken to propagate the provided doubleSubmitToken into the headers passed to createValidation, ensuring the corresponding X-CSRF-Token header is available when double-submit validation is enabled. Preserve the existing boolean skipCSRFCheck behavior and direct createAuthAPI invocation path.
62-79: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInject supplied double-submit tokens before validation. Both handlers only consume
doubleSubmitTokenas a boolean, while their public wrappers delegate withoutassertDoubleSubmitToken; valid caller-supplied tokens never reach theX-CSRF-Tokenheader.
packages/core/src/api/revokeToken.ts#L62-L79: normalize the public options withassertDoubleSubmitTokenbefore invoking this handler, or inject the header here.packages/core/src/api/signOut.ts#L12-L16: normalize the public options withassertDoubleSubmitTokenbefore invoking this handler, or inject the header here.🤖 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 `@packages/core/src/api/revokeToken.ts` around lines 62 - 79, Ensure supplied doubleSubmitToken values are normalized and injected into the CSRF header before validation. Update the public revoke-token wrapper invoking the handler around revokeToken.ts lines 62-79 and the public sign-out wrapper around signOut.ts lines 12-16 to use assertDoubleSubmitToken, or perform equivalent header injection in both handlers; preserve boolean-only behavior when no token is supplied.docs/src/content/docs/(core)/api-reference/server/api/refreshUserInfo.mdx (1)
88-100: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winStale "Delegating CSRF protection to a different layer" heading now shows a manual double-submit example. In both files, the code sample under this heading was rewritten to
skipCSRFCheck: falseplus a manually-setx-csrf-tokenheader — that's the manual double-submit flow, not delegation to another layer (which would useskipCSRFCheck: trueand no CSRF headers at all). A separate "Enable Double-Submit Cookie CSRF protection" section was added right after with nearly the same content, making the old section redundant/misleading.signInCredentials.mdxhandles the equivalent case correctly by renaming/replacing the section outright.
docs/src/content/docs/(core)/api-reference/server/api/refreshUserInfo.mdx#L88-L100: rename or remove this section (and itsskip-csrf.tstitle) so it doesn't contradict the new "Enable Double-Submit Cookie CSRF protection" section, mirroring howsignInCredentials.mdxrestructured it.docs/src/content/docs/(core)/api-reference/server/api/revokeToken.mdx#L95-L107: same fix — rename/remove the stale heading and its now-inaccurate// only if enforced elsewherecomment.🤖 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 `@docs/src/content/docs/`(core)/api-reference/server/api/refreshUserInfo.mdx around lines 88 - 100, The stale CSRF delegation sections now describe manual double-submit protection and are redundant. In docs/src/content/docs/(core)/api-reference/server/api/refreshUserInfo.mdx lines 88-100, rename or remove the section and its skip-csrf.ts sample title; in docs/src/content/docs/(core)/api-reference/server/api/revokeToken.mdx lines 95-107, apply the same correction and remove the inaccurate “only if enforced elsewhere” comment, mirroring the restructuring used in signInCredentials.mdx.
🤖 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 `@packages/core/src/api/signOut.ts`:
- Around line 12-16: Update the signOut flow to call assertDoubleSubmitToken
with doubleSubmitToken before destroying the session, ensuring the validated
token is added to the request headers as X-CSRF-Token. Pass the resulting
headers into ctx.sessionStrategy.destroySession while preserving the existing
skipCSRFCheck behavior.
In `@packages/core/src/api/updateSession.ts`:
- Around line 15-22: Update updateSession to apply doubleSubmitToken
independently of skipCSRFCheck: inject the token value into the headers used by
refreshSession and response validation before CSRF checks, then pass the
refreshed headers returned by refreshSession to createValidation(...).execute().
Preserve skipCSRFCheck as a separate control and ensure valid and invalid
double-submit tokens reach the existing CSRF validation flow.
---
Outside diff comments:
In `@docs/src/content/docs/`(core)/api-reference/server/api/refreshUserInfo.mdx:
- Around line 88-100: The stale CSRF delegation sections now describe manual
double-submit protection and are redundant. In
docs/src/content/docs/(core)/api-reference/server/api/refreshUserInfo.mdx lines
88-100, rename or remove the section and its skip-csrf.ts sample title; in
docs/src/content/docs/(core)/api-reference/server/api/revokeToken.mdx lines
95-107, apply the same correction and remove the inaccurate “only if enforced
elsewhere” comment, mirroring the restructuring used in signInCredentials.mdx.
In `@packages/core/src/api/revokeToken.ts`:
- Around line 62-79: Update the validation setup in revokeToken to propagate the
provided doubleSubmitToken into the headers passed to createValidation, ensuring
the corresponding X-CSRF-Token header is available when double-submit validation
is enabled. Preserve the existing boolean skipCSRFCheck behavior and direct
createAuthAPI invocation path.
- Around line 62-79: Ensure supplied doubleSubmitToken values are normalized and
injected into the CSRF header before validation. Update the public revoke-token
wrapper invoking the handler around revokeToken.ts lines 62-79 and the public
sign-out wrapper around signOut.ts lines 12-16 to use assertDoubleSubmitToken,
or perform equivalent header injection in both handlers; preserve boolean-only
behavior when no token is supplied.
🪄 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 Plus
Run ID: f89c0b4e-f080-4242-ab42-f148ff674af1
📒 Files selected for processing (21)
docs/src/content/docs/(core)/api-reference/server/api/refreshUserInfo.mdxdocs/src/content/docs/(core)/api-reference/server/api/revokeToken.mdxdocs/src/content/docs/(core)/api-reference/server/api/signInCredentials.mdxdocs/src/content/docs/(core)/api-reference/server/api/signOut.mdxdocs/src/content/docs/(core)/api-reference/server/api/signUp.mdxdocs/src/content/docs/(core)/api-reference/server/api/updateSession.mdxpackages/core/src/api/createApi.tspackages/core/src/api/getProviderTokens.tspackages/core/src/api/refreshUserInfo.tspackages/core/src/api/revokeToken.tspackages/core/src/api/signInCredentials.tspackages/core/src/api/signOut.tspackages/core/src/api/signUp.tspackages/core/src/api/updateSession.tspackages/core/src/shared/utils/api.tspackages/core/test/api/stateless/refreshUserInfo.test.tspackages/core/test/api/stateless/revokeToken.test.tspackages/core/test/api/stateless/signInCredentials.test.tspackages/core/test/api/stateless/signOut.test.tspackages/core/test/api/stateless/signUp.test.tspackages/core/test/api/stateless/updateSession.test.ts
💤 Files with no reviewable changes (1)
- packages/core/src/api/getProviderTokens.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/shared/utils/api.ts
- packages/core/src/api/createApi.ts
Comment on lines
+12
to
+16
| doubleSubmitToken = undefined, | ||
| }: FunctionAPIContext<SignOutAPIOptions>): Promise<SignOutAPIReturn> => { | ||
| let responseHeaders = new Headers(headersInit) | ||
| let responseHeaders = new Headers(headersInit ?? requestInit?.headers) | ||
| try { | ||
| responseHeaders = await ctx.sessionStrategy.destroySession(responseHeaders, skipCSRFCheck) | ||
| responseHeaders = await ctx.sessionStrategy.destroySession(responseHeaders, skipCSRFCheck && !!doubleSubmitToken) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Propagate doubleSubmitToken before destroying the session.
The public wrapper delegates directly to signOut without assertDoubleSubmitToken, while destroySession receives only headers and a boolean. Therefore the new documented doubleSubmitToken option never reaches X-CSRF-Token and cannot enable double-submit validation.
🤖 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 `@packages/core/src/api/signOut.ts` around lines 12 - 16, Update the signOut
flow to call assertDoubleSubmitToken with doubleSubmitToken before destroying
the session, ensuring the validated token is added to the request headers as
X-CSRF-Token. Pass the resulting headers into ctx.sessionStrategy.destroySession
while preserving the existing skipCSRFCheck behavior.
Comment on lines
+15
to
22
| doubleSubmitToken = undefined, | ||
| }: FunctionAPIContext<UpdateSessionAPIOptions<DefaultUser>>): Promise<UpdateSessionAPIReturn<DefaultUser>> => { | ||
| try { | ||
| const { session, headers } = await ctx.sessionStrategy.refreshSession( | ||
| new Headers(headersInit), | ||
| sessionInit, | ||
| skipCSRFCheck | ||
| skipCSRFCheck && !!doubleSubmitToken | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg 'packages/core/(src/api/updateSession\.ts|test/api/stateless/updateSession\.test\.ts|src/shared/utils|src/.*csrf|.*session)' || true
echo
echo "updateSession.ts:"
cat -n packages/core/src/api/updateSession.ts | sed -n '1,120p'
echo
echo "updateSession tests relevant lines:"
cat -n packages/core/test/api/stateless/updateSession.test.ts | sed -n '380,490p'
echo
echo "Search doubleSubmitToken usages:"
rg -n "doubleSubmitToken|skipCSRFCheck|X-CSRF-Token|refreshSession" packages/core/src packages/core/test || trueRepository: aura-stack-ts/auth
Length of output: 39326
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "api.ts verification helpers:"
cat -n packages/core/src/shared/utils/api.ts | sed -n '1,220p'
echo
echo "utils.ts refresh csrf:"
cat -n packages/core/src/shared/utils.ts | sed -n '140,195p'
echo
echo "signOut/updateSession stateless API files:"
cat -n packages/core/src/api/signOut.ts | sed -n '1,45p'
cat -n packages/core/src/api/updateSession.ts | sed -n '1,45p'
echo
echo "refreshSession session implementations signatures/bodies:"
cat -n packages/core/src/session/stateless.ts | sed -n '268,293p'
cat -n packages/core/src/session/stateful.ts | sed -n '304,325p'Repository: aura-stack-ts/auth
Length of output: 16439
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Standalone verifier: model the expressions from updateSession.ts around refreshSession calls
# and compare against each documented test outcome/assertion without reading repo files.
python3 - <<'PY'
cases = [
{"name": "missing CSRF header, default skipCSRFCheck=false, doubleSubmitToken undefined", "skipCSRFCheck": False, "doubleSubmitToken": None, "header_csrf": None},
{"name": "valid X-CSRF-Token in cookie, default skipCSRFCheck=false, doubleSubmitToken set to same valid token", "skipCSRFCheck": False, "doubleSubmitToken": "cookie_csrf_token", "header_csrf": None},
{"name": "valid X-CSRF-Token in cookie, invalid doubleSubmitToken, default skipCSRFCheck=false", "skipCSRFCheck": False, "doubleSubmitToken": "invalid-token", "header_csrf": None},
]
for c in cases:
passed_to_refresh = c["skipCSRFCheck"] and bool(c["doubleSubmitToken"])
print(f"{c['name']}: refreshSession third arg = {passed_to_refresh}")
PY
echo
echo "Behavioral probe to show doubleSubmitToken never changes passed argument under current expression:"
python3 - <<'PY'
from itertools import product
for skip, token in product([False, True], [None, "cookie_csrf_token", "invalid-token"]):
print(f"skipCSRFCheck={skip!r}, doubleSubmitToken={token!r} => {skip and bool(token)}")
PYRepository: aura-stack-ts/auth
Length of output: 987
Apply doubleSubmitToken before the CSRF checks and pass it independently.
With skipCSRFCheck defaulting to false, skipCSRFCheck && !!doubleSubmitToken is always false, so refreshSession never receives double-submit handling. updateSession also passes fresh headersInit into refreshSession, and the later createValidation(...).execute() is called with the same original headers instead of the returned refreshed headers, so even the CSRF cookie read there may be stale. The tests for valid vs invalid doubleSubmitToken can’t pass unless the token value is injected into the refresh/response headers and the guard condition no longer requires skipCSRFCheck.
🤖 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 `@packages/core/src/api/updateSession.ts` around lines 15 - 22, Update
updateSession to apply doubleSubmitToken independently of skipCSRFCheck: inject
the token value into the headers used by refreshSession and response validation
before CSRF checks, then pass the refreshed headers returned by refreshSession
to createValidation(...).execute(). Preserve skipCSRFCheck as a separate control
and ensure valid and invalid double-submit tokens reach the existing CSRF
validation flow.
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
1 participant
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.
Description
This pull request introduces the
doubleSubmitTokenoption for the server-side APIs exposed bycreateAuth().api.The new option enables explicit Double-Submit Cookie validation for server-side authentication actions by allowing the CSRF token to be provided directly. This gives applications the ability to enforce the same CSRF protection used by client-side requests when desired.
By default, Double-Submit Cookie validation remains disabled for server-side API calls because they execute in a trusted server environment where attackers cannot inject requests or access the CSRF token. However, standard CSRF validation is still performed to ensure request integrity. Providing a
doubleSubmitTokenenables the additional Double-Submit Cookie verification.Key Changes
doubleSubmitTokenoption to all APIs exposed bycreateAuth().api.skipCSRFCheckoption in favor ofdoubleSubmitToken.Note
Server-side API functions execute in a trusted environment, so Double-Submit Cookie validation is disabled by default. If
doubleSubmitTokenis provided, the additional Double-Submit Cookie validation is performed. Regardless of this option, the standard CSRF validation remains enabled.Warning
This PR deprecates the
skipCSRFCheckoption. Although it enabled or disabled Double-Submit Cookie validation, its name suggested that all CSRF validation was skipped, which was misleading.The new
doubleSubmitTokenoption makes the behavior explicit by requiring the CSRF token when Double-Submit Cookie validation should be enforced.skipCSRFCheckwill remain available for backward compatibility but is deprecated and scheduled for removal in v1.0.0.Usage
Before (
skipCSRFCheck)Now (
doubleSubmitToken)Note
doubleSubmitTokenreplaces the deprecatedskipCSRFCheckoption with a more explicit API. Instead of toggling validation with a boolean, you now provide the CSRF token directly, making it clear that Double-Submit Cookie validation is being enforced.