Skip to content

The forge guard consumes a flag value before allowlisting it, hiding a denied flag #181

Description

@devantler

🤖 Generated by the Agentic Engineer

Evidence

forge-readonly-guard.sh consumes a value-taking flag's next word before that word is checked against the read-only allowlist. So a denied flag placed immediately after any value flag is never classified at all.

Measured on origin/main (unmodified guard, f8da2a50's base):

rc=0  gh pr list --repo --web      :: allow
rc=0  gh pr list --state --web     :: allow
rc=0  gh pr view 1 --limit --web   :: allow

--web is precisely the flag the guard's own comments name as the reason read verbs are allowlisted at all:

gh pr view --web opens a URL through $BROWSER, which runs a local program the guard never classified.

The same shape applies to every entry in GH_VERB_VALUE_FLAGS and GH_API_VALUE_FLAGS.

Why this is a Bug and not Security

It is not exploitable as written, and I would rather say so than inflate the Security queue. Real gh uses pflag, which parses --web as the value of --repo and then errors on the malformed repo argument — no browser opens. The defect is that the guard's safety currently depends on the downstream parser rejecting it, rather than on the guard's own classification. That is a latent correctness gap in an enforcement control: it holds today for a reason the guard does not itself assert, and it would stop holding if any value flag ever gained a permissive value grammar.

Expected behaviour

A word consumed as a flag value should still be rejected when it is itself a denied flag — or the guard should assert explicitly why consuming it is safe for that specific flag.

Note the tension to resolve rather than paper over: some legitimate values genuinely begin with - (a --search expression such as -label:foo, a --jq program). So this is not a blanket "reject dash-leading values" — it needs a deliberate rule, which is why it is filed rather than patched inline.

Acceptance criteria

  • A denied flag appearing as a value-flag's value is either rejected, or explicitly justified per flag with a comment stating why consuming it is safe.
  • Legitimate dash-leading values (--search -label:x, --jq programs) keep working — pinned by tests in forge-readonly-guard.test.sh in both directions.
  • Negative controls prove the new rule fires, per this repo's ablation convention.

Rough size: S–M — one classification site, but the value-grammar question needs deciding per flag family.

Provenance

Found while fixing the P1 on #177 (the guard denying the mandated bare --json vocabulary probe). Reported rather than folded into that PR to keep it one concern; #177's fix neither introduces nor widens this.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions