Skip to content

fix(scripts): broaden support bundle secret-key redaction denylist#4242

Open
DaoyuanLi2816 wants to merge 1 commit into
bytedance:mainfrom
DaoyuanLi2816:fix/support-bundle-secret-redaction-allowlist
Open

fix(scripts): broaden support bundle secret-key redaction denylist#4242
DaoyuanLi2816 wants to merge 1 commit into
bytedance:mainfrom
DaoyuanLi2816:fix/support-bundle-secret-redaction-allowlist

Conversation

@DaoyuanLi2816

Copy link
Copy Markdown
Contributor

Summary

scripts/support_bundle.py redacts secret-shaped config keys off a fixed keyword allowlist when walking arbitrary config dicts. This PR broadens that key-name match to mirror the wildcard denylist backend/packages/harness/deerflow/sandbox/env_policy.py already uses for sandbox env-scrubbing, closing a gap that was flagged on PR #3886's review before merge but not fully addressed.

Background

PR #3886 (which added the support-bundle generator) received a review from @willem-bd flagging this exact class of gap before merge: #3886 (review)

[P2] Key-name allowlist misses literal secrets under non-standard keys (esp. MCP env blocks)
... redaction of structured config keys on a fixed 9-keyword allowlist (api_key|token|secret|password|passwd|authorization|cookie|credential|private_key). When a key name is outside that list, the value falls through to redact_text ... A literal secret stored under a non-matching key name ... is emitted verbatim into config-summary.json / extensions-summary.json ...
Suggested fix: redact env (and headers/args) values by default rather than by key name ... Broaden the keyword set (key, pwd, auth, pat, access[_-]?key, encrypt, signing). A denylist-of-known-safe is safer than an allowlist-of-secret for a bundle marketed as public-safe.

The PR merged with that review comment unresolved (COMMENTED, not CHANGES_REQUESTED). The current main does already blanket-redact MCP env blocks and *header* dicts by default (covering the concrete example the review used, MCP servers' env block), but the underlying generic key-name allowlist that gates every other structured config path was never broadened to match.

The gap

GuardrailProviderConfig.config (backend/packages/harness/deerflow/config/guardrails_config.py) is a real, documented, open-ended dict of provider-specific kwargs — not an env or *header* block, so it never gets the blanket treatment. A custom guardrail provider configured with keys like db_pass, encryption_key, redis_pass, or webhook_signing_key has those values emitted verbatim into config-summary.json, while manifest.json in the same bundle declares "redacted_secret_fields": true.

I confirmed this against the real, unmodified create_support_bundle() (before writing any fix): a config.yaml with a guardrails.provider.config block containing those four keys produced a config-summary.json with every value present verbatim, while manifest.json's privacy block still claimed redaction.

The fix

Broaden SECRET_KEY_RE in scripts/support_bundle.py to add the same wildcard concepts env_policy.py uses for sandbox env-scrubbing — *KEY*/*SECRET*/*TOKEN*/*PASS*/*CREDENTIAL*/*DSN* — plus a small exact-name set mirroring its no-flag credential sources (GH_PAT/GITHUB_PAT/REDIS_AUTH/REDISCLI_AUTH/PGSERVICEFILE).

Direct import from the harness package wasn't used: scripts/ is a standalone, dependency-light root tool (it already runs with PyYAML optional and has no deerflow/app imports anywhere in the directory), so growing a runtime dependency on the harness package being installed just to reuse one denylist didn't seem worth it. The breadth is mirrored independently instead.

The new bare key/pass/dsn alternatives are boundary-guarded ((?<![a-zA-Z])key(?![a-zA-Z]), same shape for pass/dsn) so they match only their own delimited token, not an unrelated word that happens to start with the same letters. Two real, non-secret fields in this repo's own schema would otherwise have regressed:

  • extensions_config.json's MCP routing.keywords hint list (a real field in extensions_config.example.json)
  • config.yaml's guardrails.passport path/ID field

Both are covered by a new regression test and continue to render unredacted after this change.

Connection-string keys (DATABASE_URL, REDIS_URL, ...) are deliberately left out of the new exact-name set: their embedded credentials are already stripped in place by the existing URL_USERINFO_RE pass, which is more useful for a support bundle than blanking the whole field (it keeps host/port/db-name visible for diagnosis, already covered by an existing test). Adding them to a blanket-redact set would have regressed that existing, tested behavior.

Tests

  • test_redact_data_masks_secret_shaped_keys_in_arbitrary_provider_config — the guardrails.provider.config scenario above, including a SUPABASE_SERVICE_ROLE_KEY-shaped key (the concrete example from the feat(scripts): add redacted community support bundle generator #3886 review) and a gh_pat-shaped key, both outside any env/header wrapper.
  • test_redact_data_does_not_over_redact_lookalike_non_secret_keys — asserts routing.keywords and guardrails.passport still render unredacted.
  • test_create_support_bundle_masks_provider_config_secret_shaped_keys — end-to-end through the real create_support_bundle() zip output, also checks the manifest.json privacy claim.
  • Fail-before/pass-after: reverting only scripts/support_bundle.py via git apply -R on a saved patch (tests untouched) makes the 2 new leak tests fail with the literal secret values present instead of <redacted>; reapplying the patch makes all tests pass again.
  • tests/test_support_bundle.py full file: 29 passed (27 pre-existing + 2 new leak tests; the 3rd new test, the over-redaction guard, already passed before this fix since nothing over-redacted yet).
  • Broader regression sweep: full tests/ backend suite — 57 failed, 7534 passed, 52 skipped. All 57 failures are pre-existing and unrelated to this change (Windows-only symlink-privilege, hostPath-volume, macOS-specific, and POSIX file-permission tests failing on a Windows dev machine, in files this change never touches) — none reference support_bundle.
  • ruff check and ruff format --check (line length 240) are clean on both changed files.

SECRET_KEY_RE only matched a fixed keyword allowlist, so a secret stored
under an unanticipated key name inside an open-ended config dict (e.g.
guardrails.provider.config, an arbitrary provider-kwargs dict) was emitted
verbatim into config-summary.json even though manifest.json claims
redacted_secret_fields=true. This gap was flagged on PR bytedance#3886's review
before merge but not fully addressed.

Broaden the key-name match to mirror env_policy.py's wildcard denylist
(*KEY*/*SECRET*/*TOKEN*/*PASS*/*CREDENTIAL*/*DSN*) already used for sandbox
env-scrubbing, plus its no-flag credential exact names (GH_PAT/GITHUB_PAT/
REDIS_AUTH/REDISCLI_AUTH/PGSERVICEFILE). The new bare key/pass/dsn
alternatives are boundary-guarded so they match only their own delimited
token, not an unrelated word that starts with the same letters (routing
"keywords", guardrails "passport").
@github-actions github-actions Bot added area:ci GitHub Actions, CI config, repo tooling risk:medium Medium risk: regular code changes size/M PR changes 100-300 lines labels Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:ci GitHub Actions, CI config, repo tooling risk:medium Medium risk: regular code changes size/M PR changes 100-300 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant