fix(audit-7): keepsake viewer integrity, readable archives, quota_tolerance floor

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 `<script>window.__EXPORT_DATA__={…}</script>` -- 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
`</script><img src=x onerror=…>` through a real Chromium parser and it round-trips
inert.

It does not stop the caption steering the HTML TOKENIZER. `<!--<script` with no later
`-->` drives the parser into script-data-double-escaped state, where the template's own
`</script>` 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:
`<!--<script>alert(1)</script>-->` 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 `</`. `<` never appears in JSON structural
syntax -- only inside string values -- so a global replace is sound, and one rule covers
`</script`, `<!--` and `<script` together. That is the point: the old escape was named
for the single case it handled. Only the INLINED copy is escaped; data.json is written
separately, in no HTML context, and stays literal.

2. EVERY ENTRY IN BOTH ARCHIVES WAS STORED MODE 0000.

`ZipEntryBuilder::new` leaves the external file attribute at zero and async_zip's host
compatibility defaults to Unix, so `unzip -Z` showed `?---------` on every line of both
Gallery.zip and Memories.zip. Windows Explorer ignores Unix modes, which is why this
survived; on Linux and macOS `unzip` faithfully applies what the archive asks for and
the guest gets a folder of photos none of which they can open.

Unconditional -- every keepsake ever produced, no hostile input required -- and
invisible server-side: the export succeeds, the ZIP is well-formed, the job writes
`done`, /export/status is green.

Found by accident. The browser test for defect 1 failed with ERR_ACCESS_DENIED on
file://, which looked exactly like a Playwright sandbox quirk; I twice "worked around"
it (fresh context, then a separately launched browser) before checking the extracted
files and finding mode 000. The workaround was suppressing a real bug. Both workarounds
are gone -- the ordinary `page` fixture loads the archive fine now.

Fix: all six ZipEntryBuilder sites route through one `keepsake_entry` helper stamping
`S_IFREG | 0644`, so the mode cannot be forgotten at a call site.

Tests: 3 unit (no `<` survives; the payload still decodes to the original value, because
this is a transport encoding and not a sanitiser; a clean payload is untouched) and 2
e2e that release for real, download the real archives, and check them from outside the
app -- one opening index.html over file:// in Chromium and asserting the viewer booted,
the captions came back verbatim and nothing executed; one asserting every stored mode
and every extracted file is readable. Both assertions verified to FAIL against the
pre-fix artifacts.

──────── fix(admin): reject quota_tolerance = 0 instead of silently blocking every upload

Zero is inside the documented 0–1 range and catastrophic. The per-user limit is
`free_disk * tolerance / active_uploaders`, so a tolerance of 0 makes every 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. An admin reaching for an off-switch
wants `storage_quota_enabled`; the rejection now says so.

Rejecting the value rather than raising the floor. A floor of 0.01 was the obvious fix
and it is wrong: very small tolerances are legitimate -- they are how a large disk is
throttled down to a sensible per-guest ceiling, and how the quota specs steer it
(tolerance = target * active / free lands around 1e-5 on the 174 GB volume this suite
runs on). A floor would forbid real configurations, and would have broken the entire
storage-quota describe block, to prevent one typo. Verified: those four tests still pass.

Tests: the rejection, that the stored value is untouched (validation fully precedes any
write), and the mirror -- 0.00001 still round-trips -- so the guard can't quietly become
a floor later.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
fabi
2026-07-29 21:37:39 +02:00
parent 06bc9ddcb3
commit 08d92b7531
5 changed files with 396 additions and 8 deletions

View File

@@ -68,6 +68,40 @@ test.describe('Admin — config API', () => {
expect(cfg.privacy_note).toBe(note);
await api.patchConfig(adminToken, { privacy_note: '' });
});
test('quota_tolerance = 0 is rejected, with a pointer to the real off-switch', async ({
api,
adminToken,
}) => {
// Zero is inside the documented 01 range and catastrophic: the per-user limit is
// `free_disk * tolerance / active_uploaders`, so 0 refuses EVERY upload — mid-event, with
// "Du hast dein Upload-Limit für dieses Event erreicht", which names the wrong cause
// entirely. `storage_quota_enabled` is what an admin reaching for an off-switch wants.
const res = await fetch(
(process.env.E2E_FRONTEND_URL ?? 'http://localhost:3101') + '/api/v1/admin/config',
{
method: 'PATCH',
headers: { Authorization: `Bearer ${adminToken}`, 'Content-Type': 'application/json' },
body: JSON.stringify({ quota_tolerance: '0' }),
}
);
expect(res.status).toBe(400);
expect(
(await res.text()).toLowerCase(),
'the error must name the switch the admin actually wanted'
).toContain('speicher-quote');
// The value is untouched — validation fully precedes any write.
expect((await api.getConfig(adminToken)).quota_tolerance).toBe('0.75');
});
test('a very small quota_tolerance is still accepted', async ({ api, adminToken }) => {
// The mirror. Rejecting 0 must not become a floor: small tolerances are how a large disk is
// throttled to a sensible per-guest ceiling, and how the quota specs steer it (~1e-5 on a
// 174 GB volume). A floor of 0.01 would forbid real configurations to prevent one typo.
await api.patchConfig(adminToken, { quota_tolerance: '0.00001' });
expect((await api.getConfig(adminToken)).quota_tolerance).toBe('0.00001');
await api.patchConfig(adminToken, { quota_tolerance: '0.75' });
});
});
test.describe('Admin — stats', () => {