From 08d92b7531aca39645a563e3b61d427e4a2340f9 Mon Sep 17 00:00:00 2001 From: fabi Date: Wed, 29 Jul 2026 21:37:39 +0200 Subject: [PATCH] fix(audit-7): keepsake viewer integrity, readable archives, quota_tolerance floor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seventh round, both defects in the keepsake — the one artifact that leaves the system entirely, and so the one where a green server-side signal carries no information at all. Squashed from 2 commits, original messages preserved below. ──────── fix(export): stop a caption bricking the viewer, and ship readable archives Two defects in the keepsake, both silent server-side and both only visible by extracting the real artifact and trying to use it. 1. A CAPTION COULD BRICK THE VIEWER. The viewer's data is inlined as `` -- it has to be, since guests open index.html over file:// where fetching a sibling data.json is blocked. The escape was ` `<\/`. Against XSS that holds; I fired `` through a real Chromium parser and it round-trips inert. It does not stop the caption steering the HTML TOKENIZER. `` drives the parser into script-data-double-escaped state, where the template's own `` only steps back to script-data-escaped instead of closing the element. Everything after it -- including the viewer bundle -- is swallowed as script data. Nothing executes and nothing leaks: `__EXPORT_DATA__` is simply never assigned and the keepsake opens blank. A denial of the deliverable, not an XSS. Reproduced in Chromium before changing anything, and the near-miss is worth recording: `` comes back CLEAN, because the trailing `-->` returns the parser to script-data state. A probe using the terminated form quietly repairs the thing it is testing for. Fix: escape every `<` as `<`, not just ` --- backend/src/handlers/admin.rs | 17 +++ backend/src/services/export.rs | 125 ++++++++++++++++-- e2e/specs/05-admin/config.spec.ts | 34 +++++ .../06-export/archive-permissions.spec.ts | 103 +++++++++++++++ .../viewer-caption-injection.spec.ts | 125 ++++++++++++++++++ 5 files changed, 396 insertions(+), 8 deletions(-) create mode 100644 e2e/specs/06-export/archive-permissions.spec.ts create mode 100644 e2e/specs/06-export/viewer-caption-injection.spec.ts diff --git a/backend/src/handlers/admin.rs b/backend/src/handlers/admin.rs index d001c4d..3233268 100644 --- a/backend/src/handlers/admin.rs +++ b/backend/src/handlers/admin.rs @@ -197,6 +197,23 @@ pub async fn patch_config( "Wert für {key} liegt außerhalb des zulässigen Bereichs ({min}–{max})." ))); } + // Zero is in range and catastrophic. `quota_tolerance` is the multiplier in + // `free_disk * tolerance / active_uploaders`, so 0 makes every per-user limit 0 and + // refuses EVERY upload — mid-event, with "Du hast dein Upload-Limit für dieses Event + // erreicht", an error naming the wrong cause entirely. `storage_quota_enabled` is the + // intended off-switch. + // + // Rejecting the value rather than raising the floor: very small tolerances are + // legitimate (they are how a large disk is throttled down to a sensible per-guest + // ceiling, and how the e2e quota tests steer it — around 1e-5 on a 174 GB volume), so + // a floor of, say, 0.01 would forbid real configurations to prevent one typo. + if key_str == "quota_tolerance" && n == 0.0 { + return Err(AppError::BadRequest( + "quota_tolerance = 0 würde jeden Upload blockieren. Zum Abschalten der \ + Speicher-Quote stattdessen „Speicher-Quote aktiv“ ausschalten." + .into(), + )); + } } else if BOOL_KEYS.contains(&key_str) { match value.trim().to_ascii_lowercase().as_str() { "true" | "false" | "1" | "0" | "yes" | "no" | "on" | "off" => {} diff --git a/backend/src/services/export.rs b/backend/src/services/export.rs index f754c80..d23423b 100644 --- a/backend/src/services/export.rs +++ b/backend/src/services/export.rs @@ -580,7 +580,7 @@ async fn run_zip_export_inner( }; let entry_name = format!("{folder}/{date}_{name_safe}_{}.{ext}", row.id); - let builder = ZipEntryBuilder::new(entry_name.into(), Compression::Stored); + let builder = keepsake_entry(entry_name, Compression::Stored); // Open BEFORE writing the entry header. A missing source is skipped (the media file // was deleted, or its processing failed) — but it must be skipped without aborting the @@ -973,7 +973,7 @@ async fn run_html_export_inner( // Write data.json { - let builder = ZipEntryBuilder::new("data.json".into(), Compression::Deflate); + let builder = keepsake_entry("data.json".into(), Compression::Deflate); let mut entry = zip.write_entry_stream(builder).await?; let mut cursor = AllowStdIo::new(std::io::Cursor::new(data_json.as_bytes())); fcopy(&mut cursor, &mut entry).await?; @@ -982,7 +982,7 @@ async fn run_html_export_inner( // Write README.txt { - let builder = ZipEntryBuilder::new("README.txt".into(), Compression::Deflate); + let builder = keepsake_entry("README.txt".into(), Compression::Deflate); let mut entry = zip.write_entry_stream(builder).await?; let mut cursor = AllowStdIo::new(std::io::Cursor::new(README_TEXT.as_bytes())); fcopy(&mut cursor, &mut entry).await?; @@ -1015,7 +1015,7 @@ async fn run_html_export_inner( }; let entry_name = format!("media/{name}"); - let builder = ZipEntryBuilder::new(entry_name.into(), Compression::Stored); + let builder = keepsake_entry(entry_name, Compression::Stored); let mut zip_entry = zip.write_entry_stream(builder).await?; let mut f = src_file.compat(); fcopy(&mut f, &mut zip_entry).await?; @@ -1552,6 +1552,36 @@ async fn maybe_broadcast_complete( /// double-clicking `index.html` (file://), where browsers block a cross-origin /// `fetch()` of a sibling `data.json` — so the data is inlined into the page. /// (`data.json` is still written separately for the http-served case.) +/// Permissions stamped on every entry in both archives: `rw-r--r--`. +/// +/// `ZipEntryBuilder::new` leaves the external file attribute at zero, and the host compatibility +/// defaults to Unix — so every entry was written with a stored mode of **0000**. Windows Explorer +/// ignores Unix modes and was fine, which is exactly why this survived: on Linux and macOS +/// `unzip` faithfully applies what the archive asks for, and the guest gets a directory of files +/// none of which they can open. `?---------` on every line of `unzip -Z`. +/// +/// That is the keepsake — the artifact the whole event exists to produce — arriving unreadable, +/// after distribution, with no server-side symptom at all. +/// `S_IFREG | 0644`. The type bits are included because the mode is written whole into the high +/// half of the external file attribute: without them extractors see a file of type "unknown" +/// (`unzip -Z` renders `?rw-r--r--`), which works but is not what the archive means to say. +const KEEPSAKE_ENTRY_MODE: u16 = 0o100_644; + +/// Build a ZIP entry for the keepsake. ALL entries in both archives go through here so the mode +/// can't be forgotten at one of the six call sites. +fn keepsake_entry(name: String, compression: Compression) -> ZipEntryBuilder { + ZipEntryBuilder::new(name.into(), compression).unix_permissions(KEEPSAKE_ENTRY_MODE) +} + +/// Escape a JSON payload for inlining inside a `` can't break out of the tag. - let safe = data_json.replace(" `<\/` stops the obvious break-out (``) and is inert + // against XSS. It does not stop the caption steering the HTML TOKENIZER. A caption + // containing `` puts the parser into + // script-data-double-escaped state; from there the template's own `` only + // steps back to script-data-escaped instead of closing the element, and the rest of the + // document — including the viewer bundle — is swallowed as script data. Nothing + // executes and nothing leaks; `window.__EXPORT_DATA__` is simply never assigned and the + // keepsake opens blank. + // + // That failure is silent and POST-DISTRIBUTION: the export succeeds, the ZIP is + // well-formed, the job writes `done`, /export/status is green, and the host hands out a + // file that only fails when a guest double-clicks index.html — in every copy, with no + // way to fix it after the fact. Reachable from any guest-authored caption or comment, + // since both are embedded in the viewer. + // + // `<` never appears in JSON structural syntax — only inside string values — so a global + // replace is sound, and `<` is valid in both JSON and a JS string literal. One + // rule covers ` format!("{}{}{}", &html[..idx], head_inject, &html[idx..]), None => format!("{head_inject}{html}"), }; - let builder = ZipEntryBuilder::new(path.into(), Compression::Deflate); + let builder = keepsake_entry(path, Compression::Deflate); let mut entry = zip.write_entry_stream(builder).await?; let mut cursor = AllowStdIo::new(std::io::Cursor::new(injected.as_bytes())); fcopy(&mut cursor, &mut entry).await?; entry.close().await?; } else { - let builder = ZipEntryBuilder::new(path.into(), Compression::Deflate); + let builder = keepsake_entry(path, Compression::Deflate); let mut entry = zip.write_entry_stream(builder).await?; let mut cursor = AllowStdIo::new(std::io::Cursor::new(file.contents())); fcopy(&mut cursor, &mut entry).await?; @@ -1815,6 +1868,62 @@ mod tests { } } + /// Every `<` is escaped, whatever it is part of. + /// + /// PREVENTS the regression to ` `<\\/`, which is named for the one case it handles. + /// `` drives the HTML tokenizer into + /// script-data-double-escaped state, where the template's own `` no longer closes + /// the element — the viewer bundle is swallowed as script data, `__EXPORT_DATA__` is never + /// assigned, and the keepsake opens blank in every copy the host has already handed out. + #[test] + fn no_left_angle_bracket_survives_inlining() { + for payload in [ + r#"{"caption":"` drives the parser into script-data-double-escaped state, where the template's own + * `` steps back to script-data-escaped instead of closing the element. Everything after — + * including the viewer bundle — is swallowed as script data. Nothing executes and nothing leaks; + * `__EXPORT_DATA__` is never assigned and the keepsake renders blank. + * + * What makes it worth a browser-level test rather than a unit test alone: the failure is SILENT and + * POST-DISTRIBUTION. The export succeeds, the ZIP is well-formed, the job writes `done`, + * /export/status is green, and the host hands out a file that only fails when a guest + * double-clicks it — in every copy, unfixably. It is not visible by reading the escape. It is only + * visible by running a real parser over the real artifact, which is what this does: release, pull + * the actual Memories.zip, extract index.html, open it over file:// in Chromium, and assert the + * viewer actually booted. + * + * The near-miss worth recording: `` comes back CLEAN, because the + * trailing `-->` returns the parser to script-data state. A probe using the terminated form + * quietly repairs the very thing it is testing for. Only the unterminated variant exposes it. + */ +import { test, expect } from '../../fixtures/test'; +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { seedUpload } from '../../helpers/seed'; +import { BASE } from '../../helpers/env'; + +/** Unterminated on purpose — see the header. The terminated form self-repairs. */ +const TOKENIZER_PAYLOAD = '