fix(core): redact apikey in nab keys + http recursion logs (follow-up #947)#952
fix(core): redact apikey in nab keys + http recursion logs (follow-up #947)#952slawrensen wants to merge 2 commits into
Conversation
…iren070#947) - http.ts: makeUrlLogSafe docstring updated to match behavior (apikey already stripped). - http.ts: PossibleRecursiveRequestError warn/throw now sanitized via makeUrlLogSafe, matching the sibling DEBUG log on the same path. Benefits all callers, not just nab. - builtins/base/nab/api.ts: cacheKey and lockKey hashed with existing getSimpleTextHash helper. Keys flow into DistributedLock and searchWithBackgroundRefresh log lines; partitioning preserved, raw secret no longer reaches logs. Outbound apikey unchanged. No new deps, no public API change, no tests (matching Viren070#947). One-time cache-bust on deploy for nab search/caps.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughHash API keys used in cache/lock identifiers and improve URL redaction so logs and recursion-detection errors no longer expose ChangesAPI Key Redaction in Cache, Lock Keys, and Error Messages
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
There was a problem hiding this comment.
Pull request overview
Follow-up to #947 that finishes redacting Newznab apikey values from log output. It updates the makeUrlLogSafe docstring, wraps the previously-raw URL strings in the recursive-request warn/throw paths, and hashes this.apiKey in cache/lock keys (which are logged downstream) using the existing getSimpleTextHash helper.
Changes:
- Wrap
urlObj.toString()withmakeUrlLogSafeinPossibleRecursiveRequestErrorwarn log and thrown error message. - Replace raw
apiKeyinterpolation in nabcacheKey/lockKeywith agetSimpleTextHashof the key (preserves per-credential partitioning while keeping logs safe). - Update
makeUrlLogSafedoc comment to reflect thatapikeyis also masked (case-insensitive).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/core/src/utils/http.ts | Doc comment fix; recursion warn/throw now go through makeUrlLogSafe. |
| packages/core/src/builtins/base/nab/api.ts | Import getSimpleTextHash; hash this.apiKey in cacheKey and lockKey to avoid leaking it via downstream logs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- makeUrlLogSafe: parse URL and redact userinfo (username/password) and any 'password'/'apikey' query OR fragment param (case-insensitive). Falls back to legacy regex stripping when URL parsing fails. Also tightens the password= regex to /gi to match the apikey= line. - makeRequest debug log: extract requestMode and run options.forceProxy through makeUrlLogSafe so a proxy URL with userinfo (http://user:pass@host) does not leak.
|
Pushed an amend addressing two more leak surfaces on the same code path:
Same scope (#947 follow-up, leak redaction), no behavior change for outbound traffic, no new deps. Out of scope but worth flagging for a separate PR: |
Follow-up to #947. That PR landed the Newznab error/debug log redaction but left a few of the review comments unaddressed. This picks those up, plus one related surface I noticed while looking at the same paths again.
Changes (3, smallest possible):
packages/core/src/utils/http.ts—makeUrlLogSafedocstring updated. The function already stripsapikey=(line 46) but the comment only mentionedpassword. Doc now matches behavior.packages/core/src/utils/http.ts—PossibleRecursiveRequestErrorwarn and throw embedded the rawurlObj.toString(), while the sibling DEBUG log on the same path already runs throughmakeUrlLogSafe. Wrapped both so recursion errors don't leak apikey for any caller (not just nab).packages/core/src/builtins/base/nab/api.ts—cacheKeyandlockKeyembedded rawthis.apiKey. Both keys flow intologger.*calls insideDistributedLockandsearchWithBackgroundRefresh. Hashed with the existinggetSimpleTextHashhelper so credential partitioning is kept but the raw key never reaches a log line.Note: the cache/lock key change is a one-time cache-bust on deploy (existing entries won't match the new key shape). No data loss, just a cold cache for nab search/caps.
Outbound request behavior is unchanged — the apikey is still sent in plain to the upstream Newznab endpoint via
searchParams.set('apikey', ...).No new deps, no public API change, no tests added (matching #947). Happy to add tests if you want them.
Summary by CodeRabbit