forked from 1b-agent/opencadstudio-upstream
web: fix red-team breaks in restore, drop-open, and plot paths
Three attack agents (state-machine tracing, executable input fuzzing,
browser-API review) found two breaks and several degradations; all are
fixed here, with the fuzzer's 22 tests kept as regression suites.
Breaks:
- Untitled tabs (and dot-names like ".dxf") autosave as DXF, but the
restore name defaulted to .dwg — the worker dispatches its reader by
the name's extension, so the only copy of unsaved work could never
be restored and was re-offered forever. recovered_name now uses the
worker's rsplit('.') convention and defaults extensionless names to
dxf, matching new-document serialization.
- " (recovered)" stacked one suffix per crash-restore cycle; the
marker is now stripped idempotently before appending.
- Opening a file (Ctrl+O / OPEN / automation) while a web open was in
flight clobbered it: the in-flight open's bytes were lost and every
queued restore prompt stalled for the session. on_open_file now
refuses while an open is running (web only).
Hardening:
- Escape on the Recover Drawing modal defers instead of deleting the
only snapshot; the Discard button still removes it.
- A cap-rejected restore enqueue warns in-app and re-offers the next
snapshot; the drop queue cap rises to 32.
- Exiting mid-round no longer races snapshot removals (deferrals are
drained, the sweep skips while a round is in flight); closing a tab
with "Don't save" now also removes its web snapshot; the snapshot
key derivation is shared via web_autosave_key.
- The plot-preview overlay no longer leaks a full PDF copy per preview
(the Download closure lives in a replaceable registry).
- A corrupt autosave index.json fails the round/boot instead of
silently resetting every row; plot pages with non-finite or non-
positive sheet dimensions error instead of emitting a NaN MediaBox.
- The drop filter accepts dot-less "DWG"/"dxf" names the worker
parses (same rsplit convention on both gates).
This commit is contained in:
1 parent
64160fd68d
commit
3eab00bfbf
35 files changed
+1356
-76
No files matched your search
@@ -0,0 +1,95 @@
|
||||
//! RED-TEAM: the web export filename pipeline
|
||||
//! `pick_stl_path_owned`/`pick_step_path_owned` (src/io/stl.rs:102-104,
|
||||
//! src/io/step.rs:258-260, wasm-only one-liners) fed by
|
||||
//! `tab.current_path.file_stem()` (src/app/update/mod.rs:1310-1315) and
|
||||
//! consumed by `sys::download_bytes` via `path.file_name()`
|
||||
//! (src/app/update/file.rs:1717-1721, src/sys.rs:348-372).
|
||||
//!
|
||||
//! The wasm functions are cfg'd out on native, so the exact composition is
|
||||
//! replicated here (it is pure `Path` semantics + `format!`) and attacked
|
||||
//! with hostile current_paths. `download_bytes` itself only ever passes the
|
||||
//! string to `a.set_download` — a plain JS property set that cannot panic in
|
||||
//! Rust; browsers sanitize separators/control characters in the attribute.
|
||||
#![cfg(not(target_arch = "wasm32"))]
|
||||
|
||||
/// Verbatim composition of the web flow: stem extraction (mod.rs:1310-1315)
|
||||
/// -> virtual path (stl.rs:103 / step.rs:259) -> download name
|
||||
/// (file.rs:1717-1721).
|
||||
fn stl_download_name(current_path: Option<&std::path::Path>) -> String {
|
||||
let stem = current_path
|
||||
.and_then(|p| p.file_stem())
|
||||
.map(|s| s.to_string_lossy().into_owned())
|
||||
.unwrap_or_else(|| "drawing".into());
|
||||
let path = std::path::PathBuf::from(format!("{stem}.stl"));
|
||||
path.file_name()
|
||||
.map(|n| n.to_string_lossy().into_owned())
|
||||
.unwrap_or_else(|| "export.stl".into())
|
||||
}
|
||||
|
||||
fn step_download_name(current_path: Option<&std::path::Path>) -> String {
|
||||
let stem = current_path
|
||||
.and_then(|p| p.file_stem())
|
||||
.map(|s| s.to_string_lossy().into_owned())
|
||||
.unwrap_or_else(|| "drawing".into());
|
||||
let path = std::path::PathBuf::from(format!("{stem}.step"));
|
||||
path.file_name()
|
||||
.map(|n| n.to_string_lossy().into_owned())
|
||||
.unwrap_or_else(|| "export.step".into())
|
||||
}
|
||||
|
||||
/// Path separators coming from a hostile display name never survive into the
|
||||
/// download: `file_stem()` already drops the directory part, and the
|
||||
/// re-parsed virtual path's `file_name()` drops it again. No matter what
|
||||
/// "current_path" a host page injects (the JS entry `ocs_web_drop_file`
|
||||
/// accepts arbitrary name strings), the download filename cannot traverse.
|
||||
#[test]
|
||||
fn separators_and_dotdots_never_reach_the_download_name() {
|
||||
// Host page drops "a/b.dwg" — name with a forward separator.
|
||||
let name = stl_download_name(Some(std::path::Path::new("a/b.dwg")));
|
||||
assert_eq!(name, "b.stl");
|
||||
// Windows-style separator (web targets unix path rules: not a separator
|
||||
// there, but `file_name()` strips any leading "./" etc.; backslashes
|
||||
// survive as literal characters — browsers strip them in the download
|
||||
// attribute, and they cannot traverse OPFS because the name is never
|
||||
// used as a path, only as a display string + FNV-hash key input).
|
||||
let name = stl_download_name(Some(std::path::Path::new("..\\evil.dwg")));
|
||||
#[cfg(windows)]
|
||||
assert_eq!(name, "evil.stl");
|
||||
#[cfg(not(windows))]
|
||||
assert_eq!(name, "..\\evil.stl");
|
||||
// Pure ".." cannot be a stem: file_stem() of ".." is None -> "drawing".
|
||||
assert_eq!(stl_download_name(Some(std::path::Path::new(".."))), "drawing.stl");
|
||||
assert_eq!(step_download_name(Some(std::path::Path::new(".."))), "drawing.step");
|
||||
// No path at all (untitled tab).
|
||||
assert_eq!(stl_download_name(None), "drawing.stl");
|
||||
assert_eq!(step_download_name(None), "drawing.step");
|
||||
// Extension-less drawing name.
|
||||
assert_eq!(stl_download_name(Some(std::path::Path::new("Drawing1"))), "Drawing1.stl");
|
||||
}
|
||||
|
||||
/// Quotes, newlines, control characters and unicode ride through as inert
|
||||
/// string bytes: the Rust side does no filesystem or shell operation with
|
||||
/// them (`download_bytes` builds a Blob and sets the `download` attribute —
|
||||
/// src/sys.rs:356-369 — which cannot panic). NOTE: even an interior NUL
|
||||
/// survives the whole Rust pipeline (std::path only rejects it at OS
|
||||
/// boundaries, and there are none on web) — JS strings legally contain NUL,
|
||||
/// and the browser sanitizes it in the download attribute. No injection, no
|
||||
/// panic — verified by construction of the pipeline.
|
||||
#[test]
|
||||
fn quote_and_control_char_stems_stay_inert_strings() {
|
||||
for hostile in [
|
||||
"ev\"il", "ev'il", "line\nbreak", "tab\there", "cr\r lf", "nul\u{0}ish",
|
||||
"café", "図面", "..\\..\\evil", "con", "..",
|
||||
] {
|
||||
let path = std::path::PathBuf::from(format!("{hostile}.dwg"));
|
||||
let name = stl_download_name(Some(&path));
|
||||
assert!(name.ends_with(".stl"), "{name:?}");
|
||||
let step_name = step_download_name(Some(&path));
|
||||
assert!(step_name.ends_with(".step"), "{step_name:?}");
|
||||
}
|
||||
// Pin the surprising-but-harmless case: an interior NUL is NOT stripped
|
||||
// anywhere in Rust; it would reach `set_download` as a JS string
|
||||
// containing U+0000 (legal), and the browser scrubs it.
|
||||
let name = stl_download_name(Some(std::path::Path::new("nul\u{0}ish.dwg")));
|
||||
assert_eq!(name, "nul\u{0}ish.stl");
|
||||
}
|
||||
Reference in new issue
Block a user