Skip to content

Add the fail-closed path_within_root guard to ComposerAutoload::resolve (PSR-4 FQCN → file resolution containment) - #225

Merged
mikebronner merged 2 commits into
mainfrom
fix/222-add-the-fail-closed-pathwithinroot-guard-to-compos
Jun 18, 2026
Merged

mikebronner merged 2 commits into
mainfrom
fix/222-add-the-fail-closed-pathwithinroot-guard-to-compos

Conversation

@mikebronner

@mikebronner mikebronner commented Jun 18, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Implements #222 — closes the last unguarded FS-touching resolver in the path_within_root containment lineage (#130 → #143 → #148 → #194 → #199 → #201 → #214 → #218).

ComposerAutoload::resolve mapped a PSR-4 FQCN to a candidate file by splitting the post-prefix remainder on \ and PathBuf::push-ing each segment onto the mapped source_root, then returned the candidate on a bare candidate.exists() — with no containment guard. push/join appends a .. segment literally, and source_root derives from a PSR-4 mapping value in composer.json / vendor/composer/installed.json, so a ..-bearing FQCN (or a mapping / under-root symlink pointing outside the tree) yielded a candidate that escaped the project root and was then stat'd and returned — an out-of-root read primitive. This is the higher-priority branch in class_locator.rs (it runs before the heuristic find_php_class_file_by_fqcn that #218/PR #221 guarded).

Changes

  • composer_autoload.rs — gate every candidate in resolve with the fail-closed path_within_root(&candidate, &self.project_root) guard before the on-disk exists() check; a candidate that canonicalizes outside the root (or can't be proven in-root) is skipped to the next source_root.
  • project_root stored on the struct at construction (in load, which for_project feeds) — see the design note below.
  • tests/composer_autoload_containment.rs (new) — follows the fqcn_resolution_containment.rs convention; wired into tests/mod.rs.

Design note — project root threading (AC bullets 2 & 3)

AC bullet 2 offered two options: a new &Path parameter on resolve, or storing the root on ComposerAutoload at construction. I chose struct storage because:

  1. Can't be misused — ComposerAutoload is a per-project, process-cached (&'static) struct whose PSR-4 source roots are already computed relative to the project root. Binding the guard to the stored root means resolve always checks against exactly the root its data was loaded for; a caller can't pass a mismatched root (which the parameter approach permits).
  2. Smaller, safer diff — zero call-site churn and zero churn to the existing resolve unit tests.

Consequently AC bullet 3 (update call sites) is satisfied by construction, not by signature change: the root flows into resolve via load/for_project, both of which already receive project_root. I verified the only ComposerAutoload::resolve callers are find_php_class_file (class_locator.rs:42) and find_php_class_file_in_app_or_vendor (:79) — the main.rs/command_disk_cache .resolve(...) calls are a different command-index method.

Acceptance Criteria

  • In resolve, gate each candidate with the fail-closed path_within_root(&candidate, &project_root) guard before exists(), skipping to the next source_root on false
  • Project root threaded into resolve — stored on the ComposerAutoload struct at construction (the struct option of bullet 2)
  • Call sites supply the project root — satisfied by construction (root flows via load/for_project); confirmed no other ComposerAutoload::resolve callers exist
  • New tests/composer_autoload_containment.rs with:
    • Negative: ..-escaping PSR-4 FQCN → None, with a precondition asserting the file exists outside root
    • Positive control: normal in-root PSR-4 FQCN → Some(path)
    • #[cfg(unix)] negative: under-root symlink in the PSR-4 source path resolving outside root → None, precondition confirms the candidate canonicalizes outside root
  • tests/mod.rs updated with mod composer_autoload_containment;
  • No regression in existing composer_autoload/tests.rs unit tests or class_locator_and_properties.rs integration tests

Test Plan

  • cargo fmt clean, cargo check clean, cargo clippy --all-targets clean
  • New tests pass (3/3, incl. #[cfg(unix)] symlink case)
  • composer_autoload unit tests (10) + class_locator tests pass unchanged
  • Full lib + src/tests suites green (1894 + 426 passing, 0 failed)
  • [ℹ] 8 pre-existing failures in the separate integration_tests.rs binary (env-file / route / test-project fixture tests) reproduce on clean main and are unrelated to this change

Fixes #222

…root guard

`ComposerAutoload::resolve` mapped a PSR-4 FQCN to a candidate file by
splitting the post-prefix remainder on `\` and `PathBuf::push`-ing each
segment onto the mapped `source_root`, then returned the candidate on a
bare `candidate.exists()` — with no containment guard. `push`/`join`
appends a `..` segment literally, and `source_root` derives from a PSR-4
mapping value in composer.json / installed.json, so a `..`-bearing FQCN
(or a mapping / under-root symlink pointing outside the tree) yielded a
candidate that escaped the project root and was then stat'd and returned:
an out-of-root read primitive.

`resolve` is the higher-priority branch in `class_locator.rs` (it runs
before the heuristic `find_php_class_file_by_fqcn` that #218/PR #221
guarded), yet was the one FS-touching resolver in the lineage
(#130 → #143 → #148 → #194 → #199 → #201 → #214 → #218) with no guard.

Gate every candidate with the fail-closed `path_within_root` guard before
the on-disk check. The project root is stored on `ComposerAutoload` at
construction (`load`/`for_project` both already receive it) rather than
threaded per-call, so resolution is bound to exactly the root the PSR-4
mappings were resolved against and no caller can pass a mismatched root —
keeping the two `class_locator.rs` call sites and the existing unit tests
unchanged.

Add `tests/composer_autoload_containment.rs`: a `..`-escaping FQCN → None
(with an out-of-root precondition so None can only be the guard), an
in-root positive control → Some, and a `#[cfg(unix)]` under-root-symlink
escape → None.

Fixes #222
@mikebronner
mikebronner marked this pull request as ready for review June 18, 2026 16:50

@mr-sherlock-holmes mr-sherlock-holmes Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approved

Review Summary

  • Adds the fail-closed path_within_root guard to ComposerAutoload::resolve, closing the PSR-4 FQCN → file out-of-root read primitive (issue #222, containment lineage #130 → … → #218 → #222).
  • All six acceptance criteria met. AC #2/#3 are met by a deliberate divergence from the literal wording, and it's a strict improvement — see below.
  • CI green (LSP test/fmt/clippy, Analyze, wasm/clippy). Guard verified to sit at composer_autoload.rs:103, before candidate.exists() at line 106, with continue to the next source_root — so no out-of-root path is ever stat'd-and-returned.

Acceptance criteria

  • AC #1 — guard in the resolve loop: ✅ if !path_within_root(&candidate, &self.project_root) { continue; } at composer_autoload.rs:103, after candidate construction (l.93) and before candidate.exists() (l.106).
  • AC #2 — root threaded into resolve: ✅ by divergence — the PR took the AC's blessed alternative (store on the struct, project_root: PathBuf at l.48, populated in the sole load constructor at l.211) rather than a per-call &Path parameter. for_project inherits via load. The doc comment argues this is safer ("resolution is bound to exactly the root the PSR-4 mappings were resolved against — a caller can't hand resolve a mismatched root"). Nothing the AC cared about is dropped; the guard runs against the correct root on every path.
  • AC #3 — update call sites: ✅ by divergence — because the root lives on the struct, resolve's signature is unchanged, so the two callers in class_locator.rs (find_php_class_file, find_php_class_file_in_app_or_vendor) need no edit. grep confirms those are the only resolve call sites; both already pass root to for_project, which is exactly where the binding now lives. The AC's intent — every resolution path guarded against the right root — is fully satisfied.
  • AC #4 — three containment tests: ✅ All present in tests/composer_autoload_containment.rs and discriminating: each negative writes the escaping target to disk and asserts via precondition that the candidate canonicalizes to it, so a None can only come from the guard (would return Some(out-of-root) if the guard were removed). Negative ..-FQCN (l.49), positive control (l.90), #[cfg(unix)] under-root-symlink (l.119, with the extra !starts_with(root) precondition).
  • AC #5 — tests/mod.rs: ✅ mod composer_autoload_containment; added alphabetically (l.13).
  • AC #6 — no regression: ✅ Only the new guard changes behaviour; existing tests resolve in-root files that pass the guard. CI confirms.

What's Good

  • The struct-stored-root choice is the right call and well-justified in the doc comment — it makes a mismatched-root bug structurally impossible rather than relying on every caller to pass the right root.
  • Tests are genuinely honest: the precondition .unwrap() on the canonicalized candidate is self-enforcing — if the fixture were misbuilt the test panics rather than passing for the wrong reason.
  • project_root stored uncanonicalized is correct and documented — path_within_root canonicalizes both sides, so the macOS /var→/private/var case is handled.

📋 Non-blocking follow-ups

  • resolve_namespace_dirs is the next unguarded surface in this lineage — composer_autoload.rs:123-154. It builds dir = source_root.join(&rel) and returns it on a bare dir.is_dir() (l.146-147) with no path_within_root guard and no access to self.project_root. Two downstream legs then touch the result out-of-root: salsa_impl.rs:2355 (dir.join(class).with_extension("php"); file.exists()) and main.rs:12576 → scan_dir (WalkDir::new(dir).follow_links(true), no containment on discovered paths). (A third leg, resolve_component_existing_file in main.rs, does guard post-stat.) Same defense-in-depth class as this issue, low practical risk under the LSP threat model — filed as its own anchor per the lineage's one-surface-per-issue pattern (issue opened by me, linked below). A test for an out-of-root source_root mapping value (vs. .. in the FQCN) belongs there too — that vector is already covered by this PR's guard at runtime but is not separately pinned.

Ready for @mikebronner to merge.

@mikebronner
mikebronner merged commit 7b87d29 into main Jun 18, 2026
5 checks passed
@mikebronner
mikebronner deleted the fix/222-add-the-fail-closed-pathwithinroot-guard-to-compos branch June 18, 2026 17:16
mikebronner added a commit that referenced this pull request Sep 8, 2026
…#382)

Version 1.13.0 failed to compile for any no_std consumer. Commit 9f25466
added a bounded range to hold the transitive resolution at 1.12.0, as an
explicitly temporary measure.

Lokathor/tinyvec#226 merged on 2026-09-04 and closed issue #225. Version
1.13.1 published two minutes later, and 1.13.2 followed. The call in
src/tinyvec.rs is now qualified as alloc::vec![...].

The constraint is removed outright, not relaxed to a floor. A floor would
keep a permanent phantom direct dependency on a crate this project never
uses. It buys nothing over cargo's max-version selection, since 1.13.0
is unreachable once the upper bound is gone.

Verified from a regenerated lockfile rather than from the manifest edit:
a fresh resolution selects tinyvec 1.13.2, reached only through
sqlx -> sqlx-postgres -> stringprep -> unicode-normalization.

Fixes: #377
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add the fail-closed path_within_root guard to ComposerAutoload::resolve (PSR-4 FQCN → file resolution containment)

1 participant