From a13a3b263dbfb3980af31595307e8ec9193d8c66 Mon Sep 17 00:00:00 2001 From: Kritoooo Date: Tue, 4 Aug 2026 11:01:50 +0800 Subject: [PATCH 1/2] fix(approval): escape '+' in generated matchers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit quoteBytesForRegexp passed 15 bytes through verbatim, including '+' — a regex quantifier. Of the punctuation on that allowlist (@%=:,/_-) it was the only metacharacter, and it broke matchers in both directions: - a matcher failed to match the command it was generated from, so the grant silently never matched and the operator re-approved forever; - the same pattern matched shorter variants, so an approved "deploy --tag v1+build" also authorized "deploy --tag v1build". Confirmed in production state: of 87 stored session grants, the 3 containing '+' were exactly the 3 that failed to match their own source, and one persistent host rule authorized a command nobody approved. The string-level invariant (anchors, no \s, no .*) cannot catch this — it inspects the pattern's text, not its meaning. So the fix is paired with construction-time proofs: - literalSafePunct is now a named const, with a test asserting every byte satisfies QuoteMeta(b) == b, so re-adding a metacharacter fails CI; - newMatcher gates every generated matcher: it must compile, match its own SourceCmd, and — for exact matchers — parse to exactly Concat[BeginText, Literal(SourceCmd), EndText]. Self-match alone does not imply exactness, and it is the exactness half that catches widening; - FuzzExactMatcherSelfMatch proves exactness by single-byte mutation; FuzzGeneralizeNoWiden covers the prefix path. Construction failures return an error rather than panicking: Generalize runs on agent-supplied input on the Authorize path, and a panic inside ApplyDecision would land between the audit append and the grant write. Those guards surfaced two further pre-existing defects: - invalid UTF-8: Go regexp reads \xF3 as rune U+00F3, not byte 0xF3, so a matcher cannot express a lone invalid byte and would authorize a different command. Rejected via ErrInvalidUTF8Command, like NUL. - leading whitespace: splitASCIIWords drops it while prefixMatcher rebuilds from tokens, so the pattern missed its own source. Now exact. Persisted state is repaired asymmetrically, because only one store keeps the approved text. Session grants carry source_cmd and every one is minted by Exact(req.Cmd), so re-deriving reproduces what approving the same command today would produce — strictly narrowing, and it avoids re-running the very re-approval churn this bug caused (sessionFileVersion 2 + normalizeGrants). policy.yaml has no source_cmd, so legacy rules are detected and refused rather than rewritten from an inferred source; the command falls back to exit 7 and one approval writes a correct rule. Also closes two secondary failure modes found along the way: - a single uncompilable grant aborted the whole locked session, failing every command in it rather than only its own; - applyHostGrant validated the candidate only after the resolution and the approval_granted audit record were written, so a rejection left an approval that could never be re-decided and an audit trail asserting a grant that did not exist. The check now runs first, and additionally verifies the candidate matches its own command. Co-Authored-By: Claude Opus 5 (1M context) --- docs/plans/approval-system.md | 6 + internal/approval/adjudicate.go | 29 ++- internal/approval/authorize.go | 24 ++- internal/approval/generalize.go | 53 ++++- internal/approval/generalize_fuzz_test.go | 107 ++++++++++ internal/approval/generalize_test.go | 169 ++++++++++++++++ internal/approval/legacy_matcher_test.go | 233 ++++++++++++++++++++++ internal/approval/matcher.go | 110 +++++++++- internal/approval/session_store.go | 72 ++++++- 9 files changed, 782 insertions(+), 21 deletions(-) create mode 100644 internal/approval/generalize_fuzz_test.go create mode 100644 internal/approval/legacy_matcher_test.go diff --git a/docs/plans/approval-system.md b/docs/plans/approval-system.md index 31b9cbd..c8079e8 100644 --- a/docs/plans/approval-system.md +++ b/docs/plans/approval-system.md @@ -241,6 +241,10 @@ SKILL.md 指南:拿到 `run` 的 `exit 7` → 把 `id` + 原命令 + 候选范 1. **同 uid 自批准**(核心):见 §11。同 uid 下审批是「护栏 + 审计」,不是沙箱;唯一硬化办法 = 让 agent 跑独立 OS 用户/容器 + broker(可选 hard 模式)。密码学签名方案经红队否决(被改 `policy.yaml` / 直连 ssh 两扇侧门绕过)。 2. **非锚定 / `\s` 正则缺陷 = 立即注入**:`Generalize` 一旦漏锚或用 `\s`,`ls\nrm -rf /` 即可绕过(引擎子串匹配 + `\s` 吃换行,均已实测)。缓解:代码内不变量自检(缺锚、含 `\s`/换行、含 `.*` 即测试失败)+ 注入语料表驱动测试 + fuzz。**最高优先级正确性风险**。 +2b. **转义放行表里的元字符 = 静默改写正则语义(2026-08-03 实测发生)**:`quoteBytesForRegexp` 曾把 `+` 当字面量放行,而 `+` 是量词。后果双向:批准 `echo a+b` 生成的 `\Aecho\x20a+b\z` **既不匹配自己的源串**(grant 静默失效 → 反复重批,一次远端部署里同一命令 12h 内被批 3 次),**又匹配 `echo ab`/`echo aab`**(授权操作者从未批准的命令)。串级不变量(锚定/无 `\s`/无 `.*`)全部通过,挡不住这类缺陷 —— 它们只看正则字符串,不看正则**含义**。 + 缓解(已实施):① 放行表提为 `literalSafePunct` 常量,并有测试逐字节断言 `regexp.QuoteMeta(b) == b`,再加回任何元字符即 CI 失败;② 构造闸门 `newMatcher` 断言**能编译 + 匹配自己的 `SourceCmd` + (exact 档)语法树恰为 `Concat[BeginText, Literal(SourceCmd), EndText]`** —— 最后一条是「只匹配这一条命令」的结构性证明,仅靠自匹配挡不住放宽;③ `FuzzExactMatcherSelfMatch` 用单字节增删改变异证明精确性。 + 同一批护栏另外抓出两个既有缺陷:**非法 UTF-8 命令**(Go 正则里 `\xF3` 表示 *rune* U+00F3 而非字节 0xF3,matcher 无法表达单个非法字节 → 现返回 `ErrInvalidUTF8Command`,与 NUL 同样按 hard-deny 处理)和**前导空格命令**(`splitASCIIWords` 丢前导空格,prefix 正则由 token 重建后匹配不上源串 → 改走 exact)。 + 存量修复:session grant 存了 `source_cmd`,读时按它重算(`sessionFileVersion` 2 + `normalizeGrants`),**严格收窄**且与今天重新审批的结果逐比特相同;`policy.yaml` 没有 `source_cmd`,故只用 `looksLegacyEscaped` 判定后**拒绝采信、不改写**(从可轮转的审计日志反推原文去改写操作者可见的 allow 规则不可靠),命令回落 exit 7 重批一次即可。 3. **`prefix` 档(需显式开)的过度放宽**:会放宽写类多路子命令(`systemctl restart *`、`docker run *`、`git push *`)与读任意文件 leaf(`cat *`)。**默认 `safe-prefix` 不放宽这些**;开 `prefix` 即接受该取舍。边界仍由 解释器/特权/破坏性 denylist + 分离引擎兜。 4. **审计 hash 回归**:新字段必须 `,omitempty` 且 `Record`/`canonicalRecord` 同步;用 golden 链 + 逐字段 tamper 测试守住。 5. **resolution 重放 / 陈旧 pending**:`req_digest` 绑定 + `O_EXCL` + `id` 归属校验;pending/responses 按 TTL 清扫。无 HMAC,故不防同 uid **伪造**(同 §11),但防住**意外串号/重放**。 @@ -249,6 +253,8 @@ SKILL.md 指南:拿到 `run` 的 `exit 7` → 把 `id` + 原命令 + 候选范 ## 14. 测试要点 - `Generalize`:护栏 denylist、元字符/控制字符/Unicode 空白拒绝、锚定 + 无 `\s`/`.*` 不变量、prefix/exact 分流、字节级切词器;表驱动 + 注入语料(`\n \r \t \f \v` / Unicode 空白 / `$()` / 管道 / 反斜杠续行 / 引号 / glob)+ fuzz;**专测 `ls\nrm -rf /` 不被任何前缀 grant 命中**。 +- **matcher 语义(见 §13.2b)**:放行表逐字节 `QuoteMeta` 自检;每个可打印字节 `ab` 既匹配源串、又不匹配去掉该字节的变体(量词探针);`+` 放宽用例(`echo a+b` 不得命中 `echo ab`);非法 UTF-8 与前导空格分流;`FuzzExactMatcherSelfMatch`(单字节增删改变异证明只匹配源串)+ `FuzzGeneralizeNoWiden`(不跨 `;`/换行/管道延伸)。 +- **存量修复**:v1 session 文件里的 legacy grant 按 `source_cmd` 重算且**修复被落盘**(不是每次读都重做);无法重算的丢弃;单条坏 grant 不得使整个 session 的匹配失败;legacy host 规则不再进 `hostMatchers`。 - `Authorize`:**host_overrides 物理在前 + global 有显式 deny → 必返 HardDeny**;session/host grant 命中;实时重判(运行前新增 deny 使 grant 失效);once 在并发重跑下只被消费一次。 - session store:flock 并发、TTL 过期、`session end` 清除、`host` 绑定。 - request/resolution:`req_digest` 不符当未批;`O_EXCL` 防覆盖;`approval wait/status` 各退出码与缺失/过期/畸形分支。 diff --git a/internal/approval/adjudicate.go b/internal/approval/adjudicate.go index c566d64..3f767db 100644 --- a/internal/approval/adjudicate.go +++ b/internal/approval/adjudicate.go @@ -50,7 +50,13 @@ func ApplyDecision(opts ApplyOptions, id string, verdict Verdict, scope Scope) ( if !req.Candidate.Promotable { return ApplyResult{}, fmt.Errorf("approval %s cannot be promoted to host scope", req.ID) } - if err := validateMatcherInvariant(req.Candidate.Regex); err != nil { + // Validate the stored candidate BEFORE anything durable is written. + // applyHostGrant persists req.Candidate.Regex verbatim, so a request + // minted by an older binary can carry a matcher that does not match + // its own command. Rejecting here — ahead of Resolve and the audit + // append — keeps a rejection from burning the resolution and leaving + // an approval that can never be re-decided. + if err := validateHostCandidate(req); err != nil { return ApplyResult{}, err } } else if _, err := Exact(req.Cmd); err != nil { @@ -116,7 +122,7 @@ func applyHostGrant(opts ApplyOptions, req PendingRequest) (string, error) { if !req.Candidate.Promotable { return "", fmt.Errorf("approval %s cannot be promoted to host scope", req.ID) } - if err := validateMatcherInvariant(req.Candidate.Regex); err != nil { + if err := validateHostCandidate(req); err != nil { return "", err } next := opts.Bundle @@ -160,6 +166,25 @@ func applyHostGrant(opts ApplyOptions, req PendingRequest) (string, error) { return ruleName, nil } +// validateHostCandidate proves a pending request's stored matcher is safe to +// persist as a host rule: it must satisfy the string invariant, compile, and +// actually match the command the operator reviewed. The last check is what +// stops a request minted before the '+' escaping fix from being promoted into +// a permanent rule that authorizes commands nobody approved. +func validateHostCandidate(req PendingRequest) error { + expr, err := compileMatcher(req.Candidate.Regex) + if err != nil { + return err + } + if !expr.MatchString(req.Cmd) { + return fmt.Errorf("approval %s carries a matcher that does not match its own command; have the agent re-submit the request", req.ID) + } + if looksLegacyEscaped(req.Candidate.Regex) { + return fmt.Errorf("approval %s was created by an older AgentSSH with an unsafe matcher; have the agent re-submit the request", req.ID) + } + return nil +} + func appendApprovalAudit(opts ApplyOptions, req PendingRequest, verdict Verdict, scope Scope) error { if opts.Audit.Path == "" { return nil diff --git a/internal/approval/authorize.go b/internal/approval/authorize.go index ca2e4ba..e5f3a08 100644 --- a/internal/approval/authorize.go +++ b/internal/approval/authorize.go @@ -126,7 +126,7 @@ func authorize(cfg policy.Config, inv inventory.Inventory, sessionStore SessionS } } matcher, err := Generalize(command, runtime.HostGrantMode) - if errors.Is(err, ErrNULCommand) { + if errors.Is(err, ErrNULCommand) || errors.Is(err, ErrInvalidUTF8Command) { return Authorization{Status: AuthHardDeny, Decision: decision}, nil } if err != nil { @@ -162,6 +162,16 @@ func splitPolicy(cfg policy.Config, host string) (policy.Config, []Matcher) { continue } if key == policy.HostRulesKey(host) { + // A rule minted before '+' was escaped authorizes commands the + // operator never approved, and policy.yaml stores no source + // command to repair it from. Refuse to honor it: the command + // falls back to needs-approval, and approving once writes a + // correct rule alongside. The stale rule stays in the file as + // inert text — rewriting an operator-owned allow rule from an + // inferred source would be worse than one re-approval. + if err := CheckHostGrantRule(rule); err != nil { + continue + } hostMatchers = append(hostMatchers, Matcher{ Kind: MatcherExact, Regex: rule.Match.CmdRegex, @@ -200,9 +210,19 @@ func copyPolicyConfig(cfg policy.Config) policy.Config { return next } +// CheckHostGrantRule reports whether a persisted approval rule is still safe to +// honor. Beyond the string invariant it rejects patterns generated before '+' +// was escaped: those act as quantifiers and authorize commands the operator +// never approved. func CheckHostGrantRule(rule policy.Rule) error { if rule.Group != policy.ApprovalGroup { return fmt.Errorf("rule is not an approval host rule") } - return validateMatcherInvariant(rule.Match.CmdRegex) + if _, err := compileMatcher(rule.Match.CmdRegex); err != nil { + return err + } + if looksLegacyEscaped(rule.Match.CmdRegex) { + return fmt.Errorf("approval rule %s was generated by an older AgentSSH and may match commands that were never approved; re-approve the command and remove this rule", rule.Name) + } + return nil } diff --git a/internal/approval/generalize.go b/internal/approval/generalize.go index a88d4c7..2c60f12 100644 --- a/internal/approval/generalize.go +++ b/internal/approval/generalize.go @@ -18,8 +18,28 @@ const ( var ErrNULCommand = errors.New("approval command contains NUL") +// ErrInvalidUTF8Command rejects commands that are not valid UTF-8. Go's regexp +// has no way to express a single invalid byte: `\xF3` in a pattern denotes the +// *rune* U+00F3 (encoded C3 B3), not the byte 0xF3. A matcher generated from +// such a command would therefore fail to match the approved command while +// matching a different one, so these commands are rejected outright rather +// than approved with a matcher that cannot mean what it says. +var ErrInvalidUTF8Command = errors.New("approval command is not valid UTF-8") + const tailTokenClass = `[A-Za-z0-9@%+=:,./_-]+` +// literalSafePunct lists the punctuation bytes quoteBytesForRegexp may emit +// verbatim into a regex. Every byte here MUST be a regex non-metacharacter: +// the output is spliced into a pattern outside any character class, so a +// quantifier ('+', '*', '?') would silently rewrite the pattern's meaning. +// '+' used to live here and did exactly that — an approved "a+b" produced +// \Aa+b\z, which failed to match its own source yet matched "ab" and "aab", +// authorizing commands the operator never approved. +// TestLiteralAllowlistContainsNoRegexMetachar enforces this invariant. +// Note: tailTokenClass above is a character class, where '+' is literal and +// the trailing '+' is a deliberate quantifier — that one is correct as-is. +const literalSafePunct = `@%=:,/_-` + var interpreterOrEscapable = map[string]struct{}{ "sh": {}, "bash": {}, "dash": {}, "zsh": {}, "ksh": {}, "env": {}, "find": {}, "xargs": {}, "awk": {}, "gawk": {}, "sed": {}, "perl": {}, "ruby": {}, "node": {}, "php": {}, @@ -57,13 +77,16 @@ func Generalize(command string, mode HostGrantMode) (Matcher, error) { if strings.Contains(command, "\x00") { return Matcher{}, ErrNULCommand } + if !utf8.ValidString(command) { + return Matcher{}, ErrInvalidUTF8Command + } if mode == "" { mode = HostGrantSafePrefix } forceExact := scanForExactOnly(command) tokens := splitASCIIWords(command) if len(tokens) == 0 { - return exactMatcher(command, true), nil + return exactMatcher(command, true) } head := commandBase(tokens[0]) promotable := true @@ -74,27 +97,37 @@ func Generalize(command string, mode HostGrantMode) (Matcher, error) { if isInterpreterOrEscapable(head) || isDestructiveLeaf(head) || hasEnvPrefix(command) { forceExact = true } + // splitASCIIWords drops leading spaces, and prefixMatcher rebuilds the + // pattern from tokens — so a leading-space command would produce a prefix + // matcher that cannot match the command it came from. The exact matcher + // preserves bytes, and is the narrower choice regardless. + if strings.HasPrefix(command, " ") { + forceExact = true + } if forceExact || mode == HostGrantExact { - return exactMatcher(command, promotable), nil + return exactMatcher(command, promotable) } prefix := prefixForMode(tokens, head, mode) tail := tokens[len(prefix):] if len(prefix) == 0 || !tailTokensSafe(tail) || (mode == HostGrantSafePrefix && !safePrefixTailTokensSafe(head, tail)) { - return exactMatcher(command, promotable), nil + return exactMatcher(command, promotable) } - return prefixMatcher(command, prefix, promotable), nil + return prefixMatcher(command, prefix, promotable) } func Exact(command string) (Matcher, error) { if strings.Contains(command, "\x00") { return Matcher{}, ErrNULCommand } - return exactMatcher(command, true), nil + if !utf8.ValidString(command) { + return Matcher{}, ErrInvalidUTF8Command + } + return exactMatcher(command, true) } -func exactMatcher(command string, promotable bool) Matcher { - return mustValidateMatcher(Matcher{ +func exactMatcher(command string, promotable bool) (Matcher, error) { + return newMatcher(Matcher{ Kind: MatcherExact, Regex: `\A` + quoteBytesForRegexp(command) + `\z`, Promotable: promotable, @@ -102,12 +135,12 @@ func exactMatcher(command string, promotable bool) Matcher { }) } -func prefixMatcher(command string, prefix []string, promotable bool) Matcher { +func prefixMatcher(command string, prefix []string, promotable bool) (Matcher, error) { parts := make([]string, 0, len(prefix)) for _, token := range prefix { parts = append(parts, quoteBytesForRegexp(token)) } - return mustValidateMatcher(Matcher{ + return newMatcher(Matcher{ Kind: MatcherPrefix, Regex: `\A` + strings.Join(parts, `[ \t]+`) + `(?:[ \t]+` + tailTokenClass + `)*[ \t]*\z`, Prefix: append([]string(nil), prefix...), @@ -299,7 +332,7 @@ func quoteBytesForRegexp(value string) string { for i := 0; i < len(value); { b := value[i] if (b >= 'A' && b <= 'Z') || (b >= 'a' && b <= 'z') || (b >= '0' && b <= '9') || - b == '@' || b == '%' || b == '+' || b == '=' || b == ':' || b == ',' || b == '/' || b == '_' || b == '-' { + strings.IndexByte(literalSafePunct, b) >= 0 { builder.WriteByte(b) i++ continue diff --git a/internal/approval/generalize_fuzz_test.go b/internal/approval/generalize_fuzz_test.go new file mode 100644 index 0000000..32805ec --- /dev/null +++ b/internal/approval/generalize_fuzz_test.go @@ -0,0 +1,107 @@ +package approval + +import ( + "regexp" + "strings" + "testing" + "unicode/utf8" +) + +// FuzzExactMatcherSelfMatch asserts the two properties an exact matcher must +// hold for any command: it matches the command it was generated from, and it +// matches nothing else. The '+' defect violated both at once — the generated +// pattern failed against its own source while matching shorter variants — and +// went unnoticed because the hand-written corpus happened to contain no '+'. +func FuzzExactMatcherSelfMatch(f *testing.F) { + seeds := []string{ + "echo a+b", + "x++", + "a.+b", + "deploy --tag v1+build", + `docker exec c python -c "g=lambda p:u.urlopen(b+p,timeout=30)"`, + "systemctl status nginx", + "ls -la /var", + "日志 --tail 20", + "", + " ", + } + // Every byte quoteBytesForRegexp may emit verbatim, so a future edit to the + // allowlist is exercised here as well as in the table test. + for i := 0; i < len(literalSafePunct); i++ { + seeds = append(seeds, "a"+string(rune(literalSafePunct[i]))+"b") + } + for _, seed := range seeds { + f.Add(seed) + } + + f.Fuzz(func(t *testing.T, command string) { + // NUL and invalid UTF-8 are rejected by contract (a matcher cannot + // express a lone invalid byte), and long inputs only slow the mutation + // sweep below without reaching new generator paths. + if strings.ContainsRune(command, 0) || !utf8.ValidString(command) || len(command) > 256 { + t.Skip() + } + matcher, err := Exact(command) + if err != nil { + t.Fatalf("Exact(%q): %v", command, err) + } + expr, err := regexp.Compile(matcher.Regex) + if err != nil { + t.Fatalf("generated regex does not compile: %v regex=%s", err, matcher.Regex) + } + if !expr.MatchString(command) { + t.Fatalf("matcher does not match its source command %q: regex=%s", command, matcher.Regex) + } + // Exactness: no single-byte edit of the command may still match. Each + // mutation changes the length by exactly one, so a mutant can never + // coincide with the original and there are no false failures. + limit := min(len(command), 64) + for i := 0; i < limit; i++ { + mutations := []string{ + command[:i] + command[i+1:], // delete byte i + command[:i] + command[i:i+1] + command[i:], // duplicate byte i + command[:i] + "X" + command[i:], // insert at i + } + for _, mutation := range mutations { + if expr.MatchString(mutation) { + t.Fatalf("matcher for %q also matched %q: regex=%s", command, mutation, matcher.Regex) + } + } + } + }) +} + +// FuzzGeneralizeNoWiden covers the prefix path, where a matcher legitimately +// matches more than its source: it must still never extend across a command +// separator into an unapproved command. +func FuzzGeneralizeNoWiden(f *testing.F) { + for _, seed := range []string{ + "systemctl status nginx", + "ls -la /var", + "git diff HEAD", + "kubectl get pods", + "echo a+b", + } { + f.Add(seed) + } + f.Fuzz(func(t *testing.T, command string) { + if strings.ContainsRune(command, 0) || !utf8.ValidString(command) || len(command) > 256 { + t.Skip() + } + for _, mode := range []HostGrantMode{HostGrantExact, HostGrantSafePrefix, HostGrantPrefix} { + matcher, err := Generalize(command, mode) + if err != nil { + t.Fatalf("Generalize(%q, %s): %v", command, mode, err) + } + matched, err := matcher.Match(command) + if err != nil || !matched { + t.Fatalf("matcher does not match its source command %q (mode %s): matched=%v err=%v regex=%s", command, mode, matched, err, matcher.Regex) + } + for _, suffix := range []string{"; rm -rf /", "\nid", " | id", " && id", "`id`"} { + if matched, err := matcher.Match(command + suffix); err != nil || matched { + t.Fatalf("matcher for %q (mode %s) reached across %q: matched=%v err=%v regex=%s", command, mode, suffix, matched, err, matcher.Regex) + } + } + } + }) +} diff --git a/internal/approval/generalize_test.go b/internal/approval/generalize_test.go index 5530ec0..1c40c63 100644 --- a/internal/approval/generalize_test.go +++ b/internal/approval/generalize_test.go @@ -137,6 +137,31 @@ func TestGeneralizeRejectsNUL(t *testing.T) { } } +// Go's regexp cannot express a single invalid byte: `\xF3` in a pattern means +// the rune U+00F3 (encoded C3 B3), not the byte 0xF3. A matcher built from an +// invalid-UTF-8 command would fail to match that command while matching a +// different one, so such commands are rejected instead of approved. +func TestGeneralizeRejectsInvalidUTF8(t *testing.T) { + for _, command := range []string{"\xf3", "ls \xff", "echo \xc3(", "\xed\xa0\x80"} { + t.Run(command, func(t *testing.T) { + if _, err := Generalize(command, HostGrantSafePrefix); err != ErrInvalidUTF8Command { + t.Fatalf("Generalize err = %v, want ErrInvalidUTF8Command", err) + } + if _, err := Exact(command); err != ErrInvalidUTF8Command { + t.Fatalf("Exact err = %v, want ErrInvalidUTF8Command", err) + } + }) + } + // Valid multibyte UTF-8 stays supported. + matcher, err := Exact("echo 日志") + if err != nil { + t.Fatalf("Exact on valid UTF-8: %v", err) + } + if matched, err := matcher.Match("echo 日志"); err != nil || !matched { + t.Fatalf("valid UTF-8 matcher should match its source: matched=%v err=%v", matched, err) + } +} + func TestPrefixMatcherDoesNotMatchNewlineInjection(t *testing.T) { matcher, err := Generalize("ls /var", HostGrantSafePrefix) if err != nil { @@ -170,6 +195,150 @@ func TestMatcherInvariantRejectsUnsafeRegexes(t *testing.T) { } } +// The bytes quoteBytesForRegexp emits verbatim are spliced into a pattern +// outside any character class, so any regex metacharacter among them silently +// rewrites the pattern's meaning. '+' used to be on this list and did exactly +// that. This test is what makes the narrow allowlist safe to keep. +func TestLiteralAllowlistContainsNoRegexMetachar(t *testing.T) { + for i := 0; i < len(literalSafePunct); i++ { + b := literalSafePunct[i] + t.Run(string(rune(b)), func(t *testing.T) { + if quoted := regexp.QuoteMeta(string(b)); quoted != string(b) { + t.Fatalf("literalSafePunct contains regex metacharacter %q (QuoteMeta = %q); it must be escaped, not passed through", string(b), quoted) + } + }) + } +} + +// Every printable byte must survive the round trip: the generated matcher has +// to match the command it came from, and must not match the command with that +// byte removed. The second half is the one that catches quantifiers — a +// quantifier makes the preceding byte optional or repeatable, so the pattern +// widens to commands the operator never approved. +func TestQuoteBytesForRegexpEscapesEveryPrintableByte(t *testing.T) { + for b := byte(0x20); b <= 0x7E; b++ { + command := "a" + string(rune(b)) + "b" + t.Run(command, func(t *testing.T) { + matcher, err := Exact(command) + if err != nil { + t.Fatalf("Exact(%q): %v", command, err) + } + assertMatcherInvariant(t, matcher.Regex) + if matched, err := matcher.Match(command); err != nil || !matched { + t.Fatalf("matcher must match its source command: matched=%v err=%v regex=%s", matched, err, matcher.Regex) + } + if matched, err := matcher.Match("ab"); err != nil || matched { + t.Fatalf("matcher for %q widened to %q: matched=%v err=%v regex=%s", command, "ab", matched, err, matcher.Regex) + } + }) + } +} + +// Regression detail for the '+' defect. Before the fix each of these matchers +// failed to match its own source (forcing endless re-approval) while matching +// a command the operator never saw. +func TestExactMatcherPlusEscalations(t *testing.T) { + tests := []struct { + name string + command string + widened []string + }{ + { + name: "single plus", + command: "echo a+b", + widened: []string{"echo ab", "echo aab", "echo aaaaab"}, + }, + { + name: "plus in a tag argument", + command: "deploy --tag v1+build", + widened: []string{"deploy --tag v1build", "deploy --tag v11build"}, + }, + { + name: "escaped dot followed by plus", + command: "a.+b", + widened: []string{"a..b", "a.b"}, + }, + { + name: "urlopen expression from the production incident", + command: `python -c "g=lambda p:u.urlopen(b+p,timeout=30)"`, + widened: []string{`python -c "g=lambda p:u.urlopen(bp,timeout=30)"`}, + }, + { + // Nested quantifiers used to make the pattern fail to compile, and + // it was stored anyway — surfacing only at match time, against + // unrelated commands. Escaping '+' removes the failure mode + // entirely: this is now an ordinary literal. + name: "double plus is a literal, not a nested quantifier", + command: "x++", + widened: []string{"x", "x+", "x+++"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + matcher, err := Exact(tt.command) + if err != nil { + t.Fatalf("Exact(%q): %v", tt.command, err) + } + assertMatcherInvariant(t, matcher.Regex) + if matched, err := matcher.Match(tt.command); err != nil || !matched { + t.Fatalf("matcher must match its source command: matched=%v err=%v regex=%s", matched, err, matcher.Regex) + } + for _, widened := range tt.widened { + if matched, err := matcher.Match(widened); err != nil || matched { + t.Fatalf("matcher for %q authorized unapproved command %q: matched=%v err=%v regex=%s", tt.command, widened, matched, err, matcher.Regex) + } + } + }) + } +} + +func TestLooksLegacyEscapedDetectsBarePlusOnly(t *testing.T) { + legacy, err := Generalize("ls -la /var", HostGrantSafePrefix) + if err != nil { + t.Fatalf("Generalize: %v", err) + } + if legacy.Kind != MatcherPrefix { + t.Fatalf("kind = %s, want prefix (structural '+' must be present to test the strip)", legacy.Kind) + } + // A prefix matcher's structural '+' in `[ \t]+` and tailTokenClass is + // legitimate and must not be mistaken for the legacy defect. + if looksLegacyEscaped(legacy.Regex) { + t.Fatalf("prefix matcher misreported as legacy: %s", legacy.Regex) + } + current, err := Exact("echo a+b") + if err != nil { + t.Fatalf("Exact: %v", err) + } + if looksLegacyEscaped(current.Regex) { + t.Fatalf("current exact matcher misreported as legacy: %s", current.Regex) + } + if !looksLegacyEscaped(`\Aecho\x20a+b\z`) { + t.Fatal("legacy pattern with a bare '+' was not detected") + } +} + +// splitASCIIWords drops leading spaces, so rebuilding a prefix pattern from +// tokens would lose them and the matcher would miss its own source command. +func TestGeneralizeLeadingSpaceForcesExact(t *testing.T) { + for _, command := range []string{" ls -la /var", " systemctl status nginx", " git diff HEAD"} { + t.Run(command, func(t *testing.T) { + matcher, err := Generalize(command, HostGrantSafePrefix) + if err != nil { + t.Fatalf("Generalize: %v", err) + } + if matcher.Kind != MatcherExact { + t.Fatalf("kind = %s, want exact: %#v", matcher.Kind, matcher) + } + if matched, err := matcher.Match(command); err != nil || !matched { + t.Fatalf("matcher must match its source command: matched=%v err=%v regex=%s", matched, err, matcher.Regex) + } + if matched, err := matcher.Match(strings.TrimLeft(command, " ")); err != nil || matched { + t.Fatalf("matcher widened across leading whitespace: matched=%v err=%v regex=%s", matched, err, matcher.Regex) + } + }) + } +} + func TestSplitASCIIWordsDoesNotUseUnicodeWhitespace(t *testing.T) { got := splitASCIIWords("a\tb c\u00a0d e") want := []string{"a\tb", "c\u00a0d", "e"} diff --git a/internal/approval/legacy_matcher_test.go b/internal/approval/legacy_matcher_test.go new file mode 100644 index 0000000..e844965 --- /dev/null +++ b/internal/approval/legacy_matcher_test.go @@ -0,0 +1,233 @@ +package approval + +import ( + "encoding/json" + "fmt" + "os" + "strings" + "testing" + "time" + + "github.com/Praeviso/AgentSSH/internal/inventory" + "github.com/Praeviso/AgentSSH/internal/policy" +) + +// The '+' byte used to be emitted into the pattern verbatim, where it acts as +// a quantifier. Grants and host rules minted before the fix are still on disk: +// they fail to match the command the operator approved while matching commands +// the operator never saw. These tests cover the repair of that persisted state. + +// legacyExactRegex reproduces what the pre-fix generator emitted for a command: +// every byte escaped except the old allowlist, which included '+'. +func legacyExactRegex(command string) string { + matcher, err := Exact(command) + if err != nil { + panic(err) + } + return strings.ReplaceAll(matcher.Regex, `\x2B`, "+") +} + +func writeLegacySessionFile(t *testing.T, dir string, sessionID string, grants []Grant) { + t.Helper() + doc := sessionFile{Version: 1, SessionID: sessionID, Updated: "2026-08-03T00:00:00Z", Grants: grants} + data, err := json.MarshalIndent(doc, "", " ") + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(sessionPath(dir, sessionID), append(data, '\n'), 0o600); err != nil { + t.Fatal(err) + } +} + +func TestSessionStoreRepairsLegacyPlusGrantOnRead(t *testing.T) { + const command = `docker exec c python -c "g=lambda p:u.urlopen(b+p,timeout=30)"` + dir := t.TempDir() + now := time.Date(2026, 8, 3, 0, 0, 0, 0, time.UTC) + writeLegacySessionFile(t, dir, "s_legacy", []Grant{{ + Scope: ScopeSession, + Kind: MatcherExact, + Regex: legacyExactRegex(command), + SourceCmd: command, + Host: "web-1", + GrantedTS: "2026-08-03T00:00:00Z", + ExpiresTS: now.Add(12 * time.Hour).UTC().Format(time.RFC3339), + ApprovalID: "ap_0123456789abcdef01234567", + ReqID: "r1", + }}) + store := SessionStore{Dir: dir, Now: func() time.Time { return now }} + + // The approved command is authorized again — this is the incident. + if _, ok, err := store.Peek("s_legacy", "web-1", command, ""); err != nil || !ok { + t.Fatalf("repaired grant should authorize the approved command: ok=%v err=%v", ok, err) + } + // And the plus-stripped variant, which the legacy pattern authorized, is not. + widened := strings.ReplaceAll(command, "+", "") + if _, ok, err := store.Peek("s_legacy", "web-1", widened, ""); err != nil || ok { + t.Fatalf("repaired grant must not authorize unapproved command %q: ok=%v err=%v", widened, ok, err) + } + + var doc sessionFile + data, err := os.ReadFile(sessionPath(dir, "s_legacy")) + if err != nil { + t.Fatal(err) + } + if err := json.Unmarshal(data, &doc); err != nil { + t.Fatal(err) + } + if doc.Version != sessionFileVersion { + t.Fatalf("version = %d, want %d", doc.Version, sessionFileVersion) + } + if len(doc.Grants) != 1 || strings.Contains(doc.Grants[0].Regex, "+") { + t.Fatalf("repair was not persisted: %#v", doc.Grants) + } +} + +func TestSessionStoreDropsUnrepairableGrant(t *testing.T) { + dir := t.TempDir() + now := time.Date(2026, 8, 3, 0, 0, 0, 0, time.UTC) + expires := now.Add(12 * time.Hour).UTC().Format(time.RFC3339) + good, err := Exact("systemctl status nginx") + if err != nil { + t.Fatal(err) + } + writeLegacySessionFile(t, dir, "s_mixed", []Grant{ + { + // No SourceCmd to re-derive from, and the pattern is legacy. + Scope: ScopeSession, Kind: MatcherExact, Regex: `\Aecho\x20a+b\z`, + Host: "web-1", ExpiresTS: expires, ApprovalID: "ap_bad", ReqID: "r1", + }, + { + Scope: ScopeSession, Kind: good.Kind, Regex: good.Regex, SourceCmd: good.SourceCmd, + Host: "web-1", ExpiresTS: expires, ApprovalID: "ap_good", ReqID: "r2", + }, + }) + store := SessionStore{Dir: dir, Now: func() time.Time { return now }} + + if _, ok, err := store.Peek("s_mixed", "web-1", "echo ab", ""); err != nil || ok { + t.Fatalf("dropped grant must not authorize anything: ok=%v err=%v", ok, err) + } + // A poisoned neighbour must not take the healthy grant down with it. + if _, ok, err := store.Peek("s_mixed", "web-1", "systemctl status nginx", ""); err != nil || !ok { + t.Fatalf("healthy grant should still authorize: ok=%v err=%v", ok, err) + } +} + +// A grant whose stored pattern does not compile used to abort the whole locked +// session, failing every command in it rather than only its own. +func TestSessionStoreUncompilableGrantDoesNotBrickSession(t *testing.T) { + dir := t.TempDir() + now := time.Date(2026, 8, 3, 0, 0, 0, 0, time.UTC) + expires := now.Add(12 * time.Hour).UTC().Format(time.RFC3339) + good, err := Exact("systemctl status nginx") + if err != nil { + t.Fatal(err) + } + doc := sessionFile{Version: sessionFileVersion, SessionID: "s_poison", Grants: []Grant{ + {Scope: ScopeSession, Kind: MatcherExact, Regex: `\Ax++\z`, SourceCmd: "x++", + Host: "web-1", ExpiresTS: expires, ApprovalID: "ap_bad", ReqID: "r1"}, + {Scope: ScopeSession, Kind: good.Kind, Regex: good.Regex, SourceCmd: good.SourceCmd, + Host: "web-1", ExpiresTS: expires, ApprovalID: "ap_good", ReqID: "r2"}, + }} + data, err := json.MarshalIndent(doc, "", " ") + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(sessionPath(dir, "s_poison"), append(data, '\n'), 0o600); err != nil { + t.Fatal(err) + } + store := SessionStore{Dir: dir, Now: func() time.Time { return now }} + if _, ok, err := store.Peek("s_poison", "web-1", "systemctl status nginx", ""); err != nil || !ok { + t.Fatalf("healthy grant should authorize despite a poisoned neighbour: ok=%v err=%v", ok, err) + } +} + +func TestLegacyHostRuleStopsAuthorizing(t *testing.T) { + const command = "deploy --tag v1+build" + cfg := policy.Config{ + Version: 1, + HostOverrides: map[string]policy.HostOverride{ + policy.HostRulesKey("web-1"): {Rules: []policy.Rule{{ + Name: "approval/legacy", + Match: policy.Match{CmdRegex: legacyExactRegex(command)}, + Action: policy.ActionAllow, + Group: policy.ApprovalGroup, + }}}, + }, + } + inv := inventory.Inventory{Hosts: map[string]inventory.Host{"web-1": {}}} + store := SessionStore{Dir: t.TempDir()} + runtime := RuntimeConfig{Enabled: true, HostGrantMode: HostGrantSafePrefix} + + // The rule authorized the plus-stripped command, which nobody approved. + widened := strings.ReplaceAll(command, "+", "") + auth, err := PreflightAuthorize(cfg, inv, store, runtime, "s_1", "web-1", widened, "") + if err != nil { + t.Fatal(err) + } + if auth.Status != AuthNeedsApproval { + t.Fatalf("legacy rule still authorizes unapproved command %q: status=%s rule=%s", widened, auth.Status, auth.Decision.Rule) + } + // It never matched the approved command, so that falls back to approval too. + auth, err = PreflightAuthorize(cfg, inv, store, runtime, "s_1", "web-1", command, "") + if err != nil { + t.Fatal(err) + } + if auth.Status != AuthNeedsApproval { + t.Fatalf("status = %s, want needs_approval", auth.Status) + } + if err := CheckHostGrantRule(cfg.HostOverrides[policy.HostRulesKey("web-1")].Rules[0]); err == nil { + t.Fatal("CheckHostGrantRule accepted a legacy pattern") + } +} + +// The exact commands from the production incident of 2026-08-03: the same +// command was re-approved three times inside one 12h session TTL because the +// grant's pattern, generated with an unescaped '+', could not match the +// command it was minted from. The load-bearing fragment is `u.urlopen(b+p, +// timeout=30)`. +const incidentCommand = `docker exec handrail-handrail-harness-1 python -c "import json,urllib.request as u; b='http://handrail-api:8080'; g=lambda p:json.load(u.urlopen(b+p,timeout=30)); ss=[s for s in g('/sessions') if s.get('template_id')=='lite-deployment-e2e']; assert ss,'e2e session missing'; s=max(ss,key=lambda x:x['created_at']); sid=s['id']; s=g('/sessions/'+sid); assert s['status']=='completed' and s['current_run_status']=='completed' and s['final_output'].strip()=='LITE_E2E_OK'; p=g('/admin/model-providers/cliproxy'); assert p['status']=='active' and p['default_model']=='deepseek-v4-flash' and p['protocol']=='openai_chat_completions'; v=g('/admin/agent-versions/'+s['agent_version_id']); assert v['model']=='deepseek-v4-flash' and v['model_provider_id']=='cliproxy'; es=g('/sessions/'+sid+'/events'); ts=[e['type'] for e in es]; assert 'run_completed' in ts and 'output_exported' in ts and 'tool_failed' not in ts; ars=g('/sessions/'+sid+'/artifacts'); a=[x for x in ars if x['kind']=='task_output' and x['name']=='lite-proof.txt']; assert len(a)==1; raw=u.urlopen(b+'/sessions/'+sid+'/artifacts/'+a[0]['id']+'/download?actor_type=user&actor_id=deployment-e2e',timeout=30).read(); assert raw.decode().strip()=='LITE_E2E_OK'; print(json.dumps({'status':'passed','session_id':sid,'run_id':s['current_run_id'],'model':v['model'],'artifact_id':a[0]['id'],'artifact_bytes':len(raw)},sort_keys=True))"` + +const incidentHostCommand = `docker exec handrail-handrail-harness-1 python -c "from pathlib import Path; p=Path('/run/handrail/secrets/model-providers/cliproxy/api-key'); ok=p.is_file() and p.stat().st_size>0; print('cliproxy_secret=' + ('present bytes=' + str(p.stat().st_size) if ok else 'missing')); raise SystemExit(0 if ok else 1)"` + +func TestAuthorizeGrantForPlusCommandFromProductionIncident(t *testing.T) { + inv := inventory.Inventory{Hosts: map[string]inventory.Host{"greencloud-sg-1212": {}}} + runtime := RuntimeConfig{Enabled: true, HostGrantMode: HostGrantSafePrefix} + for _, command := range []string{incidentCommand, incidentHostCommand} { + t.Run(fmt.Sprintf("%d bytes", len(command)), func(t *testing.T) { + store := SessionStore{Dir: t.TempDir()} + matcher, err := Exact(command) + if err != nil { + t.Fatalf("Exact: %v", err) + } + if _, err := store.Grant("s_71d8e139", "greencloud-sg-1212", ScopeSession, matcher, "", "ap_0123456789abcdef01234567", "r1", time.Hour, ChannelCLI); err != nil { + t.Fatal(err) + } + + // The incident: the approved command asked for approval again. + auth, err := Authorize(policy.Config{}, inv, store, runtime, "s_71d8e139", "greencloud-sg-1212", command, "", "req-1") + if err != nil { + t.Fatalf("Authorize: %v", err) + } + if auth.Status != AuthAllowByGrant || auth.GrantScope != ScopeSession { + t.Fatalf("approved command was not authorized by its own grant: status=%s scope=%s", auth.Status, auth.GrantScope) + } + + // The other half, which the incident never surfaced: the grant also + // authorized the command with every '+' removed. + widened := strings.ReplaceAll(command, "+", "") + auth, err = Authorize(policy.Config{}, inv, store, runtime, "s_71d8e139", "greencloud-sg-1212", widened, "", "req-2") + if err != nil { + t.Fatalf("Authorize widened: %v", err) + } + if auth.Status != AuthNeedsApproval { + t.Fatalf("grant authorized a command the operator never approved: status=%s rule=%s", auth.Status, auth.Decision.Rule) + } + }) + } +} diff --git a/internal/approval/matcher.go b/internal/approval/matcher.go index 001ef3f..7f6edf3 100644 --- a/internal/approval/matcher.go +++ b/internal/approval/matcher.go @@ -5,6 +5,7 @@ import ( "encoding/hex" "fmt" "regexp" + "regexp/syntax" "strconv" "strings" ) @@ -49,10 +50,7 @@ type Matcher struct { } func (m Matcher) Match(command string) (bool, error) { - if err := validateMatcherInvariant(m.Regex); err != nil { - return false, err - } - expr, err := regexp.Compile(m.Regex) + expr, err := compileMatcher(m.Regex) if err != nil { return false, err } @@ -72,6 +70,29 @@ func matcherSHA12(m Matcher) string { return sum[:12] } +// looksLegacyEscaped reports whether a stored pattern was generated before '+' +// was escaped, i.e. it carries a bare '+' that acts as a quantifier. Such a +// pattern authorizes commands the operator never approved (an approved "a+b" +// also matches "ab" and "aab"), so callers must stop honoring it. +// +// Order matters: prefix matchers legitimately contain structural '+' in +// `[ \t]+` and in tailTokenClass's trailing quantifier. Those fixed fragments +// are stripped first, so only a generator-emitted bare '+' remains. Exact +// matchers contain none of the fragments, so the strip is a no-op for them. +func looksLegacyEscaped(pattern string) bool { + stripped := pattern + for _, fragment := range []string{ + `(?:[ \t]+` + tailTokenClass + `)*`, + `[ \t]+`, + `[ \t]*`, + `\A`, + `\z`, + } { + stripped = strings.ReplaceAll(stripped, fragment, "") + } + return strings.Contains(stripped, "+") +} + func validateMatcherInvariant(pattern string) error { switch { case !strings.HasPrefix(pattern, `\A`): @@ -95,3 +116,84 @@ func mustValidateMatcher(m Matcher) Matcher { } return m } + +// compileMatcher enforces the string invariant and then compiles. Callers that +// only hold a stored pattern (no SourceCmd) use this; construction goes through +// newMatcher, which additionally proves the pattern against its source. +func compileMatcher(pattern string) (*regexp.Regexp, error) { + if err := validateMatcherInvariant(pattern); err != nil { + return nil, err + } + expr, err := regexp.Compile(pattern) + if err != nil { + return nil, fmt.Errorf("approval matcher does not compile: %w", err) + } + return expr, nil +} + +// newMatcher is the single gate every generated matcher passes through. It +// proves three things a string-level invariant check cannot: +// +// 1. the pattern compiles — a generator bug that emits an invalid quantifier +// is caught here rather than at match time, where the error would surface +// against unrelated commands; +// 2. the pattern matches the command it was generated from — a matcher that +// cannot authorize its own source is silently useless and forces the +// operator to re-approve the same command forever; +// 3. for exact matchers, the pattern matches *nothing but* that command. +// Self-match alone is not enough: a pattern can match its source and still +// match more, which is authorization widening. +// +// Returns an error rather than panicking: this runs on the Authorize hot path +// over an agent-supplied command string, so a panic would be a denial of +// service reachable from untrusted input, and inside ApplyDecision it would +// land between the audit append and the grant write, destroying the operator's +// decision. +func newMatcher(m Matcher) (Matcher, error) { + expr, err := compileMatcher(m.Regex) + if err != nil { + return Matcher{}, err + } + if !expr.MatchString(m.SourceCmd) { + return Matcher{}, fmt.Errorf("approval matcher does not match the command it was generated from: %q", m.Regex) + } + if m.Kind == MatcherExact { + if err := assertExactLiteral(m.Regex, m.SourceCmd); err != nil { + return Matcher{}, err + } + } + return m, nil +} + +// assertExactLiteral proves an exact matcher is anchored around one literal +// equal to the source command, so it can match that command and nothing else. +// The parsed form of \A\z is Concat[BeginText, Literal, EndText] — +// the parser folds adjacent escapes into a single literal run — degenerating +// to Concat[BeginText, EndText] for the empty command. +func assertExactLiteral(pattern string, source string) error { + parsed, err := syntax.Parse(pattern, syntax.Perl) + if err != nil { + return fmt.Errorf("approval matcher does not parse: %w", err) + } + parsed = parsed.Simplify() + widened := fmt.Errorf("exact approval matcher is not a single anchored literal (would match more than the approved command): %q", pattern) + if parsed.Op != syntax.OpConcat || len(parsed.Sub) == 0 { + return widened + } + if parsed.Sub[0].Op != syntax.OpBeginText || parsed.Sub[len(parsed.Sub)-1].Op != syntax.OpEndText { + return widened + } + switch len(parsed.Sub) { + case 2: + if source != "" { + return widened + } + case 3: + if parsed.Sub[1].Op != syntax.OpLiteral || string(parsed.Sub[1].Rune) != source { + return widened + } + default: + return widened + } + return nil +} diff --git a/internal/approval/session_store.go b/internal/approval/session_store.go index bf45619..f61bc0a 100644 --- a/internal/approval/session_store.go +++ b/internal/approval/session_store.go @@ -53,6 +53,57 @@ type sessionFile struct { Grants []Grant `json:"grants"` } +// sessionFileVersion 2 marks a file whose grants have been checked against the +// matcher rules current at the time of writing. Version 1 files may contain +// grants minted before '+' was escaped, whose patterns fail to match the +// approved command while matching commands the operator never saw. +const sessionFileVersion = 2 + +// normalizeGrants repairs grants written by an older AgentSSH. Every session +// grant is minted by applySessionGrant → Exact(req.Cmd), so an exact grant's +// pattern is a pure function of SourceCmd: re-deriving reproduces exactly what +// approving the same command today would produce, preserves the operator's +// intent, and is strictly narrowing (the repaired pattern matches only +// SourceCmd, where the old one matched a superset). A grant that cannot be +// re-derived — no SourceCmd to derive from, or still not self-matching +// afterwards — is dropped rather than trusted. +// +// It returns the number of grants repaired and dropped. +func normalizeGrants(doc *sessionFile) (repaired int, dropped int) { + kept := make([]Grant, 0, len(doc.Grants)) + for _, grant := range doc.Grants { + if grant.Kind != MatcherExact || grant.SourceCmd == "" { + // Prefix grants carry no reliable way back to their source, and a + // grant with no source cannot be proven at all. Keep it only if it + // still matches nothing it should not. + if expr, err := compileMatcher(grant.Regex); err == nil && !looksLegacyEscaped(grant.Regex) { + if grant.SourceCmd == "" || expr.MatchString(grant.SourceCmd) { + kept = append(kept, grant) + continue + } + } + dropped++ + continue + } + matcher, err := Exact(grant.SourceCmd) + if err != nil { + dropped++ + continue + } + if matcher.Regex == grant.Regex { + kept = append(kept, grant) + continue + } + grant.Regex = matcher.Regex + grant.Kind = matcher.Kind + grant.Prefix = append([]string(nil), matcher.Prefix...) + kept = append(kept, grant) + repaired++ + } + doc.Grants = kept + return repaired, dropped +} + func (s SessionStore) Grant(sessionID string, host string, scope Scope, matcher Matcher, stdinSHA256 string, approvalID string, reqID string, ttl time.Duration, channel string) (Grant, error) { if scope != ScopeOnce && scope != ScopeSession { return Grant{}, fmt.Errorf("session store cannot grant scope %q", scope) @@ -76,8 +127,8 @@ func (s SessionStore) Grant(sessionID string, host string, scope Scope, matcher StdinSHA256: stdinSHA256, } err := s.withLockedSession(sessionID, func(doc *sessionFile) error { - if doc.Version == 0 { - doc.Version = 1 + if doc.Version < sessionFileVersion { + doc.Version = sessionFileVersion } if doc.SessionID == "" { doc.SessionID = sessionID @@ -207,7 +258,11 @@ func (s SessionStore) match(sessionID string, host string, command string, stdin matcher := grant.matcher() matches, err := matcher.Match(command) if err != nil { - return err + // A grant whose stored pattern will not compile is poisoned + // data: drop it and keep going. Aborting here would fail every + // command in the session, not just this one. + changed = true + continue } if !matches { remaining = append(remaining, grant) @@ -296,7 +351,18 @@ func (s SessionStore) withLockedSession(sessionID string, fn func(*sessionFile) if err != nil { return err } + // Snapshot before repairing, so a repair counts as a change and is written + // back rather than being redone on every read. before, _ := json.Marshal(doc) + // Repair grants written by an older AgentSSH before anything reads them, so + // a stale pattern can neither authorize an unapproved command nor silently + // fail to authorize an approved one. + if doc.SessionID != "" && doc.Version < sessionFileVersion { + if repaired, dropped := normalizeGrants(&doc); repaired > 0 || dropped > 0 { + fmt.Fprintf(os.Stderr, "agentssh: repaired %d and dropped %d approval grant(s) written by an older AgentSSH\n", repaired, dropped) + } + doc.Version = sessionFileVersion + } if err := fn(&doc); err != nil { return err } From 9949e379c769044ecbea7ea41154b48c05e12286 Mon Sep 17 00:00:00 2001 From: Kritoooo Date: Tue, 4 Aug 2026 11:09:24 +0800 Subject: [PATCH 2/2] fix(approval): drop now-unused mustValidateMatcher newMatcher replaced it as the construction gate, leaving it uncalled. Caught by golangci-lint's unused check. Co-Authored-By: Claude Opus 5 (1M context) --- internal/approval/matcher.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/internal/approval/matcher.go b/internal/approval/matcher.go index 7f6edf3..afc4430 100644 --- a/internal/approval/matcher.go +++ b/internal/approval/matcher.go @@ -110,13 +110,6 @@ func validateMatcherInvariant(pattern string) error { } } -func mustValidateMatcher(m Matcher) Matcher { - if err := validateMatcherInvariant(m.Regex); err != nil { - panic(err) - } - return m -} - // compileMatcher enforces the string invariant and then compiles. Callers that // only hold a stored pattern (no SourceCmd) use this; construction goes through // newMatcher, which additionally proves the pattern against its source.