fix: production-readiness hardening from full-codebase review
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.
This commit is contained in:
@@ -32,12 +32,15 @@
|
||||
</script>
|
||||
|
||||
{#if snacks.length > 0}
|
||||
<div use:portal class="snackbar-container">
|
||||
<div use:portal class="snackbar-container" role="region" aria-label={t('snackbar.region')}>
|
||||
{#each snacks as snack (snack.id)}
|
||||
<div
|
||||
in:fly={{ y: 40, duration: 300 }}
|
||||
out:fade={{ duration: 200 }}
|
||||
class="snack-item"
|
||||
role={snack.type === 'error' ? 'alert' : 'status'}
|
||||
aria-live={snack.type === 'error' ? 'assertive' : 'polite'}
|
||||
aria-atomic="true"
|
||||
style="--snack-accent: {accentMap[snack.type]};"
|
||||
>
|
||||
<span class="snack-icon" style="color: {accentMap[snack.type]};">
|
||||
|
||||
@@ -1141,7 +1141,8 @@
|
||||
},
|
||||
"snackbar": {
|
||||
"showDetails": "Show details",
|
||||
"hideDetails": "Hide details"
|
||||
"hideDetails": "Hide details",
|
||||
"region": "Notifications"
|
||||
},
|
||||
"timezone": {
|
||||
"searchPlaceholder": "Search cities or IANA codes…",
|
||||
|
||||
@@ -1141,7 +1141,8 @@
|
||||
},
|
||||
"snackbar": {
|
||||
"showDetails": "Показать детали",
|
||||
"hideDetails": "Скрыть детали"
|
||||
"hideDetails": "Скрыть детали",
|
||||
"region": "Уведомления"
|
||||
},
|
||||
"timezone": {
|
||||
"searchPlaceholder": "Поиск по городам или IANA-кодам…",
|
||||
|
||||
@@ -13,6 +13,7 @@ export const immichDescriptor: ProviderDescriptor = {
|
||||
icon: 'mdiImageMultiple',
|
||||
hasUrl: true,
|
||||
urlPlaceholder: undefined, // uses generic i18n placeholder
|
||||
supportsAutoOrganize: true,
|
||||
|
||||
configFields: [
|
||||
{
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -499,6 +499,7 @@
|
||||
<a
|
||||
href={child.href}
|
||||
class="nav-link nav-link-child group flex items-center gap-2 px-2.5 py-1.5 rounded-lg text-sm transition-all duration-200 relative {isActive(child.href) ? 'active' : ''}"
|
||||
aria-current={isActive(child.href) ? 'page' : undefined}
|
||||
>
|
||||
{#if isActive(child.href)}
|
||||
<div class="active-indicator" style="position: absolute; left: -13px; top: 50%; transform: translateY(-50%); width: 3px; height: 60%; border-radius: 0 3px 3px 0; background: var(--color-primary); box-shadow: 0 0 8px var(--color-glow-strong);"></div>
|
||||
@@ -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)}
|
||||
<div class="active-indicator" style="position: absolute; left: 0; top: 50%; transform: translateY(-50%); width: 3px; height: 60%; border-radius: 0 3px 3px 0; background: var(--color-primary); box-shadow: 0 0 8px var(--color-glow-strong);"></div>
|
||||
|
||||
@@ -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<any>(`/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
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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 "***"
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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"}
|
||||
|
||||
@@ -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")
|
||||
)
|
||||
|
||||
|
||||
|
||||
@@ -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(
|
||||
|
||||
Reference in New Issue
Block a user