bug_fix: fixed issue #6283 #6284
Conversation
|
|
@PrashanthKumar0 is attempting to deploy a commit to the Clerk Production Team on Vercel. A member of the Team first needs to authorize it. |
📝 Walkthrough""" WalkthroughThe change updates the Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🔭 Outside diff range comments (2)
packages/express/src/webhooks.ts (2)
23-32: Add explicit return type and improve error handling.The function is missing an explicit return type, which violates the coding guidelines. Additionally, there's no error handling or input validation for the
JSON.stringifyoperation.-export async function verifyWebhook(req: ExpressRequest, options?: VerifyWebhookOptions) { +export async function verifyWebhook(req: ExpressRequest, options?: VerifyWebhookOptions): Promise<any> { const webRequest = incomingMessageToRequest(req); // Cloning instead of implementing the body inside incomingMessageToRequest // to make it more predictable // we must pass in body as json string + let serializedBody: string; + try { + serializedBody = JSON.stringify(req.body); + } catch (error) { + throw new Error(`Failed to serialize request body: ${error instanceof Error ? error.message : 'Unknown error'}`); + } const clonedRequest = new Request(webRequest, { - body: JSON.stringify(req.body), + body: serializedBody, }); return verifyWebhookBase(clonedRequest, options); }
23-32: Add tests to cover the webhook verification changes.Since this is a bug fix that changes how the request body is handled, it's important to add tests to verify the behavior works correctly and prevent regressions.
Consider adding tests that cover:
- Normal webhook verification with JSON object body
- Webhook verification with string body
- Webhook verification with Buffer body
- Error handling for non-serializable bodies
- Edge cases like undefined/null bodies
Would you like me to help generate comprehensive test cases for this webhook verification function?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/express/src/webhooks.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
`**/*.{js,jsx,ts,tsx}`: All code must pass ESLint checks with the project's conf...
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Use Prettier for consistent code formatting
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Validate all inputs and sanitize outputs
Implement proper logging with different levels
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/express/src/webhooks.ts
`packages/**/*.ts`: TypeScript is required for all packages
packages/**/*.ts: TypeScript is required for all packages
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/express/src/webhooks.ts
`packages/**/*.{ts,tsx,d.ts}`: Packages should export TypeScript types alongside runtime code
packages/**/*.{ts,tsx,d.ts}: Packages should export TypeScript types alongside runtime code
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/express/src/webhooks.ts
`**/*.{ts,tsx}`: Use proper TypeScript error types
**/*.{ts,tsx}: Use proper TypeScript error types
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/express/src/webhooks.ts
`**/*.{ts,tsx}`: Always define explicit return types for functions, especially p...
**/*.{ts,tsx}: 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
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Document public functions and APIs with JSDoc-style comments including @param, @returns, @throws, and @example
Define custom error classes for domain-specific errors
Use the Result pattern for error handling instead of throwing exceptions
Use optional chaining and nullish coalescing for safe property access
Let TypeScript infer types when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use readonly arrays and objects for immutability
Use immutable update patterns (spread, etc.) for objects and arrays
Use lazy loading for large types
Preferunknownoveranyfor performance
Use type-only imports:import type { ... }
Use branded types for domain safety
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints in TypeScript generics
No unused type parameters in generics
Proper use of utility types instead of manual type construction
Type-only imports where possible for performance
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
📄 Source: CodeRabbit Inference Engine (.cursor/rules/typescript.mdc)
List of files the instruction was applied to:
packages/express/src/webhooks.ts
`**/*.ts`: If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
**/*.ts: If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
packages/express/src/webhooks.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: dstaley
PR: clerk/javascript#6116
File: .changeset/tangy-garlics-say.md:1-2
Timestamp: 2025-06-13T16:09:53.061Z
Learning: In the Clerk JavaScript repository, contributors create intentionally empty changeset files (containing only the YAML delimiters) when a PR touches only non-published parts of the codebase (e.g., sandbox assets). This signals that no package release is required, so such changesets should not be flagged as missing content.
Learnt from: wobsoriano
PR: clerk/javascript#6099
File: packages/backend/src/api/endpoints/IdPOAuthAccessTokenApi.ts:7-14
Timestamp: 2025-06-10T20:38:08.982Z
Learning: Methods in `packages/backend/src/api/endpoints` (e.g., `IdPOAuthAccessTokenApi.verifySecret`) are currently not exposed publicly, so renaming them does not constitute a breaking change.
Learnt from: CR
PR: clerk/javascript#0
File: .cursor/rules/development.mdc:0-0
Timestamp: 2025-06-30T10:29:42.997Z
Learning: Update documentation for API changes
🔇 Additional comments (2)
packages/express/src/webhooks.ts (2)
27-27: Good addition of clarifying comment.The comment helps explain why the body needs to be serialized as a JSON string, which improves code maintainability.
29-29: Please confirm JSON.stringify(req.body) preserves the exact raw payload for signature verificationSwitching from the raw IncomingMessage stream to a stringified, parsed object can alter whitespace or property order, which may break HMAC checks. We didn’t find any existing tests or references to issue #6283 in this package, so please:
• Add an integration test that:
– Uses a known Clerk webhook payload (raw JSON string) and signing secret
– Generates the Svix signature header
– Sends it through your Express route (with JSON body parsing + the currentverifyWebhookimplementation)
– Asserts thatverifyWebhook()succeeds• Manually verify in a real webhook flow that the stringified
req.bodymatches the original payload bytes used to compute the signature.
Description
Fixes #6283
check the issue for more description.
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit