Conversation
A bare #[tauri::command] on a synchronous function compiles to what tauri-macros calls the sync path, which runs the body inline on the thread handling the IPC message. Every compression froze the window for its whole duration, and check_server froze it for the 30 seconds of ureq's connect timeout whenever someone mistyped an address in the settings panel. The doc comment on compress_path claimed the opposite, that Tauri ran it on a blocking pool and the window stayed responsive. compress_path, extract_archive and check_server now carry #[tauri::command(async)]. is_directory does not: it is one stat, called while the user is still choosing, so the round trip would cost more than the call. The module header records what the attribute does and, as importantly, what it does not: it moves the body to the async runtime, a multi-thread tokio with one worker per core, not to the blocking pool, so the body occupies a worker for its whole duration. That is fine while the UI runs one operation at a time and disables itself meanwhile; anyone wanting several at once should reach for spawn_blocking rather than add more of these. Pinned by a test in tests/ipc.rs, which is the only place it can be pinned: a command is an ordinary function, so every other test calls these directly and gets the same answer whatever the attribute says. Every command has to appear in one of its two lists, so whether a new one can freeze the window is something someone decided rather than something nobody noticed. Verified falsifiable: removing the attribute from compress_path turns it red.
Take the commands that can block off the UI thread
reap called Storage::delete_job as a statement, forgot the row and counted the job regardless, so a removal that failed left files with no row: exactly the leak reconcile exists to clean up, recreated by the reaper, while the log said it had collected them. Reproduced by making a job's staging directory unwritable and watching the sweep report reaped=1 with the archive still on disk. The conflation is fixed at its source rather than worked around at each call site. delete_job returned a bare bool for two different outcomes, the directory was not there and the removal failed, and it now returns io::Result<bool>: Ok(true) removed it, Ok(false) there was nothing to remove, Err says why. That also closes the gap between the exists check and the removal, which were two separate questions to the filesystem. reap forgets the row and counts it when the files are gone, and on a failure keeps the row, warns with the reason and counts it separately so the next sweep retries. The trade-off is recorded where it is made: a kept row also keeps the startup pass off that directory, so a permanent failure leaks until the reaper wins, which is accepted because the usual failure is transient and a loud retry self-heals it, while forgetting the row would leave the API advertising a job whose archive is half deleted. The log line reports both numbers, following the shape Reconciled already had. The principle was already written in this codebase, in the comment above the same situation in reconcile: a report that claims a cleanup it did not do is worse than one that admits the problem. Closes #78.
sevenz-rust2 implements Display as Debug, and core called to_string() on
it, so a 7z failure reached the user as a struct dump with the absolute
path inside it. The same mistake that gave zip and tar
"IO error: No such file or directory (os error 2)" gave 7z
Io(Os { code: 2, kind: NotFound, message: "..." }, "/Users/...").
One mapping function now covers the dependency's error enum. The
variants that carry an io::Error become CompressionError::Io, so 7z
finally agrees with the other two; the rest get hand-written sentences
with nothing in them that could be a Debug form or a path.
The match is exhaustive on purpose: a dependency bump that adds a
variant should break the build rather than fall through a catch-all back
into a dump.
Two writer.finish() calls lost their map_err entirely, because
SevenZWriter::finish returns io::Result rather than the crate's own
error, so that code was stringifying a plain io::Error for no reason.
The desktop test that special-cased 7z out of its "IO error" assertion
is simpler now that it does not have to.
Layer 2 of the issue, giving CompressionError a #[source] so callers can
tell a corrupt archive from a full disk, is a wider design choice and
stays open.
Refs #66.
…ocal run
The dispatch was server.as_deref().filter(|s| !s.is_empty()), so the two
front ends disagreed about the same string: the desktop turned Some("")
into a local compression and Some(" ") into a network attempt against
an empty address, while the CLI sent both. A stale value in localStorage
or a wrapper script with an unset variable landed wherever the front end
happened to decide.
The answer moves into collapse-remote, which is the argument that
created the crate and the same move that fixed the path guards in
v0.7.0: an address that is blank or only whitespace is rejected at
compress_path and check_health, so no front end can forget, and the
message names the mistake and shows what a real address looks like.
An error rather than a quiet local run, for both. The desktop UI never
sends a blank (urlFor returns null for this computer and a real URL
otherwise), so one can only arrive from a stale stored value or a
caller's bug, and compressing locally would hide it. On the CLI,
--server " " is a flag someone typed wrong, and saying so beats
guessing what they meant.
The two tests that pinned the old contract, one of them marked KNOWN
DEFECT, now assert the refusal.
Closes #65.
434 Rust tests now, 342 of them in the workspace.
The blank guard asked is_empty of the trimmed input but returned the untrimmed one, so two shapes that are not addresses got through. "///" has no whitespace to trim, so it was not blank; trimming its slashes afterwards left an empty base, and every endpoint was joined onto nothing. The user was told "cannot reach the server at : ", naming a server with no name, which is the message issue #65 was filed about, reached by the one road the fix did not cover. And a trailing space defeated the slash trim, so "http://host:8000/ " kept the separator base_url exists to remove. Normalizing before deciding folds both into the same refusal, and the order is the point: trimming the whitespace first is what lets the slash trim see the slash. Found by an adversarial coverage pass over the fix that introduced it, which pinned both as known defects. Those two pins, and a third in tests/client.rs that asserted the nameless-server message where a user meets it, now assert the refusal instead.
An adversarial pass over each fix, starting from the diff rather than
from the tests: enumerate what the change alters, then ask which of
those a test would catch. Every gap below was confirmed by mutation, by
making the change and watching the suite stay green.
reconcile was adapted to the new io::Result and its error arm was
untested, which is the same class of bug this branch fixes, in the
function that inspired the fix. Two mutations survived: counting an
orphan it could not remove, and returning the error, which would make
the server refuse to boot over one unremovable directory.
Reaped::is_quiet was only ever read on an all-zero report, so
self.collected == 0 passed everything: a sweep that collected nothing
but could not remove a job would have logged nothing at all, which is
the exact silence the fix exists to end. No pass had more than one
expired job either, so giving up at the first failure was invisible.
DELETE /jobs/{id} had no test through the HTTP surface for either arm of
its new match.
For 7z, compress_7z_dir's three call sites had no coverage at all: the
whole-directory half of the backend could revert to to_string() on its
own with every test still green. Two existing tests asserted only
is_err(), which cannot tell a sentence from a struct dump.
For the blank address, what "blank" means was never pinned beyond three
characters, the refusal was proven to precede reading a file but not
packing a directory (the ordering the guard's comment justifies itself
with), and dropping the desktop's filter changed what Some(_) means
while only the blank third of that was tested: nothing said a real
address still crosses the wire.
Also strengthened: several assertions used contains where the message is
the contract, and could not have failed for the mutation they looked
like they guarded.
459 Rust tests now, 365 of them in the workspace.
Three small fixes: the reaper's accounting, 7z error messages, blank server addresses
Photos.ZIP was refused as an unknown format. The extension is a file name, not a wire value: Windows and macOS fold case in the filesystem and plenty of tools write .ZIP, so a perfectly good archive was unreadable for the spelling of its name alone. The same match also drives the CLI's format inference, so -o backup.7Z fell through to the zip default and wrote a zip under a name promising a 7z, an archive this same CLI then refused to extract. Case folding is ASCII rather than Unicode: the three extensions are ASCII, and Unicode folding has surprises (Turkish dotless i among them) that have no business deciding an archive format. Two things deliberately left alone, and now pinned so they stay that way. FromStr keeps its strict match: it parses the algorithm= query parameter of POST /compress and the CLI's --format, both wire values with a documented enum, and loosening it would silently widen the API. And extension() keeps returning lowercase, since it names the files this toolkit writes, so a case-insensitive reader can never start producing photos.ZIP. Magic-byte sniffing is a different feature (it buys archives with the wrong extension or none at all) with real decisions attached, and stays in the issue. The desktop test that pinned the old refusal as a KNOWN LIMITATION now asserts the fix, and checks that the complaint about a .7Z holding zip bytes is about the bytes rather than the name, which is what tells the two apart. Core had no test for any of this and now has five; the CLI has one for the inference road. Closes #80.
Read an archive's extension whatever its case
…it in place Issue #70: a compression that fails midway left a partial archive, and for zip and tar the leftover was a VALID archive silently missing entries, because both finalise on drop. The user saw an error, opened the archive, found it opened fine, and could delete the originals. Two pieces, and they only work together. The archive is written to a staging file in the same directory as the output and renamed into place once it is known good. A guard removes it on any early return, so a failure, a panic or a rejected verification cannot leave anything at the destination or beside it. That also ends the hardlink write-through pinned as a KNOWN LIMITATION, and makes a failed run leave a previous archive untouched on the local path as it already did on the remote one. Before that rename, the archive is checked at one of two depths. Verify::Index reads its listing back and confirms it names exactly the entries that were meant to go in, decompressing nothing: it costs milliseconds and it is what catches the bug above. Verify::Contents also decompresses every entry into a sink, never to disk, and is the caller's choice because it roughly doubles the work. What Contents buys differs by format and the doc comment says so rather than implying otherwise: zip carries a CRC32 per entry and 7z a CRC per file, both checked on read, while tar carries nothing over an entry's data, so for tar it can only mean reading every entry through and confirming the archive is well formed. A test pins that difference, so nobody can quietly start claiming more. An enum rather than a bool, because compress(.., 3, true) says nothing where it is called. Verification failures get their own error variant: a caller should be able to tell 'the archive I just wrote is wrong' from 'the compressor failed'. Two latent bugs surfaced while writing the tests and are fixed here. compress_zip created the output before opening the source, which is the documented zero-byte .zip quirk, now gone. And compress_zip_dir named each member in the archive before reading it, so a member that could not be read still appeared as an entry with the CRC of nothing behind it: an archive no reader could fault, holding an empty file where a real one belonged. That one would have defeated Contents verification too, since the checksum matched the emptiness. The CLI gains --verify and the server a verify query parameter, both off by default. They move in this commit because the signature change makes them one edit: the workspace does not compile split apart. For the server that meant a schema migration, since the worker receives only a job id and reads the rest from the registry. SCHEMA_VERSION is 3, following how v1 added server_version: a store written by an older build opens and works, one written by a newer build is still refused. Refs #70.
A Verify row beside Where, Format and Level, off by default, with a hint that says the cheap half always runs: 'The archive's listing is always checked before it is saved.' Without that line the checkbox would read as the difference between checking and not checking, which is not what it is. Disabled when the destination is a server, because the compression happens there and the box would promise something this machine cannot do. Absent in extract mode along with the rest of the compress options, which is what that panel already did. The window is 80px taller so the new row fits without the panel clipping at the default size. Three things had to move together, and nothing type checks the crossing: generate_handler!, the invoke payload in App.vue and the stub switch in the Vitest suite. tests/ipc.rs pins that crossing including parameter types, so its frozen signature table carries the new argument and would have failed if any side had been forgotten. Refs #70.
architecture.md gains what the two depths check and, more usefully, what they cannot: tar carries no checksum over an entry's data, so Contents means something weaker there than it does for zip and 7z. server.md documents the query parameter, registry.md the third schema version and what an older or newer store does on open, and the README counts move to 519 Rust tests.
Verify an archive before it replaces anything
Extracting an archive whose entry name the host cannot write was broken two ways. A name that is legal on Unix but not on Windows died with an opaque OS message and left half a directory behind. And a name holding a colon was worse, because Windows ACCEPTS it: notes.txt:hidden becomes a stream attached to notes.txt rather than a file, silently, and the listing named an entry that existed as no file. Both issues say to decide them together, and both list the same three answers: refuse, sanitize blindly, or skip what cannot be written. The answer taken here is a fourth, and it is why this is worth the size: ask the caller. The engine says what it cannot write and why, and takes a map from offending character to replacement. The rules are data, not cfg. NameRules::windows() can be asked for on 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, and that habit is what left a data-loss guard broken there for months. The Windows rules were read rather than remembered, and the tests pin what memory gets wrong: a device name is matched without its extension (CON.txt is reserved), case insensitively, splitting on the FIRST dot (NUL.tar.gz), and without catching COM10 or CONSOLE.txt. A colon is reported as reinterpreted rather than rejected, because the danger is that the write succeeds. Replacements are applied before the structural adjustments, which is not the obvious order: CO?1 answered with M would otherwise be left as the device COM1. An answer is validated (it cannot be unwritable itself, nor carry a path separator, nor leave an empty component or ..), and a collision is refused naming both entries rather than silently renaming one, which is how a file goes missing without anyone noticing. Extraction returns the names actually written, never the archive's, or a front end would list files that are not on disk. tar needed its own containment guard: it never called sanitize_entry_path, so the check zip and 7z share had no equivalent there. Per-entry write errors now name the entry and the destination. That is piece one of the issue and it helps everywhere: today a read-only output directory, a full disk and a permission error all produce the same blank message. extract(archive, output_dir) keeps working for callers with nothing to answer, so nothing had to change at once. Refs #63, #64.
The flow the feature is for: choose an archive, choose a destination, and if it holds names this computer cannot write, nothing is extracted. A sheet says what is wrong and offers one text field per offending character, prefilled, whatever the number of entries carrying it. Answer and it extracts; cancel and nothing was touched. One field per character rather than per entry, because a hundred files holding a question mark is one question, not a hundred. The problems with no character to replace, a trailing dot or space and a reserved device name, are stated with the adjustment that will be applied rather than given a field that asks nothing. Refusals come back into the sheet rather than out to the error banner: a replacement this computer cannot write either, and a set of answers that would land two entries on one name, both keep the question open with the reason, because they are answerable. Anything else is a real failure and belongs in the banner. Inspecting is its own command now, so the archive is read before a byte of it is written. That is a second crossing of an untyped boundary, and tests/ipc.rs pins both including parameter types. Verified by the Vitest suite rather than by looking: this sheet cannot appear on macOS, since the rules are the host's and macOS writes these names happily. The test that matters asserts extract_archive is never called before the question is answered, which a component that extracted first and asked afterwards would fail while passing everything else.
No dialog here: a CLI cannot ask mid-run without becoming interactive, which this one is not. What it can do is fail well, which it did not. An archive holding a name Windows cannot write died with a bare OS message naming nothing, after writing part of the archive. Extraction now refuses up front, before anything is written, listing the entries it cannot write and why each one is a problem. Ordinary write failures name the entry too, so a read-only output directory and a full disk stop being indistinguishable.
architecture.md gains the new core module, and says the thing worth remembering about it: the rules are data rather than cfg, so the Windows behaviour is testable from any machine. 578 Rust tests now, 463 of them in the workspace.
Ask which characters to replace, instead of failing or guessing
`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.
Split an archive entry the way the format defines, not the host
All eight version strings and the four lockfiles that carry them. The release guard checks only `apps/cli/Cargo.toml` and `tauri.conf.json` (issue #77), so the other six are bumped by hand and by eye until that is fixed. Minor rather than patch: two features landed since v0.7.0 (post-compression verification, and asking the user which characters to substitute in a name the host cannot write), alongside a fix for writing outside the output directory.
Release 0.8.0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release of everything on
devsince v0.7.0: six feature PRs plus the version bump. Version strings and lockfiles are already at 0.8.0, which is what the release guard checks before it will publish.Features
Post-compression verification (#86). An archive is now checked before it replaces anything at
output, at one of two depths. The shallow pass reads the archive's own listing back and compares it against the entries it was meant to hold, which decompresses nothing and catches the failure this exists for: a compression that died half way through and finalised a valid-looking archive anyway. The deep pass decompresses every entry as well, so zip's and 7z's per-entry checksums are checked; tar stores no checksum over entry data, so there it can only confirm the archive is complete and well formed. It roughly doubles the work, which is why it is the user's call: a CLI flag, a desktop checkbox and an HTTP parameter.Entry names the host cannot write (#87). An archive built on Linux can carry names Windows refuses outright or, worse, silently reinterprets. Extraction now judges every name against the host's rules before writing anything and refuses rather than guessing; the desktop asks the user which character should stand in for which, one field per character, and extracts with those answers. The whole listing is planned before the first byte, so an unanswerable name leaves the output directory as it was found.
Fixes
Archive entry names are no longer split with the host's path parser (#88). They were, and
std::pathis#[cfg]-dependent while the rules judging them are data, so the seam was only half portable. On Unix a backslash is a legal file name character that the splitter did not treat as a separator, so collapse could build an archive it then refused to extract, failing after the user might have deleted the originals. On Windows a leading drive-like component was silently discarded, soa:b/c.txtwas reported clean and written asb/c.txt: a hole in the colon handling that #63 exists for, on the only platform #63 concerns. An entry name is not a host path, so the split is now on/everywhere.Path traversal in zip and 7z (#88). Both sanitized an entry name, joined it to the output and wrote, with no check on where the join landed. With a symlink already present in the output directory, an archive holding
link/evil.txtwrote straight through it and returned success. tar was immune becauseunpack_inresolves the parent; zip and 7z now do the same. This one predates v0.7.0 and is fixed for the first time here. It needs a symlink already in the destination, so a hostile archive cannot trigger it alone, but extracting into a directory that already has one is ordinary.Smaller (#85, #84, #81). Archive extensions read case-insensitively, so
Photos.ZIPopens; three small fixes; the Tauri commands moved off the UI thread so the window keeps repainting during a long job.Verification
585 Rust tests and 113 Vitest, on three platforms rather than one. The macOS and Windows legs run on shipping branches, which is what caught #88 in the first place, and both are green on this branch.
Closes #63
Closes #64
Note
Only those two close. Nothing else on the board is finished, and a release does not change that.