Switch to asymmetric JWT auth for v2 endpoints #147
Conversation
|
Claude finished @lourou's task —— View job Code Review Complete ✅I've completed a thorough review of this PR that migrates JWT authentication from symmetric HS256 to asymmetric ES256 (ECDSA P-256). Overall, this is a well-executed security improvement with proper key caching and validation. Here's my feedback: 🔒 Security Considerations✅ Strengths:
⚡ Performance✅ Major Improvement (addressed in latest commit):
📊 Performance characteristics:
🏗️ Code Quality & Best Practices✅ Strengths:
🐛 Potential Issues🔴 Critical: 🟡 Minor:
🧪 Test Coverage❌ Major Gap:
Recommendation: describe('v2 JWT', () => {
describe('createV2JwtToken', () => {
it('creates valid ES256 token with deviceId')
it('includes metadata when provided')
it('rejects metadata exceeding size limit')
it('sets correct issuer and subject claims')
});
describe('verifyV2JwtToken', () => {
it('verifies valid token successfully')
it('rejects token with wrong issuer')
it('rejects expired token')
it('rejects token signed with wrong key')
it('rejects malformed token')
});
describe('validateJWTKeys', () => {
it('validates correct PEM keys')
it('rejects invalid private key format')
it('rejects invalid public key format')
});
});✅ Backward CompatibilityExcellent work here:
📋 Summary & RecommendationsThis PR is production-ready with the following minor improvements suggested:
What's working great:
Approval recommendation: ✅ Approve with optional follow-ups The core implementation is solid and secure. The suggested improvements (error handling consistency, tests, documentation) can be addressed in follow-up PRs and don't block this security enhancement. |
|
Warning Rate limit exceeded@lourou has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 29 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (3)
WalkthroughAdds asymmetric JWT v2 (ES256): new ECDSA key-generation script and README updates; config now validates/exports PEM private/public keys and issuer; JWT utils import/validate keys, sign with ES256 and verify enforcing issuer; startup validates keys before launching the server. Changes
Sequence DiagramsequenceDiagram
autonumber
rect rgb(245,250,255)
Note over Dev,Backend: ES256 JWT key generation & usage
end
Dev->>Backend: run `dev/scripts/generateEcdsaKeys.ts`
Backend->>Backend: store `JWT_PRIVATE_KEY` in .env
Backend->>Backend: validateJWTKeys() — import & validate PEMs
Backend->>Backend: createToken(deviceId) with sub, iss
Backend->>Backend: sign token using ES256 (private PEM)
Backend->>Gateway: send JWT
Gateway->>Gateway: import `JWT_PUBLIC_KEY`
Gateway->>Gateway: verify JWT (ES256, iss="convos.org")
alt verification success
Gateway->>Gateway: extract sub → deviceId
else verification failure
Gateway->>Gateway: log & reject request
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Switch v2 JWT on backend and gateway to ES256 with asymmetric keys for token signing and verificationIntroduce ES256 signing and verification for v2 JWTs using 📍Where to StartStart with Macroscope summarized 18d8caa. |
![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)
src/utils/v2/jwt.ts (2)
54-81: Consider caching the imported private key at module level.The private key is imported on every call to
createV2JwtToken, which involves parsing the PEM string repeatedly. For better performance, import and cache the key once at module initialization.Apply this pattern:
+// Import and cache keys at module initialization +let cachedPrivateKey: jose.KeyLike | null = null; + +async function getPrivateKey() { + if (!cachedPrivateKey) { + const { data, error } = await tryCatch( + jose.importPKCS8(JWT_PRIVATE_KEY, "ES256"), + ); + if (error) { + logger.error("Failed to import JWT private key", error); + throw new AppError(500, "Failed to import JWT private key", error); + } + cachedPrivateKey = data; + } + return cachedPrivateKey; +} + export const createV2JwtToken = async (args: { deviceId: string; metadata?: V2JWTMetadata; expirationTime?: string; }) => { // ... validation code ... - // Import ECDSA private key - const { data: privateKey, error: importError } = await tryCatch( - jose.importPKCS8(JWT_PRIVATE_KEY, "ES256"), - ); - - if (importError) { - logger.error("Failed to import JWT private key", importError); - throw new AppError(500, "Failed to import JWT private key", importError); - } + const privateKey = await getPrivateKey(); // Create JWT token with ECDSA ES256 const { data: jwt, error: jwtError } = await tryCatch(
84-116: Consider caching the imported public key at module level.The public key is imported on every call to
verifyV2JwtToken. Since this function is called on every authenticated request, the repeated key import could significantly impact performance.Apply this pattern:
+let cachedPublicKey: jose.KeyLike | null = null; + +async function getPublicKey() { + if (!cachedPublicKey) { + const { data, error } = await tryCatch( + jose.importSPKI(JWT_PUBLIC_KEY, "ES256"), + ); + if (error) { + logger.error("Failed to import JWT public key for verification", error); + throw new AppError(500, "Failed to import JWT public key", error); + } + cachedPublicKey = data; + } + return cachedPublicKey; +} + export const verifyV2JwtToken = async (args: { token: string }) => { - // Import ECDSA public key for verification - const { data: publicKey, error: importError } = await tryCatch( - jose.importSPKI(JWT_PUBLIC_KEY, "ES256"), - ); - - if (importError) { - logger.error( - "Failed to import JWT public key for verification", - importError, - ); - throw new AppError(500, "Failed to import JWT public key", importError); - } + const publicKey = await getPublicKey(); // Verify JWT token using the public key
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(2 hunks)dev/scripts/generateEcdsaKeys.ts(1 hunks)src/config.ts(2 hunks)src/utils/v2/jwt.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*
📄 CodeRabbit inference engine (.cursorrules)
Follow consistent file naming (kebab-case)
Files:
README.mdsrc/utils/v2/jwt.tsdev/scripts/generateEcdsaKeys.tssrc/config.ts
**/README.md
📄 CodeRabbit inference engine (.cursorrules)
**/README.md: Document environment setup
Keep README up to date
Document API endpoints
Include example usage
Files:
README.md
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: TypeScript with strict mode and ESNext features
Implement consistent error handling using AppError class
Use object parameter syntax for function arguments: '(args: { paramOne: string }) => {}' instead of '(paramOne: string) => {}' for better readability and extensibility
Keep functions focused and avoid optional parameters. If a function has many optional parameters, consider splitting it into multiple specialized functions for clearer intent and better maintainability
Prefer type inference from Prisma models
Keep types close to their usage (same directory)
Use type exports for shared types
Leverage path aliases (@/) for imports
Don't specify return type on functions. Prefer inferring the value.
Use transactions for multi-table operations
Include necessary relations in queries using include
Use custom AppError class for application errors
Log errors with context using logError utility
Log security-relevant events
Implement caching where needed
Optimize database queries
Use appropriate batch operations
Add JSDoc comments for public APIs
Files:
src/utils/v2/jwt.tsdev/scripts/generateEcdsaKeys.tssrc/config.ts
🧠 Learnings (1)
📚 Learning: 2025-09-04T14:37:41.542Z
Learnt from: nmalzieu
PR: ephemeraHQ/convos-backend#133
File: src/api/v1/devices/handlers/get-device.handler.ts:15-16
Timestamp: 2025-09-04T14:37:41.542Z
Learning: For authenticated endpoints in the convos-backend Express.js application, res.locals.xmtpId is guaranteed to be present when handlers execute because the auth middleware validates JWT tokens and returns 401 before reaching handlers if authentication fails. Additional guards for res.locals.xmtpId are not needed in authenticated route handlers.
Applied to files:
src/config.ts
🧬 Code graph analysis (1)
src/utils/v2/jwt.ts (3)
src/utils/try-catch.ts (1)
tryCatch(14-23)src/config.ts (3)
JWT_PRIVATE_KEY(69-69)JWT_ISSUER(72-72)JWT_PUBLIC_KEY(70-70)src/utils/errors.ts (1)
AppError(1-11)
🪛 GitHub Check: Typecheck, format check, lint
src/config.ts
[failure] 19-19: error: JWT_PRIVATE_KEY is not configured
at /home/runner/work/convos-backend/convos-backend/src/config.ts:19:9
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: claude-review
- GitHub Check: push_to_registry
- GitHub Check: Review for correctness
🔇 Additional comments (3)
dev/scripts/generateEcdsaKeys.ts (1)
1-57: LGTM! Key generation script is well-structured.The script correctly generates ES256 key pairs, exports them in the proper PEM formats (PKCS#8 for private, SPKI for public), and provides clear output with security warnings.
README.md (1)
38-44: LGTM! Clear documentation for key generation.The instructions provide a clear workflow for generating and distributing the JWT keys.
src/config.ts (1)
18-70: LGTM! Thorough validation with clear error messages.The validation logic properly checks for the presence and non-emptiness of the JWT keys, with descriptive error messages that guide developers on what's needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/index.ts (1)
83-85: Log startup validation failures with structured error context.Line 84 currently passes the
erroras a second positional argument, so pino treats it as string interpolation and we lose the structured stack trace in logs. Wrap the error in an object to keep the context and comply with our logging conventions.- logger.error("Failed to validate JWT keys at startup", error); + logger.error({ error }, "Failed to validate JWT keys at startup");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.env.example(1 hunks)src/index.ts(2 hunks)src/utils/v2/jwt.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- .env.example
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: TypeScript with strict mode and ESNext features
Implement consistent error handling using AppError class
Use object parameter syntax for function arguments: '(args: { paramOne: string }) => {}' instead of '(paramOne: string) => {}' for better readability and extensibility
Keep functions focused and avoid optional parameters. If a function has many optional parameters, consider splitting it into multiple specialized functions for clearer intent and better maintainability
Prefer type inference from Prisma models
Keep types close to their usage (same directory)
Use type exports for shared types
Leverage path aliases (@/) for imports
Don't specify return type on functions. Prefer inferring the value.
Use transactions for multi-table operations
Include necessary relations in queries using include
Use custom AppError class for application errors
Log errors with context using logError utility
Log security-relevant events
Implement caching where needed
Optimize database queries
Use appropriate batch operations
Add JSDoc comments for public APIs
Files:
src/utils/v2/jwt.tssrc/index.ts
**/*
📄 CodeRabbit inference engine (.cursorrules)
Follow consistent file naming (kebab-case)
Files:
src/utils/v2/jwt.tssrc/index.ts
**/index.ts
📄 CodeRabbit inference engine (.cursorrules)
Use index.ts files for exports
Files:
src/index.ts
🧠 Learnings (1)
📚 Learning: 2025-08-05T14:39:39.983Z
Learnt from: CR
PR: ephemeraHQ/convos-backend#0
File: .cursorrules:0-0
Timestamp: 2025-08-05T14:39:39.983Z
Learning: Applies to **/{middleware,api,routes}/**/*.{ts,tsx} : Implement rate limiting
Applied to files:
src/index.ts
🧬 Code graph analysis (2)
src/utils/v2/jwt.ts (3)
src/config.ts (3)
JWT_PRIVATE_KEY(69-69)JWT_PUBLIC_KEY(70-70)JWT_ISSUER(72-72)src/utils/try-catch.ts (1)
tryCatch(14-23)src/utils/errors.ts (1)
AppError(1-11)
src/index.ts (2)
src/utils/v2/jwt.ts (1)
validateJWTKeys(31-53)src/config.ts (1)
IS_DEVELOPMENT(76-76)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: claude-review
- GitHub Check: Review for correctness
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/config.ts (3)
9-15: Replace plainErrorwithAppErrorfor consistent error handling.These lines throw plain
Errorinstances, which violates the coding guideline requiring theAppErrorclass for application errors. As previously noted, all configuration validation errors should useAppError.Apply this diff:
+import { AppError } from "./utils/errors"; + // Validate required environment variables if (!process.env.XMTP_NOTIFICATION_SECRET) { - throw new Error("XMTP_NOTIFICATION_SECRET is not configured"); + throw new AppError(500, "XMTP_NOTIFICATION_SECRET is not configured"); } if (!process.env.NOTIFICATION_SERVER_URL) { - throw new Error("NOTIFICATION_SERVER_URL is not configured"); + throw new AppError(500, "NOTIFICATION_SERVER_URL is not configured"); }Based on coding guidelines.
32-39: Replace plainErrorwithAppErrorfor JWT private key validation.Line 36 throws a plain
Errorinstance, which violates the coding guideline. All configuration validation should use theAppErrorclass.Apply this diff:
if ( typeof JWT_PRIVATE_KEY_RAW !== "string" || JWT_PRIVATE_KEY_RAW.trim().length === 0 ) { - throw new Error( + throw new AppError( + 500, "Missing `JWT_PRIVATE_KEY`: set a non-empty PEM-encoded ECDSA private key in environment before starting the app", ); }Based on coding guidelines.
42-49: Replace plainErrorwithAppErrorfor JWT public key validation.Line 46 throws a plain
Errorinstance, which violates the coding guideline requiring consistent error handling via theAppErrorclass.Apply this diff:
if ( typeof JWT_PUBLIC_KEY_RAW !== "string" || JWT_PUBLIC_KEY_RAW.trim().length === 0 ) { - throw new Error( + throw new AppError( + 500, "Missing `JWT_PUBLIC_KEY`: set a non-empty PEM-encoded ECDSA public key in environment before starting the app", ); }Based on coding guidelines.
🧹 Nitpick comments (1)
src/config.ts (1)
32-49: Consider extracting validation logic to reduce duplication.The validation logic for
JWT_PRIVATE_KEYandJWT_PUBLIC_KEYis nearly identical. While this duplication is minor at module load time, extracting it to a helper could improve maintainability.Example helper:
function validatePEMKey(key: string | undefined, keyName: string): string { if (typeof key !== "string" || key.trim().length === 0) { throw new AppError( 500, `Missing \`${keyName}\`: set a non-empty PEM-encoded ECDSA ${keyName.toLowerCase().includes('private') ? 'private' : 'public'} key in environment before starting the app`, ); } return key; } // Usage: export const JWT_PRIVATE_KEY = validatePEMKey(process.env.JWT_PRIVATE_KEY, "JWT_PRIVATE_KEY"); export const JWT_PUBLIC_KEY = validatePEMKey(process.env.JWT_PUBLIC_KEY, "JWT_PUBLIC_KEY");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/config.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: TypeScript with strict mode and ESNext features
Implement consistent error handling using AppError class
Use object parameter syntax for function arguments: '(args: { paramOne: string }) => {}' instead of '(paramOne: string) => {}' for better readability and extensibility
Keep functions focused and avoid optional parameters. If a function has many optional parameters, consider splitting it into multiple specialized functions for clearer intent and better maintainability
Prefer type inference from Prisma models
Keep types close to their usage (same directory)
Use type exports for shared types
Leverage path aliases (@/) for imports
Don't specify return type on functions. Prefer inferring the value.
Use transactions for multi-table operations
Include necessary relations in queries using include
Use custom AppError class for application errors
Log errors with context using logError utility
Log security-relevant events
Implement caching where needed
Optimize database queries
Use appropriate batch operations
Add JSDoc comments for public APIs
Files:
src/config.ts
**/*
📄 CodeRabbit inference engine (.cursorrules)
Follow consistent file naming (kebab-case)
Files:
src/config.ts
🧠 Learnings (1)
📚 Learning: 2025-09-04T14:37:41.542Z
Learnt from: nmalzieu
PR: ephemeraHQ/convos-backend#133
File: src/api/v1/devices/handlers/get-device.handler.ts:15-16
Timestamp: 2025-09-04T14:37:41.542Z
Learning: For authenticated endpoints in the convos-backend Express.js application, res.locals.xmtpId is guaranteed to be present when handlers execute because the auth middleware validates JWT tokens and returns 401 before reaching handlers if authentication fails. Additional guards for res.locals.xmtpId are not needed in authenticated route handlers.
Applied to files:
src/config.ts
🪛 GitHub Check: Typecheck, format check, lint
src/config.ts
[failure] 36-36: error: Missing JWT_PRIVATE_KEY: set a non-empty PEM-encoded ECDSA private key in environment before starting the app
at /home/runner/work/convos-backend/convos-backend/src/config.ts:36:9
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: push_to_registry
- GitHub Check: claude-review
🔇 Additional comments (3)
src/config.ts (3)
20-25: Good backward compatibility approach.The optional v1 JWT handling is well-structured. Making
JWT_SECREToptional while conditionally derivingJWT_SECRET_BYTESproperly maintains backward compatibility for any existing v1 endpoints without forcing migration.
51-55: Clean export pattern after validation.The type assertions to
stringare safe here since the validation above ensures these values are non-empty strings. The hardcoded issuer is appropriate for this stage.
27-49: JWT key validation configuration is properly set up—no action required.Verification confirms both requirements are met:
.env.exampleincludesJWT_PRIVATE_KEYandJWT_PUBLIC_KEYplaceholdersvalidateJWTKeys()is imported and invoked at startup (src/index.ts, line 60)The two-layer validation approach is correct: basic non-empty string checks in src/config.ts, with deeper PEM format validation performed at application startup via
validateJWTKeys().
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| export const validateJWTKeys = async () => { | ||
| try { | ||
| // Validate and cache private key | ||
| await loadPrivateKey(); |
There was a problem hiding this comment.
validateJWTKeys never checks that the public key matches the private key. This can pass startup yet cause every verifyV2JwtToken to fail (401). Consider signing a throwaway JWT with the private key and verifying it with the public key during startup to fail fast on mismatched pairs.
- await loadPrivateKey();
+ const privateKey = await loadPrivateKey();
@@
- await loadPublicKey();
+ const publicKey = await loadPublicKey();
+ try {
+ const testJwt = await new jose.SignJWT({ k: "chk" })
+ .setProtectedHeader({ alg: "ES256" })
+ .setIssuer(JWT_ISSUER)
+ .setIssuedAt()
+ .setExpirationTime("1m")
+ .sign(privateKey);
+ await jose.jwtVerify(testJwt, publicKey, { issuer: JWT_ISSUER, algorithms: ["ES256"] });
+ logger.info("JWT key pair match validation successful");
+ } catch (error) {
+ logger.error({ error }, "JWT key pair validation failed (mismatch)");
+ throw new Error("JWT_PRIVATE_KEY and JWT_PUBLIC_KEY do not form a valid pair");
+ }
🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
This is done to support JWT verification on the payer gateway using a public key
Summary by CodeRabbit
Documentation
New Features
Behavior