fix: stop logging FetcherConfig in callback reporter - #902
Merged
Conversation
✅ Deploy Preview for opal-docs canceled.
|
…al redaction Asserts that report_update_results does not include the Authorization token, HttpFetcherConfig repr, or `headers=` substring in the loguru record when LOG_SERIALIZE=true. Also simplifies the parallel-lists pattern in the reporter — `urls` is now derived from `callback_requests` once before logging, so the two can no longer drift. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a sensitive-data logging issue in the OPAL client callbacks reporting flow by ensuring CallbacksReporter logs only callback URLs (not the full (url, FetcherConfig) structures that may include credentials), and adds a regression test covering the serialized logging scenario described in #901.
Changes:
- Update
CallbacksReporter.report_update_results()to log only a URL list rather than the full callback request tuples containingFetcherConfig. - Add a regression test that captures Loguru output with
serialize=Trueand asserts that secrets /HttpFetcherConfigdo not appear in logs while URLs still do.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/opal-client/opal_client/callbacks/reporter.py | Avoids leaking credentials by logging only callback URLs while still forwarding full request tuples to the fetcher. |
| packages/opal-client/opal_client/tests/callbacks_reporter_test.py | Adds a serialized-log regression test to ensure FetcherConfig (and tokens) are not logged. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
zeevmoney
approved these changes
May 11, 2026
Contributor
|
@taurelius, thank you for the contribution! |
Zivxx
added a commit
that referenced
this pull request
Jul 5, 2026
* fix: redact credentials in logs at the model layer Follow-up to #902. A customer reported credentials still leaking into logs, this time via opal_client/data/updater.py (logger.exception serializing the full DataSourceEntry, whose `config` carries auth headers and `data` the payload). A full codebase scan found several more sites with the same pattern. Root cause: log statements interpolate credential-bearing pydantic models (`{entry}`, `{config}`, `model=...`) and loguru's serialize=True sink dumps record["extra"] as JSON, falling back to str() for non-JSON objects. Central fix: a RedactedReprMixin that overrides __repr_args__ on the credential-bearing models so repr()/str() (and thus the loguru serialize path) mask secret fields: - FetcherConfig / HttpFetcherConfig -> headers, data - DataSourceEntry -> config, data - FetchEvent -> config This protects every current and future log site at once. Wire serialization (.json()/.dict()), hashing (calc_hash uses config.json()), dedup keys, and equality are untouched, so transport and behavior are unaffected. Belt-and-suspenders spot fixes (log only safe fields): - opal-client data/updater.py (x3): log entry.url instead of the entry - opal-server data_update_publisher.py: log entry.url instead of the entry - opal-server data/api.py: drop the data_sources_config model from the error - opal-common fastapi_rpc_fetch_provider.py: stop logging rpc_arguments Adds opal-common tests/redaction_test.py covering repr/str redaction, wire-payload integrity, and the loguru serialize=True leak scenario. Note for reviewers: the publish-data-update CLI (cli/commands.py:188) echoes str(update), which now shows config/data as <redacted>. The published payload itself (update.json(), line 180) is unaffected; redacting the stdout echo is intentional since it can land in shell history / CI logs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: harden redaction after expert review (MRO-union, RPC config, url userinfo) Addresses findings from a multi-agent adversarial review of the redaction fix: - RedactedReprMixin now unions `_redacted_repr_fields` across the whole MRO, so a subclass can only ever ADD fields to redact - it can never silently drop a parent's protection by redeclaring/omitting the set. Prevents the exact failure mode below. - FastApiRpcFetchConfig: redact `rpc_arguments` (a secret-bearing FetcherConfig subclass that was missed - a live leak of the same class as the original bug). - Add http_utils.redact_url() to strip embedded `user:password@` credentials from URLs, and apply it to every `entry.url` log/exception in data/updater.py and data_update_publisher.py (DataSourceEntry.url is a free string and may be a basic-auth URL). - Tests: add nested-container (DataUpdate), RPC-config, diagnose=True, and redact_url cases. - Nits: fix pre-existing "uniqueue" typo, drop unused imports in schemas/data.py. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: close traceback + git-subsystem credential leaks Two leak vectors outside the model layer, found in the expert review: 1. LOG_DIAGNOSE defaulted to True. loguru's diagnose=True renders raw local variable VALUES in tracebacks (e.g. a bare headers dict, or the Bearer token returned by opa_client._get_auth_headers), bypassing the model-level repr redaction entirely. Default flipped to False (matches loguru's own production guidance); operators can still opt in for local debugging. 2. Git subsystem leaked credentials independently of the data models: - repo_cloner.py logged the raw clone URL (https://token@github.com/...) and the GitCommandError text (which embeds the failing command incl. the URL). - git_fetcher.py (scopes) logged source/remote URLs in many places. - git_utils/env.py forced GIT_TRACE=1 + GIT_CURL_VERBOSE=1, making git dump HTTP Authorization headers to captured stderr. Fix: add http_utils.redact_url_in_text(); redact every git URL log and the git error text; gate GIT_TRACE/GIT_CURL_VERBOSE behind LOG_DIAGNOSE. The functional clone_repository() call still receives the real URL. Adds redact_url_in_text tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: close fetch-path URL leaks + harden redact_url (round 3 review) Second adversarial review pass found residual leaks on the ACTUAL fetch path that the first sweep missed, plus correctness gaps in redact_url: - Fetch-path URL logs now redacted: http_fetch_provider._fetch_ (logged self._url raw), opal_client/data/fetcher.handle_url (3 sites), and sources/git_policy_source remote_urls. - redact_url hardened: * fix IPv6 host (brackets were dropped, producing an invalid URL) * also mask sensitive query-string params (?token=, ?access_token=, ...), not just user:password@ userinfo * username-only userinfo is stripped; non-sensitive URLs returned unchanged - redact_url_in_text is now regex-based (scrubs any scheme://userinfo@ in the text), so it catches credentials even when git normalizes/encodes the URL differently from the value we pass; still also replaces the known URL. - LOG_DIAGNOSE description now notes it also gates git protocol tracing. - Tests: IPv6, query-param, username-only, byte-identity, regex-scrub, and env-gating (GIT_TRACE/GIT_CURL_VERBOSE) cases. - Nits: drop unused FetcherConfig import; List[str] over list[str]; black/isort. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style: apply docformatter to redaction_test docstrings The docformatter pre-commit hook (v1.7.5) reformats docstrings: capitalizes the summary line and splits summary from body. Pure formatting, no logic change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(http-utils): harden redact_url/redact_url_in_text against raise, ordering, and fragment leaks - redact_url no longer raises from a log/except path: accessing parts.username/.password/.port is what actually validates a URL (urlsplit is lazy), so an out-of-range port raised ValueError straight out of an except block. Guard the credential-detection block. - redact_url_in_text now does the known-URL replacement *before* the userinfo regex; previously the regex rewrote user:pw@ -> ***@ first, destroying the verbatim URL so its query token was never masked. - redact_url now masks sensitive params in the URL *fragment* too, and masks query/fragment values in-place so untouched params keep their exact original encoding (no urlencode %20 -> + normalization). Addresses review comments: - #921 (comment) (@zeevmoney) - #921 (comment) (@zeevmoney) - #921 (comment) (@zeevmoney) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(redaction): mask url field of credential-bearing models in repr/str DataSourceEntry.url and FetchEvent.url can embed credentials (user:token@host or ?token=...), but RedactedReprMixin only masked config/data - so whole-model logging (repr/str, loguru serialize) still leaked the URL. Add a generic _redacted_url_fields set to the mixin that runs the value through redact_url (keeps host/path visible, unlike blanket masking), unioned across the MRO like the existing set, and declare {"url"} on both models. Wire serialization (.dict()/.json()) is untouched. Addresses review comments: - #921 (comment) (@zeevmoney) - #921 (comment) (@zeevmoney) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(logs): redact credentialed URLs across log sites (in-diff and beyond) Wrap raw URL log/exception sites with redact_url / redact_url_in_text so a credentialed data-source, policy-repo or callback URL (user:token@host or ?token=...) never reaches the logs: In-diff inline review: - fastapi_rpc_fetch_provider: redact self._url in the fetch log line. - updater._fetch_data: drop the raw {exc} binding (logger.exception already records the traceback; str(e) on a 3rd-party error can carry a credentialed URL and would serialize verbatim under LOG_SERIALIZE) and redact the re-raised message. - git_policy_source: drop a stray ")" in the "found existing repo" log format. Out-of-diff sites (PR-level review): - api_policy_source: redact the 404 bundle URL (log + HTTPException detail) and the "Fetching changes from remote" log. - scopes/service + scopes/loader: redact source.url / POLICY_REPO_URL. - updater: redact entry.url passed to _update_remote_status (it is persisted into the transaction log) and the error strings; stop logging the raw inline data payload in calc_hash; redact the data-sources config URL. - callbacks/reporter: redact the user-supplied callback URLs. Addresses review comments: - #921 (comment) (@copilot-pull-request-reviewer) - #921 (comment) (@copilot-pull-request-reviewer) - #921 (comment) (@copilot-pull-request-reviewer) - #921 (comment) (@zeevmoney) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(redaction): give the loguru-diagnose test teeth The diagnose test raised a bare ValueError("boom") that never referenced the model, so loguru's diagnose never rendered it - the assertion held even for an unprotected model (a tautology). Raise ValueError(entry) so the whole model is rendered in the traceback. Deliberately do NOT reach into entry.config: loguru diagnose evaluates that sub-expression and renders the raw dict, the one vector the mixin documents it cannot cover (verified - the reviewer's suggested entry.config[...] variant leaks). Addresses review comments: - #921 (comment) (@zeevmoney) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(logging): correct SSH-only rationale for git trace gating provide_git_ssh_environment early-returns for non-SSH URLs, so it only ever runs for SSH clones - which use key auth, not HTTP Authorization headers. The comment (and the LOG_DIAGNOSE docstring) claimed gating GIT_TRACE/ GIT_CURL_VERBOSE here closes an HTTP Authorization-header leak; it does not (that path is HTTPS-only and is not handled here). Correct the comment, the config docstring and the test docstring to state the real value: reducing verbose SSH protocol/host disclosure in logs. Addresses review comments: - #921 (comment) (@zeevmoney) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(logging): scrub credentials at the loguru sink + LOG_DIAGNOSE note Third-party exceptions (most notably aiohttp ClientResponseError) render the full request URL - https://user:pw@host/path?token=... - inside the traceback loguru formats. The model-layer redaction can't reach those (the leaking object isn't an OPAL model), and this fires under default config (backtrace=True). Close the vector at the sink layer: - CredentialScrubbingStream wraps the stdout/stderr sink so the final formatted record (message + exception traceback, and the JSON blob under serialize=True) is run through redact_url_in_text before write. - A loguru patcher additionally scrubs record["message"] for every record, so the optional rotating file sink (which is path-based and can't be wrapped) still gets message-level scrubbing without losing rotation/retention. - Document that OPAL_LOG_DIAGNOSE now defaults to False (operators upgrading opt back in via OPAL_LOG_DIAGNOSE=true). Addresses review comments: - #921 (comment) (@zeevmoney) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style(logging): satisfy docformatter on scrubbing.py docstring CI fixes: - pre-commit / docformatter: reflow the CredentialScrubbingStream summary line so it conforms to docformatter (new file was missed locally because it was untracked when the formatters ran). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes Issue
Closes #901
Changes proposed
Check List (Check all the applicable boxes)