fix(scanner): resolve scan roots to absolute paths - #105
Merged
Conversation
The rules are written against absolute paths, and the ones anchored at the filesystem root are anchored deliberately: `/tmp/**` stops there so it cannot reach `~/tmp/tax-return.pdf`, which is what #41 was about. jwalk builds every entry's path from the root as it was handed in, so `diskern scan tmp` from `/var` produced `tmp/systemd-private/x` and no anchored pattern could match it. The walk found the files and the rules could not tell where they were, so the scan printed "Scanned 1 files" and then nothing to clean — a plausible empty result for a directory full of matches, which is the #82 failure arriving by another route. absolute() rather than canonicalize(): no filesystem access, works on a path that does not exist, and leaves symlinks alone. canonicalize would rewrite /var/tmp to /private/var/tmp on macOS and scan somewhere other than what was asked for. It leaves `..` in place, because a/../b is only b when a is not a symlink; that limit is written down beside the code. Every path the scanner yields is now absolute, which also settles what `actions::quarantine` records as a file's original location — a relative original could not be restored from a different directory.
Changing the process working directory inside a test would race every other test in the same binary, so this drives the installed binary with `current_dir` instead and asks for `.` as the root. The rule it uses is anchored at the filesystem root, like the shipped `/tmp/**`, because an unanchored pattern matches either way and would pass with the bug still present. Verified against the unfixed scanner: the relative case fails, the absolute control passes. Unix only. Windows paths normalize to `c:/...`, so a `/`-anchored pattern cannot match there and neither can the bug.
First entry under Unreleased since v0.2.0 shipped.
`/var/tmp` is not an absolute path on Windows. It is rooted but carries no drive, so `absolute` resolves it against the current one and returns `D:\var\tmp` — correct behaviour, and what the Windows CI job caught. The guarantee being tested is that a root the rules already understand survives untouched, so each platform states it with a path that is actually absolute there.
Every item carried `#[cfg(unix)]` but the imports did not, so on Windows all four were unused. CI never saw it: the lint job runs clippy on ubuntu, and the Windows test job treats unused imports as warnings. A Windows contributor running the clippy line CONTRIBUTING asks for would have got four errors out of a file they had not touched.
Making roots absolute brought them into range of the exclude list, which is right — `is_within` matches on whole components, so `/run` covers `/run/user/1000/cache`, and that is what the list is for. But the walk then dropped every entry and the report came back empty, which reads as a clean disk. `$XDG_RUNTIME_DIR` lives under `/run` and does hold caches, so this is reachable rather than theoretical. An empty report that means "I refused to look here" is the same wrong answer as #82 and #103: plausible, silent, and indistinguishable from the real thing. It names the exclude instead. The exclude list is normalized once for the whole scan now rather than per root, since the root check and the walk both need it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #103.
What & why
Rules anchored at the filesystem root —
/tmp/**,/var/log/**— areanchored deliberately: that is what keeps
/tmpout of~/tmp/tax-return.pdf(#41). Butjwalkbuilds every entry's path fromthe root as it was handed in, so a relative root produced entries no
anchored pattern could match:
The walk found the file; the rules could not tell where it was. That is
a plausible-looking empty result rather than an error — the #82 failure
arriving by a different route, and worse under
--json, where nothingsays the paths were relative.
Roots are now resolved in
scanner::scan, so the CLI and the desktopapp both get it.
absolute(), notcanonicalize(). No filesystem access, works on apath that does not exist, and leaves symlinks alone —
canonicalizewould rewrite
/var/tmpto/private/var/tmpon macOS and scansomewhere other than what was asked for. It leaves
..in place,because
a/../bis onlybwhenais not a symlink; that limit isdocumented beside the code rather than papered over.
Side effect worth naming: every path the scanner yields is now
absolute, which settles the open question in #103 about
actions::quarantinerecording a file's original location. A relativeoriginal could not have been restored from a different working
directory.
Tests
scanner.rs:absolute_rootresolves a relative root and leaves anabsolute one untouched, including
/var/tmp.crates/diskern-cli/tests/relative_root.rs: drives the real binarywith
current_dirand.as the root, using a root-anchored rule.Changing the process working directory inside a test would race the
other tests in the same binary, so this uses a child process instead.
Unix only — a
/-anchored pattern cannot matchc:/...on Windows,so neither can the bug.
Verified against the unfixed scanner: the relative case fails, the
absolute control passes. A regression test that passes without the fix
is not one.
Validation
cargo fmt --allcargo clippy -p diskern-core -p diskern-cli --all-targets -- -D warningscargo test -p diskern-core -p diskern-cli— 70 pass, 0 failChecklist
cargo fmt --alland the required core/CLI clippy validation are cleanUpdate. Two follow-ups from a self-review, each its own commit:
#[cfg(unix)]but not itsimports, so on Windows all four were unused. CI missed it — clippy
runs on ubuntu, and the Windows test job treats them as warnings —
but a Windows contributor running the clippy line CONTRIBUTING asks
for would have got four errors from a file they had not touched. The
file is gated at module level now.
which is correct, but the walk then dropped every entry and returned
an empty report.
$XDG_RUNTIME_DIRlives under the/runexcludeand holds real caches, so a scan there came back looking like a clean
disk. That is the same wrong answer as diskern scan reports a clean, empty result for a path that doesn't exist #82 and Anchored rules never match when the scan root is relative #103, so the scan now
names the exclude instead of reporting nothing.