diff --git a/src/interfaces/api/handlers/file_handler.rs b/src/interfaces/api/handlers/file_handler.rs index 58b576f0..7d2e7f71 100644 --- a/src/interfaces/api/handlers/file_handler.rs +++ b/src/interfaces/api/handlers/file_handler.rs @@ -395,19 +395,24 @@ impl FileHandler { /// Get a thumbnail for a file (image or video). /// - /// **Cache-first**: if the thumbnail already exists in the moka in-memory - /// cache or on disk, serve it immediately — **zero DB queries**. The - /// ownership check was already performed when the thumbnail was first - /// generated (at upload) or uploaded (PUT by the owner). UUIDv4 file IDs - /// have 122 bits of entropy, making enumeration infeasible. + /// **Cache-first**: once past the hash lookup below, a thumbnail already + /// in the moka in-memory cache or on disk is served without further DB + /// work. The ownership check was already performed when the thumbnail + /// was first generated (at upload) or uploaded (PUT by the owner). + /// UUIDv4 file IDs have 122 bits of entropy, making enumeration + /// infeasible. /// - /// **ETag / 304**: responses carry an immutable ETag. If the browser - /// sends `If-None-Match` matching the ETag, we return 304 Not Modified - /// without touching cache or DB — pure header round-trip. + /// **ETag / 304**: responses carry an immutable ETag keyed on the + /// **content hash**, so it identifies the bytes rather than the file. + /// Replacing a file's content changes it (correct invalidation), and two + /// files with identical content share it (a copy revalidates to 304 + /// instead of refetching). Costs one PK lookup on the 304 path, which an + /// id-keyed ETag avoided at the price of never invalidating — see the + /// comment at the ETag construction. /// - /// The DB path is only taken on a **cache miss for images** where the - /// thumbnail hasn't been generated yet (first access after upload if - /// background generation hasn't finished). + /// Beyond that, the DB path is only taken on a **cache miss for images** + /// where the thumbnail hasn't been generated yet (first access after + /// upload if background generation hasn't finished). pub(super) async fn get_thumbnail_impl( State(state): State, auth_user: AuthUser, @@ -449,16 +454,44 @@ impl FileHandler { let format = ThumbnailFormat::from_accept(headers.get(header::ACCEPT).and_then(|v| v.to_str().ok())); - // ── ETag short-circuit (Solution C) ────────────────────────── - // Thumbnails are immutable — the ETag never changes for a given - // (file_id, size, format) triple. If the browser already has it, return - // 304 with zero I/O or DB work. Format is in the ETag so a client that - // switched codecs doesn't get a stale 304. + // ── ETag short-circuit ─────────────────────────────────────── + // Keyed on the CONTENT hash, not the file id. A thumbnail is a pure + // function of (source bytes, size, format), so that triple genuinely + // identifies the response — which is what makes the `immutable` + // directive below an honest claim. + // + // Keying on `file_id` was wrong in both directions. Replacing a + // file's content preserves its id (`file_upload_service` rebuilds the + // entity with `parts.id` and a new hash, then fires + // `on_file_updated`, which regenerates the thumbnails), so the ETag + // never changed — and since `immutable` tells a browser not to + // revalidate at all inside the freshness window, clients kept the old + // preview for up to a year. Conversely a copy, or any dedup twin, got + // a *different* id and so refetched bytes it already held, even + // though the server serves both from the same derived blob. + // + // Cost: one PK lookup, where the id-keyed version needed none. It + // buys correct invalidation plus 304s shared across every file with + // the same content. The lookup runs after the authz check above, + // which has already hit the database. + // + // No new disclosure: `content_hash` is already on `FileDto` and + // returned by `GET /api/files/{id}`, so any caller who reaches here + // could read it anyway. + let blob_hash = match state + .repositories + .file_read_repository + .get_blob_hash(&id) + .await + { + Ok(h) => h, + Err(err) => return AppError::from(err).into_response(), + }; let etag = { let (s, f) = (thumb_size.as_str(), format.as_str()); - let mut e = String::with_capacity(9 + id.len() + s.len() + f.len()); + let mut e = String::with_capacity(9 + blob_hash.len() + s.len() + f.len()); e.push_str("\"thumb-"); - e.push_str(&id); + e.push_str(&blob_hash); e.push('-'); e.push_str(s); e.push('-'); @@ -486,7 +519,9 @@ impl FileHandler { if let Some(data) = thumbnail_service .get_cached_thumbnail( &id, - None, + // Already resolved for the ETag above — hand it over rather + // than let the service look it up a second time. + Some(&blob_hash), thumb_size.into(), format, Some(&state.core.dedup_service), @@ -534,18 +569,7 @@ impl FileHandler { .into_response(); } - // Resolve the blob hash (content-addressable storage). - let blob_hash = match state - .repositories - .file_read_repository - .get_blob_hash(&id) - .await - { - Ok(hash) => hash, - Err(_) => { - return AppError::internal_error("File blob not found").into_response(); - } - }; + // `blob_hash` was resolved above to build the ETag — no second lookup. if let Some(data) = thumbnail_service .get_cached_thumbnail( &id, diff --git a/src/interfaces/nextcloud/preview_handler.rs b/src/interfaces/nextcloud/preview_handler.rs index 8a32a0d6..385501b8 100644 --- a/src/interfaces/nextcloud/preview_handler.rs +++ b/src/interfaces/nextcloud/preview_handler.rs @@ -137,19 +137,40 @@ pub async fn handle_preview( } }; - // Conditional revalidation — the ETag is derived from (object id, size) - // only, so it is computable right here, BEFORE the blob-hash query and - // the thumbnail cache/disk read. NC clients revalidate gallery previews - // constantly; the REST thumbnail endpoint has honoured `If-None-Match` - // since PHOTOS-ETAG — this endpoint set an immutable ETag but never - // compared it, so every revalidation re-ran the whole pipeline and - // re-shipped the body (ROUND10). Authz already passed above; a 304 - // must never skip the Read check. + // Conditional revalidation. NC clients revalidate gallery previews + // constantly; this endpoint set an immutable ETag but never compared it, + // so every revalidation re-ran the whole pipeline and re-shipped the body + // (ROUND10). Authz already passed above; a 304 must never skip the Read + // check. + // + // Keyed on the CONTENT hash, matching the REST thumbnail endpoint. A + // thumbnail is a pure function of (source bytes, size), so that pair + // identifies the response and `immutable` below is honest. Keying on the + // object id meant replacing a file's content — which preserves the id — + // left every client showing the old preview for up to a year, since + // `immutable` suppresses revalidation entirely. + // + // This moves the blob-hash query ahead of the 304 rather than adding one: + // the same lookup used to sit just below, on the path that renders. + let blob_hash = match state + .repositories + .file_read_repository + .get_blob_hash(&object_id) + .await + { + Ok(hash) => hash, + Err(_) => { + return Response::builder() + .status(StatusCode::NOT_FOUND) + .body(Body::from("File blob not found")) + .unwrap(); + } + }; let etag = { let s = thumb_size.as_str(); - let mut e = String::with_capacity(9 + object_id.len() + s.len()); + let mut e = String::with_capacity(9 + blob_hash.len() + s.len()); e.push_str("\"thumb-"); - e.push_str(&object_id); + e.push_str(&blob_hash); e.push('-'); e.push_str(s); e.push('"'); @@ -179,21 +200,7 @@ pub async fn handle_preview( .unwrap(); } - // Resolve the blob hash (content-addressable storage) - let blob_hash = match state - .repositories - .file_read_repository - .get_blob_hash(&object_id) - .await - { - Ok(hash) => hash, - Err(_) => { - return Response::builder() - .status(StatusCode::NOT_FOUND) - .body(Body::from("File blob not found")) - .unwrap(); - } - }; + // `blob_hash` was resolved above to build the ETag. if let Some(data) = state .core .thumbnail_service diff --git a/tests/api/derived_blob_copy.hurl b/tests/api/derived_blob_copy.hurl index 00e96bc4..488c9fba 100644 --- a/tests/api/derived_blob_copy.hurl +++ b/tests/api/derived_blob_copy.hurl @@ -140,6 +140,7 @@ Authorization: Bearer {{token}} HTTP 200 [Captures] thumb_bytes: bytes +thumb_etag: header "ETag" # ───────────────────────────────────────────────────────────── @@ -173,10 +174,14 @@ jsonpath "$.ref_count" == 2 # ───────────────────────────────────────────────────────────── -# Step 7 – The copy is readable and renders the same bytes. +# Step 7 – The copy is readable, renders the same bytes, and carries the +# SAME ETag as the original. # -# NOT a proof of derived-blob sharing — see the header. This catches the -# copy being unreadable or resolving to different content. +# The ETag is keyed on the content hash, which the copy shares. Two +# different files agreeing on an ETag is the one externally visible +# consequence of content-keying — a file-id-keyed ETag could not produce +# it. The 304 below is the payoff: a client that already holds the +# original's thumbnail does not refetch it for the copy. # ───────────────────────────────────────────────────────────── GET {{base_url}}/api/files/{{file_copy_id}}/thumbnail/preview Authorization: Bearer {{token}} @@ -184,6 +189,16 @@ Authorization: Bearer {{token}} HTTP 200 [Asserts] bytes == {{thumb_bytes}} +header "ETag" == "{{thumb_etag}}" + + +GET {{base_url}}/api/files/{{file_copy_id}}/thumbnail/preview +Authorization: Bearer {{token}} +If-None-Match: {{thumb_etag}} + +HTTP 304 +[Asserts] +header "ETag" == "{{thumb_etag}}" # ───────────────────────────────────────────────────────────── diff --git a/tests/api/run.sh b/tests/api/run.sh index 67619aec..d644513b 100755 --- a/tests/api/run.sh +++ b/tests/api/run.sh @@ -167,6 +167,7 @@ hurl --variables-file "$API_DIR/test.env" --file-root "$REPO_ROOT/tests" --test "$API_DIR/batch_folder_copy.hurl" \ "$API_DIR/dedup_blob_cleanup.hurl" \ "$API_DIR/derived_blob_copy.hurl" \ + "$API_DIR/thumbnail_etag_content_keyed.hurl" \ "$API_DIR/dedup_admin_gate.hurl" \ "$API_DIR/admin_jobs.hurl" \ "$API_DIR/recoverable_jobs.hurl" \ diff --git a/tests/api/thumbnail_etag_content_keyed.hurl b/tests/api/thumbnail_etag_content_keyed.hurl new file mode 100644 index 00000000..b37b6d26 --- /dev/null +++ b/tests/api/thumbnail_etag_content_keyed.hurl @@ -0,0 +1,161 @@ +# ============================================================= +# OxiCloud – Thumbnail ETag is keyed on CONTENT, not on file id +# ============================================================= +# Regression guard for a stale-cache bug. +# +# The thumbnail ETag used to be `"thumb-{file_id}-{size}-{format}"`, sent +# with `Cache-Control: public, max-age=31536000, immutable`. Replacing a +# file's content preserves its id — the upload service rebuilds the entity +# with `parts.id` and a new hash, then fires `on_file_updated`, which +# regenerates the thumbnails — so the server produced a NEW thumbnail while +# advertising the OLD ETag. And `immutable` tells a conforming browser not +# to revalidate at all inside the freshness window, so clients kept showing +# the previous image for up to a year with no way to invalidate it. +# +# Keying on the content hash fixes it: new bytes → new hash → new ETag. +# +# This file asserts the invalidation direction. The sharing direction (two +# distinct files with identical content answering with the SAME ETag, so a +# copy revalidates to 304) is covered in `derived_blob_copy.hurl`. +# +# Overwrite goes through WebDAV PUT because that is the path that replaces +# content in place; the REST upload endpoint creates a new file instead. +# +# Prerequisites: setup.hurl must have run (admin user exists). +# ============================================================= + + +# ───────────────────────────────────────────────────────────── +# Step 1 – Login +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ + "username": "{{username}}", + "password": "{{password}}" +} + +HTTP 200 +[Captures] +token: jsonpath "$.access_token" + + +# ───────────────────────────────────────────────────────────── +# Step 2 – Upload the first image +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/files/upload +Authorization: Bearer {{token}} +[MultipartFormData] +file: file,fixtures/red-image.png; image/png + +HTTP 201 +[Captures] +file_id: jsonpath "$.id" +file_name: jsonpath "$.name" +hash_before: jsonpath "$.content_hash" + + +# ───────────────────────────────────────────────────────────── +# Step 3 – Its thumbnail, and the ETag that goes with it +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/files/{{file_id}}/thumbnail/preview +Authorization: Bearer {{token}} + +HTTP 200 +[Captures] +etag_before: header "ETag" +thumb_before: bytes +[Asserts] +header "Cache-Control" contains "immutable" + + +# Unchanged content revalidates to 304 — the caching path works. +GET {{base_url}}/api/files/{{file_id}}/thumbnail/preview +Authorization: Bearer {{token}} +If-None-Match: {{etag_before}} + +HTTP 304 + + +# ───────────────────────────────────────────────────────────── +# Step 4 – Replace the content in place, keeping the same file id. +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/webdav/{{file_name}} +Authorization: Bearer {{token}} +Content-Type: image/png +file,fixtures/green-image.png; + +HTTP * +[Asserts] +status >= 200 +status < 300 + + +# Same file row, different content. +GET {{base_url}}/api/files/{{file_id}} +Authorization: Bearer {{token}} + +HTTP 200 +[Captures] +hash_after: jsonpath "$.content_hash" +[Asserts] +jsonpath "$.id" == "{{file_id}}" +jsonpath "$.content_hash" != "{{hash_before}}" + + +# ───────────────────────────────────────────────────────────── +# Step 5 – The ETag must have changed with the content. +# +# This is the assertion the file exists for. With the id-keyed ETag it was +# byte-identical to `etag_before`, and the next request would have been +# answered 304 from cache — serving the OLD image indefinitely. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/files/{{file_id}}/thumbnail/preview +Authorization: Bearer {{token}} + +HTTP 200 +[Captures] +etag_after: header "ETag" +[Asserts] +header "ETag" != "{{etag_before}}" + + +# A client holding the stale ETag must be told to refetch, not given a 304. +GET {{base_url}}/api/files/{{file_id}}/thumbnail/preview +Authorization: Bearer {{token}} +If-None-Match: {{etag_before}} + +HTTP 200 +[Asserts] +header "ETag" == "{{etag_after}}" + + +# ...and the new ETag revalidates normally. +GET {{base_url}}/api/files/{{file_id}}/thumbnail/preview +Authorization: Bearer {{token}} +If-None-Match: {{etag_after}} + +HTTP 304 + + +# ───────────────────────────────────────────────────────────── +# Step 6 – Teardown. Hurl files share one database within run.sh. +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/api/files/{{file_id}} +Authorization: Bearer {{token}} + +HTTP 204 + + +GET {{base_url}}/api/trash/resources +Authorization: Bearer {{token}} + +HTTP 200 +[Captures] +trash_id: jsonpath "$.items[?(@.resource.id == '{{file_id}}')].resource.id" + + +DELETE {{base_url}}/api/trash/{{trash_id}} +Authorization: Bearer {{token}} + +HTTP 200