diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d3f9b691..e77584ba5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ All notable changes to Bambuddy will be documented in this file. - **Debug logs now record what the printer reports between the last layer and the end of a print (#2547, reporter @anthonyma94)** — The finish photo wants a moment that Bambu firmware does not obviously announce: printing done, toolhead parked, filament unload not yet started. Bambuddy has been driving that capture from `stg_cur=22` ("Filament unloading"), which turns out to fire on no model at all — across 247 support bundles there is not a single stage-22 capture, including the window in which it was the only trigger in the code, where all 104 captures on A1, A1 Mini, H2C, H2D, P1S, P2S, X1C and X2D fell through to the after-the-fact fallback. Choosing a replacement was not possible from the bundles we had, because outside `stg_cur` and `mc_print_sub_stage` every stage and action field the printers send is dropped unread, and the most promising candidates (`print_real_action`, `mc_action`, `mc_stage`) are absent from A1, A1 Mini and P1S payloads entirely. With debug logging enabled, Bambuddy now dumps those raw fields for the window between the last object layer and the end of the print — opening on the first end-of-print signal (last layer reached, progress at 99+, or no remaining time), logging only what changed frame to frame, and closing on the state transition — so a single debug bundle per model can show whether any firmware marks that moment. Diagnostics only: nothing reads these values, they are printer telemetry with nothing identifying in them, and at normal log levels the probe does no work at all. Covered by tests for the window boundaries, the frame budget and the guarantee that the probe cannot break status ingest. ### Fixed +- **Setting Spoolman options over the API with a true/false value returned a server error** — `PUT /settings/spoolman` accepts a free-form body, and sending the natural JSON form for a switch — `{"spoolman_enabled": true}` rather than `{"spoolman_enabled": "true"}` — came back as a 500 with nothing useful in it. The shipped UI always sends strings, so this only affected people driving Bambuddy from a script or a Home Assistant `rest_command`, which is exactly where a real boolean is the obvious thing to send. **Root cause.** Settings are stored as text and every reader compares them as text, but the submitted value went in untouched. Deciding whether Spoolman had just been switched on called a string operation on it, which a boolean does not have; and the raw boolean was also written straight to a text column, which SQLite quietly turns into 1/0 while PostgreSQL refuses it outright — so the stored result depended on which database the install used. **Fix.** Boolean-ish settings are now converted to a canonical `true`/`false` on the way in, accepting real booleans, `1`/`0`, and the usual spellings (`True`, `yes`, `on`) case-insensitively, since this is a documented API that scripts talk to. A value with no sensible reading, such as `"banana"`, now returns a 400 naming the field instead of being stored as-is and silently treated as off. Two details are preserved deliberately: a blank value still means "use the default" for the two options that default to on, and reading a stored value stays as strict as it has always been elsewhere in the codebase, so no existing row changes meaning. Text options are checked too, so a JSON object can no longer be stored as its own printed form. One incidental improvement: a value stored as `True` by an earlier API call showed as off in the UI, which compares case-sensitively, while the backend treated it as on — canonical storage removes that disagreement. Covered by tests across the accepted spellings, the rejected values, the blank-means-default behaviour, and the read path. - **External-camera timelapses and finish photos came out empty when the live view was open (#2707, reporter @bitbarista)** — On a printer with an external camera, watching the live view while a print ran meant the layer timelapse recorded almost nothing and the finish-photo notification went out with no image attached. The reporter measured zero of 87 layer captures on one print and zero of 105 on another, both watched from start to finish. A USB camera allows exactly one program to hold it open, so every capture taken during a live view failed outright rather than merely degrading. **Root cause.** Bambuddy already knew not to do this for the printer's built-in camera: a snapshot taken while somebody is watching reuses the viewer's frame instead of opening a second connection (#1348, #1271). That rule was never extended to the external-camera paths — and could not have been, because the buffered frame it depends on was only ever published by the built-in paths. The live external stream tracked when frames arrived but never kept one, and the stream hands out frames already wrapped for the browser, so there was nothing for a consumer to reuse. **Fix.** The external stream now publishes each frame as it goes past, and every one-shot consumer — layer timelapse, the finish photo and its fallback, the notification snapshot, Obico polling and the plate check — reuses that frame instead of opening a second handle. If a viewer is attached but no frame has arrived yet, that single attempt is skipped rather than competing, because kicking the viewer off is worse than missing one frame. Two things improve as a side effect: `/camera/snapshot` and the finish-photo fallback chain can now serve an external camera's live frame, where before they found an empty buffer, and the buffer is released when the last viewer of that printer leaves. Covered by tests for the frame plumbing, a buffering failure being unable to break the live stream, and each consumer in all three states — viewer with a frame, viewer without one, and nobody watching. - **A long-running camera stream could eventually stall itself, with nothing in the log to explain it (#2707)** — ffmpeg's error output was only read when something had already gone wrong, which meant that for the entire life of a working stream nobody read it. ffmpeg writes a startup banner, its analysis of the incoming video, and then a progress line at a steady rate, and the operating system only buffers a fixed amount of that before it stops the writer. Once that happened ffmpeg would block trying to write the next line, stop producing frames, and the stream's own timeout would fire — reported as `RTSP read timeout` and a reconnect, with no indication that Bambuddy had starved it. How long it takes to reach that point is unmeasured and clearly long: one stream ran 21 minutes 36 seconds without trouble, so this is a limit that was being ignored rather than a fault anyone has reported. **Fix.** A streaming ffmpeg's error output is now read continuously and the most recent portion kept, so the limit cannot be reached. The kept portion is what gets logged when a stream does fail, which is more useful than before: it holds what ffmpeg said as things went wrong, where reading the buffer on demand returned whatever it had printed first — usually the startup banner, which is then discarded as noise. Credentials are masked on this path through the same single funnel as every other camera log. Covered by tests for continuous draining, the size bound, keeping the newest output, credential masking, and the ownership handover with the shutdown path — reading the same output from two places at once is an error, so only one owner reads it at a time. - **Reopening the camera quickly could leave the new stream invisible to Bambuddy (#2707)** — Closing a camera view and opening it again straight away could leave the newly started stream unregistered, even though it was running and showing frames. The consequences were all indirect, which is what made it hard to spot: Bambuddy believed no viewer was attached, so Obico polling and snapshots would open a second camera connection and fight the live view — precisely what the guards added in #1348 and #1271 exist to prevent; the background cleanup task saw a camera process with no stream attached to it and killed the live stream as an orphan, usually within a minute; and pressing Stop reported that it had stopped nothing while the view was still running. **Root cause.** Each printer's fan-out stream was registered under a key derived from the printer alone, so every successive stream for that printer reused the same key, and the departing stream's cleanup removed whatever was registered under it — including its own replacement. The same cleanup also cleared the printer's most recent camera frame unconditionally, discarding the new stream's frame. It needed the old and new streams to overlap, which the four-second teardown fixed above made easy to hit. **Fix.** Each stream now gets its own registry key, so one stream can only ever clean up after itself — the same approach the external-camera path already uses (#2675) — and the shared per-printer frame is only released when no stream for that printer is left running. Covered by tests for both halves, including one that drives the real cleanup path with a second stream already registered. diff --git a/backend/app/api/routes/_oidc_helpers.py b/backend/app/api/routes/_oidc_helpers.py index ace57569e..184f621eb 100644 --- a/backend/app/api/routes/_oidc_helpers.py +++ b/backend/app/api/routes/_oidc_helpers.py @@ -1,9 +1,11 @@ """Pure helper functions for OIDC routes. -Hosts the SSRF guard for admin-supplied icon URLs. Stricter than -``_spoolman_helpers.assert_safe_spoolman_url`` — Spoolman intentionally allows -loopback/RFC-1918 (same-LAN topology) while OIDC icons must be reachable on -the public internet (IdP-hosted), so private addresses there are SSRF probes. +Hosts the public-internet SSRF guard, used for both admin-supplied icon URLs +and OIDC issuer URLs (via ``schemas.auth._validate_issuer_url``). Stricter +than ``_url_safety.assert_safe_lan_service_url`` — LAN services intentionally +allow loopback/RFC-1918 (same-host/same-LAN topology) while an IdP must be +reachable on the public internet, so a private address there is an SSRF probe +rather than a configuration. """ from __future__ import annotations @@ -17,9 +19,10 @@ from backend.app.api.routes._url_safety import CLOUD_METADATA_IPS, NUMERIC_IP_RE def assert_safe_public_https_url(url: str) -> None: """Raise ValueError if *url* is unsafe to fetch as a public HTTPS resource. - Used for OIDC provider icon URLs (#1333). Stricter than the Spoolman SSRF - guard: also rejects loopback, private (RFC-1918), and link-local addresses - because an OIDC icon legitimately lives only on the public internet. + Used for OIDC provider icon URLs (#1333) and OIDC issuer URLs. Stricter + than the LAN-service SSRF guard: also rejects loopback, private + (RFC-1918), and link-local addresses because an IdP and its icon + legitimately live only on the public internet. Checks performed: - Scheme must be ``https`` (no ``http://``, ``file://``, ``gopher://``, …). @@ -35,9 +38,10 @@ def assert_safe_public_https_url(url: str) -> None: - IPv4-mapped IPv6 (``::ffff:127.0.0.1``) — unwrapped before the IP-class check so an attacker can't bypass via IPv6 encoding. - Hostname-based addresses are accepted without DNS resolution (consistent - with ``_validate_issuer_url`` policy — the operator is trusted to - configure a sensible IdP host). + Hostname-based addresses are accepted without DNS resolution — the + operator is trusted to configure a sensible IdP host, and resolving here + would both add a TOCTOU gap (DNS can change between validation and + request) and make the validator issue network requests of its own. """ parsed = urlparse(url) if parsed.scheme.lower() != "https": diff --git a/backend/app/api/routes/_spoolman_helpers.py b/backend/app/api/routes/_spoolman_helpers.py index c1471e13f..cb95115e1 100644 --- a/backend/app/api/routes/_spoolman_helpers.py +++ b/backend/app/api/routes/_spoolman_helpers.py @@ -5,17 +5,15 @@ No heavy dependencies — importable in unit tests without the full backend stac from __future__ import annotations -import ipaddress import json import logging import math import re from typing import Any -from urllib.parse import urlparse from typing_extensions import TypedDict -from backend.app.api.routes._url_safety import CLOUD_METADATA_IPS, NUMERIC_IP_RE, unwrap_ipv4_mapped +from backend.app.api.routes._url_safety import assert_safe_lan_service_url logger = logging.getLogger(__name__) @@ -80,61 +78,17 @@ class NormalizedFilament(TypedDict): def assert_safe_spoolman_url(url: str) -> None: - """Raise ValueError if *url* should be blocked as an SSRF risk. + """Raise ValueError if the Spoolman *url* should be blocked as an SSRF risk. - Bambuddy is typically deployed on a home LAN alongside Spoolman, so - loopback (127.0.0.1) and RFC-1918 private ranges (192.168.x.x, 10.x.x.x, - 172.16-31.x) must be permitted — they are THE normal Spoolman topology. - This guard therefore targets the genuinely dangerous cases only. + Thin wrapper over the shared LAN-service policy — see + ``_url_safety.assert_safe_lan_service_url`` for what is and isn't + rejected, and why loopback/RFC-1918 are deliberately permitted (running + Spoolman on the same host or home LAN is THE normal topology). - Checks performed: - - Scheme must be http or https (no file://, gopher://, dict://, etc.). - - Numeric-encoded IP addresses in decimal (e.g. ``2130706433``) or hex - (e.g. ``0x7f000001``) are rejected. Python's ``ipaddress`` module raises - ``ValueError`` for these forms so they would otherwise bypass the - explicit-IP block below, but libc (and browsers) resolve them as valid - IPv4 addresses. - - Cloud provider metadata endpoints (169.254.169.254, 100.100.100.200, - fd00:ec2::254) are blocked — the classic SSRF credential-exfil target. - - Multicast (224.0.0.0/4, ff00::/8) and unspecified (0.0.0.0, ::) addresses - are blocked — pointless as a destination and suggests misuse. - - IPv4-mapped IPv6 addresses (::ffff:x.x.x.x) are unwrapped so they cannot - bypass the checks above. - - Hostname-based addresses ("localhost", "spoolman.lan", "internal.corp") - are out of scope — DNS resolution is deliberately not performed here. + Kept as a named function because the "Spoolman URL …" wording in its + errors is user-facing and asserted by existing tests. """ - parsed = urlparse(url) - if parsed.scheme.lower() not in ("http", "https"): - raise ValueError("Spoolman URL must use http or https") - - hostname = (parsed.hostname or "").lower() - - # Reject decimal- and hex-encoded IPs (e.g. http://2130706433/ or - # http://0x7f000001/). These slip past ipaddress.ip_address() but libc - # (and browsers) parse them as IPv4 — an obvious bypass if not caught. - if NUMERIC_IP_RE.match(hostname): - raise ValueError("Spoolman URL must not use numeric-encoded IP addresses; use standard dotted-decimal notation") - - try: - addr = ipaddress.ip_address(hostname) - except ValueError: - # Not a bare IP address — includes intentional cases such as "localhost" and - # RFC-1918 hostnames ("spoolman.lan", "192.168.1.10" would be caught above as - # a dotted-decimal IP; symbolic names resolve via DNS which is out of scope). - # Running Spoolman on the same host or home LAN is the standard Bambuddy - # topology, so loopback and private ranges are deliberately NOT blocked here. - return - - # Unwrap IPv4-mapped IPv6 (::ffff:169.254.169.254 etc.) so attackers can't - # encode a blocked IPv4 into an IPv6 literal to bypass the check. - effective = unwrap_ipv4_mapped(addr) - - if effective in CLOUD_METADATA_IPS: - raise ValueError("Spoolman URL must not point to a cloud metadata endpoint") - - if effective.is_multicast or effective.is_unspecified: - raise ValueError("Spoolman URL must not point to a multicast or unspecified address") + assert_safe_lan_service_url(url, label="Spoolman URL") _COLOR_HEX_RE = re.compile(r"^[0-9A-Fa-f]{6}$") diff --git a/backend/app/api/routes/_url_safety.py b/backend/app/api/routes/_url_safety.py index 2ee2f94cb..cccf2ff5a 100644 --- a/backend/app/api/routes/_url_safety.py +++ b/backend/app/api/routes/_url_safety.py @@ -1,19 +1,31 @@ -"""Shared URL-safety primitives used by both SSRF guards in this package. +"""Shared URL-safety primitives for the SSRF guards in this package. -The two top-level assertion functions — -``_spoolman_helpers.assert_safe_spoolman_url`` (Spoolman, deliberately allows -loopback/RFC-1918 because same-LAN deployment is the standard topology) and -``_oidc_helpers.assert_safe_public_https_url`` (OIDC icons, must be reachable -on the public internet, so loopback/private are rejected) — share the -*data* (cloud-metadata IP set, numeric-encoded-IP regex) but not the -*policy*. Only the data lives here. The functions stay in their respective -modules with their distinct policies intact. +Bambuddy has exactly two outbound-URL policies, and which one applies is a +property of the *service*, not of the caller: + +- **LAN-service** (``assert_safe_lan_service_url`` below) — the service + legitimately lives on the same host or home LAN, so loopback and RFC-1918 + must be permitted; blocking them would break the normal topology. Used for + Spoolman, self-hosted notification servers (ntfy, Bark, Gotify, custom + webhooks), Home Assistant, the Obico ML endpoint and the slicer sidecars. +- **Public-internet** (``_oidc_helpers.assert_safe_public_https_url``) — the + resource can only sensibly live on the public internet, so a private + address is an SSRF probe rather than a configuration. Used for OIDC issuer + and icon URLs. + +Both reject the cases that are dangerous regardless of topology: non-HTTP +schemes, numeric-encoded IPs, cloud-metadata endpoints, multicast and +unspecified addresses, and IPv4-mapped IPv6 encodings of any of the above. + +The LAN-service policy lives here because it now has several callers; the +public-internet policy stays in ``_oidc_helpers`` next to its only consumer. """ from __future__ import annotations import ipaddress import re +from urllib.parse import urlparse # Cloud-provider metadata endpoints — the classic SSRF credential-exfil # targets. Both guards reject these unconditionally. @@ -49,3 +61,58 @@ def unwrap_ipv4_mapped( if isinstance(addr, ipaddress.IPv6Address) and addr.ipv4_mapped is not None: return addr.ipv4_mapped return addr + + +def assert_safe_lan_service_url(url: str, *, label: str) -> None: + """Raise ValueError if *url* is unsafe for a service that may live on the LAN. + + ``label`` names the setting in the error message ("Spoolman URL", "ntfy + server URL", …) so the user sees which field they need to correct. + + Loopback (127.0.0.1) and RFC-1918 private ranges are deliberately + **permitted** — Bambuddy is self-hosted and running Spoolman, ntfy, + Bark, Home Assistant, an Obico ML endpoint or a slicer sidecar on the + same host or home LAN is THE normal topology, not an attack. A blanket + private-address block would break those integrations for most installs. + + What is rejected is dangerous under any topology: + + - Schemes other than http/https. ``httpx`` already raises + ``UnsupportedProtocol`` for ``file://``/``gopher://`` etc., so this is + about returning a clear validation error at configuration time rather + than an opaque failure at delivery time. + - Numeric-encoded IPv4 (decimal ``2130706433``, hex ``0x7f000001``) — + libc and browsers resolve these, but Python's ``ipaddress`` raises + ValueError on them, so they would slip past the checks below. + - Cloud-provider metadata endpoints — the high-value SSRF target, and + never a legitimate destination for any of these services. + - Multicast and unspecified addresses — pointless as a destination and + indicative of misuse. + - IPv4-mapped IPv6 encodings of any of the above. + + Symbolic hostnames are accepted without DNS resolution, matching the + public-internet guard: resolution here would be both a TOCTOU (DNS can + change between validation and request) and a request the validator + shouldn't be making. + """ + parsed = urlparse(url) + if parsed.scheme.lower() not in ("http", "https"): + raise ValueError(f"{label} must use http or https") + + hostname = (parsed.hostname or "").lower() + + if NUMERIC_IP_RE.match(hostname): + raise ValueError(f"{label} must not use numeric-encoded IP addresses; use standard dotted-decimal notation") + + try: + addr = ipaddress.ip_address(hostname) + except ValueError: + return # symbolic hostname — out of scope by design (no DNS check) + + effective = unwrap_ipv4_mapped(addr) + + if effective in CLOUD_METADATA_IPS: + raise ValueError(f"{label} must not point to a cloud metadata endpoint") + + if effective.is_multicast or effective.is_unspecified: + raise ValueError(f"{label} must not point to a multicast or unspecified address") diff --git a/backend/app/api/routes/settings.py b/backend/app/api/routes/settings.py index 3f6acbd96..a89ab21e7 100644 --- a/backend/app/api/routes/settings.py +++ b/backend/app/api/routes/settings.py @@ -42,6 +42,88 @@ async def get_setting(db: AsyncSession, key: str) -> str | None: return setting.value if setting else None +# Accepted spellings for a boolean settings value. Settings live in a VARCHAR +# column and every reader compares them as strings, so these are normalised to +# "true"/"false" on the way in. The sets are deliberately generous: these +# endpoints are part of the documented REST surface, reached by scripts and by +# Home Assistant rest_command, where "True", "1" and "on" are all natural. +_TRUTHY_SETTING_VALUES = frozenset({"true", "1", "yes", "on"}) +_FALSY_SETTING_VALUES = frozenset({"false", "0", "no", "off"}) + + +def setting_is_true(value: object) -> bool: + """Return True if a *stored* settings value means "on". + + Deliberately narrower than the spellings ``normalize_bool_setting`` accepts: + it matches only what every other reader in the codebase treats as on + (``value.lower() == "true"``). Submitted values are canonicalised on write, + so a stored value is always "true"/"false"/""; accepting "1" or "on" here + would make this function disagree with the rest of the app about any legacy + row containing them. + + A bool is tolerated for the case of a row written before values were + normalised, where SQLite coerced a raw bool into the VARCHAR column. + """ + if isinstance(value, bool): + return value + if value is None: + return False + return str(value).strip().lower() == "true" + + +def normalize_bool_setting(key: str, value: object) -> str: + """Coerce a boolean-ish settings value to the canonical "true"/"false". + + Raises HTTPException(400) for values with no sensible interpretation, so an + API client gets a message naming the field instead of a 500. + + A JSON boolean is the natural thing for an API client to send, and before + this normalisation it caused two distinct failures on + ``PUT /settings/spoolman``: ``bool.lower()`` raised AttributeError, and the + raw bool was written into a VARCHAR column, which SQLite silently coerces + to 1/0 while asyncpg rejects outright. Both surfaced as an opaque 500. + """ + if isinstance(value, bool): # must precede the int branch — bool is an int + return "true" if value else "false" + if isinstance(value, int): + if value in (0, 1): + return "true" if value else "false" + raise HTTPException(400, f"{key} must be a boolean; got the number {value}") + if isinstance(value, str): + candidate = value.strip().lower() + if not candidate: + # Empty is stored verbatim rather than normalised to "false". + # get_spoolman_settings reads these with ``or ""``, so an + # empty stored value means "use the default" — and two of them + # (spoolman_report_partial_usage, auto_add_unknown_rfid) default to + # ON. Rewriting "" to "false" would silently switch them off for any + # client that submits a blank value. + return "" + if candidate in _TRUTHY_SETTING_VALUES: + return "true" + if candidate in _FALSY_SETTING_VALUES: + return "false" + raise HTTPException(400, f"{key} must be a boolean; got {value!r}") + raise HTTPException(400, f"{key} must be a boolean; got {type(value).__name__}") + + +def normalize_str_setting(key: str, value: object) -> str: + """Return a string settings value, rejecting types that would store garbage. + + ``str()`` on a dict or list would persist its repr, so those are refused + rather than silently written. Numbers are accepted and stringified: a port + or a bare host submitted unquoted is a plausible client mistake, not a + reason to fail the request. + """ + if isinstance(value, str): + return value + if value is None: + return "" + if isinstance(value, bool | int | float): + return str(value) + raise HTTPException(400, f"{key} must be a string; got {type(value).__name__}") + + async def get_external_login_url(db: AsyncSession) -> str: """Get the external URL for the login page. @@ -435,14 +517,20 @@ async def update_spoolman_settings( db: AsyncSession = Depends(get_db), _: User | None = RequirePermissionIfAuthEnabled(Permission.SETTINGS_UPDATE), ): - """Update Spoolman integration settings.""" + """Update Spoolman integration settings. + + The body is a free-form dict rather than a schema, so each value is + normalised before it is persisted — see ``normalize_bool_setting`` for why + a JSON boolean used to produce a 500 here. + """ if "spoolman_enabled" in settings: - old_val = await get_setting(db, "spoolman_enabled") or "false" - new_val = settings["spoolman_enabled"] + was_enabled = setting_is_true(await get_setting(db, "spoolman_enabled")) + new_val = normalize_bool_setting("spoolman_enabled", settings["spoolman_enabled"]) + now_enabled = new_val == "true" await set_setting(db, "spoolman_enabled", new_val) # Switching to Spoolman: clear built-in inventory slot assignments - if old_val.lower() != "true" and new_val.lower() == "true": + if not was_enabled and now_enabled: from backend.app.models.spool_assignment import SpoolAssignment result = await db.execute(delete(SpoolAssignment)) @@ -452,21 +540,20 @@ async def update_spoolman_settings( # spoolman_slot_assignments rows linger and would wrongly count as # "assigned" in any mode-agnostic check (e.g. the missing-spool- # assignment notification, which unions both tables — #1473). - elif old_val.lower() == "true" and new_val.lower() != "true": + elif was_enabled and not now_enabled: from backend.app.models.spoolman_slot_assignment import SpoolmanSlotAssignment result = await db.execute(delete(SpoolmanSlotAssignment)) logger.info("Cleared %d Spoolman slot assignments on switch to internal mode", result.rowcount) if "spoolman_url" in settings: - await set_setting(db, "spoolman_url", settings["spoolman_url"]) + await set_setting(db, "spoolman_url", normalize_str_setting("spoolman_url", settings["spoolman_url"])) if "spoolman_sync_mode" in settings: - await set_setting(db, "spoolman_sync_mode", settings["spoolman_sync_mode"]) - if "spoolman_disable_weight_sync" in settings: - await set_setting(db, "spoolman_disable_weight_sync", settings["spoolman_disable_weight_sync"]) - if "spoolman_report_partial_usage" in settings: - await set_setting(db, "spoolman_report_partial_usage", settings["spoolman_report_partial_usage"]) - if "auto_add_unknown_rfid" in settings: - await set_setting(db, "auto_add_unknown_rfid", settings["auto_add_unknown_rfid"]) + await set_setting( + db, "spoolman_sync_mode", normalize_str_setting("spoolman_sync_mode", settings["spoolman_sync_mode"]) + ) + for bool_key in ("spoolman_disable_weight_sync", "spoolman_report_partial_usage", "auto_add_unknown_rfid"): + if bool_key in settings: + await set_setting(db, bool_key, normalize_bool_setting(bool_key, settings[bool_key])) spoolman_changed = "spoolman_enabled" in settings or "spoolman_url" in settings diff --git a/backend/app/schemas/auth.py b/backend/app/schemas/auth.py index 597e03a1f..3d6b936ae 100644 --- a/backend/app/schemas/auth.py +++ b/backend/app/schemas/auth.py @@ -360,28 +360,37 @@ def _validate_icon_url(v: str | None) -> str | None: def _validate_issuer_url(v: str | None) -> str | None: - """Nit4: Reject non-HTTPS issuer URLs and private/loopback/link-local hosts. + """Reject non-HTTPS issuer URLs and SSRF-unsafe hosts. - HTTP is no longer accepted — OIDC providers must be reachable over TLS. - Private-network and loopback addresses are rejected to prevent SSRF attacks - where an admin-supplied URL could reach internal services. + An OIDC provider must be reachable over TLS on the public internet, so + this uses the public-internet policy: private, loopback and link-local + addresses are all rejected. + + Delegates to the runtime guard ``assert_safe_public_https_url`` for the + same reason ``_validate_icon_url`` does — no policy drift between the + schema layer and the fetcher. The hand-rolled version this replaced + checked only ``is_private | is_loopback | is_link_local``, which left + numeric-encoded IPs (``https://2130706433/``), IPv4-mapped IPv6 + (``https://[::ffff:127.0.0.1]/``), multicast and unspecified addresses + able to express a target the policy meant to forbid. The guard's + docstring already claimed the two were consistent; now they are. + + Lazy-imported because ``_oidc_helpers`` lives under ``api/routes/`` and + schemas avoid top-level imports from that layer. """ - import ipaddress - from urllib.parse import urlparse - if v is None: return v if not v.startswith("https://"): raise ValueError("issuer_url must start with https://") - host = urlparse(v).hostname or "" + from backend.app.api.routes._oidc_helpers import assert_safe_public_https_url + try: - addr = ipaddress.ip_address(host) - if addr.is_private or addr.is_loopback or addr.is_link_local: - raise ValueError("issuer_url must not point to a private, loopback, or link-local address") + assert_safe_public_https_url(v) except ValueError as exc: - if "issuer_url" in str(exc): - raise - # hostname is a domain name, not a bare IP — that's fine + # The guard's messages say "icon URL" — rewrite for this field so the + # user sees the setting they actually submitted. + detail = str(exc).replace("icon URL", "issuer_url") + raise ValueError(detail) from exc return v diff --git a/backend/app/schemas/settings.py b/backend/app/schemas/settings.py index 428c96dad..8f7c488d1 100644 --- a/backend/app/schemas/settings.py +++ b/backend/app/schemas/settings.py @@ -1,9 +1,23 @@ import json -from pydantic import BaseModel, Field, field_validator +from pydantic import BaseModel, Field, ValidationInfo, field_validator from backend.app.schemas.print_queue import TriState +# Outbound service URLs validated on save, so a bad value is rejected at +# configuration time with a clear message rather than failing opaquely at +# request time. Every one of these services is commonly self-hosted on the same +# host or LAN as Bambuddy, so the LAN-service policy applies: loopback and +# RFC-1918 stay permitted, while cloud-metadata endpoints, numeric-encoded IPs, +# IPv4-mapped IPv6 and non-HTTP schemes are rejected. See +# ``_url_safety.assert_safe_lan_service_url``. +# +# Module-level rather than a class attribute so the CI backstop in +# tests/unit/test_outbound_url_ssrf_guards.py can import the real list and +# cannot drift from it. Any new outbound-URL setting belongs here (or, if it +# must be reachable on the public internet, on the stricter OIDC guard). +LAN_SERVICE_URL_SETTINGS = ("ha_url", "obico_ml_url", "orcaslicer_api_url", "bambu_studio_api_url") + class AppSettings(BaseModel): """Application settings schema.""" @@ -600,6 +614,47 @@ class AppSettingsUpdate(BaseModel): default_sidebar_order: str | None = None forecast_global_lead_time_days: int | None = Field(default=None, ge=0) + @field_validator(*LAN_SERVICE_URL_SETTINGS) + @classmethod + def validate_lan_service_url(cls, v: str | None, info: ValidationInfo) -> str | None: + """Reject SSRF-unsafe outbound service URLs on save. + + Empty (and whitespace-only) is the documented "not configured / fall + back to the env var" value for all four fields and must keep passing. + + Values that are not absolute URLs at all ("192.168.1.10:3333", + "localhost:3333") are left alone rather than rejected. Two reasons: + + - They are inert. Every consumer of these four settings goes through + httpx, which raises UnsupportedProtocol for a URL with no scheme, so + no request is ever issued and there is nothing to guard against. + - They were storable before this validator existed, and the settings + UI is a plain text input with no scheme enforcement. Newly rejecting + them would break saves that have nothing to do with the URL: the + Obico panel, for one, sends obico_ml_url with every change and + auto-saves, so one legacy value would block toggling detection on or + off. A pre-existing misconfiguration should keep failing where it + already failed (at request time), not spread to unrelated fields. + + ``urlparse`` is no help in telling the two apart — it reads + "localhost:3333" as scheme "localhost" — so the test is the literal + "://" that makes a string an absolute URL. + """ + if v is None or not v.strip(): + return v + candidate = v.strip() + if "://" not in candidate: + return v + # Lazy-imported: schemas avoid top-level imports from api/routes, + # matching the existing pattern in auth.py's _validate_icon_url. + from backend.app.api.routes._url_safety import assert_safe_lan_service_url + + try: + assert_safe_lan_service_url(candidate, label=info.field_name or "URL") + except ValueError as exc: + raise ValueError(str(exc)) from exc + return v + @field_validator("gcode_snippets") @classmethod def validate_gcode_snippets(cls, v: str | None) -> str | None: diff --git a/backend/app/services/notification_service.py b/backend/app/services/notification_service.py index 9ddc90e73..8cf8eef79 100644 --- a/backend/app/services/notification_service.py +++ b/backend/app/services/notification_service.py @@ -64,6 +64,55 @@ def _looks_like_cloudflare_challenge(response: httpx.Response) -> bool: return "just a moment" in body or "cf-chl-bypass" in body or "cf-chl-opt" in body or "challenge-platform" in body +def _assert_safe_provider_url(url: str, *, label: str) -> str | None: + """Validate a provider URL taken from user-supplied config. + + Returns an error message on rejection, or None when the URL is + acceptable — the ``_send_*`` methods return ``tuple[bool, str]`` rather + than raising, so a message is more useful here than an exception. + + Uses the LAN-service policy: self-hosting ntfy, Bark, Gotify or a webhook + receiver on the home LAN is normal and must keep working, so loopback and + RFC-1918 stay permitted. Cloud-metadata endpoints, numeric-encoded IPs and + non-HTTP schemes are rejected. + """ + from backend.app.api.routes._url_safety import assert_safe_lan_service_url + + try: + assert_safe_lan_service_url(url, label=label) + except ValueError as exc: + return str(exc) + return None + + +def _opaque_http_failure(response: httpx.Response, *, label: str) -> str: + """Failure message for a provider whose destination host the user supplies. + + The response body is deliberately **not** returned to the caller. Provider + URLs are configurable by anyone holding ``NOTIFICATIONS_CREATE`` — which + the default Operators group carries and which does not imply + ``SETTINGS_UPDATE`` — and ``POST /notifications/test-config`` accepts a URL + straight from the request body without persisting anything. Echoing the + response body there turned an intended "does my webhook work?" check into + an authenticated read primitive against any host the Bambuddy process can + reach, including services that are not exposed to the network at all. + + Providers whose host Bambuddy hardcodes (Pushover, Telegram, CallMeBot) + keep returning the upstream body — there is no trust boundary to cross + when the destination cannot be influenced. + + The body is logged at debug level, where it stays available to whoever + already administers the host without being handed back over the API. + """ + logger.debug( + "%s delivery failed with HTTP %s; body: %s", + label, + response.status_code, + (response.text or "")[:200], + ) + return f"HTTP {response.status_code} from the configured {label} (see server logs at debug level for details)" + + class NotificationService: """Service for sending notifications through various providers.""" @@ -265,6 +314,10 @@ class NotificationService: if not device_key: return False, "Device key is required" + url_error = _assert_safe_provider_url(server, label="Bark server URL") + if url_error: + return False, url_error + payload: dict[str, Any] = { "device_key": device_key, "title": title, @@ -291,9 +344,13 @@ class NotificationService: except ValueError: body = None if isinstance(body, dict) and body.get("code") not in (200, None): - return False, f"Bark error {body.get('code')}: {str(body.get('message'))[:200]}" + # Only the numeric code is echoed. A server chosen by the caller + # controls this body too, so the free-text message is a (narrow) + # read channel of the same kind _opaque_http_failure closes. + logger.debug("Bark reported error %s: %s", body.get("code"), str(body.get("message"))[:200]) + return False, f"Bark error {body.get('code')} (see server logs at debug level for details)" return True, "Message sent successfully" - return False, f"HTTP {response.status_code}: {response.text[:200]}" + return False, _opaque_http_failure(response, label="Bark server") async def _send_ntfy( self, @@ -311,6 +368,10 @@ class NotificationService: if not topic: return False, "Topic is required" + url_error = _assert_safe_provider_url(server, label="ntfy server URL") + if url_error: + return False, url_error + url = f"{server}/{topic}" # ntfy reads Title/Message from HTTP headers. httpx enforces ASCII # for str header values, but printer names and filenames can contain @@ -363,7 +424,7 @@ class NotificationService: "Fight Mode, or front the server with Cloudflare Access using a " "service token. (#1534)" ) - return False, f"HTTP {response.status_code}: {response.text[:200]}" + return False, _opaque_http_failure(response, label="ntfy server") async def _send_pushover( self, config: dict, title: str, message: str, image_data: bytes | None = None @@ -683,6 +744,10 @@ class NotificationService: if not webhook_url: return False, "Webhook URL is required" + url_error = _assert_safe_provider_url(webhook_url, label="Webhook URL") + if url_error: + return False, url_error + # Build payload based on format if payload_format == "slack": # Slack/Mattermost format - just text field @@ -728,7 +793,7 @@ class NotificationService: if response.status_code in (200, 201, 202, 204): return True, "Webhook delivered successfully" else: - return False, f"HTTP {response.status_code}: {response.text[:200]}" + return False, _opaque_http_failure(response, label="webhook endpoint") except Exception as e: return False, f"Webhook error: {str(e)}" @@ -829,7 +894,11 @@ class NotificationService: elif response.status_code == 401: return False, "Home Assistant authentication failed - check your token" else: - return False, f"HTTP {response.status_code}: {response.text[:200]}" + # ha_url comes from global settings (SETTINGS_UPDATE, admin-only), so + # this is a narrower channel than the per-request provider URLs — but + # it lands in the same NOTIFICATIONS_CREATE-gated test response, so it + # gets the same treatment. + return False, _opaque_http_failure(response, label="Home Assistant endpoint") async def _send_to_provider( self, diff --git a/backend/tests/unit/services/test_notification_service.py b/backend/tests/unit/services/test_notification_service.py index 600300243..1d784cb1f 100644 --- a/backend/tests/unit/services/test_notification_service.py +++ b/backend/tests/unit/services/test_notification_service.py @@ -1168,16 +1168,24 @@ class TestBarkProvider: mock_client.post.assert_not_called() @pytest.mark.asyncio - async def test_send_bark_error_in_200_body(self, service): - """bark-server can wrap a failure in HTTP 200; the body code must win.""" + async def test_send_bark_error_in_200_body(self, service, caplog): + """bark-server can wrap a failure in HTTP 200; the body code must win. + + Only the numeric code is returned — the server is caller-supplied + (bark is self-hostable), so its free-text message is the same read + channel the HTTP-failure path closes. The text goes to the debug log. + """ mock_client = self._client_returning(200, {"code": 400, "message": "device token invalid"}) with patch.object(service, "_get_client", new_callable=AsyncMock) as mock_get_client: mock_get_client.return_value = mock_client - success, message = await service._send_bark({"device_key": "bad"}, "Title", "Body") + with caplog.at_level("DEBUG", logger="backend.app.services.notification_service"): + success, message = await service._send_bark({"device_key": "bad"}, "Title", "Body") assert success is False - assert "device token invalid" in message + assert "Bark error 400" in message + assert "device token invalid" not in message + assert "device token invalid" in caplog.text @pytest.mark.asyncio async def test_send_bark_http_error(self, service): @@ -2476,10 +2484,15 @@ class TestNtfyOutbound: assert " httpx.Response: + return httpx.Response(status_code=status, text=body, request=httpx.Request("POST", "http://10.0.0.1/")) + + +SECRET_BODY = "root:x:0:0:root:/root:/bin/bash" + + +def test_opaque_failure_does_not_return_the_response_body(): + message = ns._opaque_http_failure(_response(), label="webhook endpoint") + + assert SECRET_BODY not in message + assert "500" in message, "the status code is still useful and is not sensitive" + assert "webhook endpoint" in message + + +def test_opaque_failure_logs_the_body_for_the_operator(caplog): + """The body stays available to whoever administers the host — via logs, + not via the API response.""" + with caplog.at_level("DEBUG", logger=ns.__name__): + ns._opaque_http_failure(_response(), label="ntfy server") + + assert SECRET_BODY in caplog.text + + +@pytest.mark.parametrize( + "provider_label", + ["ntfy server", "Bark server", "webhook endpoint", "Home Assistant endpoint"], +) +def test_user_supplied_host_providers_use_the_opaque_path(provider_label: str): + """Guards the mapping itself: each user-supplied-host provider must route + its HTTP failure through _opaque_http_failure rather than formatting the + body inline.""" + src = inspect.getsource(ns) + assert f'_opaque_http_failure(response, label="{provider_label}")' in src + + +def test_no_user_supplied_host_provider_formats_the_body_inline(): + """Any remaining ``response.text[:200]`` must belong to a host-pinned provider. + + Pushover/Telegram/CallMeBot/Discord all target hardcoded hosts (Discord via + a webhook-prefix allowlist), so there is no trust boundary to cross. + """ + src = inspect.getsource(ns).split("\n") + host_pinned = {"_send_callmebot", "_send_pushover", "_send_telegram", "_send_discord"} + + current = None + offenders = [] + for line in src: + match = re.match(r"\s+async def (_send_\w+)", line) + if match: + current = match.group(1) + if "response.text[:200]" in line and current not in host_pinned: + offenders.append(current) + + assert not offenders, ( + f"{offenders} echo the upstream response body but do not target a " + f"hardcoded host. Route the failure through _opaque_http_failure." + ) + + +@pytest.mark.parametrize("url", UNIVERSALLY_BLOCKED) +def test_provider_url_guard_rejects_dangerous_targets(url: str): + assert ns._assert_safe_provider_url(url, label="Webhook URL") is not None + + +@pytest.mark.parametrize("url", LAN_ALLOWED) +def test_provider_url_guard_permits_self_hosted_servers(url: str): + assert ns._assert_safe_provider_url(url, label="ntfy server URL") is None + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("provider_type", "config"), + [ + ("ntfy", {"server": "http://169.254.169.254", "topic": "t"}), + ("bark", {"server": "http://169.254.169.254", "device_key": "k"}), + ("webhook", {"webhook_url": "http://169.254.169.254/latest/meta-data/"}), + ], +) +async def test_test_config_refuses_metadata_targets_without_a_request(provider_type: str, config: dict, monkeypatch): + """The end-to-end shape of the reported attack: an unsaved config aimed at + IMDS via the test endpoint. It must be refused before any HTTP call.""" + called = False + + async def _fail_if_called(*_a, **_kw): + nonlocal called + called = True + raise AssertionError("outbound request should not have been attempted") + + service = ns.NotificationService() + monkeypatch.setattr(service, "_get_client", _fail_if_called) + + success, message = await service.send_test_notification(provider_type, config) + + assert success is False + assert called is False + assert "cloud metadata" in message diff --git a/backend/tests/unit/test_spoolman_settings_value_coercion.py b/backend/tests/unit/test_spoolman_settings_value_coercion.py new file mode 100644 index 000000000..503c50502 --- /dev/null +++ b/backend/tests/unit/test_spoolman_settings_value_coercion.py @@ -0,0 +1,163 @@ +"""PUT /settings/spoolman must not 500 on a JSON boolean. + +The endpoint takes a free-form ``dict`` body, and settings are persisted in a +VARCHAR column that every reader compares as a string. Sending the natural JSON +form — ``{"spoolman_enabled": true}`` — used to fail twice over: + +- ``bool.lower()`` raised AttributeError while deciding whether the mode had + changed, surfacing as an opaque 500; +- the raw bool was handed to ``upsert_setting``, which SQLite silently coerces + to 1/0 while asyncpg rejects it — so the stored representation depended on + the deployment's database. + +The shipped UI sends strings, so this was reachable only through the REST API +(scripts, Home Assistant ``rest_command``) — which is exactly where a JSON +boolean is the obvious thing to send. + +These tests cover the normalisers directly. They are pure functions, so the +matrix stays readable and the endpoint keeps a single code path per field. +""" + +from __future__ import annotations + +import pytest +from fastapi import HTTPException + +from backend.app.api.routes.settings import ( + normalize_bool_setting, + normalize_str_setting, + setting_is_true, +) + +# --------------------------------------------------------------------------- +# The reported crash +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize(("value", "expected"), [(True, "true"), (False, "false")]) +def test_json_booleans_are_accepted_and_canonicalised(value: bool, expected: str): + """The exact input that used to 500.""" + assert normalize_bool_setting("spoolman_enabled", value) == expected + + +@pytest.mark.parametrize(("value", "expected"), [(1, "true"), (0, "false")]) +def test_json_numbers_one_and_zero_are_accepted(value: int, expected: str): + assert normalize_bool_setting("spoolman_enabled", value) == expected + + +# --------------------------------------------------------------------------- +# String spellings — generous on purpose, this is a documented REST surface +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("value", ["true", "TRUE", "True", " true ", "1", "yes", "on", "ON"]) +def test_truthy_spellings(value: str): + assert normalize_bool_setting("auto_add_unknown_rfid", value) == "true" + + +@pytest.mark.parametrize("value", ["false", "FALSE", "False", " false ", "0", "no", "off"]) +def test_falsy_spellings(value: str): + assert normalize_bool_setting("auto_add_unknown_rfid", value) == "false" + + +def test_python_style_capitalised_true_is_normalised_lowercase(): + """The frontend compares with a case-sensitive ``=== 'true'``. + + A client sending "True" previously had it stored verbatim, so the UI + rendered the setting as OFF while every backend reader (which all use + ``.lower()``) treated it as ON. + """ + assert normalize_bool_setting("spoolman_enabled", "True") == "true" + + +# --------------------------------------------------------------------------- +# Empty means "use the default" — deliberately NOT normalised to "false" +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("value", ["", " "]) +def test_empty_is_preserved_not_turned_into_false(value: str): + """get_spoolman_settings reads these with ``or ""``. + + spoolman_report_partial_usage and auto_add_unknown_rfid default to ON, so + coercing a blank submission to "false" would silently switch them off. + Whitespace-only collapses to "" so it takes the same path rather than + being stored as a truthy-but-meaningless " ". + """ + assert normalize_bool_setting("spoolman_report_partial_usage", value) == "" + + +# --------------------------------------------------------------------------- +# Values with no sensible reading get a 400 naming the field, not a 500 +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("value", ["banana", "maybe", "2", "-1", None, [], {}, 3.5, 7]) +def test_uninterpretable_values_raise_400_naming_the_field(value: object): + with pytest.raises(HTTPException) as exc: + normalize_bool_setting("spoolman_enabled", value) + + assert exc.value.status_code == 400 + assert "spoolman_enabled" in str(exc.value.detail) + + +# --------------------------------------------------------------------------- +# String settings +# --------------------------------------------------------------------------- + + +def test_str_setting_passes_strings_through_untouched(): + assert normalize_str_setting("spoolman_url", "http://192.168.1.5:7912/") == "http://192.168.1.5:7912/" + + +def test_str_setting_stringifies_numbers(): + """An unquoted host or port is a plausible client slip, not a hard error.""" + assert normalize_str_setting("spoolman_url", 7912) == "7912" + + +def test_str_setting_maps_null_to_empty(): + assert normalize_str_setting("spoolman_url", None) == "" + + +@pytest.mark.parametrize("value", [{"a": 1}, ["x"]]) +def test_str_setting_refuses_containers_rather_than_storing_a_repr(value: object): + with pytest.raises(HTTPException) as exc: + normalize_str_setting("spoolman_url", value) + + assert exc.value.status_code == 400 + + +# --------------------------------------------------------------------------- +# setting_is_true — used for the mode-switch comparison +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("stored", "expected"), + [ + ("true", True), + ("True", True), + ("TRUE", True), + (" true ", True), + ("false", False), + ("", False), + ("banana", False), + (None, False), # setting absent from the table + (True, True), # legacy row: SQLite coerced a raw bool into the column + (False, False), + ], +) +def test_setting_is_true(stored: object, expected: bool): + assert setting_is_true(stored) is expected + + +@pytest.mark.parametrize("stored", ["1", "on", "yes"]) +def test_setting_is_true_stays_narrower_than_the_write_path(stored: str): + """Reading must agree with the rest of the codebase, which only accepts "true". + + normalize_bool_setting is generous about what clients may *send*; every + reader (spoolman_tracking, filament_deficit, inventory, spoolbuddy, labels, + main) compares ``.lower() == "true"``. Accepting more here would make the + mode-switch check disagree with them about a legacy row. + """ + assert setting_is_true(stored) is False