fix(opencode): match absolute permission patterns outside the worktree - #40149
Open
iceteaSA wants to merge 1 commit into
Open
fix(opencode): match absolute permission patterns outside the worktree#40149iceteaSA wants to merge 1 commit into
iceteaSA wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
What does this PR do?
Five tool call sites send the permission matcher a worktree-relativized path:
A target outside the worktree becomes
../../../../etc/passwd.permission/index.ts:32matches that againstrule.pattern, so a rule written/etc/**or~/...is not a candidate — no leading slash — andfindLastfalls through to whatever broad rule exists, or to theaskdefault.The deny direction, against real
Permission.evaluateon currentdev: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.mdxverbatim, presented as the way to allow reads but block edits in an external directory:The
external_directoryhalf works. Theedithalf returnsask. Same operator, same block, same pattern text.Note the two relativized forms:
../../../../etc/passwdand../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-184—expand()deliberately turns~/...into an absolute pattern, called on every rule byfromConfig. The config layer manufactures absolute patterns the matcher then cannot match.tool/external-directory.ts:30-37— passes an absolute glob to the samectx.ask, which is whyexternal_directoryrules work whileeditrules don't.project/instance-context.ts:18—containsPathalready solves inside-vs-outside, including the non-gitworktree === "/"case, with a comment explaining it.The change adds one helper beside
containsPathand uses it at all five sites: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.tsis 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 likesrc/**now falls through toaskwhere it previously matched. That is the same sentinelcontainsPathalready 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
askverdict parks onDeferred.awaitatpermission/index.ts:101with 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.tsandapply_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 ·readabsolute ·~expansion ·apply_patchmulti-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 testinpackages/opencode: 3235 pass / 0 fail (baseline ondevis 3228, measured on a clean checkout).bun typecheckclean inpackages/opencodeandpackages/core.An independent OpenCode deployment replayed the change against its own permission logs before this was filed: 125,800 evaluated
edit/readlines reduced to 19,411 distinct (pattern, rule, verdict) triples, re-evaluated with the platform's ownWildcard.match. Zero regressions, zero unexpected verdict changes. 181 verdicts flipped deny→allow, every one a/tmpscratch 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.containsusespath.relative, so worktree/home/user/projcorrectly 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 withexternal-directory.tsvia the samecontainsPath), 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 rulearm 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