diff --git a/shared/prompts/code-review-criteria.md b/shared/prompts/code-review-criteria.md index 215a990613..82586d12fb 100644 --- a/shared/prompts/code-review-criteria.md +++ b/shared/prompts/code-review-criteria.md @@ -39,6 +39,10 @@ - Are there tests for new functionality? - Do existing tests still pass? - Are edge cases covered? +- **Tests must exercise the production code path.** The following patterns are blocking: + - *Self-seeding goldens* — a golden fixture generated by running the implementation under test (whether on first run or after a regeneration) pins whatever output that implementation produced. Broken detector → broken golden, pinned forever; "we regenerated it after a fix" does not rescue this. Goldens must be authored against an independently verified expectation. + - *Hand-built fixtures that bypass the production code path* — a test that directly constructs a manifest entry, serialised payload, or cache key rather than going through the production helper does not exercise that helper. A regression there would not break the test. + - *Name-vs-behaviour contradictions* — e.g. a test named `test_zero_major_hard_fails` that asserts `'0.1'` is accepted. Either the name is misleading or the assertion is wrong; resolve the contradiction before merging. **Documentation**: - Are significant changes documented? @@ -57,7 +61,10 @@ **Blocking** (request changes): - Security vulnerabilities -- Non-functional features — the feature's core purpose does not work end-to-end +- **Non-functional features** — the feature's core purpose does not work end-to-end. Two named sub-cases, both blocking: + - *Single-module break* — the unit responsible for the feature is itself broken (wrong logic, missing branch, mis-wired constant). Caught by reading the file under inspection. + - *Cross-module silent no-op* — every individual file looks internally consistent, but the producer's output is filtered, dropped, or defaulted by a downstream consumer such that the feature does nothing in its normal path. Synthetic-key dead-ends across modules are the canonical example (one module emits a synthetic sentinel; the consumer's filter excludes it). Each unit test passes; the cross-module wiring dead-ends. Caught only by tracing data flow across the changeset. +- **Operator-facing misconfiguration produces no signal** — patterns that mask invalid **operator-supplied** input: silent exception fallbacks (bare `except Exception:` followed by a default), `None`-on-error returns, or no-op default branches. These same patterns are often legitimate for non-operator inputs (graceful degradation of optional config, defensive nulling of internal state); the qualifier is operator-facing. The safety floor holding ("no crash, no security violation") does not make this non-blocking when the operator gets no feedback that their **deliberately set** input was ignored. Symlinks, schema errors, and denied paths a user deliberately set should fail loudly, not silently. - Logic errors that produce incorrect results - Breaking changes to existing functionality - Resource leaks or crashes diff --git a/shared/prompts/security-review-criteria.md b/shared/prompts/security-review-criteria.md index 72b377096c..87ebfd0556 100644 --- a/shared/prompts/security-review-criteria.md +++ b/shared/prompts/security-review-criteria.md @@ -115,7 +115,8 @@ bypass the gateway. **But** a compromised wrapper can still: 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. +`sandbox/scripts/*` as a trust-boundary change. Read-only file access +of agent-supplied paths is covered separately in §8. Verification recipe: @@ -174,6 +175,60 @@ A line-by-line code reviewer often misses these because the file under inspection looks self-consistent. The security lens runs on the full changeset and is the natural seam to flag them. +### 8. Agent-supplied paths flowing into read-only file access + +The lens has historically downgraded read-only file access +(`Path(p).read_text()`, `open(p)`, `glob(p)`) of agent-supplied paths +because there was no shell-out and no write. That is the wrong +threat-model. PR [#2105](https://github.com/jwbron/egg/pull/2105)'s +`_handle_validate_repo_config` shipped this way — `reviewer_security` +approved it as "appropriate read-only delegation," and the GHA reviewer +correctly flagged it as path traversal. **Read access to attacker-chosen +workspace-readable targets is a path-traversal bug class regardless of +whether the handler also writes or shells out.** + +Common shapes: + +- An MCP-tool / route handler accepts a path argument and passes it + to `Path(...).read_text()` without a workspace-root prefix check. +- A skill writes to or reads from `/.egg/...` where + `` is agent-supplied and unvalidated against a workspace + root. +- A validator that *rejects* an unsafe path on one entrypoint, while a + sibling entrypoint reads the same path before validation runs. + +Verification recipe: + +1. For every changed MCP tool, route, or skill that accepts a path + argument, find every place that path flows into a filesystem API. + The list is non-exhaustive — flag any API that opens the file or + returns metadata about it. Common shapes: + - **Reads / opens**: `open()`, `Path.read_text` / `read_bytes`, + `shutil.copy`. + - **Directory enumeration**: `os.scandir`, `os.walk`, `os.listdir`, + `glob`, `pathlib.Path.iterdir` — leaks names of files outside the + workspace. + - **Existence / metadata oracles**: `Path.exists()`, `Path.is_file()`, + `Path.is_dir()`, `Path.stat()`, `os.path.exists()`, + `os.path.isdir()` — a `True` / `False` return based on + `Path(agent_path).exists()` leaks filesystem layout. + - **Symlink inspection**: `Path.is_symlink()`, `os.readlink()` — + leaks the symlink target outside the workspace. + - **Loaders that take a `Path`**: `yaml.safe_load(Path(p).read_text())`, + `json.load(open(p))`, `tomllib.load(open(p, "rb"))`, + `configparser.read()`. +2. Confirm a workspace-root prefix check + (`p.resolve().is_relative_to(WORKSPACE_ROOT.resolve())` or + equivalent) runs **before** the access. `.resolve()` must run before + the prefix check, so symlinks and `..` segments are collapsed first; + checking `is_relative_to` on the unresolved `Path` lets a symlink + inside the workspace point outside it. +3. NACK on any agent-supplied read of an unconstrained path — even + when the handler does not write, does not shell out, and does not + return the contents to the caller. Reading + `/etc/shadow` / `~/.ssh/id_rsa` / `/.git/config` is the + bug. + ## How to Review 1. Read the full diff once at the security lens.