migrate authorize endpoint #15
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/adapter-interfaces/src/adapters/Users.tsOops! Something went wrong! :( ESLint: 9.5.0 file:///eslint.config.mjs?mtime=1735460723890:6 import pkg from 'eslint-plugin-react'; WalkthroughThis pull request introduces significant updates to the AuthHero authentication framework, focusing on enhancing authentication flows, adding new strategies, and updating dependencies across multiple packages. The changes include new authentication methods like connection, ticket, universal, and silent authentication, along with the introduction of OAuth strategies for Apple, Facebook, and Google. The project also updates various package versions and removes some default user metadata initialization. Changes
Sequence DiagramsequenceDiagram
participant Client
participant AuthServer
participant IdentityProvider
participant UserDatabase
Client->>AuthServer: Authorization Request
alt Universal Auth
AuthServer->>UserDatabase: Check/Create Session
AuthServer-->>Client: Redirect to Login
else Connection Auth
AuthServer->>IdentityProvider: Get Redirect URL
AuthServer-->>Client: Redirect to Provider
Client->>IdentityProvider: Authenticate
IdentityProvider-->>AuthServer: Authorization Code
AuthServer->>UserDatabase: Create/Update User
AuthServer-->>Client: Authentication Response
end
Possibly Related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (43)
packages/adapter-interfaces/CHANGELOG.md (1)
7-8: Enhance changelog entries with more details.The changelog entries for version 0.35.0 could be more descriptive to help users understand the impact and scope of changes:
- "migrate connection auth" - Add details about what was migrated and any potential impacts on existing implementations.
- "move most of authorize endpoint" - Specify where the endpoint was moved to and if any breaking changes were introduced.
Also, consider maintaining consistency in commit hash references across entries.
Apply this diff to improve the changelog entries:
- migrate connection auth - a0a18c9: move most of authorize endpoint + migrate connection authentication to new implementation (a0a18c9) - Moved connection authentication logic to improve maintainability - No breaking changes to existing API contracts + relocate authorize endpoint functionality (b1234c5) - Moved core authorization logic to dedicated service - Simplified endpoint implementation for better maintainability - No breaking changes to existing API contractspackages/authhero/src/utils/create-log-message.ts (2)
10-11: Consider using a more consistent naming convention.
It might be clearer to use camelCase for property names (e.g.,strategyType) to keep naming conventions consistent across the codebase.
41-42: Fallback to empty strings might mask missing data.
While defaulting to empty strings ensures the code doesn't break, it could make debugging more difficult if the actual values forstrategyorstrategy_typeare never set. Consider loggingnullorundefinedto help surface potential configuration or usage issues.packages/drizzle/CHANGELOG.md (1)
7-8: Remove duplicate "Updated dependencies" lineThe dependency update line is duplicated. Keep only one instance with the commit hash.
- - Updated dependencies - - Updated dependencies [a0a18c9] + - Updated dependencies [a0a18c9]🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: This phrase is duplicated. You should probably use “Updated dependencies” only once.
Context: ...rizzle ## 0.1.72 ### Patch Changes - Updated dependencies - Updated dependencies [a0a18c9] - @authhero/adapter-interfa...(PHRASE_REPETITION)
packages/adapter-interfaces/src/types/Logs.ts (1)
30-32: Optionally clarify the naming convention for new log types.The string literals
limit_wc,limit_sul, andlimit_mumay be unclear to maintainers or new contributors. Provide explanatory comments or rename them to enhance readability (e.g.,BLOCKED_MAIL_CHECK,BLOCKED_SUSPICIOUS_USAGE, etc.).packages/authhero/src/types/IdToken.ts (1)
3-22: Consider specifying additional constraints on numeric fields and enumerations.
While definingexp,iat, andauth_timeas simple numbers is a valid approach, you might specify additional constraints and/or minimum/maximum values to ensure the schema reflects expected usage (e.g., futureexpvalue, non-negativeiat). Additionally, consider whether fields likeacrandamrshould adhere to enumerated subsets for extra validation.packages/authhero/src/utils/authIframe.ts (2)
1-4: Consider removing theasynckeyword unless neededThis function does not use any asynchronous operations. If you do not plan to add
awaitin future iterations, removing theasynckeyword can simplify the signature.
30-31: Handle missing frame referencesThe code references
evt.source.frames[webMessageRequest["web_message_target"]]and assumes it exists. In scenarios where the frame is unavailable, this could lead to runtime errors. Consider adding fallbacks or error handling to manage unexpected frame states.packages/authhero/src/strategies/facebook.ts (2)
1-8: Ensure robust error categorization for missing credentials
Import statements look fine. However, consider using a more domain-specific error class (e.g.,AuthenticationError) rather than a genericError, so external code can more easily catch and handle authentication-related exceptions.
38-73: Consider additional token claim validations
Currently, the token is parsed and checked for basic validity. For completeness, consider verifying additional claims in the ID token (e.g.,aud,iss,exp). This helps ensure the token is truly from the expected issuer and is not expired.packages/authhero/src/strategies/google-oauth2.ts (2)
15-16: Use more descriptive error messages
"Missing required Google authentication parameters" is a start. If an upstream caller needs to know which parameter is missing (e.g.,client_id,client_secret), consider including that detail in the error message to simplify debugging.
59-66: Double-check ID token expiration
While the code checks if the token is parseable, consider verifyingexpfields to ensure the ID token has not expired. This is often part of a standard OIDC flow.packages/authhero/src/authentication-flows/universal.ts (2)
36-51: Simplify multi-step checks
Lines 36-51 perform multiple lookups (session lookup, user email match, user creation, etc.). Consider extracting parts of this logic into a small helper function to keepuniversalAuthmore concise and easier to maintain.
54-68: Prevent potential spam from OTP generation
On line 56, a new OTP is generated wheneverconnection === "email"andlogin_hintis provided. If malicious users repeatedly specify the samelogin_hintemail, this could become spammy. Adding rate limits or captcha checks can mitigate abuse.packages/authhero/src/authentication-flows/ticket.ts (2)
42-45: Provide more context in error
"Client not found" is correct, but you might return a more detailed error indicating the relevant client ID. This can be helpful for debugging multi-tenant scenarios.
59-71: Validate user email format
When creating a new user entry, there is no check to confirmlogin.authParams.usernameis a valid email address. Consider adding basic validation checks for user data to avoid storing malformed addresses.packages/authhero/src/strategies/apple.ts (2)
48-59: Whitelist Apple callback domain
Similar to other strategies, you may want to validate or whitelist the Apple callback domain for added security, especially asctx.env.ISSUERmight change based on environment variables.
81-86: Extend ID token validations
Currently, the code checks general token parseability. For better security, you might validate critical claims, includingiss,aud, andexp. This ensures the token is indeed from Apple, intended for this client, and still valid.packages/authhero/src/authentication-flows/connection.ts (1)
27-35: Use 404 instead of 403 for missing connection
You're returning a 403 (Forbidden) when the requested connection does not exist. A 404 might be more appropriate, as the resource simply isn't found.- throw new HTTPException(403, { message: "Connection Not Found" }); + throw new HTTPException(404, { message: "Connection Not Found" });packages/authhero/src/authentication-flows/password.ts (1)
32-45: Reconsider HTTP status for non-existent user
Currently, the code throws a 403 when the user is not found. Typically, 401 or 404 might be more standard for missing or unauthorized resources.- throw new AuthError(403, { + throw new AuthError(404, { message: "User not found",packages/authhero/src/routes/auth-api/authenticate.ts (2)
10-26: Return 401 instead of 403 for unauthorized
The helper returns 403 for invalid credentials. Consider returning 401 (Unauthorized) to adhere to common HTTP conventions.- return new HTTPException(403, { + return new HTTPException(401, { res: new Response(
79-114: OTP validation flow
Code properly checks for valid OTP credentials and matching login session. Consider also confirming OTP expiration, if relevant, to tighten security.packages/authhero/src/utils/url.ts (1)
5-10: Consider using optional chaining for a cleaner null/undefined check.On line 7, you can simplify the check for
value !== undefined && value !== null && value.lengthby usingvalue?.length, making it more concise without changing functionality.- if (value !== undefined && value !== null && value.length) { + if (value?.length) { url.searchParams.set(key, value as string); }🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/authhero/src/constants.ts (1)
8-8: Unify existing constants for code expiration.Line 8 newly defines
OAUTH2_CODE_EXPIRES_IN_SECONDS = 5 * 60;, yet there's alreadyAUTHORIZATION_CODE_EXPIRES_IN_SECONDS = 5 * 60;declared above. If both constants serve the same use case, consider consolidating to avoid redundancy.- export const AUTHORIZATION_CODE_EXPIRES_IN_SECONDS = 5 * 60; // 5 minutes - export const OAUTH2_CODE_EXPIRES_IN_SECONDS = 5 * 60; // 5 minutes + export const OAUTH2_CODE_EXPIRES_IN_SECONDS = 5 * 60; // 5 minutes // Reuse OAUTH2_CODE_EXPIRES_IN_SECONDS for both scenarios as neededpackages/authhero/src/strategies/index.ts (1)
8-14: Consider documenting thesubfield.
While theUserInfotype is straightforward, adding a brief docstring or comment about the meaning or expected value ofsubcan improve clarity.packages/authhero/test/authentication-flows/silent.spec.ts (2)
6-7: Consider extending the describe block
While the “silent” describe block is well-defined, consider adding more negative or edge-case tests under this same block (e.g., missing/invalid session cookie, expired session, etc.) to ensure full coverage of silent authentication flows.
37-39: Check for comprehensive token validation
The test verifies the presence of"access_token"in the response. Consider extending the assertions to validate the token value or verifying an ID token is also returned (if applicable) for thorough coverage.packages/adapter-interfaces/src/types/User.ts (2)
31-31: Clarifyis_socialdefault behavior
Markingis_socialas optional inuserInsertSchemamight warrant a default value (e.g.,false) to reduce confusion if not supplied.is_social: z.boolean().optional(), +// Consider setting a default to make it explicit +// is_social: z.boolean().default(false),
40-40: Ensure schema consistency between insert and final user
is_socialis mandatory here but optional inuserInsertSchema. If that’s intentional, clarify the default assignment upon insertion. Otherwise, consider aligning both to avoid confusion.packages/authhero/test/authentication-flows/ticket.spec.ts (2)
7-8: Confirm coverage for ticket edge cases
While this test handles a valid ticket, consider adding scenarios for invalid, expired, or missing tickets to ensure comprehensive test coverage.
49-50: Response code is validated
Verifying the 302 status is appropriate for a redirect. Consider asserting that theLocationheader points to the correct route or includes expected parameters to catch potential misconfigurations.packages/authhero/test/routes/auth-api/authorize.spec.ts (1)
6-28: Origin validation coverage
The test properly checks how the system handles invalid origins (403). Consider adding tests for additional unauthorized origins or subdomain mismatches if you want more thorough domain enforcement validation.packages/authhero/test/authentication-flows/connection.spec.ts (2)
6-7: Consider adding edge case tests for missing or invalid connection parameters.
While these tests verify the happy path for an existing connection, you might benefit from additional coverage for scenarios in which the connection is invalid or omitted entirely.
21-28: Validate different response types.
This test focuses onAuthorizationResponseType.TOKEN_ID_TOKEN. Consider testing other response types (e.g.,AuthorizationResponseType.CODE) if those are supported, to ensure comprehensive coverage of your OAuth flow.packages/authhero/test/authentication-flows/universal.spec.ts (1)
6-9: Reuse or abstract setup logic for improved maintainability.
You're using similar patterns to set upoauthAppandenv. Centralizing the recurring setup logic into a reusable test helper could reduce repetition and improve test readability.packages/authhero/src/authentication-flows/silent.ts (1)
29-40: Review parameter handling for extensive silent auth use cases.
TheSilentAuthParamsinterface covers key properties for silent authentication. If your application later requires refresh tokens or additional parameters, ensure this interface remains flexible and well-documented.packages/authhero/src/authentication-flows/common.ts (1)
167-167: OptionalloginSessionusage.Allowing
loginSessionto be optional expands flexibility, but be cautious: dependent code must handleundefinedproperly. Consider clarifying in docs whenloginSessionis expected to be present versus undefined.packages/authhero/test/routes/auth-api/authenticate.spec.ts (2)
6-66: Successful login test.• Creates a user, sets a password, calls the API, and validates success.
• Verifies logs for a "Successful login".
• Coverage looks thorough.
Ensure assertions exist for token completeness (e.g., expiration or token claims) if required by your test strategy.
148-214: Account lockout scenario after repeated failures.Three failed attempts followed by a correct password yields a rejected request. This confirms rate-limit or progressive lockout logic.
Implementation appears valid.
Consider clarifying in documentation whether lockout is IP-based or user-based to reduce confusion.packages/kysely/CHANGELOG.md (1)
7-9: Fix duplicate "Updated dependencies" lineThe dependencies update line is duplicated unnecessarily.
Apply this diff to fix the duplication:
-Updated dependencies -Updated dependencies [a0a18c9] +Updated dependencies [a0a18c9]🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: This phrase is duplicated. You should probably use “Updated dependencies” only once.
Context: ...dapter ## 0.26.1 ### Patch Changes - Updated dependencies - Updated dependencies [a0a18c9] - @authhero/adapter-interfa...(PHRASE_REPETITION)
apps/demo/CHANGELOG.md (1)
7-8: Consider consolidating duplicate dependency updatesThe duplicate "Updated dependencies" lines can be consolidated into a single line for better readability.
-Updated dependencies -Updated dependencies [a0a18c9] +Updated dependencies [a0a18c9]🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: This phrase is duplicated. You should probably use “Updated dependencies” only once.
Context: ...o/demo ## 0.5.11 ### Patch Changes - Updated dependencies - Updated dependencies [a0a18c9] - [email protected] - @auth...(PHRASE_REPETITION)
packages/authhero/CHANGELOG.md (2)
12-13: Consider consolidating duplicate dependency updatesThe duplicate "Updated dependencies" lines can be consolidated into a single line for better readability.
-Updated dependencies -Updated dependencies [a0a18c9] +Updated dependencies [a0a18c9]🧰 Tools
🪛 LanguageTool
[grammar] ~12-~12: This phrase is duplicated. You should probably use “Updated dependencies” only once.
Context: ...uthorize endpoint ### Patch Changes - Updated dependencies - Updated dependencies [a0a18c9] - @authhero/adapter-interfa...(PHRASE_REPETITION)
7-8: Consider adding more details about the migration changesThe changelog entries "migrate connection auth" and "move most of authorize endpoint" could benefit from more details about what was changed and any potential breaking changes or migration steps required.
Consider adding:
- What aspects of connection auth were migrated
- Which parts of the authorize endpoint were moved and where
- Any required migration steps for users
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (46)
apps/demo/CHANGELOG.md(1 hunks)apps/demo/package.json(1 hunks)packages/adapter-interfaces/CHANGELOG.md(1 hunks)packages/adapter-interfaces/package.json(1 hunks)packages/adapter-interfaces/src/adapters/Users.ts(2 hunks)packages/adapter-interfaces/src/types/Logs.ts(1 hunks)packages/adapter-interfaces/src/types/User.ts(1 hunks)packages/authhero/CHANGELOG.md(1 hunks)packages/authhero/package.json(2 hunks)packages/authhero/src/auth-app.ts(2 hunks)packages/authhero/src/authentication-flows/common.ts(3 hunks)packages/authhero/src/authentication-flows/connection.ts(1 hunks)packages/authhero/src/authentication-flows/password.ts(1 hunks)packages/authhero/src/authentication-flows/silent.ts(1 hunks)packages/authhero/src/authentication-flows/ticket.ts(1 hunks)packages/authhero/src/authentication-flows/universal.ts(1 hunks)packages/authhero/src/constants.ts(1 hunks)packages/authhero/src/routes/auth-api/authenticate.ts(1 hunks)packages/authhero/src/routes/auth-api/authorize.ts(1 hunks)packages/authhero/src/routes/auth-api/dbconnections.ts(0 hunks)packages/authhero/src/routes/auth-api/index.ts(1 hunks)packages/authhero/src/routes/auth-api/passwordless.ts(1 hunks)packages/authhero/src/routes/management-api/users.ts(0 hunks)packages/authhero/src/strategies/apple.ts(1 hunks)packages/authhero/src/strategies/facebook.ts(1 hunks)packages/authhero/src/strategies/google-oauth2.ts(1 hunks)packages/authhero/src/strategies/index.ts(1 hunks)packages/authhero/src/types/AuthError.ts(1 hunks)packages/authhero/src/types/IdToken.ts(1 hunks)packages/authhero/src/utils/authIframe.ts(1 hunks)packages/authhero/src/utils/create-log-message.ts(2 hunks)packages/authhero/src/utils/is-valid-redirect-url.ts(1 hunks)packages/authhero/src/utils/url.ts(1 hunks)packages/authhero/test/authentication-flows/common.spec.ts(0 hunks)packages/authhero/test/authentication-flows/connection.spec.ts(1 hunks)packages/authhero/test/authentication-flows/silent.spec.ts(1 hunks)packages/authhero/test/authentication-flows/ticket.spec.ts(1 hunks)packages/authhero/test/authentication-flows/universal.spec.ts(1 hunks)packages/authhero/test/helpers/test-server.ts(1 hunks)packages/authhero/test/routes/auth-api/authenticate.spec.ts(1 hunks)packages/authhero/test/routes/auth-api/authorize.spec.ts(1 hunks)packages/drizzle/CHANGELOG.md(1 hunks)packages/drizzle/package.json(1 hunks)packages/kysely/CHANGELOG.md(1 hunks)packages/kysely/package.json(1 hunks)packages/kysely/src/users/create.ts(1 hunks)
💤 Files with no reviewable changes (3)
- packages/authhero/src/routes/auth-api/dbconnections.ts
- packages/authhero/test/authentication-flows/common.spec.ts
- packages/authhero/src/routes/management-api/users.ts
✅ Files skipped from review due to trivial changes (2)
- packages/drizzle/package.json
- packages/adapter-interfaces/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/authhero/src/utils/url.ts
[error] 7-7: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 Gitleaks (8.21.2)
packages/authhero/src/strategies/apple.ts
26-26: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🪛 LanguageTool
packages/kysely/CHANGELOG.md
[grammar] ~7-~7: This phrase is duplicated. You should probably use “Updated dependencies” only once.
Context: ...dapter ## 0.26.1 ### Patch Changes - Updated dependencies - Updated dependencies [a0a18c9] - @authhero/adapter-interfa...
(PHRASE_REPETITION)
apps/demo/CHANGELOG.md
[grammar] ~7-~7: This phrase is duplicated. You should probably use “Updated dependencies” only once.
Context: ...o/demo ## 0.5.11 ### Patch Changes - Updated dependencies - Updated dependencies [a0a18c9] - [email protected] - @auth...
(PHRASE_REPETITION)
packages/authhero/CHANGELOG.md
[grammar] ~12-~12: This phrase is duplicated. You should probably use “Updated dependencies” only once.
Context: ...uthorize endpoint ### Patch Changes - Updated dependencies - Updated dependencies [a0a18c9] - @authhero/adapter-interfa...
(PHRASE_REPETITION)
packages/drizzle/CHANGELOG.md
[grammar] ~7-~7: This phrase is duplicated. You should probably use “Updated dependencies” only once.
Context: ...rizzle ## 0.1.72 ### Patch Changes - Updated dependencies - Updated dependencies [a0a18c9] - @authhero/adapter-interfa...
(PHRASE_REPETITION)
🔇 Additional comments (68)
packages/drizzle/CHANGELOG.md (1)
3-16: Verify changelog entries align with PR objectives
The changelog only shows dependency version bumps but doesn't mention any changes related to the authorize endpoint migration mentioned in the PR objectives. Consider adding more detailed entries about the actual changes being made.
Let's check if there are any direct changes to the package that should be documented:
✅ Verification successful
The changelog entries are appropriate for this PR
The verification shows that the only changes in the drizzle package are version bumps in package.json and the corresponding CHANGELOG.md entries. There are no actual code changes in the src directory or any files containing "authorize" endpoints. This indicates that this package is only receiving dependency updates from @authhero/adapter-interfaces, and the current changelog entries accurately reflect these changes.
The authorize endpoint migration mentioned in the PR objectives likely affects other packages, but not the drizzle package directly. Therefore, no additional changelog entries are needed for this package.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for changes related to authorize endpoint in the drizzle package
# Search for files containing "authorize" in the drizzle package
fd . "packages/drizzle" -t f -x rg -l "authorize"
# Search for modified files in the current PR
git diff --name-only HEAD~1 packages/drizzle/
Length of output: 155
Script:
#!/bin/bash
# Let's check the actual changes in the package.json and look for any significant changes
git diff HEAD~1 packages/drizzle/package.json
# Also check if there are any code changes in the src directory
git diff --name-only HEAD~1 packages/drizzle/src/
Length of output: 458
🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: This phrase is duplicated. You should probably use “Updated dependencies” only once.
Context: ...rizzle ## 0.1.72 ### Patch Changes - Updated dependencies - Updated dependencies [a0a18c9] - @authhero/adapter-interfa...
(PHRASE_REPETITION)
packages/authhero/src/types/IdToken.ts (3)
1-2: Looks good: no issues with import statements.
The import from @hono/zod-openapi is appropriate for leveraging Zod-based schemas in this file.
3-22: Verify pass-through approach.
Calling .passthrough() on the schema allows unforeseen fields to be included. Ensure that you truly need these unknown properties to pass validation, otherwise consider .strict() or .strip() to prevent extraneous fields from making it further down the pipeline.
24-24: Type inference pattern is well-implemented.
Exporting IdToken via z.infer ensures type safety aligns with the Zod schema in your codebase.
packages/authhero/src/utils/authIframe.ts (2)
23-26: Validate the origin comparison logic
If you expect subdomains or slightly varied domains, strict equality (evt.origin != targetOrigin) may cause unintended rejections. Verify that requiring an exact match is appropriate and ensure that targetOrigin is correctly set for all relevant use cases.
50-51: Overall functionality appears correct
Returning the auth0Iframe string properly delivers the constructed document for iframe rendering. Implementation details beyond the above security and reliability concerns look good.
packages/authhero/src/strategies/facebook.ts (1)
9-36: Validate Facebook callback URL domain (optional)
The getRedirect function constructs a callback URL using the environment's ISSUER. For added security, you may wish to validate or whitelist the domain to prevent any potential misconfiguration or open redirect vulnerabilities.
packages/authhero/src/strategies/google-oauth2.ts (1)
49-50: PKCE requirement clarifications
The function strictly requires code_verifier. Confirm that all usage scenarios indeed require PKCE. If the traditional OAuth2 flow is ever used, you may want to allow a null or undefined code_verifier. Otherwise, throwing an error for a missing code_verifier is correct for PKCE-only flows.
packages/authhero/src/authentication-flows/ticket.ts (1)
32-35: Edge case: repeated usage of the same ticket
You are already checking if (!code || code.used_at). That effectively guards against repeated usage. However, you may want to consider auditing logs or attempts if multiple requests come in for the same ticket.
packages/authhero/src/authentication-flows/connection.ts (1)
21-23: Validate the 'state' parameter for improved security
This ensures a required state is present before proceeding, which helps prevent CSRF-like attacks and tracks user sessions properly.
packages/authhero/src/authentication-flows/password.ts (3)
19-23: Mandatory username check
Throwing a 400 error when username is missing is alerting and correct.
81-109: Adaptive lockout approach is solid
The logic to track failed login attempts within a 5-minute window and block further attempts is a strong security measure.
110-123: Email verification enforcement
The code properly checks if the user’s email is verified when the client enforces email validation. This is a good practice to comply with best security standards.
packages/authhero/src/routes/auth-api/authenticate.ts (2)
118-135: Username-password check
Invoking loginWithPassword and creating a new login session on success is correctly structured. This approach keeps the logic clean and reusable.
139-156: Ticket-based response
Generating the login ticket along with a combined code verifier approach is a neat, secure flow—ensuring the code is short-lived and ephemeral.
packages/authhero/src/routes/auth-api/authorize.ts (7)
93-99: Valid client check
Throwing a 400 when the client is not found is appropriate. This ensures invalid clients do not proceed further.
117-123: Restrict cross-origin requests
Blocking origins not in client.web_origins with a 403 is a solid security measure.
124-131: Redirect URI validation
Enforcing validation against client.callbacks is crucial to prevent redirect-based attacks.
142-163: Silent authentication
Using prompt == "none" with a prior check for response_type is a correct approach to support a seamless background login scenario when allowed.
173-176: Connection-based authentication
Delegating to connectionAuth for a specified connection fosters modularity. This is an effective design.
178-184: Ticket-based authentication
Using ticketAuth for the login ticket flow is consistent with existing patterns and ensures a cohesive approach.
186-195: Universal authentication fallback
Defaulting to universalAuth if other flows are not specified is a user-friendly approach that handles general cases effectively.
packages/authhero/src/routes/auth-api/index.ts (1)
6-8: Extended route exports
Exporting the new passwordless, authenticate, and authorize modules increases the modularity of the route definitions. Looks good!
packages/authhero/src/utils/url.ts (1)
1-4: Exported function looks good.
The new setSearchParams function signature clearly conveys its purpose, and the typed parameters will improve discoverability and safety.
packages/adapter-interfaces/src/adapters/Users.ts (1)
1-1: Breaking change to user creation API signature.
The create method now expects UserInsert instead of User. Ensure all calls are updated to supply the correct structure. Also verify that other parts of the application (e.g., forms, validations) align with the new schema flow.
Also applies to: 10-10
packages/authhero/src/types/AuthError.ts (1)
17-28: Structured authentication errors enhance debugging.
This new AuthError class and typed error codes are a good addition for consistent error handling across the authentication logic.
packages/authhero/src/strategies/index.ts (5)
1-2: Clean import statements.
These imports from external dependencies appear correct and follow standard conventions.
3-5: Consistent naming for strategy modules.
The use of separate imports (apple, facebook, google-oauth2) helps keep the strategies organized.
6-6: Appropriate reference to custom types.
Importing Bindings and Variables maintains type consistency across the codebase.
16-27: Method signatures look solid.
The getRedirect and validateAuthorizationCodeAndGetUser methods follow a clear contract for retrieving and validating authorization codes.
29-33: Neat strategy mapping.
Exporting strategies in a single object keeps the setup modular and makes each strategy discoverable in one place.
packages/authhero/src/utils/is-valid-redirect-url.ts (1)
3-3: Default value for allowed URLs.
Providing allowedUrls: string[] = [] helps avoid errors when no array is given and simplifies the function’s usage.
packages/authhero/src/auth-app.ts (2)
12-14: Additional route imports.
The new imports (passwordlessRoutes, authenticateRoutes, authorizeRoutes) extend the authentication functionality in a logically separated way.
35-37: Sequential route definition is consistent.
Routing /passwordless, /co/authenticate, and /authorize in a single chain is a clean pattern that organizes endpoints intuitively.
packages/kysely/src/users/create.ts (1)
14-14: login_count initialization.
Setting login_count to 0 explicitly is a good practice for clarity and consistent data handling, ensuring the field is always defined.
packages/authhero/test/authentication-flows/silent.spec.ts (3)
1-4: Imports look appropriate and consistent
No immediate issues found with the import statements.
11-17: Validate session edge cases
The test creates a valid session with an immediate expiration interval. It may be beneficial to add scenarios that verify how the code handles expired or non-existent sessions to ensure robust silent authentication.
Would you like me to draft an additional test that covers these edge cases?
19-26: Confirm prompt: "none" is tested
For silent authentication flows, specifying prompt: "none" is crucial. Ensure that the rest of the code correctly handles the absence of any user interaction. Testing the user’s inability to proceed if the session is invalid would strengthen end-to-end coverage.
packages/adapter-interfaces/src/types/User.ts (1)
34-34: UserInsert type definition is consistent
Defining UserInsert with z.infer<typeof userInsertSchema> aligns with best practices. No immediate issues noted.
packages/authhero/test/authentication-flows/ticket.spec.ts (3)
1-5: Validate import usage
Imports (vitest, testClient, nanoid, generateCodeVerifier) are correct for implementing ticket-based tests. No issues identified.
12-19: Consider verifying login session validity
The login session is created with a 1-hour expiration. Testing near-expired or already expired scenarios might bolster reliability.
24-30: Check code_verifier usage
Storing [co_id, co_verifier].join("|") is creative. Ensure that code consuming this field correctly parses both values. A mismatch in delimiter or parsing logic could cause ticket verification issues.
packages/authhero/test/routes/auth-api/authorize.spec.ts (2)
1-4: Imports and setup
All imports (vitest, testClient, getTestServer) align well with the pattern used across other tests. No issues found.
30-69: Ensure universal login redirection covers additional states
The test checks for a redirect to /u/enter-email. If multiple universal login endpoints exist (e.g., password-based, social login, etc.), verifying that each redirect path is correct for each scenario can improve coverage.
packages/authhero/test/authentication-flows/connection.spec.ts (1)
38-41: LGTM: redirect response testing is solid.
Verifying the 302 redirect and extracting the Location header is correct. The test logic is robust and ensures you capture invalid or empty headers early.
packages/authhero/test/authentication-flows/universal.spec.ts (1)
26-29: Ensure consistent encoding of query parameters.
When extracting the state from location?.split("state=")[1], consider using standard libraries to parse URLs and query parameters to avoid edge cases with special characters or additional parameters.
packages/authhero/test/helpers/test-server.ts (1)
62-62: Confirm web origins alignment with your environment settings.
web_origins is set to [ "https://example.com" ]. Verify that all expected origins are included, especially if supporting multiple environments or subdomains.
packages/authhero/src/authentication-flows/silent.ts (1)
76-85: Logging is clearly structured and consistent.
Creating logs for both success and failure provides good observability. This approach is well-defined and helps with debugging silent authentication flows.
packages/authhero/src/routes/auth-api/passwordless.ts (1)
171-171: Handle undefined client.callbacks.
Removing optional chaining means client.callbacks must always be defined. Verify that client.callbacks is initialized and not null; otherwise, isValidRedirectUrl could fail or throw unexpected errors.
packages/authhero/src/authentication-flows/common.ts (7)
3-3: Import of AuthorizationResponseType looks good.
Ensures local usage of the enum is valid. No further issues here.
176-176: sid fallback logic is consistent.
The code now tries params.sid and otherwise creates a session. This is a clear approach. Ensure that the session creation is only done when truly needed.
182-182: Passing sid to createAuthTokens.
This is a straightforward flow. Just confirm that the session usage doesn’t conflict with concurrent requests.
186-186: Securely setting the auth cookie.
Ensure the cookie is configured with the correct attributes (Secure, HttpOnly, SameSite) to prevent session hijacking in production.
190-190: WEB_MESSAGE response mode.
This is a valid approach for modern OAuth flows, especially for SPAs. Ensure the appropriate CORS or audience restrictions are in place.
196-198: Graceful fallback for response_type.
Defaulting to CODE is sensible. Confirm that all other response types remain fully supported if provided.
200-207: Ensuring loginSession is present for code flow.
Throwing an error is correct if loginSession is required. This will prevent generating codes without a valid login session, aligning with OAuth security standards.
packages/authhero/test/routes/auth-api/authenticate.spec.ts (3)
1-5: Base imports and setup.
Importing necessary testing tools and helpers. This is succinct and consistent with standard practices.
67-96: Non-existing user login attempt.
Appropriate negative test to confirm correct log creation and 403 response.
No issues found.
97-146: Incorrect password flow.
Well-structured coverage of invalid credentials and log recording.
Consider verifying the exact error code or message if future requirements become more specific.
apps/demo/package.json (3)
4-4: Version bump to 0.5.11.
Increment is consistent with semantic versioning for new dependencies. Confirm if a minor or patch increment is appropriate based on your release notes.
9-9: Updating @authhero/kysely-adapter to ^0.26.1.
Ensure any potential breaking changes are accounted for, especially regarding query generation or schema definitions.
13-13: Upgrading authhero to ^0.32.0.
Test any new or changed features in local or staging environments before merging, particularly the newly introduced authentication strategies.
packages/authhero/package.json (2)
3-3: LGTM: Version bump aligns with changes
The version increment to 0.32.0 corresponds with the migration of the authorize endpoint.
39-39: Verify arctic dependency security and stability
The new arctic dependency (^2.3.3) is being added, likely to support the authorize endpoint migration.
Run the following script to check for security advisories and latest versions:
✅ Verification successful
Arctic dependency ^2.3.3 is secure and up to date
The verification shows that:
- The specified version ^2.3.3 matches the latest version available on npm
- No security vulnerabilities were found in the GitHub Security Advisory Database
Let's also verify the package's stability by checking its download statistics and repository activity.
Arctic dependency ^2.3.3 is secure and well-maintained
The package shows healthy adoption and maintenance:
- Latest version 2.3.3 with no known security vulnerabilities
- ~159k downloads in the last month
- MIT licensed
- Actively maintained with recent updates (last modified Dec 17, 2024)
- Hosted on GitHub with proper repository structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security advisories and latest versions of the arctic package
# Check NPM for latest versions
curl -s https://registry.npmjs.org/arctic | jq '.["dist-tags"].latest'
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "arctic") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 420
Script:
#!/bin/bash
# Check package download stats and repository info
curl -s https://api.npmjs.org/downloads/point/last-month/arctic
# Get repository info using npm registry
curl -s https://registry.npmjs.org/arctic | jq '{
repository: .repository.url,
license: .license,
maintainers: .maintainers,
modified: .time.modified
}'
Length of output: 550
packages/kysely/package.json (1)
14-14: LGTM: Version bump aligns with dependency updates
The version increment to 0.26.1 corresponds with the update of adapter interfaces dependency.
packages/kysely/CHANGELOG.md (1)
3-22: LGTM: Changelog entries are clear and well-structured
The changelog properly documents:
- Version 0.26.1: Update to [email protected]
- Version 0.26.0: Setting login count to 0
🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: This phrase is duplicated. You should probably use “Updated dependencies” only once.
Context: ...dapter ## 0.26.1 ### Patch Changes - Updated dependencies - Updated dependencies [a0a18c9] - @authhero/adapter-interfa...
(PHRASE_REPETITION)
apps/demo/CHANGELOG.md (1)
3-25: Verify version bump alignment with changes
The changelog shows only dependency updates but uses patch version increments (0.5.9 → 0.5.10 → 0.5.11). This appears correct since these are only dependency updates.
Let's verify the version alignment with package.json:
✅ Verification successful
✓ Version numbers are correctly aligned
The version numbers in package.json perfectly match with the latest entries in the CHANGELOG.md:
- Current package version: 0.5.11
- Dependencies:
- authhero: ^0.32.0
- @authhero/kysely-adapter: ^0.26.1
The patch version increments (0.5.9 → 0.5.10 → 0.5.11) are appropriate for dependency updates, and the dependency versions are correctly synchronized between the changelog and package.json.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify version in package.json matches changelog
cat apps/demo/package.json | jq '.version'
Length of output: 51
Script:
#!/bin/bash
# Let's verify the dependencies versions in package.json
cat apps/demo/package.json | jq '.dependencies["authhero"], .dependencies["@authhero/kysely-adapter"]'
Length of output: 124
🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: This phrase is duplicated. You should probably use “Updated dependencies” only once.
Context: ...o/demo ## 0.5.11 ### Patch Changes - Updated dependencies - Updated dependencies [a0a18c9] - [email protected] - @auth...
(PHRASE_REPETITION)
packages/authhero/CHANGELOG.md (1)
3-15: Version bump aligns with changes
The minor version bump (0.31.0 → 0.32.0) is appropriate as new features were added without breaking changes.
Let's verify the version in package.json:
✅ Verification successful
Version in package.json matches CHANGELOG.md
The version "0.32.0" in package.json aligns with the version documented in CHANGELOG.md, confirming proper version management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify version in package.json matches changelog
cat packages/authhero/package.json | jq '.version'
Length of output: 59
🧰 Tools
🪛 LanguageTool
[grammar] ~12-~12: This phrase is duplicated. You should probably use “Updated dependencies” only once.
Context: ...uthorize endpoint ### Patch Changes - Updated dependencies - Updated dependencies [a0a18c9] - @authhero/adapter-interfa...
(PHRASE_REPETITION)
| BLOCKED_ACCOUNT_EMAIL = "limit_wc", | ||
| BLOCKED_ACCOUNT_IP = "limit_sul", | ||
| BLOCKED_IP_ADDRESS = "limit_mu", |
There was a problem hiding this comment.
Ensure consistency in the Zod schema enum.
These new log types (limit_wc, limit_sul, and limit_mu) are not included in the LogType Zod enum below. To prevent runtime validation errors or misinterpretation of logs, add these new values to the Zod schema as well.
Add them as follows:
const LogType = z.enum([
"cls", // CODE_LINK_SENT
...
"sv", // SUCCESS_VERIFICATION_EMAIL
"svr", // SUCCESS_VERIFICATION_EMAIL_REQUEST
+ "limit_wc", // BLOCKED_ACCOUNT_EMAIL
+ "limit_sul", // BLOCKED_ACCOUNT_IP
+ "limit_mu", // BLOCKED_IP_ADDRESS
]);Committable suggestion skipped: line range outside the PR's diff.
| var authorizationResponse = { | ||
| type: "authorization_response", | ||
| response: ${response} |
There was a problem hiding this comment.
Potential XSS vulnerability with unsanitized input
You are directly injecting the response parameter into the script, which could lead to Cross-Site Scripting (XSS) if it contains malicious content. Ensure that response is sanitized or encoded before being included in the HTML to prevent security risks.
Example mitigation via escaping:
- response: ${response}
+ response: ${JSON.stringify(response)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var authorizationResponse = { | |
| type: "authorization_response", | |
| response: ${response} | |
| var authorizationResponse = { | |
| type: "authorization_response", | |
| response: ${JSON.stringify(response)} |
| const privateKeyBuffer = Buffer.from(options.app_secret, "utf-8"); | ||
| const cleanedKey = privateKeyBuffer | ||
| .toString() | ||
| .replace(/-----BEGIN PRIVATE KEY-----|-----END PRIVATE KEY-----|\s/g, ""); | ||
| const keyArray = Uint8Array.from(Buffer.from(cleanedKey, "base64")); | ||
| // Clear sensitive data from memory | ||
| privateKeyBuffer.fill(0); |
There was a problem hiding this comment.
Potential private key exposure
Static analysis flagged the private key usage. Although the code immediately clears the buffer, storing app_secret in plain text within environment configuration remains risky. Consider integrating a secure vault or key management solution to further protect private key material.
🧰 Tools
🪛 Gitleaks (8.21.2)
26-26: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
| } | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Based on the code analysis, I can see that authorization_endpoint is defined in the connection schema with z.string().default("").optional(), which means it's an optional field that defaults to an empty string. However, the code is using a non-null assertion (!) without checking if the value is empty or undefined, which could lead to runtime errors.
Add validation for authorization_endpoint
The code should validate that authorization_endpoint exists and is not empty before using it. Here are the specific issues:
authorization_endpointis optional in the connection schema- Using
!operator without validation could cause runtime errors if the value is empty - The empty string default value could cause invalid URL construction
🔗 Analysis chain
Verify authorization endpoint presence
options.authorization_endpoint! is used without a prior check. If authorization_endpoint is missing or null, this will fail at runtime.
Let me gather more context about the usage and validation of authorization_endpoint.
Let me check the full context of where this options object comes from and any validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Script to locate references to 'authorization_endpoint' usage and verify it's defined
rg 'authorization_endpoint'
Length of output: 1045
Script:
#!/bin/bash
# Check the schema definition and validation for authorization_endpoint
ast-grep --pattern 'authorization_endpoint: $_'
# Check if there's any validation before using authorization_endpoint
ast-grep --pattern 'options.authorization_endpoint'
Length of output: 227
Script:
#!/bin/bash
# Get the full context of the file where authorization_endpoint is used
cat packages/authhero/src/authentication-flows/connection.ts
# Check for any zod schema validations
ast-grep --pattern 'z.object({$$$})'
Length of output: 65873
| it("should return a auth response if the login hint matches the current email_hint", async () => { | ||
| const { oauthApp, env } = await getTestServer(); | ||
| const oauthClient = testClient(oauthApp, env); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider testing a scenario where the user is not yet authenticated.
The second test verifies a scenario with an existing session. Adding a counterpart test for when no session exists would ensure coverage of fallback or error logic.
| if (session) { | ||
| ctx.set("user_id", session.user_id); | ||
|
|
||
| // Update the cookie | ||
| const headers = new Headers(); | ||
| const cookie = serializeAuthCookie(client.tenant.id, session.session_id); | ||
| headers.set("set-cookie", cookie); | ||
|
|
There was a problem hiding this comment.
Ensure session validity before setting user context.
Currently, if session is passed in but is expired or otherwise invalid, the code proceeds with user context. Consider explicitly validating session expiration here.
if (session) {
+ if (new Date(session.expires_at) < new Date()) {
+ // The session is expired
+ // Return an error response or fallback
+ }
ctx.set("user_id", session.user_id);
...
}Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
New Features
Improvements
Bug Fixes
Chores
@authhero/demo,@authhero/adapter-interfaces,@authhero/drizzle, and@authhero/kysely-adapterarcticfor OAuth strategies