cjw/Oxicloud
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix(thumbnails): private, no-cache — the URL is gated and mutable

Browse Source 
Thumbnails were served `public, max-age=31536000, immutable`. Two
problems, and the first is a security one.

`public` on a Permission::Read gated resource lets any shared cache — a
corporate proxy, a CDN — store one user's thumbnail and serve it to
another. `Vary: Accept` was no defence: it does not vary on
Authorization. Now `private`.

`immutable` was a promise this URL cannot keep. It is keyed by file id,
and its bytes change when a preview is uploaded, when content is
replaced, or when an attachment is removed. `immutable` tells a client
not to revalidate at all during the freshness lifetime, so with a
one-year max-age a browser that fetched once would never see a new
preview — which also made the content-keyed ETag unobservable in
practice. A correct validator is worthless if nothing asks. Now
`no-cache`, which still stores the body and only requires revalidation,
answered by the ETag with a body-less 304.

The hurl tests could not have caught this: hurl always sends the
request, so If-None-Match was exercised and passed while a browser
obeying `immutable` never got that far. Same "correct on the wire, wrong
in practice" shape as the bugs before it, so the test now asserts the
directives themselves rather than only the 304 behaviour.

One definition, shared by the REST and NextCloud endpoints, which are
gated identically and must not drift. /_app/immutable is untouched:
those are hash-named static assets, genuinely content-addressed and
public, where the directive is honest.

Cost is a conditional request per thumbnail per page load. Recovering it
needs a content-addressed URL — where `immutable` would be true — but
that puts the hash in the URL of an authorized resource, so it stays
`private` regardless, and it touches the SPA and the file DTO. Separate
change.
This commit is contained in:
Edouard Vanbelle
2026-08-25 23:17:49 +02:00
parent 7d9418f63c
commit 95648f2fa3
3 changed files with 71 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/interfaces/api/handlers/file_handler.rs
+49 -21
View File
@@ -393,6 +393,35 @@ impl FileHandler {
// THUMBNAILS
// ═══════════════════════════════════════════════════════════════════════
/// Cache policy for every thumbnail response.
///
/// **`private`**, because a thumbnail is authorization-gated: the handler
/// runs a `Permission::Read` check before serving it. `public` let any
/// shared cache — a corporate proxy, a CDN — store one user's thumbnail
/// and hand it to another. `Vary: Accept` did not help, because it does
/// not vary on `Authorization`.
///
/// **`no-cache`**, not `immutable`, because this URL is keyed by file id
/// and its bytes are mutable: uploading a preview, replacing the file's
/// content, or removing an attachment all change what it serves.
/// `immutable` promises the opposite, so a client that fetched once would
/// not revalidate — for a year, under the previous `max-age` — and would
/// never see a new preview. That also made the content-keyed ETag
/// unobservable in a browser: a correct validator is worthless if nothing
/// asks.
///
/// `no-cache` still stores the body; it only requires revalidation before
/// reuse, which the ETag answers with a body-less 304.
///
/// The cost is a conditional request per thumbnail per page load. Buying
/// that back needs a content-addressed URL, where `immutable` would be
/// honest — but the hash would then be in the URL of an authorized
/// resource, so it stays `private` regardless. Separate change; it
/// touches the SPA and the file DTO.
/// Shared with the NextCloud preview endpoint, which is gated the same
/// way and must not drift from this policy.
pub(crate) const THUMBNAIL_CACHE_CONTROL: &'static str = "private, no-cache";
/// Get a thumbnail for a file (image or video).
///
/// **Cache-first**: once past the hash lookup below, a thumbnail already
@@ -402,13 +431,15 @@ impl FileHandler {
/// UUIDv4 file IDs have 122 bits of entropy, making enumeration
/// infeasible.
///
/// **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.
/// **ETag / 304**: the ETag names the **blob actually served** — an
/// uploaded preview's hash, else a derived thumbnail's, else the
/// source-keyed form (see `ThumbnailService::thumbnail_content_id`). So
/// replacing content or uploading a preview invalidates correctly, and
/// two files serving identical bytes share a validator. Costs one or two
/// indexed lookups on the 304 path, which an id-keyed ETag avoided at the
/// price of never invalidating. Cache policy is
/// [`Self::THUMBNAIL_CACHE_CONTROL`] — `private, no-cache`, since this
/// URL is authorization-gated and its bytes are mutable.
///
/// 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
@@ -455,20 +486,17 @@ impl FileHandler {
ThumbnailFormat::from_accept(headers.get(header::ACCEPT).and_then(|v| v.to_str().ok()));
// ── 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.
// Keyed on the CONTENT served, not the file id.
//
// 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.
// never changed — and the response was `immutable` with a one-year
// max-age, so clients never revalidated and kept the old preview.
// 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
@@ -511,7 +539,7 @@ impl FileHandler {
.status(StatusCode::NOT_MODIFIED)
.header(header::ETAG, &etag)
.header(header::VARY, header::ACCEPT.as_str())
.header(header::CACHE_CONTROL, "public, max-age=31536000, immutable")
.header(header::CACHE_CONTROL, Self::THUMBNAIL_CACHE_CONTROL)
.body(Body::empty())
.unwrap()
.into_response();
@@ -539,7 +567,7 @@ impl FileHandler {
crate::common::mime_detect::thumbnail_content_type(&data),
)
.header(header::CONTENT_LENGTH, data.len())
.header(header::CACHE_CONTROL, "public, max-age=31536000, immutable")
.header(header::CACHE_CONTROL, Self::THUMBNAIL_CACHE_CONTROL)
.header(header::ETAG, &etag)
.header(header::VARY, header::ACCEPT.as_str())
.body(Body::from(data))
@@ -591,7 +619,7 @@ impl FileHandler {
crate::common::mime_detect::thumbnail_content_type(&data),
)
.header(header::CONTENT_LENGTH, data.len())
.header(header::CACHE_CONTROL, "public, max-age=31536000, immutable")
.header(header::CACHE_CONTROL, Self::THUMBNAIL_CACHE_CONTROL)
.header(header::ETAG, &etag)
.header(header::VARY, header::ACCEPT.as_str())
.body(Body::from(data))
@@ -623,7 +651,7 @@ impl FileHandler {
crate::common::mime_detect::thumbnail_content_type(&data),
)
.header(header::CONTENT_LENGTH, data.len())
.header(header::CACHE_CONTROL, "public, max-age=31536000, immutable")
.header(header::CACHE_CONTROL, Self::THUMBNAIL_CACHE_CONTROL)
.header(header::ETAG, &etag)
.header(header::VARY, header::ACCEPT.as_str())
.body(Body::from(data))
@@ -655,7 +683,7 @@ impl FileHandler {
crate::common::mime_detect::thumbnail_content_type(&data),
)
.header(header::CONTENT_LENGTH, data.len())
.header(header::CACHE_CONTROL, "public, max-age=31536000, immutable")
.header(header::CACHE_CONTROL, Self::THUMBNAIL_CACHE_CONTROL)
.header(header::ETAG, &etag)
.header(header::VARY, header::ACCEPT.as_str())
.body(Body::from(data))
src/interfaces/nextcloud/preview_handler.rs
+13 -9
View File
@@ -17,6 +17,9 @@ use crate::application::ports::storage_ports::FileReadPort;
use crate::application::ports::thumbnail_ports::{ThumbnailFormat, ThumbnailPort, ThumbnailSize};
use crate::common::di::AppState;
use crate::domain::services::authorization::{Permission, Resource, Subject};
// One definition of the thumbnail cache policy, shared with the REST
// endpoint: both are Permission::Read gated, so both must stay `private`.
use crate::interfaces::api::handlers::file_handler::FileHandler;
use crate::interfaces::middleware::auth::AuthUser;
use uuid::Uuid;
@@ -143,12 +146,13 @@ pub async fn handle_preview(
// (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.
// Keyed on the CONTENT of the bytes served, matching the REST thumbnail
// endpoint. Keying on the object id meant replacing a file's content —
// which preserves the id — left the validator unchanged, and the response
// was `immutable` with a one-year max-age, so clients never revalidated
// and showed the old preview indefinitely. Both halves are fixed: the
// ETag names what is served (see `thumbnail_content_id`) and the policy
// is `private, no-cache` (see `FileHandler::THUMBNAIL_CACHE_CONTROL`).
//
// 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.
@@ -189,7 +193,7 @@ pub async fn handle_preview(
{
return Response::builder()
.status(StatusCode::NOT_MODIFIED)
.header(header::CACHE_CONTROL, "public, max-age=31536000, immutable")
.header(header::CACHE_CONTROL, FileHandler::THUMBNAIL_CACHE_CONTROL)
.header(header::ETAG, etag)
.body(Body::empty())
.unwrap();
@@ -226,7 +230,7 @@ pub async fn handle_preview(
.status(StatusCode::OK)
.header(header::CONTENT_TYPE, "image/jpeg")
.header(header::CONTENT_LENGTH, data.len())
.header(header::CACHE_CONTROL, "public, max-age=31536000, immutable")
.header(header::CACHE_CONTROL, FileHandler::THUMBNAIL_CACHE_CONTROL)
.header(header::ETAG, etag)
.body(Body::from(data))
.unwrap();
@@ -251,7 +255,7 @@ pub async fn handle_preview(
.status(StatusCode::OK)
.header(header::CONTENT_TYPE, "image/jpeg")
.header(header::CONTENT_LENGTH, data.len())
.header(header::CACHE_CONTROL, "public, max-age=31536000, immutable")
.header(header::CACHE_CONTROL, FileHandler::THUMBNAIL_CACHE_CONTROL)
.header(header::ETAG, etag)
.body(Body::from(data))
.unwrap(),
tests/api/thumbnail_etag_content_keyed.hurl
+9 -1
View File
@@ -82,7 +82,15 @@ HTTP 200
[Captures]
etag_before: header "ETag"
[Asserts]
header "Cache-Control" contains "immutable"
# `private`, because a thumbnail is Permission::Read gated — `public` let a
# shared proxy hand one user's thumbnail to another. `no-cache` rather than
# `immutable`, because this URL is keyed by file id and its bytes change
# when content is replaced or a preview uploaded; `immutable` suppressed
# revalidation entirely, which made the ETag below unobservable in a real
# client.
header "Cache-Control" contains "private"
header "Cache-Control" contains "no-cache"
header "Cache-Control" not contains "immutable"
# Unchanged content revalidates to 304 — the caching path works.