From a3a93b90ec92d471816ba6aceac8bc9e8cf58a79 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Mon, 24 Aug 2026 23:47:57 +0200 Subject: [PATCH] fix(thumbnails): key the ETag on content hash, not file id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The thumbnail ETag was "thumb-{file_id}-{size}-{format}", sent with Cache-Control: public, max-age=31536000, immutable. 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 deletes and regenerates the thumbnails — so the server produced a new thumbnail while still advertising the old ETag. Because `immutable` tells a conforming browser not to revalidate at all inside the freshness window, clients kept rendering the previous image for up to a year, unfixably. Keyed on the content hash the directive becomes honest: a thumbnail is a pure function of (source bytes, size, format), so that triple identifies the response. New content yields a new ETag. The same change fixes the opposite direction. A copy, or any dedup twin, had a different id and therefore a different ETag, so clients refetched bytes they already held even though both are served from the same derived blob. Now identical content agrees on an ETag and revalidates to 304 across files, users and copies. Both thumbnail endpoints were affected: the REST handler and the NextCloud preview handler. Cost is one PK lookup ahead of the 304 decision, where the id-keyed version needed none — paid for by no longer serving stale images. It is partly recovered: both handlers already resolved the same hash further down for the render path, and that second lookup is now gone, so the cache-miss path is unchanged and only the 304 path pays. The resolved hash is also handed to get_cached_thumbnail instead of None, saving the service its own lookup. No new disclosure: content_hash is already on FileDto and returned by GET /api/files/{id}. Tests: thumbnail_etag_content_keyed.hurl covers invalidation — overwrite in place via WebDAV PUT, assert the ETag changed, assert a client holding the stale one gets 200 rather than 304. derived_blob_copy.hurl gains the sharing direction: a copy answers with the SAME ETag and revalidates to 304, which is the one externally observable consequence of content-keying and was not previously testable. --- src/interfaces/api/handlers/file_handler.rs | 86 +++++++---- src/interfaces/nextcloud/preview_handler.rs | 57 ++++--- tests/api/derived_blob_copy.hurl | 21 ++- tests/api/run.sh | 1 + tests/api/thumbnail_etag_content_keyed.hurl | 161 ++++++++++++++++++++ 5 files changed, 267 insertions(+), 59 deletions(-) create mode 100644 tests/api/thumbnail_etag_content_keyed.hurl 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