Repository navigation
fix: Saving a role clears the entire cache instead of only the cached roles - #10618
AdrianCurtin wants to merge 2 commits into
Conversation
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
Bot
changed the title
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughCache clearing now accepts application and subcache prefixes. Cache adapters remove matching keys when given a prefix, while unscoped clearing retains full-cache behavior. Tests cover prefix isolation and role-cache invalidation without removing cached users. ChangesScoped cache clearing
Priority: ⬆️ High Estimated code review effort: 3 (Moderate) | ~20 minutes Severity of issue fixed: High Suggested reviewers: Merge Risk: ⚪ Minimal · up to Scoped cache clearing preserves unrelated entries in the supplied coverage, and no merge-blocking issue remains. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Scoped clearing substantially reduces accidental deletion across applications and unrelated Redis data. However, Redis clearing becomes a multi-step operation that role writes do not wait for, potentially extending the period in which revoked roles remain usable. The practical exposure depends on cache size, concurrency, expiration settings, and Redis failures. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes a cache-clearing security/availability issue where saving a _Role could clear the entire cache backend (including unrelated keys), by introducing scoped cache clearing via optional key-prefix support across the cache controller and adapters.
Changes:
- Extend the cache adapter contract so
clear(prefix)can optionally clear only keys under<prefix>:*, while preserving legacy “clear everything” behavior when called without a prefix. - Fix
SubCache#clear()/CacheController#clear()to pass the appropriate scope (<appId>:<subcache>), preventing role saves from flushing unrelated caches. - Implement scoped clearing for Redis (SCAN + UNLINK) and in-memory caches, and add regression/unit tests for scoped behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Controllers/CacheController.js | Passes sub-cache/app scopes into clear(prefix) to avoid unscoped adapter clears. |
| src/Adapters/Cache/RedisCacheAdapter.js | Adds scoped Redis clearing using SCAN + UNLINK, retaining FLUSHDB for unscoped clears. |
| src/Adapters/Cache/LRUCache.js | Adds prefix-scoped key removal for the in-memory LRU store. |
| src/Adapters/Cache/InMemoryCacheAdapter.js | Threads clear(prefix) through to the underlying in-memory cache implementation. |
| src/Adapters/Cache/CacheAdapter.js | Documents and exposes optional clear(prefix) parameter in the adapter contract. |
| spec/RedisCacheAdapter.spec.js | Adds tests ensuring scoped clear only removes matching prefixes and treats glob chars literally. |
| spec/InMemoryCacheAdapter.spec.js | Adds tests ensuring scoped clear only removes matching prefixes for in-memory adapter. |
| spec/CacheController.spec.js | Adds tests verifying controller/sub-cache scoping and regression test for _Role save not evicting cached users. |
Suppressed comments (1)
src/Controllers/CacheController.js:74
- The JSDoc for
CacheController#clear()says other Parse apps' keys are left untouched, but that depends on the adapter honoring the optionalprefixparameter. For adapters that ignore the parameter, this may still clear the whole cache backend.
/**
* Empty this app's cache. Keys belonging to other Parse apps sharing the
* same cache backend are left untouched.
*
* @param {String} prefix Optional sub-cache prefix to narrow the scope
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Adapters/Cache/RedisCacheAdapter.js (1)
92-113: 🩺 Stability & Availability | 🔵 TrivialScoped clear implementation is correct.
The SCAN cursor handling, glob escaping, and colon-boundary scoping are correct and match documented
node-redisv5 behavior. One point worth noting for future maintainers: SCAN provides only an eventually-consistent view of the keyspace. A key written to the target scope after its hash slot is already scanned can survive this clear. This is an accepted trade-off of SCAN+UNLINK over blockingKEYS+DEL, not a defect, but it means a_Rolesave that races with a fresh cache write to the same role key has a narrow window where the stale entry is not removed.🤖 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 `@src/Adapters/Cache/RedisCacheAdapter.js` around lines 92 - 113, No code change is required; retain the current scoped clear implementation in clear, including SCAN cursor handling, glob escaping, colon-boundary matching, and UNLINK behavior, while accepting the documented race window for keys written during the scan.spec/CacheController.spec.js (1)
79-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the fixed 200ms sleep with a poll-based wait.
The test waits a fixed 200ms for RestWrite's un-awaited role-cache clear to complete. A fixed sleep is a known source of CI flakiness: too short under load and the assertion fails spuriously, too long and the test wastes time. Poll
cacheController.role.get('someUser')in a short loop until it resolves tonullor a bounded timeout elapses, instead of a single fixed delay.♻️ Proposed polling-based wait
- await new Parse.Role('Admin', new Parse.ACL()).save(null, { useMasterKey: true }); - // The role cache is cleared without being awaited by RestWrite. - await new Promise(resolve => setTimeout(resolve, 200)); - - expect(await cacheController.role.get('someUser')).toEqual(null); + await new Parse.Role('Admin', new Parse.ACL()).save(null, { useMasterKey: true }); + // The role cache is cleared without being awaited by RestWrite; poll instead of a fixed sleep. + const deadline = Date.now() + 2000; + let roleCacheEntry; + do { + roleCacheEntry = await cacheController.role.get('someUser'); + } while (roleCacheEntry !== null && Date.now() < deadline); + + expect(roleCacheEntry).toEqual(null);🤖 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 `@spec/CacheController.spec.js` around lines 79 - 92, Replace the fixed 200ms timeout in the “should not evict cached users when a _Role is saved” test with bounded polling of cacheController.role.get('someUser'). Repeatedly check at a short interval until the value is null, while enforcing a maximum timeout so the test cannot wait indefinitely, then retain the existing role and user cache assertions.
🤖 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.
Nitpick comments:
In `@spec/CacheController.spec.js`:
- Around line 79-92: Replace the fixed 200ms timeout in the “should not evict
cached users when a _Role is saved” test with bounded polling of
cacheController.role.get('someUser'). Repeatedly check at a short interval until
the value is null, while enforcing a maximum timeout so the test cannot wait
indefinitely, then retain the existing role and user cache assertions.
In `@src/Adapters/Cache/RedisCacheAdapter.js`:
- Around line 92-113: No code change is required; retain the current scoped
clear implementation in clear, including SCAN cursor handling, glob escaping,
colon-boundary matching, and UNLINK behavior, while accepting the documented
race window for keys written during the scan.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 89cbb037-46b2-47e0-b4ab-f1a90dfef304
📒 Files selected for processing (8)
spec/CacheController.spec.jsspec/InMemoryCacheAdapter.spec.jsspec/RedisCacheAdapter.spec.jssrc/Adapters/Cache/CacheAdapter.jssrc/Adapters/Cache/InMemoryCacheAdapter.jssrc/Adapters/Cache/LRUCache.jssrc/Adapters/Cache/RedisCacheAdapter.jssrc/Controllers/CacheController.js
607bb56 to
4ed9f09
Compare
Pull Request
Issue
Closes #10617.
SubCache#clear()dropped the key prefix that every other method on the class applies, socacheController.role.clear(), fired on every_Rolewrite, reached the adapter as an unscoped clear. OnRedisCacheAdapterthat is a rawFLUSHDB, which empties the whole Redis database.The consequences, in order of how many deployments they touch:
_Rolewrite evicts all cached entries for every app on that server, including the<appId>:user:<sessionToken>entries backing session authentication. Apps that create a role per organization, workspace or team at signup destroy their own cache continuously._Rolewrite deletes that data too._Rolewrites, any client can triggerFLUSHDBat will.Approach
Scoped clearing is added as an optional part of the
CacheAdaptercontract, so third-party adapters keep working unchanged.CacheAdapter#clear(prefix)documents the new optional parameter. An adapter that ignores it empties the whole cache, which is exactly today's behavior.SubCache#clear()passes its prefix, sorole.clear()resolves to<appId>:role.CacheController#clear(prefix)joins the app id, so it resolves to<appId>or<appId>:<prefix>.RedisCacheAdapter#clear(prefix)iteratesSCANwithMATCH <prefix>:*and removes matches withUNLINK, glob-escaping the prefix so an app id containing*,?,[or]cannot widen the pattern. It keeps the existingKeyPromiseQueuebarrier. Called with no prefix it still issuesFLUSHDB.InMemoryCacheAdapterandLRUCachefilter their own keys by prefix.One scope limit worth stating: the in-memory adapter is per-process by construction, so on a multi-node deployment a clear still only evicts the node that handled the write. This PR narrows what a clear destroys, not how far it reaches, and Redis, being shared, is invalidated for the whole cluster.
Worth flagging for review:
CacheController#clear()with no arguments now scopes to the app id rather than emptying the adapter outright. Nothing in the codebase relied on the wider behavior, andTestUtilscallscacheAdapter.clear()on the adapter directly, so test teardown still performs a full flush.SCANis only reached on role writes and purges, which are rare, and it usesUNLINKso the deletion itself does not block the server.This continues the direction of #3523, which narrowed the adapter from
FLUSHALLtoFLUSHDB. Narrowing once more, to the key prefix, drops the remaining assumption that Parse Server exclusively owns the Redis database.Tests
spec/CacheController.spec.jsasserts each sub-cache clears under its own<appId>:<prefix>scope, and adds an end-to-end regression test that a_Rolesave leaves a cached user entry intact. That test fails onalphawithExpected null to equal Object({ objectId: 'someUser' })and passes here. It runs against the in-memory adapter by default and against Redis underPARSE_SERVER_TEST_CACHE=redis.spec/RedisCacheAdapter.spec.jsasserts a scoped clear removes only its own prefix while leaving another app's keys and an unrelatedqueue:defaultkey in place, that an unscoped clear still empties everything, and that glob characters in a prefix are treated literally.spec/InMemoryCacheAdapter.spec.jscovers the same scoping for the default adapter.Tasks
Summary by CodeRabbit
New Features
Bug Fixes
Tests