fix(security): redact sensitive URL parameters in logs

[s0up4200]

Summary

• Add pkg/redact package to centralize redaction of sensitive query parameters (apikey, api_key, passkey, token, password) from URLs and error messages
• Handle userinfo passwords (user:pass@host → user:REDACTED@host) and proxy path segments (/proxy/{key}/ → /proxy/REDACTED/)
• Apply redaction to all identified leak points across the codebase

Fixes #839

Summary by CodeRabbit

• New Features

  • Application-wide redaction now masks API keys, tokens, passwords, sensitive URL query params, proxy path keys, and credential details in logs and error messages to prevent leaking secrets.
  • Access and request logs no longer expose raw paths or credential values.

• Tests

  • New unit and integration tests verify redaction for URLs, query parameters, proxy paths, basic auth strings, and wrapped error messages.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Warning

Rate limit exceeded

@s0up4200 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 11 minutes and 12 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between d5454f0 and 1abe8a5.

📒 Files selected for processing (2)

• internal/proxy/retry_transport.go
• internal/services/jackett/service.go

Walkthrough

Adds a new pkg/redact package and integrates redaction across logging and error wrapping in handlers, proxy, metrics, services, and clients to prevent leaking sensitive URL/query/basic-auth data; includes unit tests and a regression test.

Changes

Cohort / File(s)	Summary
New redact package pkg/redact/redact.go, pkg/redact/redact_test.go, pkg/redact/url_error_test.go	New utilities: URLString, URLError, String, ProxyPath, BasicAuthUser and tests to sanitize URLs, query params (apikey, api_key, passkey, token, password), userinfo and proxy paths.
API handlers internal/api/handlers/jackett.go, internal/api/handlers/torrents.go	Replace raw URL/error logs with redact.URLString / redact.String to avoid leaking URLs and query params in error output.
Proxy & middleware internal/proxy/handler.go, internal/proxy/middleware.go, internal/proxy/retry_transport.go	Use redacted path/URL/error values in logs; remove some detailed path/context logging; middleware now returns immediately on missing API key; retry_transport refactors retryability checks and logs redacted URL/error content.
Metrics & server internal/metrics/server.go	Redacts basic-auth credential display via redact.BasicAuthUser in invalid-auth logging.
Jackett service & client internal/services/jackett/client.go, internal/services/jackett/service.go, internal/services/jackett/client_test.go	Wrap HTTP errors with redact.URLError, log base URLs with redact.URLString, and add regression test TestDiscoverJackettIndexers_RedactsAPIKey.
Third-party clients / gojackett / prowlarr pkg/gojackett/http.go, pkg/gojackett/methods.go, pkg/prowlarr/client.go	Redact request URLs and wrap HTTP errors with redact.URLError; use redact.URLString in retry/on-retry logging.
Logging context adjustments internal/api/middleware/logger.go, internal/proxy/handler.go, internal/proxy/middleware.go	Replace raw request path with redact.ProxyPath in access logs and shorten/omit detailed sensitive fields from logger context.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐇 I nibble logs and hide each key,

REDACTED whispers where secrets used to be,
No more traces in the stream,
Quiet logs and tidy dream,
Hop, redact, and rest with me.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 65.22% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: adding redaction for sensitive URL parameters in logs across the codebase to fix the security issue.
Linked Issues check	✅ Passed	The PR comprehensively addresses issue #839 by implementing redaction of sensitive query parameters (apikey, api_key, token, passkey, password), userinfo passwords, and proxy paths throughout the codebase.
Out of Scope Changes check	✅ Passed	All changes are directly related to redacting sensitive data in logs and errors. The new redact package and its application across handlers, services, and middleware are all in-scope for fixing the secret leakage issue.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

pkg/redact/redact.go (2)

14-23: Redaction primitives are well-scoped; consider small proxy-path regex enhancement

URL/userinfo/query redaction in URLString, plus the layered String/ProxyPath helpers, covers the sensitive cases (apikey/api_key/passkey/token/password, userinfo passwords, /proxy/{key}/ segments) without overreaching. The fallback from URLString to String on parse failure is a good safety net.

One minor, optional improvement: proxyPathRegex ((/proxy/)([^/]+)(/|$)) will not match /proxy/<key>?... when applied to arbitrary strings (because ? prevents the final (/|$) from matching). It’s already correct for real URLs via URLString (which passes only the path), but if you expect to feed full raw request lines or log messages into String, you might want to extend the terminator to [/?#] to also catch query/fragment cases.

Also applies to: 28-78, 101-118, 120-127, 129-140

---

80-99: URLError redacts correctly but may drop outer error context when misused

URLError correctly finds a *url.Error and replaces its URL with URLString(urlErr.URL), which fixes the core leak for http.Client failures. Note that it returns a new url.Error, discarding any outer wrappers that may have been added around the original error. That’s fine for current call sites that pass the raw Do error, but if you later call URLError on already-wrapped errors you’ll lose that higher-level context.

To avoid surprises, either (a) document that callers should pass the raw transport error, or (b) consider mutating urlErr.URL in place and returning the original err so any outer wrapping is preserved.

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ace0101 and c5fb29e.

📒 Files selected for processing (15)

• internal/api/handlers/jackett.go
• internal/api/handlers/torrents.go
• internal/metrics/server.go
• internal/proxy/handler.go
• internal/proxy/middleware.go
• internal/proxy/retry_transport.go
• internal/services/jackett/client.go
• internal/services/jackett/client_test.go
• internal/services/jackett/service.go
• pkg/gojackett/http.go
• pkg/gojackett/methods.go
• pkg/prowlarr/client.go
• pkg/redact/redact.go
• pkg/redact/redact_test.go
• pkg/redact/url_error_test.go

🧰 Additional context used

🧠 Learnings (4)

📚 Learning: 2025-11-06T12:11:04.963Z

Learnt from: Audionut
Repo: autobrr/qui PR: 553
File: internal/services/crossseed/service.go:1045-1082
Timestamp: 2025-11-06T12:11:04.963Z
Learning: The autobrr/qui project uses a custom go-qbittorrent client library (github.com/autobrr/go-qbittorrent) that supports both "paused" and "stopped" parameters when adding torrents via the options map. Both parameters should be set together when controlling torrent start state, as seen in internal/services/crossseed/service.go and throughout the codebase.

Applied to files:

• internal/services/jackett/client.go
• internal/api/handlers/torrents.go

📚 Learning: 2025-11-25T22:46:03.762Z

Learnt from: s0up4200
Repo: autobrr/qui PR: 632
File: internal/backups/service.go:1401-1404
Timestamp: 2025-11-25T22:46:03.762Z
Learning: In qui's backup service (internal/backups/service.go), background torrent downloads initiated during manifest import intentionally use a fire-and-forget pattern with the shared service context (s.ctx). Per-run cancellation is not needed, as orphaned downloads completing after run deletion are considered harmless and acceptable. This design prioritizes simplicity over per-run lifecycle management for background downloads.

Applied to files:

• internal/services/jackett/client.go
• internal/api/handlers/torrents.go

📚 Learning: 2025-11-27T15:33:39.007Z

Learnt from: s0up4200
Repo: autobrr/qui PR: 641
File: internal/services/jackett/service.go:309-330
Timestamp: 2025-11-27T15:33:39.007Z
Learning: In internal/services/jackett/service.go, synchronous execution paths (test executors, interactive searches with explicit indexer selection, and scheduler-unavailable fallback) intentionally pass jobID=0 to result callbacks. This is acceptable because outcome tracking via ReportIndexerOutcome is only used for async cross-seed operations that always use the scheduler and receive unique jobIDs. Interactive searches and tests do not use outcome tracking.

Applied to files:

• internal/api/handlers/jackett.go
• internal/services/jackett/client_test.go

📚 Learning: 2025-12-11T08:40:01.329Z

Learnt from: s0up4200
Repo: autobrr/qui PR: 746
File: internal/services/reannounce/service.go:480-481
Timestamp: 2025-12-11T08:40:01.329Z
Learning: In autobrr/qui's internal/services/reannounce/service.go, the hasHealthyTracker, getProblematicTrackers, and getHealthyTrackers functions intentionally match qbrr's lenient tracker health logic (skip unregistered trackers and check if any other tracker is healthy) rather than go-qbittorrent's strict isTrackerStatusOK logic (which treats unregistered as an immediate failure). For multi-tracker torrents, if one tracker is working, reannouncing won't help. The duplication of the health check logic across these three functions is acceptable as it's a simple one-liner, and extracting it would add unnecessary complexity.

Applied to files:

• internal/proxy/handler.go
• internal/api/handlers/torrents.go

🧬 Code graph analysis (8)

pkg/prowlarr/client.go (1)

pkg/redact/redact.go (1)

• URLError (83-99)

pkg/gojackett/http.go (1)

pkg/redact/redact.go (3)

• URLError (83-99)
• URLString (28-78)
• String (107-118)

internal/services/jackett/client.go (1)

pkg/redact/redact.go (2)

• URLError (83-99)
• URLString (28-78)

internal/services/jackett/service.go (1)

pkg/redact/redact.go (1)

• URLString (28-78)

internal/proxy/handler.go (1)

pkg/redact/redact.go (1)

• String (107-118)

internal/services/jackett/client_test.go (1)

internal/services/jackett/client.go (1)

• DiscoverJackettIndexers (481-565)

internal/metrics/server.go (1)

pkg/redact/redact.go (1)

• BasicAuthUser (131-140)

internal/api/handlers/torrents.go (1)

pkg/redact/redact.go (1)

• URLString (28-78)

🔇 Additional comments (23)

internal/services/jackett/client_test.go (1)

7-16: Regression test for API-key redaction looks solid

The new imports and TestDiscoverJackettIndexers_RedactsAPIKey cleanly exercise the failure path without depending on a specific error shape, while asserting both absence of the raw key and presence of apikey=REDACTED when applicable. No issues found.

Also applies to: 143-175

pkg/prowlarr/client.go (1)

17-19: SearchIndexer error wrapping now safely redacts request URLs

Wrapping c.httpClient.Do errors with redact.URLError(err) inside fmt.Errorf("prowlarr request failed: %w", …) achieves URL redaction for *url.Error without changing the surrounding message or control flow. This is aligned with the PR’s objectives and looks correct.

Also applies to: 149-152

internal/services/jackett/service.go (1)

26-31: Redacting base_url in search logs is appropriate and non-invasive

Using redact.URLString(idx.BaseURL) for the base_url log field keeps the existing observability while ensuring any misconfigured base URLs with query secrets or userinfo are masked. No functional behavior changes; this is a safe hardening step.

Also applies to: 1785-1810

internal/api/handlers/jackett.go (1)

19-22: Discovery error logging now properly redacts base URL and error message

The DiscoverIndexers handler’s error path now logs base_url via redact.URLString and the error text via redact.String(err.Error()), which should block the Prowlarr API-key leak that motivated issue #839 without altering response codes or bodies. This looks correct and consistent with the new redact utilities.

Also applies to: 840-865

internal/api/handlers/torrents.go (1)

25-29: URL logging in AddTorrent is now redaction-aware

Switching the three error logs in AddTorrent to use redact.URLString(url) ensures that any sensitive query parameters or userinfo embedded in magnet/HTTP URLs are masked before hitting logs, while keeping the rest of the context intact. No control-flow changes; this is a straightforward hardening.

Also applies to: 553-599

internal/metrics/server.go (1)

12-18: Invalid metrics basic-auth credentials are now logged without passwords

Using redact.BasicAuthUser(cred) in the warning for malformed basicAuthUsersConfig avoids exposing raw user:password pairs in logs while still indicating which entry was bad. This is a good, low-risk improvement.

Also applies to: 32-41

pkg/gojackett/methods.go (1)

13-16: GetEnclosureCtx now wraps errors with a redacted enclosure string

Changing the errors.Wrap context from the raw enclosure to redact.URLString(enclosure) preserves the usefulness of the error message while masking any sensitive data in the enclosure URL. Control flow and return types are unchanged.

Also applies to: 86-90

internal/proxy/handler.go (1)

264-269: LGTM! Appropriate redaction of proxy errors.

The change correctly applies redaction to error messages that may contain sensitive URL parameters while preserving all relevant context (client, instanceId, method) for debugging.

pkg/redact/url_error_test.go (2)

14-117: LGTM! Comprehensive test coverage for URLError redaction.

The test cases cover all critical scenarios including nil errors, various sensitive parameters (apikey, token, password, api_key, passkey), multiple params, wrapped errors, and non-url.Error cases. The assertions properly verify both the presence of REDACTED markers and the absence of actual secrets.

---

119-140: LGTM! Important validation of error type preservation.

This test ensures that URLError() maintains the url.Error type for errors.As() compatibility, which is crucial for error handling code that checks error types. The verification that the Op field is preserved and the URL is redacted is thorough.

internal/proxy/retry_transport.go (3)

52-59: LGTM! Proper redaction on successful retry.

The URL redaction here ensures that even successful retry logs don't expose sensitive query parameters or credentials.

---

66-71: LGTM! Comprehensive redaction for non-retryable errors.

Both the error message and URL are redacted, providing defense-in-depth since errors often contain the request URL with sensitive parameters.

---

80-108: LGTM! Consistent redaction across all retry paths.

The redaction is applied uniformly across all logging scenarios: non-idempotent methods, max retries exceeded, and retry attempts. This consistency ensures no log path accidentally leaks sensitive data.

pkg/gojackett/http.go (2)

30-36: LGTM! Proper error redaction in HTTP client.

The dual redaction approach (both the error via URLError and the URL parameter via URLString) ensures comprehensive protection against secret leakage in HTTP errors.

---

111-132: LGTM! Retry logic properly redacts sensitive data.

The redaction is correctly applied to both the returned error and the retry logging callback, ensuring that the full retry sequence doesn't leak secrets while maintaining debuggability.

internal/services/jackett/client.go (2)

254-262: LGTM! Consistent redaction across all caps fetch paths.

The redaction is uniformly applied to all three backend types (Jackett, Prowlarr, Native), ensuring that caps fetch errors don't leak API keys regardless of the backend configuration.

Also applies to: 289-297, 324-332

---

366-374: LGTM! Download errors properly redacted.

Both network errors and HTTP status errors are appropriately redacted. The DownloadError struct stores the redacted URL, ensuring that any subsequent error handling or logging won't accidentally leak the original URL with sensitive parameters.

internal/proxy/middleware.go (2)

112-114: LGTM! Appropriate reduction of logged API key details.

Logging only whether the key exists (rather than the key itself or portions of it) is the right approach for security. This still provides debugging capability to identify missing keys without exposing sensitive data.

---

116-122: Good catch! Return statement prevents unauthorized access.

The addition of the return statement after the "Missing API key" error response is critical. Without it, execution would continue to next.ServeHTTP(w, r.WithContext(ctx)) at line 162, potentially allowing unauthenticated requests to proceed. This is an important security fix.

pkg/redact/redact_test.go (4)

11-145: LGTM! Excellent test coverage for URLString.

The tests comprehensively cover:

• All sensitive parameter types (apikey, api_key, passkey, token, password)
• Case-insensitive matching (critical for security)
• Edge cases (empty strings, empty param values, multi-valued params)
• Combined scenarios (userinfo + query params, proxy paths)
• Non-sensitive parameters (ensuring they're preserved)

This thorough coverage helps ensure no regression in the redaction logic.

---

147-245: LGTM! Comprehensive testing of string-based redaction.

The test cases cover important real-world scenarios including:

• Error messages containing sensitive data
• Unparseable URL fragments (ensuring fallback to regex works)
• Multiple parameters in a single string
• Combination scenarios (userinfo, proxy paths)

The coverage ensures that even malformed or partial URLs in error messages don't leak secrets.

---

247-287: LGTM! Focused proxy path redaction tests.

The test cases appropriately cover the proxy path redaction scenarios including edge cases like trailing slashes and keys at the end of paths. The exact-match assertions are appropriate for this specific use case.

---

289-329: LGTM! Complete basic auth redaction tests.

The test cases cover all relevant scenarios for basic authentication credential redaction, including edge cases like empty passwords and complex password characters. The tests ensure usernames are preserved while passwords are consistently redacted.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

internal/api/middleware/logger.go (1)

46-46: Proxy path redaction correctly applied.

The change properly redacts proxy path patterns (e.g., /proxy/{key}/ → /proxy/REDACTED/) in the access log.

Note that r.URL.Path contains only the path component, so query parameters (e.g., ?apikey=...) are not logged here. This is a safe approach that prevents any query parameter leaks in this middleware. If more comprehensive request logging is desired for debugging while maintaining security, consider using redact.URLString(r.URL.String()) to log the complete URL with all sensitive components redacted.

Optional: Log complete redacted URL for more comprehensive debugging

If logging the full request URL (including query parameters but redacted) would be valuable for debugging, consider:

-						"url":        redact.ProxyPath(r.URL.Path),
+						"url":        redact.URLString(r.URL.String()),

This would log the complete URL with query parameters like apikey, passkey, token, etc. automatically redacted, providing more debugging context while maintaining security.

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 949a4a1 and d5454f0.

📒 Files selected for processing (2)

• internal/api/middleware/logger.go
• internal/proxy/handler.go

🚧 Files skipped from review as they are similar to previous changes (1)

• internal/proxy/handler.go

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-12-28T18:44:10.496Z

Learnt from: s0up4200
Repo: autobrr/qui PR: 876
File: internal/logstream/hub_test.go:188-192
Timestamp: 2025-12-28T18:44:10.496Z
Learning: In Go 1.25 (Aug 2025), use wg.Go(func()) to spawn a goroutine and automate the Add/Done lifecycle. Replace manual patterns like wg.Add(1); go func(){ defer wg.Done(); ... }() with wg.Go(func(){ ... }). Ensure the codebase builds with Go 1.25+ and apply this in relevant Go files (e.g., internal/logstream/hub_test.go). If targeting older Go versions, maintain the existing pattern.

Applied to files:

• internal/api/middleware/logger.go

🧬 Code graph analysis (1)

internal/api/middleware/logger.go (1)

pkg/redact/redact.go (1)

• ProxyPath (122-127)

🔇 Additional comments (1)

internal/api/middleware/logger.go (1)

13-14: LGTM!

The import is correctly added to support the redaction functionality used below.