Glob [!x] and [^x] now negate (such allow rules widen); placeholders are literal - #573
ronleizrowice-ant wants to merge 15 commits into
Conversation
The five cases under "adjacent aliasing behaviors" in allow-read.test.ts called a local record() helper that console.log()s a line. None of them held an expect, so they spawned sandbox-exec on every macOS leg and could not fail. Two of them cover read-deny bypasses: if a build started letting a hard link or a clone reach a denied file, the log line would change from "not readable" to "READABLE" and CI would stay green. Each case now asserts the direction this build shows: the link and the clone are never created and the secret never appears, the case-folded and firmlink spellings do not reach the file, and the standalone (deny file-link) probe is denied by name rather than by a profile that failed to compile. The two cases that depend on the machine are decided while the block is collected and skipped where they cannot run, rather than returning from the middle of the body.
… reference matcher globToRegex is the one place a glob deny becomes a pattern on all three backends, and its test surface was four cases covering *, **/, ? and a trailing **. Nothing exercised the documented [abc] form or the escape for a bracket that opens no set. Added cases for a digit and a letter range, for the two spellings used for negation elsewhere (both land in the set as members, so such a pattern matches fewer names than its author meant), and for an unclosed [ and a stray ]. Added a property that checks the compiled regex against a hand-written backtracking matcher over generated patterns and paths, so the two implementations check each other on inputs nobody picked. The property found that a path component spelled like one of the internal placeholders (__GLOBSTAR__, __GLOBSTAR_SLASH__) is restored as a wildcard. That case is marked failing rather than fixed here, so it reds the suite if it is ever passed.
… reached The idempotence property over expandWindowsFsPaths says it catches normalize-divergence between the literal and the glob branch, but its corpus was five files named f0.txt..f4.txt under a mkdtemp directory and the generator only ever selects members of it. Neither spelling can hold * or ?, so the branch key was false on every sample and expandGlobPattern was never called across the whole run. The corpus now also holds f*.txt and f?.txt, so a sample can select a pattern alongside literals and the second pass re-normalises concrete results the first pass expanded.
Every .req in the vendored suite lists its headers lowercase and already sorted, and the one header the test adds itself lands in its sorted position too. The signer lowercases and sorts the set it is handed, so neither step was ever exercised: deleting both sort calls leaves all 22 vectors passing. Each vector now signs a second time with the same set reversed and upper-cased, and must produce the same canonical request, string to sign and Authorization. The fixture count is pinned exactly instead of a lower bound, with a note naming the upstream vectors for header order and key case that are not vendored.
Five tests in the "Sandbox Integration" block opened with `if (checkLinuxDependencies().errors.length > 0) return`, and one of them also returned when the apply-seccomp binary did not resolve. A machine without bwrap or socat ran the whole block green, having wrapped nothing, which is the failure the block exists to catch. The suite's own first test wrapped its real assertions in `if (depCheck.errors.length === 0)`, so it passed whether or not the condition its name states held. The dependency check is now a beforeAll that asserts, the first test asserts unconditionally, and the two early returns for a missing binary became assertions on the architectures that have one.
src/index.ts is what package.json points main and types at, and nothing checked what it exports. Three test files import through the barrel, for two of the names; everything else in the test tree is imported from its implementation module, so dropping an export breaks embedders while the suite stays green, and tsc building declarations cannot notice a removal. Added a test that imports the entry point and compares the sorted runtime export names against a checked-in list, so a removal has to edit the list in the same commit. Types are erased before the test can see them; the list says so, since catching a removed type needs a declaration snapshot the package does not build.
The comment above the fixture writes said "ALL dangerous files from DANGEROUS_FILES", but the nine names were written out by hand and asserted by nine hand-written blocks, and the four directories the same way. The file never imported either constant, so a name added to DANGEROUS_FILES or getDangerousDirectories() got no test and a backend that stopped denying one of them was only caught for the names somebody had copied. Both loops now read the constants the two backends read. Each entry keeps the block it had; a new entry brings its own.
…ation-and-placeholders
…stays literal globToRegex parked `**` under the literal strings __GLOBSTAR__ and __GLOBSTAR_SLASH__ while it rewrote `*` and `?`, then restored them by name, so a path component actually spelled that way came back as a wildcard: `/tmp/__GLOBSTAR__/x` compiled to `^/tmp/.*/x$`, and `/tmp/__GLOBSTAR_SLASH__x` no longer matched itself. The pattern is now read once, left to right, and the regex is emitted as it goes, so nothing is parked under a marker and no text in a pattern can be taken for one. What it emits is unchanged for every other pattern, bar two corners of the bracket syntax that the rewriting left as it found them: a `[` that opens no set is escaped wherever it stands rather than only the first one, where two of them used to leave a regex that would not compile, and a set with no members (`[]`) is not a set, so its brackets are characters of the path. The walk over a read-deny glob compiles the pattern piece by piece and fenced off the two shapes it could not read back from what the rewriting made of them: one spelling a placeholder, and one with a second `[` that nothing closes. Neither is a shape of its own any more, so both fences are gone and such a pattern is followed a name at a time like the rest.
The doc comment promised gitignore-style matching, but neither spelling of a negated set was one: `[^0-9]` had its `^` escaped into the set and `[!0-9]` kept the `!` as a member, so both compiled to a positive set holding one character more than its author wrote. A deny written that way denied the opposite of what it said. A `!` or `^` first inside a set now negates it, and a negated set never matches a path separator, which is one character of one name the way the rest of the syntax reads it. `/data/[!a]*` compiles to `^/data/[^/a][^/]*$` where it used to compile to `^/data/[!a][^/]*$`. A `-` first among the members of a negated set is escaped, so that it stays that character rather than reading as a range with the separator the set already excludes.
…er has The glob syntax the README lists did not carry negation, which both spellings of a bracket set now do. On macOS a deny written that way is read by the profile's own regex engine as any character but `/` where the set's members sit next to `/` in the character order, so it covers the characters it lists as well: measured with `sandbox-exec`, and named where the syntax is. The Linux walk reads a pattern with a second unclosed `[` the way it reads the rest, so it is no longer among the patterns matched against whole paths with every directory listed.
antdres
left a comment
There was a problem hiding this comment.
Two non-blocking notes inline. The rewrite itself looks right to me. I fuzzed it against a separately written reference matcher, and the Linux walk agrees with the full-path regex on patterns that spell __GLOBSTAR__ or carry a second unclosed [, so removing the two checks there is safe.
Generated by Claude Code
| - `**` - Matches any characters including `/` (e.g., `src/**/*.ts` matches all `.ts` files in `src/`) | ||
| - `?` - Matches any single character except `/` (e.g., `file?.txt` matches `file1.txt`) | ||
| - `[abc]` - Matches any character in the set (e.g., `file[0-9].txt` matches `file3.txt`) | ||
| - `[!abc]` and `[^abc]` - Matches any character outside the set, and never `/` (e.g., `file[!0-9].txt` matches `filea.txt` but not `file3.txt`) |
There was a problem hiding this comment.
This one probably needs a release note. Any existing rule written [!x] or [^x] now covers the opposite set, allow rules included. An allowRead or allowWrite like /data/[!a]*, which used to match only names starting with ! or a, now matches almost every name in that directory. Someone upgrading would get a wider allow without changing their config.
For context, I compared the old and new compiler on 529 patterns: 44 realistic denies plus every glob string in the repo's tests and docs. Only the negated-set, empty-set and second-unclosed-[ cases changed, so this is the one user-visible change worth calling out.
Generated by Claude Code
| // A `-` first among the members would read as a range with that `/`. | ||
| if (set.negated && globPattern[i] === '-') { | ||
| regex += '\\-' | ||
| i++ | ||
| } |
There was a problem hiding this comment.
Has this \- escape been checked under sandbox-exec? The live macOS run in the description covers [!a], which has no leading -. In JavaScript, [^/\-a] reads as "not /, - or a". If the profile's regex engine treats a backslash inside brackets as a literal character, the way POSIX bracket expressions do, it would read \-a as the range from \ to a. A deny like [!-a]x would then fail to cover _x and would match -x, which is narrower than written.
It's a narrow corner, but a deny with [!-a] or [!-.] under sandbox-exec would settle it. If the escape doesn't hold there, putting the - last among the members would avoid needing it.
Generated by Claude Code
antdres
left a comment
There was a problem hiding this comment.
Approving. The two inline notes (a release note for the [!x] flip, and a sandbox-exec check of the \- escape) don't block this.
Generated by Claude Code
…ation-and-placeholders Conflicts, and how each was resolved: - test/sandbox/glob-expand.test.ts: main carries the same nine tests, from the squash of #570, which this branch carried as commits of its own, and this branch then flipped two of them. Ours whole: the file as it stood before this branch's own change is byte for byte main's, so taking ours keeps one copy of each test, with the placeholder case a passing `it` and the bracket-set case the negation tests. Nothing else conflicted. The rest of #570 is identical on both sides, and main's own changes to src/sandbox/sandbox-utils.ts and README.md sit far from the compiler and the glob syntax this branch edits, so both merged cleanly and were read again afterwards: the merged tree differs from main by this branch's three files alone, hunk for hunk as before the merge.
The string globToRegex returns is compiled twice: by JavaScript, and by the regex engine of a macOS sandbox profile. The two read a `-` inside a set alike in one place only, first among the members. A negated set whose first member is `-` was written `[^/\-a]`. The profile's engine takes a backslash inside a set for a member, so there that is "not `/`, and not `\` to `a`": a deny `[!-a]x` let `_x`, `^x` and `]x` through and refused `-x`. Writing the `-` last is no way out: that engine rejects a set that ends in one character and a `-` (`[a-]`, `[^/a-]`) as an unterminated bracket expression, and the whole profile with it. That part is older than this branch: a positive set was always passed through as written, so a rule holding `[a-]` already gave a profile that does not load. A wildcard-free set holding a `-` of its own is now compiled as a unit: the `-` goes first (`[-a]`, `[^-/a]`), a range that starts or ends at one becomes the `-` and the rest of the range (`[!--0]` is `[^-/.-0]`, `[+--]` is `[-+-,]`), and behind it only a backslash gets a backslash. Every other set compiles to the string it did before. Tests: the meaning of 24 set bodies in three spellings against JavaScript's own reading, exact strings for the shapes measured under sandbox-exec, and a rule checked over every generated set: a member `-` only first, none before the closing `]`, no backslash but a backslash's.
Comments only: the JavaScript emitted with comments removed is byte-identical for every file, and the test names are unchanged. Kept: every invariant and what could be done without it, the documentation of exported names, and the reasons for what is not done the obvious way. Cut: how the code came to be as it is, narration of the lines that follow, and explanations repeated at several sites.
…exec Inside a set only a backslash is escaped now, on every path. A macOS sandbox profile's regex engine takes a backslash inside a set for a member, so `[^/\$]` also left out `\`: a deny of `[!$]` was narrower there than written. JavaScript needs none of those escapes inside a class. A negated set that holds a wildcard keeps a leading `-` first, not as a range from the `/`. New macOS-only test: deny rules compiled from seven sets run under real sandbox-exec and must hide exactly the names JavaScript says they match. New test for the two fences the Linux walk dropped: a placeholder name and a second unclosed `[` are followed through a link. README: an example that holds for a macOS deny as well.
globToRegexcompiles a glob for all three backends. It never negated a bracket set, so[!x]and[^x]matched only!(or^) andx, and a path component spelled__GLOBSTAR__came back as a wildcard.What changes
!or^negates a set; a negated set never matches/.[that opens no set is literal, an empty set ([],[!]) included.\is escaped, and a member-is written first: the one spelling JavaScript and the macOS profile's regex engine read alike.Who sees a difference
[!x]or[^x]used to grant two names and now grants almost every name: review such rules. A deny written that way now covers what it says.[!]and[^]are literal text now: an old deny/x/[!]no longer covers/x/!.[(/d/[[) used to throw and now compile, as literal text.__GLOBSTARor a second unclosed[are now walked through symlinked directories.[a-]loads;sandbox-execrejected the whole profile before.Known limits
/in character order ([!0-9],[!.]) is wider than written, never narrower. More in the README.[*]) is still not read as a set, and[z-a]does not compile, as before.Merge order
One README line conflicts with #575 and #607. The external #581 reads a rooted brackets-only path as literal, which would undo the negated set: land one. #608's rule for a lone
[should match this one's for empty sets.Testing
Compiler swept against a reference matcher (189,751 set bodies x 95 characters). A macOS-only test runs seven compiled sets under real
sandbox-exec: it passes on CI's macOS arm64 and x86-64 legs, so both engines read the new spellings alike.