fix(restrictions): compare a directory restriction by path components - #288
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a995f04436
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Fixes directory-style path restrictions so that a restriction path allows both the restricted directory itself and any descendants, aligning behavior with webpack/enhanced-resolve and unblocking real-world restrictions usage.
Changes:
- Updates restriction checking to use component-aware
Path::starts_with, correctly allowing descendants of a restricted directory. - Normalizes
Restriction::Pathvalues once inResolveOptions::sanitizeto handle..segments and trailing separators consistently. - Adds targeted restriction boundary tests (posix via
MemoryFS, Windows via direct checks) plus example programs for cross-validation against enhanced-resolve.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib.rs |
Fixes restriction containment logic by switching to component-aware prefix checks. |
src/options.rs |
Normalizes path-based restrictions during options sanitization to avoid per-check normalization and improve correctness. |
src/tests/restrictions.rs |
Adds regression tests and boundary suites (posix + Windows) for restriction semantics. |
examples/restrictions.rs |
Adds a Rust example to reproduce/compare restriction behavior. |
examples/restrictions.js |
Adds an enhanced-resolve counterpart script for behavior comparison. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPath restrictions are normalized during option sanitization and checked using prefix matching. Tests verify descendant and sibling resolution, normalization boundaries, empty and relative restrictions, separator handling, trailing slashes, drive-letter casing, UNC paths, and device paths on non-Windows and Windows targets. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
examples/restrictions.rs (1)
13-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider validating
restrictionis absolute too.Only
pathis asserted absolute; a relativerestrictionwon't meaningfully constrain resolution since resolved candidates are absolute andPath::starts_withrequires matching path "kind". Mirroring the existingpathassertion would make the demo less surprising to use.♻️ Suggested addition
assert!(path.is_absolute(), "{path:?} must be an absolute path."); + assert!( + restriction.is_absolute(), + "{restriction:?} must be an absolute path." + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/restrictions.rs` around lines 13 - 17, Update the validation around the existing path assertions to also assert that restriction is absolute, using the same style and a clear message as the path.is_absolute check. Keep the existing directory and path validations unchanged.src/options.rs (1)
401-408: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent UTF‑8 handling for restriction normalization.
Utf8Path::from_path(path)silently no-ops when the restriction path isn't valid UTF‑8, leaving it un-normalized. Elsewhere in the crate, non-UTF‑8 paths are treated as a hard invariant violation via.expect("path should be UTF-8")(e.g. src/lib.rs). For consistency, consider failing the same way here instead of silently skipping normalization.♻️ Suggested consistency fix
- if let Some(path_utf8) = Utf8Path::from_path(path) { - *path = path_utf8.normalize().into_std_path_buf(); - } + let path_utf8 = Utf8Path::from_path(path).expect("path should be UTF-8"); + *path = path_utf8.normalize().into_std_path_buf();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/options.rs` around lines 401 - 408, Update the restriction normalization loop in the options normalization method to require UTF-8 conversion with the crate’s established hard-failure behavior, using the existing “path should be UTF-8” invariant message. Preserve normalization and replacement of valid paths while removing the silent skip for invalid UTF-8 paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@examples/restrictions.rs`:
- Around line 13-17: Update the validation around the existing path assertions
to also assert that restriction is absolute, using the same style and a clear
message as the path.is_absolute check. Keep the existing directory and path
validations unchanged.
In `@src/options.rs`:
- Around line 401-408: Update the restriction normalization loop in the options
normalization method to require UTF-8 conversion with the crate’s established
hard-failure behavior, using the existing “path should be UTF-8” invariant
message. Preserve normalization and replacement of valid paths while removing
the silent skip for invalid UTF-8 paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 98a293de-4201-4dfc-926c-0d53736a5dd8
📒 Files selected for processing (5)
examples/restrictions.jsexamples/restrictions.rssrc/lib.rssrc/options.rssrc/tests/restrictions.rs
A string entry in `restrictions` is a directory restriction, but `is_inside` only accepted a path equal to it, so every file inside a restricted directory was rejected. Replace the hand-rolled prefix check with `Path::starts_with`, which is component-aware, and normalize each restriction once in `sanitize`.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
1ce7356 to
1e09143
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release-npm.yml:
- Around line 106-107: Update the release workflow’s NAPI packaging step to run
`pnpm napi create-npm-dirs` before `pnpm napi version` and `pnpm napi
artifacts`, ensuring the target binding package directories exist before
artifacts are collected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b3bd2a2-c186-450a-bdd7-ea8c827cd82d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
.github/workflows/ci.yml.github/workflows/release-npm.ymlbindings/wasm32-wasi/package.jsonpackage.jsonpnpm-workspace.yamlsrc/lib.rssrc/options.rssrc/tests/restrictions.rs
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
247fede to
bc038f2
Compare
bc038f2 to
1271e74
Compare
`Path::starts_with(Path::new(""))` holds for every path, and normalizing
collapses `.` and `foo/..` to an empty path, so such a restriction let every
candidate through instead of none.
Supersedes rejecting an empty restriction. The cause is narrower than that:
`Utf8Path::normalize` collapses `.`, `..` and `foo/..` to an empty path, and
`Path::starts_with(Path::new(""))` holds for every path, so normalizing turned a
restriction that matched nothing into one that matched everything.
Keep the original whenever normalizing empties it. enhanced-resolve's `normalize`
never empties a relative path, and measured against both 5.23.0 and 5.24.4 it
matches every path for `""` and none for `.`, `..` and `foo/..`.
The candidate checks run while a candidate is being selected, on the spelling it has at that moment, and `load_realpath` runs afterwards. An in-root symlink whose target sits outside the restriction, and an absolute specifier carrying `..`, both passed that check and were then returned from outside the restriction — `restrictions: [<root>/allowed]` handed back `<root>/outside/secret.js`. Check the normalized result once more after `load_realpath`. The candidate checks stay, since they are what lets a rejected candidate fall through to another extension or main field. enhanced-resolve reaches the same place differently: it taps `resolved`, which sits after `SymlinkPlugin` (ResolverFactory.js:817-835), and PR #595 made the symlink-resolved path authoritative for the same reason.
|
Confirmed and fixed in 559a2e7. Reproduced first with the upstream fixture shape (
The normalized result is now checked once more after Five regressions added under |
`canonicalize` answers with a `\\?\` path on windows, where `join` collapses the `..` the traversal tests are about and the resolver cannot take the result as a specifier. Keeping the plain drive path preserves both.
stormslowly
changed the title
Why
is_insidereasoned about paths as strings, and its last step could not succeed:strip_prefixyieldsindex.jsfor/project/src/index.jsunder/project/src, never./, so only a path exactly equal to the restriction passed. A directory restriction matched nothing and was unusable — web-infra-dev/rspack#14952.<fixture>node_modules/pck1/index.jsnode_modules/pck1/index.js<fixture>/node_modules/pckWhat
Compare paths as paths.
Path::starts_withwalks components, so/project/src-otherfalls outside/project/srcon its own — enhanced-resolve needs a separate character probe afterstartsWithonly because it matches raw strings.Two things a component comparison alone still misses:
ResolveOptions::sanitizenormalizes each restriction once, so/a/x/../b/cand a trailing separator name the directory they look like. Normalizing never empties a restriction: an empty path prefixes everything, which would turn.orfoo/..from matching nothing into matching everything.load_realpathruns afterwards, so an in-root symlink pointing outside — or an absolute specifier carrying..— passed and was then returned from outside the restriction:restrictions: [<root>/allowed]handed back<root>/outside/secret.js. The normalized result is now checked once more afterload_realpath.Measured against enhanced-resolve 5.23.0 and 5.24.5 on every case above, plus
""(matches every path) and.,..,foo/..(match none). Tests are ported from webpack/enhanced-resolve@d8693b6 and webpack/enhanced-resolve#595.Known gaps
A candidate rejected by the post-realpath check cannot fall through to another extension or main field:
realpathruns once afterrequire()has committed, whereas enhanced-resolve resolves symlinks per candidate and rejects inside the loop. Follow-up PR.Windows containment stays case-sensitive below the drive letter — Rust compares
Normalcomponents andPrefix::UNCbyte-wise, and only std'sparse_drivefolds case, while upstream folds every component.