From 2d59a5b99436af3d1c54dce49be3fb28bf91b997 Mon Sep 17 00:00:00 2001 From: "alexei.dolgolyov" Date: Fri, 22 May 2026 22:47:20 +0300 Subject: [PATCH] fix: production-readiness hardening from full-codebase review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Apply six isolated, low-risk fixes surfaced by the parallel production-readiness review (backend, frontend, security, perf, UI/UX, bugs+features). Backend - Mask access_token in provider GET responses and drop it on edit when carrying the *** placeholder — fixes plaintext leak of HA long-lived tokens (security H-1). Centralized via PROVIDER_SECRET_FIELDS so all call sites stay in sync (C-5). - Hold HA status-change tasks in a module-level set with a done_callback — asyncio.create_task only keeps weak refs and the task could be GC'd before its row was written (C-1). - Roll back the request session in the Telegram-webhook catch-all so a handler exception cannot leak uncommitted writes into the next request (C-2). - Bail before reading the 1 MiB webhook body when the Gitea provider has no secret configured or the request has no signature header. For the generic webhook with bearer_token auth, verify the Authorization header before the body read. Closes the pre-auth resource-exhaustion amplifier (C-3). Frontend - Add supportsAutoOrganize capability to ProviderDescriptor and consume it from RuleEditor instead of `provider.type !== 'immich'`, bringing the last action-rule editor under CLAUDE.md rule 8 (no provider-type hardcoding in components). - Snackbar: add role="region" + per-toast role/aria-live/aria-atomic so screen readers announce success/error toasts. - Sidebar nav: add aria-current="page" on the active link so the active state has an accessible name. - New snackbar.region key in en + ru (locale parity preserved). Out of scope for this commit (tracked in .claude/reviews/README.md ship-blocker list): secret encryption at rest, JWT cookie move, Alembic adoption, webhook idempotency, deferred-dispatch crash window, persisted Telegram update watermark, bridge_self counter lock — each needs more than a mechanical edit. --- frontend/src/lib/components/Snackbar.svelte | 5 +++- frontend/src/lib/i18n/en.json | 3 +- frontend/src/lib/i18n/ru.json | 3 +- frontend/src/lib/providers/immich.ts | 1 + frontend/src/lib/providers/types.ts | 9 ++++++ frontend/src/routes/+layout.svelte | 2 ++ frontend/src/routes/actions/RuleEditor.svelte | 7 +++-- .../src/notify_bridge_server/api/providers.py | 10 ++----- .../src/notify_bridge_server/api/webhooks.py | 28 +++++++++++++++---- .../notify_bridge_server/commands/webhook.py | 9 ++++++ .../services/backup_schema.py | 9 ++++-- .../services/ha_subscription.py | 12 +++++++- 12 files changed, 77 insertions(+), 21 deletions(-) diff --git a/frontend/src/lib/components/Snackbar.svelte b/frontend/src/lib/components/Snackbar.svelte index a788d9f..d02aa93 100644 --- a/frontend/src/lib/components/Snackbar.svelte +++ b/frontend/src/lib/components/Snackbar.svelte @@ -32,12 +32,15 @@ {#if snacks.length > 0} -
+
{#each snacks as snack (snack.id)}
diff --git a/frontend/src/lib/i18n/en.json b/frontend/src/lib/i18n/en.json index da9b0d1..7521113 100644 --- a/frontend/src/lib/i18n/en.json +++ b/frontend/src/lib/i18n/en.json @@ -1141,7 +1141,8 @@ }, "snackbar": { "showDetails": "Show details", - "hideDetails": "Hide details" + "hideDetails": "Hide details", + "region": "Notifications" }, "timezone": { "searchPlaceholder": "Search cities or IANA codes…", diff --git a/frontend/src/lib/i18n/ru.json b/frontend/src/lib/i18n/ru.json index 7e55fc6..2c8e482 100644 --- a/frontend/src/lib/i18n/ru.json +++ b/frontend/src/lib/i18n/ru.json @@ -1141,7 +1141,8 @@ }, "snackbar": { "showDetails": "Показать детали", - "hideDetails": "Скрыть детали" + "hideDetails": "Скрыть детали", + "region": "Уведомления" }, "timezone": { "searchPlaceholder": "Поиск по городам или IANA-кодам…", diff --git a/frontend/src/lib/providers/immich.ts b/frontend/src/lib/providers/immich.ts index 5ba8ec0..3639019 100644 --- a/frontend/src/lib/providers/immich.ts +++ b/frontend/src/lib/providers/immich.ts @@ -13,6 +13,7 @@ export const immichDescriptor: ProviderDescriptor = { icon: 'mdiImageMultiple', hasUrl: true, urlPlaceholder: undefined, // uses generic i18n placeholder + supportsAutoOrganize: true, configFields: [ { diff --git a/frontend/src/lib/providers/types.ts b/frontend/src/lib/providers/types.ts index 338871f..2183cda 100644 --- a/frontend/src/lib/providers/types.ts +++ b/frontend/src/lib/providers/types.ts @@ -196,6 +196,15 @@ export interface ProviderDescriptor { /** Whether this provider stores incoming payload history for debugging. */ payloadHistory?: boolean; + // ── Capability flags ── + /** + * True when the provider exposes asset/people/album endpoints that the + * Auto-Organize action rule editor needs to render its people / album + * pickers (currently only Immich). Used in place of `type === 'immich'` + * checks per CLAUDE.md rule 8. + */ + supportsAutoOrganize?: boolean; + // ── Tracker-form discovery hint ── /** * Optional info banner shown on the TrackerForm to point users at related diff --git a/frontend/src/routes/+layout.svelte b/frontend/src/routes/+layout.svelte index 2d23704..eb38fe1 100644 --- a/frontend/src/routes/+layout.svelte +++ b/frontend/src/routes/+layout.svelte @@ -499,6 +499,7 @@ {#if isActive(child.href)}
@@ -518,6 +519,7 @@ href={entry.href} class="nav-link group flex items-center gap-2.5 {collapsed ? 'justify-center px-2' : 'px-3'} py-2 rounded-lg text-sm transition-all duration-200 relative {isActive(entry.href) ? 'active' : ''}" title={collapsed ? t(entry.key) : ''} + aria-current={isActive(entry.href) ? 'page' : undefined} > {#if isActive(entry.href)}
diff --git a/frontend/src/routes/actions/RuleEditor.svelte b/frontend/src/routes/actions/RuleEditor.svelte index 2222f25..3c8d2e7 100644 --- a/frontend/src/routes/actions/RuleEditor.svelte +++ b/frontend/src/routes/actions/RuleEditor.svelte @@ -7,6 +7,7 @@ import IconButton from '$lib/components/IconButton.svelte'; import EntitySelect from '$lib/components/EntitySelect.svelte'; import MultiEntitySelect from '$lib/components/MultiEntitySelect.svelte'; + import { getDescriptor } from '$lib/providers'; import type { ActionRule } from '$lib/types'; let { actionId, actionType, providerId }: { actionId: number; actionType: string; providerId: number } = $props(); @@ -54,7 +55,9 @@ async function loadProviderData() { if (actionType !== 'auto_organize') return; const provider = providersCache.items.find((p: any) => p.id === providerId); - if (!provider || provider.type !== 'immich') return; + if (!provider) return; + const descriptor = getDescriptor(provider.type); + if (!descriptor?.supportsAutoOrganize) return; try { const [p, a] = await Promise.all([ api(`/providers/${providerId}/people`), @@ -63,7 +66,7 @@ people = Array.isArray(p) ? p : []; albums = Array.isArray(a) ? a : []; } catch { - // People/album endpoints may not exist yet — degrade gracefully + // People/album endpoints may not exist yet — degrade gracefully } } diff --git a/packages/server/src/notify_bridge_server/api/providers.py b/packages/server/src/notify_bridge_server/api/providers.py index 6aa6d55..9937ffd 100644 --- a/packages/server/src/notify_bridge_server/api/providers.py +++ b/packages/server/src/notify_bridge_server/api/providers.py @@ -18,6 +18,7 @@ from ..services import ( make_immich_provider, make_gitea_provider, make_planka_provider, make_nut_provider, make_google_photos_provider, list_provider_collections, ) +from ..services.backup_schema import PROVIDER_SECRET_FIELDS from ..services.http_session import get_http_session from .helpers import get_owned_entity @@ -396,10 +397,7 @@ async def update_provider( # user saves without re-entering them. Any field that still carries # our mask placeholder ("***…") is dropped from the incoming body. incoming = dict(body.config) - for secret_field in ( - "api_key", "api_token", "webhook_secret", "password", - "client_secret", "refresh_token", - ): + for secret_field in PROVIDER_SECRET_FIELDS: value = incoming.get(secret_field) if isinstance(value, str) and value.startswith("***"): incoming.pop(secret_field, None) @@ -616,9 +614,7 @@ async def create_album_shared_link( def _provider_response(p: ServiceProvider) -> dict: """Build a safe response dict for a provider.""" config = dict(p.config) - # Mask sensitive fields - for secret_field in ("api_key", "api_token", "webhook_secret", "password", - "client_secret", "refresh_token"): + for secret_field in PROVIDER_SECRET_FIELDS: if secret_field in config: key = config[secret_field] config[secret_field] = f"***{key[-4:]}" if len(key) > 4 else "***" diff --git a/packages/server/src/notify_bridge_server/api/webhooks.py b/packages/server/src/notify_bridge_server/api/webhooks.py index 3166311..3a28854 100644 --- a/packages/server/src/notify_bridge_server/api/webhooks.py +++ b/packages/server/src/notify_bridge_server/api/webhooks.py @@ -164,17 +164,23 @@ async def gitea_webhook(token: str, request: Request): webhook_secret = (provider.config or {}).get("webhook_secret", "") - # Read raw body for HMAC check - raw_body = await _read_bounded_body(request) - + # Bail BEFORE reading the body if either the provider is missing a + # secret (admin misconfiguration) or the inbound request has no + # signature header. Either way the request can never authenticate, + # so there's no reason to spend the 1 MiB body read first. if not webhook_secret: raise HTTPException( status_code=403, detail="Webhook secret not configured on this provider", ) - signature = request.headers.get("X-Gitea-Signature", "") - if not signature or not _verify_gitea_signature(webhook_secret, raw_body, signature): + if not signature: + raise HTTPException(status_code=403, detail="Invalid signature") + + # Body needed for the HMAC check — reads at most _MAX_WEBHOOK_BODY_BYTES. + raw_body = await _read_bounded_body(request) + + if not _verify_gitea_signature(webhook_secret, raw_body, signature): raise HTTPException(status_code=403, detail="Invalid signature") # Parse event header + payload @@ -446,6 +452,18 @@ async def generic_webhook(token: str, request: Request): store_payloads = provider_config.get("store_payloads", True) max_stored = min(max(int(provider_config.get("max_stored_payloads", 20)), 1), 100) + # Reject misconfigured providers (auth_mode requires a secret but none + # set) BEFORE the 1 MiB body read. For non-HMAC modes we can also + # verify the credential header up front; HMAC needs the body. + auth_mode = provider_config.get("auth_mode", "none") + if auth_mode in {"hmac_sha256", "bearer_token"} and not provider_config.get("webhook_secret"): + raise HTTPException(status_code=403, detail="Authentication failed") + if auth_mode == "bearer_token": + auth_header = request.headers.get("Authorization", "") + secret = provider_config.get("webhook_secret", "") + if not auth_header.startswith("Bearer ") or not hmac.compare_digest(auth_header[7:], secret): + raise HTTPException(status_code=403, detail="Authentication failed") + raw_body = await _read_bounded_body(request) # Bounded read above already enforces the size cap; no need to re-check. diff --git a/packages/server/src/notify_bridge_server/commands/webhook.py b/packages/server/src/notify_bridge_server/commands/webhook.py index 486b23f..21b3971 100644 --- a/packages/server/src/notify_bridge_server/commands/webhook.py +++ b/packages/server/src/notify_bridge_server/commands/webhook.py @@ -164,6 +164,15 @@ async def telegram_webhook( "Command /%s raised after %.0f ms", cmd_name, (time.monotonic() - started) * 1000, ) + # Roll back any uncommitted writes on this request session. + # The dispatcher path may have opened its own sessions, but + # anything left dangling on the request-scoped session + # would otherwise leak into the next request (FastAPI's + # get_session dependency only closes — it does not roll back). + try: + await session.rollback() + except Exception: # noqa: BLE001 + _LOGGER.debug("Rollback after handler exception failed", exc_info=True) # Return 200 so Telegram doesn't retry the same update — we # already logged the failure and can't usefully reprocess. return {"ok": True, "error": "handler_exception"} diff --git a/packages/server/src/notify_bridge_server/services/backup_schema.py b/packages/server/src/notify_bridge_server/services/backup_schema.py index bf63ddf..c58d5e1 100644 --- a/packages/server/src/notify_bridge_server/services/backup_schema.py +++ b/packages/server/src/notify_bridge_server/services/backup_schema.py @@ -38,10 +38,13 @@ class BackupCategory(str, Enum): ALL_CATEGORIES = list(BackupCategory) -# Secret fields in provider config dicts +# Secret fields in provider config dicts. Used both to mask values on API +# responses and to scrub fields from backup exports — keep both call sites +# pointing at this constant so adding a new secret-bearing provider only +# requires editing here. PROVIDER_SECRET_FIELDS = frozenset( - ("api_key", "api_token", "webhook_secret", "password", - "client_secret", "refresh_token") + ("api_key", "api_token", "access_token", "webhook_secret", + "password", "client_secret", "refresh_token") ) diff --git a/packages/server/src/notify_bridge_server/services/ha_subscription.py b/packages/server/src/notify_bridge_server/services/ha_subscription.py index e68c88d..c676481 100644 --- a/packages/server/src/notify_bridge_server/services/ha_subscription.py +++ b/packages/server/src/notify_bridge_server/services/ha_subscription.py @@ -46,6 +46,14 @@ _LOGGER = logging.getLogger(__name__) # find and replace a single task without disturbing the rest. _running_tasks: dict[int, asyncio.Task[None]] = {} +# Fire-and-forget tasks spawned from synchronous callbacks (HA status +# transitions). Held here so they can't be GC'd before completion — +# asyncio.create_task only keeps a weak reference internally, so a task +# whose only ref is the create_task return value can disappear under +# memory pressure. See https://docs.python.org/3/library/asyncio-task.html +# (asyncio.create_task — "Save a reference to the result"). +_status_tasks: set[asyncio.Task[None]] = set() + # Keys from ``event.extra`` to copy into ``EventLog.details``. Anything not # in this list is still available to templates via the merged extras, but @@ -246,12 +254,14 @@ async def _run_provider(provider_id: int) -> None: propagate them back into the WS reader. """ try: - asyncio.create_task(_record_ha_status( + task = asyncio.create_task(_record_ha_status( provider_id=provider_id, provider_name=provider_name, state=state, detail=detail, )) + _status_tasks.add(task) + task.add_done_callback(_status_tasks.discard) except RuntimeError: # No running loop (shouldn't happen in normal operation). _LOGGER.debug(