Skip to content

fix(backend): harden file IO, decoding and markdown boundaries - #371

Merged
PathGao merged 1 commit into
sftwrdotdev:masterfrom
PathGao:fix/rust-backend-hardening
Aug 2, 2026
Merged

fix(backend): harden file IO, decoding and markdown boundaries#371
PathGao merged 1 commit into
sftwrdotdev:masterfrom
PathGao:fix/rust-backend-hardening

Conversation

@PathGao

@PathGao PathGao commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

Every item below was re-verified against master before being touched; four candidates from the original list turned out to be already fixed upstream and were dropped (the regexes are already LazyLock, and fetch_vscode_theme already has URL validation, status checks and a streaming byte cap).

Saving froze the whole app on slow storage. Nine commands were plain fn, and a synchronous Tauri command runs on the main thread — the file says so itself where window creation depends on it. Every save fsyncs twice, so on an SMB share, iCloud Drive or a slow USB stick the entire UI, all windows, stopped until both returned. They now use spawn_blocking, following open_markdown which already did. create_transfer_window stays async (#360).

The one write designed to survive process death was not atomic. save_window_state used bare fs::write — in the path whose own comment explains it exists because the last window's close kills the storage process. A crash mid-write leaves a half-written session snapshot and every tab is lost on next launch. It, plus the pinned-tag writes found beside it, now use atomic_write.

A file opened or not purely by its size. read_to_string rejects on the first invalid byte while the truncated-preview branch has always decoded leniently, so a legacy-encoded (GBK/Big5/Shift-JIS) document failed at 50 KB and succeeded at 51 KB. One lenient decoder now serves every read path. The preview also used a single read() — only guaranteed to return up to the requested length — and truncated at a raw byte offset, splitting whatever character straddled it. It now takes the full length and drops only an incomplete trailing sequence.

Read-only files were silently rewritten. Replacing an inode needs write permission on the directory, not the file, so chmod 444 was ignored on Unix and the read-only bit restored afterwards — while Windows' MoveFileExW refuses the identical operation. atomic_write now rejects a read-only target.

One stray backtick disabled markdown features for the rest of the document.

Use the ` character to start code.

![[photo.png]]

Then `run()` finishes.

The embed renders as literal text. Inline-code pairing ran across everything between fences, so the prose backtick paired with the opener of the later span and the range between them was treated as code — embeds, wikilinks and highlights all stopped rendering there. CommonMark parses inlines per block; pairing now resets at each blank line.

Embed attributes were not escaped. <img> is built by concatenation with only spaces percent-encoded, so ![[a" onerror="…]] breaks out of the attribute. DOMPurify catches it in the viewer, but the HTML export path writes the markup straight to disk.

Wikilink anchors did not match the ids comrak emits. ## 1. 概述 produced 1.-概述 where comrak renders 1-概述, so the link went nowhere. It now calls comrak's own Anchorizer rather than reimplementing its rules — the test asserts against the ids comrak actually renders, so it fails on drift instead of on a stale copy of the rules.

Also: fetch_vscode_theme had no timeout and bounded zip entries by their declared size rather than the bytes actually produced; watch_file dropped the existing watcher before learning whether the new one could be created; lock().unwrap() is replaced by a helper that recovers from poisoning.

One pre-existing test fixed rather than weakened

multiline_wikilinks_do_not_shift_task_source_positions (from #345) passed by accident: the old id generation kept the embedded newline, preserving the line count. comrak's anchorizer strips it, collapsing two source lines into one and shifting every task checkbox below. Since a heading id can never contain a newline, such a target is unresolvable anyway — multi-line wikilinks are now left literal, which preserves the invariant by construction.

scripts/macosOpenDocumentPaths.test.ts asserted the literal text startup_files.lock().unwrap().push(...). The behaviour is unchanged; only the lock helper is. The assertions were rewritten to match the intent, and each was checked to still match real code rather than passing vacuously.

No frontend contract change

Nine commands went from fn to async fn, but invoke() returns a Promise either way and every call site already awaits. Parameter names, argument types and return types are unchanged.

Validation

  • cargo test — 62 → 76, 0 failed (14 new, including the exact backtick repro above, real GBK bytes, a read-only target, a zip entry whose header understates its size, and the anchor comparison against comrak's own output)
  • cargo clippy --all-targets — 4 → 3 warnings, all pre-existing and untouched; no new ones
  • npm test — 198/198

Needs a decision (not addressed here)

Unifying on lenient decoding makes a GBK file open with replacement characters instead of erroring — and if the user then saves, the mojibake is written back over the original. Strict decoding refused to open it, which was safer but inconsistent. The mainstream answer (VS Code, Typora, Sublime) is neither: detect the encoding and preserve it on save, which needs an encoding_rs dependency and belongs in its own change. This PR fixes the inconsistency; it is not an encoding feature.

Read-only files now return an error rather than saving. VS Code prompts "file is read-only — overwrite?" instead; matching that needs a frontend contract and is left as a follow-up.

**Saving froze the whole app on slow storage.** Nine commands were plain
`fn`, and a synchronous Tauri command runs on the main thread — the file
notes this itself where window creation depends on it. Every save fsyncs
twice, so on an SMB share, iCloud Drive or a slow USB stick the entire UI,
all windows included, stopped until both returned. They now use
`spawn_blocking`, following `open_markdown`, which already did.
`create_transfer_window` stays async (sftwrdotdev#360).

**The one write designed to survive process death was not atomic.**
`save_window_state` used bare `fs::write`, in the path whose own comment
explains it exists because the last window's close kills the storage
process. A crash mid-write leaves a half-written session snapshot and every
tab is lost on the next launch. It, and the pinned-tag writes found beside
it, now use `atomic_write`.

**A file opened or not purely by size.** `read_to_string` rejects on the
first invalid byte while the truncated-preview branch has always decoded
leniently, so a legacy-encoded document failed at 50 KB and succeeded at
51 KB. Every read path now shares one lenient decoder. The preview also read
with a single `read()` — which is only guaranteed to return *up to* the
requested length — and truncated at a raw byte offset, splitting whatever
character straddled it; it now takes the full length and drops only an
incomplete trailing sequence.

**Read-only files were silently rewritten.** Replacing an inode needs write
permission on the directory, not on the file, so `chmod 444` was ignored on
Unix and the read-only bit restored afterwards — while Windows' `MoveFileExW`
refuses the same operation. `atomic_write` now rejects a read-only target.

**One stray backtick disabled markdown features for the rest of the
document.** Inline-code pairing ran across everything between fences, so a
backtick in prose paired with the opener of a later code span and everything
between them was treated as code — embeds, wikilinks and highlights in that
range stopped rendering. CommonMark parses inlines per block, so pairing now
resets at each blank line.

**Embed attributes were not escaped.** `<img>` is built by concatenation
with only spaces percent-encoded, so a quote in a wikilink target broke out
of the attribute. DOMPurify catches this in the viewer; the HTML export path
writes the markup straight to disk.

**Wikilink anchors did not match the ids comrak emits.** `## 1. 概述`
produced `1.-概述` while comrak renders `1-概述`, so the link went nowhere.
It now calls comrak's own `Anchorizer` instead of reimplementing its rules.

Also: `fetch_vscode_theme` had no timeout and bounded zip entries by their
declared size rather than the bytes actually produced; `watch_file` dropped
the existing watcher before learning whether the new one could be created;
and `lock().unwrap()` is replaced by a helper that recovers from poisoning.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@PathGao
PathGao merged commit 37e69f2 into sftwrdotdev:master Aug 2, 2026
4 checks passed
@PathGao
PathGao deleted the fix/rust-backend-hardening branch August 2, 2026 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant