feat(core): support signIn in stateful strategy - #240
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughOAuth sign-in and callback processing now runs through session strategies, with shared context and callback validation. Stateful and stateless strategies expose OAuth methods, OAuth transaction fields use ChangesOAuth strategy flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SignInAPI
participant SessionStrategy
participant OAuthProvider
participant Storage
Client->>SignInAPI: signIn(oauth, request, redirectTo)
SignInAPI->>SessionStrategy: signIn(oauth, request, redirectTo)
SessionStrategy->>Storage: createOAuthTransaction
SessionStrategy-->>Client: authorization URL and headers
Client->>SignInAPI: callback with code and state
SignInAPI->>SessionStrategy: oauthCallback(oauth, request, code, state)
SessionStrategy->>Storage: retrieve and consume transaction
SessionStrategy->>OAuthProvider: exchange code and fetch user info
SessionStrategy->>Storage: upsert account and create session
SessionStrategy-->>Client: redirect response
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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: 10
🧹 Nitpick comments (5)
packages/core/src/session/stateful.ts (1)
1202-1204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the
as anyand use a code that describes the failure.
DATABASE_TOKEN_HASH_NOT_FOUNDis a valid catalog code (used at Line 90), so the cast is unnecessary — and it is semantically wrong here: the failure is a missing PKCE verifier on the transaction, not a session token hash lookup.🤖 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/session/stateful.ts` around lines 1202 - 1204, Update the error construction in the transaction verifier check within the stateful session flow: remove the unnecessary `as any` cast and replace `DATABASE_TOKEN_HASH_NOT_FOUND` with the existing catalog code that accurately represents a missing PKCE code verifier. Preserve the current AuraAuthError behavior.packages/core/test/api/stateful/signIn.test.ts (2)
62-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
vi.unstubAllGlobals()doesn't undovi.stubEnv.These three tests stub env but not globals; the intended cleanup is
vi.unstubAllEnvs()(or nothing, givenunstubEnvs: true). Also, placing cleanup after assertions means it is skipped when an assertion throws — preferafterEach.Also applies to: 130-130, 166-166
🤖 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/stateful/signIn.test.ts` at line 62, Replace the per-test vi.unstubAllGlobals() cleanup in the affected tests with environment cleanup via vi.unstubAllEnvs(), or remove it if unstubEnvs is already enabled. Move the cleanup into an afterEach hook so it runs even when assertions fail, covering all three tests.
27-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated transaction fixture.
The same ~15-line
createOAuthTransactionmock payload is duplicated five times, andsignInignores the resolved value entirely — a shared helper (or plainvi.fn()) would cut most of this file.🤖 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/stateful/signIn.test.ts` around lines 27 - 41, Extract the repeated OAuth transaction payload used by createOAuthTransactionMock into a shared fixture or helper, and reuse it across all five tests in signIn.test.ts. Since signIn does not consume the resolved transaction value, simplify createOAuthTransactionMock to a plain vi.fn() where the resolved payload is unnecessary, while preserving any tests that explicitly require the transaction data.packages/core/src/session/stateless.ts (2)
480-512: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueCookie extraction throws before the clearing headers exist.
getCookiethrowsCOOKIE_NOT_FOUND/COOKIE_INVALID_VALUE(seepackages/core/src/cookie.tsLines 83-93), so a callback replayed after cookies expired propagates an error while the remaining protocol cookies stay set. BuildclearCookieHeadersfirst and attach it to the failure path for consistency with the state-mismatch branch.🤖 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/session/stateless.ts` around lines 480 - 512, Move construction of clearCookieHeaders in oauthCallback before any getCookie calls, then catch cookie extraction failures and return a protocol error response with clearCookieHeaders attached. Preserve the existing state-mismatch response behavior and ensure replayed callbacks clear all OAuth protocol cookies.
434-478: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
signInis byte-identical to the stateful implementation.Per the stack context,
createStatefulStrategy.signIncontains the same provider resolution, authorization-URL branch, and protocol-cookie construction. Extract it into a shared helper (e.g.shared/utils/authorization.ts) so the two strategies cannot drift on security-relevant cookie handling.🤖 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/session/stateless.ts` around lines 434 - 478, Extract the duplicated sign-in flow from the stateless implementation around provider resolution, createOIDCAuthorizationURL/createAuthorizationURL branching, and HeadersBuilder cookie construction into a shared authorization helper. Update both createStatefulStrategy.signIn and the corresponding stateless signIn to call that helper while preserving provider-specific state, redirect, code-verifier, and optional nonce cookies and existing logging/results.
🤖 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/model.md`:
- Around line 7-8: Update the OAuthAccounts and CredentialAccounts relationship
declarations in the model diagram to use zero-or-one cardinality on each side,
so an Accounts row may have either subtype or neither. Do not encode mutual
exclusivity in these relationships; document or enforce it separately if
supported by the model.
In `@packages/core/src/api/signIn.ts`:
- Around line 42-52: Update the toResponse callback in the sign-in return object
to use the existing success value from ctx.sessionStrategy.signIn instead of
hardcoding true, while preserving the current redirect, signInURL, headers, and
response status behavior.
In `@packages/core/src/session/stateful.ts`:
- Around line 1062-1073: Guard the provider lookup in signIn and oauthCallback
before calling isOIDCProvider or resolveOpenIDProvider; when oauth[oauthId] is
missing, throw AuraAuthError with code "UNSUPPORTED_OAUTH_CONFIGURATION". Apply
this at packages/core/src/session/stateful.ts lines 1062-1073 and 1136-1137, and
remove the unnecessary provider! assertions after the guards.
- Around line 1319-1328: The OAuth callback around sessionPayload and
createSession must preserve the actual authentication method and transaction
device metadata instead of relying on createSession’s credentials defaults.
Update createSession to accept and persist authenticatedWith and deviceId, then
pass the OAuth method and transaction deviceId from this flow, or write the
session record directly with those values.
- Around line 1254-1279: Update the user lookup and creation flow around
getUserByEmail and createUser so provider email-based account linking occurs
only when userInfo.email is non-empty and explicitly verified via the provider’s
email_verified claim. Avoid querying with the empty-string fallback, and set
emailVerifiedAt only when that verification is asserted; otherwise preserve an
unverified state.
In `@packages/core/src/session/stateless.ts`:
- Around line 564-571: Update the OAuth redirect response in the stateless
session flow to return a 302 response with the existing headers but no body; do
not pass the provider registry variable oauth to Response.json or otherwise
serialize provider configuration containing client secrets.
- Around line 533-550: Update the redirect validation block in the stateless
session flow to compute and validate origins only for non-relative
cookieRedirectTo values, avoiding the unconditional getOriginURL call. Ensure
malformed absolute redirect values are caught and treated as invalid so they log
POTENTIAL_OPEN_REDIRECT_ATTACK_DETECTED and throw AuraAuthError instead of
leaking a TypeError, while keeping origins and requestOrigin in scope for the
existing logger payload.
- Around line 423-433: Guard the provider lookup in both signIn and
oauthCallback before calling isOIDCProvider or accessing provider fields. When
oauth[oauthId] is missing, return the existing UNSUPPORTED_OAUTH_CONFIGURATION
response/error pattern used by getProviderTokens instead of dereferencing the
undefined provider; preserve the existing flow for configured providers.
In `@packages/core/test/api/stateful/signIn.test.ts`:
- Around line 65-93: Update the test’s authInstance configuration in “signIn
with baseURL in context stores OAuth transaction in database” to explicitly
provide the expected base URL override. Keep the redirect_uri assertion
unchanged and ensure the test no longer relies on environment state from another
test.
In `@packages/core/vitest.config.ts`:
- Line 20: Remove the signIn.test.ts exclusion from the Vitest configuration in
the core project and preserve Vitest’s default exclusions by avoiding a
root-level replacement of the default exclude list. If the sign-in suite is
temporarily failing, gate it directly with test.skip or describe.skip and add a
TODO to track re-enabling it.
---
Nitpick comments:
In `@packages/core/src/session/stateful.ts`:
- Around line 1202-1204: Update the error construction in the transaction
verifier check within the stateful session flow: remove the unnecessary `as any`
cast and replace `DATABASE_TOKEN_HASH_NOT_FOUND` with the existing catalog code
that accurately represents a missing PKCE code verifier. Preserve the current
AuraAuthError behavior.
In `@packages/core/src/session/stateless.ts`:
- Around line 480-512: Move construction of clearCookieHeaders in oauthCallback
before any getCookie calls, then catch cookie extraction failures and return a
protocol error response with clearCookieHeaders attached. Preserve the existing
state-mismatch response behavior and ensure replayed callbacks clear all OAuth
protocol cookies.
- Around line 434-478: Extract the duplicated sign-in flow from the stateless
implementation around provider resolution,
createOIDCAuthorizationURL/createAuthorizationURL branching, and HeadersBuilder
cookie construction into a shared authorization helper. Update both
createStatefulStrategy.signIn and the corresponding stateless signIn to call
that helper while preserving provider-specific state, redirect, code-verifier,
and optional nonce cookies and existing logging/results.
In `@packages/core/test/api/stateful/signIn.test.ts`:
- Line 62: Replace the per-test vi.unstubAllGlobals() cleanup in the affected
tests with environment cleanup via vi.unstubAllEnvs(), or remove it if
unstubEnvs is already enabled. Move the cleanup into an afterEach hook so it
runs even when assertions fail, covering all three tests.
- Around line 27-41: Extract the repeated OAuth transaction payload used by
createOAuthTransactionMock into a shared fixture or helper, and reuse it across
all five tests in signIn.test.ts. Since signIn does not consume the resolved
transaction value, simplify createOAuthTransactionMock to a plain vi.fn() where
the resolved payload is unnecessary, while preserving any tests that explicitly
require the transaction data.
🪄 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: 31e628a1-e761-441b-950b-3ae4c9311284
📒 Files selected for processing (13)
packages/core/model.mdpackages/core/src/@types/session.tspackages/core/src/actions/callback/callback.tspackages/core/src/api/signIn.tspackages/core/src/cookie.tspackages/core/src/router/context.tspackages/core/src/schemas.tspackages/core/src/session/stateful.tspackages/core/src/session/stateless.tspackages/core/src/session/strategy.tspackages/core/src/shared/assert.tspackages/core/test/api/stateful/signIn.test.tspackages/core/vitest.config.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 (2)
packages/core/src/session/stateful.ts (2)
1254-1281: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAccount linking still trusts an unverified provider email. The empty-string fallback is gone, but
getUserByEmail(userInfo.email)at Line 1259 links the OAuth identity to any pre-existing local user with that email without checking the provider assertedemail_verified, and Line 1274 marks newly created users verified unconditionally. A provider allowing arbitrary unverified emails enables takeover of local accounts.🤖 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/session/stateful.ts` around lines 1254 - 1281, Update the account-linking flow around getUserInfo and getUserByEmail to require the provider’s asserted email_verified value before looking up or creating a local user; reject unverified or absent verification with the existing authentication error path. Only set emailVerifiedAt for newly created users when the provider email is verified, and do not link unverified identities to existing accounts.
1318-1328: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReusing
createSessionfor the OAuth flow mislabels the session and rewrites the user.createSession(Lines 289–299) hardcodesauthenticatedWith: "credentials"anddeviceId: null, so OAuth sessions are recorded as credential logins and the transaction'sdeviceId/device metadata is dropped. It also re-runsgetUserById→updateUser/createUser(Lines 247–287) on a user this method just upserted at Lines 1267/1269, producing a second write per callback that can clobber the fields set above (and nestsattributes: {}insideattributes). Consider threadingauthenticatedWith/deviceIdintocreateSessionplus a flag to skip the user upsert, or writing the session record directly 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/session/stateful.ts` around lines 1318 - 1328, Update the OAuth flow in the surrounding method so it creates the session with the OAuth authentication type and preserves the transaction’s deviceId and metadata, without re-running the user upsert already performed earlier in the flow. Prefer extending createSession with these values and a skip-upsert option, or write the session record directly, while preserving the existing session and CSRF behavior.
🧹 Nitpick comments (2)
packages/core/test/actions/callback/stateful.test.ts (2)
203-337: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting the session and CSRF cookies on the redirect response. Both workflows verify status/
Locationand adapter calls but never check thatSet-Cookiecarries the session token and CSRF token written atpackages/core/src/session/stateful.tsLines 1337-1339 — the part that actually authenticates the user after callback.Also applies to: 371-446
🤖 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/actions/callback/stateful.test.ts` around lines 203 - 337, Extend the OAuth callback redirect assertions in the stateful GET test to verify the response Set-Cookie headers include both the session cookie and CSRF cookie written by the callback flow. Apply the same assertions to the additional workflow noted by the review, while preserving the existing status, Location, and adapter-call checks.
277-277: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a resolved mock assertion for the async return. This mock returns a promise instance;
toHaveReturnedWith(Promise.resolve(transaction))compares promise instances rather than the mocked function’s resolved value, so it is safer to usetoHaveResolvedWith(transaction)here. Or drop this line if the resolved value is already constrained by the call pinning.🤖 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/actions/callback/stateful.test.ts` at line 277, Update the assertion for getOAuthTransactionByStateMock to verify its resolved value with toHaveResolvedWith(transaction) instead of comparing a newly created Promise instance; alternatively remove the assertion if the existing call pinning already validates the resolved value.
🤖 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/session/stateful.ts`:
- Around line 1138-1140: Update oauthCallback to validate oauthConfig before
passing it to isOIDCProvider, matching the guard used by signIn. For an
unconfigured oauthId, return or throw the established
UNSUPPORTED_OAUTH_CONFIGURATION error instead of allowing a TypeError; preserve
the existing callback flow for configured providers.
In `@packages/core/test/api/stateful/signIn.test.ts`:
- Around line 122-129: Update the non-redirect signIn path in stateful signIn()
to create and persist the OAuth transaction’s state and codeVerifier, then
return the generated redirectURI itself as signInURL instead of the generic
/signIn URL. Preserve the existing response shape and ensure the callback can
resolve the persisted transaction state.
---
Outside diff comments:
In `@packages/core/src/session/stateful.ts`:
- Around line 1254-1281: Update the account-linking flow around getUserInfo and
getUserByEmail to require the provider’s asserted email_verified value before
looking up or creating a local user; reject unverified or absent verification
with the existing authentication error path. Only set emailVerifiedAt for newly
created users when the provider email is verified, and do not link unverified
identities to existing accounts.
- Around line 1318-1328: Update the OAuth flow in the surrounding method so it
creates the session with the OAuth authentication type and preserves the
transaction’s deviceId and metadata, without re-running the user upsert already
performed earlier in the flow. Prefer extending createSession with these values
and a skip-upsert option, or write the session record directly, while preserving
the existing session and CSRF behavior.
---
Nitpick comments:
In `@packages/core/test/actions/callback/stateful.test.ts`:
- Around line 203-337: Extend the OAuth callback redirect assertions in the
stateful GET test to verify the response Set-Cookie headers include both the
session cookie and CSRF cookie written by the callback flow. Apply the same
assertions to the additional workflow noted by the review, while preserving the
existing status, Location, and adapter-call checks.
- Line 277: Update the assertion for getOAuthTransactionByStateMock to verify
its resolved value with toHaveResolvedWith(transaction) instead of comparing a
newly created Promise instance; alternatively remove the assertion if the
existing call pinning already validates the resolved value.
🪄 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: 1c1b7f1a-d73e-4893-b2f2-62e354402e0c
📒 Files selected for processing (13)
packages/core/model.mdpackages/core/src/@types/entities.tspackages/core/src/api/signIn.tspackages/core/src/session/stateful.tspackages/core/src/session/stateless.tspackages/core/test/actions/callback/stateful.test.tspackages/core/test/api/stateful/signIn.test.tspackages/elysia/prisma/schema.prismapackages/prisma/prisma/migrations/20260729162431_schema/migration.sqlpackages/prisma/prisma/schema.prismapackages/prisma/src/lib/mappers.tspackages/prisma/src/model.mdpackages/shared/src/adapter-suite.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/api/signIn.ts
- packages/core/model.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/elysia/test/stateful/index.test.ts (1)
89-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded version string in assertion is fragile.
Asserting a literal
"Aura Auth/0.8.1"User-Agent ties this test to the exact package version; every version bump will require updating this test. Consider importing/deriving the expected user-agent string from a shared constant or the package version.🤖 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/elysia/test/stateful/index.test.ts` around lines 89 - 96, Update the mockFetch assertion in the stateful request test to derive the expected User-Agent from the shared package version or user-agent constant instead of hardcoding "Aura Auth/0.8.1". Preserve the existing request method, headers, authorization, and signal expectations.
🤖 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/elysia/test/stateful/index.test.ts`:
- Around line 20-22: Restore the global fetch stub created in the test setup
around mockFetch after each test, using the file’s existing lifecycle hooks or
adding an afterEach cleanup with vi.unstubAllGlobals(). Ensure cleanup runs for
every test so the real fetch is restored and stubs do not leak.
---
Nitpick comments:
In `@packages/elysia/test/stateful/index.test.ts`:
- Around line 89-96: Update the mockFetch assertion in the stateful request test
to derive the expected User-Agent from the shared package version or user-agent
constant instead of hardcoding "Aura Auth/0.8.1". Preserve the existing request
method, headers, authorization, and signal expectations.
🪄 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: f647798d-21e4-4603-a18c-041b2f6225a2
📒 Files selected for processing (4)
packages/core/src/session/stateful.tspackages/elysia/package.jsonpackages/elysia/test/stateful/app.tspackages/elysia/test/stateful/index.test.ts
💤 Files with no reviewable changes (1)
- packages/core/src/session/stateful.ts
This was referenced Jul 31, 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
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 adds Stateful session support for the OAuth and OpenID Connect (OIDC) sign-in flow through both the
api.signIn()API and theGET /signIn/:providerendpoint.With this change, applications using the Stateful session strategy can initiate OAuth/OIDC authentication flows in the same way as the existing Stateless strategy. The implementation includes support for both the sign-in and callback phases, ensuring that temporary authentication state is securely managed throughout the authorization process.
To support the Stateful strategy, OAuth state values and other temporary authentication data are securely handled during the authorization flow, providing protection against CSRF and replay attacks.
Key Changes
api.signIn().GET /signIn/:providerendpoint.Note
This PR is part of the ongoing effort to implement the Stateful session strategy in
@aura-stack/auth. The implementation has been split into a series of smaller pull requests to keep reviews focused and manageable.@coderabbitai ignore