diff --git a/CHANGELOG.md b/CHANGELOG.md index 05ee9da01..bd95570b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,17 @@ variables and such a key would otherwise silently shadow the built-in helper; manifests that previously used either name as a variable now fail to parse ([#79](https://github.com/leynos/netsuke/issues/79)) +- Open the capability used for glob metadata checks at the pattern's longest + literal directory prefix rather than at the filesystem root or the working + directory, so glob expansion holds only the authority its pattern can reach; + a pattern whose literal prefix is missing or names something other than a + directory now expands to no matches, and a match reached through a symbolic + link — the match itself or an intermediate directory — that resolves + outside that prefix or dangles is skipped rather than failing the + expansion, though a cyclic symbolic link still fails the expansion; opening + a symbolic-link prefix now fails, and glob tracing redacts caller-controlled + path fields as `` + ([#173](https://github.com/leynos/netsuke/issues/173)) ### Removed @@ -69,6 +80,13 @@ `BuildTargets::as_slice().is_empty()` ([#75](https://github.com/leynos/netsuke/issues/75)) +### Fixed + +- Expand parent-relative glob patterns such as `glob('../shared/*.h')`; their + matches previously reached the working-directory capability as `../…` and + were rejected as sandbox escapes + ([#173](https://github.com/leynos/netsuke/issues/173)) + ## [0.1.0] - 2026-07-28 _Initial release._ diff --git a/Cargo.lock b/Cargo.lock index 0d0710807..a4e2378d0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1472,6 +1472,7 @@ dependencies = [ "anyhow", "assert_cmd", "camino", + "cap-primitives", "cap-std", "clap", "clap_mangen", diff --git a/Cargo.toml b/Cargo.toml index b000e1abd..2ba91d200 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -97,6 +97,7 @@ clap = { version = "4.5.0", features = ["derive"] } serde = { version = "1", features = ["derive"] } serde-saphyr = "0.0.6" minijinja = { version = "2.12.0", features = ["loader"] } +cap-primitives = "3.4.4" cap-std = { version = "3.4.4", features = ["fs_utf8"] } camino = "1.2.0" semver = { version = "1", features = ["serde"] } diff --git a/docs/adr-010-scope-glob-capability-to-literal-prefix.md b/docs/adr-010-scope-glob-capability-to-literal-prefix.md new file mode 100644 index 000000000..93b8d5fef --- /dev/null +++ b/docs/adr-010-scope-glob-capability-to-literal-prefix.md @@ -0,0 +1,155 @@ +# Architecture decision record (ADR): Scope the glob metadata capability to the literal prefix + +## Status + +Accepted. + +## Date + +2026-08-09 + +## Context and problem statement + +Manifest glob expansion (`src/manifest/glob/mod.rs`) matches a caller-supplied +pattern such as `src/**/*.c` and returns the matching file paths. Matching +itself runs through the `glob` crate, which walks the filesystem directly. +Filtering the walk's results down to regular files, however, goes through a +metadata check, and that check was routed through a `cap_std::fs::Dir` +capability handle rather than a raw filesystem call. + +The handle's authority was disproportionate to what any single pattern could +need. `glob_paths` opened it at `/` for an absolute pattern and at `.` for a +relative one, so the capability covered the entire filesystem root or the +entire working directory regardless of how narrow the pattern's own literal +component was. A pattern such as `src/**/*.c` names only the `src/` subtree, +yet the metadata check could resolve any path reachable from the root or the +working directory. Issue #173 asked for this ambient authority to be +reviewed against the least-privilege principle the capability was meant to +enforce. + +## Decision + +Open the metadata capability at the pattern's longest literal directory +prefix instead of at a fixed root: + +- **Literal prefix extraction.** `walk::literal_dir_prefix` scans the + normalized pattern up to the first glob metacharacter (`*`, `?`, `[`, or + `{`) and trims the result back to the last path separator, yielding the + deepest directory the pattern names without wildcards. For `src/**/*.c` + this is `src/`. A pattern with no literal directory component, such as + `*.c`, yields `.`, keeping the working-directory scope for patterns that + cannot narrow it. The scan steps over bracketed literal escapes such as + `[*]`, so `src/[*]x/generated/*.c` reaches `src/[*]x/generated/` rather + than stopping at the first `[`. +- **`GlobRoot` couples the handle and the prefix.** `walk::open_root_dir` + opens a `cap_std::fs::Dir` at the literal prefix one component at a time, + refusing symbolic links, and wraps it together with the lexical prefix in a + `GlobRoot`. Every subsequent metadata lookup relativizes the matched path + against the prefix (`GlobRoot::relativise`) before resolving it through the + handle, so the capability only ever sees paths inside the subtree it was + opened at. +- **A missing or non-directory prefix yields no capability at all.** + `open_root_dir` returns `Ok(None)` when the prefix does not exist or is not + a directory (`walk::prefix_is_unopenable`), and `glob_paths` returns an + empty match set in that case, mirroring the empty result the matcher would + produce anyway. `diagnostics::record_unopenable_prefix` records the + outcome so a degraded expansion remains observable. +- **The matcher still walks ambiently.** The `glob` crate's own traversal is + unchanged; only the metadata check used to filter directories out of its + results is capability-scoped. Narrowing the capability's opening point + therefore narrows what the metadata check can resolve, not what the walk + itself can see on disk. + +## Rationale + +- **Least privilege follows the pattern's own scope.** A pattern can only + ever match inside its literal prefix, so opening the capability there gives + the metadata check exactly the authority the pattern could use, rather + than the authority of the whole filesystem root or working directory. +- **Relativization keeps the capability boundary honest.** Resolving a + matched path against the capability requires rebasing it onto the prefix + first (`GlobRoot::relativise`); a path that does not start with the prefix + cannot be looked up at all, so the capability cannot be handed an + absolute or differently rooted path by accident. +- **An unopenable prefix is an empty result, not an error.** A pattern whose + literal prefix does not exist can match nothing, so treating a missing or + non-directory prefix as an empty expansion (rather than a hard failure) + matches the semantics a caller already expects from a glob that matches no + files. +- **A symbolic-link prefix is rejected.** Opening each literal directory + component without following links prevents a pattern such as `src/link/*.c` + from gaining the authority of `link`'s target, while retaining the lexical + prefix in `GlobRoot` for later match relativization. +- **Symbolic-link handling is explicit and bounded.** + `GlobRoot::metadata_relative` classifies a failed + metadata lookup as a skipped link only when `is_unresolvable_link` identifies + `PermissionDenied` or `NotFound` and `traverses_symlink` confirms a final or + intermediate symbolic link. Other failures, including symlink loops, still + propagate. This classification is paired with bounded skipped-match + recording via `diagnostics::record_unreachable_symlink`. + +## Consequences + +- **The matcher's own traversal remains ambient.** Scoping the metadata + capability does not scope the `glob` crate's directory walk; that crate + still reads the filesystem directly rather than through `cap_std`. + Replacing it with a capability-native matcher would close this remaining + gap but was out of scope for this change (see Alternatives considered). +- **A match reached through an escaping symbolic link is silently skipped.** + If a matched path resolves through a symlink whose target leaves the + literal prefix, the capability cannot follow it. `GlobRoot::metadata` + treats this the same as a dangling link: the match is dropped from the + results rather than failing the whole expansion, and the drop is recorded + via `diagnostics::record_unreachable_symlink`. A symlink loop is treated + differently and still fails the expansion, because a cycle describes a + broken tree rather than a file that is simply unreachable through the + capability. +- **An escaping symlink and a permission-denied symlink are + indistinguishable.** `cap_std` reports both an out-of-prefix resolution + and a genuine permission failure inside the prefix as + `io::ErrorKind::PermissionDenied`. The capability cannot tell these apart, + so a link that is legitimately unreadable inside the prefix is skipped + alongside one that escapes it, rather than being reported as a distinct + error. +- **Parent-relative patterns now work.** A pattern such as `../*.txt` + previously reached the working-directory handle as a `../…` lookup and was + rejected as a sandbox escape. Because the capability is now opened at the + pattern's own literal prefix (which can itself be `../`), a parent-relative + pattern gets a capability rooted at that parent directory instead of being + rejected outright. +- **Diagnostics stay bounded.** `diagnostics.rs` records the unopenable-prefix + and unreachable-symlink outcomes as low-cardinality counters + (`netsuke_manifest_glob_expansions_total`, + `netsuke_manifest_glob_entries_skipped_total`) labelled only by a closed + set of outcome and reason strings. Tracing replaces every caller-controlled + path field — patterns, prefixes, and sampled relative matches — with the + stable `` marker. Errors may retain the original caller input so + invalid patterns can be explained precisely. + +## Alternatives considered + +- **Keep the ambient root.** Rejected. Opening the handle at `/` or `.` + regardless of the pattern's own scope gives the metadata check no + least-privilege benefit at all; the capability wraps the check in `cap_std` + API surface without constraining what it can resolve. +- **Always open at the working directory.** Rejected. This breaks absolute + patterns, which need a handle rooted above the working directory to + resolve at all, and it does not help parent-relative patterns such as + `../*.txt`, which would still need to escape the working-directory handle + to resolve. +- **Replace the `glob` crate with a capability-native matcher.** Rejected for + this change. This would close the remaining gap where the matcher's own + traversal reads the filesystem ambiently, but it is a materially larger + change than scoping the existing metadata check, and is named here as the + way to close that boundary rather than attempted as part of this decision. + +## Implementation references + +- Capability scoping and symbolic-link handling: + [`src/manifest/glob/walk.rs`](../src/manifest/glob/walk.rs) +- Expansion entry point and capability composition: + [`src/manifest/glob/mod.rs`](../src/manifest/glob/mod.rs) +- Bounded diagnostics for unopenable prefixes and skipped matches: + [`src/manifest/glob/diagnostics.rs`](../src/manifest/glob/diagnostics.rs) +- Developer guide: + [`docs/developers-guide.md`](developers-guide.md#manifest-glob-module-boundary) diff --git a/docs/contents.md b/docs/contents.md index 40978a005..f3315a18f 100644 --- a/docs/contents.md +++ b/docs/contents.md @@ -52,14 +52,17 @@ operator, user, and contributor references are easier to find. - [adr-009-bounded-redacted-manifest-telemetry.md](adr-009-bounded-redacted-manifest-telemetry.md): Manifest telemetry decision record, separating observability from evaluation and bounding and redacting the emitted metrics and spans. +- [adr-010-scope-glob-capability-to-literal-prefix.md](adr-010-scope-glob-capability-to-literal-prefix.md): + Glob capability-scoping decision record, opening the metadata capability at + a pattern's literal directory prefix instead of an ambient root. ## User and operator guides - [quickstart.md](quickstart.md): First-run walkthrough for building with Netsuke. - [v0-1-0-migration-guide.md](v0-1-0-migration-guide.md): Migration notes for - the v0.1.0 child-environment API additions, and the stability caveat that - covers them. + the v0.1.0 child-environment API additions and glob behaviour, and the + stability caveat that covers them. - [users-guide.md](users-guide.md): End-user reference for authoring and running Netsuke manifests, including executable discovery and `command_available` branch selection. diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 2bd610e15..3b4f21a24 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1601,6 +1601,87 @@ sibling submodule needs them; widen the boundary only by adding a deliberate re-export in `src/manifest/mod.rs`. The comments in the source are supporting detail for these rules, not a substitute for them. +### Capability scope + +The metadata check that filters directories out of a glob's results goes +through a `cap_std::fs::Dir` handle rather than a raw filesystem call. +`walk::open_root_dir` opens that handle at the pattern's longest literal +directory prefix, computed by `walk::literal_dir_prefix`: the pattern text up +to the first `*`, `?`, `[`, or `{`, trimmed back to the last path separator. +For `src/**/*.c` that prefix is `src/`. + +`walk::open_literal_prefix` owns the opening policy: it opens the lexical root +or current directory ambiently once, then opens each normal literal component +without following symbolic links. It is used only to establish `GlobRoot`; +metadata lookups remain the responsibility of that root. + +- **Bracketed literal escapes do not stop the scan.** The `[*]`, `[?]`, + `[[]`, `[]]`, `[{]`, and `[}]` forms that `normalize::force_literal_escapes` + produces from `\*`, `\?`, and the like name a literal character rather than + a wildcard, so `src/[*]x/generated/*.c` reaches `src/[*]x/generated/`, not + `src/`. A genuine character class such as `[ab]` is still a wildcard and + still stops the scan. The resulting prefix is still pattern text, so + `walk::unescape_literal_escapes` resolves it to the path it names + (`src/[*]x/` becomes the directory `src/*x/`) before the capability is + opened and before any match is stripped of it. +- **`GlobRoot` couples the handle with the prefix.** Matches keep the + pattern's own rooting as they arrive from the `glob` crate's walker — an + absolute pattern yields absolute matches, while a parent-relative pattern + such as `../*.txt` yields matches like `../out.txt` — so + `GlobRoot::relativise` rebases each one onto the prefix before the + metadata lookup. A path that does not start with the prefix is rejected + outright rather than resolved through a wider capability. +- **No literal directory component falls back to the working directory.** A + pattern such as `*.c` yields a prefix of `.`. `walk::prefix_is_unopenable` + treats a missing prefix, and a prefix that names something other than a + directory, as no capability at all; `glob_paths` then returns an empty + match set rather than an error. Any other failure to open the prefix + propagates. +- **`walk::is_unresolvable_link` governs which failed lookups are skipped + rather than fatal.** Only `io::ErrorKind::PermissionDenied` (an escape from + the capability's tree, or a genuine permission failure the capability + cannot distinguish from one) and `io::ErrorKind::NotFound` (a dangling + link) count, and only when some component of the matched path is actually + a symbolic link. A `FilesystemLoop` is a broken tree rather than an absent + file, so it propagates instead of being skipped. +- **The boundary that remains.** The match walk itself is the `glob` crate's, + and that crate traverses the filesystem ambiently. Only the metadata check + is capability-scoped, so narrowing the capability's opening point narrows + what the metadata check can resolve, not what the walk itself can see on + disk. + +[ADR-010](adr-010-scope-glob-capability-to-literal-prefix.md) records the +decision to scope the capability this way and the alternatives it rejected. + +### Glob expansion observability + +`src/manifest/glob::expand_glob` returns bounded observations for two outcomes +of the capability-scoped walk that are expected rather than erroneous, so +neither reaches the top-level diagnostics: a literal prefix that names no +directory, and matches dropped because a symbolic link cannot be resolved +through the capability, including an unreadable link within the prefix. It +aggregates every skipped entry while retaining at most the first four +unreachable-symlink paths as a trace sample. The +`src/manifest/mod.rs` adapter records those observations after the query at the +Jinja `glob` helper's orchestration boundary, via `glob::record_expansion`. +Keeping recording there leaves the expansion query free of metrics and tracing +side effects while keeping a degraded expansion visible without having to +reproduce it. + +- **Metrics** — `netsuke_manifest_glob_expansions_total`, labelled + `outcome` (`matched`, `unopenable_prefix`), and + `netsuke_manifest_glob_entries_skipped_total`, labelled `reason` + (`unreachable_symlink`, `not_a_file`). The skipped-entry counter includes + every skipped entry, not only the sampled paths. Labels carry only these + closed sets, never the pattern or a path, in line with the low-cardinality + rule in `AGENTS.md`. +- **Tracing** — every caller-controlled path field is replaced with the stable + `` marker: patterns, prefixes, and sampled relative matches. A + skipped unreachable-symlink event is emitted only for the retained sample, + with no more than four such events per expansion. Metrics retain only + bounded aggregate status and reason data; errors may retain the caller's + original pattern so invalid input can be explained precisely. + ## Test isolation utilities Environment variable mutations and working-directory changes are process-global diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index c13fd8a03..08161b06e 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -1062,9 +1062,10 @@ providing a secure bridge to the underlying system. - `glob(pattern: &str) -> Result, Error>`: Expand filesystem patterns (e.g., `src/**/*.c`) into a list of matched paths. Results are - yielded in lexicographic order by the iterator and returned unchanged. - Symlinks are followed by the `glob` crate by default. Matching is case- - sensitive on all platforms. `glob_with` enforces + yielded in lexicographic order by the iterator and returned unchanged. A + match reached through a symbolic link that escapes the pattern's literal + directory prefix, or that dangles, is skipped; a symlink loop fails the + expansion. Matching is case-sensitive on all platforms. `glob_with` enforces `require_literal_separator = true` internally, so wildcards do not cross path separators unless `**` is used. Callers may use `/` or `\\` in patterns; these are normalized to the host platform before matching. Results contain @@ -1079,6 +1080,16 @@ providing a secure bridge to the underlying system. `config\*.yml` maps to `config/*.yml`. On Windows, backslash escapes are not supported. This provides globbing support not available in Ninja itself, which does not support globbing.[^3] + + The metadata check that filters directories out of the results runs + through a capability opened at the pattern's literal directory prefix + (`src/` for `src/**/*.c`) rather than at an ambient root; the match walk + itself remains the `glob` crate's own, which traverses the filesystem + ambiently. + [ADR-010](adr-010-scope-glob-capability-to-literal-prefix.md) records this + decision; see the + [developer's guide](developers-guide.md#capability-scope) for the prefix + computation and symlink-handling rules. - `python_version(requirement: &str) -> Result`: An example of a domain-specific helper function that demonstrates the extensibility of this architecture. This function would execute `python --version` or diff --git a/docs/users-guide.md b/docs/users-guide.md index e21cb04cb..fe10f83ad 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -384,6 +384,20 @@ Matching is case-sensitive. `*` and `?` do not cross directory separators; use are returned. The [quick-start guide](quickstart.md) shows a complete runnable example. +Patterns may be absolute or relative to the working directory, including +parent-relative patterns such as `glob('../shared/*.h')`. Expansion is scoped +to the pattern's longest literal directory prefix — the text up to the first +`*`, `?`, `[` or `{`, trimmed back to the last separator, so `src/` for +`src/**/*.c`. If that prefix does not exist, or names something that is not a +directory, the call returns an empty list rather than failing. A symbolic-link +literal prefix, such as `src/link/*.c`, cannot establish the capability and +causes expansion to fail. A match is skipped rather than reported as an error +when the metadata lookup cannot resolve a symbolic link — the match itself or +a directory reached on the way to it — because it is unreadable within the +prefix, dangling, or resolves outside that prefix. A cyclic symbolic link is +reported as an error rather than skipped, since it describes a broken tree +rather than a missing file. + ### Define reusable macros Macros return rendered text and can accept default arguments: @@ -1035,6 +1049,14 @@ Netsuke reduces some common quoting mistakes, but it is not a sandbox: - `script` uses `/bin/sh -e` in v0.1.0-beta1. - `shell`, `grep`, `fetch`, filesystem helpers, and ordinary recipes interact with the host. +- `glob` restricts its filesystem metadata access to a capability handle + scoped to the pattern's literal directory prefix, so it cannot inspect + anything outside the subtree the pattern can match; the pattern match walk + itself still uses ambient filesystem access. +- Verbose glob tracing replaces every caller-controlled path field — patterns, + prefixes, and sampled relative matches — with the stable `` marker. + Aggregate metrics retain only bounded status and reason data. Error messages + may retain the original input so invalid patterns can be explained. - `raw` template output and handwritten shell fragments remain the manifest author's responsibility. - Literal shell dollar expressions currently require Ninja-aware escaping, diff --git a/docs/v0-1-0-migration-guide.md b/docs/v0-1-0-migration-guide.md index cb4d9493e..bc8865330 100644 --- a/docs/v0-1-0-migration-guide.md +++ b/docs/v0-1-0-migration-guide.md @@ -23,6 +23,7 @@ Table: v0.1.0 child-environment API additions and their impact | Convenience wrappers | Unchanged. `run_ninja` and `run_ninja_tool` behave exactly as before, inheriting the process environment. | [Users' guide](users-guide.md) | | Child environment | New opt-in `netsuke::runner::CommandEnv` carries additive variable overrides and an injected `PATH` for Ninja child processes. | [Users' guide](users-guide.md) | | Request types | New `netsuke::runner::NinjaBuildRequest` and `netsuke::runner::NinjaToolRequest` name the program, build file, and targets or tool for the `*_with` run functions. | [Users' guide](users-guide.md) | +| Glob expansion | Parent-relative patterns such as `glob('../shared/*.h')` now expand. Metadata checks use a capability rooted at the pattern's longest literal directory prefix; missing or non-directory prefixes return no matches, and unresolvable symlink matches are skipped. | [Users' guide](users-guide.md) and [ADR-010](adr-010-scope-glob-capability-to-literal-prefix.md) | ## Nothing to change for existing callers diff --git a/src/manifest/glob/diagnostics.rs b/src/manifest/glob/diagnostics.rs new file mode 100644 index 000000000..ad96edce9 --- /dev/null +++ b/src/manifest/glob/diagnostics.rs @@ -0,0 +1,106 @@ +//! Bounded observability for glob expansion. +//! +//! Two outcomes of the capability-scoped walk are expected rather than +//! erroneous, so neither reaches the top-level diagnostics: a literal prefix +//! that names no directory, and a match that the capability cannot resolve +//! because a symbolic link escapes the prefix. Both are recorded here so a +//! degraded expansion is visible without having to reproduce it. +//! +//! What is recorded is deliberately bounded and redacted. +//! +//! Metric labels carry only a closed set of outcome and reason strings, never +//! the pattern or a path, in line with the low-cardinality rule in `AGENTS.md`. +//! +//! Tracing events replace every caller-controlled path with the stable +//! `` marker. Errors still retain the caller's pattern so they can +//! explain invalid input precisely; tracing does not need that detail to +//! identify the expansion outcome. +//! +//! What tracing does not carry is a matched path. A skipped entry is recorded +//! with the same redaction, so its relative form cannot disclose a filename +//! selected by the pattern. + +use super::{GlobExpansion, GlobOutcome, GlobSkippedEntries}; +use metrics::{counter, describe_counter}; +use std::sync::Once; + +const EXPANSIONS_TOTAL: &str = "netsuke_manifest_glob_expansions_total"; +const ENTRIES_SKIPPED_TOTAL: &str = "netsuke_manifest_glob_entries_skipped_total"; +const REDACTED_PATH: &str = ""; + +/// Register the metric descriptions once per process. +fn describe_metrics() { + static DESCRIBE: Once = Once::new(); + DESCRIBE.call_once(|| { + describe_counter!( + EXPANSIONS_TOTAL, + "Counts glob expansions labelled by outcome: matched, or \ + unopenable_prefix when the pattern's literal directory prefix \ + names no directory." + ); + describe_counter!( + ENTRIES_SKIPPED_TOTAL, + "Counts matched entries dropped from a glob expansion, labelled \ + by reason: unreachable_symlink for a link the capability cannot \ + resolve, not_a_file for a directory or other non-file." + ); + }); +} + +/// Record the observations returned by the pure glob expansion query. +pub(super) fn record(expansion: &GlobExpansion) { + match &expansion.outcome { + GlobOutcome::Matched => record_expansion_matched(expansion), + GlobOutcome::UnopenablePrefix => record_unopenable_prefix(), + } + record_skipped_entries(&expansion.skipped); +} + +/// Record an expansion that stopped because the literal prefix is unusable. +fn record_unopenable_prefix() { + describe_metrics(); + counter!(EXPANSIONS_TOTAL, "outcome" => "unopenable_prefix").increment(1); + tracing::debug!( + pattern = REDACTED_PATH, + prefix = REDACTED_PATH, + "glob literal prefix names no directory; expanding to no matches" + ); +} + +/// Record an expansion that ran the walk to completion. +fn record_expansion_matched(expansion: &GlobExpansion) { + describe_metrics(); + counter!(EXPANSIONS_TOTAL, "outcome" => "matched").increment(1); + tracing::debug!( + pattern = REDACTED_PATH, + matches = expansion.paths.len(), + "glob expansion complete" + ); +} + +/// Record a match dropped because the capability cannot resolve it. +/// +/// `relative` is the match relative to the literal prefix, so it stays within +/// the scope the pattern already named. +fn record_skipped_entries(skipped: &GlobSkippedEntries) { + describe_metrics(); + if skipped.unreachable_symlinks != 0 { + counter!(ENTRIES_SKIPPED_TOTAL, "reason" => "unreachable_symlink") + .increment(u64::try_from(skipped.unreachable_symlinks).unwrap_or(u64::MAX)); + for _ in &skipped.unreachable_symlink_samples { + record_unreachable_symlink(); + } + } + if skipped.not_a_file != 0 { + counter!(ENTRIES_SKIPPED_TOTAL, "reason" => "not_a_file") + .increment(u64::try_from(skipped.not_a_file).unwrap_or(u64::MAX)); + } +} + +/// Trace an unreachable symbolic-link entry retained in the bounded sample. +fn record_unreachable_symlink() { + tracing::debug!( + relative = REDACTED_PATH, + "glob match traverses a symbolic link the capability cannot resolve; skipping" + ); +} diff --git a/src/manifest/glob/mod.rs b/src/manifest/glob/mod.rs index 531e88b9e..f3492019a 100644 --- a/src/manifest/glob/mod.rs +++ b/src/manifest/glob/mod.rs @@ -1,6 +1,31 @@ -//! Utilities for normalising and validating manifest glob patterns. +//! Filesystem glob expansion for manifest templates. +//! +//! [`glob_paths`] is the module's only boundary: `manifest` re-exports it as +//! the `glob()` Jinja helper and nothing else here is reachable from the crate +//! root. It takes a raw pattern, expands it, and returns the matching file +//! paths in the order the `glob` crate yields them, with directories filtered +//! out. +//! +//! The work is split across four private submodules: +//! +//! - `validate` rejects unbalanced braces before any filesystem access. +//! - `normalize` maps separators onto the platform's and, on Unix, rewrites +//! backslash escapes into the bracket classes the `glob` crate understands. +//! [`GlobPattern`] pairs the caller's text with that normalised form. +//! - `walk` owns the filesystem side: it computes the pattern's literal +//! directory prefix, opens a capability-scoped `cap_std` handle there, and +//! runs the metadata check that filters each match. +//! - `diagnostics` records the bounded data the pure expansion query returns +//! at the manifest orchestration boundary. +//! +//! Matching itself belongs to the `glob` crate, which traverses the filesystem +//! ambiently; only the metadata check is capability-scoped. `walk`'s module +//! documentation and +//! [ADR-010](https://github.com/leynos/netsuke/blob/main/docs/adr-010-scope-glob-capability-to-literal-prefix.md) +//! describe that boundary and why it remains. use minijinja::Error; +mod diagnostics; mod errors; mod normalize; mod validate; @@ -77,6 +102,60 @@ impl GlobPattern { /// view; they live there because that is where rustdoc will run them. type GlobEntryResult = std::result::Result; +/// A completed glob expansion and the bounded outcomes it observed. +pub(super) struct GlobExpansion { + paths: Vec, + outcome: GlobOutcome, + skipped: GlobSkippedEntries, +} + +/// Terminal outcome of a glob expansion. +enum GlobOutcome { + Matched, + UnopenablePrefix, +} + +/// Maximum unreachable-symlink paths retained for tracing one expansion. +const MAX_UNREACHABLE_SYMLINK_SAMPLES: usize = 4; + +/// Bounded diagnostic data about entries omitted from an expansion. +#[derive(Default)] +struct GlobSkippedEntries { + unreachable_symlinks: usize, + unreachable_symlink_samples: Vec, + not_a_file: usize, +} + +impl GlobSkippedEntries { + /// Record an unreachable symlink while retaining a bounded trace sample. + fn record_unreachable_symlink(&mut self, relative: camino::Utf8PathBuf) { + self.unreachable_symlinks += 1; + if self.unreachable_symlink_samples.len() < MAX_UNREACHABLE_SYMLINK_SAMPLES { + self.unreachable_symlink_samples.push(relative); + } + } + + /// Record an entry that does not name a regular file. + const fn record_not_a_file(&mut self) { + self.not_a_file += 1; + } +} + +/// Entry selected by the capability-scoped metadata query. +#[derive(Debug)] +pub(super) enum GlobEntry { + Path(String), + UnreachableSymlink(camino::Utf8PathBuf), + NotAFile, +} + +impl GlobExpansion { + /// Consume the expansion and return the paths the query selected. + pub(super) fn into_paths(self) -> Vec { + self.paths + } +} + /// Expand a glob pattern and collect the matching UTF-8 file paths. /// /// This is the only public item in the glob module: `netsuke::manifest` @@ -111,6 +190,11 @@ type GlobEntryResult = std::result::Result; /// instead if the rustdoc harness wiring breaks, so the `compile_fail` block /// cannot pass vacuously. pub fn glob_paths(pattern: &str) -> std::result::Result, Error> { + expand_glob(pattern).map(GlobExpansion::into_paths) +} + +/// Expand a pattern and return its bounded diagnostic data without recording it. +pub(super) fn expand_glob(pattern: &str) -> std::result::Result { use glob::{MatchOptions, glob_with}; let opts = MatchOptions { @@ -120,37 +204,64 @@ pub fn glob_paths(pattern: &str) -> std::result::Result, Error> { }; let pattern_state = GlobPattern::new(pattern)?; - - let root = open_root_dir(&pattern_state).map_err(|e| { + let entries = glob_with(pattern_state.normalized(), opts).map_err(|e| { create_glob_error( &GlobErrorContext { pattern: pattern_state.raw().to_owned(), error_char: char::from(0), position: 0, - error_type: GlobErrorType::IoError, + error_type: GlobErrorType::InvalidPattern, }, Some(e.to_string()), ) })?; - let entries = glob_with(pattern_state.normalized(), opts).map_err(|e| { + let Some(root) = open_root_dir(&pattern_state).map_err(|e| { create_glob_error( &GlobErrorContext { pattern: pattern_state.raw().to_owned(), error_char: char::from(0), position: 0, - error_type: GlobErrorType::InvalidPattern, + error_type: GlobErrorType::IoError, }, Some(e.to_string()), ) - })?; + })? + else { + // The pattern's literal directory prefix does not exist, so the + // pattern cannot match anything. + return Ok(GlobExpansion { + outcome: GlobOutcome::UnopenablePrefix, + paths: Vec::new(), + skipped: GlobSkippedEntries::default(), + }); + }; + let mut paths = Vec::new(); + let mut skipped = GlobSkippedEntries::default(); for entry in entries { - if let Some(p) = process_glob_entry(entry, &pattern_state, &root)? { - paths.push(p); + match process_glob_entry(entry, &pattern_state, &root)? { + GlobEntry::Path(path) => paths.push(path), + GlobEntry::UnreachableSymlink(relative) => { + skipped.record_unreachable_symlink(relative); + } + GlobEntry::NotAFile => skipped.record_not_a_file(), } } - Ok(paths) + Ok(GlobExpansion { + paths, + outcome: GlobOutcome::Matched, + skipped, + }) +} + +/// Record the bounded observations from a completed expansion. +/// +/// `glob_paths` deliberately does not call this function: it is a pure query. +/// The manifest-template adapter records observations after it calls +/// [`expand_glob`]. +pub(super) fn record_expansion(expansion: &GlobExpansion) { + diagnostics::record(expansion); } #[cfg(test)] diff --git a/src/manifest/glob/tests/capability.rs b/src/manifest/glob/tests/capability.rs new file mode 100644 index 000000000..7d808365d --- /dev/null +++ b/src/manifest/glob/tests/capability.rs @@ -0,0 +1,377 @@ +//! Tests for the capability handle the glob metadata checks run through. +use super::super::walk::{literal_dir_prefix, open_root_dir}; +use super::super::{GlobPattern, glob_paths}; +use anyhow::{Context, Result, anyhow, ensure}; +use camino::{Utf8Path, Utf8PathBuf}; +use minijinja::ErrorKind; +use rstest::{fixture, rstest}; +use tempfile::{TempDir, tempdir}; +use test_support::cwd_guard::CwdGuard; +use test_support::env_lock::EnvLock; +use test_support::fs as test_fs; + +/// A tree with one file inside `scoped/` and one sibling outside it. +/// +/// Shared by the scoping tests so each asserts against the same layout: +/// `scoped/in.txt` is reachable through a capability rooted at `scoped/`, +/// whereas `out.txt` is not. +#[fixture] +fn scoped_tree() -> Result { + let temp = tempdir()?; + let scoped = temp.path().join("scoped"); + test_fs::create_dir(&scoped)?; + test_fs::write(scoped.join("in.txt"), "in")?; + test_fs::write(temp.path().join("out.txt"), "out")?; + Ok(temp) +} + +#[cfg(unix)] +#[rstest] +#[case("src/*.c", "src/")] +#[case("src/sub/**/*.c", "src/sub/")] +#[case("*.c", ".")] +#[case("a.txt", ".")] +#[case("/tmp/x/*.txt", "/tmp/x/")] +#[case("/*.txt", "/")] +#[case("src/a.txt", "src/")] +#[case("src/{a,b}/*.c", "src/")] +// A bracketed literal escape names a character, not a wildcard, so the scan +// steps over it and keeps the directories beyond it in the prefix. +#[case("src/[*]x/generated/*.c", "src/[*]x/generated/")] +#[case("[[]dir/*.c", "[[]dir/")] +// A genuine character class is a wildcard and still stops the scan. +#[case("src/[ab]x/*.c", "src/")] +#[case("src/[a]x/*.c", "src/")] +fn literal_dir_prefix_stops_at_first_metacharacter(#[case] pattern: &str, #[case] expected: &str) { + assert_eq!(literal_dir_prefix(pattern), expected, "pattern {pattern}"); +} + +/// A prefix naming nothing, and one naming a regular file, must both yield no +/// capability at all — not merely an empty match set, which an unscoped +/// implementation would also produce. +#[rstest] +#[case("no-such-dir", "missing directory")] +#[case("out.txt", "regular file")] +fn open_root_dir_declines_unopenable_prefix( + scoped_tree: Result, + #[case] prefix: &str, + #[case] desc: &str, +) -> Result<()> { + let temp = scoped_tree?; + let pattern = GlobPattern::new(&format!("{}/{prefix}/*.txt", temp.path().display()))?; + let root = open_root_dir(&pattern).with_context(|| format!("open root for {desc}"))?; + ensure!( + root.is_none(), + "{desc} prefix should yield no capability at all" + ); + ensure!( + glob_paths(pattern.raw())?.is_empty(), + "{desc} prefix should expand to no matches" + ); + Ok(()) +} + +/// A directory whose name contains a glob metacharacter is still a literal +/// directory, so the capability reaches into it rather than stopping at its +/// parent — and the prefix is unescaped before it meets the filesystem. +#[cfg(unix)] +#[test] +fn open_root_dir_scopes_past_an_escaped_metacharacter() -> Result<()> { + let temp = tempdir()?; + let odd = temp.path().join("*x"); + test_fs::create_dir(&odd)?; + test_fs::write(odd.join("in.txt"), "in")?; + test_fs::write(temp.path().join("out.txt"), "out")?; + + let pattern = GlobPattern::new(&format!(r"{}/\*x/*.txt", temp.path().display()))?; + let root = open_root_dir(&pattern) + .context("open capability root")? + .ok_or_else(|| anyhow!("the escaped directory exists, so a root was expected"))?; + + let expected_prefix = Utf8PathBuf::try_from(odd)?; + ensure!( + root.prefix() == expected_prefix, + "capability should reach the escaped directory, got {prefix}", + prefix = root.prefix() + ); + ensure!( + root.dir().metadata("in.txt").is_ok(), + "the escaped directory's contents must be reachable" + ); + + let results = glob_paths(pattern.raw())?; + ensure!( + results.iter().all(|p| p.ends_with("in.txt")) && results.len() == 1, + "expected only the file inside the escaped directory: {results:?}" + ); + Ok(()) +} + +/// A literal prefix must not follow a symbolic link before its capability is +/// established, even when the linked directory would contain a valid match. +#[cfg(unix)] +#[test] +fn open_root_dir_rejects_a_symlinked_literal_prefix() -> Result<()> { + let temp = tempdir()?; + let target = temp.path().join("target"); + test_fs::create_dir(&target)?; + test_fs::write(target.join("in.c"), "in")?; + test_fs::symlink("target", temp.path().join("link"))?; + + let pattern = GlobPattern::new(&format!("{}/link/*.c", temp.path().display()))?; + ensure!( + open_root_dir(&pattern).is_err(), + "a symbolic-link prefix must not receive a capability" + ); + ensure!( + glob_paths(pattern.raw()).is_err(), + "a symbolic-link prefix must fail rather than traverse its target" + ); + Ok(()) +} + +/// Restores a directory's mode so the temporary tree can still be removed. +#[cfg(unix)] +struct ModeGuard(Utf8PathBuf); + +#[cfg(unix)] +impl Drop for ModeGuard { + fn drop(&mut self) { + if let Err(err) = test_fs::set_mode(&self.0, 0o755) { + tracing::warn!("failed to restore mode on {path}: {err}", path = self.0); + } + } +} + +/// A prefix that exists but cannot be opened is a genuine failure, not an +/// empty match set: only a missing or non-directory prefix short-circuits, so +/// anything else must reach the caller as an error. +#[cfg(unix)] +#[test] +fn open_root_dir_propagates_an_unreadable_prefix() -> Result<()> { + let temp = tempdir()?; + let locked = temp.path().join("locked"); + test_fs::create_dir(&locked)?; + test_fs::create_dir(locked.join("inner"))?; + test_fs::write(locked.join("inner").join("in.txt"), "in")?; + test_fs::set_mode(&locked, 0o000)?; + let _restore = ModeGuard(Utf8PathBuf::try_from(locked.clone())?); + + let pattern = GlobPattern::new(&format!("{}/inner/*.txt", locked.display()))?; + let Err(err) = open_root_dir(&pattern) else { + // A privileged user bypasses the mode, so there is nothing to observe. + tracing::warn!("skipping: the mode-000 prefix stayed readable"); + return Ok(()); + }; + ensure!( + err.kind() == std::io::ErrorKind::PermissionDenied, + "unexpected error kind {kind:?}", + kind = err.kind() + ); + + let expansion = glob_paths(pattern.raw()) + .expect_err("an unreadable prefix must fail the expansion, not silently match nothing"); + ensure!( + expansion.kind() == ErrorKind::InvalidOperation, + "unexpected error kind {kind:?}", + kind = expansion.kind() + ); + Ok(()) +} + +#[rstest] +fn open_root_dir_scopes_capability_to_literal_prefix(scoped_tree: Result) -> Result<()> { + // The capability must be opened at the pattern's literal prefix, not at + // the filesystem root, so a sibling of the prefix is unreachable through + // the handle even though it exists on disk. + let temp = scoped_tree?; + let pattern = GlobPattern::new(&format!("{}/scoped/*.txt", temp.path().display()))?; + let root = open_root_dir(&pattern) + .context("open capability root")? + .ok_or_else(|| anyhow!("literal prefix exists, so a root was expected"))?; + + let expected_prefix = Utf8PathBuf::try_from(temp.path().join("scoped"))?; + ensure!( + root.prefix() == expected_prefix, + "capability prefix {prefix} should be the literal prefix {expected_prefix}", + prefix = root.prefix() + ); + ensure!( + root.dir().metadata("in.txt").is_ok(), + "files under the prefix must be reachable through the capability" + ); + ensure!( + root.dir().metadata("../out.txt").is_err(), + "the capability must not reach outside the literal prefix" + ); + Ok(()) +} + +/// The walker yields matches rooted the way the pattern was, so they only +/// resolve through the scoped handle once relativised against the prefix. +#[rstest] +fn glob_root_relativises_matches_against_the_prefix(scoped_tree: Result) -> Result<()> { + let temp = scoped_tree?; + let root_path = + Utf8PathBuf::try_from(temp.path().to_path_buf()).context("temp dir path is not UTF-8")?; + let pattern = GlobPattern::new(&format!("{root_path}/scoped/*.txt"))?; + let root = open_root_dir(&pattern) + .context("open capability root")? + .ok_or_else(|| anyhow!("literal prefix exists, so a root was expected"))?; + + let metadata = root + .metadata(&root_path.join("scoped/in.txt")) + .context("metadata for a match inside the prefix")? + .ok_or_else(|| anyhow!("in.txt should resolve through the capability"))?; + ensure!(metadata.is_file(), "in.txt should be a regular file"); + + // A sibling outside the prefix cannot be relativised, so it is rejected + // rather than silently resolved through a wider capability. + let err = root + .metadata(&root_path.join("out.txt")) + .expect_err("a sibling outside the prefix must not resolve"); + ensure!( + err.kind() == std::io::ErrorKind::InvalidInput, + "unexpected error kind {kind:?}", + kind = err.kind() + ); + Ok(()) +} + +#[rstest] +fn glob_paths_matches_only_within_literal_prefix(scoped_tree: Result) -> Result<()> { + let temp = scoped_tree?; + let pattern = format!("{}/scoped/*.txt", temp.path().display()); + let results = glob_paths(&pattern)?; + ensure!( + results.iter().all(|p| p.ends_with("in.txt")), + "only files under the literal prefix should match: {results:?}" + ); + ensure!(results.len() == 1, "expected one match: {results:?}"); + Ok(()) +} + +#[test] +fn glob_paths_matches_parent_relative_patterns() -> Result<()> { + // Scoping the capability at the literal prefix also reaches patterns that + // ascend past the working directory: a `..` component in a match used to + // be rejected by the working-directory handle as a sandbox escape. + let temp = tempdir()?; + let sub = temp.path().join("sub"); + test_fs::create_dir(&sub)?; + test_fs::write(temp.path().join("out.txt"), "out")?; + + let _lock = EnvLock::acquire(); + let _guard = CwdGuard::acquire()?; + std::env::set_current_dir(&sub).context("switch to the subdirectory")?; + + let results = glob_paths("../*.txt")?; + ensure!( + results == vec!["../out.txt".to_owned()], + "expected the parent-relative match, got {results:?}" + ); + Ok(()) +} + +/// A symbolic link pointing out of the literal prefix is unreadable through +/// the capability. It names no file the expansion can offer, so it is skipped +/// rather than aborting the whole traversal — whether the link is the match's +/// final component or an intermediate directory it is reached through. +#[cfg(unix)] +#[rstest] +#[case::final_component("src/*.c", "escaped.c", "../vendor/escaped.c", "real.c")] +#[case::intermediate_directory("src/*/*.c", "link", "../vendor", "real/real.c")] +fn glob_paths_skips_symlinks_escaping_the_prefix( + #[case] pattern_tail: &str, + #[case] link_name: &str, + #[case] link_target: &str, + #[case] kept: &str, +) -> Result<()> { + let temp = tempdir()?; + let src = temp.path().join("src"); + let vendor = temp.path().join("vendor"); + test_fs::create_dir(&src)?; + test_fs::create_dir(&vendor)?; + test_fs::write(vendor.join("escaped.c"), "escaped")?; + if let Some(parent) = Utf8Path::new(kept) + .parent() + .filter(|p| !p.as_str().is_empty()) + { + test_fs::create_dir_all(src.join(parent))?; + } + test_fs::write(src.join(kept), "kept")?; + test_fs::symlink(link_target, src.join(link_name))?; + + let pattern = format!("{}/{pattern_tail}", temp.path().display()); + let results = glob_paths(&pattern).context("an escaping symlink must not abort the walk")?; + ensure!( + results.iter().any(|p| p.ends_with(kept)), + "valid matches should be preserved: {results:?}" + ); + ensure!( + results.iter().all(|p| !p.ends_with("escaped.c")), + "a symlink resolving outside the prefix should be skipped: {results:?}" + ); + Ok(()) +} + +/// A link with no target is absent rather than broken, so it is skipped like +/// an escaping one. +#[cfg(unix)] +#[test] +fn glob_paths_skips_dangling_symlinks() -> Result<()> { + let temp = tempdir()?; + let src = temp.path().join("src"); + test_fs::create_dir(&src)?; + test_fs::write(src.join("real.c"), "real")?; + test_fs::symlink("nowhere.c", src.join("dangling.c"))?; + + let pattern = format!("{}/src/*.c", temp.path().display()); + let results = glob_paths(&pattern).context("a dangling symlink must not abort the walk")?; + ensure!( + results.iter().any(|p| p.ends_with("real.c")), + "valid matches should be preserved: {results:?}" + ); + ensure!( + results.iter().all(|p| !p.ends_with("dangling.c")), + "a dangling symlink should be skipped: {results:?}" + ); + Ok(()) +} + +/// A cyclic link describes a broken tree rather than an absent file, so it +/// must not be quietly dropped along with the escaping and dangling links. +#[cfg(unix)] +#[test] +fn glob_paths_reports_symlink_loops() -> Result<()> { + let temp = tempdir()?; + let src = temp.path().join("src"); + test_fs::create_dir(&src)?; + test_fs::symlink("loop.c", src.join("loop.c"))?; + + let pattern = format!("{}/src/*.c", temp.path().display()); + let err = glob_paths(&pattern).expect_err("a symlink loop should surface as an error"); + ensure!( + err.kind() == ErrorKind::InvalidOperation, + "unexpected error kind {kind:?}", + kind = err.kind() + ); + Ok(()) +} + +#[test] +fn open_root_dir_falls_back_to_cwd_without_a_literal_prefix() -> Result<()> { + // Patterns whose first component is a wildcard have no literal directory + // component, so the capability stays scoped to the working directory — + // the pre-existing behaviour for relative patterns. + let pattern = GlobPattern::new("*.txt")?; + let root = open_root_dir(&pattern) + .context("open capability root")? + .ok_or_else(|| anyhow!("the working directory always exists"))?; + ensure!( + root.prefix() == Utf8Path::new("."), + "unexpected prefix {prefix}", + prefix = root.prefix() + ); + Ok(()) +} diff --git a/src/manifest/glob/tests/diagnostics.rs b/src/manifest/glob/tests/diagnostics.rs new file mode 100644 index 000000000..c95234f1b --- /dev/null +++ b/src/manifest/glob/tests/diagnostics.rs @@ -0,0 +1,271 @@ +//! Tests for the counters and tracing events glob expansion records. +//! +//! Each case records data returned by the pure expansion query through a +//! subscriber scoped to the call. The recorder and subscriber are both +//! thread-local, so no test-wide lock is needed. + +use super::super::{MAX_UNREACHABLE_SYMLINK_SAMPLES, expand_glob, glob_paths, record_expansion}; +use anyhow::{Context, Result, ensure}; +use metrics::SharedString; +use metrics_util::{ + CompositeKey, MetricKind, + debugging::{DebugValue, DebuggingRecorder}, +}; +use rstest::rstest; +use tempfile::tempdir; +use test_support::fs as test_fs; +use tracing::level_filters::LevelFilter; + +type Snapshot = Vec<( + CompositeKey, + Option, + Option, + DebugValue, +)>; + +/// Run `expand` with a local metrics recorder and a capturing subscriber. +fn recorded(expand: impl FnOnce() -> T) -> (T, Vec, Snapshot) { + let recorder = DebuggingRecorder::new(); + let snapshotter = recorder.snapshotter(); + let (value, events) = metrics::with_local_recorder(&recorder, || { + crate::test_tracing_capture::with_test_subscriber(LevelFilter::DEBUG, |captured| { + let value = expand(); + (value, captured.snapshot()) + }) + }); + (value, events, snapshotter.snapshot().into_vec()) +} + +/// Expand and record at the manifest adapter's telemetry boundary. +fn expand_and_record(pattern: &str) -> Result> { + let expansion = expand_glob(pattern)?; + record_expansion(&expansion); + Ok(expansion.into_paths()) +} + +/// Value of the counter `name` carrying the label `label = value`. +fn counter_value(snapshot: &Snapshot, name: &str, label: (&str, &str)) -> Option { + snapshot.iter().find_map(|(key, _, _, debug_value)| { + if key.kind() != MetricKind::Counter || key.key().name() != name { + return None; + } + let carries_label = key + .key() + .labels() + .any(|found| found.key() == label.0 && found.value() == label.1); + match debug_value { + DebugValue::Counter(count) if carries_label => Some(*count), + _ => None, + } + }) +} + +const EXPANSIONS: &str = "netsuke_manifest_glob_expansions_total"; +const SKIPPED: &str = "netsuke_manifest_glob_entries_skipped_total"; + +#[rstest] +fn a_completed_expansion_counts_its_matches() -> Result<()> { + let temp = tempdir()?; + test_fs::write(temp.path().join("a.txt"), "a")?; + test_fs::write(temp.path().join("b.txt"), "b")?; + let pattern = format!("{}/*.txt", temp.path().display()); + + let (results, events, snapshot) = recorded(|| expand_and_record(&pattern)); + ensure!(results?.len() == 2, "both files should match"); + + ensure!( + counter_value(&snapshot, EXPANSIONS, ("outcome", "matched")) == Some(1), + "a completed expansion should count once as matched: {snapshot:?}" + ); + let expansion_event = events + .iter() + .find(|event| event.contains("glob expansion complete")) + .context("expected a completed-expansion event")?; + ensure!( + expansion_event.contains("matches=2") && expansion_event.contains("pattern=\"\""), + "expected bounded fields in the trace event: {expansion_event}" + ); + ensure!( + !expansion_event.contains(&temp.path().display().to_string()), + "the event must not disclose the absolute path: {expansion_event}" + ); + Ok(()) +} + +#[rstest] +fn an_unopenable_prefix_counts_and_names_the_prefix() -> Result<()> { + let temp = tempdir()?; + let pattern = format!("{}/no-such-dir/*.txt", temp.path().display()); + + let (results, events, snapshot) = recorded(|| expand_and_record(&pattern)); + ensure!(results?.is_empty(), "a missing prefix should match nothing"); + + ensure!( + counter_value(&snapshot, EXPANSIONS, ("outcome", "unopenable_prefix")) == Some(1), + "an unopenable prefix should count once: {snapshot:?}" + ); + ensure!( + counter_value(&snapshot, EXPANSIONS, ("outcome", "matched")).is_none(), + "an expansion that never ran must not count as matched: {snapshot:?}" + ); + let prefix_event = events + .iter() + .find(|event| event.contains("glob literal prefix names no directory")) + .context("expected an unopenable-prefix event")?; + ensure!( + prefix_event.contains("pattern=\"\"") + && prefix_event.contains("prefix=\"\""), + "expected bounded fields in the trace event: {prefix_event}" + ); + ensure!( + !prefix_event.contains(&temp.path().display().to_string()), + "the event must not disclose the absolute path: {prefix_event}" + ); + Ok(()) +} + +/// A skipped match is counted by reason and traced without its path. +#[cfg(unix)] +#[rstest] +fn a_skipped_symlink_counts_and_redacts_its_relative_path() -> Result<()> { + let temp = tempdir()?; + let src = temp.path().join("src"); + let vendor = temp.path().join("vendor"); + test_fs::create_dir(&src)?; + test_fs::create_dir(&vendor)?; + test_fs::write(vendor.join("escaped.txt"), "escaped")?; + test_fs::symlink("../vendor/escaped.txt", src.join("escaped.txt"))?; + let pattern = format!("{}/src/*.txt", temp.path().display()); + + let (results, events, snapshot) = recorded(|| expand_and_record(&pattern)); + ensure!(results?.is_empty(), "the only match should be skipped"); + + ensure!( + counter_value(&snapshot, SKIPPED, ("reason", "unreachable_symlink")) == Some(1), + "the skipped link should count once: {snapshot:?}" + ); + let skip_event = events + .iter() + .find(|event| event.contains("cannot resolve")) + .context("expected a skipped-match event")?; + ensure!( + skip_event.contains("relative=\"\""), + "the event should carry a redacted relative path: {skip_event}" + ); + ensure!( + !skip_event.contains(&temp.path().display().to_string()), + "the event must not disclose the absolute path: {skip_event}" + ); + ensure!( + !skip_event.contains("escaped.txt"), + "the event must not disclose the relative path: {skip_event}" + ); + Ok(()) +} + +/// All skipped links contribute to metrics, but traces retain only four entries. +#[cfg(unix)] +#[rstest] +fn skipped_symlink_diagnostics_retain_a_bounded_sample() -> Result<()> { + let temp = tempdir()?; + let src = temp.path().join("src"); + let vendor = temp.path().join("vendor"); + test_fs::create_dir(&src)?; + test_fs::create_dir(&vendor)?; + let skipped_count = MAX_UNREACHABLE_SYMLINK_SAMPLES + 2; + for index in 0..skipped_count { + let name = format!("escaped-{index:02}.txt"); + test_fs::write(vendor.join(&name), "escaped")?; + test_fs::symlink(format!("../vendor/{name}"), src.join(name))?; + } + let pattern = format!("{}/src/*.txt", temp.path().display()); + + let (results, events, snapshot) = recorded(|| expand_and_record(&pattern)); + ensure!(results?.is_empty(), "every match should be skipped"); + ensure!( + counter_value(&snapshot, SKIPPED, ("reason", "unreachable_symlink")) + == Some(skipped_count as u64), + "the aggregate counter should include every skipped link: {snapshot:?}" + ); + let sampled_events: Vec<_> = events + .iter() + .filter(|event| event.contains("cannot resolve")) + .collect(); + ensure!( + sampled_events.len() == MAX_UNREACHABLE_SYMLINK_SAMPLES, + "expected exactly the bounded trace sample: {sampled_events:?}" + ); + for sampled_event in &sampled_events { + ensure!( + sampled_event.contains("relative=\"\""), + "sampled events must redact their paths: {sampled_events:?}" + ); + } + ensure!( + !sampled_events + .iter() + .any(|event| event.contains("escaped-")), + "sampled events must not disclose retained paths: {sampled_events:?}" + ); + Ok(()) +} + +#[rstest] +fn a_relative_unopenable_prefix_redacts_caller_controlled_fields() -> Result<()> { + let pattern = "glob-diagnostics-no-such-prefix/*.txt"; + + let (results, events, _snapshot) = recorded(|| expand_and_record(pattern)); + ensure!(results?.is_empty(), "a missing prefix should match nothing"); + + let prefix_event = events + .iter() + .find(|event| event.contains("glob literal prefix names no directory")) + .context("expected an unopenable-prefix event")?; + ensure!( + prefix_event.contains("pattern=\"\"") + && prefix_event.contains("prefix=\"\""), + "expected redacted fields in the trace event: {prefix_event}" + ); + ensure!( + !prefix_event.contains("glob-diagnostics-no-such-prefix"), + "the event must not disclose the relative pattern: {prefix_event}" + ); + Ok(()) +} + +#[rstest] +fn a_directory_match_counts_as_not_a_file() -> Result<()> { + let temp = tempdir()?; + test_fs::create_dir(temp.path().join("sub"))?; + test_fs::write(temp.path().join("a.txt"), "a")?; + let pattern = format!("{}/*", temp.path().display()); + + let (results, _events, snapshot) = recorded(|| expand_and_record(&pattern)); + ensure!(results?.len() == 1, "only the file should survive"); + + ensure!( + counter_value(&snapshot, SKIPPED, ("reason", "not_a_file")) == Some(1), + "the directory should count once as not a file: {snapshot:?}" + ); + Ok(()) +} + +/// Direct callers can reuse the glob query without receiving global telemetry. +#[rstest] +fn glob_paths_is_a_pure_query() -> Result<()> { + let temp = tempdir()?; + test_fs::write(temp.path().join("a.txt"), "a")?; + let pattern = format!("{}/*.txt", temp.path().display()); + + let (results, events, snapshot) = recorded(|| glob_paths(&pattern)); + ensure!(results?.len() == 1, "the file should match"); + ensure!( + events.is_empty(), + "the query must not emit trace events: {events:?}" + ); + ensure!( + snapshot.is_empty(), + "the query must not record metrics: {snapshot:?}" + ); + Ok(()) +} diff --git a/src/manifest/glob/tests/expansion.rs b/src/manifest/glob/tests/expansion.rs new file mode 100644 index 000000000..bbe5235cd --- /dev/null +++ b/src/manifest/glob/tests/expansion.rs @@ -0,0 +1,87 @@ +//! Tests for the match set [`glob_paths`] returns. +use super::super::walk::{GlobRoot, process_glob_entry}; +use super::super::{GlobPattern, glob_paths}; +use anyhow::{Context, Result, anyhow, ensure}; +use cap_std::{ambient_authority, fs::Dir}; +use minijinja::ErrorKind; +use rstest::rstest; +use tempfile::tempdir; +use test_support::fs as test_fs; + +#[test] +fn glob_paths_filters_directories() -> Result<()> { + let temp = tempdir()?; + let dir = temp.path().join("dir"); + test_fs::create_dir(&dir)?; + let file = temp.path().join("dir").join("file.txt"); + test_fs::write(&file, "data")?; + + let pattern = format!("{}/dir/*", temp.path().display()); + let results = glob_paths(&pattern)?; + ensure!( + results.iter().any(|p| p.ends_with("file.txt")), + "expected file match" + ); + ensure!( + results.iter().all(|p| !p.ends_with("/dir")), + "directories should be filtered out" + ); + Ok(()) +} + +#[test] +fn glob_paths_rejects_unmatched_brace() { + let err = glob_paths("foo{bar").expect_err("brace mismatch should error"); + assert_eq!(err.kind(), ErrorKind::SyntaxError); +} + +#[rstest] +fn glob_paths_rejects_an_invalid_pattern_before_a_missing_prefix() { + let err = glob_paths("missing/[").expect_err("an invalid pattern should error"); + assert_eq!(err.kind(), ErrorKind::SyntaxError); +} + +#[cfg(unix)] +#[test] +fn glob_paths_accepts_escaped_braces_and_matches_files() -> Result<()> { + let temp = tempdir()?; + let file = temp.path().join("{file}.txt"); + test_fs::write(&file, "data")?; + + let pattern = format!("{}/\\{{file\\}}.txt", temp.path().display()); + let normalized = GlobPattern::new(&pattern)?; + ensure!( + normalized.normalized().contains("[{]file[}]"), + "unexpected normalized pattern: {}", + normalized.normalized() + ); + let results = glob_paths(&pattern)?; + ensure!( + results.iter().any(|p| p.ends_with("{file}.txt")), + "escaped brace pattern should match literal braces" + ); + Ok(()) +} + +#[cfg(unix)] +#[test] +fn process_glob_entry_rejects_non_utf8_paths() -> Result<()> { + use std::ffi::OsString; + use std::os::unix::ffi::OsStringExt; + + let dir = Dir::open_ambient_dir("/", ambient_authority()).context("open ambient root dir")?; + let root = GlobRoot::new(dir, camino::Utf8PathBuf::from("/")); + let path = std::path::PathBuf::from(OsString::from_vec(b"bad\xFF".to_vec())); + let pattern = GlobPattern::new("pattern")?; + match process_glob_entry(Ok(path), &pattern, &root) { + Ok(value) => Err(anyhow!("expected non-UTF-8 error but received {value:?}")), + Err(err) => { + ensure!( + err.kind() == ErrorKind::InvalidOperation, + "unexpected error kind {kind:?}", + kind = err.kind() + ); + Ok(()) + } + } +} diff --git a/src/manifest/glob/tests/mod.rs b/src/manifest/glob/tests/mod.rs new file mode 100644 index 000000000..267ce819a --- /dev/null +++ b/src/manifest/glob/tests/mod.rs @@ -0,0 +1,14 @@ +//! Tests for glob validation and expansion helpers. +//! +//! Split by concern: [`pattern`] covers normalisation and brace validation, +//! [`expansion`] covers the matches [`super::glob_paths`] returns, +//! [`capability`] covers the capability handle the metadata checks run +//! through, [`diagnostics`] covers the counters and events it records, and +//! [`property`] covers the prefix and relativisation invariants the fixed +//! cases are examples of. + +mod capability; +mod diagnostics; +mod expansion; +mod pattern; +mod property; diff --git a/src/manifest/glob/tests.rs b/src/manifest/glob/tests/pattern.rs similarity index 64% rename from src/manifest/glob/tests.rs rename to src/manifest/glob/tests/pattern.rs index 9ca8fe5ec..724e75453 100644 --- a/src/manifest/glob/tests.rs +++ b/src/manifest/glob/tests/pattern.rs @@ -1,17 +1,13 @@ -//! Tests for glob validation and expansion helpers. +//! Tests for glob pattern normalisation and brace validation. +use super::super::GlobPattern; #[cfg(unix)] -use super::normalize::force_literal_escapes; -use super::normalize::normalize_separators; -use super::validate::validate_brace_matching; -use super::walk::process_glob_entry; -use super::{GlobPattern, glob_paths}; +use super::super::normalize::force_literal_escapes; +use super::super::normalize::normalize_separators; +use super::super::validate::validate_brace_matching; use crate::localization::{self, keys}; use anyhow::{Context, Result, anyhow, ensure}; -use cap_std::{ambient_authority, fs::Dir}; use minijinja::ErrorKind; use rstest::rstest; -use tempfile::tempdir; -use test_support::fs as test_fs; /// Helper to assert that a pattern produces a syntax error. fn assert_syntax_error(pattern: &str, context_msg: &str) -> Result<()> { @@ -134,77 +130,6 @@ fn validate_brace_matching_rejects_unmatched_opening() -> Result<()> { } } -#[test] -fn glob_paths_filters_directories() -> Result<()> { - let temp = tempdir()?; - let dir = temp.path().join("dir"); - test_fs::create_dir(&dir)?; - let file = temp.path().join("dir").join("file.txt"); - test_fs::write(&file, "data")?; - - let pattern = format!("{}/dir/*", temp.path().display()); - let results = glob_paths(&pattern)?; - ensure!( - results.iter().any(|p| p.ends_with("file.txt")), - "expected file match" - ); - ensure!( - results.iter().all(|p| !p.ends_with("/dir")), - "directories should be filtered out" - ); - Ok(()) -} - -#[test] -fn glob_paths_rejects_unmatched_brace() { - let err = glob_paths("foo{bar").expect_err("brace mismatch should error"); - assert_eq!(err.kind(), ErrorKind::SyntaxError); -} - -#[cfg(unix)] -#[test] -fn glob_paths_accepts_escaped_braces_and_matches_files() -> Result<()> { - let temp = tempdir()?; - let file = temp.path().join("{file}.txt"); - test_fs::write(&file, "data")?; - - let pattern = format!("{}/\\{{file\\}}.txt", temp.path().display()); - let normalized = GlobPattern::new(&pattern)?; - ensure!( - normalized.normalized().contains("[{]file[}]"), - "unexpected normalized pattern: {}", - normalized.normalized() - ); - let results = glob_paths(&pattern)?; - ensure!( - results.iter().any(|p| p.ends_with("{file}.txt")), - "escaped brace pattern should match literal braces" - ); - Ok(()) -} - -#[cfg(unix)] -#[test] -fn process_glob_entry_rejects_non_utf8_paths() -> Result<()> { - use std::ffi::OsString; - use std::os::unix::ffi::OsStringExt; - - let root = Dir::open_ambient_dir("/", ambient_authority()).context("open ambient root dir")?; - let path = std::path::PathBuf::from(OsString::from_vec(b"bad\xFF".to_vec())); - let pattern = GlobPattern::new("pattern")?; - match process_glob_entry(Ok(path), &pattern, &root) { - Ok(value) => Err(anyhow!("expected non-UTF-8 error but received {value:?}")), - Err(err) => { - ensure!( - err.kind() == ErrorKind::InvalidOperation, - "unexpected error kind {kind:?}", - kind = err.kind() - ); - Ok(()) - } - } -} - #[test] fn glob_pattern_new_normalizes_and_validates() -> Result<()> { #[cfg(unix)] diff --git a/src/manifest/glob/tests/property.rs b/src/manifest/glob/tests/property.rs new file mode 100644 index 000000000..7cbdda4c0 --- /dev/null +++ b/src/manifest/glob/tests/property.rs @@ -0,0 +1,135 @@ +//! Property tests for literal-prefix extraction and capability relativisation. +//! +//! The fixed cases elsewhere in this module pin a handful of shapes. These +//! cover the invariants those shapes are examples of, across arbitrary +//! metacharacter placement: that the extracted prefix really is a +//! metacharacter-free directory prefix and really is the longest one, and that +//! relativising a match against it accepts exactly the paths inside the prefix +//! and rejects everything else. +//! +//! Both properties are pure. The [`GlobRoot`] used for relativisation holds a +//! capability that its prefix logic never consults, so one handle on the +//! working directory serves every case. + +use super::super::walk::{GlobRoot, literal_dir_prefix}; +use camino::{Utf8Path, Utf8PathBuf}; +use cap_std::{ambient_authority, fs::Dir}; +use proptest::prelude::*; +use std::path::MAIN_SEPARATOR; + +const METACHARACTERS: [char; 4] = ['*', '?', '[', '{']; + +/// Generate patterns mixing literal segments, separators and metacharacters. +fn pattern() -> impl Strategy { + proptest::collection::vec( + prop_oneof![ + "[a-z]{1,4}".prop_map(|s| s), + Just(MAIN_SEPARATOR.to_string()), + Just("*".to_owned()), + Just("?".to_owned()), + Just("[ab]".to_owned()), + Just("{a,b}".to_owned()), + ], + 0..8, + ) + .prop_map(|parts| parts.concat()) +} + +/// Build a `GlobRoot` at `prefix` whose capability is never dereferenced. +fn root_at(prefix: &str) -> Result { + let dir = Dir::open_ambient_dir(".", ambient_authority()) + .map_err(|err| TestCaseError::fail(format!("open the working directory: {err}")))?; + Ok(GlobRoot::new(dir, Utf8PathBuf::from(prefix))) +} + +proptest! { + /// The prefix is `.` or a genuine prefix of the pattern. + #[test] + fn prefix_is_a_prefix_of_the_pattern(pattern in pattern()) { + let prefix = literal_dir_prefix(&pattern); + prop_assert!( + prefix == "." || pattern.starts_with(prefix), + "prefix {prefix:?} is not a prefix of {pattern:?}" + ); + } + + /// The prefix never contains a glob metacharacter. + #[test] + fn prefix_is_free_of_metacharacters(pattern in pattern()) { + let prefix = literal_dir_prefix(&pattern); + prop_assert!( + !prefix.contains(METACHARACTERS), + "prefix {prefix:?} of {pattern:?} contains a metacharacter" + ); + } + + /// The prefix is `.` or a directory path ending at a separator. + #[test] + fn prefix_is_a_directory_path(pattern in pattern()) { + let prefix = literal_dir_prefix(&pattern); + prop_assert!( + prefix == "." || prefix.ends_with(MAIN_SEPARATOR), + "prefix {prefix:?} of {pattern:?} is not a directory path" + ); + } + + /// The prefix is the longest one available: what follows it holds no + /// further separator that is still free of metacharacters. + #[test] + fn prefix_is_maximal(pattern in pattern()) { + let prefix = literal_dir_prefix(&pattern); + let consumed = if prefix == "." { 0 } else { prefix.len() }; + let Some(rest) = pattern.get(consumed..) else { + return Err(TestCaseError::fail("prefix is not a character boundary")); + }; + let literal_rest = rest + .find(METACHARACTERS) + .map_or(Some(rest), |idx| rest.get(..idx)) + .ok_or_else(|| TestCaseError::fail("metacharacter is not a character boundary"))?; + prop_assert!( + !literal_rest.contains(MAIN_SEPARATOR), + "prefix {prefix:?} of {pattern:?} left a literal separator in {literal_rest:?}" + ); + } + + /// Any path under the prefix relativises to its remainder. + #[test] + fn matches_inside_the_prefix_relativise( + prefix in "[a-z]{1,4}(/[a-z]{1,4}){0,3}", + tail in "[a-z]{1,4}(/[a-z]{1,4}){0,3}", + ) { + let root = root_at(&prefix)?; + let matched = Utf8PathBuf::from(&prefix).join(&tail); + let relative = root + .relativise(&matched) + .map_err(|err| TestCaseError::fail(format!("{matched} should relativise: {err}")))?; + prop_assert_eq!(relative, Utf8Path::new(&tail)); + } + + /// The prefix itself relativises to the capability root. + #[test] + fn the_prefix_itself_relativises_to_the_root(prefix in "[a-z]{1,4}(/[a-z]{1,4}){0,3}") { + let root = root_at(&prefix)?; + let matched = Utf8PathBuf::from(&prefix); + let relative = root + .relativise(&matched) + .map_err(|err| TestCaseError::fail(format!("{matched} should relativise: {err}")))?; + prop_assert_eq!(relative, Utf8Path::new(".")); + } + + /// Any path outside the prefix is rejected rather than resolved through a + /// wider capability. + #[test] + fn matches_outside_the_prefix_are_rejected( + prefix in "[a-z]{1,4}(/[a-z]{1,4}){0,3}", + outside in "[a-z]{1,4}(/[a-z]{1,4}){0,3}", + ) { + prop_assume!(!Utf8Path::new(&outside).starts_with(&prefix)); + let root = root_at(&prefix)?; + let err = root + .relativise(Utf8Path::new(&outside)) + .err() + .ok_or_else(|| TestCaseError::fail(format!("{outside} escaped prefix {prefix}")))?; + prop_assert_eq!(err.kind(), std::io::ErrorKind::InvalidInput); + } +} diff --git a/src/manifest/glob/walk.rs b/src/manifest/glob/walk.rs index 7f5d635fa..23d4b41c2 100644 --- a/src/manifest/glob/walk.rs +++ b/src/manifest/glob/walk.rs @@ -1,21 +1,358 @@ //! Filesystem traversal helpers for glob expansion. -use super::{GlobEntryResult, GlobErrorContext, GlobErrorType, GlobPattern, create_glob_error}; -use camino::{Utf8Path, Utf8PathBuf}; +//! +//! Glob matching itself is performed by the `glob` crate, which walks the +//! filesystem ambiently. The metadata checks used to filter directories out +//! of the results, however, go through a capability-scoped +//! [`cap_std::fs::Dir`] handle. To honour least privilege, that handle is +//! opened at the pattern's longest literal directory prefix (for example +//! `src/` for `src/**/*.c`) rather than at the filesystem root, so the +//! capability covers only the subtree the pattern can actually match. A +//! symbolic link whose target escapes that subtree is therefore unreadable +//! through the capability; such a match is skipped rather than failing the +//! expansion. +//! +//! The prefix is pattern text, so it is unescaped before it meets the +//! filesystem: a directory the pattern names as `[*]x` is the directory `*x` +//! on disk. Matches keep whatever rooting the pattern had — absolute for an +//! absolute pattern, `../…` for a parent-relative one — so each is rebased +//! onto the prefix before its metadata lookup. + +use super::{ + GlobEntry, GlobEntryResult, GlobErrorContext, GlobErrorType, GlobPattern, create_glob_error, +}; +use camino::{Utf8Component, Utf8Path, Utf8PathBuf}; +use cap_primitives::fs::{FollowSymlinks, open_dir_nofollow, open_parent_dir, stat}; use cap_std::{ambient_authority, fs::Dir}; use minijinja::Error; +use std::io; -/// Open the ambient directory to use as the glob root. +/// Capability root for a glob expansion. /// -/// Returns the filesystem root for absolute patterns and the current working -/// directory for relative patterns. -pub(super) fn open_root_dir(pattern: &GlobPattern) -> std::io::Result { - let candidate = pattern.normalized(); - let path = Utf8Path::new(candidate); - if path.is_absolute() { - Dir::open_ambient_dir("/", ambient_authority()) +/// Couples the [`Dir`] handle opened at the pattern's literal prefix with +/// that prefix, so matched paths can be relativised before metadata lookups. +pub(super) struct GlobRoot { + dir: Dir, + prefix: Utf8PathBuf, +} + +impl GlobRoot { + #[cfg(test)] + pub(super) const fn new(dir: Dir, prefix: Utf8PathBuf) -> Self { + Self { dir, prefix } + } + + /// Directory the capability is scoped to. + #[cfg(test)] + pub(super) const fn dir(&self) -> &Dir { + &self.dir + } + + /// Literal pattern prefix the capability was opened at. + #[cfg(test)] + pub(super) fn prefix(&self) -> &Utf8Path { + self.prefix.as_path() + } + + /// Fetch metadata for a matched path via the capability-scoped handle. + /// + /// Returns `Ok(None)` only when the match is unresolvable in one of the two + /// ways a symbolic link makes it so: the link escapes the literal prefix, + /// which `cap_std` reports as [`io::ErrorKind::PermissionDenied`], or it + /// dangles, which surfaces as [`io::ErrorKind::NotFound`]. The link may be + /// the final component or an intermediate directory. Either way the match + /// names no file reachable within the capability, so it is skipped rather + /// than aborting the whole expansion. + /// + /// Every other failure propagates, including a symlink loop + /// ([`io::ErrorKind::FilesystemLoop`]): a cyclic link is a broken tree + /// rather than an absent file, and silently dropping it would hide the + /// breakage. A link whose target is genuinely unreadable inside the prefix + /// is skipped along with the escapes, because `cap_std` reports both as + /// `PermissionDenied` and the capability cannot tell them apart. + #[cfg(test)] + pub(super) fn metadata(&self, path: &Utf8Path) -> io::Result> { + self.metadata_relative(self.relativise(path)?) + } + + fn metadata_relative(&self, relative: &Utf8Path) -> io::Result> { + match self.dir.metadata(relative) { + Ok(metadata) => Ok(Some(metadata)), + Err(err) if is_unresolvable_link(&err) && self.traverses_symlink(relative) => Ok(None), + Err(err) => Err(err), + } + } + + /// Report whether any component of `relative` is a symbolic link. + /// + /// `symlink_metadata` does not follow its final component, so the match + /// itself can be settled in one lookup. That is the common case — a link + /// named directly by the pattern — and it is tried first so the ancestor + /// walk, which costs one lookup per directory, is only paid for when an + /// intermediate component is the culprit. + fn traverses_symlink(&self, relative: &Utf8Path) -> bool { + if self + .dir + .symlink_metadata(relative) + .is_ok_and(|link| link.is_symlink()) + { + return true; + } + self.ancestor_is_symlink(relative) + } + + /// Report whether a directory on the way to `relative` is a symbolic link. + /// + /// Walks from the capability root outwards, inspecting each ancestor + /// without following it. The final component is skipped because + /// [`Self::traverses_symlink`] has already settled it. + fn ancestor_is_symlink(&self, relative: &Utf8Path) -> bool { + let mut ancestor = Utf8PathBuf::new(); + let mut components = relative.components(); + components.next_back(); + for component in components { + ancestor.push(component); + match self.dir.symlink_metadata(&ancestor) { + Ok(metadata) if metadata.is_symlink() => return true, + Ok(_) => {} + // This ancestor cannot be inspected at all, so no link has + // been found and the caller's error stands. + Err(_) => return false, + } + } + false + } + + /// Rebase a matched path onto the capability prefix. + pub(super) fn relativise<'a>(&self, path: &'a Utf8Path) -> io::Result<&'a Utf8Path> { + let relative = if self.prefix == "." { + path + } else { + path.strip_prefix(&self.prefix).map_err(|_| { + io::Error::new( + io::ErrorKind::InvalidInput, + format!( + "glob match {path} does not start with capability prefix {}", + self.prefix + ), + ) + })? + }; + Ok(if relative.as_str().is_empty() { + Utf8Path::new(".") + } else { + relative + }) + } +} + +/// Report whether `err` is how a link the capability cannot follow surfaces. +/// +/// `cap_std` raises [`io::ErrorKind::PermissionDenied`] for a resolution that +/// leaves the capability's tree, and the platform raises +/// [`io::ErrorKind::NotFound`] for a link with no target. Nothing else counts: +/// a loop, an I/O failure, or an invalid argument all describe a tree that is +/// broken rather than a match that is simply not there. +fn is_unresolvable_link(err: &io::Error) -> bool { + matches!( + err.kind(), + io::ErrorKind::PermissionDenied | io::ErrorKind::NotFound + ) +} + +/// Characters [`super::normalize::force_literal_escapes`] wraps in a bracket +/// class to force a literal match. +const LITERAL_ESCAPES: [char; 6] = ['[', ']', '*', '?', '{', '}']; + +/// The character a bracketed literal escape at the start of `rest` stands for, +/// paired with the escape's byte length. +/// +/// Recognises exactly the six forms `force_literal_escapes` emits — `[*]`, +/// `[?]`, `[[]`, `[]]`, `[{]`, `[}]` — and nothing else. A genuine character +/// class such as `[ab]` is a wildcard and yields `None`, as does a +/// single-character class like `[a]`, which is treated conservatively even +/// though it matches only one character. +fn literal_escape(rest: &str) -> Option<(char, usize)> { + let mut chars = rest.chars(); + if chars.next()? != '[' { + return None; + } + let escaped = chars.next()?; + if chars.next()? != ']' || !LITERAL_ESCAPES.contains(&escaped) { + return None; + } + Some(( + escaped, + '['.len_utf8() + escaped.len_utf8() + ']'.len_utf8(), + )) +} + +/// Byte offset of the first character that makes `normalized` a wildcard. +/// +/// A bracketed literal escape is a literal character rather than a +/// metacharacter, so the scan steps over it instead of stopping there. +fn first_metacharacter(normalized: &str) -> usize { + let mut idx = 0; + while let Some(rest) = normalized.get(idx..) { + let Some(next) = rest.chars().next() else { + break; + }; + if next == '[' { + match literal_escape(rest) { + Some((_, len)) => idx += len, + None => return idx, + } + } else if matches!(next, '*' | '?' | '{') { + return idx; + } else { + idx += next.len_utf8(); + } + } + normalized.len() +} + +/// Longest literal directory prefix of a normalised pattern. +/// +/// Scans up to the first glob metacharacter (`*`, `?`, `[`, `{`) and trims +/// back to the last path separator, yielding the deepest directory that the +/// pattern names literally. Bracketed literal escapes are stepped over, so +/// `src/[*]x/generated/*.c` reaches `src/[*]x/generated/` rather than stopping +/// at `src/`. Returns `.` when the pattern has no literal directory component. +/// +/// The result is still pattern text: [`unescape_literal_escapes`] turns it +/// into the filesystem path it names. +pub(super) fn literal_dir_prefix(normalized: &str) -> &str { + let meta_idx = first_metacharacter(normalized); + let literal = normalized.get(..meta_idx).unwrap_or_default(); + // Keep the trailing separator so absolute roots stay absolute ("/"). + literal + .rfind(std::path::MAIN_SEPARATOR) + .and_then(|idx| literal.get(..=idx)) + .unwrap_or(".") +} + +/// Resolve bracketed literal escapes to the characters they stand for. +/// +/// The prefix names a directory to open, so `src/[*]x/` has to become the +/// path `src/*x/` before it reaches the filesystem — and before a match is +/// stripped of it, since the walker yields real paths. +pub(super) fn unescape_literal_escapes(prefix: &str) -> String { + let mut out = String::with_capacity(prefix.len()); + let mut idx = 0; + while let Some(rest) = prefix.get(idx..) { + let Some(next) = rest.chars().next() else { + break; + }; + if let Some((escaped, len)) = literal_escape(rest) { + out.push(escaped); + idx += len; + } else { + out.push(next); + idx += next.len_utf8(); + } + } + out +} + +/// Open the directory used as the capability root for the glob. +/// +/// Returns `Ok(None)` when the literal prefix does not exist (or is not a +/// directory); the pattern can match nothing in that case, mirroring the +/// empty result the matcher would produce. +pub(super) fn open_root_dir(pattern: &GlobPattern) -> io::Result> { + let prefix = literal_dir_path(pattern); + match open_literal_prefix(Utf8Path::new(&prefix)) { + Ok(dir) => Ok(Some(GlobRoot { + dir, + prefix: Utf8PathBuf::from(prefix), + })), + Err(err) if prefix_is_unopenable(&err) => Ok(None), + Err(err) => Err(err), + } +} + +/// Open `prefix` without following a symbolic link in its literal components. +/// +/// The only ambient opening establishes the lexical filesystem root for an +/// absolute prefix, or the current directory for a relative one. Subsequent +/// normal components use `open_dir_nofollow`, while `..` deliberately moves +/// through the parent-directory capability so parent-relative patterns retain +/// their existing behaviour. +fn open_literal_prefix(prefix: &Utf8Path) -> io::Result { + let (base, remainder) = if prefix.is_absolute() { + let root = prefix.ancestors().last().ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidInput, + "an absolute glob prefix must have a filesystem root", + ) + })?; + let remainder = prefix.strip_prefix(root).map_err(|_| { + io::Error::new( + io::ErrorKind::InvalidInput, + "an absolute glob prefix must start with its filesystem root", + ) + })?; + (root, remainder) } else { - Dir::open_ambient_dir(".", ambient_authority()) + (Utf8Path::new("."), prefix) + }; + let mut dir = Dir::open_ambient_dir(base, ambient_authority())?.into_std_file(); + + for component in remainder.components() { + dir = match component { + Utf8Component::CurDir => dir, + Utf8Component::ParentDir => open_parent_dir(&dir, ambient_authority())?, + Utf8Component::Normal(name) => { + // `open_dir_nofollow` alone accepts directory symlinks with + // the pinned capability implementation, so inspect the + // component before opening it. + if stat(&dir, name.as_ref(), FollowSymlinks::No)?.is_symlink() { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "a glob literal prefix cannot traverse a symbolic link", + )); + } + open_dir_nofollow(&dir, name.as_ref())? + } + Utf8Component::Prefix(_) | Utf8Component::RootDir => { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "a glob prefix remainder cannot contain a filesystem root", + )); + } + }; } + + Ok(Dir::from_std_file(dir)) +} + +/// Return the filesystem path represented by a pattern's literal prefix. +fn literal_dir_path(pattern: &GlobPattern) -> String { + unescape_literal_escapes(literal_dir_prefix(pattern.normalized())) +} + +/// Report whether `err` means the literal prefix names no usable directory. +/// +/// A missing path and a non-directory path both mean the pattern can match +/// nothing. On Windows `cap_primitives` signals the latter by constructing a +/// raw `ERROR_DIRECTORY`, so the raw code is matched alongside the portable +/// [`io::ErrorKind`]s rather than relying on the standard library's mapping of +/// that code. Every other failure — a genuine permission error, say — is left +/// to propagate. +fn prefix_is_unopenable(err: &io::Error) -> bool { + if matches!( + err.kind(), + io::ErrorKind::NotFound | io::ErrorKind::NotADirectory + ) { + return true; + } + #[cfg(windows)] + { + /// `ERROR_DIRECTORY`: the path is not a directory. + const ERROR_DIRECTORY: i32 = 267; + return err.raw_os_error() == Some(ERROR_DIRECTORY); + } + #[cfg(not(windows))] + false } fn create_io_error(pattern: &GlobPattern, position: usize, detail: String) -> Error { @@ -35,37 +372,29 @@ fn create_io_error(pattern: &GlobPattern, position: usize, detail: String) -> Er pub(super) fn process_glob_entry( entry: GlobEntryResult, pattern: &GlobPattern, - root: &Dir, -) -> std::result::Result, Error> { - match entry { - Ok(path) => { - let utf_path = Utf8PathBuf::try_from(path).map_err(|_| { - create_io_error( - pattern, - pattern.raw().len(), - "glob matched a non-UTF-8 path".to_owned(), - ) - })?; - let metadata = fetch_metadata(root, &utf_path) - .map_err(|err| create_io_error(pattern, pattern.raw().len(), err.to_string()))?; - if !metadata.is_file() { - return Ok(None); - } - Ok(Some(utf_path.as_str().replace('\\', "/"))) - } - Err(e) => Err(create_io_error(pattern, 0, e.to_string())), - } + root: &GlobRoot, +) -> std::result::Result { + let path = entry.map_err(|e| create_io_error(pattern, 0, e.to_string()))?; + let utf_path = Utf8PathBuf::try_from(path).map_err(|_| { + create_io_error( + pattern, + pattern.raw().len(), + "glob matched a non-UTF-8 path".to_owned(), + ) + })?; + names_a_file(root, &utf_path) + .map_err(|err| create_io_error(pattern, pattern.raw().len(), err.to_string())) } -fn fetch_metadata(root: &Dir, path: &Utf8Path) -> std::io::Result { - if path.is_absolute() { - let stripped = path.as_str().trim_start_matches(['/', '\\']); - if stripped.is_empty() { - root.metadata(Utf8Path::new(".")) - } else { - root.metadata(stripped) - } - } else { - root.metadata(path) +/// Classify whether a match names a regular file reachable through the +/// capability, returning a bounded reason when it does not. +fn names_a_file(root: &GlobRoot, path: &Utf8Path) -> io::Result { + let relative = root.relativise(path)?; + let Some(metadata) = root.metadata_relative(relative)? else { + return Ok(GlobEntry::UnreachableSymlink(relative.to_path_buf())); + }; + if metadata.is_file() { + return Ok(GlobEntry::Path(path.as_str().replace('\\', "/"))); } + Ok(GlobEntry::NotAFile) } diff --git a/src/manifest/mod.rs b/src/manifest/mod.rs index 32bc1a894..bad26f40a 100644 --- a/src/manifest/mod.rs +++ b/src/manifest/mod.rs @@ -128,7 +128,11 @@ fn from_str_named( jinja.add_function("env", move |var_name: String| { env_var_with(&var_name, |key| reader(key)) }); - jinja.add_function("glob", |pattern: String| glob_paths(&pattern)); + jinja.add_function("glob", |pattern: String| { + let expansion = glob::expand_glob(&pattern)?; + glob::record_expansion(&expansion); + Ok(expansion.into_paths()) + }); let _stdlib_state = match stdlib_config { Some(config) => crate::stdlib::register_with_config(&mut jinja, config), None => crate::stdlib::register(&mut jinja), diff --git a/src/manifest/tests/glob_telemetry.rs b/src/manifest/tests/glob_telemetry.rs new file mode 100644 index 000000000..c56a18293 --- /dev/null +++ b/src/manifest/tests/glob_telemetry.rs @@ -0,0 +1,88 @@ +//! Telemetry coverage for the manifest Jinja `glob()` adapter. +//! +//! This test reaches `from_str` rather than glob's private query and recorder, +//! pinning that the adapter records the bounded observations it receives. + +use super::super::from_str; +use crate::test_tracing_capture::with_test_subscriber; +use anyhow::{Context, Result, ensure}; +use metrics::SharedString; +use metrics_util::{ + CompositeKey, MetricKind, + debugging::{DebugValue, DebuggingRecorder}, +}; +use rstest::rstest; +use tempfile::tempdir; +use test_support::manifest::manifest_yaml; +use tracing::level_filters::LevelFilter; + +const EXPANSIONS_TOTAL: &str = "netsuke_manifest_glob_expansions_total"; + +type Snapshot = Vec<( + CompositeKey, + Option, + Option, + DebugValue, +)>; + +/// Run `parse` with local metrics and tracing capture. +fn recorded(parse: impl FnOnce() -> T) -> (T, Vec, Snapshot) { + let recorder = DebuggingRecorder::new(); + let snapshotter = recorder.snapshotter(); + let (value, events) = metrics::with_local_recorder(&recorder, || { + with_test_subscriber(LevelFilter::DEBUG, |captured| { + let value = parse(); + (value, captured.snapshot()) + }) + }); + (value, events, snapshotter.snapshot().into_vec()) +} + +/// Value of the glob-expansion counter labelled with `outcome`. +fn expansion_count(snapshot: &Snapshot, outcome: &str) -> Option { + snapshot.iter().find_map(|(key, _, _, value)| { + if key.kind() != MetricKind::Counter || key.key().name() != EXPANSIONS_TOTAL { + return None; + } + let has_outcome = key + .key() + .labels() + .any(|label| label.key() == "outcome" && label.value() == outcome); + match value { + DebugValue::Counter(count) if has_outcome => Some(*count), + _ => None, + } + }) +} + +#[rstest] +fn jinja_glob_adapter_records_an_unopenable_prefix() -> Result<()> { + let temp = tempdir()?; + let pattern = format!("{}/missing/*.txt", temp.path().display()); + let yaml = manifest_yaml(&format!( + "targets:\n - foreach: glob('{pattern}')\n name: no-match\n command: echo hi\n" + )); + + let (manifest, events, snapshot) = recorded(|| from_str(&yaml)); + ensure!( + manifest?.targets.is_empty(), + "the missing prefix has no matches" + ); + ensure!( + expansion_count(&snapshot, "unopenable_prefix") == Some(1), + "the Jinja adapter must record the unopenable prefix: {snapshot:?}" + ); + let event = events + .iter() + .find(|event| event.contains("glob literal prefix names no directory")) + .context("the Jinja adapter must emit its trace event")?; + ensure!( + event.contains("pattern=\"\"") && event.contains("prefix=\"\""), + "the adapter event must retain bounded fields: {event}" + ); + ensure!( + !event.contains(&temp.path().display().to_string()), + "the adapter event must not disclose its absolute path: {event}" + ); + Ok(()) +} diff --git a/src/manifest/tests/mod.rs b/src/manifest/tests/mod.rs index b2790fd7a..ceb5ac540 100644 --- a/src/manifest/tests/mod.rs +++ b/src/manifest/tests/mod.rs @@ -9,3 +9,4 @@ mod vars_reserved_property; mod workspace; mod env_function; +mod glob_telemetry; diff --git a/tests/manifest_glob_tests.rs b/tests/manifest_glob_tests.rs index a576212d6..486610880 100644 --- a/tests/manifest_glob_tests.rs +++ b/tests/manifest_glob_tests.rs @@ -345,3 +345,6 @@ fn glob_is_case_sensitive_on_windows(temp_dir: tempfile::TempDir) -> Result<()> ensure!(manifest.targets.is_empty()); Ok(()) } + +#[path = "manifest_glob_tests/capability_scope.rs"] +mod capability_scope; diff --git a/tests/manifest_glob_tests/capability_scope.rs b/tests/manifest_glob_tests/capability_scope.rs new file mode 100644 index 000000000..a32f3d506 --- /dev/null +++ b/tests/manifest_glob_tests/capability_scope.rs @@ -0,0 +1,98 @@ +//! Manifest-level coverage of the capability-scoped expansion. +//! +//! The unit tests drive `glob_paths` directly. These go through the +//! manifest and its Jinja rendering, so they also pin that a skipped or +//! empty expansion reaches `foreach` as an ordinary list rather than +//! surfacing as a parse error. + +use super::{manifest_yaml, target_names, temp_dir}; +use anyhow::{Context, Result, ensure}; +use rstest::rstest; +use test_support::{cwd_guard::CwdGuard, env_lock::EnvLock}; + +/// Build a manifest with one `foreach` target per glob match. +fn glob_manifest(pattern: &str) -> String { + manifest_yaml(&format!( + concat!( + "targets:\n", + " - foreach: glob('{pattern}')\n", + " name: \"{{{{ item }}}}\"\n", + " command: echo hi\n", + ), + pattern = pattern, + )) +} + +/// A prefix that names nothing, and one that names a file, both expand to +/// no targets rather than failing the manifest. +#[rstest] +#[case::missing_directory("no-such-dir")] +#[case::regular_file("a.txt")] +fn unopenable_prefix_yields_no_targets( + temp_dir: tempfile::TempDir, + #[case] prefix: &str, +) -> Result<()> { + test_support::fs::write(temp_dir.path().join("a.txt"), "a")?; + let pattern = format!("{}/{prefix}/*.txt", temp_dir.path().display()); + let manifest = netsuke::manifest::from_str(&glob_manifest(&pattern)) + .context("an unopenable prefix should parse, not fail")?; + ensure!( + manifest.targets.is_empty(), + "expected no targets, got {:?}", + manifest.targets.len() + ); + Ok(()) +} + +/// A parent-relative pattern expands against the working directory. +#[rstest] +fn parent_relative_pattern_expands(temp_dir: tempfile::TempDir) -> Result<()> { + let sub = temp_dir.path().join("sub"); + test_support::fs::create_dir(&sub)?; + test_support::fs::write(temp_dir.path().join("out.txt"), "out")?; + + let _lock = EnvLock::acquire(); + let _guard = CwdGuard::acquire()?; + std::env::set_current_dir(&sub).context("switch to the subdirectory")?; + + let manifest = netsuke::manifest::from_str(&glob_manifest("../*.txt"))?; + ensure!( + target_names(&manifest)? == vec!["../out.txt".to_owned()], + "expected the parent-relative match" + ); + Ok(()) +} + +/// A symbolic link that escapes the literal prefix, and one that dangles, +/// are both dropped from the expansion without failing the manifest. +#[cfg(unix)] +#[rstest] +#[case::escaping("../vendor/escaped.txt")] +#[case::dangling("nowhere.txt")] +fn unresolvable_symlinks_are_skipped( + temp_dir: tempfile::TempDir, + #[case] link_target: &str, +) -> Result<()> { + let src = temp_dir.path().join("src"); + let vendor = temp_dir.path().join("vendor"); + test_support::fs::create_dir(&src)?; + test_support::fs::create_dir(&vendor)?; + test_support::fs::write(vendor.join("escaped.txt"), "escaped")?; + test_support::fs::write(src.join("real.txt"), "real")?; + test_support::fs::symlink(link_target, src.join("linked.txt"))?; + + let pattern = format!("{}/src/*.txt", temp_dir.path().display()); + let manifest = netsuke::manifest::from_str(&glob_manifest(&pattern)) + .context("an unresolvable link should not fail the manifest")?; + let names = target_names(&manifest)?; + ensure!( + names.iter().any(|n| n.ends_with("/src/real.txt")), + "the resolvable match should survive: {names:?}" + ); + ensure!( + names.iter().all(|n| !n.ends_with("linked.txt")), + "the unresolvable link should be skipped: {names:?}" + ); + ensure!(names.len() == 1, "expected exactly one target: {names:?}"); + Ok(()) +} diff --git a/typos.local.toml b/typos.local.toml index 424b465b4..fea64c0be 100644 --- a/typos.local.toml +++ b/typos.local.toml @@ -6,7 +6,28 @@ schema = 1 stems = [] [words] -accepted = [] +accepted = [ + "relativisable", + "relativisably", + "relativisation", + "relativisations", + "relativise", + "relativised", + "relativiser", + "relativisers", + "relativises", + "relativising", + "relativizable", + "relativizably", + "relativization", + "relativizations", + "relativize", + "relativized", + "relativizer", + "relativizers", + "relativizes", + "relativizing", +] [words.corrections] diff --git a/typos.toml b/typos.toml index 864c49969..8f776003d 100644 --- a/typos.toml +++ b/typos.toml @@ -2185,6 +2185,26 @@ extend-ignore-re = [ "reinitializers" = "reinitializers" "reinitializes" = "reinitializes" "reinitializing" = "reinitializing" +"relativisable" = "relativisable" +"relativisably" = "relativisably" +"relativisation" = "relativisation" +"relativisations" = "relativisations" +"relativise" = "relativise" +"relativised" = "relativised" +"relativiser" = "relativiser" +"relativisers" = "relativisers" +"relativises" = "relativises" +"relativising" = "relativising" +"relativizable" = "relativizable" +"relativizably" = "relativizably" +"relativization" = "relativization" +"relativizations" = "relativizations" +"relativize" = "relativize" +"relativized" = "relativized" +"relativizer" = "relativizer" +"relativizers" = "relativizers" +"relativizes" = "relativizes" +"relativizing" = "relativizing" "reorganisable" = "reorganizable" "reorganisably" = "reorganizably" "reorganisation" = "reorganization"