feat: signal handshake nonce support for all flows #5908
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
clerk-cookie
added
types
clerk-js
remix
backend
express
tanstack
react-router
and removed
clerk-js
labels
jacekradko
force-pushed
the
feat/signal-handshake-nonce-support-oauth
branch
from
453695c to
c8379a0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/backend/src/util/__tests__/handshakeUtils.test.ts (1)
1-8: Fix import sorting order.The ESLint rule
simple-import-sort/importsrequires imports to be sorted. Consider running the autofix or manually reordering.Apply this diff to fix the import order:
-import { describe, expect, it } from 'vitest'; -import { constants, SUPPORTED_HANDSHAKE_FORMAT } from '../../constants'; -import type { AuthenticateContext } from '../../tokens/authenticateContext'; -import { - appendHandshakeFormatToOAuthCallback, - createHandshakeFormatHeaders, - getHandshakeFormatCookie, -} from '../handshakeUtils'; +import { describe, expect, it } from 'vitest'; + +import { constants, SUPPORTED_HANDSHAKE_FORMAT } from '../../constants'; +import type { AuthenticateContext } from '../../tokens/authenticateContext'; +import { + appendHandshakeFormatToOAuthCallback, + createHandshakeFormatHeaders, + getHandshakeFormatCookie, +} from '../handshakeUtils';packages/backend/src/util/handshakeUtils.ts (1)
1-1: Remove unused import.The import
SUPPORTS_HANDSHAKE_NONCEis not used anywhere in this file and should be removed to fix ESLint errors.Apply this diff to remove the unused import:
-import { constants, SUPPORTED_HANDSHAKE_FORMAT, SUPPORTS_HANDSHAKE_NONCE } from '../constants'; +import { constants, SUPPORTED_HANDSHAKE_FORMAT } from '../constants';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/backend/src/tokens/__tests__/handshake.test.ts(1 hunks)packages/backend/src/tokens/authenticateContext.ts(3 hunks)packages/backend/src/tokens/handshake.ts(2 hunks)packages/backend/src/util/__tests__/handshakeUtils.test.ts(1 hunks)packages/backend/src/util/handshakeUtils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/backend/src/tokens/authenticateContext.ts
🧰 Additional context used
📓 Path-based instructions (6)
`**/*.{js,ts,tsx,jsx}`: All code must pass ESLint checks with the project's configuration. Use Prettier for consistent code formatting.
**/*.{js,ts,tsx,jsx}: All code must pass ESLint checks with the project's configuration.
Use Prettier for consistent code formatting.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/backend/src/tokens/__tests__/handshake.test.tspackages/backend/src/util/__tests__/handshakeUtils.test.tspackages/backend/src/util/handshakeUtils.tspackages/backend/src/tokens/handshake.ts
`**/*.{ts,tsx}`: Maintain comprehensive JSDoc comments for public APIs.
**/*.{ts,tsx}: Maintain comprehensive JSDoc comments for public APIs.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/backend/src/tokens/__tests__/handshake.test.tspackages/backend/src/util/__tests__/handshakeUtils.test.tspackages/backend/src/util/handshakeUtils.tspackages/backend/src/tokens/handshake.ts
`**/*.{test,spec}.{js,ts,tsx,jsx}`: Unit tests are required for all new functionality.
**/*.{test,spec}.{js,ts,tsx,jsx}: Unit tests are required for all new functionality.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/backend/src/tokens/__tests__/handshake.test.tspackages/backend/src/util/__tests__/handshakeUtils.test.ts
`**/__tests__/**/*.{js,ts,tsx,jsx}`: Test files should be co-located with source files or in `__tests__` directories.
**/__tests__/**/*.{js,ts,tsx,jsx}: Test files should be co-located with source files or in__tests__directories.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/backend/src/tokens/__tests__/handshake.test.tspackages/backend/src/util/__tests__/handshakeUtils.test.ts
`packages/**`: All publishable packages under the @clerk namespace must be located in the packages/ directory.
packages/**: All publishable packages under the @clerk namespace must be located in the packages/ directory.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/global.mdc)
List of files the instruction was applied to:
packages/backend/src/tokens/__tests__/handshake.test.tspackages/backend/src/util/__tests__/handshakeUtils.test.tspackages/backend/src/util/handshakeUtils.tspackages/backend/src/tokens/handshake.ts
`**/*.ts`: Always define explicit return types for functions, especially public ...
**/*.ts: Always define explicit return types for functions, especially public APIs.
Use proper type annotations for variables and parameters where inference isn't clear.
Avoidanytype; preferunknownwhen type is uncertain, then narrow with type guards.
Useinterfacefor object shapes that might be extended; usetypefor unions, primitives, and computed types.
Preferreadonlyproperties for immutable data structures.
Useprivatefor internal implementation details,protectedfor inheritance, andpublicexplicitly for clarity in public APIs.
Prefer composition and interfaces over deep inheritance chains; use mixins for shared behavior.
Use ES6 imports/exports consistently; avoid barrel files (index.ts re-exports) to prevent circular dependencies.
Use type-only imports (import type { ... }) where possible.
Useas constfor literal types and thesatisfiesoperator for type checking without widening.
Enable--incrementaland--tsBuildInfoFilefor faster builds.
Use ESLint with @typescript-eslint/recommended rules and Prettier for formatting.
Use lint-staged and Husky for pre-commit checks.
Use type-coverage to measure type safety.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/typescript.mdc)
List of files the instruction was applied to:
packages/backend/src/tokens/__tests__/handshake.test.tspackages/backend/src/util/__tests__/handshakeUtils.test.tspackages/backend/src/util/handshakeUtils.tspackages/backend/src/tokens/handshake.ts
🧬 Code Graph Analysis (3)
packages/backend/src/util/__tests__/handshakeUtils.test.ts (3)
packages/backend/src/tokens/authenticateContext.ts (1)
AuthenticateContext(315-315)packages/backend/src/util/handshakeUtils.ts (3)
appendHandshakeFormatToOAuthCallback(12-21)getHandshakeFormatCookie(29-34)createHandshakeFormatHeaders(42-51)packages/backend/src/constants.ts (2)
constants(80-86)SUPPORTED_HANDSHAKE_FORMAT(7-7)
packages/backend/src/util/handshakeUtils.ts (2)
packages/backend/src/tokens/authenticateContext.ts (1)
AuthenticateContext(315-315)packages/backend/src/constants.ts (2)
constants(80-86)SUPPORTED_HANDSHAKE_FORMAT(7-7)
packages/backend/src/tokens/handshake.ts (1)
packages/backend/src/constants.ts (2)
constants(80-86)SUPPORTED_HANDSHAKE_FORMAT(7-7)
🪛 ESLint
packages/backend/src/util/__tests__/handshakeUtils.test.ts
[error] 1-8: Run autofix to sort these imports!
(simple-import-sort/imports)
packages/backend/src/util/handshakeUtils.ts
[error] 1-1: 'SUPPORTS_HANDSHAKE_NONCE' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 1-1: 'SUPPORTS_HANDSHAKE_NONCE' is defined but never used.
(unused-imports/no-unused-imports)
packages/backend/src/tokens/handshake.ts
[error] 1-1: 'SUPPORTS_HANDSHAKE_NONCE' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 1-1: 'SUPPORTS_HANDSHAKE_NONCE' is defined but never used.
(unused-imports/no-unused-imports)
🪛 GitHub Actions: CI
packages/backend/src/tokens/handshake.ts
[error] 1-1: TypeScript error TS2305: Module '../constants' has no exported member 'SUPPORTS_HANDSHAKE_NONCE'.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
packages/backend/src/tokens/handshake.ts (1)
152-157: Excellent implementation of handshake format parameter.The addition of the handshake format query parameter is well-documented and correctly implemented. The comment clearly explains the purpose - indicating the backend's capability to handle nonce-based handshakes to FAPI.
packages/backend/src/tokens/__tests__/handshake.test.ts (2)
431-432: Good addition to existing test coverage.Adding the handshake format assertion to the existing comprehensive test ensures the parameter is always included in the required parameters check.
434-467: Comprehensive test coverage for handshake format feature.The new test cases thoroughly verify the handshake format parameter is included in both production and development contexts. The tests are well-structured and follow the existing patterns.
packages/backend/src/util/__tests__/handshakeUtils.test.ts (1)
10-88: Excellent comprehensive test suite.The test coverage is thorough and well-structured:
- Tests all three utility functions with both nonce and token contexts
- Verifies edge cases like preserving existing query parameters
- Includes type checks for returned objects
- Mock objects are appropriately typed
packages/backend/src/util/handshakeUtils.ts (1)
12-51: Well-implemented utility functions for handshake format handling.The three utility functions are well-designed and documented:
appendHandshakeFormatToOAuthCallback: Correctly uses URL API and preserves existing parametersgetHandshakeFormatCookie: Appropriate cookie attributes (SameSite=None; Secure) for cross-site OAuth flowscreateHandshakeFormatHeaders: Good separation of concerns by reusing the cookie functionThe conditional logic based on
canHandleNonceHandshake()ensures the handshake format is only applied when supported.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/backend/src/util/__tests__/handshakeUtils.test.ts (1)
1-9: Fix import sorting to comply with ESLint rules.The imports need to be sorted according to the project's ESLint configuration.
-import { describe, expect, it } from 'vitest'; -import { constants, SUPPORTED_HANDSHAKE_FORMAT } from '../../constants'; -import type { AuthenticateContext } from '../../tokens/authenticateContext'; -import { - appendHandshakeFormatToOAuthCallback, - createHandshakeFormatHeaders, - enhanceOAuthRedirectUrl, - getHandshakeFormatCookie, -} from '../handshakeUtils'; +import { describe, expect, it } from 'vitest'; + +import { constants, SUPPORTED_HANDSHAKE_FORMAT } from '../../constants'; +import type { AuthenticateContext } from '../../tokens/authenticateContext'; +import { + appendHandshakeFormatToOAuthCallback, + createHandshakeFormatHeaders, + enhanceOAuthRedirectUrl, + getHandshakeFormatCookie, +} from '../handshakeUtils';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/backend/src/tokens/request.ts(4 hunks)packages/backend/src/util/__tests__/handshakeUtils.test.ts(1 hunks)packages/backend/src/util/handshakeUtils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/backend/src/tokens/request.ts
🧰 Additional context used
📓 Path-based instructions (6)
`**/*.{js,ts,tsx,jsx}`: All code must pass ESLint checks with the project's configuration. Use Prettier for consistent code formatting.
**/*.{js,ts,tsx,jsx}: All code must pass ESLint checks with the project's configuration.
Use Prettier for consistent code formatting.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/backend/src/util/handshakeUtils.tspackages/backend/src/util/__tests__/handshakeUtils.test.ts
`**/*.{ts,tsx}`: Maintain comprehensive JSDoc comments for public APIs.
**/*.{ts,tsx}: Maintain comprehensive JSDoc comments for public APIs.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/backend/src/util/handshakeUtils.tspackages/backend/src/util/__tests__/handshakeUtils.test.ts
`packages/**`: All publishable packages under the @clerk namespace must be located in the packages/ directory.
packages/**: All publishable packages under the @clerk namespace must be located in the packages/ directory.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/global.mdc)
List of files the instruction was applied to:
packages/backend/src/util/handshakeUtils.tspackages/backend/src/util/__tests__/handshakeUtils.test.ts
`**/*.ts`: Always define explicit return types for functions, especially public ...
**/*.ts: Always define explicit return types for functions, especially public APIs.
Use proper type annotations for variables and parameters where inference isn't clear.
Avoidanytype; preferunknownwhen type is uncertain, then narrow with type guards.
Useinterfacefor object shapes that might be extended; usetypefor unions, primitives, and computed types.
Preferreadonlyproperties for immutable data structures.
Useprivatefor internal implementation details,protectedfor inheritance, andpublicexplicitly for clarity in public APIs.
Prefer composition and interfaces over deep inheritance chains; use mixins for shared behavior.
Use ES6 imports/exports consistently; avoid barrel files (index.ts re-exports) to prevent circular dependencies.
Use type-only imports (import type { ... }) where possible.
Useas constfor literal types and thesatisfiesoperator for type checking without widening.
Enable--incrementaland--tsBuildInfoFilefor faster builds.
Use ESLint with @typescript-eslint/recommended rules and Prettier for formatting.
Use lint-staged and Husky for pre-commit checks.
Use type-coverage to measure type safety.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/typescript.mdc)
List of files the instruction was applied to:
packages/backend/src/util/handshakeUtils.tspackages/backend/src/util/__tests__/handshakeUtils.test.ts
`**/*.{test,spec}.{js,ts,tsx,jsx}`: Unit tests are required for all new functionality.
**/*.{test,spec}.{js,ts,tsx,jsx}: Unit tests are required for all new functionality.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/backend/src/util/__tests__/handshakeUtils.test.ts
`**/__tests__/**/*.{js,ts,tsx,jsx}`: Test files should be co-located with source files or in `__tests__` directories.
**/__tests__/**/*.{js,ts,tsx,jsx}: Test files should be co-located with source files or in__tests__directories.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/backend/src/util/__tests__/handshakeUtils.test.ts
🧬 Code Graph Analysis (1)
packages/backend/src/util/handshakeUtils.ts (2)
packages/backend/src/tokens/authenticateContext.ts (1)
AuthenticateContext(315-315)packages/backend/src/constants.ts (2)
constants(80-86)SUPPORTED_HANDSHAKE_FORMAT(7-7)
🪛 ESLint
packages/backend/src/util/__tests__/handshakeUtils.test.ts
[error] 1-9: Run autofix to sort these imports!
(simple-import-sort/imports)
[error] 76-76: Forbidden non-null assertion.
(@typescript-eslint/no-non-null-assertion)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
packages/backend/src/util/handshakeUtils.ts (5)
1-2: LGTM: Clean imports and type safety.The imports are well-organized and use type-only imports where appropriate, following TypeScript best practices.
4-21: LGTM: Well-documented OAuth callback URL enhancement.The function correctly implements the OAuth callback URL modification logic with proper:
- JSDoc documentation explaining FAPI compliance purpose
- Conditional logic based on nonce handshake support
- URL manipulation using the native URL API
- Explicit return type annotation
The implementation safely adds the handshake format parameter only when the backend supports nonce handshakes.
23-63: LGTM: Robust URL enhancement with proper error handling.This function demonstrates excellent defensive programming:
- Early return for invalid inputs (empty URL or unsupported handshake)
- Comprehensive error handling with try-catch blocks to prevent crashes
- Support for multiple URL patterns (oauth_callback, oauth-callback)
- Nested callback URL handling for complex OAuth flows
- Graceful fallback to original URL on parsing failures
The logic correctly identifies OAuth callback URLs both directly and within nested redirect parameters.
65-77: LGTM: Secure cookie generation with appropriate attributes.The cookie generation follows security best practices:
Path=/for application-wide availabilitySameSite=Noneappropriate for cross-site OAuth flowsSecureflag for HTTPS-only transmission- Conditional generation based on nonce handshake support
These attributes are essential for OAuth flows that involve cross-origin redirects.
79-94: LGTM: Clean header creation with proper composition.The function correctly:
- Creates a new Headers instance
- Reuses the cookie generation logic for consistency
- Conditionally adds the Set-Cookie header
- Returns the Headers object for further manipulation
Good separation of concerns by delegating cookie generation to the dedicated function.
packages/backend/src/util/__tests__/handshakeUtils.test.ts (5)
11-21: LGTM: Well-structured test setup with proper mocks.The mock AuthenticateContext objects correctly simulate both nonce and token handshake scenarios, providing comprehensive test coverage for different authentication contexts.
22-48: LGTM: Comprehensive test coverage for OAuth callback URL modification.The tests effectively verify:
- Handshake format parameter addition for nonce support
- No modification when token handshake is used
- Preservation of existing query parameters
Good test structure with clear assertions and meaningful test descriptions.
50-111: LGTM: Thorough testing of complex URL enhancement logic.Excellent test coverage including:
- Direct OAuth callback URL enhancement
- Support for both
oauth_callbackandoauth-callbackpatterns- Nested redirect URL handling
- Edge cases (empty URL, malformed URL)
- Parameter preservation
The tests validate the robust error handling implemented in the utility function.
113-127: LGTM: Proper cookie string validation.The tests correctly verify:
- Cookie string format and attributes
- Conditional cookie generation based on handshake support
- Exact string matching for cookie attributes
The cookie string validation ensures security attributes are properly set.
129-151: LGTM: Complete header creation testing.The tests effectively validate:
- Set-Cookie header presence for nonce support
- Empty headers for token handshake
- Headers instance type verification
Good coverage of both positive and negative scenarios.
jacekradko
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/backend/src/util/__tests__/handshakeUtils.test.ts (1)
1-9: Fix import sorting to comply with ESLint rules.The imports need to be sorted according to the project's ESLint configuration.
+import { describe, expect, it } from 'vitest'; + -import { describe, expect, it } from 'vitest'; import { constants, SUPPORTED_HANDSHAKE_FORMAT } from '../../constants'; import type { AuthenticateContext } from '../../tokens/authenticateContext'; import { appendHandshakeFormatToOAuthCallback, createHandshakeFormatHeaders, enhanceOAuthRedirectUrl, getHandshakeFormatCookie, } from '../handshakeUtils';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/backend/src/tokens/handshake.ts(2 hunks)packages/backend/src/util/__tests__/handshakeUtils.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/backend/src/tokens/handshake.ts
🧰 Additional context used
📓 Path-based instructions (6)
`**/*.{js,ts,tsx,jsx}`: All code must pass ESLint checks with the project's configuration. Use Prettier for consistent code formatting.
**/*.{js,ts,tsx,jsx}: All code must pass ESLint checks with the project's configuration.
Use Prettier for consistent code formatting.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/backend/src/util/__tests__/handshakeUtils.test.ts
`**/*.{ts,tsx}`: Maintain comprehensive JSDoc comments for public APIs.
**/*.{ts,tsx}: Maintain comprehensive JSDoc comments for public APIs.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/backend/src/util/__tests__/handshakeUtils.test.ts
`**/*.{test,spec}.{js,ts,tsx,jsx}`: Unit tests are required for all new functionality.
**/*.{test,spec}.{js,ts,tsx,jsx}: Unit tests are required for all new functionality.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/backend/src/util/__tests__/handshakeUtils.test.ts
`**/__tests__/**/*.{js,ts,tsx,jsx}`: Test files should be co-located with source files or in `__tests__` directories.
**/__tests__/**/*.{js,ts,tsx,jsx}: Test files should be co-located with source files or in__tests__directories.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/backend/src/util/__tests__/handshakeUtils.test.ts
`packages/**`: All publishable packages under the @clerk namespace must be located in the packages/ directory.
packages/**: All publishable packages under the @clerk namespace must be located in the packages/ directory.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/global.mdc)
List of files the instruction was applied to:
packages/backend/src/util/__tests__/handshakeUtils.test.ts
`**/*.ts`: Always define explicit return types for functions, especially public ...
**/*.ts: Always define explicit return types for functions, especially public APIs.
Use proper type annotations for variables and parameters where inference isn't clear.
Avoidanytype; preferunknownwhen type is uncertain, then narrow with type guards.
Useinterfacefor object shapes that might be extended; usetypefor unions, primitives, and computed types.
Preferreadonlyproperties for immutable data structures.
Useprivatefor internal implementation details,protectedfor inheritance, andpublicexplicitly for clarity in public APIs.
Prefer composition and interfaces over deep inheritance chains; use mixins for shared behavior.
Use ES6 imports/exports consistently; avoid barrel files (index.ts re-exports) to prevent circular dependencies.
Use type-only imports (import type { ... }) where possible.
Useas constfor literal types and thesatisfiesoperator for type checking without widening.
Enable--incrementaland--tsBuildInfoFilefor faster builds.
Use ESLint with @typescript-eslint/recommended rules and Prettier for formatting.
Use lint-staged and Husky for pre-commit checks.
Use type-coverage to measure type safety.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/typescript.mdc)
List of files the instruction was applied to:
packages/backend/src/util/__tests__/handshakeUtils.test.ts
🧬 Code Graph Analysis (1)
packages/backend/src/util/__tests__/handshakeUtils.test.ts (3)
packages/backend/src/tokens/authenticateContext.ts (1)
AuthenticateContext(315-315)packages/backend/src/util/handshakeUtils.ts (4)
appendHandshakeFormatToOAuthCallback(12-21)enhanceOAuthRedirectUrl(31-63)getHandshakeFormatCookie(71-77)createHandshakeFormatHeaders(85-94)packages/backend/src/constants.ts (2)
constants(80-86)SUPPORTED_HANDSHAKE_FORMAT(7-7)
🪛 ESLint
packages/backend/src/util/__tests__/handshakeUtils.test.ts
[error] 1-9: Run autofix to sort these imports!
(simple-import-sort/imports)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
packages/backend/src/util/__tests__/handshakeUtils.test.ts (5)
12-21: Well-designed mock objects for testing.The mock
AuthenticateContextobjects effectively simulate both nonce and token handshake scenarios, providing clear separation for testing different code paths.
22-48: Comprehensive test coverage for URL parameter handling.The tests effectively validate the
appendHandshakeFormatToOAuthCallbackfunction across multiple scenarios:
- Nonce handshake support adds the format parameter
- Token handshake doesn't modify the URL
- Existing query parameters are preserved
50-111: Excellent test coverage for OAuth redirect URL enhancement.The test suite thoroughly covers all scenarios for
enhanceOAuthRedirectUrl:
- Direct OAuth callback URL enhancement
- Nested redirect URL handling
- Error resilience with malformed URLs
- Parameter preservation
- Proper handling when nonce is not supported
The use of type assertion on line 76 (
as string) is appropriate since the preceding assertion ensures the value is defined.
113-127: Clear and focused cookie generation tests.The tests properly validate cookie string generation with appropriate security attributes (Path, SameSite, Secure) and correct conditional behavior based on handshake format support.
129-152: Thorough validation of HTTP header creation.The tests verify both the content and type of headers returned by
createHandshakeFormatHeaders, ensuring proper Set-Cookie header generation and empty headers when appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/clerk-js/src/core/resources/SignUp.ts (1)
292-301: Improve type safety for authParams object.The
authParamsobject is typed asany, which reduces type safety. Consider defining a proper interface or type for the authentication parameters.- const authParams: any = { + const authParams: Record<string, unknown> = { strategy, redirectUrl: SignUp.clerk.buildUrlWithAuth(redirectUrl), actionCompleteRedirectUrl: redirectUrlComplete, unsafeMetadata, emailAddress, legalAccepted, oidcPrompt, };packages/clerk-js/src/core/resources/SignIn.ts (1)
237-242: Improve type safety for createParams object.Similar to the SignUp implementation, the
createParamsobject is typed asany, which reduces type safety. Consider using a more specific type.- const createParams: any = { + const createParams: Record<string, unknown> = { strategy, identifier, redirectUrl: SignIn.clerk.buildUrlWithAuth(redirectUrl), actionCompleteRedirectUrl: redirectUrlComplete, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/clerk-js/src/core/auth/AuthCookieService.ts(4 hunks)packages/clerk-js/src/core/auth/handshakeFormat.ts(1 hunks)packages/clerk-js/src/core/clerk.ts(1 hunks)packages/clerk-js/src/core/resources/SignIn.ts(1 hunks)packages/clerk-js/src/core/resources/SignUp.ts(2 hunks)packages/types/src/signIn.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/clerk-js/src/core/clerk.ts
- packages/clerk-js/src/core/auth/handshakeFormat.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/types/src/signIn.ts
🧰 Additional context used
📓 Path-based instructions (4)
`**/*.{js,ts,tsx,jsx}`: All code must pass ESLint checks with the project's configuration. Use Prettier for consistent code formatting.
**/*.{js,ts,tsx,jsx}: All code must pass ESLint checks with the project's configuration.
Use Prettier for consistent code formatting.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/clerk-js/src/core/resources/SignUp.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/resources/SignIn.ts
`**/*.{ts,tsx}`: Maintain comprehensive JSDoc comments for public APIs.
**/*.{ts,tsx}: Maintain comprehensive JSDoc comments for public APIs.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/clerk-js/src/core/resources/SignUp.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/resources/SignIn.ts
`packages/**`: All publishable packages under the @clerk namespace must be located in the packages/ directory.
packages/**: All publishable packages under the @clerk namespace must be located in the packages/ directory.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/global.mdc)
List of files the instruction was applied to:
packages/clerk-js/src/core/resources/SignUp.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/resources/SignIn.ts
`**/*.ts`: Always define explicit return types for functions, especially public ...
**/*.ts: Always define explicit return types for functions, especially public APIs.
Use proper type annotations for variables and parameters where inference isn't clear.
Avoidanytype; preferunknownwhen type is uncertain, then narrow with type guards.
Useinterfacefor object shapes that might be extended; usetypefor unions, primitives, and computed types.
Preferreadonlyproperties for immutable data structures.
Useprivatefor internal implementation details,protectedfor inheritance, andpublicexplicitly for clarity in public APIs.
Prefer composition and interfaces over deep inheritance chains; use mixins for shared behavior.
Use ES6 imports/exports consistently; avoid barrel files (index.ts re-exports) to prevent circular dependencies.
Use type-only imports (import type { ... }) where possible.
Useas constfor literal types and thesatisfiesoperator for type checking without widening.
Enable--incrementaland--tsBuildInfoFilefor faster builds.
Use ESLint with @typescript-eslint/recommended rules and Prettier for formatting.
Use lint-staged and Husky for pre-commit checks.
Use type-coverage to measure type safety.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/typescript.mdc)
List of files the instruction was applied to:
packages/clerk-js/src/core/resources/SignUp.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/resources/SignIn.ts
🧬 Code Graph Analysis (2)
packages/clerk-js/src/core/resources/SignUp.ts (1)
packages/react/src/components/uiComponents.tsx (1)
SignUp(157-183)
packages/clerk-js/src/core/auth/AuthCookieService.ts (1)
packages/clerk-js/src/core/auth/handshakeFormat.ts (2)
HandshakeFormatDetector(6-8)createHandshakeFormatDetector(14-28)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
packages/clerk-js/src/core/resources/SignUp.ts (1)
302-306: LGTM: Clean implementation of nonce handshake support.The conditional logic for adding nonce format support is well-implemented and follows a clean pattern. The feature detection through the internal method ensures backward compatibility.
packages/clerk-js/src/core/auth/AuthCookieService.ts (4)
19-20: LGTM: Proper import organization.The imports for the handshake format detector types and factory function are correctly added and follow the existing import patterns.
47-47: LGTM: Appropriate encapsulation.The private member declaration for
handshakeFormatDetectorfollows the established pattern of other service dependencies and maintains proper encapsulation.
85-85: LGTM: Consistent initialization pattern.The initialization of the handshake format detector in the constructor follows the same pattern as other service dependencies and correctly passes the required cookieSuffix parameter.
255-257: LGTM: Clean delegation pattern.The public method
supportsNonceHandshake()provides a clean interface and properly delegates to the detector. The explicit return type annotation enhances type safety.packages/clerk-js/src/core/resources/SignIn.ts (1)
244-249: LGTM: Consistent nonce handshake implementation.The conditional logic for nonce format support is consistent with the SignUp implementation, ensuring uniform behavior across authentication flows. The feature detection approach is well-designed.
Description
Use handshake nonce in OAUTH flow
Related: #5905
Related: SDKI-979
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Style