Skip to content

Split an archive entry the way the format defines, not the host - #88

Merged
otsobide merged 5 commits into
devfrom
feature/portable-entry-splitting
Aug 26, 2026
Merged

Split an archive entry the way the format defines, not the host#88
otsobide merged 5 commits into
devfrom
feature/portable-entry-splitting

Conversation

@otsobide

Copy link
Copy Markdown
Owner

Windows CI went red on dev after #87 merged (test (rust, windows), two failures in apps/core/tests/names.rs). The PR run was green because the cross-OS matrix only runs on shipping branches, which is the gate working one merge too late.

Chasing it turned up three problems with one cause, plus a fourth that predates all of this.

One cause

normal_components split entry names with Path::new(name).components(). std::path is #[cfg]-dependent while NameRules is data, so the rules were portable and the splitting they ran over was not.

Unix regression, currently on dev. A backslash is a legal Unix file name character and was not a separator, so dir\f.txt was one component that rewrite then refused. Collapse could build an archive it would not extract:

collapse compress tree -o tree.zip   exit 0, and it passes the #86 verification
collapse extract tree.zip -o back    exit 1, `back` is never created
   error: the archive entry "tree/a\b.txt" cannot be written:
          "a\b.txt" becomes "a\b.txt", which is not a name this system can hold

All three formats, and it fails at extract time, which is after the user may have deleted the originals. In the desktop it was worse than an error: nameProblem renders only inside the naming sheet, which is not open on that path, so the Extract button did nothing and said nothing.

Windows, issue #63's own hole. A leading a: parses as a drive prefix, and a prefix is not a Normal component, so it was silently discarded. a:b/c.txt was judged, reported and written as b/c.txt: the report never asked about that colon, on the only platform the colon matters.

The CI failure itself. rewrite_entry("a:b/CON/c?.txt.") answered b\CON_\c_.txt.

An archive entry name is not a host path. ZIP mandates / (APPNOTE 4.4.17.1) and tar has used it since v7, so entry_components splits on / everywhere and NameRules::windows() now answers the same from a Mac as from Windows. The backslash moves into WINDOWS_REJECTED, where it belongs: an ordinary character that Unix holds and Windows cannot.

That also removes the post-rewrite contains('\\') check, which was dead code except when it was wrong. check_replacement already refuses both separators before either is pushed, so it could not fire for its stated reason.

The fourth, which is older

zip and 7z sanitized an entry name, joined it to the output and wrote, with no check on where the join landed. tar has always resolved the parent through unpack_in. Measured on dev before this branch, with a symlink named link already in the output directory:

extract(archive holding "link/evil.txt") -> Ok(["link/evil.txt"])
file written outside the output directory: true

origin/main has the same shape, so this is in the released v0.7.0, not something #87 introduced. We never create symlinks, but extracting into a directory that already has one is ordinary.

ensure_inside now resolves the directory about to be written into at every zip and 7z write site. It also covers the Windows case where PathBuf::push clears the buffer on a component that parses as a drive (docs/c:evil.txt resolving against the current directory of C:), and sanitize_entry_path re-parses each component to close that at the source.

Tests

585 Rust (was 578) and 113 Vitest (was 112). Every new test was checked against the unfixed code rather than assumed:

test fails without the fix
an_entry_splits_the_same_way_on_every_host yes
only_windows_refuses_a_backslash_inside_a_component yes
a_unix_name_holding_a_backslash_survives_the_round_trip yes
the_report_sees_a_colon_in_the_first_component_too Windows only, pin
no_format_writes_through_a_symlink_already_in_the_output yes
a_renamed_entry_cannot_be_written_through_such_a_symlink yes
a_colon_in_a_later_component_cannot_clear_the_path_being_built Windows only, pin
desktop: naming refusal with no dialog open yes

The two assertions that failed on Windows CI compared the message against the path the test built, while the message is rendered from a canonicalized root: on Windows that carries a \\?\ verbatim prefix and expands the runner's 8.3 short name. They now compare against what extraction actually resolved. The identical assertion in apps/cli/tests/names.rs was a third Windows failure that run never reached, because make stops at the first red app.

Notes

`normal_components` was `Path::new(name).components()`, and `std::path` is
`#[cfg]`-dependent while `NameRules` is data. The rules were portable; the
splitting they ran over was not, so the seam only modelled Windows half way
and broke Unix outright.

Three failures, one cause:

- On Unix a backslash is an ordinary character, so `dir\f.txt` was one
  component that `rewrite` then refused as "not a name this system can hold".
  A legal Unix file name became an archive collapse could build and then
  would not extract, failing at extract time with nothing written. All three
  formats, through the public `extract`.
- On Windows a leading `a:` parses as a drive prefix, and a prefix is not a
  `Normal` component, so it was silently discarded: `a:b/c.txt` was judged,
  reported and written as `b/c.txt`. The report therefore never asked about
  that colon, which is the hole issue #63 exists to close, on the only
  platform issue #63 is about.
- CI caught the same thing as a Windows-only test failure:
  `rewrite_entry("a:b/CON/c?.txt.")` answered `b\CON_\c_.txt`.

An archive entry name is not a host path. ZIP mandates the forward slash
(APPNOTE 4.4.17.1) and tar has used it since v7, so `entry_components` splits
on `/` on every machine and `NameRules::windows()` now answers the same
question from a Mac as from Windows.

That makes the backslash an ordinary character inside a component, which is
exactly what it is, so it moves into WINDOWS_REJECTED where Unix keeps
allowing it and Windows does not.

It also removes the post-rewrite `contains('\\')` check, which was dead code
except when it was wrong: `check_replacement` already refuses both separators
before either is pushed, so the test could not fire for its stated reason,
and the only thing it ever caught was a name Unix holds perfectly well. The
`/` half stays, structurally, since no ruleset lists the separator.
zip and 7z sanitized an entry name, joined it to the output and wrote. There
was no check that what they were about to write into was still inside, which
tar has always had through `unpack_in`'s `validate_inside_dst`. Two ways past
the lexical guard, and the first is not hypothetical:

- A symlink already sitting in the output directory. Extracting an archive
  holding `link/evil.txt` into a directory that contains a symlink named
  `link` wrote straight through it and reported success. Measured before this
  change: `Ok(["link/evil.txt"])`, with the file outside the chosen output.
  We never create symlinks ourselves, but extracting into a directory that
  already has one is ordinary. tar was immune; zip and 7z were not, and this
  predates the naming work.
- On Windows, a component that parses as a drive. `PathBuf::push` replaces
  what it holds when handed a path carrying a prefix, and a prefix is parsed
  only at the head of a path, so `docs/c:evil.txt` offers `sanitize_entry_path`
  no `Prefix` component to reject, yet pushing the second component discards
  `docs` and leaves a drive-relative path resolving against the current
  directory of C:.

`ensure_inside` resolves the directory just created and requires it to still
be under the output, at every zip and 7z write site. `sanitize_entry_path`
additionally re-parses each part and demands it still be exactly one `Normal`
component, which closes the drive case at the source and leaves Unix, where
the same string is an ordinary file name, untouched.
Four tests over the splitting itself, three of which fail against the old
`Path::components` splitter on this Mac and would have caught the Unix
regression before it merged. The fourth pins the Windows half, which no
machine here can fail, so that the two can never diverge again.

Two containment tests in security.rs for the symlink already sitting in the
output directory, one on the untouched write path and one on the planned
rename, plus a cross-platform pin for a colon in a non-leading component.
Both symlink tests fail without `ensure_inside`.

Also fixes the two assertions that failed on Windows CI, here and the twin in
the CLI suite that the same run never reached because make stops at the first
red app. They compared the error message against the path the test built,
while the message is rendered from a canonicalized root: on Windows that
carries a `\?\` verbatim prefix and expands the runner's 8.3 short name, so
the assertion was comparing against a path production never prints. They now
compare against what extraction actually resolved, which is the thing worth
proving.
`nameProblem` is rendered only inside the naming sheet, and the sheet only
opens when the pre-flight report found something to ask about. The two are
separate passes over the archive and they can disagree: a listing the first
pass could not read is reported as "nothing to ask", and extraction then
refuses a name. With no sheet to hold the message, the Extract button did
nothing at all, and said nothing about it.

A refusal with a dialog open is still a question and still belongs in the
sheet. Without one it goes to the error banner, which is where "this did not
work" already lives.
threat_model.md had nothing about entry names the host cannot write, which
was the last piece of issue #63 still owed. It now covers the colon and the
NTFS stream it becomes, the quieter members of the same family, and why the
rules are data rather than `#[cfg]`.

It also records the hole that shipped in v0.7.0 and how it was closed, since
the interesting part is not that the rules were right but that what they ran
over was not, and a reader deciding whether to trust the defence should be
able to see that failure mode named.

Section 2 gains the case it was quietly missing: a symlink the archive did
not bring, already sitting in the output directory. Only tar defended against
that; the sentence about `unpack_in` was describing tar's guard as if it were
everyone's.

Counts: 585 Rust and 113 Vitest, 470 offline.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

full-matrix Run the macOS and Windows suites on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant