fix(rules): resolve symlinks before matching files-scoped globs - #53
Open
wazum wants to merge 4 commits into
Open
fix(rules): resolve symlinks before matching files-scoped globs#53wazum wants to merge 4 commits into
wazum wants to merge 4 commits into
Conversation
Node's process.cwd() always returns the physical (symlink-resolved) form of a directory, but an agent's reported path can use whatever string it was given (e.g. a symlinked home-dir alias). The two need a common form before they can be compared. realpath alone can't handle a write's target, since it may not exist yet, so this resolves the longest existing ancestor and re-appends the rest.
…h too Block-level files globs are anchored at the config directory using process.cwd(), which the OS always resolves through symlinks. Action.path is whatever absolute form the agent reported, which isn't resolved. When a project is reached through a symlinked alias (e.g. a firmlink like macOS's /Users/x/Work -> /Volumes/dev), the two strings never compare equal, so every files-scoped rule silently matched nothing and failed open. Matching against both the reported and the canonicalized path closes that gap while still keeping a symlink that points out of the project in scope. Also fixes the Action doc comment, which claimed the engine relativizes paths against the config root at match time. It doesn't — the config loader anchors the globs instead, which is the actual mechanism this commit changes.
Each vendor is exercised with a config whose project root is only reachable by symlink from the agent's reported cwd. Pre-fix, the anchored glob and the write's path never compared equal and the rule silently allowed the write. Post-fix, the rule engages and denies as expected.
path.dirname(filepath) preserves symlinks: with auto-discovery this is harmless because process.cwd() already forces the canonical form, but --config takes whatever path the caller passed, so pointing it through a symlinked alias anchors globs there instead of at the canonical root that Action.path's own canonicalization compares against. Neither the reported nor the canonical form of the action path could then match, reopening the same fail-open this fixes for auto-discovery. Reuses canonicalizePath, which already normalizes to POSIX.
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.
Problem
If the project is reached through a symlink — a macOS firmlink like
/Users/x/Work -> /Volumes/dev, a linked dev environment,--configpointed at an aliased path — everyfiles-scoped rule block silently stops applying. No error, no trace entry, just an empty response (allow).Root cause
Block-level
filesglobs get anchored at the config directory (path.dirname(configFilepath), see ADR-0005). Under auto-discovery this happens to already be symlink-resolved, becauseprocess.cwd()is always canonical on POSIX.Action.path(what a rule matches against) is whatever absolute form the agent reported, which isn't resolved. When the project sits behind an alias, the two strings never compare equal, andactionMatchesFilesScopereturnsfalsefor every write.Confirmed on the real binary, same payload, only the path form differs:
Same file, same rule, same config. Only the alias makes the difference.
Fix
canonicalizePath(new) — symlink-resolved form of an absolute path. AWritecan target a file that doesn't exist yet, so it resolves the longest existing ancestor and re-appends the rest. Normalizes to POSIX either way, sinceAction.path's contract is POSIX-only (ADR-0004) but the native realpath returns OS-native separators.actionMatchesFilesScopenow matches a write's path againstfilesglobs using both the reported path and its canonicalized form. Union, not canonical-only: a symlink pointing out of the project (e.g. a linked package) must stay in scope under its reported name even though canonicalizing it resolves outside every glob.loadConfiganchors at the canonical config directory too. Auto-discovery already got this for free fromprocess.cwd(), but--configtakes whatever path the caller passed — pointed through an alias, it reopens the exact same bug from the other side.A carve-out I tried and reverted
First pass made an exclusion on the reported path authoritative, so an in-tree symlink couldn't resolve an explicitly-excluded path back into scope. That closes one narrow case, but it reopens a much more common one: a workspace symlink like
node_modules/pkg -> srcwould then silently bypass a!**/node_modules/**exclusion, even though the write lands squarely insidesrc/**(this repo's own e2e fixtures use exactly that symlink shape). Reverted — a path is now only out of scope when both the reported and canonical spelling agree it's excluded. Left a comment and a regression test at the call site so nobody re-adds it by accident.Tests
npm run checksclean — 552 tests, lint/format/typecheck all pass.canonicalizePath— symlinked ancestor (existing + not-yet-created file), Windows-shaped realpath output (backslash normalization, injected resolver since I don't have a Windows box), the//-at-root edge case.actionMatchesFilesScope— matches through a symlink, stays in scope when a symlink resolves out of the glob, negation still excludes a subtree reached via symlink, and thenode_modules-style case above.test/integration/scenarios.e2e.test.ts— project root reached through a symlink, all four vendors. Confirmed this fails without the fix (reverted, rebuilt, ran it — denies dropped to allows across the board) and passes with it.config.test.ts—--configthrough a symlink anchors at the realpath, not the alias.Also fixed a stale doc comment on
Action(types.ts) claiming the engine relativizes paths at match time — it doesn't, the config loader anchors the globs instead, which is the actual mechanism this PR touches.