From 1ac448d881eb9a94bdf7b75be16df702d323075c Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 07:48:06 -0400 Subject: [PATCH] refactor: four small cleanups from the review pass (#3072) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Items 2-5 of #3072. Item 1 (the per-row sweep inserts) is separate. 2. .fc-bad was not merely duplicated — it is .fc-weak under a second name. Both local definitions were `color: rgb(var(--v-theme-error))`, identical to the global .fc-weak, and GpuAgentCard was already using .fc-weak to colour exactly what GpuActivityPanel coloured .fc-bad (an errored count, red when non-zero). So rather than promoting a synonym to app.css, both call sites now use .fc-weak and the local defs are gone. app.css's status-colour comment records why there is no .fc-bad, next to the existing note on why .fc-ok is deliberately NOT global. 3. GalleryItem.vue's obsidian literals now use --v-theme-background, which IS obsidian (vuetify-theme.js maps background -> surfaces. obsidian). Preferred over --fc-chrome-rgb: same value, but that variable is named for the nav fade, not for the palette entry. The ticket said these were the only three real uses in the tree. They are not — GalleryItem itself had two more in the artist-label gradient (fixed here, so the file is now consistent), and ~13 more live in SeriesView, SeriesReaderView, ImageViewer, ArtistHeader, ExploreView and GalleryFilterBar. Those are a separate sweep, filed rather than folded in here. 4. The attachment download path had two hand-formatted copies. One definition now, `attachment_download_url`, next to the model both serializers already import. The test pins it by MATCHING the built path against the app's real URL map rather than comparing to a literal — a string-equality test would still pass after someone renamed the route, which is the drift the helper exists to prevent. 5. Extension API key now compares with hmac.compare_digest. Compared as BYTES, not str: compare_digest's str form raises TypeError on non-ASCII, and this value comes straight from an attacker-controlled header, so the str form would turn a junk key into a 500 instead of a 403. Low stakes either way — the API is unauthenticated-by-design on a LAN — but it costs nothing. Refs #3072 --- backend/app/api/extension.py | 11 ++++++++++- backend/app/models/__init__.py | 3 ++- backend/app/models/post_attachment.py | 12 ++++++++++++ backend/app/services/post_feed_service.py | 3 ++- backend/app/services/provenance_service.py | 3 ++- frontend/src/components/gallery/GalleryItem.vue | 9 +++++---- .../settings/DownloadsActivityPanel.vue | 3 +-- .../src/components/settings/GpuActivityPanel.vue | 3 +-- frontend/src/styles/app.css | 7 ++++++- tests/test_api_attachments.py | 15 ++++++++++++++- 10 files changed, 55 insertions(+), 14 deletions(-) diff --git a/backend/app/api/extension.py b/backend/app/api/extension.py index 20b1b30..082bc1e 100644 --- a/backend/app/api/extension.py +++ b/backend/app/api/extension.py @@ -6,6 +6,7 @@ from __future__ import annotations import asyncio import hashlib +import hmac import re from pathlib import Path @@ -41,7 +42,15 @@ async def _ext_key_required(session) -> bool: stored = (await session.execute( select(AppSetting.value).where(AppSetting.key == "extension_api_key") )).scalar_one_or_none() - return stored is not None and supplied == stored + if stored is None: + return False + # compare_digest, not `==`: the stored key is a shared secret, and a + # short-circuiting compare leaks its prefix through timing. Costs nothing + # here — it is not that this route is exposed (#3072). Compared as BYTES: + # compare_digest's str form rejects non-ASCII with TypeError, and this + # header is attacker-supplied, so a str compare would turn a junk key into + # a 500 instead of a 403. + return hmac.compare_digest(supplied.encode("utf-8"), stored.encode("utf-8")) def _extract_version(xpi_name: str) -> str: diff --git a/backend/app/models/__init__.py b/backend/app/models/__init__.py index f06d74d..a11d267 100644 --- a/backend/app/models/__init__.py +++ b/backend/app/models/__init__.py @@ -27,7 +27,7 @@ from .patreon_seen_media import PatreonSeenMedia from .pixiv_failed_media import PixivFailedMedia from .pixiv_seen_media import PixivSeenMedia from .post import Post -from .post_attachment import PostAttachment +from .post_attachment import PostAttachment, attachment_download_url from .presentation_review import PresentationReview from .series_chapter import SeriesChapter from .series_page import SeriesPage @@ -58,6 +58,7 @@ __all__ = [ "SubscribeStarSeenMedia", "Post", "PostAttachment", + "attachment_download_url", "PresentationReview", "SeriesChapter", "SeriesPage", diff --git a/backend/app/models/post_attachment.py b/backend/app/models/post_attachment.py index 1edb7f3..f522cdf 100644 --- a/backend/app/models/post_attachment.py +++ b/backend/app/models/post_attachment.py @@ -65,3 +65,15 @@ class PostAttachment(Base): captured_at: Mapped[datetime] = mapped_column( DateTime(timezone=True), nullable=False, server_default=func.now() ) + + +def attachment_download_url(attachment_id: int) -> str: + """The path that streams this attachment's bytes. + + Both serializers that expose an attachment to the frontend + (`provenance_service`, `post_feed_service`) built this literal themselves, + so changing the route in `api/attachments.py` meant two edits and only one + would be remembered (#3072). `test_attachment_download_url` pins it against + the app's registered rule, so the drift is caught rather than trusted to. + """ + return f"/api/attachments/{attachment_id}/download" diff --git a/backend/app/services/post_feed_service.py b/backend/app/services/post_feed_service.py index c1304ac..d4aec83 100644 --- a/backend/app/services/post_feed_service.py +++ b/backend/app/services/post_feed_service.py @@ -24,6 +24,7 @@ from ..models import ( Post, PostAttachment, Source, + attachment_download_url, ) from ..utils.html_sanitize import ( extract_img_srcs, @@ -360,7 +361,7 @@ class PostFeedService: "ext": att.ext, "mime": att.mime, "size_bytes": att.size_bytes, - "download_url": f"/api/attachments/{att.id}/download", + "download_url": attachment_download_url(att.id), }) return out diff --git a/backend/app/services/provenance_service.py b/backend/app/services/provenance_service.py index 6fee555..02f1a64 100644 --- a/backend/app/services/provenance_service.py +++ b/backend/app/services/provenance_service.py @@ -16,6 +16,7 @@ from ..models import ( Post, PostAttachment, Source, + attachment_download_url, ) from ..utils.html_sanitize import sanitize_post_html @@ -53,7 +54,7 @@ def _attachment_dict(a: PostAttachment) -> dict: "original_filename": a.original_filename, "size_bytes": a.size_bytes, "ext": a.ext, - "download_url": f"/api/attachments/{a.id}/download", + "download_url": attachment_download_url(a.id), } diff --git a/frontend/src/components/gallery/GalleryItem.vue b/frontend/src/components/gallery/GalleryItem.vue index cab8b9e..f5fd815 100644 --- a/frontend/src/components/gallery/GalleryItem.vue +++ b/frontend/src/components/gallery/GalleryItem.vue @@ -139,9 +139,9 @@ function onThumbError() { thumbError.value = true } position: absolute; top: 8px; left: 8px; width: 22px; height: 22px; border-radius: 4px; border: 2px solid rgba(232, 228, 216, 0.8); - background: rgba(20, 23, 26, 0.45); + background: rgba(var(--v-theme-background), 0.45); display: grid; place-items: center; - color: #14171A; z-index: 11; + color: rgb(var(--v-theme-background)); z-index: 11; } .fc-gallery-item__checkbox.on { background: rgb(var(--v-theme-accent)); @@ -152,7 +152,7 @@ function onThumbError() { thumbError.value = true } min-width: 22px; height: 22px; padding: 0 5px; border-radius: 11px; background: rgb(var(--v-theme-accent)); - color: #14171A; font-size: 12px; font-weight: 700; + color: rgb(var(--v-theme-background)); font-size: 12px; font-weight: 700; display: grid; place-items: center; z-index: 11; pointer-events: none; } @@ -160,7 +160,8 @@ function onThumbError() { thumbError.value = true } position: absolute; left: 0; right: 0; bottom: 0; padding: 14px 8px 6px; background: linear-gradient( - to top, rgba(20, 23, 26, 0.78), rgba(20, 23, 26, 0) + to top, rgba(var(--v-theme-background), 0.78), + rgba(var(--v-theme-background), 0) ); font-size: 12px; line-height: 1.2; white-space: nowrap; overflow: hidden; text-overflow: ellipsis; diff --git a/frontend/src/components/settings/DownloadsActivityPanel.vue b/frontend/src/components/settings/DownloadsActivityPanel.vue index d8f9d16..380b44a 100644 --- a/frontend/src/components/settings/DownloadsActivityPanel.vue +++ b/frontend/src/components/settings/DownloadsActivityPanel.vue @@ -35,7 +35,7 @@ All subscription sources healthy.

- {{ failing.length }} failing source(s): + {{ failing.length }} failing source(s): {{ failingNames }}

@@ -72,5 +72,4 @@ onUnmounted(() => { if (pollId) clearInterval(pollId) }) diff --git a/frontend/src/components/settings/GpuActivityPanel.vue b/frontend/src/components/settings/GpuActivityPanel.vue index 39bba29..1f85a5b 100644 --- a/frontend/src/components/settings/GpuActivityPanel.vue +++ b/frontend/src/components/settings/GpuActivityPanel.vue @@ -25,7 +25,7 @@
done
-
{{ q.error }}
+
{{ q.error }}
errored
@@ -104,5 +104,4 @@ onUnmounted(() => { if (pollId) clearInterval(pollId) }) font-size: 11px; text-transform: uppercase; letter-spacing: 0.04em; color: rgb(var(--v-theme-on-surface-variant)); } -.fc-bad { color: rgb(var(--v-theme-error)); } diff --git a/frontend/src/styles/app.css b/frontend/src/styles/app.css index 3e24a75..fbd9380 100644 --- a/frontend/src/styles/app.css +++ b/frontend/src/styles/app.css @@ -50,7 +50,12 @@ /* Status text colours (DRY pass #161): fc-good = success, fc-weak = error, consolidated from the GPU / heads cards. fc-ok is intentionally NOT global — - it means on-surface in HeadsCard but success in QueuesTable. */ + it means on-surface in HeadsCard but success in QueuesTable. + + No `.fc-bad` (#3072): it was defined locally and identically in the Downloads + and GPU activity panels, and it is fc-weak under a second name — GpuAgentCard + and GpuActivityPanel were colouring the same "errored" count with different + class names. Both now use fc-weak. Reach for fc-weak, not a new synonym. */ .fc-good { color: rgb(var(--v-theme-success)); } .fc-weak { color: rgb(var(--v-theme-error)); } diff --git a/tests/test_api_attachments.py b/tests/test_api_attachments.py index 95e31f2..7918008 100644 --- a/tests/test_api_attachments.py +++ b/tests/test_api_attachments.py @@ -1,6 +1,6 @@ import pytest -from backend.app.models import Artist, PostAttachment +from backend.app.models import Artist, PostAttachment, attachment_download_url pytestmark = pytest.mark.integration @@ -33,3 +33,16 @@ async def test_download_streams_with_disposition(client, db, tmp_path): async def test_download_404(client): resp = await client.get("/api/attachments/999999/download") assert resp.status_code == 404 + + +@pytest.mark.asyncio +async def test_attachment_download_url_routes_to_the_download_endpoint(app): + """The two serializers no longer hand-format this path (#3072) — but a + single definition is only worth having if it still matches the route. Pin + it by MATCHING against the real URL map rather than comparing to a literal: + a string equality test would pass just as happily after someone renamed the + route, which is the exact drift the helper exists to prevent.""" + built = attachment_download_url(4242) + endpoint, args = app.url_map.bind("localhost").match(built) + assert endpoint == "attachments.download" + assert args == {"attachment_id": 4242}