refactor(log): source sensitive redaction lists from config as pure extensions#3961
Conversation
lib/ must stay module-agnostic — SENSITIVE_PATH_MARKERS and SENSITIVE_QUERY_KEYS in lib/helpers/redactUrl.js hardcoded auth/invitations route vocabulary owned by feature modules (#3935). Move both lists to config.log.sensitivePathMarkers / config.log.sensitiveQueryKeys (current values as defaults in config/defaults/development.config.js) so a module extends redaction coverage from its own config without editing shared lib/. redactUrl.js reads the lists from config once at module load, falling back to the built-in literals when config or the key is absent — redaction degrades safely instead of silently redacting nothing in an edge context. Exports/API unchanged (existing consumers: lib/middlewares/analytics.js, lib/services/express.js). Closes #3953 Claude-Session: https://claude.ai/code/session_01WfNC8bt1TgL4AsiYgCEGup
deepMerge replaces arrays (no union), so a module-level config.log.sensitivePathMarkers/sensitiveQueryKeys extension clobbered the global default instead of extending it, and a `??` fallback let an explicit empty array silently disable redaction for that vector. Compute the effective lists as a Set-deduped union of the built-in DEFAULT_SENSITIVE_* base with config.log.*, so config is purely additive from any layer and redaction can never be disabled via config. development.config.js's entries become empty arrays (the extension point), killing the literal duplication with redactUrl.js. Claude-Session: https://claude.ai/code/session_01WfNC8bt1TgL4AsiYgCEGup
Phase-0 iteration-2 review findings on #3953: - Remove log.sensitivePathMarkers/sensitiveQueryKeys from config/defaults/development.config.js. deepMerge (config/index.js) replaces arrays wholesale, so declaring these as [] at Layer 2 (global defaults) silently clobbered any Layer 1 (module config) extension of the same key. Omitting the key lets a module value pass through untouched; a comment documents the extension point. - Guard redactUrl.js against non-array config values via a sanitizeConfigList helper: an object/number would throw at module-eval (boot crash), a string would spread char-by-char (silent over-redaction). Malformed values now fall back to [] (defaults still apply) and log a console.warn identifying the offending config path. - Add malformed-value tests (object + string) and a lockstep guard test pinning that development.config.js never re-declares either key. Claude-Session: https://claude.ai/code/session_01WfNC8bt1TgL4AsiYgCEGup
|
Warning Review limit reached
Next review available in: 57 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughURL redaction now accepts optional configured sensitive query keys and path markers, unions them with built-in defaults, warns on invalid list types, and preserves defaults when configuration is absent. Documentation comments and Jest coverage describe and verify the configuration behavior. ChangesURL redaction configuration
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3961 +/- ##
==========================================
+ Coverage 92.70% 92.72% +0.02%
==========================================
Files 169 169
Lines 5563 5580 +17
Branches 1791 1794 +3
==========================================
+ Hits 5157 5174 +17
Misses 326 326
Partials 80 80
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
![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
🤖 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 `@lib/helpers/tests/redactUrl.unit.tests.js`:
- Around line 197-206: Ensure both affected tests in
lib/helpers/tests/redactUrl.unit.tests.js (lines 197-206 and 208-217) always
restore their console.warn spies: wrap each test body in try/finally and move
warnSpy.mockRestore() into the finally block, preserving the existing assertions
and behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e30538f2-26bf-43ad-9f24-db5f210930f2
📒 Files selected for processing (3)
config/defaults/development.config.jslib/helpers/redactUrl.jslib/helpers/tests/redactUrl.unit.tests.js
…3972) OAuth callbacks (GET /api/auth/:strategy/callback?code=...&state=...) wrote the one-time authorization code and state into the morgan access log in cleartext. Same single-use-secret-in-URL leak class already closed for inviteToken/reset/verify-email (#3955/#3961). Add 'code' and 'state' to DEFAULT_SENSITIVE_QUERY_KEYS in lib/helpers/redactUrl.js so they are redacted by default everywhere, without requiring per-project config. Update the JSDoc and the development.config.js comment that documents the built-in defaults, and add unit tests covering the OAuth callback shape. Closes #3967 Claude-Session: https://claude.ai/code/session_01WfNC8bt1TgL4AsiYgCEGup
1 participant
Summary
SENSITIVE_PATH_MARKERSandSENSITIVE_QUERY_KEYS(module route vocabulary) were hardcoded insidelib/helpers/redactUrl.js. They now read optionalconfig.log.sensitivePathMarkers/config.log.sensitiveQueryKeyskeys and Set-union them with the authoritative built-in defaults, behind a shape guard (non-array / malformed config values are ignored, never thrown).lib/. Config extension is additive-only by design: config can never shrink, disable, or crash redaction, since the built-ins always apply regardless of what config provides.Scope
lib/helpers/redactUrl.js(shared lib),config/defaults/development.config.jsnone— pure additive extension point, existing callers unaffected (empty/absent config = built-in-only behavior, byte-identical to before)lowValidation
npm run lintnpm testGuardrails check
.env*,secrets/**, keys, tokens)Notes for reviewers
config/defaults/development.config.jsdeliberately omits the two keys — a downstream/module-layer default that did set them would otherwise get clobbered bydeepMerge's array-replace semantics; a lockstep test pins this (defaults file has no markers/keys) so the omission can't regress silently.redactUrl.jsnow importsconfig— verified no circular dependency; degrades gracefully (falls back to built-ins) if config resolution fails.development.config.js(prod/test configs don't define these keys today); extending the pin to those configs if/when they start defining the keys is a cheap follow-up, not required for this change.Tests: 2148 green,
redactUrl.jsat 100% coverage — new cases cover union semantics, empty config, malformed config (shape guard), and the defaults-omission lockstep.https://claude.ai/code/session_01WfNC8bt1TgL4AsiYgCEGup
Summary by CodeRabbit
New Features
Bug Fixes
Documentation