fix(cache): invalidate both raw and hashed publishable keys and encrypt jwtSecret in Redis#342
Open
Kirtan-pc wants to merge 3 commits into
Open
Conversation
added 2 commits
June 29, 2026 15:08
Kirtan-pc
requested review from
Nitin-kumar-yadav1307 and
yash-pouranik
as code owners
Contributor
📝 WalkthroughWalkthroughRedis caching now encrypts ChangesRedis Cache Security Fixes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 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}
Contributor
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
Comment on lines
+46
to
+48
| if (parsedData && parsedData.jwtSecret && typeof parsedData.jwtSecret === "object") { | ||
| parsedData.jwtSecret = decrypt(parsedData.jwtSecret); | ||
| } |
Contributor
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.
Member
|
please fix coderabbits comments |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // a publishable key may be supplied via the `key` query parameter instead. | ||
| const headerKey = req.header("x-api-key"); | ||
| const queryKey = | ||
| typeof req.query?.key === "string" ? req.query.key : undefined; |
Contributor
Author
|
Hey @yash-pouranik, |
Contributor
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
Comment on lines
+102
to
+110
| const cacheKey = isSecret | ||
| ? hashedApi | ||
| : project[keyField] === apiKey | ||
| ? apiKey | ||
| : hashedApi; | ||
| await setProjectByApiKeyCache(cacheKey, project); | ||
| } | ||
|
|
||
| if (!project) { | ||
| return next(new AppError(401, 'Please use a valid API key or regenerate a new one from the dashboard.', 'API key is expired or invalid.')); | ||
| } | ||
| if (!project.owner.isVerified) { |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Move the cache write after owner verification.
Line 107 caches the project before Line 110 rejects unverified owners, so a request made before verification can store owner.isVerified: false and keep rejecting from Redis until the cache expires. Defer setProjectByApiKeyCache until after the owner check.
Proposed fix
+ let cacheKey;
+
if (!project) {
@@
- const cacheKey = isSecret
+ cacheKey = isSecret
? hashedApi
: project[keyField] === apiKey
? apiKey
: hashedApi;
- await setProjectByApiKeyCache(cacheKey, project);
}
- if (!project.owner.isVerified) {
+ if (!project.owner?.isVerified) {
return next(
new AppError(
401,
@@
);
}
+
+ if (cacheKey) {
+ await setProjectByApiKeyCache(cacheKey, project);
+ }📝 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.
Suggested change
| const cacheKey = isSecret | |
| ? hashedApi | |
| : project[keyField] === apiKey | |
| ? apiKey | |
| : hashedApi; | |
| await setProjectByApiKeyCache(cacheKey, project); | |
| } | |
| if (!project) { | |
| return next(new AppError(401, 'Please use a valid API key or regenerate a new one from the dashboard.', 'API key is expired or invalid.')); | |
| } | |
| if (!project.owner.isVerified) { | |
| let cacheKey; | |
| cacheKey = isSecret | |
| ? hashedApi | |
| : project[keyField] === apiKey | |
| ? apiKey | |
| : hashedApi; | |
| } | |
| if (!project.owner?.isVerified) { | |
| return next( | |
| new AppError( | |
| 401, | |
| ... | |
| ) | |
| ); | |
| } | |
| if (cacheKey) { | |
| await setProjectByApiKeyCache(cacheKey, project); | |
| } |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 106-106: Avoid using the initial state variable in setState
Context: setProjectByApiKeyCache(cacheKey, project)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🤖 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/middlewares/verifyApiKey.js` around lines 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🚀 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
Bug Fixes
Tests