From 462526f9022a24fd6721f06ec44dcc392c9845b9 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Sun, 26 Apr 2026 21:55:53 -0700 Subject: [PATCH 1/4] patterns: drop sandbox/scripts wholesale block, move enforcement to reviewer_security (#2133) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wholesale block on `sandbox/scripts/` in CODER_PATTERNS was defense-in-depth, not a runtime control: the gateway sidecar is the sole egress chokepoint, the sandbox container holds no credentials, and any request a wrapper emits is independently re-validated by the gateway. The block's only effect was to force a two-step dance for new wrappers (#1556 jira, #1931 confluence) — stage at .egg-state/agent-outputs/, exempt in patterns.py, deploy, then git mv post-merge — because the gateway sidecar enforces the patterns.py it loaded at startup, not the one the coder just edited. Changes: - Remove "sandbox/scripts/" from CODER_PATTERNS.blocked_patterns and drop the now-redundant sandbox/scripts/jira block_exempt entry. - Update the four pattern tests that asserted on the wholesale block to assert it is now writable, with rationale comments pointing at the security reviewer. - Add a "Credential-shim modifications under sandbox/scripts/" section to security-review-criteria.md (priority slot 5) covering the actual threat surface a compromised wrapper exposes (misleading the caller, smuggling fields into request bodies, route confusion, session-token exfiltration through allowed requests). Marked as a blocking finding for any deviation from the documented wrapper shape. Also adds sandbox/scripts/ to the trust-boundary list in "How to Review". - Update docs/reference/agent-roles.md and docs/guides/sdlc-pipeline.md to reflect the new policy. --- docs/guides/sdlc-pipeline.md | 2 +- docs/reference/agent-roles.md | 10 ++-- .../test_agent_restrictions_comprehensive.py | 2 +- .../tests/test_agent_restrictions_patterns.py | 16 +++--- shared/egg_restrictions/patterns.py | 11 ---- shared/prompts/security-review-criteria.md | 54 +++++++++++++++++-- shared/tests/test_egg_restrictions.py | 16 +++--- 7 files changed, 77 insertions(+), 34 deletions(-) diff --git a/docs/guides/sdlc-pipeline.md b/docs/guides/sdlc-pipeline.md index c665b9b2dc..fa8dcef8fe 100644 --- a/docs/guides/sdlc-pipeline.md +++ b/docs/guides/sdlc-pipeline.md @@ -588,7 +588,7 @@ Agents are organized into five categories (execution, analysis, review, utility, | Role | Category | Purpose | File Access | |------|----------|---------|-------------| -| **Coder** | Execution | Implements code changes | All files except docs, tests, `.egg-state/`, `.github/`, `sandbox/scripts/` (blocklist-complement; see [Agent Roles Reference](../reference/agent-roles.md#coder)) | +| **Coder** | Execution | Implements code changes | All files except docs, tests, `.egg-state/`, `.github/` (blocklist-complement; see [Agent Roles Reference](../reference/agent-roles.md#coder)) | | **Tester** | Execution | Finds gaps, writes tests, runs linters and reports issues to coder | Test files and infrastructure only: `tests/`, `test/`, `**/test_*.py`, `**/*_test.go`, `**/*.test.{ts,tsx,js,jsx}`, `**/*.spec.{ts,tsx,js,jsx}`, `**/conftest.py` (see [Agent Roles Reference](../reference/agent-roles.md#tester)) | | **Documenter** | Execution | Updates documentation | Documentation and markdown only: `docs/`, `**/*.md`, `**/README.md` (see [Agent Roles Reference](../reference/agent-roles.md#documenter)) | | **Autofixer** | Utility | Auto-fixes lint/format/type-check issues | Source and config files (no docs or contracts) | diff --git a/docs/reference/agent-roles.md b/docs/reference/agent-roles.md index eb419ff2a2..0c6b2e4c9d 100644 --- a/docs/reference/agent-roles.md +++ b/docs/reference/agent-roles.md @@ -161,10 +161,12 @@ each surface so reviewers know to keep them in sync. (`**/*_test.py`, `**/test_*.py`, `**/*_test.go`, `**/test_*.go`, `**/*.test.{ts,tsx,js,jsx}`, `**/*.spec.{ts,tsx,js,jsx}`), `**/conftest.py` (tester's scope); `.egg-state/` (pipeline state); - plus defense-in-depth blocks on `.github/` (CI workflows and - CODEOWNERS — preserves the branch-protection invariant) and - `sandbox/scripts/` (gateway credential shims — preserves the - credential-routing invariant). + plus a defense-in-depth block on `.github/` (CI workflows and + CODEOWNERS — preserves the branch-protection invariant). + `sandbox/scripts/` is **writable** — the gateway is the sole egress + chokepoint, so credential-shim modifications are reviewed by + `reviewer_security` rather than blocked at the role-pattern layer. + See [security-review-criteria.md](../../shared/prompts/security-review-criteria.md). - Block exemptions (always writable, overriding the blocks above): `.egg-state/agent-outputs/` (coder's handoff output), `.egg-state/agent-anchors/` (per-agent anchor state, scoped by diff --git a/gateway/tests/test_agent_restrictions_comprehensive.py b/gateway/tests/test_agent_restrictions_comprehensive.py index 7e31dd7c36..d8e9651445 100644 --- a/gateway/tests/test_agent_restrictions_comprehensive.py +++ b/gateway/tests/test_agent_restrictions_comprehensive.py @@ -573,6 +573,7 @@ class TestThreeRoleBehavior1901: ".egg-state/agent-outputs/coder.json", # carved-back exempt "skills/my-skill/SKILL.md", # skills exempt "sandbox/agent-config/rules/foo.md", # rules exempt + "sandbox/scripts/gh", # credential shim — gateway is the chokepoint ], ) def test_coder_allowed_blocklist_complement(self, path): @@ -592,7 +593,6 @@ def test_coder_allowed_blocklist_complement(self, path): ".egg-state/secrets/key", # future subdir ".github/workflows/ci.yml", # branch-protection invariant ".github/CODEOWNERS", # branch-protection invariant - "sandbox/scripts/gh", # credential-routing invariant ], ) def test_coder_blocked_blocklist_complement(self, path): diff --git a/gateway/tests/test_agent_restrictions_patterns.py b/gateway/tests/test_agent_restrictions_patterns.py index 467dda558e..8442acd36e 100644 --- a/gateway/tests/test_agent_restrictions_patterns.py +++ b/gateway/tests/test_agent_restrictions_patterns.py @@ -359,13 +359,15 @@ def test_allows_sandbox_egg(self, pattern): def test_allows_sandbox_bin_egg_health_inspect(self, pattern): assert pattern.can_write("sandbox/bin/egg-health-inspect") is True - def test_blocks_sandbox_scripts_gh(self, pattern): - """sandbox/scripts/ is blocked (credential-routing invariant).""" - assert pattern.can_write("sandbox/scripts/gh") is False - - def test_blocks_sandbox_scripts_git_credential_helper(self, pattern): - """sandbox/scripts/ is blocked (credential-routing invariant).""" - assert pattern.can_write("sandbox/scripts/git-credential-github-token") is False + def test_allows_sandbox_scripts_gh(self, pattern): + """sandbox/scripts/ is writable; the gateway is the sole egress + chokepoint, so credential-shim modifications are reviewed by + reviewer_security rather than blocked at the role-pattern layer.""" + assert pattern.can_write("sandbox/scripts/gh") is True + + def test_allows_sandbox_scripts_git_credential_helper(self, pattern): + """sandbox/scripts/ is writable; see test_allows_sandbox_scripts_gh.""" + assert pattern.can_write("sandbox/scripts/git-credential-github-token") is True def test_blocks_github_workflows(self, pattern): """.github/ is blocked (branch-protection invariant).""" diff --git a/shared/egg_restrictions/patterns.py b/shared/egg_restrictions/patterns.py index 5c05e83032..f0208bfa28 100644 --- a/shared/egg_restrictions/patterns.py +++ b/shared/egg_restrictions/patterns.py @@ -231,9 +231,6 @@ def _matches_pattern(file_path: str, pattern: str) -> bool: # Defense-in-depth: CI workflows and CODEOWNERS — preserves the # branch-protection invariant. ".github/", - # Defense-in-depth: gateway credential shims — preserves the - # credential-routing invariant. - "sandbox/scripts/", ], block_exempt_patterns=[ # Coder's handoff directory — the only .egg-state/ subdir the coder @@ -257,14 +254,6 @@ def _matches_pattern(file_path: str, pattern: str) -> bool: "sandbox/agent-config/commands/*.md", # Top-level skills directory (skill definitions are functional code) "skills/", - # Jira sandbox wrapper — new in #1556. ``sandbox/scripts/`` is - # blocked wholesale to keep agents from retargeting the existing - # ``gh`` / ``git`` credential shims; this exemption lets the coder - # land the NEW ``jira`` wrapper alongside them. The file is - # functionally identical in shape (bash wrapper that POSTs to a - # gateway REST endpoint — never holds credentials locally) and is - # the only addition permitted under the broader block. - "sandbox/scripts/jira", ], ) diff --git a/shared/prompts/security-review-criteria.md b/shared/prompts/security-review-criteria.md index 0a581257df..7367006dcf 100644 --- a/shared/prompts/security-review-criteria.md +++ b/shared/prompts/security-review-criteria.md @@ -91,7 +91,54 @@ Verification recipe: 3. Flag broken symlinks (`ls -l ` returns a missing target) as a blocking finding regardless of code quality. -### 5. Secret leakage +### 5. Credential-shim modifications under `sandbox/scripts/` + +The wrappers under `sandbox/scripts/` (`gh`, `git`, `jira`, …) are the +sandbox's **only** egress path to credential-bearing services. The +gateway sidecar is the actual security boundary — these wrappers hold +no credentials and any request they emit is independently re-validated +by the gateway against its policy. So a compromised wrapper cannot +bypass the gateway. **But** a compromised wrapper can still: + +- **Mislead the agent calling it** — return fake success on a request + the gateway rejected, swallow error output, or print misleading + diagnostics that hide a failed operation. +- **Smuggle data into request bodies** — append attacker-chosen + fields the gateway happens to forward verbatim (e.g. PR body text, + issue comments, commit messages) to an external system. +- **Re-route to a different gateway endpoint** that has a more + permissive policy than the one the wrapper's name implies (e.g. + `gh` quietly POSTing to a `/jira/` route). +- **Exfiltrate session-scoped state** the wrapper has access to + (`EGG_SESSION_TOKEN`, environment, stdin) by including it in a + request the gateway *would* allow. + +The role-level write filter does **not** block writes under +`sandbox/scripts/` — the credential-routing invariant is enforced by +this lens, not by `patterns.py`. Treat any diff that touches +`sandbox/scripts/*` as a trust-boundary change. + +Verification recipe: + +1. Enumerate every changed file under `sandbox/scripts/`. For each, + confirm it is a thin bash/POSIX wrapper that POSTs to a + `/api/v1//*` gateway route — no inline secrets, no calls + to the real `gh`/`git`/`jira` binaries, no network calls outside + the gateway URL, no writes outside the wrapper's documented stdout. +2. Confirm the wrapper's gateway route matches its name (a wrapper + named `gh` POSTs to `/api/v1/github/*`, not `/api/v1/jira/*`). +3. Confirm output handling is faithful: a non-2xx gateway response + surfaces as a non-zero exit and an error on stderr; the wrapper + does not silently swallow errors or fabricate success output. +4. For NEW wrappers, confirm a corresponding gateway route exists + (or is added by the same diff) and is itself reviewed for policy + correctness — a permissive new route is a real security finding + even if the wrapper looks innocuous. + +This is a **blocking** finding regardless of code quality: any +deviation from the documented wrapper shape is a NACK. + +### 6. Secret leakage Any new code path that may emit secrets, tokens, credentials, or identity-bearing tokens to: @@ -108,7 +155,7 @@ Pay special attention to redaction-bypass patterns: a regex that redacts `password=...` but not `passwd=`; a sanitiser applied to one log channel but not another. -### 6. Cross-file OWASP top-10 patterns +### 7. Cross-file OWASP top-10 patterns OWASP top-10 patterns where the source and the sink live in **different changed files**. Common shapes: @@ -134,7 +181,8 @@ flag them. the reach, you have not found the bug. 3. Trust-boundary changes get extra scrutiny: any new public endpoint, any change to a decorator stack, any new file in `gateway/` or - `auth/`, any change to allowlists or regex patterns. + `auth/`, any change to allowlists or regex patterns, and any change + under `sandbox/scripts/` (the credential-shim wrappers). 4. Cross-reference [`code-review-criteria.md`](./code-review-criteria.md) for the base review rules — your verdict format, severity classification, and BRC ACK/NACK lifecycle inherit from there. diff --git a/shared/tests/test_egg_restrictions.py b/shared/tests/test_egg_restrictions.py index 1aa434f52b..d93882852f 100644 --- a/shared/tests/test_egg_restrictions.py +++ b/shared/tests/test_egg_restrictions.py @@ -216,13 +216,15 @@ def test_allows_sandbox_egg_script(self): def test_allows_sandbox_bin_egg_health_inspect(self): assert CODER_PATTERNS.can_write("sandbox/bin/egg-health-inspect") - def test_blocks_sandbox_scripts_gh_shim(self): - """sandbox/scripts/ is blocked (credential-routing invariant).""" - assert not CODER_PATTERNS.can_write("sandbox/scripts/gh") - - def test_blocks_sandbox_scripts_git_credential_helper(self): - """sandbox/scripts/ is blocked (credential-routing invariant).""" - assert not CODER_PATTERNS.can_write("sandbox/scripts/git-credential-github-token") + def test_allows_sandbox_scripts_gh_shim(self): + """sandbox/scripts/ is writable; the gateway is the sole egress + chokepoint, so credential-shim modifications are reviewed by + reviewer_security rather than blocked at the role-pattern layer.""" + assert CODER_PATTERNS.can_write("sandbox/scripts/gh") + + def test_allows_sandbox_scripts_git_credential_helper(self): + """sandbox/scripts/ is writable; see test_allows_sandbox_scripts_gh_shim.""" + assert CODER_PATTERNS.can_write("sandbox/scripts/git-credential-github-token") def test_blocks_github_workflows(self): """.github/ is blocked (branch-protection invariant).""" From 2929e3dd085dbbdf4c454b8bba78a1fedacaab1f Mon Sep 17 00:00:00 2001 From: "egg-reviewer[bot]" <261018737+egg-reviewer[bot]@users.noreply.github.com> Date: Mon, 27 Apr 2026 05:12:15 +0000 Subject: [PATCH 2/4] address review: sync .egg/phase-permissions.json and fix criteria contradiction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - .egg/phase-permissions.json: remove sandbox/scripts/ from coder file_restrictions blocked_patterns. The patterns.py change in this PR is not load-bearing alone — the gateway also enforces this JSON file via PhaseFilter._file_restrictions / check_file_restrictions, which uses startswith matching. Without this sync, a coder push of sandbox/scripts/ still gets denied by the gateway with Push denied: Role 'coder' cannot modify: sandbox/scripts/<...>, defeating the PR's goal. Updates blocked_reason and description to match patterns.py and to point reviewers at the security lens. - gateway/tests/test_phase_filter_restrictions.py: add test_coder_allowed_for_sandbox_scripts under TestThreeRoleFileRestrictions to lock the cross-layer synchronization between .egg/phase-permissions.json and shared/egg_restrictions/patterns.py. - shared/prompts/security-review-criteria.md: rephrase the section 5 closing paragraph that contradicted the advisory-lens scope at the top of the file. The new wording calls deviations a 'mandatory NACK' (the reviewer must record the finding) while making the advisory posture explicit and pointing at #1997 as the tracker for promotion to a critical (deadlock-capable) lens for sandbox/scripts/* diffs. --- .egg/phase-permissions.json | 7 +++---- gateway/tests/test_phase_filter_restrictions.py | 10 ++++++++++ shared/prompts/security-review-criteria.md | 9 +++++++-- 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/.egg/phase-permissions.json b/.egg/phase-permissions.json index cb05effd81..de106c669b 100644 --- a/.egg/phase-permissions.json +++ b/.egg/phase-permissions.json @@ -215,11 +215,10 @@ "**/*.spec.js", "**/*.spec.jsx", "**/conftest.py", - ".github/", - "sandbox/scripts/" + ".github/" ], - "blocked_reason": "Coder cannot modify tests, docs, pipeline-state, CI workflows, or credential shims; see #1901.", - "description": "TODO(#1903): keep in sync with CODER_PATTERNS / TESTER_PATTERNS / DOCUMENTER_PATTERNS in shared/egg_restrictions/patterns.py. Coder: everything except tester, documenter, .egg-state/, .github/, and sandbox/scripts/ scopes." + "blocked_reason": "Coder cannot modify tests, docs, pipeline-state, or CI workflows; see #1901. sandbox/scripts/ is writable — credential-shim modifications are reviewed by reviewer_security (see shared/prompts/security-review-criteria.md), not blocked at this layer.", + "description": "TODO(#1903): keep in sync with CODER_PATTERNS / TESTER_PATTERNS / DOCUMENTER_PATTERNS in shared/egg_restrictions/patterns.py. Coder: everything except tester, documenter, .egg-state/, and .github/ scopes." }, { "role": "tester", diff --git a/gateway/tests/test_phase_filter_restrictions.py b/gateway/tests/test_phase_filter_restrictions.py index e2fdd962d4..95699a1199 100644 --- a/gateway/tests/test_phase_filter_restrictions.py +++ b/gateway/tests/test_phase_filter_restrictions.py @@ -587,6 +587,16 @@ def test_coder_allowed_for_extensionless_script(self): result = check_file_restrictions("coder", ["bin/egg"]) assert result.allowed is True + def test_coder_allowed_for_sandbox_scripts(self): + """sandbox/scripts/ is writable by the coder at the gateway + file_restrictions layer (#2133). Credential-shim modifications + are reviewed by reviewer_security rather than blocked at the + role-pattern layer; this test locks the synchronization between + .egg/phase-permissions.json and shared/egg_restrictions/patterns.py + so the two layers do not drift apart.""" + result = check_file_restrictions("coder", ["sandbox/scripts/gh"]) + assert result.allowed is True + # --- tester role --- def test_tester_blocked_from_contracts(self): diff --git a/shared/prompts/security-review-criteria.md b/shared/prompts/security-review-criteria.md index 7367006dcf..cc6a09098c 100644 --- a/shared/prompts/security-review-criteria.md +++ b/shared/prompts/security-review-criteria.md @@ -135,8 +135,13 @@ Verification recipe: correctness — a permissive new route is a real security finding even if the wrapper looks innocuous. -This is a **blocking** finding regardless of code quality: any -deviation from the documented wrapper shape is a NACK. +Any deviation from the documented wrapper shape is a **mandatory NACK** +— do not silently approve a credential-shim diff that fails the recipe +above. Note that the security lens is advisory today (see *Scope* at the +top of this file), so the NACK is recorded as a finding on the approval +matrix rather than deadlocking consensus; promotion to a critical +(deadlock-capable) reviewer for `sandbox/scripts/*` diffs is tracked in +[#1997](https://github.com/jwbron/egg/issues/1997). ### 6. Secret leakage From a8d1d83f07dbf508c5320503f4b2cd10559fc602 Mon Sep 17 00:00:00 2001 From: "egg-reviewer[bot]" <261018737+egg-reviewer[bot]@users.noreply.github.com> Date: Mon, 27 Apr 2026 05:25:26 +0000 Subject: [PATCH 3/4] test: cover git-credential-github-token in sandbox/scripts test Defense-in-depth follow-up to review feedback on PR #2135. Mirrors the patterns.py test layer (which covers both gh and git-credential-github-token) so the gateway-layer synchronization test locks in coverage of both wrappers a coder might land under sandbox/scripts/. --- gateway/tests/test_phase_filter_restrictions.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/gateway/tests/test_phase_filter_restrictions.py b/gateway/tests/test_phase_filter_restrictions.py index 95699a1199..d755e44714 100644 --- a/gateway/tests/test_phase_filter_restrictions.py +++ b/gateway/tests/test_phase_filter_restrictions.py @@ -593,9 +593,14 @@ def test_coder_allowed_for_sandbox_scripts(self): are reviewed by reviewer_security rather than blocked at the role-pattern layer; this test locks the synchronization between .egg/phase-permissions.json and shared/egg_restrictions/patterns.py - so the two layers do not drift apart.""" + so the two layers do not drift apart. Mirrors the patterns.py + layer coverage of both `gh` and `git-credential-github-token`.""" result = check_file_restrictions("coder", ["sandbox/scripts/gh"]) assert result.allowed is True + result = check_file_restrictions( + "coder", ["sandbox/scripts/git-credential-github-token"] + ) + assert result.allowed is True # --- tester role --- From dccf9de066a7bc00326723c2cd33c93e8a14ac18 Mon Sep 17 00:00:00 2001 From: egg Date: Mon, 27 Apr 2026 05:26:49 +0000 Subject: [PATCH 4/4] Fix checks: apply automated formatting fixes --- gateway/tests/test_phase_filter_restrictions.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/gateway/tests/test_phase_filter_restrictions.py b/gateway/tests/test_phase_filter_restrictions.py index d755e44714..db02a85bf2 100644 --- a/gateway/tests/test_phase_filter_restrictions.py +++ b/gateway/tests/test_phase_filter_restrictions.py @@ -597,9 +597,7 @@ def test_coder_allowed_for_sandbox_scripts(self): layer coverage of both `gh` and `git-credential-github-token`.""" result = check_file_restrictions("coder", ["sandbox/scripts/gh"]) assert result.allowed is True - result = check_file_restrictions( - "coder", ["sandbox/scripts/git-credential-github-token"] - ) + result = check_file_restrictions("coder", ["sandbox/scripts/git-credential-github-token"]) assert result.allowed is True # --- tester role ---