fix(hooks): merge_guard auth-token + dangerous-pattern defects (#933) by michael-wojcik · Pull Request #1000 · Synaptic-Labs-AI/PACT-Plugin

michael-wojcik · 2026-06-20T08:45:02Z

Summary

Fixes three interrelated defects in the merge_guard hook pair — the SECURITY control that gates destructive git/gh operations behind AskUserQuestion approval. All three were surfaced during the #924 dogfood probe.

Defects fixed

D1 — branch-delete authorization (the headline bug). extract_context captured the delete flag as the branch name (the char class [a-zA-Z0-9/_.-] included -), and _token_matches_command failed --delete/quoted forms — so an approved git branch -D <name> was permanently rejected. The writer branch regex is now flag-agnostic (skips leading flag tokens + first-char name guard); the matcher normalizes surrounding quotes at compare time (capture unchanged → no regex widening → no false-negative risk). PREPARE also found a second, independent matcher bug beyond the issue text, now fixed.

D2 — DANGEROUS_PATTERNS false-positive. A dangerous-op literal inside a non-executing gh issue/pr creation argument (e.g. an issue --title) tripped the pattern scan, blocking benign commands. Adds a 7th strip carrier scoped to the carrier command span (gh …create\b[^&|;]*), then strips each --title/--body/-t/-b quoted value within the span.

Safety invariant (INV-D2): the [^&|;]* span boundary cannot extend past a shell separator, so a compound's executing tail (gh issue create --title "x" && git branch -D real) falls outside the span and is never stripped → it stays caught. The only failure mode is under-strip → over-block (safe). gh pr close --delete-branch is excluded from the carrier verbs by construction, so it stays blocked. Verified empirically by the auditor (compound tail survives verbatim) and pinned by a mechanism-level test.

D3 — push-to-main classifier asymmetry. The post-hook keyword ladder now recognizes a direct push to main/master as force-push, matching the read-side shared classifier (closes the write/read divergence for prose that omits a backtick-quoted command).

Out of scope (tracked separately)

merge_guard: bare merged-only local-ref removal is unclassified (currently ungated) — decide whether to gate #999 — gating the bare merged-only git branch --delete <x> form (currently unclassified). Deferred: it's a net-new detection that would newly gate a previously-allowed operation (a user-facing behavior change). The D1 writer/matcher fixes handle the --delete form correctly once a token reaches them, so no dead code results.
merge_guard_pre: compound-detector false-positives 2>&1 + classification asymmetry + single-use token forces re-approval on retries #720 Option-2 — a structured AskUserQuestion field would eliminate both prose-parsers (D1 writer regex + D3 ladder); a design-shape change, flagged for its own issue.

Testing

40 net-new adversarial tests (test_merge_guard.py, +552 LOC) across 5 classes pinning every fix surface. Each security-critical negative is proven non-vacuous by a distinct mutation toward the forbidden behavior it guards (removal-revert for fix-guards; broaden-the-pattern for the exclusion carve-out). Two phantom-green traps were caught and corrected during proving:

the gh pr close carve-out behavioral assertion was vacuous w.r.t. the carrier → replaced with a mechanism-level pin (a quoted value on a gh pr close --body must survive the strip);
the compound-tail guard is defense-in-depth (span boundary AND quoted-value-only scope), so its faithful regression is the whole-span over-strip.

The D3 end-to-end test mints a real authorization token (write_token in tmp_path), exercising the real token-file seam.

Gate: 1015 passed / 0 failed / 0 errors (rtk proxy python -m pytest + plain pytest cross-check). Both source hooks confirmed clean at HEAD (all proving mutations restored).

Version

PATCH bump 4.4.32 → 4.4.33 (bug fix + additive tests; no new user-facing capability).

Closes #933.

…tic-Labs-AI#933) Three interrelated defects in the merge_guard prose-parsing surface: - D1 (auth token): extract_context captured the delete FLAG as the branch name (char class included '-'), and _token_matches_command failed --delete / quoted forms — so an approved branch force-delete was permanently rejected. The writer branch regex is now flag-agnostic (skips leading flag tokens + first-char name guard); the matcher normalizes surrounding quotes at compare time (capture unchanged, no regex widening, no false-negative risk). - D2 (dangerous-pattern false-positive): a dangerous-op literal inside a non-executing gh issue/pr CREATION argument (e.g. an issue --title) tripped DANGEROUS_PATTERNS. Adds a 7th strip carrier scoped to the carrier command span ([^&|;]*) so a compound's executing tail falls outside the span and stays caught. gh pr close --delete-branch is excluded from the carrier verbs by construction. - D3 (classifier asymmetry): the post-hook keyword ladder now recognizes a direct push to main/master as force-push, matching the read-side shared classifier. Bare merged-only branch-deletion gating deferred to a follow-up issue. Completes ACs of Synaptic-Labs-AI#933; closure pending the TEST phase.

…fixes 40 net-new tests across 5 classes pinning the test contract for the three Synaptic-Labs-AI#933 defects. Every security-critical negative is proven non-vacuous by a distinct mutation toward the forbidden behavior it guards (all mutations restored; both source files clean at HEAD): - TestD1WriterBranchExtraction / TestD1MatcherQuoteNormalization — branch name extraction (never the flag) and quote-normalization positives, plus exact-equality no-widening negatives. - TestD2GhCarrierStrip — false-positive fix, two-value strip, and the INV-D2 security negatives (a compound's executing tail survives the strip; command-substitution and pipe-to-shell preserved). The gh-pr-close carve-out is pinned at the MECHANISM level (a quoted value on a `gh pr close --body` must survive the strip) because the behavioral is_dangerous assertion alone was vacuous w.r.t. the carrier. - TestD3LadderPushToMain / TestD3PushToMainAuthorizationEndToEnd — push-to-main classification and token mint, with a merge-into-main over-reach negative. Gate: 1015 passed / 0 failed / 0 errors (rtk proxy, errors token scanned; cross-checked with plain pytest). Completes ACs of Synaptic-Labs-AI#933; closure pending merge.

…ynaptic-Labs-AI#933) Addresses two non-blocking review findings on PR Synaptic-Labs-AI#1000, both on the merge_guard security control. Committed BEFORE the test additions so the test phase's revert-proving mutations (git checkout HEAD --) restore to the fixed code, not the unfixed baseline. - M1 (D3 ladder, merge_guard_post.py): the push-to-main force-push arm was PR-introduced catastrophic backtracking (write-side ReDoS on operator- authored question text — not a detection bypass). Replaced the nested- quantifier arm with the lazy single-quantifier form push\b.*?\b(?:main|master)\b (+ lazy direct-push variant). Behavior- identical across the ladder; linear (109KB worst case ~7ms vs ~5.6s). - M2+M3 (D2 carrier, merge_guard_pre.py): the carrier span [^&|;]* truncated inside a quoted --title/--body value, over-blocking a benign title that contained a separator or newline (M3 + multi-line). Replaced with a quote- aware scanner that consumes quoted values atomically (internal separators no longer truncate) while stopping at the first UNQUOTED separator (INV-D2 preserved). Inner strip / verb set / guards unchanged; comment corrected. INV-D2 (never weaken detection of a real executing op) was independently re-verified on the as-built code by the security review (byte-identical regexes + live-module behavioral, incl. the escape-parity desync attack). Adversarial test pins (the §7 quote-aware vectors + the M1 perf-bound revert-proving test) follow in a separate commit. Completes ACs of Synaptic-Labs-AI#933 review remediation; closure pending merge.

…2 quote-aware span + D3 ReDoS) 23 net-new tests in test_merge_guard.py pinning the two peer-review remediations committed in the preceding commit. Every security-critical negative and the perf bound is proven non-vacuous by the mutation that exposes the specific guarantee it guards. - TestD2QuoteAwareSpanRemediation (22): the quote-aware D2 span — multi-line / internal-separator titles strip fully (positives), under-block negatives stay caught, escaped/unbalanced-quote desync negatives caught, carve-out regression, span is ReDoS-linear. Positives embed a dangerous literal AFTER the internal separator so the under-consume revert exposes it (RED); under-block negatives flip only under the over-consume whole-span mutation; the gh-pr-close carve-out flips under a close-as-carrier mutation. - TestD3LadderReDoSPerfBound (1): the push-to-main arm completes a 35KB worst-case input under a 100ms bound (fixed ~2-3ms; a greedy-arm revert ~630ms blows the bound by >6x — revert-proving without timing flakiness). Gate: 1038 passed / 0 failed / 0 errors (rtk proxy + plain pytest). Completes ACs of Synaptic-Labs-AI#933 review remediation; closure pending merge.

…ocess-sub closure (#1001, #1002) (#1003) merge_guard_pre hardening — closes two pre-existing defects surfaced in the #933/#1000 review. #1001 (perf/ReDoS): bound the O(n^2) backtracking in the shared global-flag prefixes AND the push-flag-walk — segment-bounded `{0,32}`, disjoint-segment, provably linear on both quadratic call sites (is_dangerous_command + detect_command_operation_type). #1002 (security/under-block): close the output-side process-substitution-to-shell under-block via a two-arm detector (`>&`, `>|`, path-qualified `>(/bin/bash)`, tee-fanout, metachar-separated) wired into ALL 5 stdout-producing strip carriers (echo/printf, gh-creation, heredoc, commit-msg, here-string). INV-D2-monotonic (skip-guard; cannot under-block by construction); no fix-introduced ReDoS; over-block bounded to shell targets. Process: full PACT cycle → 4-reviewer panel → 2 remediation cycles (form-completeness + K=32 + carrier-gating) → final whole-state verify-only round (security + architect + backend all PASS; the whole-state interaction angle caught + closed the carrier-gating gap). Full suite 9343 passed / 0 failures / 0 errors. v4.4.34 (PATCH). Follow-up: #1004 (pre-existing multi-anchor `git push -<flag>` perf quadratic — out of scope, write-side, not a bypass). Closes #1001 Closes #1002

michael-wojcik added 3 commits June 20, 2026 04:14

chore: bump plugin version to 4.4.33

3a1124f

This was referenced Jun 20, 2026

merge_guard: pre-existing O(n^2) backtracking in non-executable-content strip carriers #1001

Closed

merge_guard: process-substitution-to-shell guard misses output-side redirection form (echo/printf under-block) #1002

Closed

michael-wojcik added 2 commits June 20, 2026 10:00

michael-wojcik merged commit 4bd0eff into Synaptic-Labs-AI:main Jun 20, 2026
1 check passed

This was referenced Jun 20, 2026

merge_guard: bare merged-only local-ref removal is unclassified (currently ungated) — decide whether to gate #999

Closed

fix(hooks): merge_guard hardening — bound ReDoS prefixes + output-side process-sub (#1001, #1002) #1003

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(hooks): merge_guard auth-token + dangerous-pattern defects (#933)#1000

fix(hooks): merge_guard auth-token + dangerous-pattern defects (#933)#1000
michael-wojcik merged 5 commits into
Synaptic-Labs-AI:mainfrom
michael-wojcik:fix/933-merge-guard-auth-token

michael-wojcik commented Jun 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

michael-wojcik commented Jun 20, 2026

Summary

Defects fixed

Out of scope (tracked separately)

Testing

Version

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant