Repository navigation
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
I checked this, and the fix works. I ran the reproduction script from #55728 from source on both branches. On the base, a read deny rule written as ./Workshop/Checklist.md was ignored and the file contents reached the model. With this change, the read is denied and nothing leaks. The permission and file-access tests pass (135), and so does bun typecheck in packages/core.
The public API is unchanged. This only changes how existing read/edit rules match, and rules without ./ work as before.
The code change looks good. Stripping ./ inside Permission.evaluate means it applies to rules from every source, not only config. My one request is about the size of the new tests (inline comment). Heads-up: #53365 changes the same matching code, so whichever lands second may need a rebase.
| expect(Permission.evaluate("external_directory", "C:/Users/x/other/*", rules).effect).toBe("ask") | ||
| }) | ||
|
|
||
| test("treats a leading ./ on read and edit rules as the project-relative path", () => { |
There was a problem hiding this comment.
This table test has 14 checks for a 16-line fix. Could you cut it down to the core cases? A ./ read rule denying the relative path, an edit case, a ./dir/* glob, and the shell rule staying literal would be enough. Cases like ././, .//, ./ → ., .\ and the rule-ordering checks can go. The new file-access.test.ts test mostly checks existing resolve behaviour, and the Service.assert test in this file already covers the fix end to end, so that one could be dropped too.
There was a problem hiding this comment.
Trimmed the table to a ./ read deny, an edit deny, a ./Workshop/* glob, and the shell rule staying literal. Removed the extra ././, .//, .\, and ordering checks, and dropped the file-access resolve test. The Permission.Service.assert case is still the end-to-end check.
Re-ran on Bun 1.4.2 from packages/core: permission, file-access, and file-access-path tests are 134 pass, and bun typecheck passes.
There was a problem hiding this comment.
Thanks, this is what I meant. I checked 50ed15db. The table now has the core cases: a ./ read deny, an edit deny, a ./Workshop/* glob, and the shell rule staying literal. The extra file-access test is gone, and permission.ts is the same as the version I tested earlier. I have nothing else on the tests.
Issue for this PR
Closes #55728
Type of change
What does this PR do?
A read or edit deny rule written as
./Workshop/Checklist.mdnever matched. File tools authorize the location-relative pathWorkshop/Checklist.md, and the rule was compared as literal text, so the file was still returned.readandeditnow treat a leading./as that same relative path. Shell commands and search patterns stay literal.How did you verify your code works?
Bun 1.4.2.
Before:
FileAccess.resolve("./Workshop/Checklist.md")producedWorkshop/Checklist.md. With an allow-all read rule followed by a deny for./Workshop/Checklist.md,Permission.evaluatereturned allow andPermission.Service.assertdid not block.After: the same resolve plus evaluate returns deny, and
assertfails withPermission.BlockedError.Workshop/Other.mdstays allowed. A shell rule starting with./still matches only the literal command.Passed, from
packages/coreon50ed15db4f:bun run test test/permission.test.ts test/file-access.test.ts test/file-access-path.test.ts: 134 passbun typecheck: passPassed on push:
bun run check(oxlint, ast-grep, sdk docs, turbo typecheck 36/36).Passed on this head:
check-compliance,check-standards.check-duplicatesandadd-contributor-labelpassed on the previous head and did not re-run.test,typecheck, andnix-evalare not in the check list and did not fail. Combined status ispendingwith 0 statuses. The TUI drive script on the issue and the fullpackages/coresuite were not run.Screenshots / recordings
Not a UI change.
Checklist