Repository navigation
Conversation
|
Implemented zero-token deterministic public-surface embargo scrub mod. |
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
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_repobefore
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
;,&,|, sogit 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 commitbranch returns before theghbranches, sogit 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 --amendreusing a message,git tag -m,gh pr create --fill,gh pr edit --title/--body,gh pr review --body,gh issue commenton<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.
|
Thanks for the thorough review @potiuk. I’ve addressed all findings: the override is now restricted strictly to The parser now splits multi-command pipelines across all shell separators (including newlines), supports attached short flags ( 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
left a comment
potiuk
left a comment
There was a problem hiding this comment.
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 /
-Chandling — 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,--reporead from a body, directory-name heuristics). - Scope, and the bare
advisory— folded into the term-list comment: deriving the list fromDEFAULT_TERMSsettles 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.
…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
Bot
added
family:setup
|
will resolve CI shortly! |
…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.
cffec98 to
4a89ad2
Compare
|
Thanks @potiuk! Addressed all review items:
Ready for another look! |
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
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 /--repoin 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:
cdis followed now, but aproject.mdin 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 importingDEFAULT_TERMSdirectly 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
$TMPDIRfirst, 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.jsonis generated and sync-checked, but nothing reads it.-m 'costs $HOME'in single quotes is blocked as dynamic input, because the$VARcheck 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
-Fon 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.
|
Hi @potiuk, Thanks for the detailed review! All findings from review have been addressed in the latest commit:
All 60 tests in |
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Thanks for this round: the hook now reads e.command, returns { deny } and reads the bypass through $.env.get; the project.md shortcut is gone; PR URLs, GH_REPO=, gh pr review --body-file, --input / @file, a lone & and single-quoted $VAR are handled; unrelated repositories pass; and the docs and the issue-fix mention are in. I've checked each of those against the code. Four groups of shapes still let embargoed text reach the upstream unchecked: the first two follow on from last round's threads, the other two are new.
Major: an upstream session without project.md now passes everything (plugins/magpie-security/hooks/public-surface-scrub.ts:170)
The boundary never blurs, even when the draft looks reliable enough to short-circuit the gate.
— PRINCIPLES.md
content.match(/\|\s*`?local_upstream`?\s*\|\s*`?([^`|\r\n]+)`?/i) ??
content.match(/^\s*local_upstream:\s*`?([^`\r\n#]+)`?/im);
The user.md fallback looks for upstream_repo or local_upstream, but user.md declares neither: the upstream clone is environment.upstream_clone, and it is a filesystem path, not an owner/repo slug (plugins/magpie-setup/templates/fix-workflow.md:58). So in a session started in the upstream clone, with no project.md there, upstreamRepo stays undefined, inspectDirectoryRemote returns determined: true, isUpstream: false, and the new if (!remoteInfo.isUpstream) continue; at :1315 lets git commit -m "Fix CVE-2026-1234 SSRF" through. Last round this case blocked everything; now it allows everything, which is case (2) of the earlier thread.
Suggested fix: read environment.upstream_clone from user.md, and treat a cwd (or -C / cd target) under that path as upstream. Separately, '~/.config/apache-magpie/user.md' is passed to $.fs.read literally; please confirm $.fs.read expands ~, or resolve it from $HOME. Add a register-level test for the upstream-clone-without-project.md case.
The same missing upstreamRepo also skips every command with an explicit target (:1300, if (parsed.targetRepo && !isTargetUpstreamOrFork) continue;), so gh pr comment 1 -R apache/airflow -b "Fix CVE-…" passes too. A linked worktree gets there as well: resolveProjectConfig probes .git/apache-magpie/project.md, which in a worktree is a file, not the git common dir.
Flagged by both the primary and adversarial reviewers.
Major: a gh api graphql mutation from the tracker checkout is exempt (plugins/magpie-security/hooks/public-surface-scrub.ts:1313)
Probabilistic at the input, deterministic at every state change.
— PRINCIPLES.md
if (remoteInfo.isTracker) {
continue; // Safe tracker repository
}
The mutation text is now extracted, but a graphql call carries no repos/… path and no -R, so targetRepo is unset and the cwd's remote decides. From the tracker checkout (where the security skills run), gh api graphql -f query='mutation{addComment(input:{subjectId:"<upstream PR node id>",body:"Fix CVE-2026-1234 SSRF"})}' is exempted here and posts to the public upstream PR. The new test at public-surface-scrub.test.ts:803 adds -R apache/airflow, which gh api does not use to target a mutation, so it does not exercise this path.
Suggested fix: a graphql mutation names its target by node id, which the hook cannot resolve, so scan it (or fail closed) regardless of cwd, and drop -R from the test.
Related, in the same branch (:1089): only fields named body, title or query are read, so -f query='mutation($t:String!){addComment(input:{subjectId:"…",body:$t})…}' -f t='Fix SSRF' passes even from the upstream checkout. Scan every -f / -F value of a graphql call.
Flagged by both the primary and adversarial reviewers.
Major: commit and tag messages from sources the parser does not read (plugins/magpie-security/hooks/public-surface-scrub.ts:583)
Add the trailer with
git commit --trailer, never by typing it into the message body or the-F <file>:
— AGENTS.md → Commit and PR conventions
if (token === '--amend') {
// If amending without explicit message or file, flag as dynamic reuse
Each of these puts text into a public commit or tag that the hook never inspects:
git commit -m 'Improve validation' --trailer 'References: CVE-2026-1234'—--traileris not parsed, and it is the form AGENTS.md prescribes for attribution;git commit -C HEAD,--reuse-message=…,--reedit-message=…,--fixup=…/--squash=…— only--amendis treated as message reuse;git tag -am 'Fix CVE-2026-1234' v1—'-am'fails thehasMsgprefix test at:675, so the tag is not a public-surface command at all.
Suggested fix: scan --trailer values, treat every message-reuse option as dynamic input (fail closed), and recognise clustered short flags containing m / F in the hasMsg gate, with a test for each.
Flagged by codex (adversarial review); cross-checked.
Major: the target directory and remote are resolved wrongly in three shapes (plugins/magpie-security/hooks/public-surface-scrub.ts:1223)
The boundary never blurs, even when the draft looks reliable enough to short-circuit the gate.
— PRINCIPLES.md
if (slug && cleanUrl.includes(slug)) {
return true;
}
Each of these makes the hook inspect the tracker while the command runs against the upstream:
- Substring slug match (here): with
tracker_repo = example/projectandupstream_repo = example/project-core, the upstream remote matches both, and the tracker check at:1312wins. Any tracker remote also exempts a checkout that has an upstream remote beside it. Match the slug exactly after normalising the URL, and let an upstream match take precedence. cdinside a pipeline (:1274):splitShellCommandsdrops the separator kind, socd ../tracker | gh pr comment 1 -b "…"is evaluated as if thecdapplied, although each side of a pipe runs in its own subshell. Only apply acdacross&&,;and newlines.- Relative
git -C(:1282):parsed.gitCwd || virtualCwduses the-Cvalue as-is, socd ../upstream && git -C . commit -m "…"reads the tracker's.git/config. Resolve-CagainstvirtualCwd, and compose repeated-Cs as git does.
Flagged by codex (adversarial review); cross-checked.
Smaller observations
- See inline comments on
plugins/magpie-security/hooks/public-surface-scrub.ts:80,plugins/magpie-security/hooks/public-surface-scrub.ts:1365,plugins/magpie-security/hooks/public-surface-scrub.test.ts:880.
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.
…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.
54adcb6 to
f13243a
Compare
github-actions
Bot
added
capability:reassess
|
All four Major items and the smaller observations have been resolved and verified.
|
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Thanks for round 4: every point from the last review is in, and I checked each against the code — environment.upstream_clone (with ~ expanded) and the worktree gitdir pointer, graphql mutations no longer exempt from the tracker checkout with every -f / -F value scanned, --trailer and the message-reuse options, clustered git tag -am, exact slug matching with upstream precedence, cd only across && / ; / newlines, -C resolved and composed, same-command redirections, and the dead reporterNames / forbidden-terms.json code removed. What remains is narrower: three shapes still let the target be decided by something that does not decide where the command acts, plus a few flag spellings the parser does not read.
Major: a tracker URL in the text being published exempts the command (plugins/magpie-security/hooks/public-surface-scrub.ts:850)
Tracker URLs and
#NNNidentifiers are public-safe. A URL of
the formhttps://github.com/<tracker>/issues/NNN, a
#issuecomment-<C>anchor, or a<tracker>#NNNreference may
appear on any surface — public<upstream>PR descriptions,
— AGENTS.md → Confidentiality of the tracker repository
const urlMatch = token.match(/https?:\/\/github\.com\/([a-z0-9_.-]+\/[a-z0-9_.-]+)\/(?:pull|issues)\/\d+/i);
if (urlMatch) {
result.targetRepo = urlMatch[1].toLowerCase();
The URL search runs over every token, including the -b / --body / --title text, and the match is not anchored. So gh pr comment 1 -b "Fix CVE-2026-1234, see https://github.com/<tracker>/issues/9" sets targetRepo to the tracker, and step 1 at :1452 exempts it, from the upstream checkout. Per AGENTS.md a public fix PR is expected to cite its tracker URL, so this is the ordinary shape of a security-fix PR body, not an edge case.
Suggested fix: take a URL as the target only from the positional PR / issue argument (anchored, ^https://github.com/…/(pull|issues)/\d+$), never from a flag value or body text, and add a test with a tracker URL in the body.
Flagged by codex (adversarial review); cross-checked.
Major: GH_REPO= and {owner}/{repo} decide the target for commands that ignore them (plugins/magpie-security/hooks/public-surface-scrub.ts:515)
Probabilistic at the input, deterministic at every state change.
— PRINCIPLES.md
const ghRepoMatch = envToken.match(/^GH_REPO=([^\s]+)$/i);
if (ghRepoMatch) {
result.targetRepo = ghRepoMatch[1].toLowerCase();
Three shapes give a command a target it does not act on, and the target then exempts it at :1452 (tracker) or :1496 (unrelated repository):
GH_REPO=beforegit: it is read before the binary is known, soGH_REPO=<tracker> git commit -m "Fix CVE-2026-1234"is exempted in the upstream checkout. Git ignoresGH_REPO.GH_REPO=beforegh api graphql: the round-4 change makes a mutation non-exempt only whentargetRepois unset (:1504), soGH_REPO=<tracker> gh api graphql -f query='mutation{addComment(...)}'is exempted again.gh api graphqlignoresGH_REPO; the node id decides where it posts.gh api repos/{owner}/{repo}/issues/1/comments -f body=…(:1160): the literal{owner}/{repo}becomestargetRepo, fails the upstream match and is skipped as unrelated.ghfills those placeholders from the current repository, which is the upstream.
Suggested fix: honour GH_REPO only for gh subcommands that use it (not git, not gh api); never let a target exempt a graphql mutation; resolve {owner}/{repo} from the cwd remote (or fail closed). A test for each.
Flagged by both the primary and adversarial reviewers.
Major: other spellings of the flags the parser reads (plugins/magpie-security/hooks/public-surface-scrub.ts:868)
The boundary never blurs, even when the draft looks reliable enough to short-circuit the gate.
— PRINCIPLES.md
if (token === '--fill' || token === '--fill-first') {
result.hasDynamicInput = true;
Each of these is a documented spelling the parser does not recognise, and each publishes text uninspected:
gh pr create -fand--fill-verbose(here): only--fill/--fill-firstmark the body as dynamic, so the commit messages go into the public PR unread;git commit -CHEAD/-cHEAD(:590): attached short arguments miss the reuse check, which matches only-C,-C=,-c,-c=;gh api … -fbody="…"/-Fbody=@file(:1195): only separate-f/-Ftokens and the long--field=forms are read.
Suggested fix: treat any -f, --fill* on gh pr create as dynamic; match attached short forms (/^-[Cc]./, /^-[fF]./) wherever a short flag takes a value, and add a test per spelling.
Flagged by codex (adversarial review); cross-checked.
Smaller observations
- See inline comments on
plugins/magpie-security/hooks/public-surface-scrub.ts:525,plugins/magpie-security/hooks/public-surface-scrub.ts:1482.
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.
| if (!result.targetRepo) { | ||
| for (let i = cmdIdx + 1; i < tokens.length; i++) { | ||
| const token = tokens[i]; | ||
| const urlMatch = token.match(/https?:\/\/github\.com\/([a-z0-9_.-]+\/[a-z0-9_.-]+)\/(?:pull|issues)\/\d+/i); |
There was a problem hiding this comment.
This search also matches URLs inside the -b / --title text, so a public comment that cites its tracker URL (which AGENTS.md says public fix PRs do) sets targetRepo to the tracker and is exempted at :1452. Please only read the positional PR/issue argument, anchored.
| let cmdIdx = 0; | ||
| while (cmdIdx < tokens.length && /^[a-zA-Z_][a-zA-Z0-9_]*=/.test(tokens[cmdIdx])) { | ||
| const envToken = tokens[cmdIdx]; | ||
| const ghRepoMatch = envToken.match(/^GH_REPO=([^\s]+)$/i); |
There was a problem hiding this comment.
GH_REPO= is applied before the binary is known, so GH_REPO=<tracker> git commit -m "Fix CVE-…" and GH_REPO=<tracker> gh api graphql -f query='mutation{…}' are exempted at :1452, though neither git nor gh api graphql uses it. Related: repos/{owner}/{repo}/… at :1160 becomes a literal "unrelated" target. Details in the review body.
|
|
||
| for (let i = cmdIdx + 3; i < tokens.length; i++) { | ||
| const token = tokens[i]; | ||
| if (token === '--fill' || token === '--fill-first') { |
There was a problem hiding this comment.
-f and --fill-verbose are also gh pr create autofill spellings and pass uninspected here. Same class: attached -CHEAD / -cHEAD at :590, and attached -fbody=… on gh api at :1195. Details in the review body.
| return result; | ||
| } | ||
|
|
||
| const binary = tokens[cmdIdx]; |
There was a problem hiding this comment.
command git …, env … gh … and ( … ) grouping are not recognised: the binary must be the first word, so these pass unread; unwrap the common wrappers and subshell parentheses, or fail closed on a git/gh token in an unrecognised position
| } | ||
| } | ||
| if (!upstreamRepo && remoteInfo.detectedUpstreamSlug) { | ||
| upstreamRepo = remoteInfo.detectedUpstreamSlug.toLowerCase(); |
There was a problem hiding this comment.
with no upstream_repo configured, any repository's first non-tracker remote becomes upstreamRepo for explicit targets, so gh pr comment -R <this repo> -b "…CVE-…" in an unrelated repository is blocked while the same command without -R passes; use detectedUpstreamSlug only when the cwd is under upstream_clone
…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.
… 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
…rub hook
- Parse environment.upstream_clone from user.md to detect upstream
clone sessions without project.md; expand ~ via HOME/USERPROFILE
- Follow .git gitdir pointer in linked worktrees to find common dir
- Never exempt gh api graphql mutations from tracker checkout;
inspect all -f/-F field values in graphql calls, not just body/title
- Extract --trailer values; treat -C, -c, --reuse-message,
--reedit-message, --fixup, --squash as dynamic input (fail closed)
- Recognise clustered short flags containing m/F in git tag hasMsg gate
- Use exact owner/repo slug matching (extractSlugFromUrl) instead of
substring .includes(); upstream match takes precedence over tracker
- Preserve separator type in splitShellSubcommands; cd only persists
across sequential operators (&&, ;, newline), not pipes
- Resolve git -C relative to virtualCwd; compose repeated -C flags
- Track same-command file redirections; flag referenced files written
earlier in the composite command as dynamic input
- Remove dead reporterNames/privateTexts/shingle code and orphaned
forbidden-terms.json; sync-forbidden-terms.py generates .ts only
- Remove (e?.env ?? {}) fallback; read bypass exclusively via $.env.get
- Guard process.env access with typeof check for sandbox safety
- Add 9 targeted tests for review round 4 hardening
896846d to
abc8aa0
Compare
|
Thanks for round 5! All points from this review have been addressed and verified with targeted test cases:
All 79 tests in |
Summary
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.-m,-am,-sm,-sam,-F), attached short flags (-m"...",-Fpath), PR descriptions/comments, heredocs, and shell pipelines.;,&&,||,|,\n).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).upstream_repoandtracker_repofrom<project-config>/project.mdviaresolveProjectConfig(), scoping the scrub strictly toupstream_repoand exemptingtracker_repoper subcommand without hardcoded naming conventions.--amend/--fill, and indeterminate dynamic inputs ($VAR,$(...), stdin-). Resolves-F/--body-filerelative togit -C <dir>without Node built-in imports.options.env.MAGPIE_ALLOW_EMBARGO_TERMS=1environment escape hatch for legitimate post-coordinated-disclosure maintenance (in-command bypasses removed).Type of change
.claude/skills/<name>/) — eval fixtures updated belowtools/<system>/*.md)tools/*/withpyproject.toml)docs/,README.md,CONTRIBUTING.md)projects/_template/)prek, workflows, validators)plugins/magpie-security/hooks/)Test plan
claude plugin validate --strict plugins/magpie-security(Validation passed, minimal API surface:$.fs.read).claude plugin test plugins/magpie-security(47 pass, 0 fail).claude plugin test plugins/magpie-utilitiesandplugins/magpie-setupfor regression testing (83 total tests pass across repo).preklinters (end-of-file-fixer,trailing-whitespace,mixed-line-ending) pass cleanly.RFC-AI-0004 compliance
$.fs.read), nonode:fsornode:pathbuilt-in imports, deterministic execution.issue-fixskill workflow and Markdown verification checklist remain completely untouched for non-Claude harnesses.Linked issues
Notes for reviewers (optional)
tool.callfor Bash tools, halting execution with actionable remediation if unredacted embargo terms appear in public-surface commands.