Merge branch 'fix/edit-upload-keepsake-and-coverage-2026-07-15'

This commit is contained in:
fabi
2026-07-15 19:48:31 +02:00
9 changed files with 325 additions and 0 deletions

View File

@@ -98,6 +98,13 @@ pub async fn truncate_all(
// surviving ticket is a dangling reference to a user that no longer exists. // surviving ticket is a dangling reference to a user that no longer exists.
state.sse_tickets.clear(); state.sse_tickets.clear();
// Invalidate any in-flight/queued compression task spawned by the previous test. Without this a
// task still waiting on the concurrency semaphore wakes AFTER this wipe, fails to find its
// (now-deleted) file, and broadcasts upload-error/upload-deleted into the NEXT test's SSE
// stream. (Export workers are already inert across a truncate: they are epoch-guarded on the
// event row, and truncate gives the event a fresh random UUID, so their writes match nothing.)
state.compression.bump_generation();
Ok(StatusCode::NO_CONTENT) Ok(StatusCode::NO_CONTENT)
} }

View File

@@ -416,6 +416,15 @@ pub async fn edit_upload(
// Caption update + hashtag wipe-then-relink in one transaction, so a crash // Caption update + hashtag wipe-then-relink in one transaction, so a crash
// mid-relink can't leave the upload with its hashtags stripped. // mid-relink can't leave the upload with its hashtags stripped.
//
// Editing is intentionally allowed while uploads are locked or the gallery is released — like
// comments and likes, the lock freezes *new uploads* only (USER_JOURNEYS §9.3). But a caption
// is embedded in the HTML viewer keepsake (the ZIP holds media only — see export.rs), so an
// edit AFTER release must regenerate the viewer, or the downloadable keepsake keeps showing the
// old caption forever while the live feed shows the new one. Same atomicity as delete_upload:
// the edit and its invalidation share one tx so a dropped handler can't leave them disagreeing.
// `Affects::ViewerOnly` carries the finished ZIP forward (the media didn't change); when the
// gallery isn't released, `invalidate_and_arm` returns None and this is a no-op.
let mut tx = state.pool.begin().await?; let mut tx = state.pool.begin().await?;
if let Some(ref caption) = body.caption { if let Some(ref caption) = body.caption {
Upload::update_caption(&mut *tx, upload_id, Some(caption)).await?; Upload::update_caption(&mut *tx, upload_id, Some(caption)).await?;
@@ -427,7 +436,16 @@ pub async fn edit_upload(
Hashtag::link_to_upload(&mut *tx, upload_id, h.id).await?; Hashtag::link_to_upload(&mut *tx, upload_id, h.id).await?;
} }
} }
let regen = crate::services::export::invalidate_and_arm(
&mut tx,
&state.config.event_slug,
crate::services::export::Affects::ViewerOnly,
)
.await?;
tx.commit().await?; tx.commit().await?;
if let Some(r) = regen {
crate::handlers::host::start_regen(&state, r);
}
Ok(StatusCode::OK) Ok(StatusCode::OK)
} }

View File

@@ -1,4 +1,5 @@
use std::path::{Path, PathBuf}; use std::path::{Path, PathBuf};
use std::sync::atomic::{AtomicU64, Ordering};
use std::sync::Arc; use std::sync::Arc;
use anyhow::{Context, Result}; use anyhow::{Context, Result};
@@ -15,6 +16,10 @@ pub struct CompressionWorker {
pool: PgPool, pool: PgPool,
media_path: PathBuf, media_path: PathBuf,
sse_tx: broadcast::Sender<SseEvent>, sse_tx: broadcast::Sender<SseEvent>,
/// Bumped whenever the underlying data is reset out from under in-flight work (only the e2e
/// TRUNCATE does this today). A task captures the value at spawn and abandons itself if it has
/// changed by the time it runs — see `process`.
generation: Arc<AtomicU64>,
} }
impl CompressionWorker { impl CompressionWorker {
@@ -24,14 +29,32 @@ impl CompressionWorker {
pool, pool,
media_path, media_path,
sse_tx, sse_tx,
generation: Arc::new(AtomicU64::new(0)),
} }
} }
/// Invalidate all in-flight and queued compression work. Called by the e2e TRUNCATE endpoint:
/// truncating deletes the upload rows and wipes `media/`, so a worker that was queued on the
/// semaphore when the wipe happened would otherwise wake in the NEXT test, fail to find its
/// file, and broadcast `upload-error` / `upload-deleted` into that test's live SSE stream —
/// corrupting any test that asserts on toasts or feed contents. Bumping the generation makes
/// those stale tasks return silently instead. A no-op in production (never called there).
pub fn bump_generation(&self) {
self.generation.fetch_add(1, Ordering::SeqCst);
}
/// Spawn a background task to process an uploaded file. /// Spawn a background task to process an uploaded file.
pub fn process(&self, upload_id: Uuid, original_path: String, mime_type: String) { pub fn process(&self, upload_id: Uuid, original_path: String, mime_type: String) {
let worker = self.clone(); let worker = self.clone();
let born_at = worker.generation.load(Ordering::SeqCst);
tokio::spawn(async move { tokio::spawn(async move {
let _permit = worker.semaphore.acquire().await; let _permit = worker.semaphore.acquire().await;
// The data this task was queued against may have been reset while it waited for a permit
// (e2e TRUNCATE). If so, its file and row are gone; doing anything — including
// broadcasting a failure — would leak into an unrelated test. Abandon quietly.
if worker.generation.load(Ordering::SeqCst) != born_at {
return;
}
match worker.do_process(upload_id, &original_path, &mime_type).await { match worker.do_process(upload_id, &original_path, &mime_type).await {
Ok(_) => { Ok(_) => {
tracing::info!("compression completed for upload {upload_id}"); tracing::info!("compression completed for upload {upload_id}");

View File

@@ -137,6 +137,11 @@ export class ApiClient {
}); });
} }
async listPinResetRequests(token: string): Promise<any[]> {
const { body } = await this.request<any[]>('GET', '/host/pin-reset-requests', { token });
return body;
}
async closeEvent(token: string) { async closeEvent(token: string) {
return this.request<void>('POST', '/host/event/close', { token, expectedStatus: [200, 204] }); return this.request<void>('POST', '/host/event/close', { token, expectedStatus: [200, 204] });
} }

View File

@@ -59,6 +59,26 @@ export const db = {
}); });
}, },
async countSessionsForUser(userId: string): Promise<number> {
return withClient(async (c) => {
const r = await c.query<{ count: string }>(
`SELECT COUNT(*)::text AS count FROM session WHERE user_id = $1`,
[userId]
);
return Number(r.rows[0].count);
});
},
async countPinResetRequestsForUser(userId: string): Promise<number> {
return withClient(async (c) => {
const r = await c.query<{ count: string }>(
`SELECT COUNT(*)::text AS count FROM pin_reset_request WHERE user_id = $1`,
[userId]
);
return Number(r.rows[0].count);
});
},
async setExportReleased(slug: string, released: boolean) { async setExportReleased(slug: string, released: boolean) {
await withClient((c) => await withClient((c) =>
c.query(`UPDATE event SET export_released_at = $2 WHERE slug = $1`, [ c.query(`UPDATE event SET export_released_at = $2 WHERE slug = $1`, [

View File

@@ -0,0 +1,58 @@
/**
* "Sign out everywhere" — DELETE /api/v1/sessions. A security control (revoke every device after a
* lost/stolen phone) that had ZERO coverage. Sessions are validated per-request against the DB
* (auth middleware resolves the token hash to a live session row), so revocation is observable: a
* revoked token must stop authenticating.
*/
import { test, expect } from '../../fixtures/test';
const BASE = process.env.E2E_FRONTEND_URL ?? 'http://localhost:3101';
const ctx = (jwt: string) =>
fetch(`${BASE}/api/v1/me/context`, { headers: { Authorization: `Bearer ${jwt}` } });
test.describe('Auth — sign out everywhere', () => {
test('DELETE /sessions revokes ALL of the caller\'s sessions, not just the current one', async ({
api,
guest,
db,
}) => {
// Two devices for one guest: the join session, plus a second session from a recover login.
const g = await guest('MultiDevice');
const second = await api.recover(g.displayName, g.pin);
const secondJwt = second.body.jwt;
// Both tokens work, and there really are two distinct sessions.
expect((await ctx(g.jwt)).status).toBe(200);
expect((await ctx(secondJwt)).status).toBe(200);
expect(await db.countSessionsForUser(g.userId)).toBeGreaterThanOrEqual(2);
// Sign out everywhere, authenticated as device one.
const res = await fetch(`${BASE}/api/v1/sessions`, {
method: 'DELETE',
headers: { Authorization: `Bearer ${g.jwt}` },
});
expect(res.status).toBe(204);
// BOTH devices are now logged out — the whole point of the control. If it only killed the
// caller's own session, device two would still be live and a stolen phone would keep access.
expect((await ctx(g.jwt)).status, 'the calling device is signed out').toBe(401);
expect((await ctx(secondJwt)).status, 'the OTHER device is signed out too').toBe(401);
expect(await db.countSessionsForUser(g.userId)).toBe(0);
});
test('one user signing out everywhere does not touch another user\'s sessions', async ({
guest,
}) => {
const a = await guest('SignsOut');
const b = await guest('StaysIn');
await fetch(`${BASE}/api/v1/sessions`, {
method: 'DELETE',
headers: { Authorization: `Bearer ${a.jwt}` },
});
expect((await ctx(a.jwt)).status).toBe(401);
expect((await ctx(b.jwt)).status, "another user's session must be untouched").toBe(200);
});
});

View File

@@ -0,0 +1,119 @@
/**
* USER_JOURNEYS §4 — the "I forgot my PIN" in-app request lifecycle, end to end.
*
* This whole journey had ZERO functional coverage (only the 403 gate, from the authz sweep). The
* invariants below are all security-relevant and none of them were tested:
* - the always-204 non-enumeration contract (an unknown name must look identical to a known one)
* - the per-IP+name throttle (3 / 15 min)
* - admins are EXCLUDED from the request queue (they recover via password, not PIN)
* - a host reset REVOKES the target's sessions (the forgotten/compromised device is logged out)
*/
import { test, expect } from '../../fixtures/test';
const BASE = process.env.E2E_FRONTEND_URL ?? 'http://localhost:3101';
const requestReset = (displayName: string) =>
fetch(`${BASE}/api/v1/recover/request`, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ display_name: displayName }),
});
test.describe('PIN reset — in-app request lifecycle', () => {
test('a request for a real guest queues exactly one request the host can see', async ({
api,
host,
guest,
db,
}) => {
const g = await guest('ForgetfulFrida');
expect((await requestReset('ForgetfulFrida')).status).toBe(204);
// The host sees it...
const list = await api.listPinResetRequests(host.jwt);
expect(list.some((r: any) => r.user_id === g.userId)).toBe(true);
// ...and it's deduped: a second request does not stack a duplicate (ON CONFLICT DO NOTHING).
expect((await requestReset('ForgetfulFrida')).status).toBe(204);
expect(await db.countPinResetRequestsForUser(g.userId)).toBe(1);
});
test('an UNKNOWN name is indistinguishable from a known one (204, no queue, no enumeration)', async ({
host,
api,
guest,
}) => {
// Same status code and no error body as the real path — the only observable difference must be
// in the host's queue, which the requester cannot see.
const known = await guest('KnownKarl');
expect((await requestReset('KnownKarl')).status).toBe(204);
expect((await requestReset('no-such-guest-' + Date.now())).status).toBe(204);
// The unknown name queued nothing; the known one queued exactly one.
const list = await api.listPinResetRequests(host.jwt);
expect(list).toHaveLength(1);
expect(list[0].user_id).toBe(known.userId);
});
test('an ADMIN never gets a reset request queued (admins recover via password)', async ({
api,
host,
db,
adminToken,
}) => {
// The admin user exists (adminToken logged them in). Their display name is "Admin" by
// convention; request a reset for it and confirm the queue stays empty — the INSERT filters
// `role <> 'admin'`, so queuing a reset for an admin would be a privilege-relevant leak.
const admin = (await api.listUsers(host.jwt)).find((u: any) => u.role === 'admin');
expect(admin, 'an admin user must exist').toBeTruthy();
expect((await requestReset(admin.display_name)).status).toBe(204);
expect(await db.countPinResetRequestsForUser(admin.id)).toBe(0);
expect(await api.listPinResetRequests(host.jwt)).toHaveLength(0);
});
test('requests are throttled to 3 per 15 min per IP+name', async ({ api, adminToken, guest }) => {
await api.patchConfig(adminToken, { rate_limits_enabled: 'true' });
await guest('ThrottleTarget');
const statuses: number[] = [];
for (let i = 0; i < 5; i++) statuses.push((await requestReset('ThrottleTarget')).status);
// First 3 accepted, the rest throttled — proving the limiter is real and keyed.
expect(statuses.filter((s) => s === 204)).toHaveLength(3);
expect(statuses.filter((s) => s === 429).length).toBeGreaterThan(0);
// Restore so the next test's fixtures aren't affected (rate limits share the backend).
await api.patchConfig(adminToken, { rate_limits_enabled: 'false' });
});
test('a host PIN reset revokes the targets sessions and clears the request', async ({
api,
host,
guest,
db,
}) => {
const g = await guest('CompromisedCarl');
await requestReset('CompromisedCarl');
// The guest has a live session (the join minted one) and a queued request.
expect(await db.countSessionsForUser(g.userId)).toBeGreaterThan(0);
expect(await db.countPinResetRequestsForUser(g.userId)).toBe(1);
// Host resets the PIN.
await api.resetUserPin(host.jwt, g.userId);
// The old device is logged out: the guest's pre-reset JWT no longer authenticates. This is the
// security point of a reset — sessions are token-bound, so without the revoke the compromised
// device would stay logged in with full access despite the new PIN.
const stale = await fetch(`${BASE}/api/v1/me/context`, {
headers: { Authorization: `Bearer ${g.jwt}` },
});
expect(stale.status, 'the pre-reset session must be revoked').toBe(401);
expect(await db.countSessionsForUser(g.userId)).toBe(0);
// And the pending request was resolved.
expect(await db.countPinResetRequestsForUser(g.userId)).toBe(0);
});
});

View File

@@ -56,4 +56,42 @@ test.describe('Media gating — moderation revokes preview access (F2)', () => {
const afterDelete = await fetch(`${BASE}/api/v1/upload/${id}/preview`); const afterDelete = await fetch(`${BASE}/api/v1/upload/${id}/preview`);
expect(afterDelete.status, 'moderation must revoke preview access').toBe(404); expect(afterDelete.status, 'moderation must revoke preview access').toBe(404);
}); });
test('the preview is revoked when the UPLOADER is banned (not just on delete)', async ({
host,
api,
guest,
}) => {
test.setTimeout(30_000);
// The header of this file claims delete AND ban-hide both revoke access, but only delete was
// ever exercised. A ban hides the user's content everywhere (the visibility check filters
// `is_banned` inside `find_by_id_visible`, which gates preview AND thumbnail identically), and
// its whole point is that a direct-URL holder loses the image — so it must 404 the gated
// preview too, exactly like a delete.
const offender = await guest('BannedUploader');
const id = await seedUpload(offender.jwt, { caption: 'to be hidden' });
// Wait for the compression worker to produce the preview.
await expect
.poll(
async () => {
const row = (await api.getFeed(host.jwt)).uploads?.find((u: any) => u.id === id);
return row?.preview_url;
},
{ timeout: 20_000, intervals: [500] }
)
.toBe(`/api/v1/upload/${id}/preview`);
// Served while the uploader is in good standing.
expect((await fetch(`${BASE}/api/v1/upload/${id}/preview`)).status).toBe(200);
// Ban the uploader (default: hide their uploads).
await api.banUser(host.jwt, offender.userId);
// Must now 404 — the direct-URL holder loses the image, same as a takedown.
expect(
(await fetch(`${BASE}/api/v1/upload/${id}/preview`)).status,
"a banned uploader's preview must be revoked"
).toBe(404);
});
}); });

View File

@@ -518,6 +518,43 @@ test.describe('Flow re-review — reopen→re-release export integrity + stale-k
expect(await zipJobEpoch()).toBe(await eventEpoch()); expect(await zipJobEpoch()).toBe(await eventEpoch());
}); });
test('editing a caption AFTER release regenerates the viewer but carries the ZIP forward', async ({
host,
}) => {
// edit_upload used to update the caption and NOTHING else — no keepsake invalidation. A caption
// lives in the HTML viewer (the ZIP holds media only), so after release the downloadable viewer
// kept showing the OLD caption forever while the live feed showed the new one. Editing is
// intentionally allowed post-release (like comments), so the fix is to regenerate, not forbid:
// invalidate_and_arm(ViewerOnly) — rebuild the viewer, carry the finished ZIP forward untouched.
const uploadId = await seedUpload(host.jwt, { caption: 'tippfehlr' });
expect((await post('/api/v1/host/gallery/release', host.jwt)).status).toBe(204);
await waitExportDone(host.jwt);
const zipEpochBefore = await zipJobEpoch();
const eventEpochBefore = await eventEpoch();
expect(zipEpochBefore).toBe(eventEpochBefore);
// The edit is allowed (not blocked by the release lock) ...
const res = await fetch(BASE + `/api/v1/upload/${uploadId}`, {
method: 'PATCH',
headers: { Authorization: `Bearer ${host.jwt}`, 'Content-Type': 'application/json' },
body: JSON.stringify({ caption: 'korrigiert' }),
});
expect(res.status).toBe(200);
// ... and it bumped the epoch (the viewer must rebuild) ...
await expect.poll(async () => await eventEpoch(), { timeout: 10_000 }).toBeGreaterThan(eventEpochBefore);
// ... while the ZIP was CARRIED FORWARD, not rebuilt: its row rides the new epoch (the media
// didn't change, so a full ZIP rebuild would be wasted work).
await waitExportDone(host.jwt);
expect(await zipJobEpoch(), 'the ZIP must be carried forward to the new epoch').toBe(
await eventEpoch()
);
// The keepsake is still downloadable with its single photo intact.
expect(await downloadZipEntries(host.jwt)).toHaveLength(1);
});
test('a comment deleted while the ZIP is still BUILDING does not strand the ZIP', async ({ test('a comment deleted while the ZIP is still BUILDING does not strand the ZIP', async ({
host, host,
guest, guest,