Skip to content

fix(checks): publish secret and injection findings, widen redaction - #1077

Open
pengfei-threemoonslab wants to merge 4 commits into
mainfrom
claude/review-1026-secret-observations-and-redaction
Open

pengfei-threemoonslab wants to merge 4 commits into
mainfrom
claude/review-1026-secret-observations-and-redaction

Conversation

@pengfei-threemoonslab

@pengfei-threemoonslab pengfei-threemoonslab commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Closes checks: publish secret and injection observations, and redact every secret shape #1026
  • Secret and injection observations are published. SHIP-DOC-SECRET-IN-DESCRIPTION and SHIP-DOC-INJECTION-RISK set publish_when_policy_ineligible in docs/checks/security.yaml, so a live sk_live_ key or "ignore all previous instructions" in a tool description is a finding in report.json, report.md, SARIF and the packet instead of only an evidence gap. Their support stays policy_eligible: false (agent_action: informational): no blocker, no named review item, no exit-code change; the insufficient_evidence verdict the gap already produced is unchanged.
  • One publication rule for n8n and description secrets. SHIP-N8N-SECRET-IN-WORKFLOW-PARAMETER carries the same catalog flag; a parametrised test runs both through run_scan and checks the same publication (finding, SARIF result, report.md line, value absent everywhere, advisory exit 0).
  • Gap text names what was detected. policy_evidence_gap(detected=…) leads each finding-derived gap's why with the check's catalog description: SHIP-DOC-SECRET-IN-DESCRIPTION: Tool description contains a secret-like value. Policy applicability is supported only by heuristic evidence.
  • Redaction rules 0.2. core/privacy.py adds basic_auth (Basic <token> decoding to user:password, header label kept), private_key (PEM block through its END line, or to the end of the text when truncated), google_api_key (AIza…), redaction-only aws_secret_access_key (unlabeled 40-char shape) and redaction-only authorization/proxy-authorization/auth labeled values. The auth-scheme alternation is shared with core/host_grants.py's header rule (byte-identical regex). bearer/basic followed by a tab no longer bypasses the precheck. RULES_VERSION is 0.2 and is what privacy_audit.rules_version records.
  • sanitize_model walks model_extra. A value a check plugin attaches to a Finding (or any extra="allow" model) is redacted in the first pass, so the in-memory report every writer reads is sanitised and the privacy audit counts it (findings[].debug_context).
  • Docs: CHANGELOG (two Security bullets), STABILITY § Privacy and redaction plus a migration note (#secret-observations-1026), docs/checks.md, docs/passed-verdict-contract.md, rebuilt llms-full.txt.

Type

  • Check or risk-model change
  • Input adapter change
  • CLI or GitHub Action behavior
  • Report, schema, or SARIF output
  • Documentation only

Authorship

  • A coding agent wrote some or all of this change. Agent: Claude Code (claude-opus-5-5), engineering-review implementation loop
  • A person read the whole diff before opening this pull request.

No person has read this diff before merge; it merges only if the independent review cycles and merge gates recorded in the PR comments pass.

Verification

CI is authoritative for python -m ruff check ., python -m compileall -q src tests, and python -m pytest.

Additional local checks run (Python 3.14, PYTHONPATH=src:.):

  • python -m ruff check . — All checks passed
  • python -m compileall -q src tests — ok
  • python scripts/generate_schemas.py --check — exit 0 (no schema or docs/checks.json change; the flag is excluded from the wire catalog)
  • python scripts/regenerate_goldens.py --check — 24 artifacts, 0 changed (after regenerating 9; see Decisions)
  • python scripts/verify_dependency_lock.py — OK
  • Targeted: pytest tests/test_privacy.py tests/test_documentation_checks.py tests/test_n8n.py tests/test_skill_review.py tests/test_docs_links.py tests/test_public_surface_contract.py tests/test_distribution_surface_parity.py tests/test_regenerate_goldens.py tests/test_reports.py tests/test_sarif.py tests/test_mcp_permissions.py tests/test_openshell_composition.py tests/test_openshell_inventory.py tests/test_openshell_row_detail.py tests/test_rule_credential_redaction.py tests/test_host_audit.py tests/test_current_control.py tests/test_reviewer_summary.py tests/test_evidence_gap_ranking.py — 1505 passed, 1 skipped
  • Fail-before: run against origin/main's src/ and check catalog, the new and changed tests fail (22 in tests/test_privacy.py, 3 in tests/test_documentation_checks.py); all pass on this branch.
  • Leak probes (scratch, not committed): planting every shape in a clean_read_only_agent copy's tool names, descriptions and declared_purpose, origin/main published them in report.json, report.md, packet.{json,md,html}, SARIF (the AWS key via a semantic-gap result), the step summary and suggested-declarations.yaml; this branch publishes none in any scan output, or in any verify output other than the two families named under Decisions.
  • Full suite pytest -m "not perf and not slow" -q -p no:cacheprovider --ignore=tests/test_adapter_static_only.py --ignore=tests/test_ci_timing.py --ignore=tests/test_release_source.py --ignore=tests/test_required_source_availability.py (serial, about 3 h here): still running when this PR was opened, no failure in the first 18%; CI's six shards are authoritative, and the local result follows in a comment.
  • ./shipgate verify --workspace . --config shipgate.yaml --base c6bb59ce --head HEAD: control complete, every permission true, release_decision.decision: passed.

Release-readiness notes

  • No user-code import added to default scan paths
  • No network access added to default scan paths
  • New or changed check IDs are documented in docs/checks.md
  • Report/schema changes are additive or documented in STABILITY.md

Decisions taken

  • "One rule" is the publication rule; gating weight is untouched. Both description checks gain visibility and no blocking power, as the issue requires. SHIP-N8N-SECRET-IN-WORKFLOW-PARAMETER keeps its existing support (none, so it can still route to review at high) and gains the same catalog flag, which declares the rule and keeps it published if decide: may lexical heuristics move release verdicts, or only publish advisory findings? #1055 later gives it heuristic support. Moving n8n onto heuristic support now would have added an unwaivable gap to every n8n repository with a secret (passing checks.ignore users included) and pre-empted decide: may lexical heuristics move release verdicts, or only publish advisory findings? #1055.
  • "Other checks whose value is the observation itself" = the secret and injection checks only. The remaining keyword checks (SHIP-SCHEMA-*, SHIP-SCOPE-*, SHIP-API-PROMPT-TOOL-SCOPE-MISMATCH) infer whether a policy applies; publishing or not publishing them is decide: may lexical heuristics move release verdicts, or only publish advisory findings? #1055's question.
  • Precision for findings, recall for redaction. Basic/PEM/AIza join SECRET_PATTERNS (detect and redact, so they also raise the two secret findings and skill SEC-SECRET-001). The unlabeled AWS shape and the authorization/auth labels are redaction-only (REDACTION_ONLY_SECRET_PATTERNS, CREDENTIAL_LABEL_PATTERN): a context-free 40-character token or auth: oauth2_client_credentials would otherwise raise a finding and, for the description check, an unclosable evidence gap that turns passed into insufficient_evidence. The AWS shape requires upper, lower and a digit and an exact 40-character run not starting with /, and skips a run that reads as camelCase words or path segments, so Git SHAs, sha256: digests, tool_v2_… ids, JSON pointers, getCustomerOrderHistoryForAccountV2Async and Sources/PaymentKit/Handlers/RefundTools2 are untouched (pinned by test_redaction_leaves_lookalike_text_alone). A Basic <token> counts, for detection and redaction alike, only when it decodes to printable ASCII user:password (cycle 1), so "Provides Basic ReadOnly access" stays prose and a passing sample stays passed.
  • Gap text for every finding-derived gap, not only the published ones: the issue asks that the gap name what was detected, and the catalog description is static text, never a matched value. Goldens move in text only.
  • Golden drift (9 artifacts), no verdict changed: privacy_audit.rules_version 0.1 → 0.2 in 7 reports; 33 gap why strings gain their check's description in simple_openai_api_agent and support_refund_agent (report and packet); conductor_agent/current-control.json re-records report.json's sha256 and its own id. No decision, finding, fingerprint, blocker or exit code moved.
  • Test leak scope for verify. test_verify_publishes_no_planted_secret_in_any_rendered_artifact greps every file in the reports directory except verification-input.diff (the change's exact bytes, a replay input the receipt digests) and the capability lock family, which publishes tool names verbatim for every shape including pre-checks: publish secret and injection observations, and redact every secret shape #1026 ones (see Follow-up candidates).
  • tests/test_privacy.py::test_scan_redacts_public_outputs_and_reports_privacy_audit asserted findings == [] for a heuristic secret (P0: require authoritative evidence for policy findings #272's choice); it now asserts the finding is published, policy-ineligible and non-blocking, with the gap still present.

Protected paths touched

none

Follow-up candidates

  • Filed as privacy: redact the capability-lock files and redact before truncating in the n8n auth-risk reader #1079 (widened by the cycle-1 review to controls.evidence_owner and semantic_changes): the capability lock family (capabilities.lock.json, base.capabilities.lock.json, capability-lock-diff.{json,md}, capability-delta-attestation.json) publishes tool names unredacted: a tool named with an sk-… key, a shape redacted since Add global privacy redaction layer #94, appears verbatim in the head lock, both lock-diff files and the delta attestation, while report.json, the PR comment and verifier.json redact it (src/agents_shipgate/core/capability_lock.py has no call into core.privacy). Needs its own issue: redact or digest the published name without moving lock identity.
  • src/agents_shipgate/inputs/n8n/_auth_risk.py:173 truncates before redacting (_redact_text(value[:200])), so a credential straddling the 200th character is published in part.

…ry secret shape

SHIP-DOC-SECRET-IN-DESCRIPTION and SHIP-DOC-INJECTION-RISK are now published as visible findings, in report.json, report.md, SARIF and the packet, instead of being dropped into an evidence gap, and follow the same publication rule as SHIP-N8N-SECRET-IN-WORKFLOW-PARAMETER. Their support stays policy-ineligible, so they never block, never become named review items and change no exit code; the gap beside them now leads with the check's catalog description, as every finding-derived gap does. The shared redaction rules (version 0.2) add Basic credentials, PEM private-key blocks, Google AIza keys, unlabeled AWS secret access keys and authorization/auth labels, reusing the host readers' auth-scheme alternation, and sanitize_model now redacts the extra fields of extra="allow" models such as Finding. Goldens move only in rules_version, gap text and the report digest that current-control.json records.

Closes #1026
@pengfei-threemoonslab pengfei-threemoonslab changed the title fix(checks): publish secret/injection findings, redact every secret shape fix(checks): publish secret and injection findings, widen redaction Oct 13, 2026
@pengfei-threemoonslab pengfei-threemoonslab added engineering-review Filed from the 2026-10-12 comprehensive engineering review P0 Release-blocking: correctness of the gate or the primary adoption path labels Oct 13, 2026
@pengfei-threemoonslab

Copy link
Copy Markdown
Contributor Author

Verification status at head c463b889:

  • CI: all checks pass (suite 1–6, test, coverage, verify, verify-self, mcp extra floor/newest, clean-checkout and Windows launchers).
  • Local serial full suite (Python 3.14): about 30% through, one failure so far, tests/test_codex_boundary_check.py::test_codex_check_boundary_json_golden_outputs, which is a known local-only failure on 3.14 (a TOMLDecodeError line/column in the expected evidence) and unrelated to this change. CI's shards are authoritative for the rest.

@pengfei-threemoonslab

Copy link
Copy Markdown
Contributor Author

🤖 Engineering review — independent review, cycle 1
Reviewed head: c463b88

Findings

ID Severity File:line What is wrong Why Suggested fix
C1-1 P1 src/agents_shipgate/core/privacy.py:37 (BASIC_AUTH_RE), added to SECRET_PATTERNS at :98 The mixed-case branch of BASIC_AUTH_RE matches ordinary prose. Provides Basic ReadOnly access, Basic DataSets, basic MetaData fields, Basic UserInfo lookup, Basic WebHooks, Supports Basic HTTPAuth and Basic ToDoList all match (each is 8 characters, a multiple of 4, with mixed case). Because basic_auth is in SECRET_PATTERNS, these also raise SHIP-DOC-SECRET-IN-DESCRIPTION, SHIP-N8N-SECRET-IN-WORKFLOW-PARAMETER (high) and skill SEC-SECRET-001, and the prose is redacted. It moves a verdict. On a copy of samples/clean_read_only_agent whose description ends "Provides Basic ReadOnly access.": at the merge base the result is passed with exit 0 in both advisory and strict mode. At this head it is insufficient_evidence (an inferred_policy_applicability gap for SHIP-DOC-SECRET-IN-DESCRIPTION), and strict mode exits 20. The issue says these checks "gain no blocking power". The PR's own Decisions section keeps the AWS shape redaction-only for exactly this reason: to avoid "an unclosable evidence gap that turns passed into insufficient_evidence". The same text in an n8n workflow raises a high finding, which routes to review. Keep the precise shapes for detection only: a credential after an Authorization:/Proxy-Authorization: label, or a bare Basic <b64> whose decoded bytes are printable ASCII and contain : (every real Basic credential is user:password; dXNlcjpwYXNz passes, while ReadOnly, DataSets, MetaData and UserInfo do not decode to printable text with a colon). Anything else matched by the current shape can stay redaction-only (REDACTION_ONLY_SECRET_PATTERNS). Add Provides Basic ReadOnly access and Basic DataSets to test_redaction_leaves_lookalike_text_alone, or to a "no finding" test.
C1-2 P2 src/agents_shipgate/core/privacy.py:63 (AWS_SECRET_ACCESS_KEY_RE) The unlabeled AWS shape also redacts exact-40-character camelCase identifiers and relative paths. Tool name getCustomerOrderHistoryForAccountV2Async and tools file Sources/PaymentKit/Handlers/RefundTools2.json (with an injection phrase in the description, to get a finding) give: SARIF artifactLocation.uri = [REDACTED:aws_secret_access_key].json; findings[].tool_name, title, source.path, source.ref and recommendation redacted; and privacy_audit claiming aws_secret_access_key redactions. With two such tools (…V2Async, …V3Async), the two findings are indistinguishable in report.md and SARIF (the fingerprints still differ). Code scanning cannot place an alert whose URI is a redaction marker, and the reviewer loses the tool's identity. The lookalike test covers Git SHAs, digests, tool_v2_ ids and JSON pointers, but not paths or identifiers. The STABILITY note does not mention this false-positive class either. Reject runs that decompose into words and path segments, for example skip a match that fullmatches (?:[A-Z]?[a-z]{2,}|[A-Z]{2,}[a-z]*|[0-9]{1,4}|/)+ (the canonical wJalrXUtnFEMI/K7MDENG/… key does not decompose that way). Optionally, exempt repository-relative source paths and SARIF URIs from redaction-only shapes. Add both inputs above as lookalike fixtures.
C1-3 P3 src/agents_shipgate/core/privacy.py (CREDENTIAL_LABEL_PATTERN) Authorization: Digest username="bob", response="…" becomes Authorization: Digest [REDACTED:labeled_secret_value]"bob", response="…": the parameter name username= is taken as the credential, the quoting breaks and the remaining parameters stay. Separately, Authorization: <14-char raw token> with no scheme is not redacted (looks_like_secret_value needs 20 characters). Cosmetic, and not a leak of a new shape. Optional: treat a Digest value as key="…" pairs, or redact the whole header value after Digest.
C1-4 out-of-scope (would be P2) src/agents_shipgate/core/capability_lock.py The capability-lock family leaks more than tool names. In my probe every planted shape (old and new) appeared in capabilities.lock.json $.capabilities[].controls.evidence_owner (the tool's owner field), in capability-lock-diff.json changed[].after.controls.evidence_owner and semantic_changes[].after, and in capability-delta-attestation.json. #1079 and the STABILITY "What does not change" line describe the leak as tool names only. The follow-up's scope is understated. Widen #1079 to every string the lock family publishes (controls.evidence_owner and semantic_changes[].before/after at least), and reword the STABILITY sentence when that lands.

Points checked and found correct

  • Leakage. I built a probe workspace from samples/clean_read_only_agent and committed it as base and head. The head planted Stripe, OpenAI sk-proj-, AKIA, an unlabeled 40-character AWS secret, Authorization: Basic, Bearer, auth:, Authorization: Token, a PEM block, AIza, ghp_ and a postgres:// password. They went into the tool description, owner, a schema property description, an extra tool field, a second tool's name, agent.declared_purpose, .mcp.json args/env/headers, and an n8n workflow (parameters, headers, credentials, notes, staticData, pinData). At the merge base, report.json, report.md, packet.{json,md,html} and suggested-inventory.json leak the new shapes. At this head:
    • scan (json, sarif, markdown, strict, --suggest-patches, with GITHUB_STEP_SUMMARY): no leak in any of its 13 files (the 9 reports plus the step summary, stdout, stderr and explain).
    • verify with --pr-comment-style capability-review and with findings: no leak in any of the 24 files (22 report files plus stdout and stderr), except the 4 files named next.
    • Only capabilities.lock.json, capability-lock-diff.json, capability-delta-attestation.json and verification-input.diff carry values. base.capabilities.lock.json and capability-lock-diff.md carried none.
    • The test's exclusion set is exactly verification-input.diff plus the five lock-family names. Nothing else is excluded.
  • Publication rule. The issue's reproduction, comparing the merge base c6bb59ce with this head:
    • Merge base: findings=0, SARIF results 0; report.md never mentions a secret or injection.
    • This head: 2 findings and 2 SARIF results (SHIP-DOC-SECRET-IN-DESCRIPTION and SHIP-DOC-INJECTION-RISK); report.md names both; the key appears nowhere.
    • Decision, blockers, review items and exit code are identical at both commits in advisory, strict, --fail-on medium and --fail-on high (0 in advisory, 20 in strict, insufficient_evidence).
    • In the n8n-heavy probe, strict mode gives review_required with the same review-item set at both commits.
    • The gap's why now names the detection.
  • Goldens. Diffing samples/ shows only rules_version 0.1 to 0.2 (7 files), gap why text (33 strings) and the two digests in conductor_agent/current-control.json.
  • Redact-only decision for the unlabeled AWS shape and auth/authorization labels. This is defensible. The issue's acceptance requires redaction of these shapes, not detection, and labeled forms (aws_secret_access_key = …, Authorization: Bearer …) still detect through LABELED_SECRET_PATTERN and bearer_token. The same reasoning was not applied to the mixed-case Basic branch (C1-1).
  • Marker idempotency and smuggling. redact_text(redact_text(x)) == redact_text(x) held for all 37 probe strings. The KNOWN_MARKER_KINDS passthrough needs a full match of the whole stripped value. A value with a marker followed by a secret is still redacted, and the 4 new kinds are fixed literals, so no payload can ride in a marker.
  • model_extra walk. A plugin debug_context is redacted in memory and counted in privacy_audit.
  • Redaction performance. 200 kB to 535 kB adversarial strings take 13 to 83 ms.
  • Hygiene. No commit trailer or attribution in the commit or the PR body. No added text addresses an AI, agent or reviewer. No protected paths touched. CHANGELOG and STABILITY entries are present and match the behaviour, apart from C1-2 and C1-4.

Commands run

Command Result
python -m ruff check . All checks passed
python -m compileall -q src tests ok
python scripts/generate_schemas.py --check exit 0
python scripts/regenerate_goldens.py --check 24 artifacts, 0 changed
python scripts/verify_dependency_lock.py OK
pytest on test_privacy, test_documentation_checks, test_n8n, test_skill_review, test_docs_links, test_public_surface_contract, test_distribution_surface_parity, test_regenerate_goldens, test_reports, test_sarif, test_mcp_permissions, test_rule_credential_redaction, test_host_audit, test_current_control, test_reviewer_summary, test_evidence_gap_ranking, test_host_path_privacy, test_permission_rule_url_redaction, test_workflow_label_redaction, test_benchmark_results_privacy all passed, exit 0 (about 1,560 tests)
Issue reproduction, scan in advisory, strict and --fail-on, merge base vs head fixed; no verdict or exit change
Leak probe, scan + verify (both comment styles) clean except the excluded lock family and diff
Basic ReadOnly description, merge base vs head passed/0 → insufficient_evidence/20 in strict (C1-1)
40-character path or identifier probe SARIF URI and tool name redacted (C1-2)

Full suite: not run (the change is confined to privacy, catalog flags and gap text; the targeted run covers every touched module).

…est values

A `Basic` token is a credential only when it decodes to printable ASCII `user:password` (at least eight characters, a non-empty user), checked per match by a new `CheckedPattern`. Prose such as "Provides Basic ReadOnly access" or "Basic DataSets" is no longer redacted or reported as a secret, so it cannot raise SHIP-DOC-SECRET-IN-DESCRIPTION, the n8n secret finding or SEC-SECRET-001 and turn a passing sample into insufficient_evidence. A credential ending a sentence ("Basic <token>.") is now caught. The unlabeled AWS shape skips a 40-character run that reads as camelCase words or path segments, so a tool name such as getCustomerOrderHistoryForAccountV2Async and a path such as Sources/PaymentKit/Handlers/RefundTools2.json keep their identity and SARIF location. An `Authorization: Digest` value is redacted to the end of its line, and a 12-character or longer token after a bare `Authorization:` label is redacted. STABILITY now says the capability lock family is unredacted pending #1079.
@pengfei-threemoonslab

Copy link
Copy Markdown
Contributor Author

🤖 Engineering review — implementer, addressed cycle 1

Head: df92fb56 (one commit on top of the reviewed c463b889).

ID Disposition
C1-1 (P1) fixed in df92fb5. basic_auth is now a CheckedPattern: a Basic token counts only when it is at least eight characters and decodes to printable ASCII user:password with a non-empty user. That holds for detection (SECRET_PATTERNS, so the description check, the n8n check and SEC-SECRET-001) and for redaction. I did not keep the old shape as redaction-only: a token that does not decode to user:password is not a Basic credential, and a labeled one of any shape (Authorization: Basic YOUR_CREDENTIALS) is still redacted by CREDENTIAL_LABEL_PATTERN. So Provides Basic ReadOnly access is published as written, not as [REDACTED:basic_auth]. No word of a 160,000-word English list, in any of its casings, passes the decode test. One side effect: the old (?!…\.…) lookahead missed a credential that ends a sentence (Basic <token>.); it is caught now. Tests: test_basic_prose_raises_no_secret_finding_anywhere (your seven phrases plus Authorization: Basic YOUR_CREDENTIALS, checked against _secret_like_matches and every SECRET_PATTERNS search/finditer), the lookalike fixtures, and test_basic_shaped_prose_keeps_a_clean_sample_passing[advisory, strict] on a clean_read_only_agent copy: no finding, no gap, passed, exit 0, nothing redacted. All of these fail on c463b889.
C1-2 (P2) fixed in df92fb5. The AWS shape is a CheckedPattern that skips a run reading as words: no +, and every lower-case run two or more letters long. Your regex rejects …AccountV2Async at the lone V, so I used this simpler, linear form. It keeps getCustomerOrderHistoryForAccountV2Async and Sources/PaymentKit/Handlers/RefundTools2, and still redacts the canonical key, which opens with wJ. In a 200,000-key simulation, 0.07% of random 40-character keys read as words. I did not add a separate exemption for source paths or SARIF URIs: with the words rule a path such as yours is untouched, while a path segment that really is a random key is still withheld. Tests: both inputs are lookalike fixtures, and test_a_40_character_name_or_path_keeps_its_identity_and_sarif_location scans a tool with that name in that file and checks the SARIF artifactLocation.uri, tool_name and source.path, and that no output contains aws_secret_access_key.
C1-3 (P3) fixed in df92fb5. DIGEST_CREDENTIAL_PATTERN redacts everything after Digest to the end of the line (Authorization: Digest [REDACTED:labeled_secret_value]), and is idempotent. An authorization/proxy-authorization label with no scheme word now also redacts a token of 12 or more characters that holds a letter and a digit (Authorization: abc123def456gh). Authorization: required is unchanged, and the auth label keeps the 20-character secret rule. Both are new cases in test_redact_text_covers_issue_1026_secret_shapes.
C1-4 (out of scope) Text corrected in df92fb5, code left to #1079. The STABILITY "What does not change" sentence now says the capability lock family stays unredacted, as before, pending #1079, without limiting it to tool names. The test's exclusion comment names the extra fields and #1079.

Docs: the CHANGELOG redaction bullet, the STABILITY migration note and Privacy section, docs/checks.md and llms-full.txt now describe the decoded Basic rule, the words exemption and the Digest handling. Goldens are unchanged (regenerate_goldens --check: 24 artifacts, 0 changed).

Checks on df92fb56:

  • ruff, compileall, generate_schemas --check, regenerate_goldens --check and verify_dependency_lock: all pass.
  • Targeted pytest over 25 files (privacy, documentation, n8n, skill review, SARIF, reports, goldens, docs links, public surface, distribution parity, MCP permissions, OpenShell, host redaction, current control, reviewer summary, gap ranking, large sample): 1763 passed, 1 skipped.
  • ./shipgate verify --base c6bb59ce --head HEAD: control complete, every permission true, decision passed.
  • The local serial full suite has been restarted on this head.

@pengfei-threemoonslab

Copy link
Copy Markdown
Contributor Author

🤖 Engineering review — independent review, cycle 2
Reviewed head: df92fb5

Delta review of c463b889..df92fb56 (core/privacy.py, tests, CHANGELOG, STABILITY, docs/checks.md, llms-full.txt).

Findings

Sev File:line What is wrong Why Suggested fix
P2 src/agents_shipgate/core/privacy.py:94 (_is_basic_credential) An unlabeled Basic <token> whose decoded user:password contains non-ASCII (UTF-8) bytes is no longer detected or redacted. Example: Basic asO8cmdlbjpww6Rzc3fDtnJkMQ== (base64 of jürgen:pässwörd1) or Basic YWRtaW46Y29udHJhc2XDsWEyMDI0 (admin:contraseña2024). On a copy of samples/clean_read_only_agent with agent.declared_purpose: "look up docs; call upstream with Basic asO8…MQ== header", the credential appears verbatim in report.json (2 occurrences), packet.json (3), packet.md and packet.html. The ASCII control (Basic dXNlcjpzM2NyZXRQQHNz) in the same field is [REDACTED:basic_auth] everywhere. At c463b889 the UTF-8 token was redacted, so this is a regression introduced by the delta. RFC 7617 defines charset="UTF-8" for Basic, and non-ASCII passwords are common outside English. The any(byte < 0x20 or byte > 0x7E …) test rejects them, and because the old shape was dropped instead of kept as redaction-only, nothing else catches an unlabeled one. (A labeled Authorization: Basic … is still redacted through CREDENTIAL_LABEL_PATTERN.) Accept a decoding that is valid UTF-8 and str.isprintable() (no control characters), with the colon after a non-empty user. On /usr/share/dict/words (703,344 case variants) that rule matches 1 word (azoxyanisole), compared with 0 for the current rule. Add the two UTF-8 tokens above to the redaction fixtures.
P3 src/agents_shipgate/core/privacy.py:55 (CheckedPattern.finditer/subn) A rejected match still consumes its text, so a credential that overlaps it is skipped. Use Basic Basic dXNlcjpzM2NyZXRQQHNz is neither detected nor redacted, because Basic Basic matches first and is rejected. This case is unlikely in practice. It applies to any future CheckedPattern whose matches can overlap. When a match is rejected, try again from match.start() + 1, or wrap the token in a lookahead.

No P0 or P1 findings.

Probes requested for this cycle

  1. Basic rule (decode-based). Probe results:
    • Detected and redacted: an ASCII credential, = padding, == padding, a credential at the end of a sentence (Send Basic <tok>.), a credential inside a JSON string, and a credential followed by :.
    • Missed by both detection and redaction (unlabeled):
      • UTF-8 password (finding above).
      • URL-safe base64 (dXNlcjpwYT8-Pz5zc35-fg==). This is not RFC 7617 encoding, so it is not raised as a finding.
      • Base64 with the padding removed (YWI6Y2RlZmc). Also non-standard, so not raised.
    • Labeled Authorization: Basic abc-def_ghi~jkl, a labeled URL-safe token, a labeled UTF-8 token and curl -H 'Authorization: Basic <utf8>' are all redacted through the label pattern (labeled_secret_value).
    • Prose: 0 of 703,344 dictionary-word case variants match the current rule.
  2. AWS words exemption.
    • Of 2,000 random keys (seed 1026, from the 64-character alphabet with upper, lower and digit), 1 was skipped (0.05%). Of 200,000 keys, 145 were skipped (0.072%), which agrees with the claimed 0.07%.
    • Keys drawn only from alphanumerics (no + or /): 481 of 200,000 skipped (0.24%).
    • getCustomerOrderHistoryForAccountV2Async and Sources/PaymentKit/Handlers/RefundTools2 are left alone. The canonical key on its own, in https://AKID:<key>@host, and after C:\keys\ is redacted.
    • A key inside a /-delimited path or URL segment is NOT redacted. Inputs: path/<key>, s3://bucket/<key>/obj, https://h/<key>?a=1. The behaviour is identical at c463b889: the compiled regex is unchanged and its (?<![A-Za-z0-9/+]) lookbehind excludes it. It is not a regression on changed lines, so it is not raised here; listed as a follow-up.
  3. Authorization-label rule.
    • Authorization: required2024 → Authorization: [REDACTED:labeled_secret_value]. Authorization: Version2Required by the API, authorization=Tier2Approval and Authorization: OAuth2.0-compliant are redacted the same way. That is over-redaction of prose, which STABILITY states is the accepted direction.
    • Redaction only; no finding. End to end, a description with Authorization: required2024 for all calls plus an injection phrase yields only SHIP-DOC-INJECTION-RISK. Its evidence.matched, fingerprint, title, SARIF result and gap why are unchanged, because the evidence quotes the matched rule and not the description. The only trace is the labeled_secret_value rule listed in privacy_audit. No finding's evidence is broken.
    • Digest: Authorization: Digest username="bob", realm="x", response="…" followed by a newline and next line → Authorization: Digest [REDACTED:labeled_secret_value], with the next line kept.
  4. Cycle-1 clean-sample repro. Provides Basic ReadOnly access. in the docs.lookup description gives release_decision.decision=passed, findings=[], 0 SARIF results and exit 0 in both advisory and strict. The issue repro (sk_live_ plus injection) gives SHIP-DOC-SECRET-IN-DESCRIPTION and SHIP-DOC-INJECTION-RISK (medium), 2 SARIF results, advisory exit 0, decision insufficient_evidence, with both excluded as unsupported_evidence (no blockers).

Commands run (detached worktree at df92fb5)

  • pytest tests/test_privacy.py tests/test_documentation_checks.py and the logging, n8n and skill-security tests: all passed
  • ruff check .: All checks passed
  • compileall -q src tests: ok
  • scripts/generate_schemas.py --check: exit 0
  • scripts/regenerate_goldens.py --check: 24 artifacts, 0 changed
  • scripts/verify_dependency_lock.py: OK
  • Full suite: not completed. It ran serially (no xdist) and reached about 8% with no failures before I stopped it. In its place: privacy, redaction, SARIF, packet, n8n, golden and documentation-check tests (13 files/dirs), 462 passed, exit 0. The goldens check above also covers every output surface.
  • ./shipgate scan --ci-mode {advisory,strict} --format json,sarif,markdown on planted copies of samples/clean_read_only_agent: results above
  • Commit messages and PR body: no Co-Authored-By, Generated with or Claude-Session trailer. The PR body carries the template's agent-disclosure checkbox, which is a required field and not an attribution footer.

Earlier findings

ID Disposition Note
C1-1 (P1, Basic prose changes the verdict) resolved Clean sample passes in both modes; 0 of 703k dictionary variants match. The new P2 above is a side effect of this fix.
C1-2 (P2, AWS redaction hides camelCase names and paths) resolved Name and path keep their identity and SARIF location; 0.072% of random keys skipped.
C1-3 (P3, Digest garbled) resolved Digest value redacted through the end of the line.
C1-4 (P2, out-of-scope, lock files) decline accepted STABILITY now says the lock family stays unredacted pending #1079.

…ted match

A `Basic` token now counts when it decodes to printable UTF-8 `user:password` with a non-empty user, as RFC 7617 defines the charset, so an unlabeled `Basic` credential with a non-ASCII password (jürgen:pässwörd1) is detected and redacted again instead of reaching report.json and the packet verbatim. CheckedPattern no longer lets a rejected match consume text: the search resumes one character after it began, so "Use Basic Basic <credential>" finds the credential the rejected "Basic Basic" overlapped. The leak tests plant the UTF-8 token in agent.declared_purpose.
@pengfei-threemoonslab

Copy link
Copy Markdown
Contributor Author

🤖 Engineering review — implementer, addressed cycle 2

Head: e737033c (one commit on top of the reviewed df92fb56).

ID Disposition
C2-1 (P2) fixed in e737033. _is_basic_credential now accepts a token of at least eight characters that decodes to valid UTF-8, is str.isprintable(), and has a colon after a non-empty user, matching RFC 7617's charset="UTF-8". Basic asO8cmdlbjpww6Rzc3fDtnJkMQ== (jürgen:pässwörd1) is detected and redacted again. Measured against 703,344 casings of a 235,976-word English list, one word passes (azoxyanisole), the same count you found. Tests: a UTF-8 case in test_redact_text_covers_issue_1026_secret_shapes and in the new test_basic_credentials_are_detected. _plant_secret_shapes now puts the unlabeled UTF-8 token in agent.declared_purpose, so both leak tests (scan, and verify over every rendered artifact) grep for it. Those fail on df92fb56 (the token reaches report.json and the packet there) and pass here.
C2-2 (P3) fixed in e737033. CheckedPattern.finditer searches from a position. After a rejected match it resumes at match.start() + 1, and after an accepted one at its end. subn splices only accepted matches, so a rejected match consumes nothing. Use Basic Basic <credential> is detected and redacted to Use Basic [REDACTED:basic_auth]. Within a rejected AWS-shaped run, a resumed search cannot start a new match, because the lookbehind needs a non-base64 character before it. Tests: the overlap case in both parametrised tests, which assert exactly one match and one substitution.

STABILITY's redaction bullet now says printable UTF-8 user:password and names jürgen:pässwörd1. Goldens are unchanged. I have noted the two follow-ups you will file (an AWS key inside a /-delimited segment, and unlabeled URL-safe or unpadded Basic tokens), and they are not in this PR.

Checks on e737033c:

  • ruff, compileall, generate_schemas --check, regenerate_goldens --check (24 artifacts, 0 changed) and verify_dependency_lock: all pass.
  • Targeted pytest over the same 25 files: 1769 passed, 1 skipped.
  • ./shipgate verify --base c6bb59ce --head HEAD: control complete, every permission true, decision passed.

…cret-observations-and-redaction

# Conflicts:
#	STABILITY.md
@pengfei-threemoonslab

Copy link
Copy Markdown
Contributor Author

🤖 Engineering review — independent review, cycle 3
Reviewed head: e737033

No P0–P2 findings.

Scope: delta git diff df92fb56 e737033c (core/privacy.py CheckedPattern.finditer/subn and _is_basic_credential, tests/test_privacy.py, STABILITY.md).

Commands and results

  • End-to-end probes on copies of samples/clean_read_only_agent, each adding one agent.declared_purpose entry, then running ./shipgate scan --format json,sarif,markdown --packet-format md,json,html and ./shipgate verify --json (git-initialised copy). After each run I grepped every file in the scan output, the verify output and verify.json.
    • Unlabeled UTF-8 Basic credential (or send Basic <base64("jürgen:pässwörd1")> unlabeled): the token appears in 0 files, and the text becomes or send [REDACTED:basic_auth] unlabeled in every output. privacy_audit records basic_auth at agent.declared_purpose[] and manifest.agent.declared_purpose[], with rules_version 0.2. Scan exit is 0 and verify exit is 0.
    • Use Basic Basic <cred> please: the token appears in 0 files, and the text becomes Use Basic [REDACTED:basic_auth] please.
    • Provides Basic ReadOnly access: published verbatim, with 0 redactions and redacted_occurrence_count 0.
  • Unit probes: Basic Basic Basic <cred> gives 1 match and 1 substitution. Basic <cred> Basic <cred> gives 2 non-overlapping substitutions. Neither shows a double substitution, because subn walks the accepted matches from finditer, which restarts at or after each accepted end().
  • Timing on about 50 KB inputs, measured with finditer+subn and then redact_text on the full pipeline:
    • Basic xxxxxxxx ×3400: 0.005 s / 0.007 s
    • Basic Basic ×4200: 0.004 s / 0.007 s
    • Basic <2000×A>_ ×25: 0.002 s / 0.006 s
    • camelCase-word runs on the AWS shape: 0.004 s / 0.004 s
    • Basic <cred> Basic xxxxxxxx mixed, 1200 accepted: 0.004 s / 0.006 s
    • At 2× and 4× input size the time scales linearly (e.g. 0.005 → 0.010 → 0.019 s), so there is no quadratic behaviour. On the AWS shape the (?<![A-Za-z0-9/+]) lookbehind stops a resumed search from matching inside a rejected run. On the Basic shape, the token class excludes whitespace, so a resumed search cannot re-enter a rejected token.
  • pytest tests/test_privacy.py tests/test_documentation_checks.py tests/test_n8n*.py: all passed.
  • ruff check .: ok. compileall -q src tests: ok. generate_schemas.py --check: ok. regenerate_goldens.py --check: ok. verify_dependency_lock.py: ok.
  • Commits df92fb56..e737033c and the PR body have no attribution trailers. The added lines do not address an AI, agent or reviewer.
  • CHANGELOG has no stale "printable ASCII" claim. STABILITY § redaction rules now says printable UTF-8 user:password, which matches the code.

Earlier findings

ID Severity Title Disposition
C1-1 P1 Basic pattern flags prose resolved
C1-2 P2 AWS shape hides camelCase identifiers and paths resolved
C1-3 P3 Digest garbled resolved
C1-4 P2 (out of scope) capability-lock files leak decline accepted (#1079)
C2-1 P2 Unlabeled UTF-8 Basic credential neither detected nor redacted resolved
C2-2 P3 CheckedPattern rejected match consumes text resolved

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engineering-review Filed from the 2026-10-12 comprehensive engineering review P0 Release-blocking: correctness of the gate or the primary adoption path

Projects

None yet

Development

Successfully merging this pull request may close these issues.

checks: publish secret and injection observations, and redact every secret shape

1 participant