test: credential redaction verification and metadata filter fuzz hard… - #147
Conversation
|
Warning Review limit reached
More reviews will be available in 42 minutes and 18 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR hardens credential redaction across all tool output paths and introduces validated, depth-limited metadata filter validation. It adds ChangesCredential Redaction and Metadata Filter Validation Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 1
🧹 Nitpick comments (2)
src/core/server/metadata-filter.test.ts (2)
90-94: ⚖️ Poor tradeoffConsider extracting
deepAndto a shared test utility.The
deepAndhelper is duplicated between this file andmetadata-filter.fuzz.test.ts(lines 28-31). Extracting it to a shared test utility module would eliminate duplication and provide a single source of truth.🤖 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/core/server/metadata-filter.test.ts` around lines 90 - 94, The deepAnd helper is duplicated; extract the function deepAnd(levels: number): Record<string, unknown> into a shared test utility module (e.g., tests/util/filters or similar), export it, and replace the local implementations in both metadata-filter.test.ts and metadata-filter.fuzz.test.ts with an import of that shared deepAnd; ensure the exported helper has the same signature and semantics so existing tests (which call deepAnd) require no other changes.
54-63: ⚡ Quick winUse
MAX_FILTER_DEPTHconstant instead of hardcoding depth values.The depth values
10and11are hardcoded here, but the fuzz test file correctly imports and usesMAX_FILTER_DEPTHfrom the validator module. If the maximum depth ever changes, these hardcoded values will become incorrect.♻️ Refactor to use the constant
import { describe, expect, it } from 'vitest'; -import { validateMetadataFilter, validateMetadataFilterDetailed } from './metadata-filter.js'; +import { + MAX_FILTER_DEPTH, + validateMetadataFilter, + validateMetadataFilterDetailed, +} from './metadata-filter.js';Then update the test cases:
it('accepts filter at maximum nesting depth', () => { - expect(validateMetadataFilterDetailed({ x: deepAnd(10) })).toBeNull(); + expect(validateMetadataFilterDetailed({ x: deepAnd(MAX_FILTER_DEPTH) })).toBeNull(); }); it('rejects filter exceeding maximum nesting depth', () => { - const d = validateMetadataFilterDetailed({ x: deepAnd(11) }); + const d = validateMetadataFilterDetailed({ x: deepAnd(MAX_FILTER_DEPTH + 1) }); expect(d).not.toBeNull(); expect(d!.message).toContain('maximum depth');🤖 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/core/server/metadata-filter.test.ts` around lines 54 - 63, Replace the hardcoded depth values in the two tests with the MAX_FILTER_DEPTH constant from the validator module: call validateMetadataFilterDetailed({ x: deepAnd(MAX_FILTER_DEPTH) }) for the acceptance case and validateMetadataFilterDetailed({ x: deepAnd(MAX_FILTER_DEPTH + 1) }) for the rejection case; ensure MAX_FILTER_DEPTH is imported from the same module that exports the validator (the module where validateMetadataFilterDetailed is defined) so the tests remain correct if the max depth changes.
🤖 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 `@package.json`:
- Line 89: The repo now lists a new dependency "fast-check" in package.json but
package-lock.json was not updated; run a lockfile regen (e.g., npm install) to
update package-lock.json, verify with npm ci or npm install --package-lock-only
that there is no drift, add the updated package-lock.json to the commit, and
push so CI will pass; target the change that added "fast-check" in package.json
when updating and committing the lockfile.
---
Nitpick comments:
In `@src/core/server/metadata-filter.test.ts`:
- Around line 90-94: The deepAnd helper is duplicated; extract the function
deepAnd(levels: number): Record<string, unknown> into a shared test utility
module (e.g., tests/util/filters or similar), export it, and replace the local
implementations in both metadata-filter.test.ts and metadata-filter.fuzz.test.ts
with an import of that shared deepAnd; ensure the exported helper has the same
signature and semantics so existing tests (which call deepAnd) require no other
changes.
- Around line 54-63: Replace the hardcoded depth values in the two tests with
the MAX_FILTER_DEPTH constant from the validator module: call
validateMetadataFilterDetailed({ x: deepAnd(MAX_FILTER_DEPTH) }) for the
acceptance case and validateMetadataFilterDetailed({ x: deepAnd(MAX_FILTER_DEPTH
+ 1) }) for the rejection case; ensure MAX_FILTER_DEPTH is imported from the
same module that exports the validator (the module where
validateMetadataFilterDetailed is defined) so the tests remain correct if the
max depth changes.
🪄 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: ba174d4f-82da-4897-967e-594db55f8c6e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
docs/SECURITY.mdpackage.jsonsrc/core/pinecone/rerank.tssrc/core/server/metadata-filter.fuzz.test.tssrc/core/server/metadata-filter.test.tssrc/core/server/metadata-filter.tssrc/core/server/redaction.test.tssrc/core/server/tool-response.tssrc/logger.ts
Lockfile was produced with npm 11 locally; GitHub Actions uses npm 10, which failed npm ci due to missing @emnapi/core/runtime@1.11.0 entries. Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #147 +/- ##
=======================================
Coverage ? 85.26%
=======================================
Files ? 39
Lines ? 1459
Branches ? 502
=======================================
Hits ? 1244
Misses ? 213
Partials ? 2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review again |
|
✅ Action performedFull review finished. |
3 participants
Summary
Item 5 (week_plan_v1) closes two pre-1.0 security gaps: MCP tool responses now sanitize known credential-bearing fields before JSON serialization, and the metadata filter validator rejects degenerate/oversized inputs with structured errors instead of passing them through to Pinecone. Property-based fuzz tests (
fast-check) and targeted redaction tests lock in the behavior.Issues addressed
Changes
5a — Credential redaction
redactApiKey()extended withpcsk_[A-Za-z0-9_-]{40,}pattern for modern Pinecone keysredactSensitiveFields()— key-targeted walk overmessage,suggestion,degradation_reason(preserves document UUIDs in metadata)jsonResponse/jsonErrorResponsesanitize payloads beforeJSON.stringify(single choke point for all 9 tools)rerank.tsredacts SDK error text when buildingdegradation_reasondocs/SECURITY.mddocuments MCP response redaction (not only stderr)5b — Metadata filter hardening + fuzz
MAX_FILTER_DEPTH = 10with explicitdepthcounter (depth 10 passes, 11 rejects)$and,$or,$in,$ninwith clear validation messagesfast-checkdev dependency +metadata-filter.fuzz.test.ts(4 properties × 100 runs)metadata-filter.test.tsregression cases for depth boundary and empty-array guardsAcceptance criteria
#136 (5a — redaction)
messageandsuggestionredacted in MCP responsesHybridQueryResult.degradation_reasonredacted (queryhandler +jsonResponse)guided_queryresponse path redactsresult.degradation_reason(sensitive fields flow throughjsonResponse)classifyToolCatchErrorcatch-all redacted in DEBUG mode viajsonErrorResponsepcsk_…fixturestool-response.ts,rerank.ts,logger.ts#137 (5b — filter fuzz)
$in0/1000+, special field names, unknown operators, invalid$inelementsVALIDATIONonqueryfor empty$infast-checkTest plan
npm test— 211 tests passnpm run test:coverage— passesnpm run typecheck/npm run lint/npm run build— passredaction.test.ts—pcsk_, UUID, Bearer, DEBUG-mode errors, degradation_reason, UUID preservationmetadata-filter.fuzz.test.ts— never-throws, malformed→error, valid grammar, special keys, large$inmetadata-filter.test.ts— depth 10/11 boundary, empty$and/$or/$in/$ninNotes for reviewers
redactSensitiveFieldsis not the same as privateredactValue: logs redact every string; MCP responses redact only allowlisted keys so search-result metadata UUIDs are preserved.decision_trace.explanationis intentionally excluded from the sensitive key list (routing text, not SDK error output).depthparameter onvalidateMetadataFilterRecord(notpath.length), incremented at each$and/$ornested record.- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests