diff --git a/.dockerignore b/.dockerignore index 052f643..f682a8c 100644 --- a/.dockerignore +++ b/.dockerignore @@ -14,7 +14,6 @@ apps/landing/ # Never ship history or local tooling state into the context. .git/ .github/ -.claude/ **/node_modules/ **/.DS_Store diff --git a/.github/workflows/deploy-landing.yml b/.github/workflows/deploy-landing.yml index fd36709..855b5fa 100644 --- a/.github/workflows/deploy-landing.yml +++ b/.github/workflows/deploy-landing.yml @@ -3,13 +3,13 @@ name: Deploy landing # Build the Nuxt landing and publish the compiled static site to a branch that # holds only the build output: # - push to main → pages/landing (production) -# - push to dev → pages/landing-dev (staging, to preview before main) +# - push to develop → pages/landing-dev (staging, to preview before main) on: push: - branches: [main, dev] + branches: [main, develop] workflow_dispatch: -# One deploy per source branch at a time; don't let main and dev cancel each other. +# One deploy per source branch at a time; don't let main and develop cancel each other. concurrency: group: deploy-landing-${{ github.ref_name }} cancel-in-progress: true @@ -39,7 +39,9 @@ jobs: env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | - # main goes to production; every other branch (dev) goes to staging. + # main goes to production; every other branch (develop) goes to staging. + # The target branch keeps its -dev name: it is what serves the staging + # site, and renaming it would change that URL for no gain. if [ "$GITHUB_REF_NAME" = "main" ]; then TARGET="pages/landing" else diff --git a/.github/workflows/gitflow.yml b/.github/workflows/gitflow.yml index 2d21507..49734b3 100644 --- a/.github/workflows/gitflow.yml +++ b/.github/workflows/gitflow.yml @@ -1,19 +1,19 @@ name: Gitflow # Enforce where changes come from: -# - PRs into dev must come from a gitflow branch (feature/*, hotfix/* or +# - PRs into develop must come from a gitflow branch (feature/*, hotfix/* or # release/*), or from main itself (a post-release back-merge). -# - PRs into main are releases and must come from dev, only. +# - PRs into main are releases and must come from develop, only. # Both checks are required by the branch rulesets, so a PR from any other # source branch cannot be merged. on: pull_request: - branches: [dev, main] + branches: [develop, main] jobs: branch-name: name: gitflow branch name - if: github.base_ref == 'dev' + if: github.base_ref == 'develop' runs-on: ubuntu-latest steps: - name: Check the head branch follows gitflow @@ -24,7 +24,7 @@ jobs: feature/*|hotfix/*|release/*|main) echo "ok: $HEAD_REF" ;; *) - echo "PRs into dev must come from a feature/*, hotfix/* or release/* branch (got: $HEAD_REF)" + echo "PRs into develop must come from a feature/*, hotfix/* or release/* branch (got: $HEAD_REF)" exit 1 ;; esac @@ -33,13 +33,13 @@ jobs: if: github.base_ref == 'main' runs-on: ubuntu-latest steps: - - name: Check the release comes from dev + - name: Check the release comes from develop env: HEAD_REF: ${{ github.head_ref }} run: | - if [ "$HEAD_REF" = "dev" ]; then - echo "ok: release PR from dev" + if [ "$HEAD_REF" = "develop" ]; then + echo "ok: release PR from develop" else - echo "PRs into main are releases and must come from dev (got: $HEAD_REF)" + echo "PRs into main are releases and must come from develop (got: $HEAD_REF)" exit 1 fi diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index f6eb7d3..fb99cd9 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -30,7 +30,7 @@ concurrency: jobs: # A release must correspond to a released state of the code: refuse any # v* tag whose commit is not part of main's history (e.g. a tag placed on - # dev by mistake). The publish job requires this to pass. + # develop by mistake). The publish job requires this to pass. verify: name: tag is on main if: github.event_name == 'push' && startsWith(github.ref, 'refs/tags/') diff --git a/.github/workflows/test-and-build.yml b/.github/workflows/test-and-build.yml index 5f02cab..b2e92af 100644 --- a/.github/workflows/test-and-build.yml +++ b/.github/workflows/test-and-build.yml @@ -5,7 +5,7 @@ name: test-and-build # landing has no test suite and is built by deploy-landing.yml instead. on: push: - branches: [main, dev] + branches: [main, develop] # `labeled` is not in the default set, and without it the `full-matrix` label # below would be a documented escape hatch that never fires: adding a label # would change nothing until the next push. @@ -245,11 +245,11 @@ jobs: # times a Linux one in billed minutes and feature work does not need them. # # They run when the code is on, or heading to, a shipping branch: - # - any push to dev or main (the `on:` block already limits pushes to those) - # - a pull request FROM dev (the release PR into main), from main (a + # - any push to develop or main (the `on:` block already limits pushes to those) + # - a pull request FROM develop (the release PR into main), from main (a # back-merge) or from a release/* branch # A pull request from feature/* or hotfix/* therefore stays Linux only, and - # picks these up when it lands on dev. When such a branch does touch + # picks these up when it lands on develop. When such a branch does touch # platform-specific code, label the pull request `full-matrix` to ask for # them anyway rather than renaming the branch. # @@ -263,7 +263,7 @@ jobs: if: >- github.event_name == 'push' || github.event_name == 'workflow_dispatch' || - github.head_ref == 'dev' || + github.head_ref == 'develop' || github.head_ref == 'main' || startsWith(github.head_ref, 'release/') || contains(github.event.pull_request.labels.*.name, 'full-matrix') @@ -534,22 +534,22 @@ jobs: run: make desktop/compile # The same compile on the other two platforms it ships to, on the release - # path only: a release/* branch, and the dev to main pull request itself. + # path only: a release/* branch, and the develop to main pull request itself. # # That second one is not optional. The pull request into main IS the release # gate, the last thing that runs before a tag exists, so a desktop build # skipping there would mean the app was never compiled for macOS or Windows - # on the commit about to ship. Its head branch is dev, not release/*, so it + # on the commit about to ship. Its head branch is develop, not release/*, so it # needs naming separately. # - # Not on dev, deliberately. The test jobs above already compile this crate on + # Not on develop, deliberately. The test jobs above already compile this crate on # both platforms: `make desktop/test-rust` runs `cargo test` inside src-tauri, # which goes through generate_context!(), runs build.rs (so rc.exe, the icon # and the manifest on Windows) and links executables. All this adds is the # release profile, which for this crate means lto, codegen-units = 1 and # panic = "abort": a real difference, but a narrow one, and the slowest kind # of build there is. Paying ten to fifteen minutes per platform on every push - # to dev for that is not a good trade. + # to develop for that is not a good trade. # # On a release branch it is, because the alternative is finding out after the # tag is pushed. Note what it still does not cover: it compiles rather than diff --git a/.gitignore b/.gitignore index 51f92f8..e08c794 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,2 @@ target/ .DS_Store -CLAUDE.md diff --git a/README.md b/README.md index 5638742..79720cb 100644 --- a/README.md +++ b/README.md @@ -60,8 +60,18 @@ Formats: `zip` (default, or inferred from `-o`'s extension), `7z`, `tar`. Level next to the source; override with `-o`. It **won't overwrite** an existing archive unless you pass `--force`, and `--force` still refuses to write onto its own source or onto a file inside the folder being compressed, because either -would destroy the data instead of archiving it. Run `collapse --help` for the -full surface. +would destroy the data instead of archiving it. + +**Every local compression is read back before it is reported.** A compressor +here finalises on drop, so a run that dies partway through still closes out an +archive that opens cleanly and is quietly missing whatever had not been written +yet — nothing about the file itself says so. By default the check is the +archive's own listing, which is what catches that. `--verify` upgrades it to +reading every entry back, roughly twice the work, which also checks the +per-entry checksums zip and 7z store (tar stores none). It cannot be combined +with `--server`, since the archive is built on the far side. + +Run `collapse --help` for the full surface. Prebuilt macOS binaries (Apple Silicon and Intel tarballs, with sha256 checksums) are attached to every @@ -188,8 +198,8 @@ Requires **Rust 1.88+** (2021 edition). ```bash make build # build the Rust crates -make test # run every suite (620 Rust tests + 116 Vitest cases) -make test/rust # only the Rust tests that need no Node toolchain (505) +make test # run every suite (621 Rust tests + 116 Vitest cases) +make test/rust # only the Rust tests that need no Node toolchain (506) ``` `make test` includes the desktop app's own Rust suite, which compiles Tauri, so @@ -244,9 +254,9 @@ make fmt make lint # format / clippy both Rust workspaces make fmt/check # fail instead of reformatting (the tree is rustfmt clean) ``` -Work happens on `dev`, merged into `main` per release (see +Work happens on `develop`, merged into `main` per release (see [git flow](docs/git_flow.md)). CI invokes these same `make` targets on every -pull request and on pushes to `dev` and `main`. +pull request and on pushes to `develop` and `main`. ## License diff --git a/apps/cli/src/lib.rs b/apps/cli/src/lib.rs index 4df923d..945b564 100644 --- a/apps/cli/src/lib.rs +++ b/apps/cli/src/lib.rs @@ -437,17 +437,18 @@ fn run_extract(archive: PathBuf, output_dir: PathBuf) -> Result CliError { @@ -143,50 +143,50 @@ fn the_refusal_counts_the_entries_each_character_holds_up() { ); } -/// A trailing dot and a device name have one correct answer and nobody to ask, -/// so they are not what the refusal is about. Refusing them too would leave a -/// Windows user unable to extract an archive whose only fault is a file called -/// `aux.log`, which the desktop app would open without a word. +/// A trailing dot and a device name used to be adjusted rather than refused, on +/// the reasoning that they had one correct answer and nobody to ask. Core +/// adjusts nothing now, so `run_extract` refuses on the whole report instead of +/// on its `characters` half, and these three entries are exactly what that +/// widening is about: an archive whose only fault is a file called `aux.log` is +/// refused on Windows rather than arriving as `aux_.log`. +/// +/// Judged through `NameReport` rather than by running the command, because the +/// faults are Windows-only and this suite runs on Linux. #[test] -fn a_name_that_needs_no_answer_is_adjusted_rather_than_refused() { +fn a_name_that_needs_no_answer_is_refused_like_any_other() { let windows = NameRules::windows(); let report = NameReport::of(&["notes.txt.", "CON.txt", "aux.log", "fine.txt"], windows); + // The half the CLI used to consult is empty: not one of these is a question + // anybody could be asked. Under the old rule the archive extracted. + assert!(report.characters.is_empty(), "none of these is a question"); + // The half it consults now is not, which is the whole of the change. + assert!(!report.is_empty(), "the archive is refused all the same"); + + let named: Vec<&str> = report.entries.iter().map(|e| e.entry.as_str()).collect(); assert_eq!( - adjustments(&report, windows), - vec![ - Adjustment { - entry: "notes.txt.".to_string(), - written: "notes.txt".to_string(), - }, - Adjustment { - entry: "CON.txt".to_string(), - written: "CON_.txt".to_string(), - }, - Adjustment { - entry: "aux.log".to_string(), - written: "aux_.log".to_string(), - }, - ], - "the device keeps the extension that says what the file is, and the writable name is absent" + named, + ["notes.txt.", "CON.txt", "aux.log"], + "every offending entry, and the writable one absent" ); } -/// An entry needing an answer has no adjustment to report, because there is no -/// answer to apply. That is what keeps the two lists disjoint: what `run` -/// refuses is exactly what this cannot rewrite. +/// The two kinds of fault now arrive at the same place, which is what stops the +/// refusal being half a list. A report mixing one of each names both, so a +/// user is not refused for `what?.txt`, fixed nothing, and then refused again +/// for `notes.txt.`. #[test] -fn an_entry_needing_a_replacement_has_no_adjustment() { +fn a_report_mixing_both_kinds_of_fault_names_all_of_them() { let windows = NameRules::windows(); let report = NameReport::of(&["what?.txt", "notes.txt."], windows); - assert_eq!( - adjustments(&report, windows), - vec![Adjustment { - entry: "notes.txt.".to_string(), - written: "notes.txt".to_string(), - }], - ); + assert!(!report.is_empty()); + let named: Vec<&str> = report.entries.iter().map(|e| e.entry.as_str()).collect(); + assert_eq!(named, ["what?.txt", "notes.txt."]); + + let message = windows_refusal("mixed.zip", &["what?.txt", "notes.txt."]); + assert!(message.contains("what?.txt"), "{message}"); + assert!(message.contains("notes.txt."), "{message}"); } // ------------------------------------------------------ end to end, on any host -- diff --git a/apps/core/src/compression.rs b/apps/core/src/compression.rs index 4137783..9ef50fc 100644 --- a/apps/core/src/compression.rs +++ b/apps/core/src/compression.rs @@ -16,7 +16,7 @@ pub use self::tar::{compress_tar, compress_tar_dir, extract_tar}; pub use self::verify::{verify_archive, Verify}; pub use self::zip::{compress_zip, compress_zip_dir, extract_zip}; -pub(crate) use self::names::{plan_names, NamePlan}; +pub(crate) use self::names::{refuse_unwritable_names, NamePlan}; pub(crate) use self::sevenz::{list_7z_entries, read_7z_entries}; pub(crate) use self::tar::{list_tar_entries, read_tar_entries}; pub(crate) use self::walk::walk_tree; @@ -371,9 +371,9 @@ pub fn compress_dir( /// /// A separate type rather than more arguments on [`extract`], and /// [`extract_with`] rather than a replacement for it: every existing caller -/// (the CLI, the server, the desktop, this crate's own tests) has no -/// substitutions to offer and should not have to say so, and the next knob -/// extraction grows should not add a third function. +/// (the CLI, the server, the desktop, this crate's own tests) has nothing to +/// say here and should not have to say so, and the next knob extraction grows +/// should not add a third function. #[derive(Debug, Clone, Default)] #[non_exhaustive] pub struct ExtractOptions { @@ -394,7 +394,13 @@ impl ExtractOptions { self } - /// The caller's answers for the characters the host cannot write. + /// **Inert.** Kept so the callers that pass answers still compile while + /// their own naming dialogs are being taken out; nothing reads it. + /// + /// Extraction no longer substitutes anything: a character this filesystem + /// cannot write fails the extraction rather than being replaced, so there + /// is no answer for a caller to supply. Removing this, [`Substitutions`] + /// and the two front-end dialogs that fill it is follow-up work. pub fn with_replacements(mut self, replacements: Substitutions) -> Self { self.replacements = replacements; self @@ -404,6 +410,8 @@ impl ExtractOptions { self.rules } + /// The answers this was handed. Read by nobody; see + /// [`Self::with_replacements`]. pub fn replacements(&self) -> &Substitutions { &self.replacements } @@ -471,11 +479,14 @@ fn list_entries(archive: &Path, algorithm: Algorithm) -> Result, Com /// What an archive holds that this machine cannot write as ordinary files. /// /// Reads the listing and nothing else: no entry is decompressed and nothing is -/// created, so a front end can ask this before it asks the user anything. Feed -/// the answers back through [`ExtractOptions::with_replacements`]. +/// created, so a front end can ask this before it makes anyone wait. /// -/// An empty report ([`NameReport::is_empty`]) means extraction has no naming -/// question to ask, which on Unix is nearly always the case. +/// **This is now a prediction, not a questionnaire.** A non-empty report +/// ([`NameReport::is_empty`]) means [`extract`] will refuse this archive on +/// this machine, and the report says which entries and why so a front end can +/// explain it. There is nothing to answer: the entries it names are the reason +/// the extraction will not happen, not a form to fill in. On Unix the report is +/// nearly always empty. pub fn unwritable_names(archive: &Path) -> Result { unwritable_names_with(archive, NameRules::host()) } @@ -493,44 +504,55 @@ pub fn unwritable_names_with( /// Extract an archive into `output_dir`. /// -/// Returns the list of extracted file paths (relative to `output_dir`), which -/// are the names **as written**: an entry the host had to be given a different -/// name for is reported under the name that is on disk, never under the -/// archive's, or a front end would list files nobody can find. +/// Returns the list of extracted file paths, relative to `output_dir` and +/// spelled exactly as the archive spells them. +/// +/// **All of the archive, under its own names, or none of it.** An entry this +/// filesystem cannot hold as spelled fails the whole extraction +/// ([`CompressionError::Name`]) before anything is written, rather than being +/// adjusted to fit: a renamed file is not the file the archive named, and +/// nothing downstream can tell the two apart afterwards. Ask +/// [`unwritable_names`] first if the answer is worth showing someone before +/// they wait for it. /// /// The algorithm is detected from the archive file extension. pub fn extract(archive: &Path, output_dir: &Path) -> Result, CompressionError> { extract_with(archive, output_dir, &ExtractOptions::default()) } -/// [`extract`], with the caller's answers for the entry names this machine -/// cannot write. +/// [`extract`], against a chosen set of [`NameRules`] rather than the host's. /// -/// Naming is settled over the whole listing before the first byte is written, -/// and a listing that cannot be read stops it there (issue #89), so the answers -/// nothing can recover from (a character with no replacement, two entries that -/// would land on one name, an archive too damaged to read) leave the output -/// directory as they found it. +/// Same policy as [`extract`], which is the point of it: this exists so a test +/// on one machine can ask what another machine would do, not so a caller can +/// soften the answer. +/// +/// Every name is judged against the whole listing before the first byte is +/// written, and a listing that cannot be read stops it there (issue #89), so +/// an archive this system cannot hold leaves the output directory exactly as +/// it found it. pub fn extract_with( archive: &Path, output_dir: &Path, options: &ExtractOptions, ) -> Result, CompressionError> { let algorithm = algorithm_of(archive)?; - // Before the archive is even opened: an answer that is itself unwritable is - // wrong whether or not any entry needs it. - options.rules().check_replacements(options.replacements())?; - let (names, plan) = plan_for(archive, algorithm, options)?; - refuse_overwriting_the_archive(archive, output_dir, &names, &plan)?; - + let names = listing_for(archive, algorithm)?; + // Judged against the whole listing before a byte is written, so an archive + // this host cannot hold leaves the output directory as it found it. + refuse_unwritable_names(&names, options.rules())?; + refuse_overwriting_the_archive(archive, output_dir, &names)?; + + // The plain backends, because there is no longer any plan to hand them: + // every entry is written under the name the archive spells or the run + // stopped above. match algorithm { - Algorithm::SevenZ => self::sevenz::extract_7z_planned(archive, output_dir, &plan), - Algorithm::Tar => self::tar::extract_tar_planned(archive, output_dir, &plan), - Algorithm::Zip => self::zip::extract_zip_planned(archive, output_dir, &plan), + Algorithm::SevenZ => self::sevenz::extract_7z(archive, output_dir), + Algorithm::Tar => self::tar::extract_tar(archive, output_dir), + Algorithm::Zip => self::zip::extract_zip(archive, output_dir), } } -/// Work out what every entry will be called, from the listing. +/// The archive's listing, read before anything is written. /// /// **A listing that cannot be read stops the extraction here**, before anything /// is written. @@ -540,8 +562,8 @@ pub fn extract_with( /// layer would only replace that message with a worse one. The reasoning was /// right about the message and wrong about the timing (issue #89): the /// extractor fails **while streaming**, so by the time it notices it has -/// already written every entry before the fault, and written them with no plan -/// at all, which means no rewriting, no refusal and no collision check. An +/// already written every entry before the fault, and written them with nothing +/// judged at all, so no name was refused however badly it fitted the host. An /// archive holding `notes.txt:hidden` was refused outright when its listing was /// intact and written as an invisible NTFS stream when it was not: the harm of /// issue #63, performed without the user ever being asked. One bad 512 byte @@ -558,27 +580,26 @@ pub fn extract_with( /// and so never come through here; `recovering_from_a_damaged_archive_is_still_ /// possible_through_the_backend` pins that. /// -/// It costs one listing per extraction, paid even when nothing needs renaming, +/// It costs one listing per extraction, paid even when every name is ordinary, /// because the only way to know that is to read the names. For zip and 7z that /// is a header read; for tar it is a second walk over the headers, seeking past /// each member rather than reading it. -fn plan_for( - archive: &Path, - algorithm: Algorithm, - options: &ExtractOptions, -) -> Result<(Vec, NamePlan), CompressionError> { - let names = list_entries(archive, algorithm).map_err(unreadable_archive)?; - let plan = plan_names(&names, options.rules(), options.replacements())?; - Ok((names, plan)) +fn listing_for(archive: &Path, algorithm: Algorithm) -> Result, CompressionError> { + list_entries(archive, algorithm).map_err(unreadable_archive) } /// Refuse an extraction that would write one of the archive's own entries over /// the archive. /// /// Free to run, in the sense that matters: the listing it needs has already -/// been read and paid for by the planning pass, so this adds one identity check +/// been read and paid for by [`listing_for`], so this adds one identity check /// per entry and no extra pass over the file. /// +/// It compares the name the archive spells, because that is now the only name +/// an entry can be written under. It used to have to follow a rewritten name +/// as well, since a substitution could land an entry on the archive that the +/// archive's own name did not match; nothing rewrites a name any more. +/// /// By **file identity**, not by path. A hardlink is a second name for one file /// and never resolves to the same string, which is exactly how `--force` used /// to be able to overwrite its own source on the compression side before that @@ -587,7 +608,6 @@ fn refuse_overwriting_the_archive( archive: &Path, output_dir: &Path, names: &[String], - plan: &NamePlan, ) -> Result<(), CompressionError> { for name in names { let Some(natural) = sanitize_entry_path(name) else { @@ -595,8 +615,7 @@ fn refuse_overwriting_the_archive( // that is the message worth keeping. continue; }; - let rel = plan.written_as(name).map_or(natural, Path::to_path_buf); - if crate::paths::same_file(&output_dir.join(&rel), archive) { + if crate::paths::same_file(&output_dir.join(&natural), archive) { return Err(CompressionError::WouldOverwriteArchive { archive: archive.to_path_buf(), entry: name.clone(), diff --git a/apps/core/src/compression/names.rs b/apps/core/src/compression/names.rs index 551e225..80dbdc0 100644 --- a/apps/core/src/compression/names.rs +++ b/apps/core/src/compression/names.rs @@ -1,5 +1,4 @@ -//! Which entry names the host can write, and what to do with the ones it -//! cannot. +//! Which entry names the host can write, and which ones stop an extraction. //! //! Extraction takes names from an archive, which is to say from another //! machine. Containment ([`sanitize_entry_path`](super::sanitize_entry_path)) @@ -10,6 +9,18 @@ //! directory, and `notes.txt:hidden` is accepted as the `hidden` stream of //! `notes.txt` rather than as a file, with no error at all. //! +//! **The answer to all four is the same: refuse the archive.** This module used +//! to negotiate instead. It asked the user to supply a character in place of +//! the `?`, dropped the trailing dot on its own and suffixed the device name, +//! then extracted under the names it had arrived at. The output was then a tree +//! whose names were this crate's invention rather than the archive's, with no +//! way for anything downstream to tell which files those were. An extraction +//! that cannot reproduce what the archive says is one that should not happen, +//! so [`refuse_unwritable_names`] stops it before the first byte and says which +//! entry and why. The machinery for the old answer is still here and unused, +//! because the front ends still hand it answers; see +//! [`super::ExtractOptions::with_replacements`]. +//! //! The rules are **data** ([`NameRules`]) rather than `#[cfg]`, and that is the //! point: [`NameRules::windows`] can be asked for from any machine, so every //! rule here is exercised on a Mac and on Linux CI as well as on the Windows @@ -633,10 +644,59 @@ pub enum NameError { result: String, }, + /// The host cannot hold this name exactly as the archive spells it. + /// + /// The whole extraction stops here. Extraction writes what the archive + /// says or it writes nothing: a name this filesystem would refuse, or + /// silently store under a different name, is reported rather than adjusted, + /// because the adjusted file is not the file the archive named and nothing + /// downstream can tell the difference afterwards. + #[error( + "the archive entry {entry:?} cannot be written on this system: {component:?} {}. \ + Nothing was extracted; extract it on a system that can hold the name.", + describe(.problems) + )] + Unwritable { + entry: String, + component: String, + problems: Vec, + }, + #[error("{key:?} is not a single character, so there is nothing to replace")] NotOneCharacter { key: String }, } +/// Why one component cannot be written, as a phrase that follows its name. +/// +/// Rendered here rather than on [`NameProblem`] itself: this is the sentence a +/// refusal reads in, and the same data has to render differently in the desktop +/// dialog, which shows it as structure rather than as prose. +fn describe(problems: &[NameProblem]) -> String { + problems + .iter() + .map(|problem| match problem { + NameProblem::Character { + character, + fault: CharacterFault::Rejected, + } => format!("contains {character:?}, which this system refuses in a file name"), + NameProblem::Character { + character, + fault: CharacterFault::Reinterpreted, + } => format!( + "contains {character:?}, which this system reads as something other than part \ + of the name" + ), + NameProblem::TrailingCharacters { removed } => { + format!("ends in {removed:?}, which this system does not preserve") + } + NameProblem::ReservedDevice { device } => { + format!("is the reserved device name {device:?}") + } + }) + .collect::>() + .join("; and ") +} + impl NameError { /// Re-point an error raised over one component at the whole entry, which is /// the only name the person reading it has ever seen. @@ -661,10 +721,14 @@ impl NameError { /// The name every entry will be written under, for the entries whose name /// changes. /// -/// Built from the whole listing before anything is written, because two of the -/// three answers it can give ("no replacement for this" and "these two entries -/// collide") must stop the extraction while the output directory is still -/// empty, and a collision cannot be seen one entry at a time. +/// **Always the identity now, and kept only so the write paths do not have to +/// change in the same commit as the policy.** Extraction no longer renames an +/// entry ([`refuse_unwritable_names`]), so nothing constructs a plan that +/// says anything: every backend receives [`Self::identity`] and every +/// [`Self::written_as`] answers `None`. Removing this type, the +/// `extract_*_planned` variants that take it and tar's second write path is +/// follow-up work, deliberately separate because that path carries the +/// containment guard and should not be moved by a commit about naming. #[derive(Debug, Clone, Default)] pub(crate) struct NamePlan { /// Only the entries whose name changed. An entry that is absent is written @@ -685,55 +749,55 @@ impl NamePlan { } } -/// Work out what each name in `names` becomes, and refuse the two situations -/// nothing downstream could recover from. -pub(crate) fn plan_names>( +/// Refuse a listing holding a name this filesystem cannot write exactly as the +/// archive spells it. +/// +/// **Nothing is renamed.** This used to be a planning pass: it applied the +/// caller's replacements, dropped a trailing dot or space and suffixed a +/// reserved device, so an archive Windows could not hold was extracted anyway +/// under names it could. That traded one problem for a worse one. The file on +/// disk was then not the file the archive named, nothing downstream could tell +/// the two apart, and the listing handed back was the only record that a +/// substitution had happened at all. An extraction either reproduces what the +/// archive says or it does not happen. +/// +/// So the question is now the narrow one [`NameRules::can_write`] answers, and +/// the answer is yes or no rather than a rewrite: can this host hold this +/// component, spelled this way. A component it would refuse (`what?.txt` on +/// Windows), silently store elsewhere (`notes.txt:hidden`, an NTFS stream), +/// silently rename (`notes.txt.`, whose trailing dot is dropped) or resolve to +/// a device (`CON`) all fail the same way, because from the caller's side they +/// are the same failure: the name they asked for is not the name they would +/// get. +/// +/// The whole listing is judged before a byte is written, so a refusal leaves +/// the output directory exactly as it found it. +/// +/// The first offending component stops it. Reporting every one at once is a +/// front end's job, and both have the listing to do it from +/// ([`NameReport::of`]), which is also what lets them ask before extracting +/// rather than after failing. +pub(crate) fn refuse_unwritable_names>( names: &[S], rules: NameRules, - replacements: &Substitutions, -) -> Result { - let mut rewritten = HashMap::new(); - // planned name -> the first entry that claimed it, and whether that entry's - // name had to change to claim it. - let mut claimed: HashMap = HashMap::new(); - +) -> Result<(), NameError> { for name in names { let name = name.as_ref(); - let planned = rules.rewrite_entry(name, replacements)?; - if planned.as_os_str().is_empty() { - // Nothing normal in it (`.`, a bare root). Containment decides what - // becomes of those, per format, and it is not this pass's business. - continue; - } - let changed = planned != natural_path(name); - - if let Some((first, first_changed)) = claimed.get(&planned) { - // Two entries spelled the same way is an archive that was already - // like that, and extraction has always let the second win. Refusing - // it here would start rejecting archives that have nothing to do - // with this feature; a collision is only ours when a rewrite caused - // it. - if *first != name && (changed || *first_changed) { - return Err(NameError::Collision { - first: (*first).to_string(), - second: name.to_string(), - name: planned.to_string_lossy().into_owned(), + // Component by component, and split on `/` rather than by the host's + // rules: an archive entry name means the same thing on every machine, + // which is the property `entry_components` exists to preserve. + for component in entry_components(name) { + let problems = rules.problems(component); + if !problems.is_empty() { + return Err(NameError::Unwritable { + entry: name.to_string(), + component: component.to_string(), + problems, }); } } - claimed.insert(planned.clone(), (name, changed)); - if changed { - rewritten.insert(name.to_string(), planned); - } } - - Ok(NamePlan { rewritten }) -} - -/// The relative path an extractor derives from an entry name with no rules -/// applied: its `Normal` components and nothing else. -fn natural_path(name: &str) -> PathBuf { - entry_components(name).collect() + Ok(()) } /// Split an archive entry name into its components, the same way on every host. diff --git a/apps/core/tests/names.rs b/apps/core/tests/names.rs index 73619f4..304adbe 100644 --- a/apps/core/tests/names.rs +++ b/apps/core/tests/names.rs @@ -124,6 +124,19 @@ fn as_windows(replacements: Substitutions) -> ExtractOptions { .with_replacements(replacements) } +/// The message of the refusal an unwritable entry must produce. +/// +/// Panics naming what happened instead, because the two ways this goes wrong +/// need telling apart: extracting anything at all is the policy being broken, +/// while a different error is usually the fixture failing to build the name. +fn refusal(result: Result, CompressionError>, context: &str) -> String { + match result { + Err(CompressionError::Name(problem @ NameError::Unwritable { .. })) => problem.to_string(), + Err(other) => panic!("{context}: expected a naming refusal, got {other}"), + Ok(files) => panic!("{context}: expected a refusal, extracted {files:?}"), + } +} + // ------------------------------------------------------------- the ruleset -- #[test] @@ -564,55 +577,62 @@ fn inspecting_this_machine_s_own_archives_asks_nothing() { // -------------------------------------------- extracting with the answers --- #[test] -fn the_answers_are_written_and_the_listing_names_what_is_on_disk() { - // Issue #64 end to end, for all three formats. The listing is the half that - // matters most: returning the archive's names would have a front end show - // `what?.txt` next to a file called `what_.txt`. +fn every_fault_stops_the_whole_archive_the_same_way() { + // Issue #64 end to end, for all three formats, under the policy that + // replaced the answers. The four faults used to have four different + // endings: a question put to the user, a trailing run truncated in + // silence, a `_` appended to a device name. They have one ending now, and + // that is the whole of what this pins. + // + // `summary.txt` rides along in every archive and is the assertion that + // matters most. It is perfectly writable on any host, and it must still not + // be on disk: the refusal is judged over the listing before a byte is + // written, so a good entry beside a bad one goes nowhere either. for format in FORMATS { - let dir = tempfile::TempDir::new().unwrap(); - let archive = archive_with( - dir.path(), - format, - &[ - ("summary.txt", b"fine"), - ("what?.txt", b"question"), - ("notes.txt.", b"trailing"), - ("CON.txt", b"device"), - ], - ); - let out = dir.path().join("out"); - - let written = extract_with( - &archive, - &out, - &as_windows(Substitutions::new().with('?', "_")), - ) - .unwrap(); - - let expected = vec![ - "CON_.txt".to_string(), - "notes.txt".to_string(), - "summary.txt".to_string(), - "what_.txt".to_string(), - ]; - assert_eq!(sorted(written), expected, "{format}: the returned listing"); - assert_eq!(files_under(&out), expected, "{format}: what is on disk"); - assert_eq!( - fs::read(out.join("what_.txt")).unwrap(), - b"question", - "{format}: the renamed entry kept its content" - ); + for (bad, fault) in [ + ("what?.txt", "a character the host refuses"), + ("notes.txt:hidden", "a character the host reinterprets"), + ("notes.txt.", "a trailing dot"), + ("CON.txt", "a reserved device"), + ] { + let dir = tempfile::TempDir::new().unwrap(); + let archive = archive_with( + dir.path(), + format, + &[("summary.txt", b"fine"), (bad, b"trouble")], + ); + let out = dir.path().join("out"); + let context = format!("{format}, {fault}"); + + let message = refusal( + extract_with(&archive, &out, &as_windows(Substitutions::new())), + &context, + ); + + assert!(message.contains(bad), "{context}: {message}"); + assert!( + files_under(&out).is_empty(), + "{context}: {:?} was written before the refusal", + files_under(&out) + ); + } } } #[test] -fn a_colon_entry_becomes_a_file_of_its_own_and_leaves_its_neighbour_alone() { +fn a_colon_entry_is_refused_and_its_neighbour_is_never_written() { // Issue #63. On Windows the unfixed path writes these bytes into the // `hidden` stream of `notes.txt`, which changes nothing about `notes.txt` // that `dir` can see and leaves the listing naming a file that exists - // nowhere. Here the answer turns it into a file, and the assertion that - // `notes.txt` still holds its own bytes is what would fail if a future - // "simplification" let the colon through. + // nowhere. + // + // The answer used to be a substitution that turned it into a file of its + // own. That is gone: `notes.txt-hidden` is not the file the archive named + // either, and inventing it hid the problem rather than reporting it. + // + // `notes.txt` is what would catch a future "simplification" letting the + // colon through. It is the innocent half of the archive, and it must not + // exist: on a host that refuses the other entry, nothing is written. for format in FORMATS { let dir = tempfile::TempDir::new().unwrap(); let archive = archive_with( @@ -625,59 +645,34 @@ fn a_colon_entry_becomes_a_file_of_its_own_and_leaves_its_neighbour_alone() { ); let out = dir.path().join("out"); - let written = extract_with( - &archive, - &out, - &as_windows(Substitutions::new().with(':', "-")), - ) - .unwrap(); - - assert_eq!( - sorted(written), - vec!["notes.txt".to_string(), "notes.txt-hidden".to_string()], - "{format}" - ); - assert_eq!(fs::read(out.join("notes.txt")).unwrap(), b"the real file"); - assert_eq!( - fs::read(out.join("notes.txt-hidden")).unwrap(), - b"the payload" - ); - } -} - -#[test] -fn an_entry_with_no_answer_stops_before_anything_is_written() { - // The pre-pass earning its keep: judged one entry at a time, `summary.txt` - // would already be on disk when `what?.txt` was refused, and the user would - // be left with half a directory and no list of what is in it (issue #64's - // other complaint). - for format in FORMATS { - let dir = tempfile::TempDir::new().unwrap(); - let archive = archive_with( - dir.path(), + let message = refusal( + extract_with(&archive, &out, &as_windows(Substitutions::new())), format, - &[("summary.txt", b"fine"), ("what?.txt", b"question")], ); - let out = dir.path().join("out"); - let err = extract_with(&archive, &out, &as_windows(Substitutions::new())).unwrap_err(); - - let message = err.to_string(); - assert!(message.contains("what?.txt"), "{format}: {message}"); - assert!(message.contains('?'), "{format}: {message}"); + assert!(message.contains("notes.txt:hidden"), "{format}: {message}"); + assert!(message.contains(':'), "{format}: {message}"); assert!( - files_under(&out).is_empty(), - "{format}: {:?} was written before the refusal", - files_under(&out) + !out.join("notes.txt").exists(), + "{format}: the neighbour was written despite the refusal" ); + assert!(files_under(&out).is_empty(), "{format}"); } } #[test] -fn two_entries_that_would_land_on_one_name_are_refused_by_name() { - // Deliberately not disambiguated: renaming one of them to `a_b (2).txt` is - // how a user ends up with a file they never look at again. Both names are - // in the message so the answer can be changed. +fn two_entries_that_used_to_collide_are_refused_for_their_own_names() { + // These two were the collision case: `?` and `*` both answered with `_` + // made one name out of two, and the refusal named all three spellings so + // the answer could be changed. + // + // A collision can no longer be manufactured, because nothing is renamed, so + // the guard that looked for one has gone with it. The archive is still + // refused, for the plainer reason that this host cannot write either name. + // + // Only the first offending entry is reported, and the assertion that + // `a_b.txt` is absent is the one that matters: a planned name appearing in + // a message would mean something is still working one out. for format in FORMATS { let dir = tempfile::TempDir::new().unwrap(); let archive = archive_with( @@ -688,24 +683,26 @@ fn two_entries_that_would_land_on_one_name_are_refused_by_name() { let out = dir.path().join("out"); let answers = Substitutions::new().with('?', "_").with('*', "_"); - let err = extract_with(&archive, &out, &as_windows(answers)).unwrap_err(); + let message = refusal(extract_with(&archive, &out, &as_windows(answers)), format); - let message = err.to_string(); assert!(message.contains("a?b.txt"), "{format}: {message}"); - assert!(message.contains("a*b.txt"), "{format}: {message}"); - assert!(message.contains("a_b.txt"), "{format}: {message}"); assert!( - files_under(&out).is_empty(), - "{format}: a collision must leave the output alone" + !message.contains("a_b.txt"), + "{format}: nothing is renamed, so no planned name should appear: {message}" ); + assert!(files_under(&out).is_empty(), "{format}"); } } #[test] -fn a_renamed_entry_colliding_with_an_untouched_one_is_refused_too() { - // The case a "compare the rewritten names to each other" check would miss: - // only one of these two changes, and it lands on a name the archive already - // uses. +fn a_trailing_dot_is_refused_rather_than_folded_onto_the_name_beside_it() { + // `notes.txt.` used to lose its dot and land on `notes.txt`, which the + // archive already holds, and the collision guard existed to catch exactly + // that. Now the dot is never dropped, so the two names stay two names and + // the archive is refused for the one the host will not preserve. + // + // The neighbour is ordinary and still goes nowhere, which is the all-or- + // nothing half of the policy. let dir = tempfile::TempDir::new().unwrap(); let archive = archive_with( dir.path(), @@ -714,9 +711,11 @@ fn a_renamed_entry_colliding_with_an_untouched_one_is_refused_too() { ); let out = dir.path().join("out"); - let err = extract_with(&archive, &out, &as_windows(Substitutions::new())).unwrap_err(); + let message = refusal( + extract_with(&archive, &out, &as_windows(Substitutions::new())), + "zip", + ); - let message = err.to_string(); assert!(message.contains("notes.txt."), "{message}"); assert!(files_under(&out).is_empty()); } @@ -745,26 +744,32 @@ fn an_archive_that_already_names_one_entry_twice_still_extracts() { } #[test] -fn an_answer_the_host_cannot_write_is_refused_before_the_archive_is_opened() { - // Checked against a path that does not exist: if the answer were validated - // per entry instead of up front, this would fail with "no such file" - // instead, and a user would only learn their replacement was no good after - // choosing an archive. - let dir = tempfile::TempDir::new().unwrap(); - let missing = dir.path().join("nowhere.zip"); - let err = extract_with( - &missing, - &dir.path().join("out"), - &as_windows(Substitutions::new().with('?', "<")), - ) - .unwrap_err(); - assert!( - matches!( - err, - collapse_core::CompressionError::Name(NameError::UnwritableReplacement { .. }) - ), - "{err}" - ); +fn an_answer_no_longer_rescues_an_entry_the_host_cannot_write() { + // What this test used to guarantee — that a replacement the host could not + // write was refused up front, before the archive was even opened — has no + // subject left: nothing validates an answer, because nothing applies one. + // + // It is kept, turned around to face the policy itself, because that is the + // part most likely to be quietly undone. `?` answered with `_` is the most + // reasonable answer anyone could give to the most ordinary question this + // ever asked, and it must still extract nothing at all. + for format in FORMATS { + let dir = tempfile::TempDir::new().unwrap(); + let archive = archive_with(dir.path(), format, &[("what?.txt", b"question")]); + let out = dir.path().join("out"); + + let message = refusal( + extract_with( + &archive, + &out, + &as_windows(Substitutions::new().with('?', "_")), + ), + format, + ); + + assert!(message.contains("what?.txt"), "{format}: {message}"); + assert!(files_under(&out).is_empty(), "{format}"); + } } #[test] diff --git a/apps/core/tests/security.rs b/apps/core/tests/security.rs index 28c433c..f84ca8f 100644 --- a/apps/core/tests/security.rs +++ b/apps/core/tests/security.rs @@ -11,7 +11,7 @@ use collapse_core::compression::{ compress_7z_dir, compress_tar_dir, compress_zip_dir, extract_7z, extract_tar, extract_zip, NameRules, Substitutions, }; -use collapse_core::{extract, extract_with, Algorithm, ExtractOptions, Verify}; +use collapse_core::{extract, extract_with, Algorithm, CompressionError, ExtractOptions, Verify}; use sevenz_rust2::{SevenZArchiveEntry, SevenZWriter}; use tar::{Builder, EntryType, Header}; use zip::write::SimpleFileOptions; @@ -628,10 +628,17 @@ fn no_format_writes_an_entry_name_with_a_colon_as_a_stream() { #[test] fn a_replacement_cannot_carry_an_entry_out_of_the_output_directory() { - // The answer the user gives is put inside a name that containment has - // already cleared, so an unchecked replacement is a traversal by the back - // door: `?` answered with `../..` would write above the output directory - // with nothing left to notice it. + // The answer the user gave used to be pushed inside a name that containment + // had already cleared, so an unchecked replacement was a traversal by the + // back door: `?` answered with `../..` would write above the output + // directory with nothing left to notice it. `check_replacements` stood in + // front of that. + // + // The hole is now closed a layer earlier and by construction rather than by + // a check: an answer is never applied to anything, so it cannot reach a + // path at all, and the entry that would have carried it is refused for its + // own name. The hostile answers are still offered here, and must still + // change nothing. for (ext, build) in [ ("zip", malicious_zip as fn(&Path, &str)), ("7z", malicious_7z), @@ -737,7 +744,17 @@ fn no_format_writes_through_a_symlink_already_in_the_output() { /// planned rename must not be able to land outside either. #[cfg(unix)] #[test] -fn a_renamed_entry_cannot_be_written_through_such_a_symlink() { +fn an_entry_that_cannot_be_named_is_refused_before_any_symlink_is_followed() { + // This entry used to take a second write path. Windows rules made the `?` a + // question, the answer renamed it, and the renamed branch wrote through a + // route that `unpack_in`'s own containment check never saw — so it needed + // its own proof that a symlink already sitting in the output could not be + // followed. + // + // That branch is unreachable now: the name is refused and nothing is + // written by any path. The test stays because the thing it guards is the + // file outside the output directory, and that assertion does not care which + // of the two reasons kept it safe. for ext in ["zip", "7z", "tar"] { let dir = tempfile::TempDir::new().unwrap(); let outside = dir.path().join("outside"); @@ -753,12 +770,8 @@ fn a_renamed_entry_cannot_be_written_through_such_a_symlink() { _ => malicious_tar(&archive, "link/ev?l.txt"), } - // Windows rules make the `?` a question, so this entry is renamed and - // takes the planned write path rather than the untouched one. - let options = ExtractOptions::new() - .with_rules(NameRules::windows()) - .with_replacements(Substitutions::new().with('?', "i")); - let escaped = outside.join("evil.txt"); + let options = ExtractOptions::new().with_rules(NameRules::windows()); + let escaped = outside.join("ev?l.txt"); assert_contained(extract_with(&archive, &out, &options), &escaped); } } @@ -894,18 +907,29 @@ fn the_same_archive_extracts_normally_somewhere_else() { /// lands exactly on the archive. Checking the archive's own spelling would /// miss it. #[test] -fn the_check_follows_the_renamed_name_not_the_archive_s() { +fn an_entry_that_cannot_be_named_never_reaches_the_archive_it_would_overwrite() { + // This used to be the case for following the *planned* name: `v?.zip` + // answered with `_` became `v_.zip`, which is the archive being read, and + // the overwrite guard had to see that even though the archive's own name + // matched no entry. + // + // Nothing is renamed any more, so that arrangement cannot be built. What + // is left is the ordering, and it is worth pinning: the naming refusal + // comes first, so the archive is never opened for writing at all. The + // guarantee a person cares about is unchanged and is the last assertion — + // the archive is still there, byte for byte. let dir = tempfile::TempDir::new().unwrap(); let archive = dir.path().join("v_.zip"); malicious_zip(&archive, "v?.zip"); let before = std::fs::read(&archive).unwrap(); - let options = ExtractOptions::new() - .with_rules(NameRules::windows()) - .with_replacements(Substitutions::new().with('?', "_")); + let options = ExtractOptions::new().with_rules(NameRules::windows()); let err = extract_with(&archive, dir.path(), &options) - .expect_err("the planned name lands on the archive"); - assert!(err.to_string().contains("over the archive itself"), "{err}"); + .expect_err("a name this host cannot write is refused"); + assert!( + matches!(err, CompressionError::Name(_)), + "refused for the naming reason, before the overwrite check: {err}" + ); assert_eq!(std::fs::read(&archive).unwrap(), before); } diff --git a/apps/desktop/src-tauri/src/names.rs b/apps/desktop/src-tauri/src/names.rs index 59f7ab4..d66d887 100644 --- a/apps/desktop/src-tauri/src/names.rs +++ b/apps/desktop/src-tauri/src/names.rs @@ -47,9 +47,11 @@ pub struct NameInspection { /// /// Sent as data rather than reimplemented in JavaScript. The dialog only /// picks the wording; whether a character is acceptable is answered here by - /// the same ruleset the extraction will use, and the extraction checks - /// again regardless (`extract_with` validates the answers before it opens - /// the archive). + /// the same ruleset the extraction will use. + /// + /// Nothing downstream re-checks it any more, and nothing needs to: core no + /// longer substitutes anything, so an answer cannot reach a file name. This + /// whole exchange is inert until the dialog is taken out. pub rejected_in_replacement: String, } diff --git a/apps/desktop/src-tauri/tests/names.rs b/apps/desktop/src-tauri/tests/names.rs index 4941c3c..8540d25 100644 --- a/apps/desktop/src-tauri/tests/names.rs +++ b/apps/desktop/src-tauri/tests/names.rs @@ -316,21 +316,27 @@ fn an_ordinary_archive_extracts_with_no_answers_at_all() { } #[test] -fn an_answer_containing_a_separator_is_refused_before_anything_is_written() { - // `?` is writable on this host, so nothing in this archive needs answering: - // the answer is refused on its own account, by the ruleset, before the - // archive is opened. That is what keeps a bad answer from being discovered - // half way through an extraction. +fn a_hostile_answer_changes_nothing_because_no_answer_is_applied() { + // This used to be the guard on the answer itself: `?` replaced with + // `../escaped` would have carried an entry out of the output directory, so + // the ruleset refused the answer before the archive was opened. + // + // There is no answer to refuse any more. Nothing in this archive is + // unwritable, so it extracts, and the point is what does *not* happen: the + // replacement reaches no name, creates no directory and moves nothing. A + // regression here would show up as `escaped` existing somewhere. let dir = TempDir::new().unwrap(); let archive = zip_with(dir.path(), &[("notes.txt", b"hello")]); let out = dir.path().join("out"); - let problem = refusal(extract_answering(&archive, &out, &[("?", "../escaped")]).unwrap()); + let outcome = extract_answering(&archive, &out, &[("?", "../escaped")]).unwrap(); - assert!(problem.contains("path separator"), "{problem}"); + assert_eq!(written(outcome), ["notes.txt"]); + assert_eq!(files_under(&out), ["notes.txt"]); assert!( - !out.exists(), - "the output directory was created before the answer was judged" + !dir.path().join("escaped").exists() && !out.join("escaped").exists(), + "the answer reached a path: {:?}", + files_under(dir.path()) ); } @@ -381,38 +387,40 @@ fn a_name_this_computer_cannot_write_is_reported_with_the_character_to_ask_about #[cfg(unix)] #[test] -fn the_answer_is_written_and_the_listing_names_what_is_on_disk() { - let dir = TempDir::new().unwrap(); - let archive = zip_with(dir.path(), &[(UNWRITABLE_HERE, b"hello")]); - let out = dir.path().join("out"); - - let outcome = extract_answering(&archive, &out, &[("\u{0}", "_")]).unwrap(); - - // The name on disk, never the archive's: reporting `bad\0name.txt` would - // name a file that exists nowhere and send the user looking for it. - assert_eq!(written(outcome), ["bad_name.txt"]); - assert_eq!(files_under(&out), ["bad_name.txt"]); - assert_eq!(fs::read(out.join("bad_name.txt")).unwrap(), b"hello"); -} - -#[cfg(unix)] -#[test] -fn an_empty_answer_removes_the_character() { - let dir = TempDir::new().unwrap(); - let archive = zip_with(dir.path(), &[(UNWRITABLE_HERE, b"hello")]); - let out = dir.path().join("out"); - - let outcome = extract_answering(&archive, &out, &[("\u{0}", "")]).unwrap(); - - assert_eq!(written(outcome), ["badname.txt"]); +fn no_answer_rescues_a_name_this_computer_cannot_write() { + // Both halves of what the dialog used to offer, and neither works now: a + // character to put in its place, and an empty answer meaning "just drop + // it". The name the archive gave is the only name that may be written, so + // an entry holding a NUL does not arrive under some other spelling — it + // does not arrive. + // + // This is the test to look at if the dialog is ever wired back up by + // accident: it is the one that says the answers are inert. + for answer in ["_", ""] { + let dir = TempDir::new().unwrap(); + let archive = zip_with(dir.path(), &[(UNWRITABLE_HERE, b"hello")]); + let out = dir.path().join("out"); + + let problem = refusal(extract_answering(&archive, &out, &[("\u{0}", answer)]).unwrap()); + + assert!( + problem.contains("cannot be written on this system"), + "answer {answer:?}: {problem}" + ); + assert!( + !out.exists(), + "answer {answer:?} wrote {:?}", + files_under(&out) + ); + } } #[cfg(unix)] #[test] -fn a_name_left_unanswered_stops_before_anything_is_written() { +fn a_name_this_computer_cannot_write_stops_before_anything_is_written() { // The other entry is perfectly writable and still does not get written: - // extraction settles every name from the listing before the first byte, so - // a user who dismissed the dialog is not left with half an archive. + // every name is judged from the listing before the first byte, so a user is + // never left with half an archive and no way to tell which half. let dir = TempDir::new().unwrap(); let archive = zip_with( dir.path(), @@ -423,40 +431,49 @@ fn a_name_left_unanswered_stops_before_anything_is_written() { let problem = refusal(extract_answering(&archive, &out, &[]).unwrap()); assert!( - problem.contains("no replacement for it was given"), + problem.contains("cannot be written on this system"), "{problem}" ); assert!(problem.contains("bad"), "the entry names itself: {problem}"); assert!( !out.exists(), - "an unanswered name wrote {:?}", + "an unwritable name wrote {:?}", files_under(&out) ); } #[cfg(unix)] #[test] -fn an_answer_this_computer_cannot_write_either_is_refused_with_the_reason() { +fn the_refusal_says_which_character_is_the_problem() { + // The reason has to travel, because the dialog renders it and it is all the + // user gets: naming the entry without naming the character leaves them + // staring at a file name that looks fine, since a NUL prints as nothing. let dir = TempDir::new().unwrap(); let archive = zip_with(dir.path(), &[(UNWRITABLE_HERE, b"hello")]); let out = dir.path().join("out"); - // Replacing the NUL with a NUL is not an answer, and it is caught before - // the archive is opened rather than by the write failing. - let problem = refusal(extract_answering(&archive, &out, &[("\u{0}", "a\u{0}b")]).unwrap()); + let problem = refusal(extract_answering(&archive, &out, &[]).unwrap()); - assert!(problem.contains("cannot write"), "{problem}"); + assert!(problem.contains("refuses in a file name"), "{problem}"); + assert!( + problem.contains("\\0"), + "the character is shown escaped: {problem}" + ); assert!(!out.exists()); } #[cfg(unix)] #[test] -fn two_entries_that_would_land_on_one_name_are_refused_naming_both() { - // Renaming one of them behind the user's back is how a file disappears - // without anyone noticing, so the answer is refused and both names are - // said out loud. The second entry here is one the host could write - // perfectly well: the collision is created by the answer, not found in the - // archive. +fn an_entry_beside_the_name_it_would_have_taken_is_still_just_refused() { + // This was the collision case: answering the NUL with `_` made this entry + // into `bad_name.txt`, which the archive already holds, and one of the two + // would have been written over the other. Both names went into the refusal + // so the answer could be changed. + // + // Nothing is renamed now, so the two names stay two names and there is no + // collision to find. The archive is refused for the NUL alone, and + // `bad_name.txt` — an entry this host writes perfectly well — is not + // written either, which is the part worth keeping. let dir = TempDir::new().unwrap(); let archive = zip_with( dir.path(), @@ -466,11 +483,9 @@ fn two_entries_that_would_land_on_one_name_are_refused_naming_both() { let problem = refusal(extract_answering(&archive, &out, &[("\u{0}", "_")]).unwrap()); - assert!(problem.contains("bad_name.txt"), "{problem}"); - assert!(problem.contains("both be written as"), "{problem}"); - assert!(!out.exists(), "a collision wrote {:?}", files_under(&out)); - - // And an answer that keeps them apart goes through. - let outcome = extract_answering(&archive, &out, &[("\u{0}", "-")]).unwrap(); - assert_eq!(written(outcome), ["bad-name.txt", "bad_name.txt"]); + assert!( + problem.contains("cannot be written on this system"), + "{problem}" + ); + assert!(!out.exists(), "wrote {:?}", files_under(&out)); } diff --git a/docs/architecture.md b/docs/architecture.md index 438eb23..d15f712 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -82,19 +82,35 @@ any platform, so every Windows rule is tested from a Mac, and only `NameRules::host()` is chosen by the compiler. A rule reachable solely under `#[cfg(windows)]` is a rule this repository cannot test. -It answers with structure rather than a string, because a front end has to -render it: each offending character (and whether the host **rejects** it or -**reinterprets** it, which is the difference between a colon failing and a -colon quietly becoming an NTFS stream), a trailing dot or space, and a reserved -device name. Only the first is a question for the user; the other two are -adjustments that need explaining, not answering. - -`unwritable_names` inspects a listing without extracting, and `extract_with` -takes the answers. Replacements are applied **before** the structural -adjustments, or `CO?1` answered with `M` would be left as the device `COM1`. -An answer that is itself unwritable, carries a path separator, or empties a -component is refused, and a collision is refused naming both entries rather -than silently renaming one. +**A name that does not fit stops the extraction.** Nothing is renamed to make it +fit, and nothing is put to the user as a question: an archive arrives under the +names it carries or it does not arrive. `refuse_unwritable_names` judges the +whole listing before the first byte and fails on the first component +`NameRules::can_write` rejects, so the output directory is left as it was found, +the writable entries included. + +The four faults it knows about are one fault as far as a caller is concerned — +a character the host **rejects** (`?`), one it **reinterprets** (`:`, which is +the difference between a write failing and a write quietly becoming an NTFS +stream), a trailing dot or space, and a reserved device name. Each would leave a +file under a name the archive did not ask for, and that is the whole of what is +being prevented. +[threat_model.md](threat_model.md#4b-entry-names-this-host-cannot-write) records +why this replaced a scheme that adjusted and asked instead. + +It still answers with **structure** rather than a string, because the refusal has +to be rendered: `unwritable_names` inspects a listing without extracting +anything and reports every entry this host would refuse and why, so a front end +can say what will not work before anyone waits for it. The CLI names every +offending entry at once from that report; core stops at the first, the run being +over either way. + +`Substitutions` and `ExtractOptions::with_replacements` survive as inert +surface, so the two front ends still compile while their naming dialogs are +taken out. `NamePlan` and the `extract_*_planned` backend variants are likewise +vestigial: nothing constructs a plan that says anything. Removing all of it, +tar's second write path included, is follow-up work kept out of the commit that +changed the policy, because that path carries the containment guard. ### `src/paths.rs` — the guards both front ends share @@ -160,7 +176,8 @@ effects — no subprocess needed. ### Command surface ``` -collapse compress [-f 7z|zip|tar] [-l 1-5] [-o ] [--force] [--server ] +collapse compress [-f 7z|zip|tar] [-l 1-5] [-o ] [--force] + [--verify] [--server ] collapse extract [-o ] ``` @@ -202,11 +219,29 @@ Aliases `c` / `e`. The CLI-local `Format` enum (`clap::ValueEnum`) converts to handles files and directories alike; the archive lands at the same output path local mode would use. The safety guards in step 4 run before any network I/O. Extraction has no remote mode. +6. **Read the archive back.** Every local compression is checked, and `--verify` + only chooses how deeply: `Verify::Index` by default, `Verify::Contents` with + the flag. The depth is bound once and then both handed to the engine and + reported, so an `Outcome` cannot name a check that did not happen. The flag + is refused alongside `--server`, because the archive is built on the far side + and the protocol has no way to ask for the deeper check; the server takes its + own `verify=` parameter instead. What each depth buys per format is in + `compression/verify.rs`, and the difference is not cosmetic: tar stores no + checksum over an entry's data at all. Extraction (`run_extract`) resolves the output directory (default the current directory) and calls `collapse_core::extract`, which creates the directory tree as needed. +Ahead of that it reads the listing once through `unwritable_names_with` and +refuses an archive holding any name this host cannot write, which core would +refuse anyway. The duplication is deliberate and is the reason the CLI's message +is worth having: core stops at the first offending component, while this has the +whole listing and names every entry at fault in one go, so a user is not refused +four times over four runs. A listing this build cannot read is deliberately not +this check's business — extraction is about to open the same archive and fail on +it in the extractor's own vocabulary, which is the message worth keeping. + ## collapse-remote — the client for a remote server A small crate holding the client side of the server's job flow: @@ -438,15 +473,21 @@ HTTP, same engine as the CLI. It compresses files and folders and extracts archives, in the cervantic visual style (warm cream + terracotta, monospace), and targets macOS, Windows and Linux from one codebase. -The backend exposes four Tauri commands: `is_directory` (UI icon/name hint), +The backend exposes five Tauri commands: `is_directory` (UI icon/name hint), `compress_path` (dispatches file vs. folder, refuses to overwrite its own source or a file inside the folder being compressed, replaces an existing output only when `overwrite` says the user agreed to it in the save dialog, and hands the work to a remote server when one is chosen), -`extract_archive`, and `check_server` (a health probe for the settings panel). +`extract_archive`, `check_server` (a health probe for the settings panel), and +`unwritable_names`, which reports the entry names this machine cannot write. + +That last one, and the dialog it feeds, are **inert**: extraction refuses such +an archive outright rather than taking answers, so the replacements the dialog +collects reach nothing. Both go when the dialog does. -All three of those carry `#[tauri::command(async)]`. A bare `#[tauri::command]` -on a synchronous function runs the body on the thread handling the IPC message, +All four of the blocking ones carry `#[tauri::command(async)]`. A bare +`#[tauri::command]` on a synchronous function runs the body on the thread +handling the IPC message, so the window stops repainting until it returns: a compression froze it for its whole duration, and a mistyped server address froze it for the 30 seconds of ureq's connect timeout. `is_directory` is the exception, being a single `stat`. @@ -509,7 +550,7 @@ outside. Source files carry no inline `#[cfg(test)] mod tests`. ## CI -`.github/workflows/test-and-build.yml` runs on every push to `main`/`dev` and +`.github/workflows/test-and-build.yml` runs on every push to `main`/`develop` and on pull requests, entirely on Linux runners. Every job is named `test ()` or `build ()`, so a check's name says what it does and which app it belongs to; the job ids match those names (`test-cli`, @@ -521,14 +562,17 @@ for four minutes. It is there because merging two individually well formatted branches can still produce an unformatted tree, and only a check on the merged result sees that. Per app, tests gate the build: `test (core)` (`make core/test`) gates `test (remote)`, `test (cli)`, `test (server-backend)` and -`test (desktop)` (the Tauri IPC is mocked, so that one needs Node only), while +`test (desktop)` (the Tauri IPC is mocked, so that one needs Node only) and +`test (desktop, rust)` (the `src-tauri` suite, which needs the Node toolchain to +build the bundle `generate_context!()` embeds, plus the webkit system libraries, +and is the only Linux job that compiles the Tauri crate at all), while `test (server-frontend)` is independent of the Rust engine and waits only on `fmt`. **Linux runs everything, on every push and every pull request.** macOS and Windows run the whole Rust suite too, as `test (rust, macos)` and -`test (rust, windows)`, but only for the branches that ship: any push to `dev` -or `main`, and any pull request from `dev`, from `main`, or from a `release/*` +`test (rust, windows)`, but only for the branches that ship: any push to `develop` +or `main`, and any pull request from `develop`, from `main`, or from a `release/*` branch. A `feature/*` or `hotfix/*` pull request stays Linux only and picks -them up when it lands on `dev`, because those runners cost several times a +them up when it lands on `develop`, because those runners cost several times a Linux one per minute. When such a branch does touch platform-specific code, labelling the pull request `full-matrix` asks for them anyway. @@ -541,13 +585,13 @@ a developer's own machine ever ran the desktop suite there. The desktop app is compiled on Linux everywhere as `build (desktop)`, and on the other two platforms as `build (desktop, macos)` and `build (desktop, -windows)` **on the release path only**: a `release/*` branch, and the `dev` to +windows)` **on the release path only**: a `release/*` branch, and the `develop` to `main` pull request itself. That second one matters, since that pull request is -the release gate and its head branch is `dev` rather than `release/*`, so +the release gate and its head branch is `develop` rather than `release/*`, so skipping there would mean the app was never compiled for macOS or Windows on the commit about to ship. -Not on `dev`, deliberately. The cross-platform test jobs already compile this +Not on `develop`, deliberately. The cross-platform test jobs already compile this crate on both platforms, since `make desktop/test-rust` runs `cargo test` inside `src-tauri`, which goes through `generate_context!()`, runs `build.rs` and links executables. All the extra build adds is the release profile (`lto`, diff --git a/docs/deployment.md b/docs/deployment.md index a9310e0..4971476 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -13,23 +13,28 @@ pipeline and [desktop.md](desktop.md) for the desktop bundle specifics. The landing is published to a **build branch** that holds only the compiled site (no source). Which branch depends on where the change landed: -| Source branch | Deploys to | Environment | Purpose | -|---------------|----------------------|-------------|-------------------------------------------| -| `dev` | `pages/landing-dev` | staging | preview changes before they reach `main` | -| `main` | `pages/landing` | production | the live site | +| Source branch | Deploys to | Environment | Purpose | +|---------------|---------------------|-------------|------------------------------------------| +| `develop` | `pages/landing-dev` | staging | preview changes before they reach `main` | +| `main` | `pages/landing` | production | the live site | -So the normal flow is: iterate on `dev` → check the result on `pages/landing-dev` -→ merge `dev` into `main` (a release) → production updates on `pages/landing`. +The staging branch keeps its `-dev` name now that the source branch is +`develop`. It is the branch that serves the staging site, so renaming it would +change that URL and buy nothing; the name is a label for the environment rather +than for the branch it is built from. + +So the normal flow is: iterate on `develop` → check the result on `pages/landing-dev` +→ merge `develop` into `main` (a release) → production updates on `pages/landing`. ``` -commit to dev ──▶ CI build ──▶ pages/landing-dev (staging) -merge to main ──▶ CI build ──▶ pages/landing (production) +commit to develop ──▶ CI build ──▶ pages/landing-dev (staging) +merge to main ──▶ CI build ──▶ pages/landing (production) ``` ## How it works The workflow is [`.github/workflows/deploy-landing.yml`](../.github/workflows/deploy-landing.yml). -On every push to `main` or `dev` (and via manual **Run workflow** / `workflow_dispatch`): +On every push to `main` or `develop` (and via manual **Run workflow** / `workflow_dispatch`): 1. **Build** — `make landing/build`, which runs `npm ci` then `nuxt generate`, producing the static site in `apps/landing/.output/public`. @@ -39,7 +44,7 @@ On every push to `main` or `dev` (and via manual **Run workflow** / `workflow_di branch: `main` → `pages/landing`, anything else → `pages/landing-dev`. The deploy commit is authored as the repo owner (via the account's GitHub noreply -email), not a bot. Concurrency is keyed per source branch, so a `dev` deploy and a +email), not a bot. Concurrency is keyed per source branch, so a `develop` deploy and a `main` deploy never cancel each other. Because each deploy force-pushes an orphan-style commit, the build branches never diff --git a/docs/desktop.md b/docs/desktop.md index bde473b..4a04ff0 100644 --- a/docs/desktop.md +++ b/docs/desktop.md @@ -25,9 +25,14 @@ apps/desktop/ src-tauri/ Rust backend src/lib.rs app wiring: the plugin, the command registry, run() src/commands.rs the commands the webview invokes: is_directory, - compress_path, extract_archive, check_server + compress_path, extract_archive, check_server, + unwritable_names src/paths.rs same_file and inside, the guards that stop an archive overwriting its own source or a file within it + src/names.rs the entry-name exchange the extract dialog was built on. + Inert: extraction refuses a name this host cannot write + rather than taking a replacement for it, so the answers + the dialog collects reach nothing. Goes when it does. tests/ Cargo integration tests (see below) tauri.conf.json window, bundle, identifier (com.cervantic.collapse) capabilities/ window permissions (core + dialog) diff --git a/docs/git_flow.md b/docs/git_flow.md index 01686ee..d765b74 100644 --- a/docs/git_flow.md +++ b/docs/git_flow.md @@ -7,15 +7,15 @@ This repository follows a **gitflow** branching model, kept deliberately simple. Where humans commit code: - **`main`** — released versions only. Every merge into `main` marks a version and gets a tag (`vX.Y.Z`). Changes arrive only through pull requests (see [Branch protections](#branch-protections)). -- **`dev`** — the integration branch where real day-to-day work happens. All work lands here first, always through a pull request from a gitflow branch; direct pushes are blocked. -- **`feature/`** — branched off `dev` for each unit of work, merged back into `dev` via pull request when done. Since direct pushes to `dev` are blocked, even small changes travel on a short-lived feature branch. +- **`develop`** — the integration branch where real day-to-day work happens. All work lands here first, always through a pull request from a gitflow branch; direct pushes are blocked. +- **`feature/`** — branched off `develop` for each unit of work, merged back into `develop` via pull request when done. Since direct pushes to `develop` are blocked, even small changes travel on a short-lived feature branch. - **`release/`** — the version bump and any last touches before a - release, branched off `dev` and merged back into `dev` by pull request, after - which `dev` goes to `main` as usual. Mechanically a feature branch; the name + release, branched off `develop` and merged back into `develop` by pull request, after + which `develop` goes to `main` as usual. Mechanically a feature branch; the name is what makes CI run the macOS and Windows suites on it, which is the point of using it for a release rather than `feature/release-`. -- **`hotfix/`** — an urgent fix for a released version. Mechanically a feature branch (off `dev`, merged into `dev` by PR); the name signals urgency, and a release (`dev` → `main`, new patch version + tag) follows immediately. PRs into `main` accept only `dev`, so there is no direct fix-to-main path; keep `dev` releasable. +- **`hotfix/`** — an urgent fix for a released version. Mechanically a feature branch (off `develop`, merged into `develop` by PR); the name signals urgency, and a release (`develop` → `main`, new patch version + tag) follows immediately. PRs into `main` accept only `develop`, so there is no direct fix-to-main path; keep `develop` releasable. ## Deploy branches (auto-generated — never commit by hand) @@ -23,7 +23,10 @@ CI force-pushes the built landing site to these branches; they contain only buil output (no source), and each deploy replaces the branch wholesale: - **`pages/landing`** — the **production** landing build, published from `main`. -- **`pages/landing-dev`** — the **staging** landing build, published from `dev`, to preview changes before they reach production. +- **`pages/landing-dev`** — the **staging** landing build, published from + `develop`, to preview changes before they reach production. It keeps its + `-dev` name: it is what serves the staging site, and the name describes the + environment rather than the branch the build came from. A ruleset blocks deleting these branches; not pushing to them by hand is a convention (see [Branch protections](#branch-protections)). @@ -39,8 +42,8 @@ bind everyone, including the repo owner: (0 required approvals, solo maintainer); force pushes and deletion are blocked. The required **release source branch** status check ([`gitflow.yml`](../.github/workflows/gitflow.yml)) fails any PR whose head - branch is not `dev`: releasing from `dev` is the only way into `main`. -- **`protect-dev`**: the same for `dev`, plus the required **gitflow branch + branch is not `develop`: releasing from `develop` is the only way into `main`. +- **`protect-develop`**: the same for `develop`, plus the required **gitflow branch name** status check ([`gitflow.yml`](../.github/workflows/gitflow.yml)), which fails any PR whose head branch is not `feature/*`, `hotfix/*`, `release/*`, or @@ -54,9 +57,9 @@ bind everyone, including the repo owner: ## Releasing -When `dev` reaches a state worth shipping, open a pull request from `dev` into `main`, merge it, and tag the merge commit with the version number. `main` therefore only ever moves forward one version at a time. +When `develop` reaches a state worth shipping, open a pull request from `develop` into `main`, merge it, and tag the merge commit with the version number. `main` therefore only ever moves forward one version at a time. -Pushing the tag triggers [`release.yml`](../.github/workflows/release.yml), which builds the CLI binaries (macOS arm64 and Intel tarballs) and the desktop app (one universal macOS `.dmg`, `.deb`/`.rpm`/`.AppImage` builds for x86_64 Linux, and an `.msi` plus NSIS setup `.exe` for x64 Windows), all unsigned for now, and publishes a GitHub release with them, their sha256 checksums, and auto-generated notes. The build also runs the Rust test suite once on the macOS runner — the one shipped OS regular CI never exercises, since CI is Linux-only — so a macOS-only test failure blocks the release. Only exact `vX.Y.Z` tags trigger it, and a guard job refuses to publish when the tagged commit is not on `main` (a tag placed on `dev` by mistake never becomes a release) or when the tag does not match the versions in `apps/cli/Cargo.toml` and `apps/desktop/src-tauri/tauri.conf.json` — bump **both** before tagging, or the CLI would report the wrong `--version` and the desktop bundles (`.dmg`, `.deb`, `.rpm`, `.AppImage`, `.msi`, setup `.exe`) would carry the wrong version in their names. The `workflow_dispatch` trigger dry-runs the builds without publishing anything, even if pointed at a tag. +Pushing the tag triggers [`release.yml`](../.github/workflows/release.yml), which builds the CLI binaries (macOS arm64 and Intel tarballs) and the desktop app (one universal macOS `.dmg`, `.deb`/`.rpm`/`.AppImage` builds for x86_64 Linux, and an `.msi` plus NSIS setup `.exe` for x64 Windows), all unsigned for now, and publishes a GitHub release with them, their sha256 checksums, and auto-generated notes. The build also runs the Rust test suite once on the macOS runner — the one shipped OS regular CI never exercises, since CI is Linux-only — so a macOS-only test failure blocks the release. Only exact `vX.Y.Z` tags trigger it, and a guard job refuses to publish when the tagged commit is not on `main` (a tag placed on `develop` by mistake never becomes a release) or when the tag does not match the versions in `apps/cli/Cargo.toml` and `apps/desktop/src-tauri/tauri.conf.json` — bump **both** before tagging, or the CLI would report the wrong `--version` and the desktop bundles (`.dmg`, `.deb`, `.rpm`, `.AppImage`, `.msi`, setup `.exe`) would carry the wrong version in their names. The `workflow_dispatch` trigger dry-runs the builds without publishing anything, even if pointed at a tag. ## Commits @@ -64,5 +67,5 @@ We favor **small commits with few changes** over large ones: - One logical change per commit — a commit should be explainable in one sentence. - Prefer several small commits over one big commit, even within a single task. -- Red commits are fine on `feature/*`/`hotfix/*` branches while work is in progress (say so in the message); whatever merges into `dev` must pass `cargo test`, so every commit on `dev` and `main` is green. +- Red commits are fine on `feature/*`/`hotfix/*` branches while work is in progress (say so in the message); whatever merges into `develop` must pass `cargo test`, so every commit on `develop` and `main` is green. - Message format: a short imperative summary line, optionally prefixed by area (`core:`, `docs:`, `chore:`), with a body only when the *why* is not obvious from the diff. diff --git a/docs/threat_model.md b/docs/threat_model.md index 709b6b7..d79d56e 100644 --- a/docs/threat_model.md +++ b/docs/threat_model.md @@ -91,7 +91,7 @@ still be exactly one `Normal` component, which closes that at the source. `zip_symlink_entry_is_not_materialized_as_symlink`, `tar_symlink_entry_is_not_materialized`, `no_format_writes_through_a_symlink_already_in_the_output`, -`a_renamed_entry_cannot_be_written_through_such_a_symlink`, +`an_entry_that_cannot_be_named_is_refused_before_any_symlink_is_followed`, `a_colon_in_a_later_component_cannot_clear_the_path_being_built`. ### 3. Hardlink escape (tar) @@ -135,9 +135,12 @@ hardlink is a second name for one file and never resolves to the same string, which is precisely how `--force` was once able to overwrite its own source on the compression side. -The check follows the **planned** name rather than the archive's spelling, since -a rename can land an entry on the archive that the archive's own name does not -match. +The check compares the name the archive spells, because that is the only name an +entry can be written under. It used to have to follow a *planned* name as well: +while extraction still renamed entries to fit the host, a substitution could +land one on the archive that the archive's own name did not match. Nothing +renames an entry any more (see [4b](#4b-entry-names-this-host-cannot-write)), so +the two names are one and the guard got simpler rather than weaker. This is the mirror of a guard compression has always had (`OutputIsSource`, and an output inside the folder being archived, neither of which `--force` unlocks). @@ -146,7 +149,7 @@ positions on the same question (issue #96). **Covered by** `no_format_writes_an_entry_over_the_archive_it_is_reading`, `a_hardlink_to_the_archive_is_not_a_way_around_it`, -`the_check_follows_the_renamed_name_not_the_archive_s`, and +`an_entry_that_cannot_be_named_never_reaches_the_archive_it_would_overwrite`, and `the_same_archive_extracts_normally_somewhere_else`, which is the one that stops the guard from being fixed by refusing too much. @@ -162,12 +165,34 @@ same family: the host silently gives the file a different name than the archive asked for. **Prevention.** Extraction judges every entry name against the host's rules -before anything is written, and refuses rather than guessing. A character the -host cannot hold is a question for the user, who supplies a replacement; the two -adjustments nobody can be asked about (a trailing run, a device name) are -applied and reported. The whole listing is planned before the first byte, so an -unanswerable name or two entries that would collide leave the output directory -as they were found. +before anything is written, and **refuses**. Not adjusts, and not asks: an +archive arrives under the names it carries or it does not arrive. + +That is a narrowing of what shipped first, and the reason is worth recording, +because the original answer looked like the kinder one. A character the host +could not hold was put to the user as a question and replaced with their answer; +a trailing run and a reserved device name were adjusted without asking, since +neither has anything anyone could be asked about. It worked. What it produced +was a directory tree whose names this program had invented: `what?.txt` arrived +as `what_.txt` and `CON.txt` as `CON_.txt`, and nothing afterwards — not the +returned listing, not a later `collapse compress` of the same folder, not the +person reading it a month later — could tell which names came from the archive +and which from us. An extraction that cannot reproduce what the archive says is +one that should not happen, and saying so is cheaper to live with than a rename +nobody remembers agreeing to. + +So the four faults have one ending now. `NameRules::can_write` is the whole +question — can this host hold this component, spelled exactly this way — and the +first component that fails it stops the run with `NameError::Unwritable`, naming +the entry, the component and the reason. The listing is judged in full before +the first byte, so a refusal leaves the output directory exactly as it found it, +the entries that were perfectly writable included. + +What replaces the question is a prediction. `unwritable_names` reads a listing +and reports every entry this host would refuse, decompressing nothing and +creating nothing, so a front end can say what will not work before anybody waits +for it. The CLI surveys the whole listing and names every offending entry at +once; core stops at the first, since by then the run is over either way. The rules are **data**, not `#[cfg]`, so a Mac can be asked what a Windows host would refuse, which is what makes them testable at all: nobody working on this @@ -194,7 +219,7 @@ the user ever seeing the question (issue #89). Recovering what a damaged archive still holds is not gone, it is simply no longer the default: the backends (`extract_tar` and friends) take no options and -never come through the planning pass. +never come through this pass. **Covered by** `apps/core/tests/names.rs`, in particular `an_entry_splits_the_same_way_on_every_host`, @@ -204,6 +229,12 @@ never come through the planning pass. `a_damaged_archive_writes_nothing_rather_than_writing_raw_names` and `the_same_names_are_refused_whether_or_not_the_archive_is_damaged`. +The policy itself is pinned by `every_fault_stops_the_whole_archive_the_same_way`, +which drives all four faults through all three formats and checks that a +writable entry sitting beside a bad one is not written either, and by +`an_answer_no_longer_rescues_an_entry_the_host_cannot_write`, which is the guard +against the substitutions being quietly reintroduced. + ## Compression measures ### 5. Symlinks are never followed out of the tree