fix(cache): invalidate both raw and hashed publishable keys and encrypt jwtSecret in Redis - #342
Conversation
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRedis project caching now encrypts ChangesRedis and API key security
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant verifyApiKey
participant Redis
participant Project
Client->>verifyApiKey: Send API key
verifyApiKey->>Redis: Read cached project
verifyApiKey->>Redis: Invalidate stale entry when jwtSecret is null
verifyApiKey->>Project: Query project
Project-->>verifyApiKey: Return project
verifyApiKey->>Redis: Cache project with encrypted jwtSecret
verifyApiKey-->>Client: Continue or return error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pr_description.md (1)
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd inline comments to the encryption and cache invalidation logic.
The PR checklist claims comments were added to hard-to-understand areas, but
redisCaching.jslacks explanatory comments for:
- Why
jwtSecretis encrypted before caching- The plaintext fallback mechanism in the get helpers
- Why both raw and hashed API keys are deleted
Consider adding brief inline comments to these security-sensitive code paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pr_description.md` at line 33, Add brief inline comments in redisCaching.js around the security-sensitive paths that the checklist refers to: explain why jwtSecret is encrypted before being cached, note the plaintext fallback behavior in the get helper methods, and clarify why both raw and hashed API keys are deleted during invalidation. Use the relevant helper functions in redisCaching.js so the comments stay attached to the exact logic that future readers will inspect.apps/public-api/src/__tests__/redisCaching.test.js (1)
49-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert cache writes don’t mutate the input project.
The production helpers clone before encrypting; add assertions so a future regression that encrypts the caller’s object is caught.
Proposed test addition
await setProjectByApiKeyCache('pk_live_123', project); + expect(project.jwtSecret).toBe('super-secret-jwt-key');await setProjectById('project_id_1', project); + expect(project.jwtSecret).toBe('id-secret');Also applies to: 90-90
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/public-api/src/__tests__/redisCaching.test.js` at line 49, The cache write test around setProjectByApiKeyCache should verify the input project object is not mutated by encryption or caching. Update the assertions in redisCaching.test.js to capture the original project state before calling setProjectByApiKeyCache and confirm the caller’s object remains unchanged afterward, matching the cloning behavior used by the production helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/common/src/redis/redisCaching.js`:
- Around line 46-48: The cached project reader in redisCaching.js is treating a
failed jwtSecret decrypt as a valid cached value; update the jwtSecret handling
in the relevant cache read paths (including the shared logic used around
parsedData.jwtSecret) so that when decrypt returns null the entry is treated as
a cache miss and the project is refetched instead of returning jwtSecret: null.
Make sure both affected reader locations use the same behavior and only return
cached data when jwtSecret decrypt succeeds.
---
Nitpick comments:
In `@apps/public-api/src/__tests__/redisCaching.test.js`:
- Line 49: The cache write test around setProjectByApiKeyCache should verify the
input project object is not mutated by encryption or caching. Update the
assertions in redisCaching.test.js to capture the original project state before
calling setProjectByApiKeyCache and confirm the caller’s object remains
unchanged afterward, matching the cloning behavior used by the production
helpers.
In `@pr_description.md`:
- Line 33: Add brief inline comments in redisCaching.js around the
security-sensitive paths that the checklist refers to: explain why jwtSecret is
encrypted before being cached, note the plaintext fallback behavior in the get
helper methods, and clarify why both raw and hashed API keys are deleted during
invalidation. Use the relevant helper functions in redisCaching.js so the
comments stay attached to the exact logic that future readers will inspect.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f33d64fe-fc14-4491-8d5f-ba4bdfe8e216
📒 Files selected for processing (3)
apps/public-api/src/__tests__/redisCaching.test.jspackages/common/src/redis/redisCaching.jspr_description.md
| if (parsedData && parsedData.jwtSecret && typeof parsedData.jwtSecret === "object") { | ||
| parsedData.jwtSecret = decrypt(parsedData.jwtSecret); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Treat failed jwtSecret decrypts as cache misses.
decrypt returns null on tamper/wrong-key failures, but these readers currently return a cached project with jwtSecret: null. That can break JWT validation until the Redis TTL expires instead of refetching a valid project.
Proposed fix
- if (parsedData && parsedData.jwtSecret && typeof parsedData.jwtSecret === "object") {
- parsedData.jwtSecret = decrypt(parsedData.jwtSecret);
- }
+ if (parsedData && parsedData.jwtSecret && typeof parsedData.jwtSecret === "object") {
+ const decryptedJwtSecret = decrypt(parsedData.jwtSecret);
+ if (decryptedJwtSecret === null) {
+ await redis.del(`project:apikey:${api}`);
+ return null;
+ }
+ parsedData.jwtSecret = decryptedJwtSecret;
+ }- if (parsedData && parsedData.jwtSecret && typeof parsedData.jwtSecret === "object") {
- parsedData.jwtSecret = decrypt(parsedData.jwtSecret);
- }
+ if (parsedData && parsedData.jwtSecret && typeof parsedData.jwtSecret === "object") {
+ const decryptedJwtSecret = decrypt(parsedData.jwtSecret);
+ if (decryptedJwtSecret === null) {
+ await redis.del(`project:id:${id}`);
+ return null;
+ }
+ parsedData.jwtSecret = decryptedJwtSecret;
+ }Also applies to: 112-114
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/common/src/redis/redisCaching.js` around lines 46 - 48, The cached
project reader in redisCaching.js is treating a failed jwtSecret decrypt as a
valid cached value; update the jwtSecret handling in the relevant cache read
paths (including the shared logic used around parsedData.jwtSecret) so that when
decrypt returns null the entry is treated as a cache miss and the project is
refetched instead of returning jwtSecret: null. Make sure both affected reader
locations use the same behavior and only return cached data when jwtSecret
decrypt succeeds.
|
please fix coderabbits comments |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Hey @yash-pouranik, |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/public-api/src/middlewares/verifyApiKey.js`:
- Around line 102-110: The cache write in verifyApiKey is happening before the
owner verification check, which can persist an unverified project state in
Redis. Move the setProjectByApiKeyCache call in verifyApiKey.js to after the
!project.owner.isVerified guard so only verified projects are cached, and keep
the existing cacheKey selection logic intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f2f7611-c8de-4f02-99d5-9c61d96ed874
📒 Files selected for processing (1)
apps/public-api/src/middlewares/verifyApiKey.js
|
@Kirtan-pc |
|
@Kirtan-pc |
|
Hey @yash-pouranik, |
|
Hey @yash-pouranik, |
Handle decryption failure for jwtSecret in cache.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/common/src/redis/redisCaching.js (1)
47-53: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDelete the actual cache key after decryption failure.
Line 49 references undeclared
cacheKey, so cleanup throws and the corrupted entry remains cached.Proposed fix
- await redis.del(cacheKey); + await redis.del(`project:apikey:${api}`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/common/src/redis/redisCaching.js` around lines 47 - 53, Replace the undeclared cacheKey reference in the decryption-failure branch of the relevant Redis cache retrieval function with the actual cache-key variable used when reading the entry, then delete that key before returning null.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/common/src/redis/redisCaching.js`:
- Around line 47-53: Replace the undeclared cacheKey reference in the
decryption-failure branch of the relevant Redis cache retrieval function with
the actual cache-key variable used when reading the entry, then delete that key
before returning null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84d2c29a-af99-4165-9857-cb9d380329a7
📒 Files selected for processing (1)
packages/common/src/redis/redisCaching.js
yash-pouranik
added
level3
|
Done |
3 participants
🚀 Pull Request Description
Fixes #286
This PR resolves two security and consistency issues related to project caching in Redis:
deleteProjectByApiKeyCache. When updatingallowedDomainsin the dashboard, the cache was not properly invalidated for publishable keys because the key was stored under the hashed value of the key (sinceverifyApiKey.jsdid not selectpublishableKeyfrom the DB, resulting in it checkingundefined === apiKeyand caching it under thehashedApi).deleteProjectByApiKeyCachenow deletes both raw and hashed versions of keys to guarantee proper invalidation.jwtSecretfield prior to writing to the Redis cache and decrypts it when retrieving. A graceful fallback is included to support reading existing plaintext entries without breaking active sessions.🛠️ Type of Change
🧪 Testing & Validation
Backend Verification:
npm testin thebackend/directory and all tests passed.Frontend Verification:
npm run lintin thefrontend/directory.📸 Screenshots / Recordings (Optional)
✅ Checklist
Built with ❤️ for urBackend.
Summary by CodeRabbit
Security Enhancements
Bug Fixes