From f64c2e2af0da162462901346426d659abfce74fc Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 5 Oct 2026 13:54:32 +0200 Subject: [PATCH 01/11] feat(file_storage): public files with anonymous serving, list search/sort, thumbnails Fixes #353, fixes #352. - StoredFile.public (default false; migration uses sa.false()), set on upload (public=true) or PATCH /files/{id}; StoredFileOut gains public/public_url. - Anonymous GET /public/{id}[/{filename}] and /public/{id}/thumbnail, exempted via register_public_routes (GET only). Only public, non-deleted rows resolve; other cases are one 404. Cross-tenant lookup by id uses all_tenants(). ETag/304, Cache-Control public, nosniff, sandbox CSP; active content (HTML, SVG, JS) forced to attachment and streamed. - SecurityHeadersMiddleware keeps a CSP the response already set. - GET /files: q, content_type (exact or "image/" prefix), sort. - GET /files/{id}/thumbnail?w=: Pillow WebP, width snapped to a whitelist, pixel-budget guard, variants cached in the storage backend and dropped with the file. - Browse screen: Public badge and make public/private action. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- docs/modules/file_storage.md | 70 ++++++- .../simple_module_hosting/middleware.py | 4 +- ...1f27b94_file_storage_stored_file_public.py | 35 ++++ .../file_storage/file_storage/constants.py | 43 +++++ .../file_storage/contracts/schemas.py | 11 ++ .../file_storage/endpoints/api.py | 95 ++++++++-- .../file_storage/endpoints/public.py | 66 +++++++ .../file_storage/file_storage/locales/en.json | 13 +- modules/file_storage/file_storage/models.py | 7 + modules/file_storage/file_storage/module.py | 14 ++ .../file_storage/pages/Browse.tsx | 28 ++- .../pages/components/FileTable.tsx | 29 ++- .../file_storage/file_storage/pages/types.ts | 3 + modules/file_storage/file_storage/queries.py | 30 ++- modules/file_storage/file_storage/reads.py | 6 +- modules/file_storage/file_storage/service.py | 12 +- modules/file_storage/file_storage/serving.py | 120 ++++++++++++ .../file_storage/file_storage/thumbnails.py | 113 +++++++++++ .../file_storage/file_storage/visibility.py | 75 ++++++++ modules/file_storage/pyproject.toml | 1 + .../file_storage/tests-js/FileTable.test.tsx | 19 +- .../tests/test_file_storage_public.py | 135 ++++++++++++++ .../tests/test_file_storage_thumbnails.py | 176 ++++++++++++++++++ packages/i18n/src/generated-resources.ts | 7 + packages/i18n/src/keys.generated.ts | 7 + 25 files changed, 1084 insertions(+), 35 deletions(-) create mode 100644 host/migrations/versions/e5a8c1f27b94_file_storage_stored_file_public.py create mode 100644 modules/file_storage/file_storage/endpoints/public.py create mode 100644 modules/file_storage/file_storage/serving.py create mode 100644 modules/file_storage/file_storage/thumbnails.py create mode 100644 modules/file_storage/file_storage/visibility.py create mode 100644 modules/file_storage/tests/test_file_storage_public.py create mode 100644 modules/file_storage/tests/test_file_storage_thumbnails.py diff --git a/docs/modules/file_storage.md b/docs/modules/file_storage.md index ba9d176b..8bf6e105 100644 --- a/docs/modules/file_storage.md +++ b/docs/modules/file_storage.md @@ -17,11 +17,70 @@ Pluggable file storage with two shipped backends — local filesystem and S3-com | Method + path | Body / response | Permission | |---|---|---| -| `POST /api/file-storage/upload` | `multipart` → `StoredFileOut` (201) | `file_storage.upload` | -| `GET /api/file-storage/files` | `?page=&per_page=` → `StoredFileListOut` | `file_storage.download` | +| `POST /api/file-storage/upload` | `multipart` (`file`, optional `public=true`) → `StoredFileOut` (201) | `file_storage.upload` | +| `GET /api/file-storage/files` | `?page=&per_page=&q=&content_type=&sort=` → `StoredFileListOut` | `file_storage.download` | | `GET /api/file-storage/files/{file_id}` | → `StoredFileOut` | `file_storage.download` | +| `PATCH /api/file-storage/files/{file_id}` | `{"public": bool}` → `StoredFileOut` | `file_storage.upload` | +| `GET /api/file-storage/files/{file_id}/thumbnail` | `?w=` → `image/webp` | `file_storage.download` | | `GET /api/file-storage/files/{file_id}/download` | → 302 (S3) or stream (filesystem) | `file_storage.download` | | `DELETE /api/file-storage/files/{file_id}` | → 204 | `file_storage.delete` | +| `GET /api/file-storage/public/{file_id}[/{filename}]` | bytes (**anonymous**) | none, `public` files only | +| `GET /api/file-storage/public/{file_id}/thumbnail` | `?w=` → `image/webp` (**anonymous**) | none, `public` files only | + +### Listing: search, filter, sort + +`GET /files` takes `q` (case-insensitive substring of the original filename; +`%` and `_` match literally), `content_type` (an exact type, or a family +ending in `/` such as `image/`) and `sort` — one of `created_at`, `-created_at` +(default), `name`, `-name`, `size`, `-size` (anything else is a `422`). `name` +sorts case-insensitively; ties break on `id` so pages never overlap. `total` +reflects the filters. + +### Thumbnails + +`GET /files/{id}/thumbnail?w=` returns a Pillow-resized WebP, aspect ratio +preserved, never enlarged. `w` is clamped to 32–1024 and **snapped up** to one +of `64, 128, 256, 512, 1024` (default 256), so a file has at most five +variants. Only `image/jpeg`, `png`, `webp` and `gif` (first frame) have +thumbnails; everything else, including SVG, is `404`. An undecodable image is +`422 file_storage.bad_image`, and an image over 64 megapixels is refused from +its header, before any decode (decompression-bomb guard). + +Variants are cached **in the storage backend** next to the original, under +`{key}.w{width}.webp`: they survive restarts, are shared by all workers, are +bounded by the width whitelist, inherit the tenant key prefix, and are deleted +with the file. A cache hit never re-reads the original. A concurrent first +request may render twice; both write identical bytes. + +### Public files + +`StoredFile.public` (default `false`) opts a file into anonymous serving. +Set it with `public=true` on upload or `PATCH /files/{id}`; both need +`file_storage.upload`, so anyone who may add files may publish them. `StoredFileOut` carries +`public` and, while public, `public_url` (`/api/file-storage/public/{id}/{filename}`), +so consumers never build the URL themselves — an `` on a public page +can use it, or `.../public/{id}/thumbnail?w=256`. + +The public routes are exempt from `AuthMiddleware` through +`register_public_routes` (GET only; uploads, PATCH and deletes stay gated). +Serving rules: + +- Only `public=True`, non-deleted rows resolve; unknown, private and deleted + ids are the same `404`, so existence is not leaked. +- Tenancy: an anonymous request binds no tenant, so the single lookup by + (unguessable) id runs under `all_tenants()` and requires `public=True`. + Making a file public is the owner's explicit choice to publish it + cross-tenant; nothing else is reachable this way. +- `Cache-Control: public, max-age=3600`, a checksum `ETag` (`304` on + `If-None-Match`) and `X-Content-Type-Options: nosniff`. +- Every public response carries `Content-Security-Policy: default-src 'none'; + style-src 'unsafe-inline'; sandbox` (the security-headers middleware now + keeps a CSP the response already set). Active content — HTML, XHTML, SVG, + XML, JavaScript — is forced to `Content-Disposition: attachment` and is + always streamed, so stored XSS on the app origin is not possible. Other + types are `inline`. +- Presigning backends (S3) answer with a `302` to the presigned URL, cached + for at most half the signature's TTL; filesystem backends stream. ### View @@ -48,7 +107,7 @@ from file_storage.contracts import ( | Class | Purpose | |---|---| -| `StoredFileOut` | File metadata: `id`, `key`, `filename`, `content_type`, `size_bytes`, `backend`, `checksum_sha256`, `uploaded_by`, `created_at`. | +| `StoredFileOut` | File metadata: `id`, `key`, `filename`, `content_type`, `size_bytes`, `backend`, `checksum_sha256`, `uploaded_by`, `created_at`, `public`, `public_url`. | | `StoredFileListOut` | `items`, `total`, `page`, `per_page`. | | `FileUploaded` (event) | `file_id`, `key`, `backend`, `size_bytes`, `uploaded_by`. Topic: `file_storage.file.uploaded`. | | `FileDeleted` (event) | `file_id`, `key`. Topic: `file_storage.file.deleted`. | @@ -69,6 +128,7 @@ from file_storage.contracts import ( | `size_bytes` | `int` | | | `backend` | `str(32)` | `"filesystem"` or `"s3"` — recorded at upload time | | `checksum_sha256` | `str(64)` | computed during stream-upload | +| `public` | `bool` | default `false`; opt-in anonymous serving | | `extra_metadata` | `dict` | per-backend extras | | audit + soft-delete | from `AuditMixin` + `SoftDeleteMixin` | | @@ -216,9 +276,9 @@ class MyModule(ModuleBase): ## Inertia pages -- `FileStorage/Browse.tsx` — file list + upload dropzone; handles the upload progress + delete confirmation flow. +- `FileStorage/Browse.tsx` — file list + upload dropzone; handles the upload progress + delete confirmation flow. Each row shows a "Public" badge and a make public / make private action. - `FileStorage/components/UploadDropzone.tsx` — drag-drop upload child component. ## Locales -Top-level keys in `file_storage/locales/en.json`: `browse`, `table`, `actions`, `delete_dialog`, `toasts`, `errors`. The `errors` namespace is keyed by error *code* (`not_found`, `too_large`, `bad_type`, `backend_error`) so the UI can render a deterministic message per `StorageError` subclass. +Top-level keys in `file_storage/locales/en.json`: `browse`, `table`, `actions`, `delete_dialog`, `toasts`, `errors`. The `errors` namespace is keyed by error *code* (`not_found`, `too_large`, `bad_type`, `backend_error`, `bad_image`) so the UI can render a deterministic message per `StorageError` subclass. diff --git a/framework/hosting/simple_module_hosting/middleware.py b/framework/hosting/simple_module_hosting/middleware.py index 91fdbd24..a4cb7c23 100644 --- a/framework/hosting/simple_module_hosting/middleware.py +++ b/framework/hosting/simple_module_hosting/middleware.py @@ -147,7 +147,9 @@ async def send_with_headers(message: Message) -> None: headers[_HEADER_X_FRAME_OPTIONS] = _XFO_SAMEORIGIN headers[_HEADER_X_XSS_PROTECTION] = _XXSS_DISABLED headers[_HEADER_REFERRER_POLICY] = _REFERRER_STRICT_ORIGIN - if self.csp: + # A response that carries its own policy (file_storage sandboxes + # user-uploaded bytes) keeps it; the app-wide one is the default. + if self.csp and _HEADER_CSP not in headers: headers[_HEADER_CSP] = self.csp if self.hsts: headers[_HEADER_HSTS] = self.hsts diff --git a/host/migrations/versions/e5a8c1f27b94_file_storage_stored_file_public.py b/host/migrations/versions/e5a8c1f27b94_file_storage_stored_file_public.py new file mode 100644 index 00000000..ae7b14e2 --- /dev/null +++ b/host/migrations/versions/e5a8c1f27b94_file_storage_stored_file_public.py @@ -0,0 +1,35 @@ +"""file_storage_stored_file: add ``public`` flag + +Opt-in anonymous serving (GH #353). Existing rows stay private. +``server_default=sa.false()`` renders ``0`` on SQLite and ``false`` on +Postgres, so the same migration runs on both. + +Revision ID: e5a8c1f27b94 +Revises: c7f2d9a41e83 +Create Date: 2026-10-05 10:00:00.000000 +""" + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision: str = "e5a8c1f27b94" +down_revision: str | None = "c7f2d9a41e83" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + +_TABLE = "file_storage_stored_file" + + +def upgrade() -> None: + op.add_column( + _TABLE, + sa.Column("public", sa.Boolean(), nullable=False, server_default=sa.false()), + ) + + +def downgrade() -> None: + with op.batch_alter_table(_TABLE) as batch: + batch.drop_column("public") diff --git a/modules/file_storage/file_storage/constants.py b/modules/file_storage/file_storage/constants.py index 935b8824..7f92cdeb 100644 --- a/modules/file_storage/file_storage/constants.py +++ b/modules/file_storage/file_storage/constants.py @@ -34,6 +34,14 @@ PATH_FILES: Final = "/files" PATH_FILE_BY_ID: Final = "/files/{file_id}" PATH_FILE_DOWNLOAD: Final = "/files/{file_id}/download" +PATH_FILE_THUMBNAIL: Final = "/files/{file_id}/thumbnail" +# Anonymous serving of ``public`` files (#353). The thumbnail route is declared +# before the named one, so a file literally called "thumbnail" is reachable +# only through the id-only form. +PUBLIC_SEGMENT: Final = "/public" +PATH_PUBLIC: Final = "/public/{file_id}" +PATH_PUBLIC_THUMBNAIL: Final = "/public/{file_id}/thumbnail" +PATH_PUBLIC_NAMED: Final = "/public/{file_id}/{filename}" # POST, not DELETE: a selection is a body, and DELETE with a body is refused # or silently stripped by enough proxies that it cannot be relied on. PATH_FILES_BULK_DELETE: Final = "/files/bulk-delete" @@ -84,6 +92,7 @@ class ErrorCode: BAD_TYPE: Final = "file_storage.bad_type" NOT_FOUND: Final = "file_storage.not_found" BACKEND_ERROR: Final = "file_storage.backend_error" + BAD_IMAGE: Final = "file_storage.bad_image" class I18nKey: @@ -93,6 +102,7 @@ class I18nKey: ERR_TOO_LARGE: Final = "file_storage.errors.too_large" ERR_BAD_TYPE: Final = "file_storage.errors.bad_type" ERR_BACKEND: Final = "file_storage.errors.backend_error" + ERR_BAD_IMAGE: Final = "file_storage.errors.bad_image" # ── Defaults ───────────────────────────────────────────────────────── @@ -107,6 +117,39 @@ class I18nKey: # uploads. The label is resolved server-side, so this is the only copy. UNKNOWN_UPLOADER: Final = "—" +# ── Public serving & thumbnails ────────────────────────────────────── +PUBLIC_MAX_AGE_SECONDS: Final = 3600 +# Types a browser would execute or render as a document on our origin. They are +# served as attachments (and sandboxed) so a public upload cannot become stored +# XSS on the app's origin. +ACTIVE_CONTENT_TYPES: Final = frozenset( + { + "text/html", + "application/xhtml+xml", + "image/svg+xml", + "text/xml", + "application/xml", + "text/javascript", + "application/javascript", + "application/x-shockwave-flash", + } +) +PUBLIC_CSP: Final = "default-src 'none'; style-src 'unsafe-inline'; sandbox" + +THUMBNAIL_WIDTHS: Final = (64, 128, 256, 512, 1024) +THUMBNAIL_DEFAULT_WIDTH: Final = 256 +THUMBNAIL_MIN_WIDTH: Final = 32 +THUMBNAIL_MAX_WIDTH: Final = 1024 +THUMBNAIL_CONTENT_TYPE: Final = "image/webp" +THUMBNAIL_SOURCE_TYPES: Final = frozenset({"image/jpeg", "image/png", "image/webp", "image/gif"}) +# Decode budget: refuse an image whose pixel count would balloon memory +# (decompression bomb) before Pillow ever decodes it. +THUMBNAIL_MAX_PIXELS: Final = 64_000_000 +THUMBNAIL_MAX_AGE_SECONDS: Final = 86400 + +# ── Listing ────────────────────────────────────────────────────────── +DEFAULT_SORT: Final = "-created_at" + # ── Menu ───────────────────────────────────────────────────────────── MENU_ICON: Final = "files" MENU_ORDER: Final = 40 diff --git a/modules/file_storage/file_storage/contracts/schemas.py b/modules/file_storage/file_storage/contracts/schemas.py index bd518b7e..991150cf 100644 --- a/modules/file_storage/file_storage/contracts/schemas.py +++ b/modules/file_storage/file_storage/contracts/schemas.py @@ -26,6 +26,17 @@ class StoredFileOut(SQLModel): description="User id from AuditMixin.created_by — populated by the audit listener.", ) created_at: datetime | None = None + public: bool = Field(default=False, description="Whether anonymous visitors may fetch it.") + public_url: str | None = Field( + default=None, + description="Anonymous URL, present only while the file is public.", + ) + + +class StoredFileUpdate(SQLModel): + """Body for PATCH /api/file-storage/files/{id}.""" + + public: bool class BulkDeleteRequest(SQLModel): diff --git a/modules/file_storage/file_storage/endpoints/api.py b/modules/file_storage/file_storage/endpoints/api.py index a4c864da..500f14f7 100644 --- a/modules/file_storage/file_storage/endpoints/api.py +++ b/modules/file_storage/file_storage/endpoints/api.py @@ -3,20 +3,32 @@ from __future__ import annotations import uuid +from typing import Literal -from fastapi import APIRouter, Depends, File, HTTPException, Query, UploadFile, status +from fastapi import ( + APIRouter, + Depends, + File, + Form, + HTTPException, + Query, + Response, + UploadFile, + status, +) from fastapi.responses import RedirectResponse, StreamingResponse from simple_module_core.events import EventBus from simple_module_hosting.i18n_deps import TranslatorDep from simple_module_hosting.permissions import RequiresPermission -from file_storage import constants +from file_storage import constants, queries from file_storage.contracts.events import FileDeleted, FileUploaded from file_storage.contracts.schemas import ( BulkDeleteRequest, BulkDeleteResult, StoredFileListOut, StoredFileOut, + StoredFileUpdate, ) from file_storage.deps import get_event_bus, get_file_storage_service from file_storage.format import format_bytes @@ -28,6 +40,7 @@ StoredFileNotFoundError, StreamDownload, ) +from file_storage.serving import thumbnail_response router = APIRouter() @@ -41,11 +54,12 @@ async def upload_file( t: TranslatorDep, file: UploadFile = File(...), + public: bool = Form(default=False), service: FileStorageService = Depends(get_file_storage_service), bus: EventBus = Depends(get_event_bus), ) -> StoredFileOut: try: - out = await service.upload(file) + out = await service.upload(file, public=public) except FileTooLargeError as exc: # The limit belongs in the sentence: "too large" is not actionable to # someone holding a 40 MB file, and every client that shows this @@ -93,9 +107,21 @@ async def list_files( # a 422 for out-of-range paging, while the Inertia views clamp instead. page: int = Query(default=1, ge=1), per_page: int = Query(default=20, ge=1, le=200), + q: str | None = Query( + default=None, description="Case-insensitive substring of the original filename." + ), + content_type: str | None = Query( + default=None, + description="Exact content type, or a family ending in '/' such as 'image/'.", + ), + sort: Literal["created_at", "-created_at", "name", "-name", "size", "-size"] = Query( + default=constants.DEFAULT_SORT + ), service: FileStorageService = Depends(get_file_storage_service), ) -> StoredFileListOut: - items, total = await service.list_files(page=page, per_page=per_page) + items, total = await service.list_files( + page=page, per_page=per_page, search=q, content_type=content_type, sort=sort + ) return StoredFileListOut(items=items, total=total, page=page, per_page=per_page) @@ -119,18 +145,45 @@ async def get_file( "message": t.t(constants.I18nKey.ERR_NOT_FOUND), }, ) from exc - return StoredFileOut.model_validate( - { - "id": row.id, - "key": row.key, - "filename": row.filename, - "content_type": row.content_type, - "size_bytes": row.size_bytes, - "backend": row.backend, - "checksum_sha256": row.checksum_sha256, - "uploaded_by": row.created_by, - "created_at": row.created_at, - } + return StoredFileOut.model_validate(queries.to_out_dict(row)) + + +@router.patch( + constants.PATH_FILE_BY_ID, + response_model=StoredFileOut, + dependencies=[Depends(RequiresPermission(constants.Permission.UPLOAD))], +) +async def update_file( + file_id: uuid.UUID, + body: StoredFileUpdate, + t: TranslatorDep, + service: FileStorageService = Depends(get_file_storage_service), +) -> StoredFileOut: + """Publish or unpublish a file (anyone allowed to upload may decide).""" + try: + row = await service.set_public(file_id, body.public) + except StoredFileNotFoundError as exc: + raise _not_found(t) from exc + return StoredFileOut.model_validate(queries.to_out_dict(row)) + + +@router.get( + constants.PATH_FILE_THUMBNAIL, + response_model=None, + dependencies=[Depends(RequiresPermission(constants.Permission.DOWNLOAD))], +) +async def file_thumbnail( + file_id: uuid.UUID, + t: TranslatorDep, + w: int | None = Query(default=None, description="Width in px; clamped and snapped."), + service: FileStorageService = Depends(get_file_storage_service), +) -> Response: + try: + row = await service.get(file_id) + except StoredFileNotFoundError as exc: + raise _not_found(t) from exc + return await thumbnail_response( + service, row, w, t, cache_control=f"private, max-age={constants.THUMBNAIL_MAX_AGE_SECONDS}" ) @@ -218,3 +271,13 @@ async def delete_file( }, ) from exc await bus.publish(FileDeleted(file_id=row.id, key=row.key)) + + +def _not_found(t: TranslatorDep) -> HTTPException: + return HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail={ + "code": constants.ErrorCode.NOT_FOUND, + "message": t.t(constants.I18nKey.ERR_NOT_FOUND), + }, + ) diff --git a/modules/file_storage/file_storage/endpoints/public.py b/modules/file_storage/file_storage/endpoints/public.py new file mode 100644 index 00000000..c1cadc0c --- /dev/null +++ b/modules/file_storage/file_storage/endpoints/public.py @@ -0,0 +1,66 @@ +"""Anonymous read routes for files marked ``public`` (#353). + +Exempted from ``AuthMiddleware`` by ``FileStorageModule.register_public_routes`` +(GET only). Every miss — unknown, private, soft-deleted — is the same 404, so +the route cannot be used to learn which ids exist. +""" + +from __future__ import annotations + +import uuid + +from fastapi import APIRouter, Depends, HTTPException, Query, Request, Response, status +from simple_module_hosting.i18n_deps import TranslatorDep + +from file_storage import constants +from file_storage.deps import get_file_storage_service +from file_storage.service import FileStorageService, StoredFileNotFoundError +from file_storage.serving import public_file_response, thumbnail_response + +router = APIRouter() + + +async def _public_row(service: FileStorageService, file_id: uuid.UUID, t: TranslatorDep): + try: + return await service.get_public(file_id) + except StoredFileNotFoundError as exc: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail={ + "code": constants.ErrorCode.NOT_FOUND, + "message": t.t(constants.I18nKey.ERR_NOT_FOUND), + }, + ) from exc + + +# Declared before the ``{filename}`` route so "thumbnail" is not read as a name. +@router.get(constants.PATH_PUBLIC_THUMBNAIL, response_model=None) +async def public_thumbnail( + file_id: uuid.UUID, + request: Request, + t: TranslatorDep, + w: int | None = Query(default=None, description="Width in px; clamped and snapped."), + service: FileStorageService = Depends(get_file_storage_service), +) -> Response: + row = await _public_row(service, file_id, t) + return await thumbnail_response( + service, + row, + w, + t, + cache_control=f"public, max-age={constants.PUBLIC_MAX_AGE_SECONDS}", + request=request, + ) + + +@router.get(constants.PATH_PUBLIC, response_model=None) +@router.get(constants.PATH_PUBLIC_NAMED, response_model=None) +async def public_file( + file_id: uuid.UUID, + request: Request, + t: TranslatorDep, + filename: str | None = None, # cosmetic: lets the URL end in a readable name + service: FileStorageService = Depends(get_file_storage_service), +) -> Response: + row = await _public_row(service, file_id, t) + return await public_file_response(service, row, request) diff --git a/modules/file_storage/file_storage/locales/en.json b/modules/file_storage/file_storage/locales/en.json index f716b3ab..d5f2e288 100644 --- a/modules/file_storage/file_storage/locales/en.json +++ b/modules/file_storage/file_storage/locales/en.json @@ -29,10 +29,13 @@ "when": "When", "actions": "Actions", "select_all": "Select every file on this page", - "select_row": "Select {name}" + "select_row": "Select {name}", + "public": "Public" }, "actions": { - "download": "Download" + "download": "Download", + "make_public": "Make public", + "make_private": "Make private" }, "delete_dialog": { "title_one": "Delete “{name}”?", @@ -48,6 +51,9 @@ "deleted_one": "“{name}” deleted", "deleted_other": "{count} files deleted", "delete_failed": "Failed to delete file", + "made_public": "“{name}” is now public", + "made_private": "“{name}” is now private", + "visibility_failed": "Could not change visibility", "uploaded_count_one": "{count} file uploaded", "uploaded_count_other": "{count} files uploaded", "upload_failed_named": "“{name}” failed to upload" @@ -56,7 +62,8 @@ "not_found": "File not found", "too_large": "File exceeds the {max_size} limit for a single upload", "bad_type": "This file type is not allowed", - "backend_error": "Storage backend error" + "backend_error": "Storage backend error", + "bad_image": "This image could not be processed" }, "filters": { "search_placeholder": "Search filenames…", diff --git a/modules/file_storage/file_storage/models.py b/modules/file_storage/file_storage/models.py index b0a67961..630cb83f 100644 --- a/modules/file_storage/file_storage/models.py +++ b/modules/file_storage/file_storage/models.py @@ -39,6 +39,13 @@ class StoredFile(Base, AuditMixin, SoftDeleteMixin, MultiTenantMixin, table=True size_bytes: int = Field() backend: str = Field(max_length=32) checksum_sha256: str = Field(max_length=64) + # Opt-in anonymous serving (#353): only ``public`` rows are reachable from + # ``GET {prefix}/public/{id}``. ``sa.false()`` renders as ``0`` on SQLite and + # ``false`` on Postgres, which a literal ``text("0")`` default would not. + public: bool = Field( + default=False, + sa_column=sa.Column(sa.Boolean(), nullable=False, server_default=sa.false()), + ) extra_metadata: dict = Field( default_factory=dict, sa_type=sa.JSON, diff --git a/modules/file_storage/file_storage/module.py b/modules/file_storage/file_storage/module.py index 73635c1a..d4d5d352 100644 --- a/modules/file_storage/file_storage/module.py +++ b/modules/file_storage/file_storage/module.py @@ -4,6 +4,7 @@ import importlib.resources import logging +import re from pathlib import Path from typing import TYPE_CHECKING @@ -13,6 +14,7 @@ from simple_module_core.menu import MenuItem, MenuRegistry, MenuSection from simple_module_core.module import ModuleBase, ModuleMeta from simple_module_core.permissions import PermissionRegistry +from simple_module_core.public_routes import PublicRouteRegistry from simple_module_core.tenancy import TenantRole, tenant_role from file_storage import constants @@ -88,11 +90,23 @@ def register_settings(self, app: FastAPI) -> None: def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None: from file_storage.endpoints.api import router as api + from file_storage.endpoints.public import router as public from file_storage.endpoints.views import router as views api_router.include_router(api) + api_router.include_router(public) view_router.include_router(views) + def register_public_routes(self, registry: PublicRouteRegistry) -> None: + """Let anyone GET a file its owner marked public (#353). + + GET-only and anchored to ``/public/{id}[/{name}|/thumbnail]``, so + uploads, deletes and the authenticated download keep requiring a + session. The handler itself still refuses anything not ``public``. + """ + base = re.escape(f"{constants.ROUTE_PREFIX_API}{constants.PUBLIC_SEGMENT}") + registry.add_regex(rf"{base}/[^/]+(/[^/]+)?$", methods={"GET"}) + def register_audit_links(self, registry: AuditLinkRegistry) -> None: """Name file rows in the audit log, and tag them with their table. diff --git a/modules/file_storage/file_storage/pages/Browse.tsx b/modules/file_storage/file_storage/pages/Browse.tsx index 22b6a376..f1500728 100644 --- a/modules/file_storage/file_storage/pages/Browse.tsx +++ b/modules/file_storage/file_storage/pages/Browse.tsx @@ -19,7 +19,7 @@ import { UploadDropzone } from './components/UploadDropzone'; import { UploadsCard } from './components/UploadsCard'; import { PERMISSIONS, RELOAD_PROPS, ROUTES } from './constants'; import { describeTypes, formatBytes } from './format'; -import type { BrowseProps, FileFilters } from './types'; +import type { BrowseProps, FileFilters, StoredFile } from './types'; import { useUploadQueue } from './upload-queue'; function Browse() { @@ -87,6 +87,30 @@ function Browse() { } } + async function handleTogglePublic(file: StoredFile) { + try { + const resp = await fetch(ROUTES.apiFile(file.id), { + method: 'PATCH', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ public: !file.public }), + }); + if (!resp.ok) throw new Error('visibility change failed'); + toast.success( + t( + file.public + ? keys.file_storage.toasts.made_private + : keys.file_storage.toasts.made_public, + { + name: file.filename, + }, + ), + ); + router.reload({ only: RELOAD_PROPS }); + } catch { + toast.error(t(keys.file_storage.toasts.visibility_failed)); + } + } + async function handleDelete() { setDeleting(true); try { @@ -188,6 +212,8 @@ function Browse() { files={files} selectedIds={selectedIds} canDelete={canDelete} + canPublish={canUpload} + onTogglePublic={handleTogglePublic} onToggleRow={(id, on) => select(on ? [...selectedIds, id] : selectedIds.filter((x) => x !== id)) } diff --git a/modules/file_storage/file_storage/pages/components/FileTable.tsx b/modules/file_storage/file_storage/pages/components/FileTable.tsx index 8f4fe76c..7faec28e 100644 --- a/modules/file_storage/file_storage/pages/components/FileTable.tsx +++ b/modules/file_storage/file_storage/pages/components/FileTable.tsx @@ -1,5 +1,6 @@ import { keys, useT } from '@simple-module-py/i18n'; import { TableEmptyRow } from '@simple-module-py/ui/components/TableEmptyRow'; +import { Badge } from '@simple-module-py/ui/components/ui/badge'; import { Button } from '@simple-module-py/ui/components/ui/button'; import { Checkbox } from '@simple-module-py/ui/components/ui/checkbox'; import { @@ -21,6 +22,9 @@ interface Props { files: StoredFile[]; selectedIds: string[]; canDelete: boolean; + /** Whether the viewer may publish/unpublish (the upload permission). */ + canPublish: boolean; + onTogglePublic: (file: StoredFile) => void; onToggleRow: (id: string, selected: boolean) => void; onToggleAll: (selected: boolean) => void; /** Rendered in place of the rows when there is nothing to show. */ @@ -52,6 +56,8 @@ export function FileTable({ files, selectedIds, canDelete, + canPublish, + onTogglePublic, onToggleRow, onToggleAll, empty, @@ -108,7 +114,14 @@ export function FileTable({ /> )} - {file.filename} + + {file.filename} + {file.public && ( + + {t(keys.file_storage.table.public)} + + )} + {file.content_type} @@ -130,6 +143,20 @@ export function FileTable({ > {t(keys.file_storage.actions.download)} + {canPublish && ( + + )} ); diff --git a/modules/file_storage/file_storage/pages/types.ts b/modules/file_storage/file_storage/pages/types.ts index 22886ea5..e78d0088 100644 --- a/modules/file_storage/file_storage/pages/types.ts +++ b/modules/file_storage/file_storage/pages/types.ts @@ -10,6 +10,9 @@ export interface StoredFile { /** ``uploaded_by`` resolved server-side to a name; "—" when nobody was recorded. */ uploaded_by_label: string; created_at: string | null; + /** Anonymous visitors may fetch it at ``public_url``. */ + public: boolean; + public_url: string | null; } export interface Pagination { diff --git a/modules/file_storage/file_storage/queries.py b/modules/file_storage/file_storage/queries.py index 4fd73ce2..d7b33d41 100644 --- a/modules/file_storage/file_storage/queries.py +++ b/modules/file_storage/file_storage/queries.py @@ -13,14 +13,28 @@ from __future__ import annotations +from urllib.parse import quote + from simple_module_db import LIKE_ESCAPE_CHAR, like_contains_pattern, like_prefix_pattern from sqlalchemy import func, select from sqlalchemy.ext.asyncio import AsyncSession +from file_storage import constants from file_storage.contracts.schemas import StoredFileOut from file_storage.models import StoredFile +def order_clauses(sort: str) -> list: + """ORDER BY for a ``sort`` token; ``id`` breaks ties so pages never overlap.""" + field = sort.lstrip("-") + column = { + "created_at": StoredFile.created_at, + "name": func.lower(StoredFile.filename), + "size": StoredFile.size_bytes, + }.get(field, StoredFile.created_at) + return [column.desc() if sort.startswith("-") else column.asc(), StoredFile.id] + + def filter_clauses( *, created_by: str | None, @@ -91,13 +105,14 @@ async def page_of_files( created_by: str | None = None, search: str | None = None, content_type: str | None = None, + sort: str = constants.DEFAULT_SORT, ) -> list[StoredFileOut]: - """One page of rows, newest first, narrowed by the same filters as the count.""" + """One page of rows, narrowed by the same filters as the count (newest first by default).""" query = select(StoredFile) for clause in filter_clauses(created_by=created_by, search=search, content_type=content_type): query = query.where(clause) result = await db.execute( - query.order_by(StoredFile.created_at.desc()).offset((page - 1) * per_page).limit(per_page) + query.order_by(*order_clauses(sort)).offset((page - 1) * per_page).limit(per_page) ) return [StoredFileOut.model_validate(to_out_dict(r)) for r in result.scalars().all()] @@ -110,6 +125,7 @@ async def list_files( created_by: str | None = None, search: str | None = None, content_type: str | None = None, + sort: str = constants.DEFAULT_SORT, ) -> tuple[list[StoredFileOut], int]: """Page plus total, for callers whose page number is already known good. @@ -118,7 +134,7 @@ async def list_files( """ filters = {"created_by": created_by, "search": search, "content_type": content_type} total = await count_files(db, **filters) - items = await page_of_files(db, page=page, per_page=per_page, **filters) + items = await page_of_files(db, page=page, per_page=per_page, sort=sort, **filters) return items, total @@ -140,9 +156,17 @@ async def content_type_facets(db: AsyncSession, *, created_by: str | None = None return [{"value": str(row[0]), "count": int(row[1])} for row in rows] +def public_url_for(file_id: object, filename: str) -> str: + """The anonymous URL of a public file, with its name as a trailing segment.""" + name = quote(filename, safe="") + return f"{constants.ROUTE_PREFIX_API}{constants.PUBLIC_SEGMENT}/{file_id}/{name}" + + def to_out_dict(row: StoredFile) -> dict: """Project ORM row → DTO dict, mapping ``created_by`` to ``uploaded_by``.""" return { + "public": row.public, + "public_url": public_url_for(row.id, row.filename) if row.public else None, "id": row.id, "key": row.key, "filename": row.filename, diff --git a/modules/file_storage/file_storage/reads.py b/modules/file_storage/file_storage/reads.py index e9a9d1a4..a10a5e22 100644 --- a/modules/file_storage/file_storage/reads.py +++ b/modules/file_storage/file_storage/reads.py @@ -17,7 +17,7 @@ from sqlalchemy.ext.asyncio import AsyncSession -from file_storage import aggregates, queries +from file_storage import aggregates, constants, queries from file_storage.contracts.schemas import StoredFileOut if TYPE_CHECKING: @@ -38,6 +38,7 @@ async def list_files( created_by: str | None = None, search: str | None = None, content_type: str | None = None, + sort: str = constants.DEFAULT_SORT, ) -> tuple[list[StoredFileOut], int]: return await queries.list_files( self.db, @@ -46,6 +47,7 @@ async def list_files( created_by=created_by, search=search, content_type=content_type, + sort=sort, ) async def count_files( @@ -67,6 +69,7 @@ async def page_of_files( created_by: str | None = None, search: str | None = None, content_type: str | None = None, + sort: str = constants.DEFAULT_SORT, ) -> list[StoredFileOut]: return await queries.page_of_files( self.db, @@ -75,6 +78,7 @@ async def page_of_files( created_by=created_by, search=search, content_type=content_type, + sort=sort, ) async def storage_aggregates(self) -> StorageAggregates: diff --git a/modules/file_storage/file_storage/service.py b/modules/file_storage/file_storage/service.py index b969856f..c6c82a76 100644 --- a/modules/file_storage/file_storage/service.py +++ b/modules/file_storage/file_storage/service.py @@ -21,12 +21,13 @@ from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession -from file_storage import constants, queries +from file_storage import constants, queries, thumbnails from file_storage.contracts.schemas import StoredFileOut from file_storage.contracts.service import StorageNotFoundError from file_storage.models import StoredFile from file_storage.reads import FileStorageReads from file_storage.scope import PLATFORM_TENANT_ID, owning_tenant, platform_scope +from file_storage.visibility import FileStoragePublic if TYPE_CHECKING: from file_storage.aggregates import AggregateCache @@ -68,7 +69,7 @@ class RedirectDownload: Download = StreamDownload | RedirectDownload -class FileStorageService(FileStorageReads): +class FileStorageService(FileStoragePublic, FileStorageReads): """Orchestrates validation, hashing, backend IO, and DB lifecycle.""" def __init__( @@ -90,7 +91,9 @@ def __init__( # ── Upload ─────────────────────────────────────────────────────── - async def upload(self, upload: UploadFile, *, platform: bool = False) -> StoredFileOut: + async def upload( + self, upload: UploadFile, *, platform: bool = False, public: bool = False + ) -> StoredFileOut: """Validate, stream-hash, persist to backend, and record metadata. The row belongs to the bound tenant, or — with ``platform=True`` — to @@ -141,6 +144,7 @@ async def _hashing_stream() -> AsyncIterator[bytes]: size_bytes=size, backend=self.backend.backend_id, checksum_sha256=sha.hexdigest(), + public=public, ) async with platform_scope(self.db, platform): self.db.add(row) @@ -232,6 +236,7 @@ async def delete_many(self, file_ids: Sequence[uuid.UUID]) -> list[StoredFile]: # problem, not the caller's. try: await self.backend.delete(row.key) + await thumbnails.delete_variants(self.backend, row.key) except StorageNotFoundError: # Acceptably absent — eg. a previous delete partially succeeded. pass @@ -256,6 +261,7 @@ async def delete(self, file_id: uuid.UUID, *, platform: bool = False) -> StoredF # Object is acceptably absent — eg. a previous delete partially succeeded. with contextlib.suppress(StorageNotFoundError): await self.backend.delete(row.key) + await thumbnails.delete_variants(self.backend, row.key) return row diff --git a/modules/file_storage/file_storage/serving.py b/modules/file_storage/file_storage/serving.py new file mode 100644 index 00000000..d844f62e --- /dev/null +++ b/modules/file_storage/file_storage/serving.py @@ -0,0 +1,120 @@ +"""HTTP responses for file bytes and thumbnails, shared by the authenticated and +anonymous routes so both apply the same headers and the same safety rules.""" + +from __future__ import annotations + +from urllib.parse import quote + +from fastapi import HTTPException, Request, Response, status +from fastapi.responses import RedirectResponse, StreamingResponse + +from file_storage import constants, thumbnails +from file_storage.models import StoredFile +from file_storage.service import FileStorageService + + +def _etag(row: StoredFile, suffix: str = "") -> str: + return f'"{row.checksum_sha256}{suffix}"' + + +def _not_modified(request: Request, etag: str) -> bool: + sent = request.headers.get("if-none-match", "") + return etag in {part.strip().removeprefix("W/") for part in sent.split(",")} + + +def is_active_content(content_type: str) -> bool: + return content_type.split(";")[0].strip().lower() in constants.ACTIVE_CONTENT_TYPES + + +def content_disposition(filename: str, *, attachment: bool) -> str: + ascii_name = filename.encode("ascii", "ignore").decode().replace('"', "").replace("\\", "") + kind = "attachment" if attachment else "inline" + return f"{kind}; filename=\"{ascii_name or 'file'}\"; filename*=UTF-8''{quote(filename)}" + + +async def thumbnail_response( + service: FileStorageService, + row: StoredFile, + width: int | None, + t, + *, + cache_control: str, + request: Request | None = None, +) -> Response: + """Serve ``row``'s resized variant; 404 for non-images, 422 if undecodable.""" + snapped = thumbnails.snap_width(width) + etag = _etag(row, f"-w{snapped}") + headers = { + "ETag": etag, + "Cache-Control": cache_control, + "X-Content-Type-Options": "nosniff", + "Content-Security-Policy": constants.PUBLIC_CSP, + } + if ( + request is not None + and _not_modified(request, etag) + and thumbnails.is_thumbnailable(row.content_type) + ): + return Response(status_code=status.HTTP_304_NOT_MODIFIED, headers=headers) + try: + data, _ = await service.thumbnail(row, snapped) + except thumbnails.NotAnImageError as exc: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail={ + "code": constants.ErrorCode.NOT_FOUND, + "message": t.t(constants.I18nKey.ERR_NOT_FOUND), + }, + ) from exc + except thumbnails.UnreadableImageError as exc: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail={ + "code": constants.ErrorCode.BAD_IMAGE, + "message": t.t(constants.I18nKey.ERR_BAD_IMAGE), + }, + ) from exc + return Response(content=data, media_type=constants.THUMBNAIL_CONTENT_TYPE, headers=headers) + + +async def public_file_response( + service: FileStorageService, row: StoredFile, request: Request +) -> Response: + """Serve a file already authorised as public. + + Active content (HTML, SVG, scripts) is forced to download and sandboxed, + and is streamed even on presigning backends so these headers always apply; + everything else may redirect to the backend's own URL. + """ + max_age = constants.PUBLIC_MAX_AGE_SECONDS + etag = _etag(row) + active = is_active_content(row.content_type) + headers = { + "ETag": etag, + "Cache-Control": f"public, max-age={max_age}", + "X-Content-Type-Options": "nosniff", + "Content-Security-Policy": constants.PUBLIC_CSP, + } + if _not_modified(request, etag): + return Response(status_code=status.HTTP_304_NOT_MODIFIED, headers=headers) + + if service.backend.supports_presigned_url and not active: + url = await service.presigned_url(row) + # A cached redirect must not outlive the signature it points at. + redirect_age = min(max_age, service.settings.s3_presign_ttl_seconds // 2) + return RedirectResponse( + url=url, + status_code=status.HTTP_302_FOUND, + headers={**headers, "Cache-Control": f"public, max-age={redirect_age}"}, + ) + + body = await service.stream(row) + return StreamingResponse( + body, + media_type=row.content_type, + headers={ + **headers, + "Content-Disposition": content_disposition(row.filename, attachment=active), + "Content-Length": str(row.size_bytes), + }, + ) diff --git a/modules/file_storage/file_storage/thumbnails.py b/modules/file_storage/file_storage/thumbnails.py new file mode 100644 index 00000000..9a5ccaad --- /dev/null +++ b/modules/file_storage/file_storage/thumbnails.py @@ -0,0 +1,113 @@ +"""Resized image variants for the browse grid, pickers and public pages (#352). + +Variants are cached **in the storage backend**, beside the original, under a +key derived from the original's (``{key}.w{width}.webp``). That is the simplest +cache that is also robust: it survives restarts and is shared by every worker, +it needs no eviction policy of its own because the width is snapped to a short +whitelist (:data:`~file_storage.constants.THUMBNAIL_WIDTHS`, so at most five +variants per file), it inherits the original's tenant prefix, and it is dropped +with the original (:func:`delete_variants`). The cost is one extra object per +size actually requested. + +Pillow work runs in a thread: decoding a large photo would otherwise stall the +event loop for every other request. +""" + +from __future__ import annotations + +import asyncio +import io +from typing import TYPE_CHECKING + +from PIL import Image, ImageOps + +from file_storage import constants +from file_storage.contracts.service import StorageNotFoundError + +if TYPE_CHECKING: + from file_storage.contracts.service import StorageBackend + + +class NotAnImageError(Exception): + """The file's type has no thumbnail (not a raster image, or SVG).""" + + +class UnreadableImageError(Exception): + """The bytes are not a decodable image, or are too large to decode safely.""" + + +def is_thumbnailable(content_type: str) -> bool: + return content_type.split(";")[0].strip().lower() in constants.THUMBNAIL_SOURCE_TYPES + + +def snap_width(width: int | None) -> int: + """Clamp to the allowed range, then round up to the next whitelisted size.""" + wanted = constants.THUMBNAIL_DEFAULT_WIDTH if width is None else width + wanted = max(constants.THUMBNAIL_MIN_WIDTH, min(wanted, constants.THUMBNAIL_MAX_WIDTH)) + return next(w for w in constants.THUMBNAIL_WIDTHS if w >= wanted) + + +def variant_key(key: str, width: int) -> str: + return f"{key}.w{width}.webp" + + +def render(data: bytes, width: int) -> bytes: + """Resize ``data`` to at most ``width`` px wide, keeping aspect; WebP out. + + Never enlarges. The header is read lazily by ``Image.open``, so the pixel + budget is checked *before* any decode — a few KB of PNG can declare a + gigapixel canvas. + """ + try: + with Image.open(io.BytesIO(data)) as img: + if img.width * img.height > constants.THUMBNAIL_MAX_PIXELS: + raise UnreadableImageError("image exceeds the pixel budget") + img.draft("RGB", (width * 2, width * 2)) # cheap JPEG downscale on decode + img = ImageOps.exif_transpose(img) # honour camera rotation + img.thumbnail((width, width * 64), Image.Resampling.LANCZOS) + mode = "RGBA" if img.mode in ("RGBA", "LA", "PA", "P") else "RGB" + out = io.BytesIO() + img.convert(mode).save(out, format="WEBP", quality=80) + return out.getvalue() + except UnreadableImageError: + raise + except (OSError, ValueError, Image.DecompressionBombError) as exc: + raise UnreadableImageError(str(exc)) from exc + + +async def _read_all(backend: StorageBackend, key: str) -> bytes: + return b"".join([chunk async for chunk in await backend.get(key)]) + + +async def get_or_create( + backend: StorageBackend, *, key: str, content_type: str, width: int | None +) -> tuple[bytes, int]: + """The variant's bytes and the snapped width, generating it on first use.""" + if not is_thumbnailable(content_type): + raise NotAnImageError(content_type) + snapped = snap_width(width) + cached_key = variant_key(key, snapped) + try: + return await _read_all(backend, cached_key), snapped + except StorageNotFoundError: + pass + + source = await _read_all(backend, key) + data = await asyncio.to_thread(render, source, snapped) + + async def _once(): + yield data + + await backend.put( + cached_key, _once(), content_type=constants.THUMBNAIL_CONTENT_TYPE, size=len(data) + ) + return data, snapped + + +async def delete_variants(backend: StorageBackend, key: str) -> None: + """Drop every cached variant of ``key``; absent ones are fine.""" + for width in constants.THUMBNAIL_WIDTHS: + try: + await backend.delete(variant_key(key, width)) + except Exception: + continue diff --git a/modules/file_storage/file_storage/visibility.py b/modules/file_storage/file_storage/visibility.py new file mode 100644 index 00000000..942d04d2 --- /dev/null +++ b/modules/file_storage/file_storage/visibility.py @@ -0,0 +1,75 @@ +"""Public-file and thumbnail operations of :class:`~file_storage.service.FileStorageService`. + +Mixed into the service so ``service.py`` stays about upload/download/delete. +""" + +from __future__ import annotations + +import uuid +from collections.abc import AsyncIterator +from typing import TYPE_CHECKING + +from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession + +from file_storage import thumbnails +from file_storage.models import StoredFile + +if TYPE_CHECKING: + from file_storage.contracts.service import StorageBackend + from file_storage.settings import FileStorageSettings + + +class FileStoragePublic: + """Anonymous-serving lookups, the public flag, and thumbnails.""" + + db: AsyncSession + backend: StorageBackend + settings: FileStorageSettings + + if TYPE_CHECKING: + + async def get(self, file_id: uuid.UUID, *, platform: bool = False) -> StoredFile: ... + + async def get_public(self, file_id: uuid.UUID) -> StoredFile: + """A ``public`` file by id, whichever tenant owns it; anything else misses. + + An anonymous request has no tenant bound, so the tenant filter would + fail closed (or hide every row). The lookup is therefore explicitly + cross-tenant — safe because it is a single-row fetch by an unguessable + UUID, restricted to ``public=True``, and the soft-delete filter still + applies. A private, deleted or unknown id all raise the same error, so + the route cannot be used to probe for existence. + """ + stmt = ( + select(StoredFile) + .where(StoredFile.id == file_id, StoredFile.public.is_(True)) + .execution_options(all_tenants=True) + ) + row = (await self.db.execute(stmt)).scalar_one_or_none() + if row is None: + from file_storage.service import StoredFileNotFoundError # circular at import time + + raise StoredFileNotFoundError(str(file_id)) + return row + + async def set_public(self, file_id: uuid.UUID, public: bool) -> StoredFile: + """Publish or unpublish one of the bound tenant's files.""" + row = await self.get(file_id) + row.public = public + await self.db.flush() + await self.db.refresh(row) + return row + + async def thumbnail(self, row: StoredFile, width: int | None) -> tuple[bytes, int]: + """Cached resized variant of ``row`` (see :mod:`file_storage.thumbnails`).""" + return await thumbnails.get_or_create( + self.backend, key=row.key, content_type=row.content_type, width=width + ) + + async def stream(self, row: StoredFile) -> AsyncIterator[bytes]: + """The object's bytes, for a row the caller has already authorised.""" + return await self.backend.get(row.key) + + async def presigned_url(self, row: StoredFile) -> str: + return await self.backend.presigned_get_url(row.key, self.settings.s3_presign_ttl_seconds) diff --git a/modules/file_storage/pyproject.toml b/modules/file_storage/pyproject.toml index fdfa4f40..9ed89c49 100644 --- a/modules/file_storage/pyproject.toml +++ b/modules/file_storage/pyproject.toml @@ -26,6 +26,7 @@ dependencies = [ "simple_module_hosting==0.0.35", "simple_module_settings==0.0.35", "aiofiles>=23", + "pillow>=10", ] [project.optional-dependencies] diff --git a/modules/file_storage/tests-js/FileTable.test.tsx b/modules/file_storage/tests-js/FileTable.test.tsx index be190c40..358c246a 100644 --- a/modules/file_storage/tests-js/FileTable.test.tsx +++ b/modules/file_storage/tests-js/FileTable.test.tsx @@ -1,6 +1,6 @@ import '@testing-library/jest-dom/vitest'; import { configureI18n } from '@simple-module-py/i18n'; -import { render, screen } from '@testing-library/react'; +import { fireEvent, render, screen } from '@testing-library/react'; import { describe, expect, test, vi } from 'vitest'; configureI18n({ @@ -15,6 +15,9 @@ configureI18n({ 'file_storage.table.when': 'When', 'file_storage.table.actions': 'Actions', 'file_storage.actions.download': 'Download', + 'file_storage.actions.make_public': 'Make public', + 'file_storage.actions.make_private': 'Make private', + 'file_storage.table.public': 'Public', }, }); @@ -32,12 +35,16 @@ const FILES: StoredFile[] = [ } as StoredFile, ]; +const onTogglePublic = vi.fn(); + function renderTable(selectedIds: string[] = []) { return render( { expect(screen.getByRole('checkbox', { name: 'Select every file on this page' })).toBeChecked(); }); }); + +describe('FileTable visibility', () => { + test('a private file offers Make public and calls back with the row', () => { + renderTable(); + + fireEvent.click(screen.getByRole('button', { name: 'Make public' })); + + expect(onTogglePublic).toHaveBeenCalledWith(FILES[0]); + }); +}); diff --git a/modules/file_storage/tests/test_file_storage_public.py b/modules/file_storage/tests/test_file_storage_public.py new file mode 100644 index 00000000..e7cc2b60 --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_public.py @@ -0,0 +1,135 @@ +"""Anonymous serving of ``public`` files (#353).""" + +from __future__ import annotations + +import uuid + +import httpx +from file_storage import constants + +API = constants.ROUTE_PREFIX_API + + +async def _upload(client, name="a.txt", data=b"hello", ctype="text/plain", **form): + resp = await client.post( + f"{API}{constants.PATH_UPLOAD}", files={"file": (name, data, ctype)}, data=form + ) + assert resp.status_code == 201, resp.text + return resp.json() + + +async def test_upload_private_by_default_has_no_public_url(authenticated_client): + body = await _upload(authenticated_client) + assert body["public"] is False + assert body["public_url"] is None + + +async def test_upload_public_returns_url_and_anonymous_get_works( + authenticated_client, client: httpx.AsyncClient +): + body = await _upload(authenticated_client, public="true") + assert body["public"] is True + assert body["public_url"] == f"{API}/public/{body['id']}/a.txt" + + for url in (f"{API}/public/{body['id']}", body["public_url"]): + resp = await client.get(url) + assert resp.status_code == 200, resp.text + assert resp.content == b"hello" + assert resp.headers["cache-control"].startswith("public, max-age=") + assert resp.headers["x-content-type-options"] == "nosniff" + assert "sandbox" in resp.headers["content-security-policy"] + assert resp.headers["content-disposition"].startswith("inline") + etag = resp.headers["etag"] + again = await client.get(f"{API}/public/{body['id']}", headers={"If-None-Match": etag}) + assert again.status_code == 304 + + +async def test_private_file_is_404_anonymously(authenticated_client, client): + body = await _upload(authenticated_client) + assert (await client.get(f"{API}/public/{body['id']}")).status_code == 404 + assert (await client.get(f"{API}/public/{uuid.uuid4()}")).status_code == 404 + + +async def test_patch_toggles_public(authenticated_client, client): + body = await _upload(authenticated_client) + url = f"{API}/files/{body['id']}" + resp = await authenticated_client.patch(url, json={"public": True}) + assert resp.status_code == 200 + assert resp.json()["public_url"] + assert (await authenticated_client.get(url)).json()["public"] is True + assert (await client.get(f"{API}/public/{body['id']}")).status_code == 200 + + resp = await authenticated_client.patch(url, json={"public": False}) + assert resp.json()["public"] is False + assert (await client.get(f"{API}/public/{body['id']}")).status_code == 404 + + +async def test_patch_requires_auth_and_known_id(authenticated_client, client): + assert ( + await authenticated_client.patch(f"{API}/files/{uuid.uuid4()}", json={"public": True}) + ).status_code == 404 + body = await _upload(authenticated_client) + anon = await client.patch(f"{API}/files/{body['id']}", json={"public": True}) + assert anon.status_code in (401, 302, 403) + + +async def test_deleted_public_file_is_404(authenticated_client, client): + body = await _upload(authenticated_client, public="true") + assert (await client.get(f"{API}/public/{body['id']}")).status_code == 200 + assert (await authenticated_client.delete(f"{API}/files/{body['id']}")).status_code == 204 + assert (await client.get(f"{API}/public/{body['id']}")).status_code == 404 + + +async def test_active_content_is_attachment_and_sandboxed(authenticated_client, client): + body = await _upload( + authenticated_client, + name="x.svg", + data=b"", + ctype="image/svg+xml", + public="true", + ) + resp = await client.get(f"{API}/public/{body['id']}") + assert resp.status_code == 200 + assert resp.headers["content-disposition"].startswith("attachment") + assert "sandbox" in resp.headers["content-security-policy"] + + +async def test_public_write_routes_stay_gated(client): + body = {"file": ("a.txt", b"x", "text/plain")} + resp = await client.post(f"{API}{constants.PATH_UPLOAD}", files=body) + assert resp.status_code in (401, 302, 403) + resp = await client.get(f"{API}/files") + assert resp.status_code in (401, 302, 403) + + +async def test_cross_tenant_public_file_served_but_private_not(app, client): + """Anonymous callers bind no tenant: a public file of any tenant resolves + by id (it is public by its owner's choice); a private one never does.""" + from file_storage.models import StoredFile + from simple_module_db import tenant_context + + ids = {} + async with app.state.sm.db.session_factory() as session: + with tenant_context("acme"): + for name, public in (("pub", True), ("priv", False)): + row = StoredFile( + key=f"acme/{name}", + filename=f"{name}.txt", + content_type="text/plain", + size_bytes=1, + backend=constants.BackendId.FILESYSTEM, + checksum_sha256="0" * 64, + public=public, + ) + session.add(row) + await session.flush() + ids[name] = row.id + await session.commit() + services = app.state.file_storage + + async def _gen(): + yield b"x" + + await services.backend.put("acme/pub", _gen(), content_type="text/plain", size=1) + assert (await client.get(f"{API}/public/{ids['pub']}")).status_code == 200 + assert (await client.get(f"{API}/public/{ids['priv']}")).status_code == 404 diff --git a/modules/file_storage/tests/test_file_storage_thumbnails.py b/modules/file_storage/tests/test_file_storage_thumbnails.py new file mode 100644 index 00000000..c70c2ca5 --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_thumbnails.py @@ -0,0 +1,176 @@ +"""Thumbnails (#352) and list search/filter/sort (#352).""" + +from __future__ import annotations + +import io + +from file_storage import constants, thumbnails +from PIL import Image + +API = constants.ROUTE_PREFIX_API + + +def _png(width=2000, height=1000) -> bytes: + buf = io.BytesIO() + Image.new("RGB", (width, height), (200, 30, 30)).save(buf, format="PNG") + return buf.getvalue() + + +async def _upload(client, name, data, ctype, **form): + resp = await client.post( + f"{API}{constants.PATH_UPLOAD}", files={"file": (name, data, ctype)}, data=form + ) + assert resp.status_code == 201, resp.text + return resp.json() + + +def _size(content: bytes) -> tuple[int, int]: + with Image.open(io.BytesIO(content)) as img: + return img.size + + +def test_snap_width_clamps_and_rounds_up(): + assert thumbnails.snap_width(None) == 256 + assert thumbnails.snap_width(1) == 64 + assert thumbnails.snap_width(100) == 128 + assert thumbnails.snap_width(5000) == 1024 + + +async def test_thumbnail_sizes_preserve_aspect(authenticated_client): + body = await _upload(authenticated_client, "p.png", _png(), "image/png") + for w, expected in ((100, 128), (None, 256), (9999, 1024)): + url = f"{API}/files/{body['id']}/thumbnail" + resp = await authenticated_client.get(url, params={"w": w} if w else None) + assert resp.status_code == 200, resp.text + assert resp.headers["content-type"] == "image/webp" + width, height = _size(resp.content) + assert width == expected + assert height == expected // 2 + + +async def test_thumbnail_never_enlarges(authenticated_client): + body = await _upload(authenticated_client, "s.png", _png(40, 20), "image/png") + resp = await authenticated_client.get(f"{API}/files/{body['id']}/thumbnail", params={"w": 512}) + assert _size(resp.content) == (40, 20) + + +async def test_thumbnail_non_image_and_svg_404(authenticated_client): + txt = await _upload(authenticated_client, "a.txt", b"hi", "text/plain") + svg = await _upload(authenticated_client, "a.svg", b"", "image/svg+xml") + for body in (txt, svg): + resp = await authenticated_client.get(f"{API}/files/{body['id']}/thumbnail") + assert resp.status_code == 404 + + +async def test_corrupt_image_is_422(authenticated_client): + body = await _upload(authenticated_client, "bad.png", b"not a png", "image/png") + resp = await authenticated_client.get(f"{API}/files/{body['id']}/thumbnail") + assert resp.status_code == 422 + + +def test_pixel_budget_refused_before_decode(monkeypatch): + monkeypatch.setattr(constants, "THUMBNAIL_MAX_PIXELS", 100) + try: + thumbnails.render(_png(50, 50), 64) + except thumbnails.UnreadableImageError: + return + raise AssertionError("expected UnreadableImageError") + + +async def test_thumbnail_is_cached_in_backend(app, authenticated_client): + body = await _upload(authenticated_client, "p.png", _png(), "image/png") + backend = app.state.file_storage.backend + key = body["key"] + assert not await backend.exists(thumbnails.variant_key(key, 128)) + first = await authenticated_client.get(f"{API}/files/{body['id']}/thumbnail", params={"w": 128}) + assert await backend.exists(thumbnails.variant_key(key, 128)) + + # Replace the source: a cache hit must not re-read or re-render it. + async def _gen(): + yield b"garbage" + + await backend.put(key, _gen(), content_type="image/png", size=7) + second = await authenticated_client.get( + f"{API}/files/{body['id']}/thumbnail", params={"w": 100} + ) + assert second.status_code == 200 + assert second.content == first.content + + +async def test_delete_drops_variants(app, authenticated_client): + body = await _upload(authenticated_client, "p.png", _png(), "image/png") + await authenticated_client.get(f"{API}/files/{body['id']}/thumbnail", params={"w": 128}) + backend = app.state.file_storage.backend + await authenticated_client.delete(f"{API}/files/{body['id']}") + assert not await backend.exists(thumbnails.variant_key(body["key"], 128)) + + +async def test_public_thumbnail(authenticated_client, client): + pub = await _upload(authenticated_client, "p.png", _png(), "image/png", public="true") + priv = await _upload(authenticated_client, "q.png", _png(), "image/png") + resp = await client.get(f"{API}/public/{pub['id']}/thumbnail", params={"w": 64}) + assert resp.status_code == 200 + assert resp.headers["cache-control"].startswith("public") + assert _size(resp.content)[0] == 64 + assert (await client.get(f"{API}/public/{priv['id']}/thumbnail")).status_code == 404 + + +# ── list search / filter / sort ────────────────────────────────────── + + +async def _seed(client): + for name, data, ctype in ( + ("Alpha.png", b"1", "image/png"), + ("beta.jpg", b"22", "image/jpeg"), + ("100%_done.txt", b"333", "text/plain"), + ("gamma.pdf", b"4444", "application/pdf"), + ): + await _upload(client, name, data, ctype) + + +async def _names(client, **params): + resp = await client.get(f"{API}/files", params=params) + assert resp.status_code == 200, resp.text + return [i["filename"] for i in resp.json()["items"]], resp.json()["total"] + + +async def test_list_search_is_case_insensitive_substring(authenticated_client): + await _seed(authenticated_client) + assert (await _names(authenticated_client, q="ALPH"))[0] == ["Alpha.png"] + assert (await _names(authenticated_client, q="a"))[1] == 3 + + +async def test_list_search_escapes_like_wildcards(authenticated_client): + await _seed(authenticated_client) + assert (await _names(authenticated_client, q="%"))[0] == ["100%_done.txt"] + assert (await _names(authenticated_client, q="_"))[0] == ["100%_done.txt"] + assert (await _names(authenticated_client, q="0%_d"))[0] == ["100%_done.txt"] + + +async def test_list_content_type_exact_and_prefix(authenticated_client): + await _seed(authenticated_client) + names, total = await _names(authenticated_client, content_type="image/", sort="name") + assert names == ["Alpha.png", "beta.jpg"] and total == 2 + assert (await _names(authenticated_client, content_type="image/png"))[0] == ["Alpha.png"] + assert (await _names(authenticated_client, content_type="application/pdf"))[1] == 1 + + +async def test_list_sort_orders(authenticated_client): + await _seed(authenticated_client) + assert (await _names(authenticated_client, sort="name"))[0] == [ + "100%_done.txt", + "Alpha.png", + "beta.jpg", + "gamma.pdf", + ] + assert (await _names(authenticated_client, sort="-name"))[0][0] == "gamma.pdf" + assert (await _names(authenticated_client, sort="size"))[0][0] == "Alpha.png" + assert (await _names(authenticated_client, sort="-size"))[0][0] == "gamma.pdf" + assert (await _names(authenticated_client, sort="created_at"))[0][0] == "Alpha.png" + # default is newest first + assert (await _names(authenticated_client))[0][0] == "gamma.pdf" + + +async def test_list_rejects_unknown_sort(authenticated_client): + resp = await authenticated_client.get(f"{API}/files", params={"sort": "bogus"}) + assert resp.status_code == 422 diff --git a/packages/i18n/src/generated-resources.ts b/packages/i18n/src/generated-resources.ts index 1575203f..3ae1acb3 100644 --- a/packages/i18n/src/generated-resources.ts +++ b/packages/i18n/src/generated-resources.ts @@ -356,6 +356,8 @@ export default { 'feature_flags.toasts.enabled': '', 'feature_flags.toasts.toggle_failed': '', 'file_storage.actions.download': '', + 'file_storage.actions.make_private': '', + 'file_storage.actions.make_public': '', 'file_storage.audit.file': '', 'file_storage.browse.clear_filters': '', 'file_storage.browse.delete_selected': '', @@ -382,6 +384,7 @@ export default { 'file_storage.delete_dialog.title_one': '', 'file_storage.delete_dialog.title_other': '', 'file_storage.errors.backend_error': '', + 'file_storage.errors.bad_image': '', 'file_storage.errors.bad_type': '', 'file_storage.errors.not_found': '', 'file_storage.errors.too_large': '', @@ -395,6 +398,7 @@ export default { 'file_storage.nav.files': '', 'file_storage.table.actions': '', 'file_storage.table.filename': '', + 'file_storage.table.public': '', 'file_storage.table.select_all': '', 'file_storage.table.select_row': '', 'file_storage.table.size': '', @@ -404,10 +408,13 @@ export default { 'file_storage.toasts.delete_failed': '', 'file_storage.toasts.deleted_one': '', 'file_storage.toasts.deleted_other': '', + 'file_storage.toasts.made_private': '', + 'file_storage.toasts.made_public': '', 'file_storage.toasts.upload_failed': '', 'file_storage.toasts.upload_failed_named': '', 'file_storage.toasts.uploaded_count_one': '', 'file_storage.toasts.uploaded_count_other': '', + 'file_storage.toasts.visibility_failed': '', 'file_storage.upload.any_type': '', 'file_storage.upload.browse_hint': '', 'file_storage.upload.cancel': '', diff --git a/packages/i18n/src/keys.generated.ts b/packages/i18n/src/keys.generated.ts index b21567cf..3ac6f242 100644 --- a/packages/i18n/src/keys.generated.ts +++ b/packages/i18n/src/keys.generated.ts @@ -452,6 +452,8 @@ export const keys = { file_storage: { actions: { download: 'file_storage.actions.download', + make_private: 'file_storage.actions.make_private', + make_public: 'file_storage.actions.make_public', }, audit: { file: 'file_storage.audit.file', @@ -490,6 +492,7 @@ export const keys = { }, errors: { backend_error: 'file_storage.errors.backend_error', + bad_image: 'file_storage.errors.bad_image', bad_type: 'file_storage.errors.bad_type', not_found: 'file_storage.errors.not_found', too_large: 'file_storage.errors.too_large', @@ -509,6 +512,7 @@ export const keys = { table: { actions: 'file_storage.table.actions', filename: 'file_storage.table.filename', + public: 'file_storage.table.public', select_all: 'file_storage.table.select_all', select_row: 'file_storage.table.select_row', size: 'file_storage.table.size', @@ -521,11 +525,14 @@ export const keys = { deleted: 'file_storage.toasts.deleted', deleted_one: 'file_storage.toasts.deleted_one', deleted_other: 'file_storage.toasts.deleted_other', + made_private: 'file_storage.toasts.made_private', + made_public: 'file_storage.toasts.made_public', upload_failed: 'file_storage.toasts.upload_failed', upload_failed_named: 'file_storage.toasts.upload_failed_named', uploaded_count: 'file_storage.toasts.uploaded_count', uploaded_count_one: 'file_storage.toasts.uploaded_count_one', uploaded_count_other: 'file_storage.toasts.uploaded_count_other', + visibility_failed: 'file_storage.toasts.visibility_failed', }, upload: { any_type: 'file_storage.upload.any_type', From 4cd14d79b209311940733c3ea931f6fddad2a6c1 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 5 Oct 2026 14:07:25 +0200 Subject: [PATCH 02/11] fix(file_storage): harden thumbnails (format allowlist, size/pixel caps, concurrency) and add tenant/permission tests Pillow only opens JPEG/PNG/WEBP/GIF, sniffed format must match the declared type, DecompressionBombWarning is an error, source bytes are capped while reading, decodes are bounded by a semaphore, first frame only, metadata not carried over. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- docs/modules/file_storage.md | 7 +- .../file_storage/file_storage/constants.py | 4 +- .../file_storage/file_storage/thumbnails.py | 75 ++++++++--- .../test_file_storage_thumbnail_hardening.py | 118 ++++++++++++++++++ 4 files changed, 183 insertions(+), 21 deletions(-) create mode 100644 modules/file_storage/tests/test_file_storage_thumbnail_hardening.py diff --git a/docs/modules/file_storage.md b/docs/modules/file_storage.md index 8bf6e105..b09f86b6 100644 --- a/docs/modules/file_storage.md +++ b/docs/modules/file_storage.md @@ -43,8 +43,11 @@ preserved, never enlarged. `w` is clamped to 32–1024 and **snapped up** to one of `64, 128, 256, 512, 1024` (default 256), so a file has at most five variants. Only `image/jpeg`, `png`, `webp` and `gif` (first frame) have thumbnails; everything else, including SVG, is `404`. An undecodable image is -`422 file_storage.bad_image`, and an image over 64 megapixels is refused from -its header, before any decode (decompression-bomb guard). +`422 file_storage.bad_image`, and an image over 25 megapixels, or a source over 20 MB, is refused before any +decode (decompression-bomb guard; `DecompressionBombWarning` is an error). Only +JPEG/PNG/WebP/GIF are ever opened (Pillow `formats=` allowlist), the sniffed +format must match the declared type, animated images yield their first frame, +metadata is not carried into the output, and at most two decodes run at once. Variants are cached **in the storage backend** next to the original, under `{key}.w{width}.webp`: they survive restarts, are shared by all workers, are diff --git a/modules/file_storage/file_storage/constants.py b/modules/file_storage/file_storage/constants.py index 7f92cdeb..2cd9390e 100644 --- a/modules/file_storage/file_storage/constants.py +++ b/modules/file_storage/file_storage/constants.py @@ -144,7 +144,9 @@ class I18nKey: THUMBNAIL_SOURCE_TYPES: Final = frozenset({"image/jpeg", "image/png", "image/webp", "image/gif"}) # Decode budget: refuse an image whose pixel count would balloon memory # (decompression bomb) before Pillow ever decodes it. -THUMBNAIL_MAX_PIXELS: Final = 64_000_000 +THUMBNAIL_MAX_PIXELS: Final = 25_000_000 +THUMBNAIL_MAX_SOURCE_BYTES: Final = 20 * 1024 * 1024 +THUMBNAIL_MAX_CONCURRENCY: Final = 2 THUMBNAIL_MAX_AGE_SECONDS: Final = 86400 # ── Listing ────────────────────────────────────────────────────────── diff --git a/modules/file_storage/file_storage/thumbnails.py b/modules/file_storage/file_storage/thumbnails.py index 9a5ccaad..71abe992 100644 --- a/modules/file_storage/file_storage/thumbnails.py +++ b/modules/file_storage/file_storage/thumbnails.py @@ -17,6 +17,7 @@ import asyncio import io +import warnings from typing import TYPE_CHECKING from PIL import Image, ImageOps @@ -51,32 +52,69 @@ def variant_key(key: str, width: int) -> str: return f"{key}.w{width}.webp" -def render(data: bytes, width: int) -> bytes: +_FORMATS_BY_TYPE = { + "image/jpeg": "JPEG", + "image/png": "PNG", + "image/webp": "WEBP", + "image/gif": "GIF", +} +# Pillow's decoders are a large native attack surface; only these four are ever +# opened (``formats=``), never SVG/EPS/PSD/TIFF/PDF-style containers. +_ALLOWED_FORMATS = tuple(_FORMATS_BY_TYPE.values()) + +# At most this many decodes run at once, so a burst of cold-cache requests +# cannot pin every CPU or hold N decoded canvases in memory together. +_DECODE_SLOTS = asyncio.Semaphore(constants.THUMBNAIL_MAX_CONCURRENCY) + + +def render(data: bytes, width: int, content_type: str | None = None) -> bytes: """Resize ``data`` to at most ``width`` px wide, keeping aspect; WebP out. - Never enlarges. The header is read lazily by ``Image.open``, so the pixel + Never enlarges. The header is parsed lazily by ``Image.open``, so the pixel budget is checked *before* any decode — a few KB of PNG can declare a - gigapixel canvas. + gigapixel canvas. The sniffed format must be on the allowlist *and* match + the declared content type (a ``.png`` that is really a PSD is refused). + Animated inputs yield their first frame; the output is re-encoded from + pixels only, so EXIF/XMP/ICC and other metadata are not carried over. """ + previous = Image.MAX_IMAGE_PIXELS + Image.MAX_IMAGE_PIXELS = constants.THUMBNAIL_MAX_PIXELS try: - with Image.open(io.BytesIO(data)) as img: - if img.width * img.height > constants.THUMBNAIL_MAX_PIXELS: - raise UnreadableImageError("image exceeds the pixel budget") - img.draft("RGB", (width * 2, width * 2)) # cheap JPEG downscale on decode - img = ImageOps.exif_transpose(img) # honour camera rotation - img.thumbnail((width, width * 64), Image.Resampling.LANCZOS) - mode = "RGBA" if img.mode in ("RGBA", "LA", "PA", "P") else "RGB" - out = io.BytesIO() - img.convert(mode).save(out, format="WEBP", quality=80) - return out.getvalue() + with warnings.catch_warnings(): + warnings.simplefilter("error", Image.DecompressionBombWarning) + with Image.open(io.BytesIO(data), formats=_ALLOWED_FORMATS) as img: + if content_type is not None and img.format != _FORMATS_BY_TYPE.get( + content_type.split(";")[0].strip().lower() + ): + raise UnreadableImageError("content does not match the declared type") + if img.width * img.height > constants.THUMBNAIL_MAX_PIXELS: + raise UnreadableImageError("image exceeds the pixel budget") + img.seek(0) # first frame only + img.draft("RGB", (width * 2, width * 2)) # cheap JPEG downscale on decode + frame = ImageOps.exif_transpose(img) + frame.thumbnail((width, width * 64), Image.Resampling.LANCZOS) + mode = "RGBA" if frame.mode in ("RGBA", "LA", "PA", "P") else "RGB" + out = io.BytesIO() + frame.convert(mode).save(out, format="WEBP", quality=80) + return out.getvalue() except UnreadableImageError: raise - except (OSError, ValueError, Image.DecompressionBombError) as exc: + except (OSError, ValueError, EOFError, Image.DecompressionBombError, Warning) as exc: raise UnreadableImageError(str(exc)) from exc + finally: + Image.MAX_IMAGE_PIXELS = previous -async def _read_all(backend: StorageBackend, key: str) -> bytes: - return b"".join([chunk async for chunk in await backend.get(key)]) +async def _read_all(backend: StorageBackend, key: str, *, limit: int | None = None) -> bytes: + """Whole object, aborting once ``limit`` bytes are exceeded (no unbounded buffer).""" + chunks: list[bytes] = [] + total = 0 + async for chunk in await backend.get(key): + total += len(chunk) + if limit is not None and total > limit: + raise UnreadableImageError("source image is too large to thumbnail") + chunks.append(chunk) + return b"".join(chunks) async def get_or_create( @@ -92,8 +130,9 @@ async def get_or_create( except StorageNotFoundError: pass - source = await _read_all(backend, key) - data = await asyncio.to_thread(render, source, snapped) + async with _DECODE_SLOTS: + source = await _read_all(backend, key, limit=constants.THUMBNAIL_MAX_SOURCE_BYTES) + data = await asyncio.to_thread(render, source, snapped, content_type) async def _once(): yield data diff --git a/modules/file_storage/tests/test_file_storage_thumbnail_hardening.py b/modules/file_storage/tests/test_file_storage_thumbnail_hardening.py new file mode 100644 index 00000000..b41be5f1 --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_thumbnail_hardening.py @@ -0,0 +1,118 @@ +"""Pillow attack-surface limits and tenant/permission edges of the new routes.""" + +from __future__ import annotations + +import io +from uuid import uuid4 + +import pytest +from file_storage import constants, thumbnails +from PIL import Image + +API = constants.ROUTE_PREFIX_API + + +def _png(width=200, height=100) -> bytes: + buf = io.BytesIO() + Image.new("RGB", (width, height), (10, 20, 30)).save(buf, format="PNG") + return buf.getvalue() + + +async def _upload(client, name, data, ctype, **form): + resp = await client.post( + f"{API}{constants.PATH_UPLOAD}", files={"file": (name, data, ctype)}, data=form + ) + assert resp.status_code == 201, resp.text + return resp.json() + + +def test_animated_gif_yields_first_frame_only(): + buf = io.BytesIO() + frames = [Image.new("RGB", (300, 150), c) for c in ((255, 0, 0), (0, 0, 255))] + frames[0].save(buf, format="GIF", save_all=True, append_images=frames[1:]) + out = thumbnails.render(buf.getvalue(), 128, "image/gif") + with Image.open(io.BytesIO(out)) as img: + assert getattr(img, "n_frames", 1) == 1 + assert img.size == (128, 64) + + +def test_exif_metadata_is_not_carried_over(): + buf = io.BytesIO() + exif = Image.Exif() + exif[0x010E] = "secret description" + Image.new("RGB", (200, 100)).save(buf, format="JPEG", exif=exif) + out = thumbnails.render(buf.getvalue(), 128, "image/jpeg") + assert b"secret description" not in out + with Image.open(io.BytesIO(out)) as img: + assert not img.getexif() + + +def test_sniffed_format_must_match_declared_type(): + with pytest.raises(thumbnails.UnreadableImageError): + thumbnails.render(_png(10, 10), 64, "image/jpeg") + + +def test_formats_outside_the_allowlist_are_refused(): + buf = io.BytesIO() + Image.new("RGB", (10, 10)).save(buf, format="BMP") + with pytest.raises(thumbnails.UnreadableImageError): + thumbnails.render(buf.getvalue(), 64, "image/png") + + +def test_pixel_budget_applies_even_under_bomb_warning(monkeypatch): + monkeypatch.setattr(constants, "THUMBNAIL_MAX_PIXELS", 100) + with pytest.raises(thumbnails.UnreadableImageError): + thumbnails.render(_png(50, 50), 64, "image/png") + + +async def test_oversized_source_is_refused_before_decode(authenticated_client, monkeypatch): + body = await _upload(authenticated_client, "p.png", _png(), "image/png") + monkeypatch.setattr(constants, "THUMBNAIL_MAX_SOURCE_BYTES", 10) + resp = await authenticated_client.get(f"{API}/files/{body['id']}/thumbnail", params={"w": 64}) + assert resp.status_code == 422 + + +async def test_mislabelled_upload_is_422(authenticated_client): + body = await _upload(authenticated_client, "x.png", b"", "image/png") + resp = await authenticated_client.get(f"{API}/files/{body['id']}/thumbnail") + assert resp.status_code == 422 + + +async def test_other_tenants_file_is_invisible_to_patch_thumbnail_download( + app, authenticated_client +): + from file_storage.models import StoredFile + from simple_module_db import tenant_context + + async with app.state.sm.db.session_factory() as session: + with tenant_context("acme"): + row = StoredFile( + key="acme/other.png", + filename="other.png", + content_type="image/png", + size_bytes=1, + backend=constants.BackendId.FILESYSTEM, + checksum_sha256="0" * 64, + ) + session.add(row) + await session.flush() + file_id = row.id + await session.commit() + url = f"{API}/files/{file_id}" + assert (await authenticated_client.patch(url, json={"public": True})).status_code == 404 + assert (await authenticated_client.get(f"{url}/thumbnail")).status_code == 404 + assert (await authenticated_client.get(f"{url}/download")).status_code == 404 + + +async def test_private_and_unknown_public_responses_are_identical(authenticated_client, client): + body = await _upload(authenticated_client, "q.png", _png(), "image/png") + private = await client.get(f"{API}/public/{body['id']}/thumbnail") + unknown = await client.get(f"{API}/public/{uuid4()}/thumbnail") + assert private.status_code == unknown.status_code == 404 + assert private.json() == unknown.json() + + +async def test_anonymous_cannot_use_authenticated_thumbnail(authenticated_client, client): + body = await _upload(authenticated_client, "q.png", _png(), "image/png", public="true") + resp = await client.get(f"{API}/files/{body['id']}/thumbnail") + assert resp.status_code in (401, 302, 403) From adbcbf9bceb740c10472b0f05fbae4e538e98e1a Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 5 Oct 2026 14:18:23 +0200 Subject: [PATCH 03/11] fix(file_storage): hold the decode slot until the worker thread finishes; stop mutating Pillow's global pixel cap Decode runs as a shielded single-flight task per variant, so a cancelled request neither frees its slot early nor lets retries multiply decodes. The semaphore is per event loop. The explicit header-only pixel budget replaces the racy process-global MAX_IMAGE_PIXELS toggle. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../file_storage/file_storage/thumbnails.py | 90 +++++++++++++------ .../test_file_storage_thumbnail_hardening.py | 65 ++++++++++++++ 2 files changed, 127 insertions(+), 28 deletions(-) diff --git a/modules/file_storage/file_storage/thumbnails.py b/modules/file_storage/file_storage/thumbnails.py index 71abe992..6d5d33bf 100644 --- a/modules/file_storage/file_storage/thumbnails.py +++ b/modules/file_storage/file_storage/thumbnails.py @@ -17,7 +17,7 @@ import asyncio import io -import warnings +import weakref from typing import TYPE_CHECKING from PIL import Image, ImageOps @@ -63,8 +63,20 @@ def variant_key(key: str, width: int) -> str: _ALLOWED_FORMATS = tuple(_FORMATS_BY_TYPE.values()) # At most this many decodes run at once, so a burst of cold-cache requests -# cannot pin every CPU or hold N decoded canvases in memory together. -_DECODE_SLOTS = asyncio.Semaphore(constants.THUMBNAIL_MAX_CONCURRENCY) +# cannot pin every CPU or hold N decoded canvases in memory together. One +# semaphore per event loop (an asyncio primitive belongs to one loop). +_SLOTS: weakref.WeakKeyDictionary[asyncio.AbstractEventLoop, asyncio.Semaphore] = ( + weakref.WeakKeyDictionary() +) +# Single-flight: concurrent cold requests for one variant share one decode. +_INFLIGHT: dict[tuple[int, str], asyncio.Task[bytes]] = {} + + +def _slots() -> asyncio.Semaphore: + loop = asyncio.get_running_loop() + if loop not in _SLOTS: + _SLOTS[loop] = asyncio.Semaphore(constants.THUMBNAIL_MAX_CONCURRENCY) + return _SLOTS[loop] def render(data: bytes, width: int, content_type: str | None = None) -> bytes: @@ -77,32 +89,30 @@ def render(data: bytes, width: int, content_type: str | None = None) -> bytes: Animated inputs yield their first frame; the output is re-encoded from pixels only, so EXIF/XMP/ICC and other metadata are not carried over. """ - previous = Image.MAX_IMAGE_PIXELS - Image.MAX_IMAGE_PIXELS = constants.THUMBNAIL_MAX_PIXELS try: - with warnings.catch_warnings(): - warnings.simplefilter("error", Image.DecompressionBombWarning) - with Image.open(io.BytesIO(data), formats=_ALLOWED_FORMATS) as img: - if content_type is not None and img.format != _FORMATS_BY_TYPE.get( - content_type.split(";")[0].strip().lower() - ): - raise UnreadableImageError("content does not match the declared type") - if img.width * img.height > constants.THUMBNAIL_MAX_PIXELS: - raise UnreadableImageError("image exceeds the pixel budget") - img.seek(0) # first frame only - img.draft("RGB", (width * 2, width * 2)) # cheap JPEG downscale on decode - frame = ImageOps.exif_transpose(img) - frame.thumbnail((width, width * 64), Image.Resampling.LANCZOS) - mode = "RGBA" if frame.mode in ("RGBA", "LA", "PA", "P") else "RGB" - out = io.BytesIO() - frame.convert(mode).save(out, format="WEBP", quality=80) - return out.getvalue() + with Image.open(io.BytesIO(data), formats=_ALLOWED_FORMATS) as img: + if content_type is not None and img.format != _FORMATS_BY_TYPE.get( + content_type.split(";")[0].strip().lower() + ): + raise UnreadableImageError("content does not match the declared type") + # Header-only so far: refuse before any pixel is decoded. This is + # our own budget; the process-wide ``Image.MAX_IMAGE_PIXELS`` is + # left alone (it is shared with every other Pillow user and racy + # to mutate from worker threads). + if img.width * img.height > constants.THUMBNAIL_MAX_PIXELS: + raise UnreadableImageError("image exceeds the pixel budget") + img.seek(0) # first frame only + img.draft("RGB", (width * 2, width * 2)) # cheap JPEG downscale on decode + frame = ImageOps.exif_transpose(img) + frame.thumbnail((width, width * 64), Image.Resampling.LANCZOS) + mode = "RGBA" if frame.mode in ("RGBA", "LA", "PA", "P") else "RGB" + out = io.BytesIO() + frame.convert(mode).save(out, format="WEBP", quality=80) + return out.getvalue() except UnreadableImageError: raise - except (OSError, ValueError, EOFError, Image.DecompressionBombError, Warning) as exc: + except (OSError, ValueError, EOFError, Image.DecompressionBombError) as exc: raise UnreadableImageError(str(exc)) from exc - finally: - Image.MAX_IMAGE_PIXELS = previous async def _read_all(backend: StorageBackend, key: str, *, limit: int | None = None) -> bytes: @@ -117,6 +127,12 @@ async def _read_all(backend: StorageBackend, key: str, *, limit: int | None = No return b"".join(chunks) +def _finished(flight: tuple[int, str], task: asyncio.Task[bytes]) -> None: + _INFLIGHT.pop(flight, None) + if not task.cancelled(): + task.exception() # mark retrieved: every waiter may have gone away + + async def get_or_create( backend: StorageBackend, *, key: str, content_type: str, width: int | None ) -> tuple[bytes, int]: @@ -130,9 +146,27 @@ async def get_or_create( except StorageNotFoundError: pass - async with _DECODE_SLOTS: + # The work runs in its own task and callers only *await* it through + # ``shield``: a client that disconnects cancels its wait, not the decode, + # so the semaphore slot is held until the worker thread has really + # finished and abort-and-retry loops cannot multiply concurrent decodes. + flight = (id(asyncio.get_running_loop()), cached_key) + task = _INFLIGHT.get(flight) + if task is None: + task = asyncio.ensure_future(_generate(backend, key, cached_key, content_type, snapped)) + _INFLIGHT[flight] = task + task.add_done_callback( + lambda t: (_INFLIGHT.pop(flight, None), t.cancelled() or t.exception()) + ) + return await asyncio.shield(task), snapped + + +async def _generate( + backend: StorageBackend, key: str, cached_key: str, content_type: str, width: int +) -> bytes: + async with _slots(): source = await _read_all(backend, key, limit=constants.THUMBNAIL_MAX_SOURCE_BYTES) - data = await asyncio.to_thread(render, source, snapped, content_type) + data = await asyncio.to_thread(render, source, width, content_type) async def _once(): yield data @@ -140,7 +174,7 @@ async def _once(): await backend.put( cached_key, _once(), content_type=constants.THUMBNAIL_CONTENT_TYPE, size=len(data) ) - return data, snapped + return data async def delete_variants(backend: StorageBackend, key: str) -> None: diff --git a/modules/file_storage/tests/test_file_storage_thumbnail_hardening.py b/modules/file_storage/tests/test_file_storage_thumbnail_hardening.py index b41be5f1..0e7dbac5 100644 --- a/modules/file_storage/tests/test_file_storage_thumbnail_hardening.py +++ b/modules/file_storage/tests/test_file_storage_thumbnail_hardening.py @@ -2,7 +2,9 @@ from __future__ import annotations +import asyncio import io +import threading from uuid import uuid4 import pytest @@ -116,3 +118,66 @@ async def test_anonymous_cannot_use_authenticated_thumbnail(authenticated_client body = await _upload(authenticated_client, "q.png", _png(), "image/png", public="true") resp = await client.get(f"{API}/files/{body['id']}/thumbnail") assert resp.status_code in (401, 302, 403) + + +class _MemBackend: + backend_id = "mem" + supports_presigned_url = False + + def __init__(self, objects): + self.objects = dict(objects) + + async def get(self, key): + from file_storage.contracts.service import StorageNotFoundError + + if key not in self.objects: + raise StorageNotFoundError(key) + + async def gen(): + yield self.objects[key] + + return gen() + + async def put(self, key, stream, *, content_type, size): + self.objects[key] = b"".join([c async for c in stream]) + + async def delete(self, key): + self.objects.pop(key, None) + + +async def test_cancelled_waiter_does_not_free_the_decode_slot(monkeypatch): + entered, release = threading.Event(), threading.Event() + calls = [] + + def blocking_render(data, width, content_type=None): + calls.append(width) + entered.set() + release.wait(5) + return b"webp" + + monkeypatch.setattr(thumbnails, "render", blocking_render) + backend = _MemBackend({"k": _png()}) + + def request(): + return asyncio.ensure_future( + thumbnails.get_or_create(backend, key="k", content_type="image/png", width=64) + ) + + first = request() + await asyncio.get_running_loop().run_in_executor(None, entered.wait, 5) + first.cancel() # the client went away + second = request() # retry joins the same decode (single flight) + await asyncio.sleep(0.05) + assert thumbnails._slots()._value == constants.THUMBNAIL_MAX_CONCURRENCY - 1 + assert len(calls) == 1 + release.set() + assert (await second)[0] == b"webp" + await asyncio.sleep(0.05) + assert thumbnails._slots()._value == constants.THUMBNAIL_MAX_CONCURRENCY + assert not thumbnails._INFLIGHT + + +def test_render_leaves_the_global_pixel_cap_alone(): + before = Image.MAX_IMAGE_PIXELS + thumbnails.render(_png(20, 20), 64, "image/png") + assert before == Image.MAX_IMAGE_PIXELS From 3535a077441a2f95c2dbf5cdf02b245e920541f7 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 6 Oct 2026 20:39:58 +0200 Subject: [PATCH 04/11] refactor(optimize): dedupe 404/headers helpers, drop dead code and one-use wrappers in file_storage Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../file_storage/file_storage/constants.py | 1 - .../file_storage/endpoints/api.py | 40 +++----------- .../file_storage/endpoints/public.py | 12 ++--- modules/file_storage/file_storage/reads.py | 2 - modules/file_storage/file_storage/serving.py | 52 +++++++++++-------- .../file_storage/file_storage/thumbnails.py | 43 +++++++-------- .../file_storage/file_storage/visibility.py | 12 +---- .../test_file_storage_thumbnail_hardening.py | 2 +- 8 files changed, 63 insertions(+), 101 deletions(-) diff --git a/modules/file_storage/file_storage/constants.py b/modules/file_storage/file_storage/constants.py index 2cd9390e..76243090 100644 --- a/modules/file_storage/file_storage/constants.py +++ b/modules/file_storage/file_storage/constants.py @@ -141,7 +141,6 @@ class I18nKey: THUMBNAIL_MIN_WIDTH: Final = 32 THUMBNAIL_MAX_WIDTH: Final = 1024 THUMBNAIL_CONTENT_TYPE: Final = "image/webp" -THUMBNAIL_SOURCE_TYPES: Final = frozenset({"image/jpeg", "image/png", "image/webp", "image/gif"}) # Decode budget: refuse an image whose pixel count would balloon memory # (decompression bomb) before Pillow ever decodes it. THUMBNAIL_MAX_PIXELS: Final = 25_000_000 diff --git a/modules/file_storage/file_storage/endpoints/api.py b/modules/file_storage/file_storage/endpoints/api.py index 500f14f7..3fe3c824 100644 --- a/modules/file_storage/file_storage/endpoints/api.py +++ b/modules/file_storage/file_storage/endpoints/api.py @@ -40,7 +40,7 @@ StoredFileNotFoundError, StreamDownload, ) -from file_storage.serving import thumbnail_response +from file_storage.serving import not_found, thumbnail_response router = APIRouter() @@ -138,13 +138,7 @@ async def get_file( try: row = await service.get(file_id) except StoredFileNotFoundError as exc: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail={ - "code": constants.ErrorCode.NOT_FOUND, - "message": t.t(constants.I18nKey.ERR_NOT_FOUND), - }, - ) from exc + raise not_found(t) from exc return StoredFileOut.model_validate(queries.to_out_dict(row)) @@ -163,7 +157,7 @@ async def update_file( try: row = await service.set_public(file_id, body.public) except StoredFileNotFoundError as exc: - raise _not_found(t) from exc + raise not_found(t) from exc return StoredFileOut.model_validate(queries.to_out_dict(row)) @@ -181,7 +175,7 @@ async def file_thumbnail( try: row = await service.get(file_id) except StoredFileNotFoundError as exc: - raise _not_found(t) from exc + raise not_found(t) from exc return await thumbnail_response( service, row, w, t, cache_control=f"private, max-age={constants.THUMBNAIL_MAX_AGE_SECONDS}" ) @@ -200,13 +194,7 @@ async def download_file( try: download = await service.download(file_id) except StoredFileNotFoundError as exc: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail={ - "code": constants.ErrorCode.NOT_FOUND, - "message": t.t(constants.I18nKey.ERR_NOT_FOUND), - }, - ) from exc + raise not_found(t) from exc if isinstance(download, RedirectDownload): return RedirectResponse(url=download.url, status_code=status.HTTP_302_FOUND) @@ -263,21 +251,5 @@ async def delete_file( try: row = await service.delete(file_id) except StoredFileNotFoundError as exc: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail={ - "code": constants.ErrorCode.NOT_FOUND, - "message": t.t(constants.I18nKey.ERR_NOT_FOUND), - }, - ) from exc + raise not_found(t) from exc await bus.publish(FileDeleted(file_id=row.id, key=row.key)) - - -def _not_found(t: TranslatorDep) -> HTTPException: - return HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail={ - "code": constants.ErrorCode.NOT_FOUND, - "message": t.t(constants.I18nKey.ERR_NOT_FOUND), - }, - ) diff --git a/modules/file_storage/file_storage/endpoints/public.py b/modules/file_storage/file_storage/endpoints/public.py index c1cadc0c..a4edc704 100644 --- a/modules/file_storage/file_storage/endpoints/public.py +++ b/modules/file_storage/file_storage/endpoints/public.py @@ -9,13 +9,13 @@ import uuid -from fastapi import APIRouter, Depends, HTTPException, Query, Request, Response, status +from fastapi import APIRouter, Depends, Query, Request, Response from simple_module_hosting.i18n_deps import TranslatorDep from file_storage import constants from file_storage.deps import get_file_storage_service from file_storage.service import FileStorageService, StoredFileNotFoundError -from file_storage.serving import public_file_response, thumbnail_response +from file_storage.serving import not_found, public_file_response, thumbnail_response router = APIRouter() @@ -24,13 +24,7 @@ async def _public_row(service: FileStorageService, file_id: uuid.UUID, t: Transl try: return await service.get_public(file_id) except StoredFileNotFoundError as exc: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail={ - "code": constants.ErrorCode.NOT_FOUND, - "message": t.t(constants.I18nKey.ERR_NOT_FOUND), - }, - ) from exc + raise not_found(t) from exc # Declared before the ``{filename}`` route so "thumbnail" is not read as a name. diff --git a/modules/file_storage/file_storage/reads.py b/modules/file_storage/file_storage/reads.py index a10a5e22..c83b8b2a 100644 --- a/modules/file_storage/file_storage/reads.py +++ b/modules/file_storage/file_storage/reads.py @@ -69,7 +69,6 @@ async def page_of_files( created_by: str | None = None, search: str | None = None, content_type: str | None = None, - sort: str = constants.DEFAULT_SORT, ) -> list[StoredFileOut]: return await queries.page_of_files( self.db, @@ -78,7 +77,6 @@ async def page_of_files( created_by=created_by, search=search, content_type=content_type, - sort=sort, ) async def storage_aggregates(self) -> StorageAggregates: diff --git a/modules/file_storage/file_storage/serving.py b/modules/file_storage/file_storage/serving.py index d844f62e..0469e520 100644 --- a/modules/file_storage/file_storage/serving.py +++ b/modules/file_storage/file_storage/serving.py @@ -17,13 +17,33 @@ def _etag(row: StoredFile, suffix: str = "") -> str: return f'"{row.checksum_sha256}{suffix}"' +def not_found(t) -> HTTPException: + """The uniform 404 body, so a miss never reveals why it missed.""" + return HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail={ + "code": constants.ErrorCode.NOT_FOUND, + "message": t.t(constants.I18nKey.ERR_NOT_FOUND), + }, + ) + + +def _headers(etag: str, cache_control: str) -> dict[str, str]: + return { + "ETag": etag, + "Cache-Control": cache_control, + "X-Content-Type-Options": "nosniff", + "Content-Security-Policy": constants.PUBLIC_CSP, + } + + def _not_modified(request: Request, etag: str) -> bool: sent = request.headers.get("if-none-match", "") return etag in {part.strip().removeprefix("W/") for part in sent.split(",")} def is_active_content(content_type: str) -> bool: - return content_type.split(";")[0].strip().lower() in constants.ACTIVE_CONTENT_TYPES + return thumbnails.base_type(content_type) in constants.ACTIVE_CONTENT_TYPES def content_disposition(filename: str, *, attachment: bool) -> str: @@ -44,12 +64,7 @@ async def thumbnail_response( """Serve ``row``'s resized variant; 404 for non-images, 422 if undecodable.""" snapped = thumbnails.snap_width(width) etag = _etag(row, f"-w{snapped}") - headers = { - "ETag": etag, - "Cache-Control": cache_control, - "X-Content-Type-Options": "nosniff", - "Content-Security-Policy": constants.PUBLIC_CSP, - } + headers = _headers(etag, cache_control) if ( request is not None and _not_modified(request, etag) @@ -57,15 +72,9 @@ async def thumbnail_response( ): return Response(status_code=status.HTTP_304_NOT_MODIFIED, headers=headers) try: - data, _ = await service.thumbnail(row, snapped) + data = await service.thumbnail(row, snapped) except thumbnails.NotAnImageError as exc: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail={ - "code": constants.ErrorCode.NOT_FOUND, - "message": t.t(constants.I18nKey.ERR_NOT_FOUND), - }, - ) from exc + raise not_found(t) from exc except thumbnails.UnreadableImageError as exc: raise HTTPException( status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, @@ -89,17 +98,14 @@ async def public_file_response( max_age = constants.PUBLIC_MAX_AGE_SECONDS etag = _etag(row) active = is_active_content(row.content_type) - headers = { - "ETag": etag, - "Cache-Control": f"public, max-age={max_age}", - "X-Content-Type-Options": "nosniff", - "Content-Security-Policy": constants.PUBLIC_CSP, - } + headers = _headers(etag, f"public, max-age={max_age}") if _not_modified(request, etag): return Response(status_code=status.HTTP_304_NOT_MODIFIED, headers=headers) if service.backend.supports_presigned_url and not active: - url = await service.presigned_url(row) + url = await service.backend.presigned_get_url( + row.key, service.settings.s3_presign_ttl_seconds + ) # A cached redirect must not outlive the signature it points at. redirect_age = min(max_age, service.settings.s3_presign_ttl_seconds // 2) return RedirectResponse( @@ -108,7 +114,7 @@ async def public_file_response( headers={**headers, "Cache-Control": f"public, max-age={redirect_age}"}, ) - body = await service.stream(row) + body = await service.backend.get(row.key) return StreamingResponse( body, media_type=row.content_type, diff --git a/modules/file_storage/file_storage/thumbnails.py b/modules/file_storage/file_storage/thumbnails.py index 6d5d33bf..347c8298 100644 --- a/modules/file_storage/file_storage/thumbnails.py +++ b/modules/file_storage/file_storage/thumbnails.py @@ -37,8 +37,13 @@ class UnreadableImageError(Exception): """The bytes are not a decodable image, or are too large to decode safely.""" +def base_type(content_type: str) -> str: + """The media type without parameters, lowercased (``Image/PNG; x=y`` -> ``image/png``).""" + return content_type.split(";")[0].strip().lower() + + def is_thumbnailable(content_type: str) -> bool: - return content_type.split(";")[0].strip().lower() in constants.THUMBNAIL_SOURCE_TYPES + return base_type(content_type) in _FORMATS_BY_TYPE def snap_width(width: int | None) -> int: @@ -92,7 +97,7 @@ def render(data: bytes, width: int, content_type: str | None = None) -> bytes: try: with Image.open(io.BytesIO(data), formats=_ALLOWED_FORMATS) as img: if content_type is not None and img.format != _FORMATS_BY_TYPE.get( - content_type.split(";")[0].strip().lower() + base_type(content_type) ): raise UnreadableImageError("content does not match the declared type") # Header-only so far: refuse before any pixel is decoded. This is @@ -127,22 +132,19 @@ async def _read_all(backend: StorageBackend, key: str, *, limit: int | None = No return b"".join(chunks) -def _finished(flight: tuple[int, str], task: asyncio.Task[bytes]) -> None: - _INFLIGHT.pop(flight, None) - if not task.cancelled(): - task.exception() # mark retrieved: every waiter may have gone away - - async def get_or_create( - backend: StorageBackend, *, key: str, content_type: str, width: int | None -) -> tuple[bytes, int]: - """The variant's bytes and the snapped width, generating it on first use.""" + backend: StorageBackend, *, key: str, content_type: str, width: int +) -> bytes: + """The variant's bytes, generating it on first use. + + ``width`` must already be snapped (:func:`snap_width`) so the number of + cached variants stays bounded. + """ if not is_thumbnailable(content_type): raise NotAnImageError(content_type) - snapped = snap_width(width) - cached_key = variant_key(key, snapped) + cached_key = variant_key(key, width) try: - return await _read_all(backend, cached_key), snapped + return await _read_all(backend, cached_key) except StorageNotFoundError: pass @@ -153,12 +155,12 @@ async def get_or_create( flight = (id(asyncio.get_running_loop()), cached_key) task = _INFLIGHT.get(flight) if task is None: - task = asyncio.ensure_future(_generate(backend, key, cached_key, content_type, snapped)) + task = asyncio.ensure_future(_generate(backend, key, cached_key, content_type, width)) _INFLIGHT[flight] = task task.add_done_callback( lambda t: (_INFLIGHT.pop(flight, None), t.cancelled() or t.exception()) ) - return await asyncio.shield(task), snapped + return await asyncio.shield(task) async def _generate( @@ -179,8 +181,7 @@ async def _once(): async def delete_variants(backend: StorageBackend, key: str) -> None: """Drop every cached variant of ``key``; absent ones are fine.""" - for width in constants.THUMBNAIL_WIDTHS: - try: - await backend.delete(variant_key(key, width)) - except Exception: - continue + await asyncio.gather( + *(backend.delete(variant_key(key, width)) for width in constants.THUMBNAIL_WIDTHS), + return_exceptions=True, + ) diff --git a/modules/file_storage/file_storage/visibility.py b/modules/file_storage/file_storage/visibility.py index 942d04d2..be81baca 100644 --- a/modules/file_storage/file_storage/visibility.py +++ b/modules/file_storage/file_storage/visibility.py @@ -6,7 +6,6 @@ from __future__ import annotations import uuid -from collections.abc import AsyncIterator from typing import TYPE_CHECKING from sqlalchemy import select @@ -61,15 +60,8 @@ async def set_public(self, file_id: uuid.UUID, public: bool) -> StoredFile: await self.db.refresh(row) return row - async def thumbnail(self, row: StoredFile, width: int | None) -> tuple[bytes, int]: - """Cached resized variant of ``row`` (see :mod:`file_storage.thumbnails`).""" + async def thumbnail(self, row: StoredFile, width: int) -> bytes: + """Cached variant of ``row`` at an already-snapped ``width``.""" return await thumbnails.get_or_create( self.backend, key=row.key, content_type=row.content_type, width=width ) - - async def stream(self, row: StoredFile) -> AsyncIterator[bytes]: - """The object's bytes, for a row the caller has already authorised.""" - return await self.backend.get(row.key) - - async def presigned_url(self, row: StoredFile) -> str: - return await self.backend.presigned_get_url(row.key, self.settings.s3_presign_ttl_seconds) diff --git a/modules/file_storage/tests/test_file_storage_thumbnail_hardening.py b/modules/file_storage/tests/test_file_storage_thumbnail_hardening.py index 0e7dbac5..0971d5b3 100644 --- a/modules/file_storage/tests/test_file_storage_thumbnail_hardening.py +++ b/modules/file_storage/tests/test_file_storage_thumbnail_hardening.py @@ -171,7 +171,7 @@ def request(): assert thumbnails._slots()._value == constants.THUMBNAIL_MAX_CONCURRENCY - 1 assert len(calls) == 1 release.set() - assert (await second)[0] == b"webp" + assert await second == b"webp" await asyncio.sleep(0.05) assert thumbnails._slots()._value == constants.THUMBNAIL_MAX_CONCURRENCY assert not thumbnails._INFLIGHT From 1d3401bab11a0b3c73a9560c2e349f16a497a251 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 6 Oct 2026 20:58:47 +0200 Subject: [PATCH 05/11] fix: address code review findings (round 1, pass 1) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../simple_module_hosting/middleware.py | 10 +++++++--- .../file_storage/backends/filesystem.py | 18 +++++++++++++++--- .../file_storage/endpoints/public.py | 2 +- modules/file_storage/file_storage/queries.py | 4 +++- modules/file_storage/file_storage/serving.py | 15 +++++++++++---- .../file_storage/file_storage/thumbnails.py | 10 +++++++++- 6 files changed, 46 insertions(+), 13 deletions(-) diff --git a/framework/hosting/simple_module_hosting/middleware.py b/framework/hosting/simple_module_hosting/middleware.py index a4cb7c23..1b86d57b 100644 --- a/framework/hosting/simple_module_hosting/middleware.py +++ b/framework/hosting/simple_module_hosting/middleware.py @@ -147,9 +147,13 @@ async def send_with_headers(message: Message) -> None: headers[_HEADER_X_FRAME_OPTIONS] = _XFO_SAMEORIGIN headers[_HEADER_X_XSS_PROTECTION] = _XXSS_DISABLED headers[_HEADER_REFERRER_POLICY] = _REFERRER_STRICT_ORIGIN - # A response that carries its own policy (file_storage sandboxes - # user-uploaded bytes) keeps it; the app-wide one is the default. - if self.csp and _HEADER_CSP not in headers: + # A response may only *tighten* the app-wide policy: one that + # carries its own ``sandbox`` directive (file_storage serving + # user-uploaded bytes) keeps it. Any other response-supplied + # policy is overwritten, so a module cannot weaken the default. + own = headers.get(_HEADER_CSP) or "" + keeps_own = "sandbox" in {d.strip().split(" ")[0].lower() for d in own.split(";")} + if self.csp and not keeps_own: headers[_HEADER_CSP] = self.csp if self.hsts: headers[_HEADER_HSTS] = self.hsts diff --git a/modules/file_storage/file_storage/backends/filesystem.py b/modules/file_storage/file_storage/backends/filesystem.py index 9b4d863d..b21c3665 100644 --- a/modules/file_storage/file_storage/backends/filesystem.py +++ b/modules/file_storage/file_storage/backends/filesystem.py @@ -2,6 +2,8 @@ from __future__ import annotations +import contextlib +import uuid from collections.abc import AsyncIterator from pathlib import Path @@ -49,9 +51,19 @@ async def put( ) -> None: path = self._resolve(key) path.parent.mkdir(parents=True, exist_ok=True) - async with aiofiles.open(path, "wb") as fh: - async for chunk in stream: - await fh.write(chunk) + # Write beside the target and rename into place: a concurrent ``get`` (a + # thumbnail cache hit mid-generation) must never see a half-written + # object, and a crash must not leave a truncated one behind. + tmp = path.with_name(f"{path.name}.{uuid.uuid4().hex}.tmp") + try: + async with aiofiles.open(tmp, "wb") as fh: + async for chunk in stream: + await fh.write(chunk) + tmp.replace(path) + except BaseException: + with contextlib.suppress(OSError): + tmp.unlink(missing_ok=True) + raise async def get(self, key: str) -> AsyncIterator[bytes]: path = self._resolve(key) diff --git a/modules/file_storage/file_storage/endpoints/public.py b/modules/file_storage/file_storage/endpoints/public.py index a4edc704..9f853c73 100644 --- a/modules/file_storage/file_storage/endpoints/public.py +++ b/modules/file_storage/file_storage/endpoints/public.py @@ -57,4 +57,4 @@ async def public_file( service: FileStorageService = Depends(get_file_storage_service), ) -> Response: row = await _public_row(service, file_id, t) - return await public_file_response(service, row, request) + return await public_file_response(service, row, request, t) diff --git a/modules/file_storage/file_storage/queries.py b/modules/file_storage/file_storage/queries.py index d7b33d41..2fbd680b 100644 --- a/modules/file_storage/file_storage/queries.py +++ b/modules/file_storage/file_storage/queries.py @@ -159,7 +159,9 @@ async def content_type_facets(db: AsyncSession, *, created_by: str | None = None def public_url_for(file_id: object, filename: str) -> str: """The anonymous URL of a public file, with its name as a trailing segment.""" name = quote(filename, safe="") - return f"{constants.ROUTE_PREFIX_API}{constants.PUBLIC_SEGMENT}/{file_id}/{name}" + base = f"{constants.ROUTE_PREFIX_API}{constants.PUBLIC_SEGMENT}/{file_id}" + # "thumbnail" as a trailing segment is the thumbnail route, not a filename. + return base if name.lower() == "thumbnail" else f"{base}/{name}" def to_out_dict(row: StoredFile) -> dict: diff --git a/modules/file_storage/file_storage/serving.py b/modules/file_storage/file_storage/serving.py index 0469e520..6f4bae9a 100644 --- a/modules/file_storage/file_storage/serving.py +++ b/modules/file_storage/file_storage/serving.py @@ -9,6 +9,7 @@ from fastapi.responses import RedirectResponse, StreamingResponse from file_storage import constants, thumbnails +from file_storage.contracts.service import StorageNotFoundError from file_storage.models import StoredFile from file_storage.service import FileStorageService @@ -47,7 +48,9 @@ def is_active_content(content_type: str) -> bool: def content_disposition(filename: str, *, attachment: bool) -> str: - ascii_name = filename.encode("ascii", "ignore").decode().replace('"', "").replace("\\", "") + ascii_name = "".join( + c for c in filename.encode("ascii", "ignore").decode() if c.isprintable() and c not in '"\\' + ) kind = "attachment" if attachment else "inline" return f"{kind}; filename=\"{ascii_name or 'file'}\"; filename*=UTF-8''{quote(filename)}" @@ -73,7 +76,8 @@ async def thumbnail_response( return Response(status_code=status.HTTP_304_NOT_MODIFIED, headers=headers) try: data = await service.thumbnail(row, snapped) - except thumbnails.NotAnImageError as exc: + except (thumbnails.NotAnImageError, StorageNotFoundError) as exc: + # A row whose object is gone is a miss, not a server error. raise not_found(t) from exc except thumbnails.UnreadableImageError as exc: raise HTTPException( @@ -87,7 +91,7 @@ async def thumbnail_response( async def public_file_response( - service: FileStorageService, row: StoredFile, request: Request + service: FileStorageService, row: StoredFile, request: Request, t ) -> Response: """Serve a file already authorised as public. @@ -114,7 +118,10 @@ async def public_file_response( headers={**headers, "Cache-Control": f"public, max-age={redirect_age}"}, ) - body = await service.backend.get(row.key) + try: + body = await service.backend.get(row.key) + except StorageNotFoundError as exc: + raise not_found(t) from exc return StreamingResponse( body, media_type=row.content_type, diff --git a/modules/file_storage/file_storage/thumbnails.py b/modules/file_storage/file_storage/thumbnails.py index 347c8298..15a7167d 100644 --- a/modules/file_storage/file_storage/thumbnails.py +++ b/modules/file_storage/file_storage/thumbnails.py @@ -17,6 +17,7 @@ import asyncio import io +import struct import weakref from typing import TYPE_CHECKING @@ -116,7 +117,14 @@ def render(data: bytes, width: int, content_type: str | None = None) -> bytes: return out.getvalue() except UnreadableImageError: raise - except (OSError, ValueError, EOFError, Image.DecompressionBombError) as exc: + except ( + OSError, + ValueError, + EOFError, + SyntaxError, # Pillow raises this for some malformed PNG/ICO chunks + struct.error, + Image.DecompressionBombError, + ) as exc: raise UnreadableImageError(str(exc)) from exc From ffe6c9b5fa83fa8a34035a91f6bbabf60fa51022 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 6 Oct 2026 21:08:39 +0200 Subject: [PATCH 06/11] fix: address code review findings (round 1, pass 1b) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- modules/file_storage/file_storage/service.py | 19 +++-- .../file_storage/file_storage/visibility.py | 30 ++++++- .../test_file_storage_delete_variants.py | 58 +++++++++++++ ..._file_storage_thumbnail_session_release.py | 83 +++++++++++++++++++ 4 files changed, 182 insertions(+), 8 deletions(-) create mode 100644 modules/file_storage/tests/test_file_storage_delete_variants.py create mode 100644 modules/file_storage/tests/test_file_storage_thumbnail_session_release.py diff --git a/modules/file_storage/file_storage/service.py b/modules/file_storage/file_storage/service.py index c6c82a76..950a6648 100644 --- a/modules/file_storage/file_storage/service.py +++ b/modules/file_storage/file_storage/service.py @@ -235,8 +235,11 @@ async def delete_many(self, file_ids: Sequence[uuid.UUID]) -> list[StoredFile]: # no row left pointing at them. A failure here is a janitor's # problem, not the caller's. try: - await self.backend.delete(row.key) - await thumbnails.delete_variants(self.backend, row.key) + try: + await self.backend.delete(row.key) + finally: + # Variants go even when the original's delete fails. + await thumbnails.delete_variants(self.backend, row.key) except StorageNotFoundError: # Acceptably absent — eg. a previous delete partially succeeded. pass @@ -258,10 +261,14 @@ async def delete(self, file_id: uuid.UUID, *, platform: bool = False) -> StoredF row.is_deleted = True row.deleted_at = datetime.now(UTC) await self.db.flush() - # Object is acceptably absent — eg. a previous delete partially succeeded. - with contextlib.suppress(StorageNotFoundError): - await self.backend.delete(row.key) - await thumbnails.delete_variants(self.backend, row.key) + try: + # Object is acceptably absent — eg. a previous delete partially succeeded. + with contextlib.suppress(StorageNotFoundError): + await self.backend.delete(row.key) + finally: + # A failed original delete still raises, but must not orphan the + # variants (``delete_variants`` itself never raises). + await thumbnails.delete_variants(self.backend, row.key) return row diff --git a/modules/file_storage/file_storage/visibility.py b/modules/file_storage/file_storage/visibility.py index be81baca..a91ece7f 100644 --- a/modules/file_storage/file_storage/visibility.py +++ b/modules/file_storage/file_storage/visibility.py @@ -8,6 +8,8 @@ import uuid from typing import TYPE_CHECKING +from simple_module_db import finalize_session +from simple_module_db.listeners import SESSION_HAS_WRITES_KEY from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession @@ -61,7 +63,31 @@ async def set_public(self, file_id: uuid.UUID, public: bool) -> StoredFile: return row async def thumbnail(self, row: StoredFile, width: int) -> bytes: - """Cached variant of ``row`` at an already-snapped ``width``.""" + """Cached variant of ``row`` at an already-snapped ``width``. + + A cold variant means reading the original, waiting for a decode slot + and decoding — seconds, not milliseconds. The request's DB connection + is handed back first so a burst of cold (possibly anonymous) requests + cannot pin the whole pool on work that needs no database. + """ + key, content_type = row.key, row.content_type + await self._release_connection(row) return await thumbnails.get_or_create( - self.backend, key=row.key, content_type=row.content_type, width=width + self.backend, key=key, content_type=content_type, width=width ) + + async def _release_connection(self, row: StoredFile) -> None: + """End a read-only request transaction early, returning its connection. + + ``row`` is detached first so its loaded attributes stay readable — the + rollback would otherwise expire it and the next access would try lazy + IO. A session holding writes is left alone: those commit (or roll back) + with the request as usual. ``finalize_session`` is re-armable, so any + later work in the request still opens a fresh transaction and commits. + """ + db = self.db + if db.info.get(SESSION_HAS_WRITES_KEY) or db.new or db.dirty or db.deleted: + return + if row in db: + db.expunge(row) + await finalize_session(db) diff --git a/modules/file_storage/tests/test_file_storage_delete_variants.py b/modules/file_storage/tests/test_file_storage_delete_variants.py new file mode 100644 index 00000000..062f4a81 --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_delete_variants.py @@ -0,0 +1,58 @@ +"""Thumbnail variants are dropped even when deleting the original fails.""" + +from __future__ import annotations + +from io import BytesIO + +import pytest +from fastapi import UploadFile +from file_storage import constants, thumbnails +from file_storage.backends.filesystem import FilesystemBackend +from file_storage.service import FileStorageService +from file_storage.settings import FileStorageSettings + + +class BrokenOriginals(FilesystemBackend): + """Deleting an original fails; deleting a variant works.""" + + async def delete(self, key: str) -> None: + if not key.endswith(".webp"): + raise OSError("backend is on fire") + await super().delete(key) + + +async def _service_with_variant(tmp_path, db_session): + settings = FileStorageSettings( + backend=constants.BackendId.FILESYSTEM, fs_root_path=str(tmp_path) + ) + backend = BrokenOriginals(root=tmp_path) + svc = FileStorageService(db_session, backend, settings) + upload = UploadFile( + filename="p.png", + file=BytesIO(b"x"), + headers={"content-type": "image/png"}, # type: ignore[arg-type] + ) + out = await svc.upload(upload) + variant = thumbnails.variant_key(out.key, 128) + + async def _once(): + yield b"v" + + await backend.put(variant, _once(), content_type="image/webp", size=1) + assert await backend.exists(variant) + return svc, out, variant + + +async def test_delete_drops_variants_when_original_delete_raises(tmp_path, db_session): + svc, out, variant = await _service_with_variant(tmp_path, db_session) + # The error still reaches the caller, unchanged. + with pytest.raises(OSError, match="on fire"): + await svc.delete(out.id) + assert not await svc.backend.exists(variant) + + +async def test_bulk_delete_drops_variants_when_original_delete_raises(tmp_path, db_session): + svc, out, variant = await _service_with_variant(tmp_path, db_session) + removed = await svc.delete_many([out.id]) + assert [r.id for r in removed] == [out.id] + assert not await svc.backend.exists(variant) diff --git a/modules/file_storage/tests/test_file_storage_thumbnail_session_release.py b/modules/file_storage/tests/test_file_storage_thumbnail_session_release.py new file mode 100644 index 00000000..85993f89 --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_thumbnail_session_release.py @@ -0,0 +1,83 @@ +"""A thumbnail request hands its DB connection back before the image work. + +Generating a cold variant reads the original, waits for a decode slot and +decodes — none of which needs the database. Holding the request's session +(and so a pool connection) through all of that let a burst of cold anonymous +requests exhaust the pool. +""" + +from __future__ import annotations + +import io + +import pytest +from file_storage import constants, thumbnails +from file_storage.visibility import FileStoragePublic +from PIL import Image + +API = constants.ROUTE_PREFIX_API + + +def _png() -> bytes: + buf = io.BytesIO() + Image.new("RGB", (40, 20), (1, 2, 3)).save(buf, format="PNG") + return buf.getvalue() + + +@pytest.fixture +def observed(monkeypatch): + """Record the request session's state at the moment generation runs.""" + seen: dict = {} + original = FileStoragePublic.thumbnail + + async def spy(self, row, width): + seen["db"] = self.db + seen["row"] = row + return await original(self, row, width) + + async def fake_generate(backend, *, key, content_type, width): + db = seen["db"] + seen["in_transaction"] = db.in_transaction() + seen["row_attached"] = seen["row"] in db + # Loaded attributes stay readable with no IO (no MissingGreenlet). + seen["key_matches"] = seen["row"].key == key + return b"fake-webp" + + monkeypatch.setattr(FileStoragePublic, "thumbnail", spy) + monkeypatch.setattr(thumbnails, "get_or_create", fake_generate) + return seen + + +async def _upload(client, **form) -> dict: + resp = await client.post( + f"{API}{constants.PATH_UPLOAD}", + files={"file": ("p.png", _png(), "image/png")}, + data=form, + ) + assert resp.status_code == 201, resp.text + return resp.json() + + +def _assert_released(seen: dict) -> None: + assert seen["in_transaction"] is False + assert seen["row_attached"] is False + assert seen["key_matches"] is True + + +async def test_public_thumbnail_releases_session_before_generating( + authenticated_client, client, observed +): + body = await _upload(authenticated_client, public="true") + resp = await client.get(f"{API}/public/{body['id']}/thumbnail", params={"w": 64}) + assert resp.status_code == 200 + assert resp.content == b"fake-webp" + _assert_released(observed) + + +async def test_authenticated_thumbnail_releases_session_before_generating( + authenticated_client, observed +): + body = await _upload(authenticated_client) + resp = await authenticated_client.get(f"{API}/files/{body['id']}/thumbnail") + assert resp.status_code == 200 + _assert_released(observed) From fd5f78d8510ce4f69e634e750900f353b0f52b70 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 6 Oct 2026 21:51:43 +0200 Subject: [PATCH 07/11] fix(file_storage): no session cookie on public responses, strict PATCH bool, 16-bit grayscale thumbnails (QA round 1) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../hosting/tests/test_middleware_order.py | 7 ++ .../file_storage/contracts/schemas.py | 7 +- .../file_storage/file_storage/cookieless.py | 68 +++++++++++++++ modules/file_storage/file_storage/module.py | 12 ++- .../file_storage/file_storage/thumbnails.py | 19 +++- .../tests/test_file_storage_public.py | 10 +++ .../test_file_storage_public_cookieless.py | 86 +++++++++++++++++++ .../test_file_storage_thumbnail_hardening.py | 16 ++++ 8 files changed, 219 insertions(+), 6 deletions(-) create mode 100644 modules/file_storage/file_storage/cookieless.py create mode 100644 modules/file_storage/tests/test_file_storage_public_cookieless.py diff --git a/framework/hosting/tests/test_middleware_order.py b/framework/hosting/tests/test_middleware_order.py index 4beaef82..d010ff04 100644 --- a/framework/hosting/tests/test_middleware_order.py +++ b/framework/hosting/tests/test_middleware_order.py @@ -40,6 +40,11 @@ cannot hide SiteLock from an anonymous visitor, because acquiring a demo session means POSTing to the demo endpoint, which SiteLock blocks first. +CookielessPublicFilesMiddleware (``file_storage``) keeps the session cookie and +``Vary: Cookie`` off anonymous public-file reads. Its place among the module +middlewares does not matter: it clears the session's accessed/modified flags as +the response starts, so reads by Auth or SiteLock further out are forgotten too. + Maintenance sits after InertiaLayoutData because its 503 page renders through Inertia and needs the shared props (auth, menus, i18n) — placed any further out it would render bare, with no layout and untranslated copy. It is @@ -74,6 +79,7 @@ "SessionMiddleware", "DemoReadOnlyMiddleware", "SiteLockMiddleware", + "CookielessPublicFilesMiddleware", "AuthMiddleware", "TenantMiddleware", "LocaleMiddleware", @@ -92,6 +98,7 @@ "SessionMiddleware", "DemoReadOnlyMiddleware", "SiteLockMiddleware", + "CookielessPublicFilesMiddleware", "AuthMiddleware", "LocaleMiddleware", "InertiaLayoutDataMiddleware", diff --git a/modules/file_storage/file_storage/contracts/schemas.py b/modules/file_storage/file_storage/contracts/schemas.py index 991150cf..24977fa4 100644 --- a/modules/file_storage/file_storage/contracts/schemas.py +++ b/modules/file_storage/file_storage/contracts/schemas.py @@ -5,7 +5,7 @@ import uuid from datetime import datetime -from pydantic import ConfigDict +from pydantic import ConfigDict, StrictBool from sqlmodel import Field, SQLModel @@ -36,7 +36,10 @@ class StoredFileOut(SQLModel): class StoredFileUpdate(SQLModel): """Body for PATCH /api/file-storage/files/{id}.""" - public: bool + # Strict: a JSON body says ``true``/``false``. Lax coercion would publish a + # file on ``"yes"``, ``"true"`` or ``1`` from a client bug. The upload form + # stays lax on purpose — multipart values are always strings. + public: StrictBool class BulkDeleteRequest(SQLModel): diff --git a/modules/file_storage/file_storage/cookieless.py b/modules/file_storage/file_storage/cookieless.py new file mode 100644 index 00000000..5c59335e --- /dev/null +++ b/modules/file_storage/file_storage/cookieless.py @@ -0,0 +1,68 @@ +"""Keep the session out of anonymous public-file responses (#353). + +Public files are served ``Cache-Control: public`` so a CDN or shared proxy can +hold them. Two headers the session layer adds would undo that: + +* ``Set-Cookie: session=…`` — the layout middleware records the locale and + i18n audience it served in the session on *every* request, so an anonymous + fetch mints a fresh 14-day session cookie. A shared cache that stores the + response would hand that cookie to every later visitor. +* ``Vary: Cookie`` — added whenever anything read the session, which splits + the cache into one entry per visitor for bytes that do not depend on who + asked (the handler never looks at the caller). + +Nothing on these routes needs to *write* the session, so this middleware tells +the session layer that nothing touched it: when the response starts it clears +the session's ``accessed``/``modified`` flags, and Starlette's +``SessionMiddleware`` then emits neither header. Clearing the flags rather than +swapping in a throwaway session keeps it correct whatever order the module +middleware ends up in — an auth layer further out may already have read the +real session on the way in. Writes made while serving are simply not persisted; +a signed-in visitor's existing cookie is left exactly as it was. +""" + +from __future__ import annotations + +import re + +from starlette.types import ASGIApp, Message, Receive, Scope, Send + +from file_storage import constants + +PUBLIC_PATH_PATTERN = ( + rf"{re.escape(constants.ROUTE_PREFIX_API + constants.PUBLIC_SEGMENT)}/[^/]+(/[^/]+)?$" +) +"""The anonymous read routes: ``/public/{id}``, ``/{id}/{name}``, ``/{id}/thumbnail``.""" + +_PUBLIC_PATH = re.compile(PUBLIC_PATH_PATTERN) +_READ_METHODS = frozenset({"GET", "HEAD"}) + + +class CookielessPublicFilesMiddleware: + """Suppress session ``Set-Cookie`` / ``Vary: Cookie`` on public file reads.""" + + def __init__(self, app: ASGIApp) -> None: + self.app = app + + async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: + if ( + scope["type"] != "http" + or scope.get("method") not in _READ_METHODS + or _PUBLIC_PATH.match(scope.get("path", "")) is None + ): + await self.app(scope, receive, send) + return + + async def send_untouched(message: Message) -> None: + if message["type"] == "http.response.start": + _forget_session_use(scope.get("session")) + await send(message) + + await self.app(scope, receive, send_untouched) + + +def _forget_session_use(session: object) -> None: + # Starlette's ``Session`` decides both headers from these two flags alone. + for flag in ("accessed", "modified"): + if hasattr(session, flag): + setattr(session, flag, False) diff --git a/modules/file_storage/file_storage/module.py b/modules/file_storage/file_storage/module.py index d4d5d352..7469411d 100644 --- a/modules/file_storage/file_storage/module.py +++ b/modules/file_storage/file_storage/module.py @@ -4,7 +4,6 @@ import importlib.resources import logging -import re from pathlib import Path from typing import TYPE_CHECKING @@ -104,8 +103,15 @@ def register_public_routes(self, registry: PublicRouteRegistry) -> None: uploads, deletes and the authenticated download keep requiring a session. The handler itself still refuses anything not ``public``. """ - base = re.escape(f"{constants.ROUTE_PREFIX_API}{constants.PUBLIC_SEGMENT}") - registry.add_regex(rf"{base}/[^/]+(/[^/]+)?$", methods={"GET"}) + from file_storage.cookieless import PUBLIC_PATH_PATTERN + + registry.add_regex(PUBLIC_PATH_PATTERN, methods={"GET"}) + + def register_middleware(self, app: FastAPI) -> None: + """Serve public files without a session cookie or ``Vary: Cookie``.""" + from file_storage.cookieless import CookielessPublicFilesMiddleware + + app.add_middleware(CookielessPublicFilesMiddleware) def register_audit_links(self, registry: AuditLinkRegistry) -> None: """Name file rows in the audit log, and tag them with their table. diff --git a/modules/file_storage/file_storage/thumbnails.py b/modules/file_storage/file_storage/thumbnails.py index 15a7167d..40858de4 100644 --- a/modules/file_storage/file_storage/thumbnails.py +++ b/modules/file_storage/file_storage/thumbnails.py @@ -109,7 +109,7 @@ def render(data: bytes, width: int, content_type: str | None = None) -> bytes: raise UnreadableImageError("image exceeds the pixel budget") img.seek(0) # first frame only img.draft("RGB", (width * 2, width * 2)) # cheap JPEG downscale on decode - frame = ImageOps.exif_transpose(img) + frame = _to_8bit(ImageOps.exif_transpose(img)) frame.thumbnail((width, width * 64), Image.Resampling.LANCZOS) mode = "RGBA" if frame.mode in ("RGBA", "LA", "PA", "P") else "RGB" out = io.BytesIO() @@ -128,6 +128,23 @@ def render(data: bytes, width: int, content_type: str | None = None) -> bytes: raise UnreadableImageError(str(exc)) from exc +_WIDE_GRAY_MODES = frozenset({"I", "I;16", "I;16B", "I;16L", "I;16N"}) + + +def _to_8bit(frame: Image.Image) -> Image.Image: + """Map 16/32-bit grayscale (a 16-bit PNG opens as ``I;16``) onto 8-bit ``L``. + + Pillow cannot ``reduce()`` these modes, which ``thumbnail`` uses for large + downscales, so a valid 16-bit PNG would render at some widths and fail at + others. Scaled by 1/256 rather than clipped, so a 16-bit image keeps its + tones instead of turning almost entirely white. Runs after the pixel-budget + check, so the wider intermediate is bounded like every other decode. + """ + if frame.mode not in _WIDE_GRAY_MODES: + return frame + return frame.convert("I").point(lambda v: v * (1 / 256)).convert("L") + + async def _read_all(backend: StorageBackend, key: str, *, limit: int | None = None) -> bytes: """Whole object, aborting once ``limit`` bytes are exceeded (no unbounded buffer).""" chunks: list[bytes] = [] diff --git a/modules/file_storage/tests/test_file_storage_public.py b/modules/file_storage/tests/test_file_storage_public.py index e7cc2b60..fcaa57c3 100644 --- a/modules/file_storage/tests/test_file_storage_public.py +++ b/modules/file_storage/tests/test_file_storage_public.py @@ -133,3 +133,13 @@ async def _gen(): await services.backend.put("acme/pub", _gen(), content_type="text/plain", size=1) assert (await client.get(f"{API}/public/{ids['pub']}")).status_code == 200 assert (await client.get(f"{API}/public/{ids['priv']}")).status_code == 404 + + +async def test_patch_public_rejects_non_boolean(authenticated_client, client): + body = await _upload(authenticated_client) + url = f"{API}/files/{body['id']}" + for value in ("yes", "true", 1, 0, None): + resp = await authenticated_client.patch(url, json={"public": value}) + assert resp.status_code == 422, (value, resp.text) + assert (await authenticated_client.get(url)).json()["public"] is False + assert (await client.get(f"{API}/public/{body['id']}")).status_code == 404 diff --git a/modules/file_storage/tests/test_file_storage_public_cookieless.py b/modules/file_storage/tests/test_file_storage_public_cookieless.py new file mode 100644 index 00000000..4d8dbdbe --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_public_cookieless.py @@ -0,0 +1,86 @@ +"""Anonymous public responses are shareable: no session cookie, no ``Vary: Cookie``. + +A response marked ``Cache-Control: public`` that also carries ``Set-Cookie`` +lets a shared cache store one visitor's session and replay it to the next, and +``Vary: Cookie`` splits the cache per visitor for content that does not depend +on who asked. +""" + +from __future__ import annotations + +import io + +import httpx +from file_storage import constants +from PIL import Image + +API = constants.ROUTE_PREFIX_API + + +def _png() -> bytes: + out = io.BytesIO() + Image.new("RGB", (40, 20), (200, 10, 10)).save(out, format="PNG") + return out.getvalue() + + +async def _upload_public(client) -> dict: + resp = await client.post( + f"{API}{constants.PATH_UPLOAD}", + files={"file": ("p.png", _png(), "image/png")}, + data={"public": "true"}, + ) + assert resp.status_code == 201, resp.text + return resp.json() + + +def _assert_shareable(resp: httpx.Response) -> None: + assert "set-cookie" not in resp.headers, resp.headers.get("set-cookie") + vary = [v.strip().lower() for v in resp.headers.get("vary", "").split(",")] + assert "cookie" not in vary, resp.headers.get("vary") + + +def _jar(client: httpx.AsyncClient) -> list[tuple[str, str | None, str]]: + return sorted((c.name, c.value, c.domain) for c in client.cookies.jar) + + +async def test_file_storage_public_responses_set_no_cookie(authenticated_client, client): + body = await _upload_public(authenticated_client) + urls = ( + f"{API}/public/{body['id']}", + body["public_url"], + f"{API}/public/{body['id']}/thumbnail?w=64", + ) + for url in urls: + resp = await client.get(url) + assert resp.status_code == 200, (url, resp.text) + assert resp.headers["cache-control"].startswith("public, max-age=") + _assert_shareable(resp) + again = await client.get(url, headers={"If-None-Match": resp.headers["etag"]}) + assert again.status_code == 304 + _assert_shareable(again) + assert not client.cookies + + +async def test_file_storage_public_miss_sets_no_cookie(client): + resp = await client.get(f"{API}/public/00000000-0000-0000-0000-000000000000") + assert resp.status_code == 404 + _assert_shareable(resp) + + +async def test_file_storage_signed_in_session_survives_public_fetch(authenticated_client): + """A signed-in visitor fetching a public file keeps their session intact.""" + body = await _upload_public(authenticated_client) + before = _jar(authenticated_client) + resp = await authenticated_client.get(f"{API}/public/{body['id']}") + assert resp.status_code == 200 + assert "set-cookie" not in resp.headers + assert _jar(authenticated_client) == before + # And the session still authenticates afterwards. + assert (await authenticated_client.get(f"{API}/files")).status_code == 200 + + +async def test_file_storage_other_routes_keep_session_behaviour(client): + """Only the public file reads are cookieless; an anonymous page still writes it.""" + resp = await client.get("/users/login") + assert resp.status_code == 200 + assert resp.headers.get("set-cookie", "").startswith("session=") diff --git a/modules/file_storage/tests/test_file_storage_thumbnail_hardening.py b/modules/file_storage/tests/test_file_storage_thumbnail_hardening.py index 0971d5b3..cedb7eaa 100644 --- a/modules/file_storage/tests/test_file_storage_thumbnail_hardening.py +++ b/modules/file_storage/tests/test_file_storage_thumbnail_hardening.py @@ -49,6 +49,22 @@ def test_exif_metadata_is_not_carried_over(): assert not img.getexif() +@pytest.mark.parametrize("width", constants.THUMBNAIL_WIDTHS) +def test_16bit_grayscale_png_renders_at_every_width(width): + """``I;16`` cannot be ``reduce()``d; it must render at small widths too.""" + src = Image.new("I;16", (1200, 600)) + src.paste(Image.new("I;16", (600, 600), 40000), (0, 0)) # left mid-high, right black + buf = io.BytesIO() + src.save(buf, format="PNG") + out = thumbnails.render(buf.getvalue(), width, "image/png") + with Image.open(io.BytesIO(out)) as img: + assert img.width == min(width, 1200) + left = img.convert("L").getpixel((1, img.height // 2)) + right = img.convert("L").getpixel((img.width - 2, img.height // 2)) + assert 140 < left < 175 # 40000 / 256 ≈ 156: scaled, not clipped to white + assert right < 10 + + def test_sniffed_format_must_match_declared_type(): with pytest.raises(thumbnails.UnreadableImageError): thumbnails.render(_png(10, 10), 64, "image/jpeg") From da03d56ceeaeae1acb48eb5c340e9cd0f5ad015e Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 6 Oct 2026 22:02:01 +0200 Subject: [PATCH 08/11] fix(file_storage): log orphaned thumbnail variant deletes (review round 1, pass 3) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- modules/file_storage/file_storage/thumbnails.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/modules/file_storage/file_storage/thumbnails.py b/modules/file_storage/file_storage/thumbnails.py index 40858de4..05deb45d 100644 --- a/modules/file_storage/file_storage/thumbnails.py +++ b/modules/file_storage/file_storage/thumbnails.py @@ -17,6 +17,7 @@ import asyncio import io +import logging import struct import weakref from typing import TYPE_CHECKING @@ -29,6 +30,8 @@ if TYPE_CHECKING: from file_storage.contracts.service import StorageBackend +_logger = logging.getLogger(__name__) + class NotAnImageError(Exception): """The file's type has no thumbnail (not a raster image, or SVG).""" @@ -206,7 +209,12 @@ async def _once(): async def delete_variants(backend: StorageBackend, key: str) -> None: """Drop every cached variant of ``key``; absent ones are fine.""" - await asyncio.gather( + results = await asyncio.gather( *(backend.delete(variant_key(key, width)) for width in constants.THUMBNAIL_WIDTHS), return_exceptions=True, ) + for result in results: + # An absent variant is the common case; anything else is an orphan + # nobody would otherwise learn about. + if isinstance(result, Exception) and not isinstance(result, StorageNotFoundError): + _logger.warning("file_storage.variant_delete_failed key=%s error=%r", key, result) From 9d3690ce42e497c5187cfca5af2d9c20ffbfd99c Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 7 Oct 2026 09:08:15 +0200 Subject: [PATCH 09/11] test: reconcile file_storage/branding tests with #401 and #404 on main Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- modules/branding/tests/test_branding_reap_ordering.py | 4 +++- .../file_storage/tests/test_file_storage_public_cookieless.py | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/modules/branding/tests/test_branding_reap_ordering.py b/modules/branding/tests/test_branding_reap_ordering.py index 2d8468e0..40ab6e8a 100644 --- a/modules/branding/tests/test_branding_reap_ordering.py +++ b/modules/branding/tests/test_branding_reap_ordering.py @@ -80,7 +80,9 @@ async def delete(key): monkeypatch.setattr(backend, "delete", delete) await reaper.reap(app, file_id, tenant_id=a.tenant_id) - assert events == ["commit", "delete"] + # The original and any cached thumbnail variants: every delete after the commit. + assert events[0] == "commit" and "delete" in events + assert set(events[1:]) == {"delete"} assert (await _row(app, file_id)).is_deleted is True diff --git a/modules/file_storage/tests/test_file_storage_public_cookieless.py b/modules/file_storage/tests/test_file_storage_public_cookieless.py index 4d8dbdbe..35ac8454 100644 --- a/modules/file_storage/tests/test_file_storage_public_cookieless.py +++ b/modules/file_storage/tests/test_file_storage_public_cookieless.py @@ -81,6 +81,7 @@ async def test_file_storage_signed_in_session_survives_public_fetch(authenticate async def test_file_storage_other_routes_keep_session_behaviour(client): """Only the public file reads are cookieless; an anonymous page still writes it.""" - resp = await client.get("/users/login") + # A real page load (Accept: text/html) — anonymous non-page calls stay cookieless (#349). + resp = await client.get("/users/login", headers={"Accept": "text/html"}) assert resp.status_code == 200 assert resp.headers.get("set-cookie", "").startswith("session=") From bccb36095066ef16861e05ecdc29ce2c59be67fb Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 7 Oct 2026 09:29:31 +0200 Subject: [PATCH 10/11] fix(migrations): chain file_storage public column after #406's timestamp revision Both branched from c7f2d9a41e83; repoint down_revision so main keeps a single mainline head. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../versions/e5a8c1f27b94_file_storage_stored_file_public.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/host/migrations/versions/e5a8c1f27b94_file_storage_stored_file_public.py b/host/migrations/versions/e5a8c1f27b94_file_storage_stored_file_public.py index ae7b14e2..05196985 100644 --- a/host/migrations/versions/e5a8c1f27b94_file_storage_stored_file_public.py +++ b/host/migrations/versions/e5a8c1f27b94_file_storage_stored_file_public.py @@ -5,7 +5,7 @@ Postgres, so the same migration runs on both. Revision ID: e5a8c1f27b94 -Revises: c7f2d9a41e83 +Revises: e5f2a8c1d7b3 Create Date: 2026-10-05 10:00:00.000000 """ @@ -16,7 +16,7 @@ # revision identifiers, used by Alembic. revision: str = "e5a8c1f27b94" -down_revision: str | None = "c7f2d9a41e83" +down_revision: str | None = "e5f2a8c1d7b3" branch_labels: str | Sequence[str] | None = None depends_on: str | Sequence[str] | None = None From cb6e1a509ed4a82be3eaf850dc11ef8c0d23883c Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 7 Oct 2026 09:55:04 +0200 Subject: [PATCH 11/11] feat(file_storage): own rate-limit bucket for anonymous public file GETs With #408's shared limiter on main, public file reads shared the 120/minute anonymous default; a page embedding many images would throttle itself. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- modules/file_storage/file_storage/constants.py | 3 +++ modules/file_storage/file_storage/module.py | 5 ++++- .../tests/test_file_storage_public_rate.py | 13 +++++++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) create mode 100644 modules/file_storage/tests/test_file_storage_public_rate.py diff --git a/modules/file_storage/file_storage/constants.py b/modules/file_storage/file_storage/constants.py index e196f275..2242d55e 100644 --- a/modules/file_storage/file_storage/constants.py +++ b/modules/file_storage/file_storage/constants.py @@ -122,6 +122,9 @@ class I18nKey: # ── Public serving & thumbnails ────────────────────────────────────── PUBLIC_MAX_AGE_SECONDS: Final = 3600 +# Per-IP budget for anonymous public file GETs (its own bucket in the shared +# rate limiter, #347), wider than the default because one page embeds many. +PUBLIC_FILES_RATE: Final = "600/minute" # Types a browser would execute or render as a document on our origin. They are # served as attachments (and sandboxed) so a public upload cannot become stored # XSS on the app's origin. diff --git a/modules/file_storage/file_storage/module.py b/modules/file_storage/file_storage/module.py index e1b4327a..d714106e 100644 --- a/modules/file_storage/file_storage/module.py +++ b/modules/file_storage/file_storage/module.py @@ -104,9 +104,12 @@ def register_public_routes(self, registry: PublicRouteRegistry) -> None: uploads, deletes and the authenticated download keep requiring a session. The handler itself still refuses anything not ``public``. """ + from file_storage.constants import PUBLIC_FILES_RATE from file_storage.cookieless import PUBLIC_PATH_PATTERN - registry.add_regex(PUBLIC_PATH_PATTERN, methods={"GET"}) + # Its own, wider bucket: one public page can embed dozens of images and + # thumbnails, which would exhaust the shared anonymous default. + registry.add_regex(PUBLIC_PATH_PATTERN, methods={"GET"}, rate=PUBLIC_FILES_RATE) def register_middleware(self, app: FastAPI) -> None: """Serve public files without a session cookie or ``Vary: Cookie``.""" diff --git a/modules/file_storage/tests/test_file_storage_public_rate.py b/modules/file_storage/tests/test_file_storage_public_rate.py new file mode 100644 index 00000000..74744ea9 --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_public_rate.py @@ -0,0 +1,13 @@ +"""Public file GETs get their own rate-limit bucket, wider than the default.""" + +from __future__ import annotations + +from file_storage import constants + + +async def test_file_storage_public_route_has_its_own_rate(app) -> None: + file_id = "00000000-0000-0000-0000-000000000000" + path = f"{constants.ROUTE_PREFIX_API}{constants.PUBLIC_SEGMENT}/{file_id}" + rule = app.state.public_routes.match("GET", path) + assert rule is not None + assert rule.rate == constants.PUBLIC_FILES_RATE