fix: scrub credentials from malformed Splunk URLs - #75
Open
JacobPEvans-personal wants to merge 9 commits into
Open
fix: scrub credentials from malformed Splunk URLs#75JacobPEvans-personal wants to merge 9 commits into
JacobPEvans-personal wants to merge 9 commits into
Conversation
`safe_target` strips `user:password@` from the URL that gets printed in prompts, JSON metadata, transport errors, and the audit log. It receives that URL exactly as typed, before anything validates that it parses, and four shapes reached the output intact: a truncated authority, a missing scheme, a credential after a path separator, and an unterminated IPv6 literal that raised with the value in the traceback. Redaction now fails closed. A target whose host cannot be read is replaced rather than repeated, and the same rule covers the path. A target carrying no credential still prints in full, so an ordinary error message stays readable. A fuzz harness checks the property on every pull request that touches code, on Linux only — atheris ships manylinux x86_64 wheels alone, so it stays out of the `dev` extra and pytest does not collect it. The three integration workflows install from a hash-pinned lock, and weekly dependency updates keep both that lock and the commit-pinned actions current. The contributor install is unchanged. Assisted-by: Claude:claude-opus-5[1m] Claude-Session: https://claude.ai/code/session_01Ji4H9nfoc3zaoegLSMXEdL
There was a problem hiding this comment.
Pull request overview
This PR strengthens URL credential redaction in vct_splunk.core.redact.safe_target so malformed Splunk targets can’t leak user:password@ in prompts/logs/errors, and adds continuous regression coverage via both example-based tests and a CI-run fuzz harness. It also hardens CI dependency installation by switching integration workflows to hash-pinned lockfiles and adds Dependabot coverage for both Python and commit-pinned GitHub Actions.
Changes:
- Make
safe_targetfail closed on malformed/unrebuildable targets (and handleurlsplitfailures) to prevent credential echoing. - Add regression tests plus a Linux-only atheris fuzz harness, wired into CI as a conditional PR job.
- Introduce hash-pinned lockfiles for CI installs (
--require-hashes+--no-deps) and configure weekly Dependabot updates.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_secret_redaction.py | Adds regression coverage for malformed targets and ensures credential-free targets remain readable. |
| tests/TESTING.md | Documents the new fuzz test group and updates CI group descriptions. |
| tests/fuzz/fuzz_redact.py | Adds atheris fuzz harness asserting safe_target never leaks the sentinel or raises. |
| src/vct_splunk/core/redact.py | Updates safe_target to fail closed and redact path cases where credentials can land outside userinfo. |
| requirements-fuzz.txt | Adds a hash-pinned lock for the fuzz job dependencies. |
| requirements-fuzz.in | Defines the fuzz lock input set (includes project + atheris). |
| requirements-ci.txt | Adds a hash-pinned lock for CI integration workflow dependencies. |
| requirements-ci.in | Defines the CI lock input set (includes project + pytest). |
| pyproject.toml | Excludes tests/fuzz from pyright due to atheris wheel/platform constraints. |
| flake.nix | Adds uv to the dev shell for regenerating the lockfiles. |
| CONTRIBUTING.md | Documents CI’s lockfile-based install and how to regenerate locks after dependency edits. |
| CHANGELOG.md | Records the redaction fix, fuzz coverage, Dependabot, and lockfile-based CI installs. |
| .github/workflows/cloud-read.yml | Switches workflow install to requirements-ci.txt with --require-hashes + project --no-deps. |
| .github/workflows/ci.yml | Switches enterprise job install to the CI lockfile, adds the fuzz job, and wires it into Merge Gate. |
| .github/workflows/acs-contract.yml | Switches workflow install to requirements-ci.txt with --require-hashes + project --no-deps. |
| .github/dependabot.yml | Adds weekly grouped Dependabot updates for GitHub Actions and pip ecosystems. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JacobPEvans-personal
force-pushed
the
chore/scorecard-hardening
branch
from
August 5, 2026 12:57
5138170 to
3441379
Compare
Resolves the ci.yml restructure from #72: the fuzz job and Merge Gate now reference the local `gate` and `test` jobs that replaced the reusable Python workflow. The hash-pinned lock stays on the three jobs that run against a live server or Splunk's public API; `gate` and `test` keep resolving `.[dev]` live, which is what #72 set out to exercise. Assisted-by: Claude:claude-opus-5[1m] Claude-Session: https://claude.ai/code/session_01Ji4H9nfoc3zaoegLSMXEdL
A credential can sit in `?token=` or `#token=` as readily as in userinfo, and neither carries the `@` the userinfo rule keys on. The fallback that returns a target it cannot rebuild therefore echoed both back. It now truncates at the first query or fragment delimiter first, so every path drops them, matching what the rebuilt target already did. The fuzz harness could not have found this: it spliced the marker into userinfo alone, so every generated target carried an `@` and took the userinfo branch. Two of its three shapes now omit both `//` and `@`, which is what reaches the fallback. Against the previous implementation the widened harness fails on two thirds of inputs; against this one it is clean over four seeds. Assisted-by: Claude:claude-opus-5[1m] Claude-Session: https://claude.ai/code/session_01Ji4H9nfoc3zaoegLSMXEdL
The query and fragment shapes reach the fallback and return before the authority is parsed, so splitting the budget evenly across all three cost coverage: edges fell from 223 to 181 and the corpus from 78 to 52 over the same 60 seconds. Half the budget goes back to the userinfo shape, which is the one that exercises parsing and rebuilding. The query leak is still caught on half of all inputs against the implementation that had it, and four seeds are clean against this one. Assisted-by: Claude:claude-opus-5[1m] Claude-Session: https://claude.ai/code/session_01Ji4H9nfoc3zaoegLSMXEdL
…edential slot CI caught a crash: `safe_target` returned a string containing the marker password, which looks like a leak. It was not one — libFuzzer's coverage-guided mutator extracts the marker as a useful byte string (it appears as a `DE:` dictionary entry) and splices it wherever the provider consumes bytes, including the fields that build the host, not only the position meant to hold the credential. `safe_target` correctly left that host-shaped text visible and correctly stripped the actual `#token=` fragment next to it; the oracle just checked for the marker in the wrong scope. Every fuzzer-controlled field is now scrubbed of the marker before assembly, so it appears only where a shape deliberately places it as the credential. Confirmed against the exact crashing input, and re-verified over 360,000 generated inputs across six seeds with zero failures against the current code and the original leak still caught on two thirds of inputs against the pre-fix implementation. Assisted-by: Claude:sonnet-5 Claude-Session: https://claude.ai/code/session_01Ji4H9nfoc3zaoegLSMXEdL
Applied from a /simplify pass over the full PR diff: - safe_target's fallback (query/fragment strip, userinfo check) ran on every call, including the well-formed common case, though it is only ever returned from the two malformed-input branches. Moved into a _fallback() helper called lazily from those branches instead. - The fuzz generator drew scheme and user unconditionally before knowing which of the four target shapes it was building, wasting two _fuzzed() calls (and the fuzzer-byte budget they consume) on the two shapes that never use them. The shape draw now happens first, and scheme/user are only consumed on the branch that needs them. - Documented the actual invariant behind hash-pinning three of five CI jobs' installs: it tracks which jobs carry live secrets, not merely which ones happen to be secondary to CONTRIBUTING.md's documented install-path check. Re-verified after both changes: safe_target's 12 known cases are byte-identical to before the refactor, and the fuzz oracle is clean across 480,000 generated inputs over 8 seeds while still catching the pre-PR leak on two thirds of inputs. Assisted-by: Claude:sonnet-5 Claude-Session: https://claude.ai/code/session_01Ji4H9nfoc3zaoegLSMXEdL
CI failed again on the same false positive, in a shape the last fix didn't cover. The sentinel is 31 characters; every fuzzed field caps at 24 or 12, so it can never fit whole inside one field regardless of per-field scrubbing — both real crashes were the mutator splitting it across the boundary between two adjacent draws, and host/tail is the only such boundary in this generator with nothing fixed between them. Per-field scrubbing was blind to that by construction. host and tail are now drawn raw and joined before scrubbing runs once on the combined string, which catches a split landing anywhere in it. The previous fix's "0 failures across N seeds" claim was weaker evidence than it looked: the local stub generates uniform random Unicode, which does not reach the specific character-for-character splice a coverage-guided mutator finds deliberately — the search space makes it produce that arrangement by chance. Verified instead with a scripted FDP that runs the real build_target()/TestOneInput() code (not a reimplementation) with the sentinel deliberately split at every position across the host/tail boundary, for both the userinfo and the query/fragment shapes, plus both exact crash inputs CI reported. All clean. Assisted-by: Claude:sonnet-5 Claude-Session: https://claude.ai/code/session_01Ji4H9nfoc3zaoegLSMXEdL
JacobPEvans-personal
changed the title
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.
Summary
A fuzz harness written to satisfy the OpenSSF Scorecard's Fuzzing check uncovered a real credential-leak bug in
core.redact.safe_target: URLs with malformed hosts would be returned verbatim, leaking any embedded credentials into logs, JSON output, and audit trails. The fix hardens the function to fail closed on any target it cannot parse and rebuild, and applies the same redaction to the path component. The fuzz harness itself required two rounds of fixes after CI runs caught false positives in its own generator logic—a normal part of fuzzing validation that improved the test's reliability.Changes
?token=/#token=query or fragment, an unterminated IPv6 literal that raised with the value in the traceback, and a credential landing in the URL path when a stray/ends the authority.tests/fuzz/fuzz_redact.py): Generates malformed targets around a marker credential and asserts the marker never leaks and calls never raise. Runs on every PR touching code; Linux-only (atherisships manylinux wheels alone). Excluded from thedevextra and pytest collection.requirements-ci.in/txt,requirements-fuzz.in/txt): Used by the three integration workflows that touch live servers or secrets (enterprise,acs-contract,cloud-read). Installed with--require-hashes; the project installs separately with--no-deps. The contributor install (pip install -e ".[dev]") remains unchanged, resolving dependencies live per PR ci: install with venv and pip instead of a separate installer #72's design..github/dependabot.yml): Weekly updates for Python packages and commit-pinned GitHub Actions (everyuses:here is now pinned to a SHA).Test Plan
/simplifypass applied to the cumulative diff.