Skip to content

feat(security): add deterministic public-surface scrub Claude Code mod (part of #1497) - #1676

Open
Kaap10 wants to merge 5 commits into
apache:mainfrom
Kaap10:feat/claude-code-mod-security-scrub-1497
Open

Kaap10 wants to merge 5 commits into
apache:mainfrom
Kaap10:feat/claude-code-mod-security-scrub-1497

Conversation

@Kaap10

@Kaap10 Kaap10 commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Adds a zero-token Claude Code mod in plugins/magpie-security/hooks/ that intercepts Bash commands (git commit, git tag, gh pr create, gh pr edit, gh pr review, gh pr comment, gh issue comment, gh api) and guards public surfaces against accidental security embargo leaks.
  • Intercepts and parses commit messages (-m, -am, -sm, -sam, -F), attached short flags (-m"...", -Fpath), PR descriptions/comments, heredocs, and shell pipelines.
  • Evaluates composite multi-command pipelines per subcommand across all shell separators (;, &&, ||, |, \n).
  • Implements strict regex matching for forbidden embargo terms aligned with plugins/magpie-security/skills/issue-fix/implementation-plan.md:31-43 (CVE references, vulnerability wording, security fix phrasing, and vulnerability classes like SSRF, XSS, CSRF, SQL injection, path traversal, RCE, privilege escalation, auth bypass).
  • Dynamically resolves upstream_repo and tracker_repo from <project-config>/project.md via resolveProjectConfig(), scoping the scrub strictly to upstream_repo and exempting tracker_repo per subcommand without hardcoded naming conventions.
  • Enforces deterministic fail-closed behavior on unreadable message files, --amend/--fill, and indeterminate dynamic inputs ($VAR, $(...), stdin -). Resolves -F / --body-file relative to git -C <dir> without Node built-in imports.
  • Provides a human-set options.env.MAGPIE_ALLOW_EMBARGO_TERMS=1 environment escape hatch for legitimate post-coordinated-disclosure maintenance (in-command bypasses removed).
  • Passes clean behavioral changes through with zero token consumption and zero latency.

Type of change

  • Skill change (.claude/skills/<name>/) — eval fixtures updated below
  • Tool / bridge contract (tools/<system>/*.md)
  • Python package (tools/*/ with pyproject.toml)
  • Groovy reference impl
  • Cross-cutting (RFC, AGENTS.md, sandbox, privacy-LLM)
  • Documentation (docs/, README.md, CONTRIBUTING.md)
  • Project template (projects/_template/)
  • CI / dev loop (prek, workflows, validators)
  • Other: Claude Code mod hooks (plugins/magpie-security/hooks/)

Test plan

  • Ran claude plugin validate --strict plugins/magpie-security (Validation passed, minimal API surface: $.fs.read).
  • Ran claude plugin test plugins/magpie-security (47 pass, 0 fail).
  • Ran claude plugin test plugins/magpie-utilities and plugins/magpie-setup for regression testing (83 total tests pass across repo).
  • Verified prek linters (end-of-file-fixer, trailing-whitespace, mixed-line-ending) pass cleanly.

RFC-AI-0004 compliance

  • HITL — any new mutation is gated on explicit user confirmation
  • Sandbox — minimal declared surface area ($.fs.read), no node:fs or node:path built-in imports, deterministic execution.
  • Vendor neutrality — mod is strictly additive; existing issue-fix skill workflow and Markdown verification checklist remain completely untouched for non-Claude harnesses.
  • Conversational + correctable — agentic-override path documented if behaviour is adopter-tunable
  • Write-access discipline — no autonomous outbound messages; drafts only, sent on confirmation
  • Privacy LLM — zero model calls made; operates 100% locally on the host machine.

Linked issues

Notes for reviewers (optional)

@github-actions github-actions Bot added the family:security security-* skills label Oct 16, 2026
@Kaap10

Kaap10 commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

Implemented zero-token deterministic public-surface embargo scrub mod.
Ready for review, please let me know if any adjustments or changes are needed!

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking up a deterministic scrub — the idea is right, but as written the agent can switch the guard off from inside the command it is checking, several ordinary shell shapes get a message past the parser, and the scope (every commit in every repository) is wider than the rule it enforces.

Outputs are probabilistic; gates are deterministic (plugins/magpie-security/hooks/public-surface-scrub.ts:237)

Skills produce drafts. Tool calls enforce schemas. Humans or deterministic checks decide whether a draft becomes state. Probabilistic at the input, deterministic at every state change. The boundary never blurs, even when the draft looks reliable enough to short-circuit the gate.

    /(?:^|\s)MAGPIE_ALLOW_EMBARGO_TERMS=1(?:\s|$)/.test(commandLine) ||
    commandLine.includes('--allow-embargo-terms')

The escape hatch can be switched on by the very command it is meant to stop. An agent that hits the block (and whose reason text tells it how to proceed) only has to prefix MAGPIE_ALLOW_EMBARGO_TERMS=1 or append --allow-embargo-terms anywhere in the command, including inside the commit message or a trailing # comment, and the guard passes it. --allow-embargo-terms is also not a real git or gh flag, so a literal use makes the underlying command fail. A deterministic gate has to be outside the agent's reach.

Suggested fix: honour only options.env (the hook process environment, set by the human before the session), drop the command-line forms, and leave the override out of the blocking message, so the agent is told to ask the maintainer rather than how to bypass the guard.

Flagged by both the primary and adversarial reviewers.

Writing a literal project value directly into a skill is a refactor bug (plugins/magpie-security/hooks/public-surface-scrub.ts:88)

Concretely, gh issue view <N> --repo <tracker> means "substitute
<tracker> for <project-config>/project.md → tracker_repo before
running this". Writing a literal project value directly into a skill is a
refactor bug — skills must stay project-agnostic so swapping projects is
a config change, not a code change.

    repo.endsWith('-s') ||
    repo.includes('-s/') ||
    repo.includes('/private') ||
    repo.includes('-private') ||
    repo.includes('security-tracker')

The private-tracker exemption is a naming guess, and it fails open: any public repository whose name ends in -s or contains -private / /private / security-tracker is exempt, so security wording on that public surface passes unchecked. A tracker named any other way (for example apache/foo-security) gets blocked instead. Only the airflow-s/airflow-s convention is encoded. It also exempts the whole command: isPrivateTrackerTarget searches the entire string and line 118 returns early, so gh pr view 1 -R org/private-tracker; gh pr comment 2 -R apache/airflow -b "CVE-… SSRF" passes although the second command targets the public upstream (found by codex).

Suggested fix: read tracker_repo (and upstream_repo) from the resolved <project-config>/project.md and exempt exactly that slug, and nothing when it cannot be resolved; decide the exemption per sub-command, not for the whole script.

Flagged by both the primary and adversarial reviewers.

This is the single forbidden-term list for every public surface of this skill. (plugins/magpie-security/hooks/public-surface-scrub.ts:128)

This is the single forbidden-term list for every public surface of this skill.

  if (/(?:^|[;&|]\s*)git\s+(?:-[^\s]+(?:\s+[^\s]+)?\s+)*commit(?:\s|$)/i.test(trimmed)) {

The list governs the public surfaces of the security-issue-fix flow, the <upstream> fix PR before its advisory ships. The hook applies it to every git commit, gh pr create and gh pr comment in every repository the plugin is loaded in. In this repository that blocks ordinary work: \bCVE\b (line 26, case-insensitive) matches fix(generate-cve-json): …, and vulnerability / security advisory appear in many routine commit messages and PR descriptions here and in any adopter's own docs.

Suggested fix: scope it to the configured upstream_repo (from project.md), and preferably to an active fix (a branch or marker the issue-fix skill sets), and let every other repository pass.

This is the single forbidden-term list for every public surface of this skill. (plugins/magpie-security/hooks/public-surface-scrub.ts:128)

This is the single forbidden-term list for every public surface of this skill.

  if (/(?:^|[;&|]\s*)git\s+(?:-[^\s]+(?:\s+[^\s]+)?\s+)*commit(?:\s|$)/i.test(trimmed)) {

Several ordinary shell shapes get a leaking message past the parser (the first four found by codex; each traced in the code):

  • a newline between commands: the detectors accept only start-of-string or ;, &, |, so git status + newline + git commit -m "CVE-… SSRF" is not recognised;
  • an attached message, git commit -m"CVE-… SSRF" (and an attached -F): the extraction requires = or whitespace after the flag;
  • a commit followed by a PR: the git commit branch returns before the gh branches, so git commit -m "neutral" && gh pr create -t "CVE-… SSRF" checks only the commit message, and && does not trigger the whole-command scan;
  • $(…), backticks or a variable in the message, or a piped body (… | git commit -F -): only the literal command text is scanned, and - is skipped;
  • not covered at all: git commit --amend reusing a message, git tag -m, gh pr create --fill, gh pr edit --title/--body, gh pr review --body, gh issue comment on <upstream>, gh api … -f body=.

Suggested fix: split the command into sub-commands on every shell separator (newline included) and evaluate each one; accept attached short-option arguments; when a message comes from command substitution, a variable or stdin, scan the whole command and block when the published text cannot be determined; add the missing subcommands, with a test per shape.

Flagged by both the primary and adversarial reviewers.

Outputs are probabilistic; gates are deterministic (plugins/magpie-security/hooks/public-surface-scrub.ts:263)

Skills produce drafts. Tool calls enforce schemas. Humans or deterministic checks decide whether a draft becomes state.

      } catch {
        // Safe skip on unreadable file; pure regex inspection continues
      }

A message file the hook cannot read is skipped and the command is allowed, and the path is read relative to the session's working directory even after git -C <dir>, where git reads <dir>/<file> (found by codex). So git -C /upstream commit -F msg.txt with an embargoed msg.txt passes whenever the hook resolves the path differently from git.

Suggested fix: resolve -F / --body-file paths against the -C directory, and block (fail closed, naming the file) when a referenced message file cannot be read.

Flagged by codex (adversarial review); cross-checked.

Smaller observations

  • See inline comments on plugins/magpie-security/hooks/public-surface-scrub.ts:28.

This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. After you've
addressed the points above and pushed an update, an Apache Magpie
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.

More on how Apache Magpie handles maintainer review:
Contributing guide.

Comment thread plugins/magpie-security/hooks/public-surface-scrub.ts Outdated
Comment thread plugins/magpie-security/hooks/public-surface-scrub.ts Outdated
Comment thread plugins/magpie-security/hooks/public-surface-scrub.ts Outdated
Comment thread plugins/magpie-security/hooks/public-surface-scrub.ts Outdated
Comment thread plugins/magpie-security/hooks/public-surface-scrub.ts Outdated
Comment thread plugins/magpie-security/hooks/public-surface-scrub.ts Outdated
@Kaap10

Kaap10 commented Oct 17, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review @potiuk. I’ve addressed all findings: the override is now restricted strictly to options.env (removing in-command bypasses and guidance text), and repository scoping resolves upstream_repo/tracker_repo dynamically from project.md evaluated per subcommand.

The parser now splits multi-command pipelines across all shell separators (including newlines), supports attached short flags (-m"...", -Fpath), and enforces deterministic fail-closed behavior on unreadable files, --amend/--fill, and indeterminate dynamic inputs ($VAR, $(...), stdin -).

All 47 unit tests pass cleanly covering each of these review shapes, and plugin validation passes strict static analysis without Node runtime built-in dependencies.

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the round of fixes — the in-command bypass is gone, the tracker / upstream slugs now come from project.md, each sub-command is judged on its own, and unreadable message files and dynamic input now fail closed. Two things still block it: the mod keeps its own copy of the term list instead of using the canonical one, and the gate still fails open in several shapes.

On my earlier threads:

  • Message-file / -C handling — fixed; resolving.
  • In-command bypass, tracker exemption, ordinary shell shapes — answered on their own terms, but the replacements open the gaps in the inline comments (the escape hatch never reaching the hook, cd … &&, git -c, escaped quotes, --repo read from a body, directory-name heuristics).
  • Scope, and the bare advisory — folded into the term-list comment: deriving the list from DEFAULT_TERMS settles both.

On the architecture question in the first inline comment, moving this into tools/agent-guard as a Python guard is worth weighing before more work goes into the TypeScript version: it would import the canonical list directly and run on every harness agent-guard supports.

Smaller, not blocking: in upstream scope this fails closed on the common git commit -m "$(cat <<'EOF' … EOF)" form; that is safe, but the block message could say -F <file> is the expected shape.

single forbidden-term list (plugins/magpie-security/hooks/public-surface-scrub.ts:28)

This is the single forbidden-term list for every public surface of this skill. 5g, Step 7, Step 9 and the Guardrails check against it rather than keeping their own copies.

export const FORBIDDEN_TERM_RULES: ReadonlyArray<{ name: string; pattern: RegExp }> = [

Derive the list from magpie_vcs.publication.DEFAULT_TERMS (generated data file plus a sync check), or move the guard into tools/agent-guard where it can import the canonical list.

Humans or deterministic checks decide whether a draft becomes state. (plugins/magpie-security/hooks/public-surface-scrub.ts:726)

Humans or deterministic checks decide whether a draft becomes state.

const evaluation = await evaluatePublicSurfaceCommand(command, {
        readFile: async (filePath: string) => {

Pass env: $.env, and add a register()-level test with MAGPIE_ALLOW_EMBARGO_TERMS=1.

The boundary never blurs (plugins/magpie-security/hooks/public-surface-scrub.ts:624)

The boundary never blurs, even when the draft looks reliable enough to short-circuit the gate.

} else if (projectConfig.isTrackerCheckout) {
        // Bare git command in the tracker repository itself without upstream context
        continue;

Track cd <dir> across sub-commands; block when the target repository cannot be determined; resolve config in the upstream clone instead of passing everything.

Probabilistic at the input, deterministic at every state change. (plugins/magpie-security/hooks/public-surface-scrub.ts:293)

Probabilistic at the input, deterministic at every state change.

const cwdMatch = trimmed.match(/(?:^|\s)git\s+.*?-C(?:=|\s+)(?:"([^"]+)"|'([^']+)'|([^\s;&|]+))/i);

Tokenise argv with a shell-word parser; read -C only from git's global options, case-sensitively; same for -R/--repo.

Smaller observations

  • See inline comments on plugins/magpie-security/hooks/public-surface-scrub.ts:613, plugins/magpie-security/hooks/hooks.json:5.

This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. After you've
addressed the points above and pushed an update, an Apache Magpie
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.

More on how Apache Magpie handles maintainer review:
Contributing guide.

Comment thread plugins/magpie-security/hooks/public-surface-scrub.ts Outdated
const command = e?.input?.command || e?.params?.command;

if (toolName === 'bash' && typeof command === 'string') {
const evaluation = await evaluatePublicSurfaceCommand(command, {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

register() passes only readFile here, never env, so the MAGPIE_ALLOW_EMBARGO_TERMS escape hatch can never fire in production, while the block message still tells the maintainer to "set the environment bypass". The test that covers it calls evaluatePublicSurfaceCommand directly, so it does not notice. Please forward $.env and add a test through register().

// Working in an unrelated repository (e.g. apache/magpie)
continue;
}
} else if (projectConfig.isTrackerCheckout) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gate passes exactly where a fix commit is most likely made. (1) A session in the tracker checkout: cd ../<upstream> && git commit -m "Fix CVE-2026-1234" passes, because cd is not followed and a bare git here is skipped. (2) A session started in the upstream clone has no project.md, so every command is allowed. When the target repository cannot be determined, this should block, not pass.

Comment thread plugins/magpie-security/hooks/public-surface-scrub.ts Outdated
Comment thread plugins/magpie-security/hooks/public-surface-scrub.ts Outdated
"$schema": "https://code.claude.com/schemas/hooks.json",
"description": "Apache Magpie — Claude Code mod for deterministic public-surface scrub against unredacted security embargo leaks.",
"modules": [
"./public-surface-scrub.ts"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the first mod in magpie-security and the PR touches no docs. docs/setup/secure-agent-setup.md should cover its trade-offs (it runs unsandboxed and reads any -F path via $.fs.read) and how the escape hatch is set, and the issue-fix skill should mention it next to the scrub step so a maintainer knows where a block came from.

Kaap10 added a commit to Kaap10/magpie that referenced this pull request Oct 20, 2026
…closed gates (apache#1676)

- Derive forbidden terms directly from canonical magpie_vcs.publication.DEFAULT_TERMS using tools/dev/sync-forbidden-terms.py, with pre-commit hook to prevent drift.
- Tokenize shell commands with POSIX shell-words parser to handle quotes and escaped quotes.
- Track directory context across chained sub-commands (cd <dir> && git ...).
- Parse git -C only from global options and disambiguate clustered/attached flags.
- Inspect remotes and worktrees to detect contributor fork checkouts as upstream scope.
- Fail closed on indeterminate dynamic input with -F <file> guidance.
- Pass environment from hook event and add register()-level tests for MAGPIE_ALLOW_EMBARGO_TERMS.
- Document mod architecture, tradeoffs, and escape hatch in secure-agent-setup.md.
@github-actions github-actions Bot added family:setup setup-* skills family:tools tools/* family:docs Docs, MISSION.md, READMEs family:ci .github workflows, prek, validators capability:fix Implement code change to resolve an issue capability:resolve Close-out: invalidate, dedupe, CVE allocate, announcement substrate:framework-dev Tool substrate: build / validate / eval the framework itself labels Oct 20, 2026
@Kaap10

Kaap10 commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

will resolve CI shortly!

…d (part of apache#1497)

Adds a zero-token Claude Code mod in plugins/magpie-security/hooks/ that intercepts Bash commands (git commit, gh pr create, gh pr comment) and guards public surfaces against accidental security embargo leaks (CVE IDs, vulnerability wording, security fix phrasing, and vulnerability classes).

- Intercepts tool.call on Bash commands, parsing commit messages (-m, -am, -sm, -F) and PR descriptions (-t, -b, -F, --body-file).
- Ingests heredocs and shell pipelines to prevent stdin bypasses.
- Exempts private security tracker repositories (-s suffix convention).
- Provides MAGPIE_ALLOW_EMBARGO_TERMS=1 maintainer escape hatch for legitimate post-coordinated-disclosure maintenance.
- Passes clean behavioral changes through with zero token consumption.
- Includes 29 unit tests covering all matching rules, command syntax, edge cases, and hook middleware integration.
…ace scrub mod

- Limit escape hatch strictly to options.env set by human, removing in-command bypasses.

- Scope checks to upstream_repo from project.md; evaluate private tracker exemption per subcommand.

- Split multi-command pipelines on shell separators (including newlines) and accept attached short flags.

- Fail closed on unreadable message files and indeterminate dynamic input.

- Resolve file paths relative to git -C directory without node:path.

- Expand test suite to 47 unit tests covering all review shapes.
…closed gates (apache#1676)

- Derive forbidden terms directly from canonical magpie_vcs.publication.DEFAULT_TERMS using tools/dev/sync-forbidden-terms.py, with pre-commit hook to prevent drift.
- Tokenize shell commands with POSIX shell-words parser to handle quotes and escaped quotes.
- Track directory context across chained sub-commands (cd <dir> && git ...).
- Parse git -C only from global options and disambiguate clustered/attached flags.
- Inspect remotes and worktrees to detect contributor fork checkouts as upstream scope.
- Fail closed on indeterminate dynamic input with -F <file> guidance.
- Pass environment from hook event and add register()-level tests for MAGPIE_ALLOW_EMBARGO_TERMS.
- Document mod architecture, tradeoffs, and escape hatch in secure-agent-setup.md.
@Kaap10
Kaap10 force-pushed the feat/claude-code-mod-security-scrub-1497 branch from cffec98 to 4a89ad2 Compare October 20, 2026 06:26
@Kaap10

Kaap10 commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @potiuk! Addressed all review items:

  • Derived forbidden terms directly from canonical magpie_vcs.publication.DEFAULT_TERMS with a pre-commit sync check.
  • Added a POSIX shell-words parser, chained cd tracking across subcommands, remote/worktree fork detection, and fail-closed dynamic inputs.
  • Forwarded env into evaluation with register-level tests, documented mod trade-offs in secure-agent-setup.md, and rebased onto main.

Ready for another look!

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this round: the term list is now generated from DEFAULT_TERMS with a prek sync check, argv is tokenised properly, -C / -c and --repo-in-body are fixed, cd is tracked, and the docs section is in. One thing still blocks it: the hook reads the wrong fields of the tool.call event, so in a real session it never sees a command. Behind that, several common shapes still let a message reach the upstream unchecked.

On my earlier points:

  • Canonical term list, tokenising argv, -C / -c / escaped quotes / --repo in a body, directory-name heuristics, heredoc guidance, docs: fixed. I've resolved those threads.
  • Escape hatch through register(): still not reachable (first item below).
  • Block when the target can't be determined: cd is followed now, but a project.md in the session cwd is taken to mean "tracker", which re-opens the upstream case (second item).
  • Moving this into tools/agent-guard: still worth answering before another round. Most of the items below would need solving in either place, and a Python guard importing DEFAULT_TERMS directly would work on every harness.

Blocking: the hook never sees the command (plugins/magpie-security/hooks/public-surface-scrub.ts:1289)

Skills produce drafts. Tool calls enforce schemas. Humans or deterministic checks decide whether a draft becomes state.
— PRINCIPLES.md

const command = e?.input?.command || e?.params?.command;
...
const hookEnv = e?.env ?? (typeof process !== 'undefined' ? process.env : {});
...
return { error: evaluation.reason };

In the mod API the tool's arguments sit at the top level of the tool.call input (e.command for Bash), so e.input.command is undefined and nothing is evaluated. A refusal is { deny: reason }; { error } is not a result the engine knows. The event carries no env, and a hooks module has no Node process, so MAGPIE_ALLOW_EMBARGO_TERMS still cannot reach the hook. The register tests (public-surface-scrub.test.ts:723, :753, :781) build { tool, input: { command }, env } by hand and assert result.error, so they pass against a shape the engine never sends.

Suggested fix: on('tool.call', { tool: 'Bash' }, …), read e.command, return { deny }, and read the bypass with await $.env.get("MAGPIE_ALLOW_EMBARGO_TERMS"). Drive the register tests through claude-code/testing's tool noun, so the call goes through the plugin's hook as in a session.

Major: a project.md in the cwd is treated as the tracker (plugins/magpie-security/hooks/public-surface-scrub.ts:1187)

The boundary never blurs, even when the draft looks reliable enough to short-circuit the gate.
— PRINCIPLES.md

if (projectConfig.isTrackerCheckout && !effectiveDir) {
  continue;
}

isTrackerCheckout is true whenever project.md is found under the cwd (:210). An adopter that commits .apache-magpie-overrides/project.md into its upstream repository has one there, and so does this repository. There, a bare git commit -m "Fix CVE-2026-1234" skips the remote check and passes, although the remote is the upstream.

Suggested fix: drop the shortcut. Always inspect the effective directory's remotes, exempt only on a tracker_repo match, and scan on an upstream_repo (or fork) match.

Major: the PR URL and GH_REPO select the repository too (plugins/magpie-security/hooks/public-surface-scrub.ts:701)

Probabilistic at the input, deterministic at every state change.
— PRINCIPLES.md

if ((token === '-R' || token === '--repo') && i + 1 < tokens.length) {

From a tracker-checkout session, all of these pass and land on the public upstream PR:

  • gh pr comment https://github.com/apache/airflow/pull/1 -b "Fix CVE-2026-1234 SSRF"
  • gh pr edit <same URL> --body "…"
  • GH_REPO=apache/airflow gh pr comment 1 -b "…"

Suggested fix: take the target from a PR/issue URL argument and from a leading GH_REPO=, with a test for each.

Major: gh pr review --body-file is not read (plugins/magpie-security/hooks/public-surface-scrub.ts:888)

Write the text to a file in the session scratch directory or $TMPDIR first, then hand the CLI the path: gh pr create --body-file, gh pr comment --body-file, gh issue comment --body-file, gh pr review --body-file,
— AGENTS.md → Commit and PR conventions

if (token === '-F' && i + 1 < tokens.length) {

Only -F is handled in the review branch. gh pr review 1 -R apache/airflow --comment --body-file evil.md passes without the file being opened.

Suggested fix: handle --body-file / --body-file= as the comment branch does.

Major: gh api coverage is narrower than documented (plugins/magpie-security/hooks/public-surface-scrub.ts:979)

gh api … --input <file> for JSON payloads
— AGENTS.md → Commit and PR conventions

const match = fieldArg.match(/^(?:body|title)=(.*)$/s);

Only an inline body= / title= field is scanned. These all pass against repos/apache/airflow/...:

  • --input body.json, the form AGENTS.md prescribes;
  • -F body=@msg.md, where gh reads the file and the hook sees only @msg.md;
  • gh api graphql -f query='mutation{addComment(input:{…body:"CVE-…"})}'.

Suggested fix: read --input and @file values, and fail closed when they can't be read. Scan or fail closed on a graphql mutation. Otherwise, narrow the claim in secure-agent-setup.md and SKILL.md.

Major: every repository without Magpie config is now scanned (plugins/magpie-security/hooks/public-surface-scrub.ts:1199)

Re-read the diff and check that every change is intentional.
— AGENTS.md → Before submitting

if (!remoteInfo.isUpstream && upstreamRepo) {
  continue;
}

With no project.md, upstreamRepo is undefined and nothing passes. git commit -m "fix(generate-cve-json): tidy" and git commit -m "docs: security advisory template" are blocked in any unrelated repository the plugin is enabled in. That contradicts the new docs line ("Commands targeting unrelated third-party or adopter documentation repositories pass unchecked").

Suggested fix: use the upstream clone path from user.md, so an upstream session is still recognised without a local project.md. Then let repositories whose remotes are known and match neither slug pass, and fail closed only when scope is genuinely unknown.

Smaller observations

  • A single & is not a separator (public-surface-scrub.ts:273). git -C ../airflow status & git -C ../airflow commit -m "Fix CVE-…" passes.
  • reporterNames / privateTexts (:79-107, :1210) are never filled in production, so the reporter-name and verbatim-text checks only run in unit tests. Wire a source for them or drop them.
  • forbidden-terms.json is generated and sync-checked, but nothing reads it.
  • -m 'costs $HOME' in single quotes is blocked as dynamic input, because the $VAR check runs on the raw string rather than the tokens. This is safe, but it is a false positive.
  • If a command writes a message file with a heredoc and then commits with -F on that file, the hook reads the old file before the new text is written.
  • /usr/bin/git, bash -c '…' and (cd … && git commit …) are not recognised. That is fine for a backstop, but then the docs should not say it "cannot be … faked".

This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. After you've
addressed the points above and pushed an update, an Apache Magpie
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.

More on how Apache Magpie handles maintainer review:
Contributing guide.

export function register(on: any): void {
on('tool.call', async ($: any, e: any, next: any) => {
const toolName = (e?.tool || '').toLowerCase();
const command = e?.input?.command || e?.params?.command;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking. tool.call spreads the tool's arguments at the top level, so for Bash the command is e.command; e.input.command is always undefined and nothing is evaluated. A refusal is { deny: reason }, not { error }, and the event has no env (read it with await $.env.get("MAGPIE_ALLOW_EMBARGO_TERMS")). See the first section of the review body.

// 3. For commands without explicit repo flag, determine repository from git context
if (!parsed.targetRepo) {
// If we are in the tracker repository checkout itself without upstream context, exempt it
if (projectConfig.isTrackerCheckout && !effectiveDir) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major. isTrackerCheckout is true whenever a project.md is under the cwd, which includes upstream repositories that commit .apache-magpie-overrides/project.md (this one does). A bare git commit -m "Fix CVE-…" there skips the remote check. Drop the shortcut and always inspect the remotes.

if (binary === 'gh') {
// Extract target repo: -R <repo>, -R=<repo>, --repo <repo>, --repo=<repo>, -R<repo>
// Linear scan skipping arguments to other flags to prevent reading --repo from inside body/title
for (let i = cmdIdx + 1; i < tokens.length; i++) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major. The target repository is read only from -R / --repo. A PR or issue URL argument, or a leading GH_REPO=, also selects it, and those shapes reach the public upstream unchecked.

result.extractedTexts.push(token.slice(2));
continue;
}
if (token === '-F' && i + 1 < tokens.length) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major. Only -F is read in the gh pr review branch; --body-file / --body-file=, the form AGENTS.md prescribes, is never opened.

continue;
}

if ((token === '-f' || token === '-F' || token === '--field' || token === '--raw-field') && i + 1 < tokens.length) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major. Only inline body= / title= fields are scanned. --input <file>, -F body=@file and graphql mutations pass, although the docs say gh api calls are gated.

continue; // Safe tracker repository
}
// If verified not upstream (and not a fork of upstream) and not tracker, allow
if (!remoteInfo.isUpstream && upstreamRepo) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major. With no project.md, upstreamRepo is undefined, so this never lets a repository pass: ordinary commits in unrelated repositories are blocked (fix(generate-cve-json): tidy), contrary to the new docs line.

current = '';
continue;
}
if (char === '&' && next === '&') {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lone & (and |&) is not treated as a separator, so git … status & git … commit -m "Fix CVE-…" is evaluated as one status command and passes.

if (parsed.hasDynamicInput) {
// First check if the commandLine itself explicitly carries forbidden terms
const dynamicTerms = findForbiddenTerms(commandLine, {
reporterNames: options?.reporterNames ?? projectConfig.reporterNames,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reporterNames / privateTexts are never filled outside the unit tests: register() passes neither and parseProjectManifest sets neither. Wire a real source, or drop the checks and say the guard covers the term list only.

… and CLI parser

- Correct Claude Code hook registration: register on('tool.call', { tool: 'Bash' }),
  read e.command directly, and return { deny: evaluation.reason } on block.
- Read environment bypass via $.env.get('MAGPIE_ALLOW_EMBARGO_TERMS') to pass strict plugin validation.
- Drop isTrackerCheckout shortcut; inspect git remotes directly and probe fallback user.md for upstream identification.
- Expand CLI subcommand parsing in parseSubcommand:
  * Extract repository targets from PR/issue URLs (https://github.com/<owner>/<repo>/pull/<id>).
  * Extract leading GH_REPO= environment variables.
  * Support --body-file in gh pr review.
  * Inspect gh api --input, -F body=@file, and GraphQL mutations.
- Enhance shell splitting and dynamic tokenization:
  * Split on single & command separator (unless part of >&).
  * Ignore literal $VAR inside single quotes (e.g. -m 'costs $HOME').
- Calibrate documentation in secure-agent-setup.md regarding zero-token deterministic guard.

Generated-by: Antigravity
@Kaap10

Kaap10 commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

Hi @potiuk,

Thanks for the detailed review! All findings from review have been addressed in the latest commit:

  1. Architecture & Scope:

  2. Hook Registration API & Engine Contract:

    • Registered with tool filter: on('tool.call', { tool: 'Bash' }, ...).
    • Reads top-level e.command directly and returns { deny: ... } when blocked.
    • Reads the bypass variable via await $.env.get('MAGPIE_ALLOW_EMBARGO_TERMS'), passing claude plugin validate --strict.
    • Unit tests now drive registration through tool.call with { command } and assert { deny }.
  3. Remote & Directory Scoping:

    • Dropped the isTrackerCheckout shortcut; remotes are always inspected.
    • Added fallback probing for user.md (upstream_repo / local_upstream) so upstream sessions are recognized even without a local project.md.
    • Commands in unrelated repositories whose remotes match neither tracker nor upstream pass unchecked.
  4. CLI Parser Gaps:

    • Extracts target repository from PR / issue URLs (https://github.com/<owner>/<repo>/pull/<id>) and leading GH_REPO=<owner>/<repo> environment variables.
    • Added --body-file and --body-file= support to gh pr review.
    • Added --input <file>, -F body=@file, and GraphQL mutation detection to gh api.
    • Corrected shell splitting on single & (when not && or >&).
    • Evaluates $VAR dynamic input only outside single quotes, avoiding false positives on strings like -m 'costs $HOME'.
  5. Docs Calibration:

    • Updated docs/setup/secure-agent-setup.md to describe the guard as a zero-token deterministic check against accidental leaks and clarified that inline command environment variables are ignored.

All 60 tests in claude plugin test plugins/magpie-security and claude plugin validate --strict are passing.

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

Labels

capability:fix Implement code change to resolve an issue capability:resolve Close-out: invalidate, dedupe, CVE allocate, announcement family:ci .github workflows, prek, validators family:docs Docs, MISSION.md, READMEs family:security security-* skills family:setup setup-* skills family:tools tools/* substrate:framework-dev Tool substrate: build / validate / eval the framework itself

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants