From 3abcaad3b3d6c307cc438654158bae471a1c8897 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sat, 22 Aug 2026 09:44:15 +0900 Subject: [PATCH] fix(security): redact router credential diagnostics --- CHANGELOG.md | 2 ++ .../agent-mention-concurrency-isolation.md | 16 ++++++++++++++++ scripts/ci/agent_mention_router.py | 1 + tests/test_agent_mention_router.py | 15 ++++++++++++++- 4 files changed, 33 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 89199da27f..1a3f08a6f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,8 @@ Semantic Versioning where the repository publishes a release. ### Fixed +- Redacted the exact agent-mention GitHub credential from bounded CLI failure + diagnostics while retaining exit status and actionable non-secret stderr. - Confined OSV base and head repository checkouts to the same `source/` child directory, so a cross-fork head checkout can replace that repository without deleting the base-scan JSON held at the workspace root. Both scans retain identical source paths and the required base/head vulnerability comparison remains fail-closed. - Restored 100% docstring coverage for the commercial-readiness GitHub transport constructor. - Refused PR Review Merge Scheduler head mutations, `update-branch` and the last-push approval head restamp, whenever the resolved mutation credential is the workflow `GITHUB_TOKEN`. GitHub starts no workflow run for events created with that credential, so the moved head collected no current-head required checks and the PR stayed permanently `BLOCKED` with a `github-actions[bot]` merge commit that no later scheduler run could repair, because the branch was no longer behind. The scheduler now waits with `head_mutation_credential_upgrade` guidance naming `PR_REVIEW_MERGE_TOKEN`, `OPENCODE_APPROVE_TOKEN`, and the OpenCode app token exchange. diff --git a/docs/doctoring/agent-mention-concurrency-isolation.md b/docs/doctoring/agent-mention-concurrency-isolation.md index 6b5272ba62..665e01070e 100644 --- a/docs/doctoring/agent-mention-concurrency-isolation.md +++ b/docs/doctoring/agent-mention-concurrency-isolation.md @@ -66,6 +66,16 @@ whose pull-list request completes first can expose its recent PR comments before a slower sibling. Dispatch remains sequential through the existing ledger and exact-head validation boundaries. +### Credential-safe transport diagnostics + +The router passes its GitHub credential only through `GH_TOKEN`, never through +the command arguments. GitHub CLI failures can still echo that credential in +standard error, so `GitHubClient.request` removes the client's exact token +before applying the existing 2,000-character diagnostic bound. Timeout errors +retain their fixed message and do not add command arguments. This follows the +OWASP logging guidance to remove access tokens while preserving actionable +operational evidence; it does not mask operational PII. + ## Preserved boundaries - No model provider, reviewer identity, repository allowlist, token name, credential scope, or branch-protection rule changes. @@ -74,6 +84,8 @@ ledger and exact-head validation boundaries. - Only trusted non-bot `OWNER`, `MEMBER`, or `COLLABORATOR` comments on open pull requests are eligible. - Pull request number, exact head and base SHAs, base branch, source comment, requested agent, and requesting actor remain bound to the invocation key. - Mention routing remains unable to approve, merge, update branches, publish, or release. +- GitHub CLI diagnostics retain exit status and sanitized error text but never + the active router credential. ## Operational acceptance @@ -86,6 +98,8 @@ After protected integration: 5. distinguish downstream provider or review failure from routing failure rather than treating every missing verdict as the same incident. 6. verify the slow-first/fast-later repository regression remains green so a delayed repository cannot starve a completed sibling's comment inventory. +7. force a GitHub CLI failure that echoes the synthetic client credential and + require `[REDACTED]` in the raised diagnostic with no credential value. A receipt proves routing and durable claim processing. It is not an approval and never substitutes for exact-head checks or branch protection. @@ -109,4 +123,6 @@ GitHub. (n.d.). *REST API endpoints for repositories: Create a repository dispat GitHub. (n.d.). *Store and share data with workflow artifacts*. GitHub Docs. Retrieved August 19, 2026, from https://docs.github.com/en/actions/tutorials/store-and-share-data +OWASP Foundation. (n.d.). *Logging cheat sheet*. OWASP Cheat Sheet Series. Retrieved August 22, 2026, from https://cheatsheetseries.owasp.org/cheatsheets/Logging_Cheat_Sheet.html + Python Software Foundation. (n.d.). *concurrent.futures — Launching parallel tasks*. Python documentation. Retrieved August 20, 2026, from https://docs.python.org/3/library/concurrent.futures.html#concurrent.futures.as_completed diff --git a/scripts/ci/agent_mention_router.py b/scripts/ci/agent_mention_router.py index aa16fecc8e..f363b858c2 100755 --- a/scripts/ci/agent_mention_router.py +++ b/scripts/ci/agent_mention_router.py @@ -97,6 +97,7 @@ def request( ) if not diagnostic: diagnostic = "no stderr output" + diagnostic = diagnostic.replace(self._token, "[REDACTED]") raise RuntimeError( f"gh api failed with exit code {return_code}: {diagnostic[:2000]}" ) diff --git a/tests/test_agent_mention_router.py b/tests/test_agent_mention_router.py index 874a79e4f5..85acdaec74 100644 --- a/tests/test_agent_mention_router.py +++ b/tests/test_agent_mention_router.py @@ -314,7 +314,7 @@ def test_dispatch_noema_only_covers_non_opencode_path() -> None: def test_github_client_validates_token_and_decodes_json(monkeypatch) -> None: - """The token-bound client never places credentials in command arguments.""" + """The token-bound client keeps credentials out of commands and errors.""" module = load_module() with pytest.raises(ValueError, match="token"): @@ -339,6 +339,19 @@ def fake_run(command, **kwargs): lambda *args, **kwargs: SimpleNamespace(stdout=" "), ) assert client.request(["repos/x/y"]) is None + monkeypatch.setattr( + module.subprocess, + "run", + lambda *args, **kwargs: SimpleNamespace( + returncode=1, + stderr="authentication failed for secret-token", + stdout="", + ), + ) + with pytest.raises(RuntimeError) as exc_info: + client.request(["repos/x/y"]) + assert "secret-token" not in str(exc_info.value) + assert "[REDACTED]" in str(exc_info.value) def test_load_event_and_main_paths(tmp_path: Path, monkeypatch, capsys) -> None: