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).
(cherry picked from commit 3eab00bfbf)
Ported onto main (#1717 review): the five standalone tests/*_redteam.rs
files stay out — no test target builds them and they exercised copies of
the functions rather than the code itself.
This commit is contained in:
1 parent
9611f40400
commit
69fa7cae4f
30 files changed
+331
-76
No files matched your search
@@ -472,6 +472,17 @@ fn append_pdf_page(
|
||||
fallback_plot_style: Option<&PlotStyleTable>,
|
||||
fonts: &SearchableFontSet,
|
||||
) -> Result<(), String> {
|
||||
// Paper dimensions are validated like the content coordinates below: a
|
||||
// NaN/inf/zero sheet (`width < MIN` compares false for NaN) would be
|
||||
// serialized verbatim into a MediaBox no conforming viewer can display,
|
||||
// and printpdf's own parser would still accept it.
|
||||
if !page.paper_w.is_finite()
|
||||
|| !page.paper_h.is_finite()
|
||||
|| page.paper_w <= 0.0
|
||||
|| page.paper_h <= 0.0
|
||||
{
|
||||
return Err("plot sheet size is not valid".into());
|
||||
}
|
||||
let PlotContent {
|
||||
wires,
|
||||
hatches,
|
||||
|
||||
+93
-26
@@ -83,24 +83,44 @@ pub(crate) fn cap_evictions(
|
||||
(entries, evicted)
|
||||
}
|
||||
|
||||
/// The marker `recovered_name` appends (and strips when it is already
|
||||
/// there, so it never stacks across crash cycles).
|
||||
#[cfg_attr(not(target_arch = "wasm32"), allow(dead_code))] // unit-tested on native
|
||||
const RECOVERED_MARKER: &str = " (recovered)";
|
||||
|
||||
/// The name a restored drawing carries as a tab and download: the drawing's
|
||||
/// stem plus " (recovered)" and its own extension (DWG when the stored name
|
||||
/// has none, matching how autosave serializes untitled tabs). The extension
|
||||
/// must survive intact because the web parser picks the format from it.
|
||||
/// stem plus " (recovered)" and its own extension. The extension split
|
||||
/// follows the WORKER convention (`name.rsplit('.').next()`, the whole name
|
||||
/// when there is no dot): a dot-file like ".dxf" is a DXF name to the parse
|
||||
/// worker, and `Path::extension` would misread it as extensionless —
|
||||
/// restoring DXF bytes under a ".dwg" name jams the restore door. A name
|
||||
/// with no extension at all defaults to DXF, matching how autosave
|
||||
/// serializes untitled tabs (`source_is_dxf` → `dwg_source_version.is_none()`).
|
||||
/// An existing trailing marker is stripped first, so "(recovered)" stacks
|
||||
/// at most once no matter how many crash cycles re-snapshotted a previously
|
||||
/// recovered name.
|
||||
#[cfg_attr(not(target_arch = "wasm32"), allow(dead_code))] // unit-tested on native
|
||||
pub(crate) fn recovered_name(drawing: &str) -> String {
|
||||
let path = std::path::Path::new(drawing);
|
||||
let stem = path
|
||||
.file_stem()
|
||||
.map(|stem| stem.to_string_lossy().into_owned())
|
||||
.filter(|stem| !stem.is_empty())
|
||||
.unwrap_or_else(|| drawing.to_string());
|
||||
let ext = path
|
||||
.extension()
|
||||
.map(|ext| ext.to_string_lossy().into_owned())
|
||||
.filter(|ext| !ext.is_empty())
|
||||
.unwrap_or_else(|| "dwg".to_string());
|
||||
format!("{stem} (recovered).{ext}")
|
||||
let (head, ext) = match drawing.rsplit_once('.') {
|
||||
Some((head, ext)) if !ext.is_empty() => (head, ext.to_string()),
|
||||
// No dot (or a trailing one): the whole name is the stem.
|
||||
_ => (drawing, "dxf".to_string()),
|
||||
};
|
||||
// A hostile stored name must not smuggle path separators into the tab
|
||||
// title / download name: keep only the final component of the head.
|
||||
let head = head.rsplit(['/', '\\']).next().unwrap_or_default();
|
||||
// When the split consumed everything (".dxf") fall back to the whole
|
||||
// stored name, like `Path::file_stem`'s empty-stem fallback did.
|
||||
let mut stem = if head.is_empty() {
|
||||
drawing
|
||||
} else {
|
||||
head
|
||||
};
|
||||
// Idempotent: strip every trailing marker, then append exactly one.
|
||||
while let Some(stripped) = stem.strip_suffix(RECOVERED_MARKER) {
|
||||
stem = stripped;
|
||||
}
|
||||
format!("{stem}{RECOVERED_MARKER}.{ext}")
|
||||
}
|
||||
|
||||
/// Overwrite (or create) the snapshot for `name`. The optional thumbnail is
|
||||
@@ -108,7 +128,7 @@ pub(crate) fn recovered_name(drawing: &str) -> String {
|
||||
#[cfg(target_arch = "wasm32")]
|
||||
pub async fn snapshot(name: &str, bytes: &[u8], thumbnail: Option<&[u8]>) -> Result<(), String> {
|
||||
let directory = autosave_directory(true).await?;
|
||||
let mut names = read_index(&directory).await;
|
||||
let mut names = read_index(&directory).await?;
|
||||
if !names.iter().any(|stored| stored == name) {
|
||||
names.push(name.to_string());
|
||||
write_index(&directory, &names).await?;
|
||||
@@ -140,7 +160,9 @@ pub async fn remove(name: &str) -> Result<(), String> {
|
||||
.await
|
||||
.map_err(super::web_recent::js_error);
|
||||
let _ = JsFuture::from(directory.remove_entry(&thumbnail_key(name))).await;
|
||||
let mut names = read_index(&directory).await;
|
||||
// A corrupt index must abort the rewrite (not reset it to `names`),
|
||||
// or this remove would orphan every other row's snapshot.
|
||||
let mut names = read_index(&directory).await?;
|
||||
let before = names.len();
|
||||
names.retain(|stored| stored != name);
|
||||
if names.len() != before {
|
||||
@@ -161,7 +183,9 @@ pub async fn scan() -> Result<Vec<WebAutosaveEntry>, String> {
|
||||
let directory = autosave_directory(false).await?;
|
||||
let mut entries = Vec::new();
|
||||
let mut dead: Vec<String> = Vec::new();
|
||||
for name in read_index(&directory).await {
|
||||
// An unreadable index fails the whole boot scan: guessing an empty list
|
||||
// would cap-evict and remove snapshots the index can no longer name.
|
||||
for name in read_index(&directory).await? {
|
||||
let file = match super::web_recent::get_file(&directory, &snapshot_key(&name)).await {
|
||||
Ok(file) => file,
|
||||
// Crash between the index and snapshot writes: the row points
|
||||
@@ -212,12 +236,23 @@ fn thumbnail_key(name: &str) -> String {
|
||||
}
|
||||
|
||||
#[cfg(target_arch = "wasm32")]
|
||||
async fn read_index(directory: &web_sys::FileSystemDirectoryHandle) -> Vec<String> {
|
||||
super::web_recent::read_entry(directory, INDEX_KEY)
|
||||
.await
|
||||
.ok()
|
||||
.and_then(|bytes| serde_json::from_slice(&bytes).ok())
|
||||
.unwrap_or_default()
|
||||
async fn read_index(
|
||||
directory: &web_sys::FileSystemDirectoryHandle,
|
||||
) -> Result<Vec<String>, String> {
|
||||
let bytes = match super::web_recent::read_entry(directory, INDEX_KEY).await {
|
||||
Ok(bytes) => bytes,
|
||||
// A missing index is a fresh store.
|
||||
Err(_) => return Ok(Vec::new()),
|
||||
};
|
||||
// An empty (freshly created or truncated) index is an empty store.
|
||||
if bytes.is_empty() {
|
||||
return Ok(Vec::new());
|
||||
}
|
||||
// A non-empty file that fails to parse is real data we cannot name:
|
||||
// returning it as empty would let the next write reset the index and
|
||||
// silently orphan every other row's snapshot. Surface the failure so
|
||||
// callers skip the round/boot and leave the file untouched.
|
||||
serde_json::from_slice(&bytes).map_err(|error| error.to_string())
|
||||
}
|
||||
|
||||
#[cfg(target_arch = "wasm32")]
|
||||
@@ -266,10 +301,42 @@ mod tests {
|
||||
fn recovered_names_keep_a_parseable_extension() {
|
||||
assert_eq!(recovered_name("plan.dwg"), "plan (recovered).dwg");
|
||||
assert_eq!(recovered_name("sketch.dxf"), "sketch (recovered).dxf");
|
||||
assert_eq!(recovered_name("Drawing1"), "Drawing1 (recovered).dwg");
|
||||
// Extensionless names (untitled tabs) restore as DXF: that is what
|
||||
// autosave serializes them as (`source_is_dxf` → no DWG source
|
||||
// version), and the parse worker dispatches on the final extension.
|
||||
assert_eq!(recovered_name("Drawing1"), "Drawing1 (recovered).dxf");
|
||||
// Multi-dot names keep everything but the final extension in the stem.
|
||||
assert_eq!(recovered_name("a.b.dwg"), "a.b (recovered).dwg");
|
||||
assert_eq!(recovered_name(""), " (recovered).dwg");
|
||||
// A dot-file like ".dxf" is a DXF name under the worker's
|
||||
// `rsplit('.')` convention — the extension must not flip to dwg.
|
||||
assert_eq!(recovered_name(".dxf"), ".dxf (recovered).dxf");
|
||||
assert_eq!(recovered_name(".dwg"), ".dwg (recovered).dwg");
|
||||
assert_eq!(recovered_name(""), " (recovered).dxf");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_recovered_marker_never_stacks() {
|
||||
// Restoring sets the tab name to the recovered name; a later
|
||||
// autosave round snapshots under it. One marker per drawing:
|
||||
assert_eq!(
|
||||
recovered_name("plan (recovered).dwg"),
|
||||
"plan (recovered).dwg"
|
||||
);
|
||||
// Idempotent even for names that already stacked (old cycles).
|
||||
assert_eq!(
|
||||
recovered_name("plan (recovered) (recovered).dwg"),
|
||||
"plan (recovered).dwg"
|
||||
);
|
||||
// Extensionless stacked names de-stack too, then gain ".dxf".
|
||||
assert_eq!(
|
||||
recovered_name("plan (recovered)"),
|
||||
"plan (recovered).dxf"
|
||||
);
|
||||
// A marker embedded in the middle (not a suffix) is left alone.
|
||||
assert_eq!(
|
||||
recovered_name("(recovered) plan.dwg"),
|
||||
"(recovered) plan (recovered).dwg"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
+29
-14
@@ -12,7 +12,8 @@ use wasm_bindgen_futures::JsFuture;
|
||||
|
||||
/// A stray multi-drop must not grow the queue without bound; extras are
|
||||
/// rejected with a console warning instead of being silently dropped.
|
||||
const QUEUE_CAP: usize = 8;
|
||||
/// Entries are just name+bytes; 32 drawings is a realistic folder drop.
|
||||
const QUEUE_CAP: usize = 32;
|
||||
|
||||
/// One queued open. `restore_key` tags an autosave-restore file so its
|
||||
/// snapshot is deleted only when THIS open succeeds — matching by identity,
|
||||
@@ -41,7 +42,9 @@ fn lock_queue() -> std::sync::MutexGuard<'static, VecDeque<QueuedOpen>> {
|
||||
.unwrap_or_else(|poisoned| poisoned.into_inner())
|
||||
}
|
||||
|
||||
fn push(entry: QueuedOpen) {
|
||||
/// Returns `false` (after a console warning) when the queue is full and the
|
||||
/// entry was NOT stored — the caller decides how to surface the loss.
|
||||
fn push(entry: QueuedOpen) -> bool {
|
||||
let name = entry.name.clone();
|
||||
let mut queue = lock_queue();
|
||||
if queue.len() >= QUEUE_CAP {
|
||||
@@ -49,35 +52,40 @@ fn push(entry: QueuedOpen) {
|
||||
warn(&format!(
|
||||
"ocs: dropped-file queue full; ignoring \"{name}\""
|
||||
));
|
||||
return;
|
||||
return false;
|
||||
}
|
||||
queue.push_back(entry);
|
||||
true
|
||||
}
|
||||
|
||||
/// Queue already-read drawing bytes (drag-and-drop) for the 50 ms poll to
|
||||
/// open through the guarded web-picker flow.
|
||||
pub fn enqueue_open(name: String, bytes: Vec<u8>) {
|
||||
/// open through the guarded web-picker flow. Returns `false` when the queue
|
||||
/// is full and the bytes were dropped.
|
||||
pub fn enqueue_open(name: String, bytes: Vec<u8>) -> bool {
|
||||
push(QueuedOpen {
|
||||
name,
|
||||
bytes,
|
||||
restore_key: None,
|
||||
});
|
||||
})
|
||||
}
|
||||
|
||||
/// Queue an autosave-restore file tagged with its snapshot key.
|
||||
pub fn enqueue_restore(name: String, bytes: Vec<u8>, restore_key: String) {
|
||||
/// Queue an autosave-restore file tagged with its snapshot key. Returns
|
||||
/// `false` when the queue is full and the bytes were dropped (the OPFS
|
||||
/// snapshot survives; the caller must not wait for an open that will
|
||||
/// never start).
|
||||
pub fn enqueue_restore(name: String, bytes: Vec<u8>, restore_key: String) -> bool {
|
||||
push(QueuedOpen {
|
||||
name,
|
||||
bytes,
|
||||
restore_key: Some(restore_key),
|
||||
});
|
||||
})
|
||||
}
|
||||
|
||||
/// JS-callable entry (same shape as `ocs_control_submit`) for hosts that
|
||||
/// forward drops themselves.
|
||||
#[wasm_bindgen::prelude::wasm_bindgen]
|
||||
pub fn ocs_web_drop_file(name: String, bytes: js_sys::Uint8Array) {
|
||||
enqueue_open(name, bytes.to_vec());
|
||||
let _ = enqueue_open(name, bytes.to_vec());
|
||||
}
|
||||
|
||||
/// Pop the oldest dropped file that the poll has not started opening yet.
|
||||
@@ -179,7 +187,10 @@ pub fn install() {
|
||||
for (name, read) in reads {
|
||||
match JsFuture::from(read).await {
|
||||
Ok(buffer) => {
|
||||
enqueue_open(name, js_sys::Uint8Array::new(&buffer).to_vec())
|
||||
// A full queue warns on the console; the drop
|
||||
// handler has no command line to talk to.
|
||||
let _ =
|
||||
enqueue_open(name, js_sys::Uint8Array::new(&buffer).to_vec());
|
||||
}
|
||||
Err(error) => {
|
||||
web_sys::console::error_1(&wasm_bindgen::JsValue::from_str(&format!(
|
||||
@@ -203,7 +214,11 @@ pub fn install() {
|
||||
}
|
||||
|
||||
fn is_drawing_name(name: &str) -> bool {
|
||||
name.rsplit_once('.').is_some_and(|(_, extension)| {
|
||||
matches!(extension.to_ascii_lowercase().as_str(), "dwg" | "dxf")
|
||||
})
|
||||
// Same convention as the worker's reader dispatch (`rsplit('.').next()`):
|
||||
// a dot-less name is its own extension, so a file literally named
|
||||
// "DWG"/"dxf" parses and must not be filtered out here.
|
||||
matches!(
|
||||
name.rsplit('.').next().unwrap_or_default().to_ascii_lowercase().as_str(),
|
||||
"dwg" | "dxf"
|
||||
)
|
||||
}
|
||||
Reference in new issue
Block a user