fix(security,perf): harden restore, CSRF, token_version + perf pass
Security
- Sign pending_restore.json (SHA256 stored in AppSetting, verified on
startup apply) + refuse path outside data_dir, tighten to 0600.
- Require same-origin Origin/Referer on POST /api/backup/apply-restart —
Bearer-in-localStorage is CSRF-reachable from any XSS'd admin tab.
- Bump token_version on role/username change and admin password reset so
demoted admins lose admin in already-issued JWTs. Guard last-admin
TOCTOU via COUNT + post-commit re-check that rolls back a race.
- SSRF guard (validate_outbound_url) in ImmichClient.__init__ and the
external_domain setter — admin-mutable URLs were bypassing the check
that webhook/slack/discord paths already used. Dev restart script now
sets NOTIFY_BRIDGE_ALLOW_PRIVATE_URLS=1 so homelab Immich still works.
- Redact + cap Immich error bodies to ~120 chars before they flow into
ActionExecution.error / EventLog.details (both UI-visible).
- Deny-list sensitive keys (api_key / token / secret / password /
authorization / cookie / ...) in template-context merges so a rogue
template can't exfiltrate provider creds via {{ api_key }}.
- Cap user-controlled Immich search params (query ≤256, person_ids ≤50,
size ≤100) so a Telegram listener can't DoS upstream.
- Stream upload reads with running byte counter + content-length precheck
instead of buffering the full body and then rejecting.
- Log Telegram parse_mode fallbacks instead of swallowing silently;
template escape bugs now surface in server logs.
- Rollback partial imports on pending-restore failure (error recorded on
a fresh session).
Performance
- Fix N+1 in _refresh_telegram_chat_titles: single IN query instead of
session.get per chat.
- Parallelize album + shared-link fetches in test_dispatch (asyncio.gather)
and per-receiver Telegram test sends in notifier (semaphore 5).
- Early-exit collect_scheduled_assets(limit=0) so the periodic-summary
test path skips full per-album filter/sample (was O(album_assets)).
- Emit explicit CREATE INDEX IF NOT EXISTS for event_log user_id /
action_id / provider_id so the first boot after upgrade isn't left
unindexed for the dashboard query.
- Add AbortController timeout (120s) to fetchAuth so uploads/downloads
don't hang indefinitely.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -300,6 +300,16 @@ class TelegramClient:
|
||||
# Retry without parse_mode on parse errors
|
||||
desc = str(result.get("description", ""))
|
||||
if "parse" in desc.lower():
|
||||
# Log loudly: a parse failure means the template author (or
|
||||
# an asset field) is producing malformed HTML. Silent
|
||||
# fallback hides bugs and makes XSS-via-unescaped-field
|
||||
# harder to spot. Do not log the full payload — it may
|
||||
# contain secrets.
|
||||
_LOGGER.warning(
|
||||
"Telegram rejected parse_mode=%s (%r); retrying as plain text. "
|
||||
"Check template output for unescaped characters.",
|
||||
payload.get("parse_mode"), desc,
|
||||
)
|
||||
payload.pop("parse_mode", None)
|
||||
async with self._session.post(telegram_url, json=payload) as retry_resp:
|
||||
retry_result = await retry_resp.json()
|
||||
|
||||
@@ -321,6 +321,12 @@ def collect_scheduled_assets(
|
||||
asset_album_map: dict[str, tuple[str, str]] = {} # asset_id → (album_id, public_url)
|
||||
collections_extra: list[dict[str, Any]] = []
|
||||
|
||||
# limit=0 is the periodic-summary test path — the caller only needs
|
||||
# per-album stats (name/url/counts), not a sample of assets. Skip the
|
||||
# expensive ``filter_assets`` + sampling loop entirely; on a 50k-asset
|
||||
# album the serial scan-then-discard pattern wasted seconds per test.
|
||||
stats_only = limit <= 0
|
||||
|
||||
for album_id, album in albums.items():
|
||||
links = shared_links.get(album_id, [])
|
||||
album_public_url = get_public_url(external_url, links) or ""
|
||||
@@ -336,6 +342,9 @@ def collect_scheduled_assets(
|
||||
"owner": album.owner,
|
||||
})
|
||||
|
||||
if stats_only:
|
||||
continue
|
||||
|
||||
filtered = filter_assets(
|
||||
list(album.assets.values()),
|
||||
favorite_only=favorite_only,
|
||||
@@ -348,6 +357,9 @@ def collect_scheduled_assets(
|
||||
asset_album_map[asset.id] = (album_id, album_public_url)
|
||||
all_eligible.append(asset)
|
||||
|
||||
if stats_only:
|
||||
return [], collections_extra
|
||||
|
||||
# Random sample
|
||||
if len(all_eligible) > limit:
|
||||
selected = random.sample(all_eligible, limit)
|
||||
|
||||
@@ -3,14 +3,47 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import logging
|
||||
import re
|
||||
from typing import Any
|
||||
|
||||
import aiohttp
|
||||
|
||||
from ...notifications.ssrf import UnsafeURLError, validate_outbound_url
|
||||
from .models import ImmichAlbumData, SharedLinkInfo
|
||||
|
||||
_LOGGER = logging.getLogger(__name__)
|
||||
|
||||
# Cap user-controlled Immich search parameters so a low-privileged command
|
||||
# listener (e.g. an Immich ``/search`` command) cannot DoS the upstream.
|
||||
MAX_SEARCH_QUERY_LEN = 256
|
||||
MAX_SEARCH_PERSON_IDS = 50
|
||||
|
||||
# User-facing error bodies — Immich responses may leak internal paths,
|
||||
# hostnames, or headers injected by intermediary proxies. These helpers keep
|
||||
# only a short, scrubbed summary; full bodies are logged server-side only.
|
||||
_REDACTED_BODY_MAX = 120
|
||||
_SECRET_PATTERN = re.compile(
|
||||
r"(?i)(bearer\s+\S+|x-api-key[:=]\s*\S+|authorization[:=]\s*\S+|cookie[:=]\s*\S+|"
|
||||
r"password[:=]?\s*\S+|token[:=]?\s*[A-Za-z0-9._\-]+)"
|
||||
)
|
||||
|
||||
|
||||
def _redact_body(text: str) -> str:
|
||||
"""Return a short, credential-scrubbed snippet safe to surface to UI callers.
|
||||
|
||||
Immich error responses are admin-configurable (via reverse proxies, custom
|
||||
error pages) and may echo request headers or environment leak. Stripping
|
||||
anything that looks like a credential + capping length keeps us from
|
||||
persisting secrets into ``ActionExecution.error`` / ``EventLog.details``
|
||||
(both of which are returned through the dashboard API).
|
||||
"""
|
||||
if not text:
|
||||
return ""
|
||||
cleaned = _SECRET_PATTERN.sub("[redacted]", text)
|
||||
if len(cleaned) > _REDACTED_BODY_MAX:
|
||||
return cleaned[:_REDACTED_BODY_MAX] + "..."
|
||||
return cleaned
|
||||
|
||||
|
||||
class ImmichClient:
|
||||
"""Async client for the Immich API."""
|
||||
@@ -25,6 +58,18 @@ class ImmichClient:
|
||||
self._url = url.rstrip("/")
|
||||
self._api_key = api_key
|
||||
self._external_domain: str | None = None
|
||||
# SSRF guard — admin-set Immich URLs are loaded from provider config
|
||||
# which can be mutated via PATCH /api/providers or imported via
|
||||
# prepare-restore, so we revalidate at construction time rather than
|
||||
# trusting DB state. ``NOTIFY_BRIDGE_ALLOW_PRIVATE_URLS=1`` bypasses
|
||||
# for dev against localhost Immich.
|
||||
if self._url:
|
||||
try:
|
||||
validate_outbound_url(self._url)
|
||||
except UnsafeURLError as err:
|
||||
raise UnsafeURLError(
|
||||
f"Refusing to build ImmichClient for unsafe URL {self._url!r}: {err}"
|
||||
) from err
|
||||
|
||||
@property
|
||||
def url(self) -> str:
|
||||
@@ -36,6 +81,15 @@ class ImmichClient:
|
||||
|
||||
@external_domain.setter
|
||||
def external_domain(self, value: str | None) -> None:
|
||||
# Mirror the constructor's SSRF guard — external_domain is used to
|
||||
# build URLs that leak into rendered notifications, but any code path
|
||||
# that eventually fetches this URL would otherwise bypass the check.
|
||||
if value:
|
||||
try:
|
||||
validate_outbound_url(value)
|
||||
except UnsafeURLError as err:
|
||||
_LOGGER.warning("Ignoring unsafe external_domain %r: %s", value, err)
|
||||
return
|
||||
self._external_domain = value
|
||||
|
||||
@property
|
||||
@@ -237,9 +291,12 @@ class ImmichClient:
|
||||
limit: int = 10,
|
||||
page: int = 1,
|
||||
) -> list[dict[str, Any]]:
|
||||
payload: dict[str, Any] = {"query": query, "page": max(1, page), "size": limit}
|
||||
# Cap user-controlled inputs — a low-privileged Telegram listener can
|
||||
# craft arbitrarily long queries to DoS the upstream Immich.
|
||||
query = (query or "")[:MAX_SEARCH_QUERY_LEN]
|
||||
payload: dict[str, Any] = {"query": query, "page": max(1, page), "size": min(max(1, limit), 100)}
|
||||
if album_ids:
|
||||
payload["albumIds"] = album_ids
|
||||
payload["albumIds"] = album_ids[:MAX_SEARCH_PERSON_IDS]
|
||||
try:
|
||||
async with self._session.post(
|
||||
f"{self._url}/api/search/smart",
|
||||
@@ -261,9 +318,10 @@ class ImmichClient:
|
||||
limit: int = 10,
|
||||
page: int = 1,
|
||||
) -> list[dict[str, Any]]:
|
||||
payload: dict[str, Any] = {"originalFileName": query, "page": max(1, page), "size": limit}
|
||||
query = (query or "")[:MAX_SEARCH_QUERY_LEN]
|
||||
payload: dict[str, Any] = {"originalFileName": query, "page": max(1, page), "size": min(max(1, limit), 100)}
|
||||
if album_ids:
|
||||
payload["albumIds"] = album_ids
|
||||
payload["albumIds"] = album_ids[:MAX_SEARCH_PERSON_IDS]
|
||||
try:
|
||||
async with self._session.post(
|
||||
f"{self._url}/api/search/metadata",
|
||||
@@ -289,7 +347,7 @@ class ImmichClient:
|
||||
to return an empty list on current servers.
|
||||
"""
|
||||
payload: dict[str, Any] = {
|
||||
"personIds": [person_id],
|
||||
"personIds": [person_id][:MAX_SEARCH_PERSON_IDS],
|
||||
"page": 1,
|
||||
"size": max(1, min(limit, 100)),
|
||||
}
|
||||
@@ -373,9 +431,17 @@ class ImmichClient:
|
||||
if isinstance(parsed, dict):
|
||||
return parsed
|
||||
return {"raw": body_text}
|
||||
# Log full body server-side (for operators), surface only a
|
||||
# redacted snippet to the caller — this string ends up in
|
||||
# ActionExecution.error / EventLog.details which are returned
|
||||
# through the dashboard API.
|
||||
_LOGGER.warning(
|
||||
"add_assets_to_album failed: HTTP %s body=%s",
|
||||
response.status, body_text[:512],
|
||||
)
|
||||
raise ImmichApiError(
|
||||
f"Failed to add assets to album {album_id}: "
|
||||
f"HTTP {response.status} body={body_text[:512]}"
|
||||
f"HTTP {response.status} {_redact_body(body_text)}"
|
||||
)
|
||||
except aiohttp.ClientError as err:
|
||||
raise ImmichApiError(f"Error adding assets to album: {err}") from err
|
||||
@@ -399,9 +465,13 @@ class ImmichClient:
|
||||
if response.status in (200, 201, 204):
|
||||
return
|
||||
body_text = await response.text()
|
||||
_LOGGER.warning(
|
||||
"set_album_thumbnail failed: HTTP %s body=%s",
|
||||
response.status, body_text[:512],
|
||||
)
|
||||
raise ImmichApiError(
|
||||
f"Failed to set album thumbnail for {album_id}: "
|
||||
f"HTTP {response.status} body={body_text[:512]}"
|
||||
f"HTTP {response.status} {_redact_body(body_text)}"
|
||||
)
|
||||
except aiohttp.ClientError as err:
|
||||
raise ImmichApiError(f"Error setting album thumbnail: {err}") from err
|
||||
|
||||
@@ -2,16 +2,67 @@
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import logging
|
||||
from datetime import datetime
|
||||
from typing import Any
|
||||
|
||||
from notify_bridge_core.models.events import ServiceEvent
|
||||
|
||||
_LOGGER = logging.getLogger(__name__)
|
||||
|
||||
# Per-target maximum video size (bytes). None = no limit.
|
||||
_MAX_VIDEO_SIZE_BY_TARGET: dict[str, int] = {
|
||||
"telegram": 50 * 1024 * 1024, # 50 MB — Telegram Bot API hard limit
|
||||
}
|
||||
|
||||
# Keys that must NEVER flow into the Jinja2 template context, even if a
|
||||
# provider stuffs them into ``event.extra`` (webhooks, Immich metadata, etc.).
|
||||
# Templates that could reach a Telegram/Discord/etc. chat would otherwise
|
||||
# expose operator credentials if a template author simply did ``{{ api_key }}``.
|
||||
# Case-insensitive substring match — any ``extra`` key containing one of these
|
||||
# tokens is dropped before the merge.
|
||||
_SENSITIVE_EXTRA_TOKENS: tuple[str, ...] = (
|
||||
"api_key",
|
||||
"apikey",
|
||||
"token",
|
||||
"secret",
|
||||
"password",
|
||||
"passwd",
|
||||
"hashed_",
|
||||
"authorization",
|
||||
"cookie",
|
||||
"session_id",
|
||||
"bearer",
|
||||
"private_key",
|
||||
"access_key",
|
||||
)
|
||||
|
||||
|
||||
def _is_sensitive_key(key: str) -> bool:
|
||||
lowered = str(key).lower()
|
||||
return any(tok in lowered for tok in _SENSITIVE_EXTRA_TOKENS)
|
||||
|
||||
|
||||
def _safe_merge_extras(ctx: dict[str, Any], extras: dict[str, Any]) -> None:
|
||||
"""Merge provider ``extras`` into ``ctx``, dropping sensitive keys.
|
||||
|
||||
Dropped keys are logged once per event (DEBUG) so operators can spot
|
||||
leaking providers without flooding the log.
|
||||
"""
|
||||
if not extras:
|
||||
return
|
||||
dropped: list[str] = []
|
||||
for key, value in extras.items():
|
||||
if _is_sensitive_key(key):
|
||||
dropped.append(key)
|
||||
continue
|
||||
ctx[key] = value
|
||||
if dropped:
|
||||
_LOGGER.debug(
|
||||
"Dropped %d sensitive key(s) from template context: %s",
|
||||
len(dropped), ", ".join(sorted(dropped)),
|
||||
)
|
||||
|
||||
|
||||
def build_template_context(
|
||||
event: ServiceEvent,
|
||||
@@ -61,8 +112,9 @@ def build_template_context(
|
||||
"preview_url": asset.preview_url or "",
|
||||
"full_url": asset.full_url or "",
|
||||
}
|
||||
# Flatten extras into asset dict for template access
|
||||
asset_dict.update(asset.extra)
|
||||
# Flatten extras into asset dict for template access — same
|
||||
# sensitive-key filtering applied as the top-level merge.
|
||||
_safe_merge_extras(asset_dict, asset.extra)
|
||||
asset_dict.setdefault("oversized", False)
|
||||
asset_dict.setdefault("file_size", None)
|
||||
asset_dict.setdefault("playback_size", None)
|
||||
@@ -138,8 +190,11 @@ def build_template_context(
|
||||
if len(locations) == 1 and "" not in locations:
|
||||
ctx["common_location"] = locations.pop()
|
||||
|
||||
# Provider-specific extras merged at top level
|
||||
ctx.update(event.extra)
|
||||
# Provider-specific extras merged at top level. Sensitive keys (tokens,
|
||||
# secrets, auth headers) are dropped — see ``_SENSITIVE_EXTRA_TOKENS``.
|
||||
# Without this, a template author could exfiltrate provider credentials
|
||||
# via ``{{ api_key }}`` in an outgoing notification body.
|
||||
_safe_merge_extras(ctx, event.extra)
|
||||
|
||||
# Ensure URL variables always exist (avoid Jinja2 undefined errors)
|
||||
ctx.setdefault("public_url", "")
|
||||
|
||||
Reference in New Issue
Block a user