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)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 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
flyingsquirrel0419
force-pushed
the
security-hardening-docs
branch
from
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
Fixes and hardens cache security behavior across HTTP middleware, namespaces, DiskLayer protection, generation cleanup, telemetry, CLI safeguards, Redis invalidation, snapshots, and the docs playground. Also updates user-facing docs and adds focused regression coverage.
Type of Change
Changes
Testing
npm test)npm run lint)npm run build:all)Additional validation run:
npm run test:coverage- 623 passed, 24 skipped; coverage increased to 97.22% statements / 97.60% linesnpm --prefix docs-web run lintnode --test docs-web/tests/current-docs-content.test.mjs docs-web/tests/reporting-cache-state.test.mjs docs-web/tests/run-timeout-controller.test.mjsnpm --prefix docs-web run buildChecklist
CHANGELOG.mdand package versions when the PR changes release scopeas any,@ts-ignore,@ts-expect-error)Additional Notes
allowLegacyPlaintextandincludeRawKeyAttributesare documented as explicit opt-ins. Unsigned Redis invalidation remains compatibility-supported but now warns through the configured logger.Summary by CodeRabbit
Release Notes
Security
keyResolveris provided.Bug Fixes
New Features
Documentation & Tests