Skip to content

fix(opencode): match absolute permission patterns outside the worktree - #40149

Open
iceteaSA wants to merge 1 commit into
anomalyco:devfrom
iceteaSA:permission-absolute
Open

fix(opencode): match absolute permission patterns outside the worktree#40149
iceteaSA wants to merge 1 commit into
anomalyco:devfrom
iceteaSA:permission-absolute

Conversation

@iceteaSA

@iceteaSA iceteaSA commented Aug 2, 2026

Copy link
Copy Markdown

Issue for this PR

Fixes #30551.

Same root cause as #20045, #22465 and #25097 (all closed unfixed) and the proposal in #22336. Those reported it as an ergonomics problem — a rule that should allow doesn't, so the tool prompts. This PR reports the other direction, which nobody filed: a rule that should deny doesn't either.

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

Five tool call sites send the permission matcher a worktree-relativized path:

tool/write.ts:56          patterns: [path.relative(instance.worktree, filepath)]
tool/edit.ts:104, :147    same
tool/read.ts:257          same
tool/apply_patch.ts:208   relativePaths, built the same way at :205

A target outside the worktree becomes ../../../../etc/passwd. permission/index.ts:32 matches that against rule.pattern, so a rule written /etc/** or ~/... is not a candidate — no leading slash — and findLast falls through to whatever broad rule exists, or to the ask default.

The deny direction, against real Permission.evaluate on current dev:

config { edit: { "*": "allow", "/etc/**": "deny" } }
ruleset after fromConfig+expand:
  [{"permission":"edit","pattern":"*","action":"allow"},
   {"permission":"edit","pattern":"/etc/**","action":"deny"}]

target                                 : /etc/passwd
pattern the tool sends (path.relative) : ../../../../etc/passwd
VERDICT                                : allow      ← matched rule "*"

same target sent as "/etc/passwd":
VERDICT                                : deny

The deny is loaded, expanded, and present in the ruleset. It loses to * because it never competes. Changing only the pattern representation flips the verdict.

The documented example has the same problem. This is docs/permissions.mdx verbatim, presented as the way to allow reads but block edits in an external directory:

config: external_directory allow + edit deny on ~/projects/personal/**

target                             : /home/icetea/projects/personal/notes.md
edit pattern sent by the tool      : ../personal/notes.md
edit VERDICT (documented: deny)    : ask
external_directory VERDICT         : allow

The external_directory half works. The edit half returns ask. Same operator, same block, same pattern text.

Note the two relativized forms: ../../../../etc/passwd and ../personal/notes.md. The ../ count is a function of where the session was started, so no relative form works portably — an operator cannot write a rule that matches an outside path from more than one project root.

The codebase already contains the correct model, in three places:

  • permission/index.ts:178-184expand() deliberately turns ~/... into an absolute pattern, called on every rule by fromConfig. The config layer manufactures absolute patterns the matcher then cannot match.
  • tool/external-directory.ts:30-37 — passes an absolute glob to the same ctx.ask, which is why external_directory rules work while edit rules don't.
  • project/instance-context.ts:18containsPath already solves inside-vs-outside, including the non-git worktree === "/" case, with a comment explaining it.

The change adds one helper beside containsPath and uses it at all five sites:

export function permissionPath(filepath: string, ctx: InstanceContext): string {
  // A root worktree is the non-git sentinel, not a project boundary.
  if (ctx.worktree === "/" || !containsPath(filepath, ctx)) return filepath
  return path.relative(ctx.worktree, filepath)
}

Inside the worktree → relative, exactly as today, so existing src/** rules are untouched. Outside → absolute, so /tmp/** and expanded ~/... rules match as the config layer intends. * still matches both.

evaluate() and last-match-wins are unchanged. external-directory.ts is unchanged. This is deliberately the smallest change that fixes the matching, not a rework of the rule semantics — #22336's broader proposal is a separate discussion.

Guidance this makes statable: relative patterns for in-worktree targets, absolute for outside. Today the second half has no working form at all.

Behaviour change worth a release note

Under a non-git project (worktree === "/"), the helper returns absolute for every target, so a relative rule like src/** now falls through to ask where it previously matched. That is the same sentinel containsPath already special-cases, and the alternative — stripping the leading slash — leaves the bug unfixed for exactly those users. Affected rules can be written **/src/** or given an absolute form.

Worth knowing alongside it: an ask verdict parks on Deferred.await at permission/index.ts:101 with no timeout and no default, resolvable only by a human reply. In an automated or headless lane that is an indefinite park rather than a prompt.

How did you verify your code works?

Seven tool-level arms in write.test.ts, read.test.ts and apply_patch.test.ts, written before the production change: inside-relative still matches · outside absolute allow fires · outside absolute deny fires · outside relative rule still does not match · read absolute · ~ expansion · apply_patch multi-path deny.

Red-first: 83 pass / 5 fail, with the inside-relative and outside-relative arms green in both states. After the fix: 88 / 0.

Mutation-checked both ways. Reverting the production change re-exposes the bug on exactly the five new arms. An over-broad mutation — returning the absolute path unconditionally — turns the inside-relative arm red, so the suite pins the boundary rather than just the fix.

bun test in packages/opencode: 3235 pass / 0 fail (baseline on dev is 3228, measured on a clean checkout). bun typecheck clean in packages/opencode and packages/core.

An independent OpenCode deployment replayed the change against its own permission logs before this was filed: 125,800 evaluated edit/read lines reduced to 19,411 distinct (pattern, rule, verdict) triples, re-evaluated with the platform's own Wildcard.match. Zero regressions, zero unexpected verdict changes. 181 verdicts flipped deny→allow, every one a /tmp scratch path an operator had already written an allow rule for. Both real-world rule forms — expanded ~/... and hand-written absolute — behaved as intended.

A cross-family security review probed the boundary and found no blocking issues. Prefix collision is safe: FSUtil.contains uses path.relative, so worktree /home/user/proj correctly rejects /home/user/proj-evil/x.ts. Two pre-existing gaps it surfaced are not introduced or changed by this PR and are left alone deliberately: a symlink inside the worktree pointing outside is classified inside (shared with external-directory.ts via the same containsPath), and literal .. segments in a target are not normalised before matching. Both deserve their own issues.

One honest weakness: the does not match an outside-worktree relative rule arm passes under both mutations, so it documents intent rather than pinning the boundary. The other three write arms pin it.

Screenshots / recordings

Not a UI change.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

write tool permission */tmp/*: allow still prompts despite matching pattern

1 participant