Fix cache security hardening#101
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements multiple security hardening measures: HTTP middleware (Express/Hono) now bypasses implicit caching for sensitive query parameters; ChangesSecurity Hardening, Observability, and Playground Refactor
Sequence Diagram(s)sequenceDiagram
participant PlaygroundClient
participant runPlaygroundInSandbox
participant Iframe
participant BlobWorker
participant MockCacheStack
PlaygroundClient->>runPlaygroundInSandbox: run(code, onMessage, onError)
runPlaygroundInSandbox->>Iframe: append hidden iframe (allow-scripts sandbox, CSP srcdoc)
Iframe-->>runPlaygroundInSandbox: load event
runPlaygroundInSandbox->>Iframe: postMessage({code, runId, messageToken, workerSource})
Iframe->>BlobWorker: new Worker(Blob from workerSource)
BlobWorker->>MockCacheStack: execute user code in sandboxed globals
BlobWorker-->>Iframe: postMessage log/done (token-filtered)
Iframe-->>runPlaygroundInSandbox: forward to parent (runId-filtered)
runPlaygroundInSandbox-->>PlaygroundClient: onMessage(log/done) → update UI
PlaygroundClient->>runPlaygroundInSandbox: stop()
runPlaygroundInSandbox->>Iframe: remove from DOM, unlisten
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🤖 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 `@docs-web/content/docs/migration.mdx`:
- Around line 34-36: The "Implicit HTTP cache keys" section in the migration
guide references "common sensitive query parameters" without providing the
complete enumeration or linking to where they are documented. Add either a
reference link to the integrations.mdx Express middleware Options section where
the full sensitive parameter list is specified (access_token, api_key, apikey,
auth, authorization, code, credentials, id_token, jwt, password, private_key,
refresh_token, secret, session, sessionid, session_id, token), or include the
complete list directly in this migration section to ensure users can understand
the full scope of parameters being handled without needing to consult additional
documentation.
In `@src/integrations/httpCacheKeys.ts`:
- Around line 44-48: The catch block that handles URL parsing failures currently
returns hasSensitiveQuery: false, which allows implicit caching when URL
inspection fails and creates a security risk. Change the return value of
hasSensitiveQuery from false to true in the error handler so that requests where
URL parsing fails are treated as sensitive by default, ensuring the middleware
bypasses implicit caching as a fail-closed security measure.
🪄 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: 4706502d-ef4a-4e7f-ad60-df4d36002ddb
📒 Files selected for processing (46)
CHANGELOG.mdREADME.mdSECURITY.mddocs-web/components/playground/PlaygroundClient.tsxdocs-web/content/docs/api.mdxdocs-web/content/docs/cli.mdxdocs-web/content/docs/distributed.mdxdocs-web/content/docs/index.mdxdocs-web/content/docs/integrations.mdxdocs-web/content/docs/layers.mdxdocs-web/content/docs/migration.mdxdocs-web/content/docs/observability.mdxdocs-web/content/index.mdxdocs-web/content/playground.mdxdocs-web/lib/playground/worker-sandbox.tsdocs-web/lib/playground/worker.tsdocs-web/tests/current-docs-content.test.mjsdocs/api.mddocs/i18n/README.es.mddocs/i18n/README.ja.mddocs/i18n/README.ko.mddocs/i18n/README.zh-CN.mddocs/migration-guide.mdsrc/CacheNamespace.tssrc/CacheStack.tssrc/cli.tssrc/integrations/express.tssrc/integrations/hono.tssrc/integrations/httpCacheKeys.tssrc/integrations/opentelemetry.tssrc/internal/CacheKeyDiscovery.tssrc/internal/CacheSnapshotFile.tssrc/internal/PayloadProtection.tssrc/invalidation/RedisInvalidationBus.tssrc/layers/DiskLayer.tstests/CacheNamespace.test.tstests/cli.test.tstests/docs/PlaygroundWorkerSandbox.test.tstests/features/GrowthFeatures.test.tstests/integrations/Integrations.test.tstests/internal/CacheKeyDiscovery.test.tstests/internal/CacheSnapshotFile.test.tstests/internal/CacheStackInternals.test.tstests/internal/PayloadProtection.test.tstests/invalidation/RedisInvalidationBus.test.tstests/layers/DiskLayer.test.ts
8ae9bd7 to
ee9249e
Compare
|
@coderabbitai review please |
|
✅ Action performedReview finished.
|
|
✅ Action performedReview finished.
|
|
@coderabbitai review please |
|
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/internal/CacheKeyDiscovery.ts (2)
14-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForward
maxMatchesto enable early termination and prevent redundant limit checking.The
maxMatchesparameter is not passed toforEachKeyWithPrefixon line 16, which defeats the streaming optimization. The internalvisitfunction inforEachKeyWithPrefixis designed to enforce the limit during key discovery, but without the parameter, it uses the defaultfalse(no limit). This causes:
- Memory waste: When the tag index uses non-streaming APIs (
keysForPrefix/matchPattern), all matching keys are materialized into an array before any limit check.- CPU waste: Layer scanning continues even after the limit is logically exceeded.
- Redundant checking: The visitor callback checks the limit again, but this happens too late.
The visitor check on line 18 also becomes redundant if
maxMatchesis properly forwarded, since the internalvisitfunction will throw before the visitor is called for the exceeding key.⚡ Proposed fix
async collectKeysWithPrefix(prefix: string, maxMatches: number | false = false): Promise<string[]> { const matches = new Set<string>() - await this.forEachKeyWithPrefix(prefix, (key) => { + await this.forEachKeyWithPrefix(prefix, (key) => { matches.add(key) - this.assertWithinMatchLimit(matches, maxMatches) - }) + }, maxMatches) return [...matches] }Note: The
assertWithinMatchLimitcall in the visitor becomes unreachable and can be removed, since the internalvisitfunction will throw before the visitor is called when the limit is exceeded.🤖 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/internal/CacheKeyDiscovery.ts` around lines 14 - 22, The `collectKeysWithPrefix` method is not forwarding the `maxMatches` parameter to the `forEachKeyWithPrefix` call, which prevents early termination and causes inefficient resource usage. Pass the `maxMatches` parameter directly to the `forEachKeyWithPrefix` method so that the internal `visit` function can enforce the limit during key discovery. Additionally, remove the now-redundant `assertWithinMatchLimit` call from the visitor callback since the internal visit function will throw before the visitor callback is invoked when the limit is exceeded.
137-141:⚠️ Potential issue | 🟡 MinorRemove unused private method
assertWithinMatchCount.The
assertWithinMatchCountmethod at lines 137–141 is defined but never called anywhere in the codebase. Since it's markedprivate, it cannot be invoked from outside the class, making this a safe removal.Cleanup
- private assertWithinMatchCount(matches: number, maxMatches: number | false): void { - if (maxMatches !== false && matches > maxMatches) { - throw new Error(`Invalidation matched too many keys (${matches} > ${maxMatches}).`) - } - }🤖 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/internal/CacheKeyDiscovery.ts` around lines 137 - 141, Remove the unused private method `assertWithinMatchCount` from the CacheKeyDiscovery class. This method is never called anywhere in the codebase and can be safely deleted. Simply remove the entire method definition including the method signature and its body.
🤖 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.
Outside diff comments:
In `@src/internal/CacheKeyDiscovery.ts`:
- Around line 14-22: The `collectKeysWithPrefix` method is not forwarding the
`maxMatches` parameter to the `forEachKeyWithPrefix` call, which prevents early
termination and causes inefficient resource usage. Pass the `maxMatches`
parameter directly to the `forEachKeyWithPrefix` method so that the internal
`visit` function can enforce the limit during key discovery. Additionally,
remove the now-redundant `assertWithinMatchLimit` call from the visitor callback
since the internal visit function will throw before the visitor callback is
invoked when the limit is exceeded.
- Around line 137-141: Remove the unused private method `assertWithinMatchCount`
from the CacheKeyDiscovery class. This method is never called anywhere in the
codebase and can be safely deleted. Simply remove the entire method definition
including the method signature and its body.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a324ac33-a93f-4c03-8a25-ab81e2c68fc5
📒 Files selected for processing (5)
src/internal/CacheKeyDiscovery.tssrc/internal/CacheSnapshotFile.tssrc/internal/CacheStackSnapshotManager.tstests/internal/CacheKeyDiscovery.test.tstests/internal/CacheSnapshotFile.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/internal/CacheStackSnapshotManager.ts
- tests/internal/CacheSnapshotFile.test.ts
- src/internal/CacheSnapshotFile.ts
2 participants
Description
Hardens cache security behavior across HTTP middleware, namespaces, protected disk payloads, generation cleanup, telemetry, CLI operations, Redis invalidation, snapshots, write coordination, structured cache keys, rate limiting, and the docs playground.
This update also closes all 10 actionable findings from the detailed follow-up review of PR #101. The fixes are covered by deterministic race/security regressions, real Redis integration tests, public migration guidance, and an architecture decision record.
Type of Change
Follow-up Review Fixes
client_secret,client_assertion, andclient_assertion_typeas sensitive and bypasses URL-only caching unless a customkeyResolveris supplied.CacheWriteSaturationErrorbackpressure.$typetags used forDate,URL,RegExp,Map, andSetcache keys; structured keys are versioned asj2:.mset()writes share one per-key ordering boundary, so stale cleanup cannot delete a newer bulk value.*and?requires--force, including**and?*.Function()/worker globals.Original PR Hardening
Compatibility and Migration Notes
wrap()argument keys rotate fromj:toj2:. Existing entries become cold misses and can expire naturally.$typetag must use a customkeyResolver.generationCleanup: truedefaults tomaxMatches: 10_000.maxMatches: falseis an explicit opt-out for deployments that impose another keyspace bound.--forceintent.The rationale, module responsibilities, data flow, alternatives, and tradeoffs are recorded in
docs/adr/0001-security-hardening-boundaries.md. API, migration, resilience, CLI, integrations, changelog, README, and security documentation were updated with the same behavior.Verification
npm run test:coverage: 45 files passed, 5 skipped; 665 tests passed, 24 skipped.npm run test:integration: 6 files, 25 real Redis integration tests passed.npm run lint.npm run buildwith ESM, CJS, and declaration output.npm --prefix docs-web run lint.npm --prefix docs-web run build.npm pack --dry-runplus ESM and CJS runtime smoke tests.0after pruning and received monotonic token50002.Checklist
CHANGELOG.mdis updated.Additional Notes
allowLegacyPlaintext,includeRawKeyAttributes,generationCleanup.maxMatches: false, and higher write-coordination limits are explicit opt-ins that should be reviewed against deployment-specific trust and memory boundaries.