Skip to content

Hardening pass from an external security scan (Codex Security CLI) #550

Description

@fujibee

scripts/ on main (053c358) was scanned end to end with @openai/codex-security, an open-source CLI — 86 files, 57 surfaces, 24 findings, none rated high. This issue tracks the ones worth acting on as one hardening pass, so the work can be sequenced rather than arriving as a dozen separate tickets. It continues the hardening the earlier static security pass over this tree produced (#514, #515, #516, #519).

This is hardening, not an incident. There is no network-facing attacker and no multi-tenant boundary here: the realistic path is a local one — a prompt-injection chain inside a team, or a project directory crafted to carry shell metacharacters. What makes it worth doing is not targeted attack, which is not a serious prospect for a project this size, but that the same shapes fire by accident: #378 is a case where a message body was evaluated by the sender's own shell with nobody attacking anything. The other reason is forward-looking — as sync moves across machines, today's convenience boundaries become real trust boundaries, and these are cheaper to fix before that than after.

The scan output itself is not reproduced here. Findings are described by class and by the file that owns the fix; exact offsets and payload shapes are left out on purpose.

The root worth fixing first

agmsg assembles shell command text by string interpolation, with no shared quoting primitive. Values are wrapped in single quotes and assumed safe, which holds until a value contains a quote.

Two places do this, and they need different fixes:

  1. Generated rule and hook files. The shared rule generator (scripts/lib/delivery-rulefile.sh) and the per-type delivery adapters interpolate the project path into command text. A project directory whose name contains an apostrophe terminates the quoted argument early, and the remainder of the path is parsed rather than passed. This is confirmed against main, not inferred: a directory named with an apostrophe produces a rule file whose command argument is no longer a single quoted token.

    Worth being precise about where the defect is not: agmsg_validate_project_path (added for delivery.sh set silently mkdirs a bogus project dir when passed a malformed path (no existence check / trim) #493 in fix(delivery): validate project_path in set instead of mkdir-ing whatever it is given (#493) #508) rejects empty values, CR/LF, non-directories, and directories it cannot enter, and it deliberately accepts an apostrophe because that is a legal POSIX path character. Narrowing the validator would break legal paths to paper over a generator bug. The generator is what has to quote.

  2. Shipped agent templates. Every type template instructs the model to construct send.sh / join.sh / actas / reset / spawn commands with raw team and agent identifiers as bare shell words. The validators reject path and control hazards but allow characters that are shell-significant, so a roster value can carry meaning into a command composed from our own documentation.

The common cause is that there is no shared quoting helper to route these through — printf '%q' appears only in error messages today. The two sites are grouped here because the missing primitive is the same; they are separate pieces of work because one fixes a generator and the other fixes documentation plus a validator contract.

Affected: scripts/lib/delivery-rulefile.sh, scripts/delivery.sh, scripts/drivers/types/*/_delivery.sh (copilot, cursor, gemini, grok-build), scripts/drivers/types/*/template.md, scripts/lib/validate.sh.

opencode is done. #570 routes its project path through printf '%q' and uses that in every command the rule generates — the turn rule, the sentinel monitor command, and the monitor fallback — with a regression that parses the generated lines with bash -n rather than matching text. It is off this list because it is fixed, not because it was reclassified. The same shape works for the adapters still listed; the reason it was done there first is that #570 was adding two new command surfaces and we did not want to widen the gap while tracking it.

Remaining surfaces

Each is one item of work; none is started.

  • Canonical containment for rule and hook writes. resolve_hooks_file checks the manifest-relative path lexically and then joins it to the project root, with no canonical check and no no-follow on the write, so a symlinked parent component redirects the write outside the project. One check covers every type, since all of them route through it. — scripts/delivery.sh, scripts/lib/delivery-rulefile.sh, scripts/drivers/types/antigravity/_delivery.sh
  • No-follow creation for bridge sidecars. Bridge turns share one writable run directory and create predictable per-role files by fixed name. Private per-role directories plus exclusive, no-follow creation. — scripts/drivers/types/codex/codex-bridge.js, scripts/drivers/types/codex/codex-bridge-launcher.sh
  • Bridge log carries response transcripts. Response deltas reach bridge stderr, which is appended to a deterministically named shared log, so bridged roles can read each other's output. Decide whether the log should carry response content at all. — same two files
  • Unread reads are unbounded. No row limit or byte budget on unread and after-watermark reads, no body-size quota on send, and the bridge runs the inbox without an explicit child-process buffer, so a large enough backlog kills the bridge instead of truncating. — scripts/inbox.sh, scripts/check-inbox.sh, scripts/watch.sh, scripts/send.sh, scripts/drivers/types/codex/codex-bridge.js
  • Control bytes can drop messages. The Stop-hook JSON encoder handles some control characters and leaves the rest of the C0 range raw; affected messages are marked read regardless, so they are lost rather than retried. — scripts/check-inbox.sh
  • Team and agent names can collide. The role-session record keys on a joined team-agent string while both validators allow hyphens, so two different (team, agent) pairs can produce one key and a later resurrection can restore the wrong session. — scripts/lib/role-session.sh, scripts/internal/resurrect-panes.sh

Confidence

Verified directly against main while triaging, not taken from the scan report:

  • the generator interpolates the project path without quoting, reproduced with a real directory;
  • resolve_hooks_file performs a lexical check only;
  • no unread or after-watermark read carries a row limit;
  • no shared shell-quoting helper exists.

Taken from the scan report and not independently reproduced: the bridge-internal items (sidecar creation, stderr transcript handling) and the C0-byte encoding behaviour. Anyone picking those up should confirm the mechanism before writing a fix.

Out of scope

Neither overlaps #515 or #516: those concern sender provenance and the framing of peer message bodies, whereas the template item above concerns identifiers appearing in text agmsg itself ships.

How this issue is meant to be used

This is a tracking issue, not a work queue to be drained in order. When a surface is actually picked up, it gets split into its own issue with the detail that work needs, and the box here is checked. No child issues exist yet.

Related: #514, #515, #516, #519, #378, #507

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions