Skip to content

Preserve literal Unix backslashes in glob result paths (#990) - #1076

Draft
leynos wants to merge 4 commits into
mainfrom
issue-990-preserve-literal-unix-backslashes-in-glob-result-paths
Draft

leynos wants to merge 4 commits into
mainfrom
issue-990-preserve-literal-unix-backslashes-in-glob-result-paths

Conversation

@leynos

@leynos leynos commented Nov 1, 2026 •

Copy link
Copy Markdown
Owner

Summary

strip_base rebased every matched glob path with an unconditional
replace('\\', "/"). On Unix a backslash is an ordinary filename character,
not a separator, so the returned string could name a different file from the
one the capability metadata check had just validated. A name such as
part\..\other.txt was rewritten into the parent-directory traversal
part/../other.txt, which the checked filesystem path never contained.

Changes

  • Gate result rebasing on cfg(unix), mirroring the platform split already
    used for pattern normalization in normalize.rs. Windows still renders \
    as / for the portable spelling callers expect; Unix returns the exact
    spelling the walk validated.
  • Leave the rest of the boundary alone: capability validation, the
    literal-prefix capability scope, and the Jinja adapter's shell-inert
    allowlist are unchanged. A Unix backslash filename still reaches the
    template adapter and is rejected there, rather than being rewritten into an
    accepted path; direct glob_paths callers stay responsible for quoting.
  • Move strip_base into normalize.rs so both sides of the separator policy
    live together and glob/mod.rs stays inside the 400-line module cap.
  • Add regressions at each reachable level: the direct glob_paths API
    (literal backslash filename, injected traversal, absolute match), the Jinja
    adapter (rejection of representative shell-sensitive names, including
    backslash forms), and manifest loading (preserve, then reject).
  • Document the platform-aware separator contract in the design, developer,
    and user guides.

Closes #990

Verification

All deterministic gates pass on 269a77bcb: make check-fmt, make lint
(cargo doc, clippy, Whitaker, ruff, pylint, df12 lints, ambrleaks, yamllint,
actionlint), make typecheck, make doc-coverage (98.23%), make test
(4950 nextest cases plus doctests), make markdownlint, and make nixie.
coderabbit review --agent on the same revision returned no findings across
the ten changed files.

References

🤖 Generated with Claude Code

Summary by Sourcery

Preserve Unix backslashes in glob results while maintaining platform-appropriate normalization and shell-safety checks.

Bug Fixes:

  • Preserve literal backslashes in Unix glob result paths so returned paths continue to identify the files validated during capability checks.
  • Prevent Unix backslash-containing filenames from being rewritten into directory traversal or other different paths before shell-safety validation.

Enhancements:

  • Centralize platform-aware matched-path normalization with glob pattern normalization while retaining portable forward-slash output on platforms where backslashes are separators.

Documentation:

  • Document platform-specific glob separator behavior and the handling of literal Unix backslashes in the design, developer, and user guides.

Tests:

  • Add regression coverage for direct glob results, absolute matches, traversal-like filenames, Jinja shell-safety rejection, and manifest loading.

@coderabbitai

coderabbitai Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.

@sourcery-ai

sourcery-ai Bot commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

Reviewer's Guide

The PR prevents Unix glob results from being rewritten into different filesystem paths by making strip_base honor platform separator semantics, while preserving Windows behavior and existing capability/Jinja boundaries. It adds layered identity and rejection regressions and documents the resulting contract.

Sequence diagram for platform-aware glob result handling

sequenceDiagram
    participant Caller
    participant GlobExpansion
    participant CapabilityWalk
    participant strip_base
    participant JinjaAdapter

    Caller->>GlobExpansion: glob_paths(pattern, base)
    GlobExpansion->>CapabilityWalk: process_glob_entry()
    CapabilityWalk-->>GlobExpansion: validated matched path
    GlobExpansion->>strip_base: strip_base(base, path)
    alt Unix
        strip_base-->>GlobExpansion: exact matched spelling
    else Windows
        strip_base-->>GlobExpansion: path with / separators
    end
    GlobExpansion-->>Caller: glob result
    Caller->>JinjaAdapter: glob()
    JinjaAdapter-->>Caller: accept safe path or InvalidOperation for backslash
Loading

Flow diagram for preserving Unix backslash filenames

flowchart TD
    A[Matched glob entry] --> B[Capability metadata validation]
    B --> C{Platform}
    C -->|Unix| D[strip_base returns exact spelling]
    C -->|Windows| E[strip_base replaces backslash with /]
    D --> F{Jinja glob adapter?}
    E --> F
    F -->|Unix backslash filename| G[Reject with InvalidOperation]
    F -->|Portable shell-safe path| H[Expose to template]
    D --> I[Direct glob_paths caller receives matched filename]
Loading

File-Level Changes

Change Details Files
Make matched-path rebasing platform-aware so Unix preserves literal backslashes while Windows continues emitting portable forward-slash paths.
  • Move strip_base into normalization code alongside the platform separator policy.
  • Return the post-base-stripping path unchanged on Unix; retain backslash-to-slash conversion elsewhere.
  • Preserve capability validation, literal-prefix scoping, and direct-caller/Jinja boundary behavior.
src/manifest/glob/mod.rs
src/manifest/glob/normalize.rs
Add regression coverage that verifies path identity and shell-safety behavior across the globbing layers.
  • Test literal backslash filenames, traversal-like names, and absolute matches through glob_paths.
  • Verify the Jinja adapter rejects preserved backslash-containing names, including mixed safe/unsafe results.
  • Verify manifest loading preserves the direct result and then rejects it at the shell-inert validation boundary.
src/manifest/glob/tests/identity.rs
src/manifest/glob/tests/base/property.rs
src/manifest/glob/tests/mod.rs
src/manifest/tests/glob/security.rs
tests/manifest_glob_tests/capability_scope.rs
Document the platform-aware result-separator contract and the unchanged security boundaries.
  • Explain that Unix backslashes remain filename characters and must not be rewritten after capability checks.
  • Clarify that Jinja rejects literal backslashes while direct glob_paths callers remain responsible for quoting.
  • Update developer, design, and user-facing glob documentation.
docs/developers-guide.md
docs/netsuke-design.md
docs/users-guide.md

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

codescene-access[bot]

This comment was marked as outdated.

@leynos

leynos commented Nov 2, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai please assess the implementation in this PR for completeness and correctness against the original issue:

## Reported concern

After validating a matched file, strip_base replaces literal Unix backslashes with directory separators. This changes the returned file identity and can introduce parent-directory traversal that was absent from the checked filesystem path.

**Evidence**
- Source/control: Attacker-created Unix filenames matched by a trusted glob pattern.
- Sink/decision: The unconditional backslash replacement in strip_base.
- Missing mitigation: The returned representation does not preserve the identity of the file that passed the capability check.

### Suggested remediation

Preserve literal backslashes on Unix and convert separators only on platforms where backslash is a separator. Keep the template adapter's rejection of shell-sensitive Unix filenames rather than rewriting them into accepted paths.

### Report provenance

- Finding ID: `db51f9f8-c98a-4e85-8bad-a3f13841f740`
- Fingerprint: `76639b398f23cc846a6c04d2ff6749c5bfe6789c60909010299ccb3823b5b693`
- Reported severity: **medium** (CWE-22).
- Reported location: [`src/manifest/glob/mod.rs:385-385`](https://github.com/leynos/netsuke/blob/e52d3ef49429392eebeb484aa4d6622ca79782dd/src/manifest/glob/mod.rs#L385-L385).
- Reported revision: `e52d3ef49429392eebeb484aa4d6622ca79782dd`.

## Acceptance criteria

- Add a focused regression test for the reported trigger and affected operation before fixing it.
- Implement the remediation so the reported unsafe behaviour is rejected or safely bounded, while preserving supported inputs.
- Exercise the relevant integration or behavioural path and document any remaining platform or dependency constraints.
- Update applicable user, architecture, or developer documentation and pass the repository's relevant quality gates.

## Tracking context

Raised from the user-supplied agent-findings export generated on 29 October 2026. The description, severity, evidence, and assumptions above are reported findings; issue creation does not establish independent reproduction. Existing open and closed issues were checked for matching identifiers and root causes.~~~

@coderabbitai

coderabbitai Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

Analysis chain

Script executed:

#!/bin/bash
git rev-parse HEAD
gh pr view 1076 --repo leynos/netsuke --json headRefOid,baseRefOid,files,commits,statusCheckRollup
gh pr diff 1076 --repo leynos/netsuke --stat
fd -H '^(AGENTS\.md|CLAUDE\.md|CONTRIBUTING.*|Makefile)$' .
ast-grep outline src/manifest/glob --items all

Length of output: 22462


Script executed:

#!/bin/bash
git rev-parse HEAD
gh pr view 1076 --repo leynos/netsuke --json headRefOid,baseRefOid,files,commits,statusCheckRollup
gh pr diff 1076 --repo leynos/netsuke --name-only
fd -H '^(AGENTS\.md|CLAUDE\.md|CONTRIBUTING.*|Makefile)$' .
ast-grep outline src/manifest/glob --items all

Length of output: 22231


Script executed:

#!/bin/bash
cat AGENTS.md
gh pr diff 1076 --repo leynos/netsuke --color never
ast-grep outline src/manifest/tests/glob/security.rs --items all
ast-grep outline tests/manifest_glob_tests/capability_scope.rs --items all
sed -n '1,230p' src/manifest/glob/normalize.rs
sed -n '1,135p' src/manifest/glob/mod.rs
sed -n '270,400p' src/manifest/glob/mod.rs
sed -n '250,410p' src/manifest/glob/walk.rs
fd -H -t f '\.(yml|yaml)$' .github
gh api repos/leynos/netsuke/commits/269a77bcbfccb506f8c0debfd7d249a74926178c/check-runs --jq '.check_runs[] | select(.name | contains("CodeScene")) | {name,conclusion,output,details_url}'

Length of output: 83760


Script executed:

#!/bin/bash
nl -ba src/manifest/glob/mod.rs | sed -n '158,245p'
nl -ba src/manifest/glob/normalize.rs | sed -n '160,200p'
nl -ba src/manifest/tests/glob/security.rs | sed -n '1,165p'
nl -ba tests/manifest_glob_tests/capability_scope.rs | sed -n '1,115p'
nl -ba src/manifest/glob/tests/base/property.rs | sed -n '200,270p'
git show --stat --oneline 0cc5c2cf4a5faea4beb65ae1836259d252121977
git show 0cc5c2cf4a5faea4beb65ae1836259d252121977:src/manifest/glob/mod.rs | tail -35
gh api repos/leynos/netsuke/actions/jobs/124826945668 --jq '{name,head_sha,conclusion,steps:[.steps[] | {name,conclusion}]}'
gh api repos/leynos/netsuke/actions/jobs/124826945696 --jq '{name,head_sha,conclusion,steps:[.steps[] | {name,conclusion}]}'
gh run view 41299376691 --repo leynos/netsuke --job 124826945668 --log | rg -n -C1 '4950|tests run|test result:|98\.23|check-fmt|make lint|make test|make typecheck|make doc-coverage|make markdownlint|make nixie|glob_paths_preserves_a_literal_backslash|glob_paths_does_not_introduce_parent|literal_backslash_filename_is_preserved_then_rejected|jinja_glob_rejects_a_literal_backslash'

Length of output: 201761


Script executed:

#!/bin/bash
gh api repos/leynos/netsuke/commits/0cc5c2cf4a5faea4beb65ae1836259d252121977 --jq '{sha,parents:[.parents[].sha],files:[.files[] | {filename,status,additions,deletions}]}'
gh api repos/leynos/netsuke/contents/src/manifest/glob/mod.rs?ref=0cc5c2cf4a5faea4beb65ae1836259d252121977 --jq .content | base64 --decode | tail -30
rg -n 'PASS.*(glob_paths_preserves_a_literal_backslash|glob_paths_does_not_introduce_parent|literal_backslash_filename_is_preserved_then_rejected|jinja_glob_rejects_a_literal_backslash|case_.*backslash|results_use_forward_slashes)' /tmp/coderabbit-shell-logs/shell-output-ZKKTs1 | head -25

Length of output: 4016


@leynos Treat the reported path-identity defect as fixed at 269a77bcb. Keep the quality-gate acceptance criterion open until the CodeScene failure is resolved. I found no correctness defect in the remediation.

Acceptance-criteria assessment

Criterion Assessment
Add a focused regression before the fix Met. Commit 0cc5c2cf adds the regressions while strip_base still contains the unconditional replacement. It precedes fix commit 92a90c49. I verified the commit contents, but did not independently execute the pre-fix tests.
Correct the unsafe behaviour and preserve supported inputs Met. In src/manifest/glob/normalize.rs, Line 178, strip_base retains lexical base stripping. The Unix branch copies the matched spelling without replacing backslashes. The non-Unix branch retains the previous separator conversion.
Exercise the integration path and document constraints Met. Direct-query, Jinja, and manifest-loading regressions cover the affected boundaries. The documentation explains the Unix/Windows distinction and direct callers’ quoting responsibility.
Update documentation and pass relevant quality gates Documentation met; gates not fully complete. The developer, design, and user guides are updated. The build and validation checks pass, but CodeScene reports a duplication violation.

Correctness and regression coverage

  • File identity: src/manifest/glob/tests/identity.rs checks a literal-backslash filename beside a directory-shaped decoy. It reads the returned path and verifies that it reaches the matched file, not the decoy.
  • Introduced traversal: The part\..\other.txt regression checks the exact returned spelling, rejects an introduced /../ component, and verifies the returned file’s contents.
  • Absolute results: A separate regression covers an absolute pattern without an injected base.
  • Template rejection: The unchanged GlobExpansion::into_template_paths checks the complete result set at src/manifest/glob/mod.rs, Line 210. Its allowlist excludes backslashes. The new tests verify rejection of both backslash forms and a mixed safe/unsafe result set.
  • Manifest loading: tests/manifest_glob_tests/capability_scope.rs, Line 77, verifies preservation through the public query followed by rejection through from_path.
  • Compatibility: Existing coverage retains forward-slash directory separators and intentional parent-relative patterns. The fix does not prohibit supported ../ patterns; it prevents filename characters from becoming new traversal components.

Verification and remaining action

I inspected CI results for the exact reviewed revision. Linux CI reports 4,950 tests passed, with seven skipped. The focused regressions all passed. Doctests and Windows build/test checks also passed. Formatting, lint, typecheck, Markdown, diagram validation, and documentation coverage passed; documentation coverage is 98.23%. I did not run repository code in the inspection sandbox.

Resolve the CodeScene duplication failure before declaring all gates complete. The check identifies src/manifest/tests/glob/security.rs. Parameterize the existing mixed-match rejection test with both unsafe;file.txt and nested\leaf.txt to retain both cases without duplicating the test body. Then rerun the affected tests and CodeScene check.

Keep the security claim scoped to representation preservation. The glob dependency still walks ambiently; capability scoping applies to the metadata check. This PR preserves that documented boundary and does not add protection against later filesystem changes.

You are interacting with an AI system.

codescene-access[bot]

This comment was marked as outdated.

leynos and others added 4 commits November 3, 2026 02:47
Cover the reported defect where strip_base rewrote literal Unix
backslashes in matched filenames into directory separators. The three
direct-query cases fail before the fix: a backslash filename beside a
directory-shaped decoy, a backslash-spelled '..' component, and an
absolute match with no injected base.

The Jinja adapter and manifest-loading cases prove that a preserved
backslash is rejected as shell-sensitive rather than accepted under a
different identity.

Co-Authored-By: Claude Code <noreply@anthropic.com>
strip_base rewrote every backslash in a matched path to '/'. On Unix a
backslash is an ordinary filename character, so the returned string no
longer named the file the capability metadata check had validated, and
a name such as `part\..\other.txt` became the parent-directory traversal
`part/../other.txt`.

Split the normalisation on the same cfg(unix) boundary as normalize.rs:
non-Unix targets keep the portable '/' rendering, while Unix returns the
checked spelling unchanged. The Jinja adapter's unchanged shell-inert
allowlist now rejects such a filename instead of accepting the rewritten
path, and direct Rust callers receive the exact matched name.

Update the design, developer, and user documentation to state the
platform-aware separator contract and the filename identity guarantee.

Co-Authored-By: Claude Code <noreply@anthropic.com>
The module-size lint caps a Rust source file at 400 lines. Documenting
the platform-aware separator contract pushed glob/mod.rs to 405, so
relocate strip_base beside the pattern-side separator normalisation it
mirrors. Both files are now well below the cap, and the matched-path
and pattern rules stay together under one cfg(unix) boundary.

No behaviour change: the focused glob suites pass unchanged.

Co-Authored-By: Claude Code <noreply@anthropic.com>
The literal-backslash mixed-set case repeated the body of the existing
shell-punctuation case verbatim apart from the fixture name, which
CodeScene scored as code duplication and reported as a failed advisory
gate on the pull request (10.00 -> 9.39).

Both cases are the same contract, so they become rows of one rstest
table keyed by the unsafe filename. Coverage is unchanged: the expansion
still fails as a whole for shell punctuation and for a preserved literal
backslash.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@leynos
leynos force-pushed the issue-990-preserve-literal-unix-backslashes-in-glob-result-paths branch from 425f47f to 9351102 Compare November 3, 2026 01:48

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Preserve literal Unix backslashes in glob result paths

1 participant