Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (48)
📝 WalkthroughWalkthroughThis PR reorganizes the TypeScript type system and utility modules by introducing new type-specific modules ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 (9)
packages/core/src/@types/errors.ts (1)
27-32: Suggested refactor forTokenRevocationErroris incorrect—it conflates different OAuth specs.The proposed fix incorrectly references
OAuthAccessTokenErrorResponse(RFC6749 access token errors) for RFC7009 token revocation errors, which have distinct error codes. Additionally,TokenRevocationErroris currently unused in the codebase (only appears in theErrorTypeunion definition).If RFC7009 compliance is needed, create a dedicated
OAuthTokenRevocationErrorResponseZod schema following RFC7009 Section 2.2.1, rather than reusing the access token error spec. For now, since this type is unused, consider whether it should remain as a placeholder or be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/`@types/errors.ts around lines 27 - 32, TokenRevocationError currently aliases OAuthError<"invalid_session_token"> and the suggested refactor in the comment wrongly conflates RFC7009 revocation errors with RFC6749 access token errors; either implement a dedicated RFC7009 schema or remove the unused placeholder: create a new OAuthTokenRevocationErrorResponse Zod schema matching RFC7009 Section 2.2.1 and change TokenRevocationError to reference that schema (or delete TokenRevocationError and remove it from the ErrorType union if you choose to drop the unused placeholder), ensuring you update any references in ErrorType and keep AuthorizationError, AccessTokenError, and OAuthError untouched.packages/core/src/router/context.ts (1)
37-37: Consider fixing the type at the source instead of casting.The cast
as InternalContext<DefaultUser>["sessionStrategy"]is needed becausecreateSessionStrategyreturnsSessionStrategywithout preserving the generic type parameter. While this cast is safe at runtime, it would be cleaner to havecreateSessionStrategyexplicitly returnSessionStrategy<DefaultUser>to avoid the need for this assertion.Suggested change in `packages/core/src/session/index.ts`
export const createSessionStrategy = <DefaultUser extends User = User>({ config, jose, cookies, logger, -}: CreateSessionStrategyOptions<DefaultUser>): SessionStrategy => { +}: CreateSessionStrategyOptions<DefaultUser>): SessionStrategy<DefaultUser> => {This would allow removing the cast here:
ctx.sessionStrategy = createSessionStrategy({ cookies: () => ctx.cookies, jose, config: config?.session, logger: ctx.logger, - }) as InternalContext<DefaultUser>["sessionStrategy"] + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/router/context.ts` at line 37, The cast is compensating for createSessionStrategy losing its generic; update createSessionStrategy's signature to return SessionStrategy<TUser> (preserve the generic) and ensure its implementation and exports use that generic type parameter (so callers can supply DefaultUser). Update any internal usages to pass the appropriate type parameter, then remove the `as InternalContext<DefaultUser>["sessionStrategy"]` assertion where `sessionStrategy` is assigned; reference symbols: createSessionStrategy, SessionStrategy, DefaultUser, InternalContext, sessionStrategy.packages/core/src/security.ts (2)
43-55: Unused variabletokenon line 45.The
tokenvariable created on line 45 is never used whencsrfCookieis provided and successfully verified (lines 46-49 return early). This is dead code in the happy path.♻️ Proposed fix to avoid unnecessary computation
export const createCSRF = async (jose: AuthRuntimeConfig["jose"], csrfCookie?: string) => { try { - const token = createSecretValue(32) if (csrfCookie) { await jose.verifyJWS(csrfCookie) return csrfCookie } + const token = createSecretValue(32) return jose.signJWS({ token }) } catch { const token = createSecretValue(32) return jose.signJWS({ token }) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/security.ts` around lines 43 - 55, The createCSRF function currently always creates a token via createSecretValue(32) even when csrfCookie is provided and verified, making the token unused; update createCSRF so it first checks if csrfCookie exists and successfully calls jose.verifyJWS(csrfCookie) and returns csrfCookie without allocating a token, and only call createSecretValue(32) (and then jose.signJWS({ token })) in the code paths where you actually need to sign a new token (the else branch and the catch block); reference createCSRF, token, jose.verifyJWS, and jose.signJWS when making the change.
73-78: Consider removing redundant length check.The
equals(cookiePayload.token.length, headerPayload.token.length)check on line 73 is redundant. ThetimingSafeEqualimplementation already handles different-length strings securely by iterating over the maximum length and checkingbufferA.length === bufferB.lengthat the end.While not a security issue (integer comparison doesn't leak timing), removing it would simplify the code.
♻️ Proposed simplification
- if (!equals(cookiePayload.token.length, headerPayload.token.length)) { - throw new AuthSecurityError("CSRF_TOKEN_INVALID", "The CSRF tokens do not match.") - } if (!timingSafeEqual(cookiePayload.token, headerPayload.token)) { throw new AuthSecurityError("CSRF_TOKEN_INVALID", "The CSRF tokens do not match.") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/security.ts` around lines 73 - 78, Remove the redundant length comparison using equals(cookiePayload.token.length, headerPayload.token.length) and rely solely on timingSafeEqual(cookiePayload.token, headerPayload.token) to validate CSRF tokens; keep the existing AuthSecurityError("CSRF_TOKEN_INVALID", "The CSRF tokens do not match.") thrown when timingSafeEqual returns false, and ensure only the timingSafeEqual call (and its error throw) remains in the token comparison logic.packages/core/src/createAuth.ts (1)
9-9: Type parameter not propagated toAuthConfig.The
createInternalConfigfunction acceptsAuthConfiginstead ofAuthConfig<DefaultUser>, which loses the custom user type information. This should use the generic parameter for type consistency.♻️ Proposed fix
-const createInternalConfig = <DefaultUser extends User = User>(authConfig?: AuthConfig): RouterConfig => { +const createInternalConfig = <DefaultUser extends User = User>(authConfig?: AuthConfig<DefaultUser>): RouterConfig => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/createAuth.ts` at line 9, The function createInternalConfig uses a generic DefaultUser but accepts AuthConfig without the generic, losing the custom user type; update the parameter type to AuthConfig<DefaultUser> in the createInternalConfig signature so the DefaultUser generic is propagated, and verify any internal uses of authConfig (within createInternalConfig) respect DefaultUser and return RouterConfig with the generic applied.packages/core/src/router/errorHandler.ts (1)
25-31: Minor: Redundant property shorthand.Line 28 uses
message: messagewhich can be simplified tomessage.Proposed fix
return Response.json( { type, - message: message, + message, }, { status: 400 } )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/router/errorHandler.ts` around lines 25 - 31, In the Response.json call inside the error handler (the return Response.json(...) block), simplify the redundant object property by using the shorthand key for message instead of message: message; update the returned object to { type, message } to match the existing shorthand used for type.packages/core/src/@types/config.ts (1)
228-232: Generic type parameter lost in alias.
AuthRuntimeConfigis defined asRouterGlobalContextwithout a type parameter, defaulting toUser. If callers need to preserve a customDefaultUsertype, they'll need to useRouterGlobalContext<CustomUser>directly. Consider whether this alias should also accept a generic.Proposed fix if generic preservation is needed
/** * Internal runtime configuration used within Aura Auth after initialization. * All optional fields from AuthConfig are resolved to their default values. */ -export type AuthRuntimeConfig = RouterGlobalContext +export type AuthRuntimeConfig<DefaultUser extends User = User> = RouterGlobalContext<DefaultUser>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/`@types/config.ts around lines 228 - 232, AuthRuntimeConfig currently aliases RouterGlobalContext without a generic, which forces the DefaultUser type and prevents preserving custom user types; update the alias to accept a generic type parameter (e.g., AuthRuntimeConfig<DefaultUser> or similar) and forward it to RouterGlobalContext so callers can use AuthRuntimeConfig<CustomUser> to retain their user type; adjust any usages/imports of AuthRuntimeConfig to pass the generic where needed and keep the existing default type to preserve backward compatibility.packages/core/src/lib/utils.ts (2)
96-118: ReDoS risk is mitigated but consider additional safeguards.Static analysis flagged the dynamic regex construction (Line 114). The current implementation has good mitigations:
- Length limit (2048 chars)
- Strict input validation via the format regex (Line 101)
- Special character escaping (Line 110)
- Anchored regex (
^...$)However, for defense-in-depth, consider adding a timeout or using a dedicated library for URL matching if this function processes untrusted input at scale.
If you want to eliminate the dynamic regex entirely, consider using URL parsing:
// Alternative: Parse and compare URL components directly const matchOrigin = (pattern: string, origin: string): boolean => { // Parse both and compare protocol, host parts }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/lib/utils.ts` around lines 96 - 118, patternToRegex currently builds a dynamic RegExp from untrusted patterns which, while mitigated, still poses ReDoS risk; replace the dynamic-regex approach by parsing and comparing URL components instead: update patternToRegex (or add a new matchOrigin function) to use URL parsing (new URL) to validate the input pattern and the target origin, then compare protocol, handle host wildcards by splitting and matching labels (supporting only *.prefix at start), and compare port explicitly — this removes the need to construct a runtime regex and keeps the same validations (length, wildcard placement, escaped characters) while avoiding dynamic regex creation; alternatively, if keeping regex, add an explicit safe-guard that rejects any pattern containing nested quantifiers or other catastrophic constructs before building the RegExp.
70-78: Specify minimum Node.js version or addbtoa()fallback if supporting older environments.
btoa()is available in Node.js 16+, which the CI tests against (Node.js 24.x). However, since no minimum Node.js version is specified inpackage.json, users could theoretically install this package on older versions or unsupported runtimes. Either:
- Add
"engines": { "node": ">=16" }to explicitly document the minimum version, or- Use
Buffer.from(credentials, 'utf-8').toString('base64')as a runtime-agnostic fallback if broader compatibility is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/lib/utils.ts` around lines 70 - 78, The createBasicAuthHeader function uses btoa(credentials), which may not exist in older Node runtimes; update the implementation to use a runtime-agnostic base64 encoder (e.g., prefer Buffer.from(credentials, 'utf-8').toString('base64') with a btoa fallback) or alternatively document a minimum Node version by adding an "engines": { "node": ">=16" } entry in package.json; locate createBasicAuthHeader in packages/core/src/lib/utils.ts and replace the btoa usage with the Buffer-based encoding (or add the engines field) to ensure predictable behavior across environments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/`@types/oauth.ts:
- Around line 47-51: The deprecation comment for the top-level responseType is
inconsistent (references authorize.params.response_type) — update the comment to
point to the actual camelCase property authorize.params.responseType; change the
deprecated JSDoc to "use `authorize.params.responseType` instead of
`responseType`" so it matches the existing params definition and avoid
snake_case reference. Ensure the unique symbols mentioned are responseType
(top-level) and authorize.params.responseType (replacement).
- Around line 23-41: The TypeScript interface declares authorize.url,
accessToken.url and userInfo.url as string but the Zod schemas are inconsistent
(authorize uses string().url() while accessToken and userInfo use url() which
accepts URL objects); fix by making the Zod validators consistent with the
TypeScript types: change the accessToken.url and userInfo.url validators to use
string().url() (same form as authorize.url) so all three endpoints validate as
strings, or alternatively update the interface to url: string | URL for
accessToken and userInfo if you prefer accepting URL objects—apply the chosen
change to the validators/types referencing authorize, accessToken, and userInfo
and ensure related param types (AuthorizeParams, ResponseType) remain unchanged.
In `@packages/core/src/router/errorHandler.ts`:
- Around line 49-64: The current security error handling uses a misleading log
key "INVALID_OAUTH_CONFIGURATION"; update the logger call in the
isAuthSecurityError branch to use a generic/auth category (e.g.,
"AUTH_SECURITY_ERROR") instead, leaving the structuredData payload (error: code,
error_description: message) intact and still returning Response.json with the
same { type, code, message } and status 400; adjust the logger invocation where
logger?.log is called so it reflects AUTH_SECURITY_ERROR rather than
INVALID_OAUTH_CONFIGURATION.
---
Nitpick comments:
In `@packages/core/src/`@types/config.ts:
- Around line 228-232: AuthRuntimeConfig currently aliases RouterGlobalContext
without a generic, which forces the DefaultUser type and prevents preserving
custom user types; update the alias to accept a generic type parameter (e.g.,
AuthRuntimeConfig<DefaultUser> or similar) and forward it to RouterGlobalContext
so callers can use AuthRuntimeConfig<CustomUser> to retain their user type;
adjust any usages/imports of AuthRuntimeConfig to pass the generic where needed
and keep the existing default type to preserve backward compatibility.
In `@packages/core/src/`@types/errors.ts:
- Around line 27-32: TokenRevocationError currently aliases
OAuthError<"invalid_session_token"> and the suggested refactor in the comment
wrongly conflates RFC7009 revocation errors with RFC6749 access token errors;
either implement a dedicated RFC7009 schema or remove the unused placeholder:
create a new OAuthTokenRevocationErrorResponse Zod schema matching RFC7009
Section 2.2.1 and change TokenRevocationError to reference that schema (or
delete TokenRevocationError and remove it from the ErrorType union if you choose
to drop the unused placeholder), ensuring you update any references in ErrorType
and keep AuthorizationError, AccessTokenError, and OAuthError untouched.
In `@packages/core/src/createAuth.ts`:
- Line 9: The function createInternalConfig uses a generic DefaultUser but
accepts AuthConfig without the generic, losing the custom user type; update the
parameter type to AuthConfig<DefaultUser> in the createInternalConfig signature
so the DefaultUser generic is propagated, and verify any internal uses of
authConfig (within createInternalConfig) respect DefaultUser and return
RouterConfig with the generic applied.
In `@packages/core/src/lib/utils.ts`:
- Around line 96-118: patternToRegex currently builds a dynamic RegExp from
untrusted patterns which, while mitigated, still poses ReDoS risk; replace the
dynamic-regex approach by parsing and comparing URL components instead: update
patternToRegex (or add a new matchOrigin function) to use URL parsing (new URL)
to validate the input pattern and the target origin, then compare protocol,
handle host wildcards by splitting and matching labels (supporting only *.prefix
at start), and compare port explicitly — this removes the need to construct a
runtime regex and keeps the same validations (length, wildcard placement,
escaped characters) while avoiding dynamic regex creation; alternatively, if
keeping regex, add an explicit safe-guard that rejects any pattern containing
nested quantifiers or other catastrophic constructs before building the RegExp.
- Around line 70-78: The createBasicAuthHeader function uses btoa(credentials),
which may not exist in older Node runtimes; update the implementation to use a
runtime-agnostic base64 encoder (e.g., prefer Buffer.from(credentials,
'utf-8').toString('base64') with a btoa fallback) or alternatively document a
minimum Node version by adding an "engines": { "node": ">=16" } entry in
package.json; locate createBasicAuthHeader in packages/core/src/lib/utils.ts and
replace the btoa usage with the Buffer-based encoding (or add the engines field)
to ensure predictable behavior across environments.
In `@packages/core/src/router/context.ts`:
- Line 37: The cast is compensating for createSessionStrategy losing its
generic; update createSessionStrategy's signature to return
SessionStrategy<TUser> (preserve the generic) and ensure its implementation and
exports use that generic type parameter (so callers can supply DefaultUser).
Update any internal usages to pass the appropriate type parameter, then remove
the `as InternalContext<DefaultUser>["sessionStrategy"]` assertion where
`sessionStrategy` is assigned; reference symbols: createSessionStrategy,
SessionStrategy, DefaultUser, InternalContext, sessionStrategy.
In `@packages/core/src/router/errorHandler.ts`:
- Around line 25-31: In the Response.json call inside the error handler (the
return Response.json(...) block), simplify the redundant object property by
using the shorthand key for message instead of message: message; update the
returned object to { type, message } to match the existing shorthand used for
type.
In `@packages/core/src/security.ts`:
- Around line 43-55: The createCSRF function currently always creates a token
via createSecretValue(32) even when csrfCookie is provided and verified, making
the token unused; update createCSRF so it first checks if csrfCookie exists and
successfully calls jose.verifyJWS(csrfCookie) and returns csrfCookie without
allocating a token, and only call createSecretValue(32) (and then jose.signJWS({
token })) in the code paths where you actually need to sign a new token (the
else branch and the catch block); reference createCSRF, token, jose.verifyJWS,
and jose.signJWS when making the change.
- Around line 73-78: Remove the redundant length comparison using
equals(cookiePayload.token.length, headerPayload.token.length) and rely solely
on timingSafeEqual(cookiePayload.token, headerPayload.token) to validate CSRF
tokens; keep the existing AuthSecurityError("CSRF_TOKEN_INVALID", "The CSRF
tokens do not match.") thrown when timingSafeEqual returns false, and ensure
only the timingSafeEqual call (and its error throw) remains in the token
comparison logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b75de73-2b50-4c56-b7b4-252617d53a26
📒 Files selected for processing (52)
packages/core/src/@types/config.tspackages/core/src/@types/errors.tspackages/core/src/@types/index.tspackages/core/src/@types/oauth.tspackages/core/src/@types/session.tspackages/core/src/actions/callback/access-token.tspackages/core/src/actions/callback/callback.tspackages/core/src/actions/callback/userinfo.tspackages/core/src/actions/csrfToken/csrfToken.tspackages/core/src/actions/session/session.tspackages/core/src/actions/signIn/authorization-url.tspackages/core/src/actions/signIn/authorization.tspackages/core/src/actions/signOut/signOut.tspackages/core/src/api/createApi.tspackages/core/src/api/getSession.tspackages/core/src/api/index.tspackages/core/src/api/signIn.tspackages/core/src/client/client.tspackages/core/src/cookie.tspackages/core/src/createAuth.tspackages/core/src/index.tspackages/core/src/jose.tspackages/core/src/lib/assert.tspackages/core/src/lib/errors.tspackages/core/src/lib/fetch-async.tspackages/core/src/lib/headers.tspackages/core/src/lib/logger.tspackages/core/src/lib/utils.tspackages/core/src/oauth/gitlab.tspackages/core/src/oauth/index.tspackages/core/src/oauth/notion.tspackages/core/src/router/context.tspackages/core/src/router/errorHandler.tspackages/core/src/schemas.tspackages/core/src/security.tspackages/core/src/session/index.tspackages/core/src/session/manager/cookie.tspackages/core/src/session/manager/jose.tspackages/core/src/session/strategies/stateless.tspackages/core/src/utils.tspackages/core/test/actions/callback/access-token.test.tspackages/core/test/actions/callback/callback.test.tspackages/core/test/actions/callback/userinfo.test.tspackages/core/test/actions/session/session.test.tspackages/core/test/actions/signOut/signOut.test.tspackages/core/test/assert.test.tspackages/core/test/context.test.tspackages/core/test/env.test.tspackages/core/test/jose.test.tspackages/core/test/request.test.tspackages/core/test/secure.test.tspackages/core/test/utils.test.ts
💤 Files with no reviewable changes (2)
- packages/core/test/utils.test.ts
- packages/core/src/utils.ts
halvaradop
force-pushed
the
chore/clean-up
branch
2 times, most recently
from
4f656ef to
e7186f5
Compare
halvaradop
force-pushed
the
chore/clean-up
branch
from
e7186f5 to
e9f6443
Compare
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 performs a cleanup and restructuring of the core package to improve organization, separation of concerns, and maintainability. The changes introduce a clearer modular structure that prepares the codebase for future expansions, including upcoming packages such as database adapters and framework-specific integrations.
The refactor focuses on reorganizing files and folders, isolating responsibilities, and improving overall code clarity without altering existing functionality.
Key Changes
sharedfolder for reusable utilities and common logicrouterfolder to isolate routing-related logiccreateJoseInstancefor external usagegenerateSecurecodeSummary by CodeRabbit
Release Notes
New Features
createJoseInstanceas a new public API for JWT token management.createErrorHandlerexport.AuthConfig,JWTConfig,SessionStrategy, and session configuration types.signIn,signOut,getSession, andcreateAuthAPIfunctions.Refactor