[WSLC] Alias cannonicalization follow-up in denied-path overlap validator

[Soham Das (SohamDas2021)]

📖 Description

Follow-up to #657 , addressing the following:

1. Tier-2 canonical-equality miss ( policy_mapping.rs ): the overlap validator now rejects a deny that canonicalizes exactly onto a mount root via an on-disk alias (which  contains_strictly  missed and D6 saw as absent), but only when the original strings differ lexically — so a literal exact match stays enforceable-by-not-mounting.
2. Dangling reparse point misclassified as  Absent  ( filesystem_canonical.rs ): a dangling junction/symlink returned FILE/PATH_NOT_FOUND and was labelled  Absent ; it now re-probes with  FILE_FLAG_OPEN_REPARSE_POINT  and returns  Unknown  (fail closed) when the link object itself exists.
3. Hard-link doc correction ( policy_mapping.rs ): fixed the comment to state hard links are names for one object and D6 groups hard-linked policy entries by file ID — the only residual gap is an unlisted hard link inside a mounted subtree.

🔗 References

🔍 Validation

✅ Checklist

• Signed the Contributor License Agreement
• Linked to an issue
• Updated documentation (if applicable)
• Updated Copilot instructions (if build, architecture, or conventions changed)
• If this PR changes Cargo.lock, the dependency-feed-check check passes (see docs/pull-requests.md)

📋 Issue Type

• Bug fix
• Feature
• Task

---

GitHub Actions runs the PR validation build automatically. The ADO pipeline
(MXC-PR-Build) is the Azure version of the PR pipeline, kept in parity with the GitHub
Actions build; it runs on merge to main, and Microsoft reviewers with write access can trigger it
on a PR with /azp run. See docs/pull-requests.md.

If the dependency-feed-check check fails on a new dependency, the crate must be added to
the feed before the PR can pass. See docs/pull-requests.md
for the steps.

Microsoft Reviewers: Open in CodeFlow

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

Strengthens WSLC denied-path alias validation and dangling reparse-point handling.

Changes:

• Detects canonical equality between denied paths and mount roots.
• Re-probes dangling reparse points.
• Corrects hard-link documentation and adds regression tests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/core/wxc_common/src/filesystem_canonical.rs	Handles dangling reparse points during canonicalization.
src/backends/wslc/common/src/policy_mapping.rs	Extends overlap validation and updates alias documentation.

[Gudge (MGudgin)]

The reparse re-probe fails open on any outcome that isn't a clean success or a true "not found", and the downstream effect is worse than a skipped check — it manufactures a confidently wrong canonical path.

The wildcard arm collapses every non-Ok(valid) result into Absent:

return match reparse {
    Ok(h) if !h.is_invalid() => { let _guard = OwnedHandle(h); PathCanonical::Unknown }
    // Nothing here at all (or a genuinely missing parent): absent.
    _ => PathCanonical::Absent,
};

_ also absorbs Err(ERROR_ACCESS_DENIED) (a DACL on the link object denying FILE_READ_ATTRIBUTES), Err(ERROR_SHARING_VIOLATION) (a holder with FILE_SHARE_NONE), filter/I-O errors, and Ok(invalid_handle). In each of those the link object may well exist — we simply couldn't examine it.

That directly contradicts the documented contract on the type being returned (filesystem_canonical.rs:27-31):

/// Cleanly missing — no object exists, so there is nothing to alias.
Absent,
/// Present (or maybe present) but unresolvable: access denied, I/O error, or
/// an unsupported build target. Callers fail closed on this when denies apply.
Unknown,

"Access denied" is enumerated under Unknown by name. It's also inconsistent with this function's own first CreateFileW branch 30 lines above, which correctly routes every non-NOT_FOUND code to Unknown.

Why this is a deny bypass rather than a conservative skip. The consumer is canonicalize_allowing_absent_tail (filesystem_canonical.rs:135), whose ancestor walk (:153-173) treats Absent as "keep climbing" and only bails out on Unknown (:172). Take a deny of C:\link\file where C:\link is a junction into a mounted tree whose target is missing and whose link object can't be opened:

1. canonicalize_path("C:\link\file") → follow-open hits the dangling junction → NOT_FOUND → probe → Absent.
2. Walk to C:\link. Follow-open → NOT_FOUND. Probe fails ACCESS_DENIED → Absent (should be Unknown).
3. Because it's Absent, the walk continues to C:\, which resolves Canonical.
4. The tail is replayed onto it (:158-170), returning Canonical("C:\link\file").

So the validator doesn't skip anything — it receives a fully-confident canonical path that still contains the unresolved junction, compares that literal spelling against the mount list, finds no overlap, and accepts the config. The junction's actual target inside the mounted subtree is never considered. Note Absent from the whole function only happens when no ancestor resolves; since the drive root essentially always resolves, the replay path above is the common case, not the corner case.

This is precisely the outcome the comment on the probe is written to prevent — "classify it Unknown so denied-path callers fail closed rather than replay a stale alias." The wildcard defeats that intent in exactly the situation it describes.

Fix — only a genuine not-found is Absent; everything else is Unknown:

return match reparse {
    // The link object itself exists: present but unresolvable → fail closed.
    Ok(h) if !h.is_invalid() => {
        let _guard = OwnedHandle(h);
        PathCanonical::Unknown
    }
    // Success with an invalid handle: no error to read, so unexaminable.
    Ok(_) => PathCanonical::Unknown,
    Err(e) => {
        let code = e.code();
        if code == HRESULT::from_win32(ERROR_FILE_NOT_FOUND.0)
            || code == HRESULT::from_win32(ERROR_PATH_NOT_FOUND.0)
        {
            // Nothing here at all (or a genuinely missing parent): absent.
            PathCanonical::Absent
        } else {
            // Present but unexaminable (access denied, sharing violation, I/O) → fail closed.
            PathCanonical::Unknown
        }
    }
};

Once that's in, canonicalize_allowing_absent_tail:172 propagates the Unknown and the walk correctly stops at the junction.

For coverage, the higher-value regression test is at the canonicalize_allowing_absent_tail level rather than on canonicalize_path alone — a dangling junction ancestor with an inaccessible link object must not yield Canonical(<path containing the junction>). That exercises the whole chain, which is where the security property actually lives. If the ACL fixture proves awkward against real Win32, factoring the HRESULT + reparse-existence → PathCanonical classification behind a small injectable seam (like the resolve closure already used in policy_mapping.rs) would make all four arms unit-testable without touching disk.

[Gudge (MGudgin)]

The rationale comment on the new equality guard credits the wrong layer, which makes a load-bearing invariant easy to break.

// Exact canonical equality is an overlap only when it comes from an
// on-disk alias (the two original strings differ lexically). A
// literal/case/separator/`..`-variant deny == mount is the
// enforceable exact-match case (collapsed most-restrictive-wins at
// parse time), so accept it.
let mount_lexical = NormalizedPath::parse(mounted);
if mount_norm == denied_norm && mount_lexical != denied_lexical {

Two mismatches with the code:

1. "the two original strings differ lexically" — the guard doesn't compare original strings. It compares NormalizedPath::parse(mounted) against NormalizedPath::parse(denied), i.e. lexically normalized forms. That distinction is doing all the work here.
2. "collapsed most-restrictive-wins at parse time" — normalize_filesystem_paths (config_parser.rs:378-412) collapses via HashSet<String> + denied.contains(p), which is byte-identical string equality. It does not fold case, separators, or ... So C:\Project vs c:/project sails straight through parse time; what actually accepts it is NormalizedPath::parse right here (policy_mapping.rs:138-174 — to_lowercase(), split(['/', '\\']), and ./.. folding).

Worth noting the pre-existing comment on contains_strictly just above (:185-191) states this correctly and narrowly:

an exact same-string deny==mount is already collapsed most-restrictive-wins at parse time (normalize_filesystem_paths)

That's accurate — normalize_filesystem_paths genuinely does handle the same-string case. The new comment reuses that same parenthetical but widens the claim to "literal/case/separator/..-variant", which is the part that isn't true of that layer. So this is a small accuracy regression against a neighbouring comment that already had it right.

Why it matters beyond wording. The behavior is correct today; the risk is the next editor. Someone reading "original strings differ lexically" could reasonably tighten this to the literal reading

if mount_norm == denied_norm && mounted != denied {

which would then reject C:\Project (mount) vs c:/project (deny) as alias-induced overlap — turning an enforceable exact-match config into a hard validation failure. Someone reading "collapsed at parse time" could equally conclude the NormalizedPath::parse calls here are redundant and drop them, with the same outcome. Neither change would fail a test today (see below).

Suggested rewrite:

// Exact canonical equality is an overlap only when it comes from an on-disk
// alias — i.e. the two spellings normalize to *different lexical* forms yet
// resolve to the same object. A case/separator/`..` variant of the same
// spelling folds to an equal `NormalizedPath` here and is the enforceable
// exact-match case (enforced by simply not mounting the path), so accept it.
// That fold is done by `NormalizedPath::parse` below, NOT by
// `normalize_filesystem_paths`, which only dedupes byte-identical strings.
// But a deny that aliases onto the mount root via a symlink, junction, or
// short name targets the mounted object with no exclusion primitive —
// `contains_strictly` (strictly-deeper only) misses that, so reject it here.

One edge case the imprecise wording hides, worth confirming is intended: the comment implies every "literal variant" is accepted, but parse doesn't fold the extended-length prefix. \\?\C:\project yields { drive: None, rooted: true, components: ["?", "c:", "project"] } while C:\project yields { drive: Some("c:"), rooted: true, components: ["project"] } — lexically different. Since canonicalize_path does collapse \\?\ prefixes (filesystem_canonical.rs:24-26), a deny written as \\?\C:\project against a mount of C:\project is canonical-equal + lexically-different, so it gets rejected as an alias — even though it's a pure spelling variant with no on-disk indirection. Failing closed is the safe direction, so this isn't urgent, but the user-facing "cannot be enforced" error would be misleading. Either fold the prefix in parse or narrow the comment's claimed accept-set.

Finally, the accept side isn't pinned by any test — the only acceptance case uses byte-identical strings, so the normalization fold this comment describes is entirely untested. Worth adding C:\Project vs c:/project and C:\project vs C:\x\..\project, both with canonical-equal resolver results, asserting the config is accepted.

[Gudge (MGudgin)]

The only test covering the new reparse path can report success without ever running.

if !status.success() {
    eprintln!("skipping dangling_reparse_point_is_unknown_not_absent: mklink /J failed");
    let _ = std::fs::remove_dir_all(&base);
    return;
}

A bare return is indistinguishable from a pass to libtest — the run prints test dangling_reparse_point_is_unknown_not_absent ... ok and the eprintln! is swallowed unless someone passes --nocapture. So if junction creation ever fails, CI stays green while the FILE_FLAG_OPEN_REPARSE_POINT branch this PR adds is never exercised.

Three reasons that's worth tightening rather than leaving as defensive tolerance:

1. mklink /J needs no privilege. Junctions only require write access to the containing directory — it's symbolic links (/D) that need SeCreateSymbolicLinkPrivilege or Developer Mode. So the usual "CI isn't elevated, skip gracefully" justification doesn't apply here. The realistic failure modes are a non-NTFS or UNC TEMP, a restricted cmd.exe, or the leftover case below — all of which you'd want to hear about loudly, not silently tolerate.

2. A single aborted run disables the test permanently on that machine. base is derived from std::process::id() and create_dir_all is idempotent, so a stale …\mxc-dangling-<pid>\link junction left behind by an earlier panic or kill will make mklink /J fail with "Cannot create a file when that file already exists" the next time that PID is recycled — and from then on the test silently no-ops. Cleanup runs before the assert, which is good, but it doesn't survive a hard abort.

3. It doesn't cover the arm that's actually broken. The test drives the Ok(h) if !h.is_invalid() success arm only, so it passes both before and after fixing the _ => PathCanonical::Absent wildcard (see my other comment). It reads like verification of the fail-closed property while leaving the failing half of that property untested — which is the more expensive kind of green.

Suggested change — fail loudly and make the fixture collision-proof:

let base = format!(
    r"{}\mxc-dangling-{}-{:?}",
    tmp.to_string_lossy().trim_end_matches('\\'),
    std::process::id(),
    std::thread::current().id()
);
let _ = std::fs::remove_dir_all(&base); // clear any leftover from an aborted run
std::fs::create_dir_all(&base).unwrap();

let status = Command::new("cmd")
    .args(["/c", "mklink", "/J", &link, &missing_target])
    .status()
    .unwrap();
assert!(
    status.success(),
    "mklink /J failed ({status}); junction creation needs no elevation, so this \
     indicates a broken test environment rather than an expected skip"
);

If you genuinely need it skippable on some target, gate it on an explicit opt-out env var (MXC_SKIP_REPARSE_TESTS) so the skip is a deliberate, visible decision rather than a silent side effect of the fixture failing.

While you're in here, the higher-value companion test is at the canonicalize_allowing_absent_tail level rather than canonicalize_path — a dangling junction ancestor must not produce a Canonical result that still contains the junction. That's where the deny-overlap security property actually lives.

(using remove_dir on the junction rather than remove_dir_all correctly deletes the link without following it into the target. Good catch.)

[Gudge (MGudgin)]

The new discriminator is only ever exercised in one direction, so the regression that would silently break it isn't detectable by the suite.

let mount_lexical = NormalizedPath::parse(mounted);
if mount_norm == denied_norm && mount_lexical != denied_lexical {

The reject side is well covered — canonical_rejects_denied_alias_resolving_into_mount, canonical_rejects_mounted_alias_containing_denied, canonical_rejects_absent_leaf_resolving_under_aliased_mount, and the new canonical_rejects_denied_alias_equal_to_mount_root all drive mount_lexical != denied_lexical true. The accept side has exactly one test, canonical_allows_literal_exact_match_that_resolves (:797-811), and it uses byte-identical strings (C:\project vs C:\project).

That leaves a specific mutation undetected:

if mount_norm == denied_norm && mounted != denied {   // raw &String compare

The entire suite still passes. The only accept-case uses identical strings, for which a raw comparison and a NormalizedPath comparison agree. In production it would now reject C:\Project (mount) vs c:/project (deny) as alias-induced overlap, turning an enforceable config into a hard "cannot be enforced" failure. (This is the concrete cost of the comment imprecision I raised separately on :369-378.)

Deleting the clause outright is caught by the existing test, so the hole is specifically lexically-different-but-normalization-equal spellings — exactly the set NormalizedPath::parse exists to fold.

Suggested tests — note resolver returns Absent for unmapped paths, so both spellings need explicit mappings:

#[test]
fn canonical_allows_case_and_separator_variant_exact_match() {
    // `NormalizedPath::parse` case-folds and folds `/` vs `\`, so these are the
    // same spelling, not an on-disk alias: enforceable by not mounting → accept.
    let resolve = resolver(vec![
        (r"C:\Project", canonical(r"C:\project")),
        ("c:/project", canonical(r"C:\project")),
    ]);
    validate_denied_path_overlap_with(
        &strings(&[r"C:\Project"]),
        &[],
        &strings(&["c:/project"]),
        resolve,
    )
    .expect("case/separator variant of the same path must be accepted");
}

#[test]
fn canonical_allows_dot_dot_variant_exact_match() {
    // `C:\x\..\project` folds to `C:\project` lexically — same spelling, not an alias.
    let resolve = resolver(vec![
        (r"C:\project", canonical(r"C:\project")),
        (r"C:\x\..\project", canonical(r"C:\project")),
    ]);
    validate_denied_path_overlap_with(
        &strings(&[r"C:\project"]),
        &[],
        &strings(&[r"C:\x\..\project"]),
        resolve,
    )
    .expect("`..`-variant of the same path must be accepted");
}

They slot in right after :811. A trailing-separator case (C:\project\ vs C:\project) is a cheap third — parse filters empty segments, so it should also be accepted.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/backends/wslc/common/src/policy_mapping.rs:380

• This exemption also accepts case, separator, .., and trailing-separator variants even though normalize_filesystem_paths only removes byte-identical conflicts. If the path is absent during D6, both entries survive; absent-tail canonicalization can make them equal here, after which WSLC still builds the looser mount and ignores deniedPaths. Thus a mount target created later can expose a path declared denied. Only an identical original string is safe to exempt; reject canonical equality whenever the original strings differ.

            let mount_lexical = NormalizedPath::parse(mounted);
            if mount_norm == denied_norm && mount_lexical != denied_lexical {