63 lines
5.7 KiB
Markdown
63 lines
5.7 KiB
Markdown
# Adversarial Bug Report: fix-harness-command-template
|
|||
|
|
|
||
|
|
Attack the fix as a hostile user / harness would, looking for ways to escape substitution, break harness invocation, or corrupt state.
|
||
|
|
|
||
|
|
## Attack vectors tried
|
||
|
|
|
||
|
|
### A1 — Can a malicious `loop.json` `harness.command` element escape argv via shell metachars?
|
||
|
|
`subprocess.run` is invoked with a list (no `shell=True`). Each list element is passed verbatim as a single argv element to the OS. A `harness.command` like `["sh", "-c", "rm -rf /"]` would invoke `sh -c "rm -rf /"` as a literal argv element — but `rm -rf /` is still the *content* of the `-c` argument, so it DOES run `rm -rf /`. **This is config-trust, not a runtime escape**: the user controls `loop.json` and could equally well write any command. Pre-fix behavior was identical (custom commands were always honored). ACCEPTED.
|
||
|
|
|
||
|
|
### A2 — Can a hostile verifier prompt inject into `{prompt_content}` for the orchestrate role?
|
||
|
|
The implement role's stdout is captured as the artifact content. The verify role's prompt is built by `_resolve_prompt` which substitutes `{artifact_content}` from the implement output. If the implement role's stdout contains `"{prompt_content}"` or `{verdict}`, it becomes part of the verify prompt content (via `_resolve_prompt`'s content substitution), and the resulting `{prompt_content}` for the verify invocation includes that text. No security boundary violation — the implement role was already allowed to influence the verify prompt (v1 behavior). ACCEPTED.
|
||
|
|
|
||
|
|
### A3 — Can `{prompt_content}` be leaked via the orchestrator's stdout capture?
|
||
|
|
The orchestrator's stdout is written to `<loop>/outputs/tickN-orchestrate.json`. If the orchestrator echoes `{prompt_content}` (which contained sensitive task content), the content is recorded. This is intended behavior — the orchestrator is supposed to see the prompt context. ACCEPTED.
|
||
|
|
|
||
|
|
### A4 — Can a path traversal in `loop_path` corrupt the prompt file write?
|
||
|
|
`_resolve_prompt` writes to `<loop_path>/outputs/tickN-<role>-prompt.md` using `out_dir.mkdir(parents=True, exist_ok=True)` and a fixed filename. No user-controlled path component — `tick_num` is an int, `role` is internal. ACCEPTED.
|
||
|
|
|
||
|
|
### A5 — Does `Path(resolved_prompt).read_text()` ignore encoding errors?
|
||
|
|
No `encoding` arg uses platform default. A prompt file with invalid bytes for the default encoding raises `UnicodeDecodeError`, which is NOT caught by the `try/except OSError` (UnicodeDecodeError is a `ValueError`, not OSError). The exception propagates up and the tick crashes.
|
||
|
|
|
||
|
|
**Wait — this is a real bug.** Let me check:
|
||
|
|
- `_invoke_harness` does `try: prompt_content = Path(resolved_prompt).read_text() except OSError`.
|
||
|
|
- `UnicodeDecodeError` is a subclass of `ValueError`, NOT `OSError`.
|
||
|
|
- So a binary prompt file (or a UTF-16 file with BOM, or any non-default-encoding text) would crash the tick.
|
||
|
|
|
||
|
|
Pre-fix behavior: `{prompt}` was just the file PATH string. No read happened in `_invoke_harness`. So this is a NEW failure surface introduced by my change.
|
||
|
|
|
||
|
|
**Severity**: LOW — prompt files are written by `_resolve_prompt` itself (markdown, UTF-8). A user would have to drop a binary file at `<loop>/<prompt_ref>` to trigger it. But the framework should not crash on a misconfigured prompt file; it should fall back to empty prompt content and halt with `verifier_failed` (graceful).
|
||
|
|
|
||
|
|
**Fix recommendation**: broaden the except clause to `(OSError, UnicodeDecodeError)` or use `except Exception` for the read. Or pass `encoding="utf-8", errors="replace"` to `read_text()`.
|
||
|
|
|
||
|
|
I'll fix this inline before transitioning to doc_review. It's a small, contained hardening — the alternative (a crash mid-tick) violates the idempotence contract.
|
||
|
|
|
||
|
|
### A6 — Can a missing prompt file slip through silently on the create path?
|
||
|
|
If `loop_path is None` (no loop context), `_resolve_prompt` is skipped and `resolved_prompt = prompt_path` (the raw ref). Then `Path(resolved_prompt).read_text()` fails with OSError, `prompt_content = ""`. Default command becomes `["opencode", "run", "--dir", "<cwd>", ""]`. The spawned opencode runs with no prompt. This matches the documented fallback (D-H2 mentions the empty-prompt fast path). Accepted.
|
||
|
|
|
||
|
|
### A7 — Can two concurrent ticks both compute the same `{prompt_content}` and clobber?
|
||
|
|
`{prompt_content}` is computed locally in each tick process. No shared state. The temp file is written by `_resolve_prompt` to `<loop>/outputs/tickN-<role>-prompt.md` where `tickN` is the current iteration count. Two ticks with the same iteration count would write to the same temp file path — but that's the same TOCTOU covered by `add-state-loop-lock` (task 2; SPEC already written). Out of scope for this task.
|
||
|
|
|
||
|
|
## Bugs found
|
||
|
|
|
||
|
|
**One LOW bug (A5)**: `Path(resolved_prompt).read_text()` raises `UnicodeDecodeError` on non-default-encoding prompt files, which is not caught by the `except OSError` clause. Causes a tick crash instead of a graceful `verifier_failed` halt.
|
||
|
|
|
||
|
|
## Fix applied inline
|
||
|
|
|
||
|
|
Broadened the except clause to also catch `UnicodeDecodeError`. See `scripts/loop-runner.py` line 319 (the `try/except` around the prompt-file read). Added `UnicodeDecodeError` to the tuple; falls back to `""` on decode failure.
|
||
|
|
|
||
|
|
Not adding a separate test for this — it's a defensive code broadening, well-narrowed by the type information.
|
||
|
|
|
||
|
|
## Five loop-death modes — coverage unchanged
|
||
|
|
|
||
|
|
| Death | Defense | Affected by fix? |
|
||
|
|
|-------|---------|------------------|
|
||
|
|
| drift | `_gate_worktree_drift` (status.py) | No |
|
||
|
|
| runaway | `_gate_iterations` (status.py) | No |
|
||
|
|
| bad verifier | `_gate_score_plateau` (status.py) + `parse_verdict` | No |
|
||
|
|
| resource burn | `_gate_budget` (status.py) | No |
|
||
|
|
| undetected halt | R8 transition refusal + audit Cat-6 | No |
|
||
|
|
|
||
|
|
## Verdict
|
||
|
|
|
||
|
|
PASS — one LOW bug found (A5), fixed inline. Proceed to doc_review.
|