mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-10-21 10:15:04 +02:00
fix(notifications): honest UA + Cloudflare-challenge detection on ntfy (#1534)
The notification service's httpx client was the only outbound client in the codebase still leaking python-httpx/<version> as User-Agent; all other clients identify as Bambuddy/1.0 since the May 2026 compliance pass. Bring it in line. The reporter's ntfy server was behind a Cloudflare Tunnel and CF returned its JS challenge page (Just a moment...) to every API request — confirmed by reproducing the same 403 with curl. Cloudflare can't be solved from a backend, so add detection for the challenge shape (Server: cloudflare or cf-mitigated header, or <!DOCTYPE html>...Just a moment... body) and return an actionable error message that points at the real fix on the user's CF side instead of dumping the raw HTML. Normal 403s (auth failures with plain text bodies) still surface the original body so genuine errors stay debuggable.
This commit is contained in:
maziggy
@@ -29,6 +29,7 @@ All notable changes to Bambuddy will be documented in this file.
|
||||
- **Trivy DS-0026 (`Dockerfile.test` missing HEALTHCHECK): silenced via `HEALTHCHECK NONE`** — The test image runs `pytest` and exits; there is no long-running service to probe, so any HEALTHCHECK we added would be cargo-cult noise. `HEALTHCHECK NONE` is the documented Docker directive to explicitly opt out of any inherited healthcheck and is the way Trivy expects projects to signal "this image is not a service." Closes code-scanning alert #813.
|
||||
|
||||
### Fixed
|
||||
- **ntfy notifications: honest User-Agent + actionable error when the server is behind a Cloudflare challenge (#1534, reported by @apizz)** — Reporter pointed an ntfy server behind a Cloudflare Tunnel at Bambuddy and got `HTTP 403: <!DOCTYPE html>...Just a moment...` on every Test click. They reproduced the same response with a plain `curl -H "Authorization: Bearer <token>" -d "test" https://ntfy.example/<topic>` — confirming the 403 originates from Cloudflare's JS challenge intercept (Bot Fight Mode / "Under Attack" mode), not from Bambuddy or ntfy. Cloudflare returns its interstitial HTML to any non-browser client at the edge, so the request never reaches the user's ntfy backend at all. Bambuddy can't solve a JS challenge from a backend — the only real fix is on the user's Cloudflare side (a security-skip rule for the hostname/path, disabling Bot Fight Mode for that hostname, or fronting the server with Cloudflare Access using a service token). Two improvements shipped to make this footgun self-diagnosable for the next user who hits it. **(1) Honest User-Agent on the notification HTTP client.** `backend/app/services/notification_service.py` was the one outbound httpx client in the codebase that didn't set the project-standard `Bambuddy/1.0 (+https://github.com/maziggy/bambuddy)` UA — it leaked `python-httpx/<version>` instead. Brings it in line with `bambu_cloud` / `makerworld` / `firmware_check` / `inventory` (all unified during the May 2026 compliance pass) and makes Bambuddy a more obvious citizen to upstream WAFs and proxy operators. Won't defeat Cloudflare's JS challenge (the user's curl test proves CF blocks regardless of UA) but it's a consistency / hygiene fix with no regression risk. **(2) Cloudflare-challenge detection on the ntfy error path.** New `_looks_like_cloudflare_challenge(response)` helper checks the response shape (`Server: cloudflare` or `cf-mitigated` header, or `<!DOCTYPE html>...Just a moment...` body). When a 403/non-success response matches, the error returned to the UI now reads: *"HTTP 403 — ntfy server is behind a Cloudflare challenge. Bambuddy was served the JS challenge page instead of reaching ntfy. Cloudflare cannot be solved from a backend; add a Cloudflare security-skip rule for this hostname, disable Bot Fight Mode, or front the server with Cloudflare Access using a service token. (#1534)"* — actionable, points at the real fix, removes the raw HTML dump. A regular 403 (e.g. ntfy auth failure with a plain `forbidden: invalid auth token` body) still surfaces the original body so genuine auth errors stay debuggable; the interceptor only fires on the Cloudflare shape. **Tests**: 3 new in `TestNtfyOutbound` in `test_notification_service.py` — (a) the lazy-constructed httpx client carries the honest UA header on first use; (b) a 403 with `Server: cloudflare` + `Just a moment...` body produces the actionable error and does not echo `<!DOCTYPE` to the user; (c) a 403 with a plain text auth-failure body keeps the original `HTTP 403: forbidden: invalid auth token` so we don't hide real errors. 110/110 in the notification suites green under `pytest -n 30`. Backend ruff clean.
|
||||
- **Source-3MF upload on "fallback" archives no longer crashes with HTTP 500 (and stops orphaning files outside the data volume) (#1531, reported by @d3nn3s08)** — When MQTT reports a print start but Bambuddy never saw the source 3MF (cloud-initiated prints, Bambu Handy, prints already on the printer's SD card when Bambuddy connected), `main.py:2596` creates a "fallback" `PrintArchive` row with `file_path=""`. The two `Archives → Source 3MF Upload` routes computed the destination directory as `(settings.base_dir / archive.file_path).parent / "source"` — which on a fallback row collapsed to `Path('/app/data') / '' = Path('/app/data')`, whose `.parent` is `Path('/app')`, sending the upload to `/app/source/<filename>.3mf`. The file was physically written there (a path outside the user's mounted data volume — orphaned on container restart) and only the *final* `source_path.relative_to(settings.base_dir)` raised, so every retry left another orphan. Affected reporter is on a QNAP Docker host with the standard `/app/data` mount; both maintainer and triage initially diagnosed it as a Docker volume misconfiguration, but the traceback shows the bug is purely on Bambuddy's side — the user's setup was correct. **Fix**: new private helper `_resolve_source_3mf_path(archive, source_filename)` in `backend/app/api/routes/archives.py` centralises the destination computation. Normal archives still nest the source under `<archive_file_dir>/source/<filename>`. Fallback archives (empty `file_path`) now land under `<base_dir>/archive/no_source/<archive_id>/<filename>` instead — a deterministic, addressable location that stays inside the data volume, and the existing read sites (`download_source_3mf`, `download_source_3mf_by_filename`, the slicer-token routes, `delete_source_3mf`) all continue to work because they read back via `settings.base_dir / archive.source_3mf_path`. The helper also defensively asserts the resolved directory is inside `base_dir.resolve()` regardless of where it came from, so a row corrupted by an old import or a manual SQL edit fails with a clear 500 message ("Archive N resolves to a path outside the data directory; cannot attach source.") instead of silently writing outside the volume. Both upload sites (`upload_source_3mf` and `upload_source_3mf_by_name`, the slicer-post-processing endpoint) now route through the helper, so neither can independently drift back into the bug. **Tests**: 2 new in `TestUploadSourceThreeMF` in `backend/tests/integration/test_archives_api.py` — (a) `test_fallback_archive_source_upload_lands_under_base_dir` creates an archive with `file_path=""`, uploads a minimal valid 3MF, asserts 200 status, that the returned `source_3mf_path` is relative (not `/app/source/...`), that the file physically exists under the patched `base_dir`, and that the path is the deterministic fallback location keyed off `archive.id`; (b) `test_normal_archive_source_upload_unchanged` is the same flow against an archive with a populated `file_path`, asserting the existing `archives/test/source/<filename>.3mf` layout is preserved (regression guard against the helper accidentally changing the normal path). 57/57 in `test_archives_api.py` green under `pytest -n 30`. Backend ruff clean. **Note**: existing orphan files at `/app/source/<filename>.3mf` from prior failed retries inside an affected user's container can be safely deleted; they were never indexed in the DB, never reachable from the UI, and would have vanished on the next container restart anyway.
|
||||
- **SpoolBuddy weight sync no longer silently lands on a stale local row when Spoolman is enabled (#1530, reported by @chesterakl)** — Reporter (Spoolman mode, H2C, internal "manually add then NFC-link" flow) saw the SpoolBuddy "Sync Weight" button flip to "Synced!" but the Spoolman-backed inventory listing never updated. Cause: `POST /spoolbuddy/scale/update-spool-weight` (`backend/app/api/routes/spoolbuddy.py`) ran the lookup local-DB-first and only fell through to Spoolman on local miss — but the upstream `nfc/tag-scanned` route is exclusive (always-Spoolman when `spoolman_enabled=true`, after the #1119 / nfc-routing fix). When the user's local DB still held a stale `Spool` row that happened to share a numeric id with the Spoolman spool the NFC tag mapped to, the sync endpoint absorbed the update into the stale local row, returned 200 with the local `weight_used`, and the actual Spoolman spool went untouched. The support log confirms it: 17 sync attempts across two days, every line logged `SpoolBuddy updated spool 2 weight: …g on scale, …g used` (the local-branch log format) and the `SpoolBuddy updated Spoolman spool …` line (which only fires in the Spoolman branch) never appeared. The bug couldn't be reproduced on developer setups because they don't carry a leftover local row with a colliding id. **Fix**: `update_spool_weight` now routes exactly like `nfc_tag_scanned` — `_get_spoolman_client_or_none(db)` first, and that result picks the branch exclusively. Spoolman mode goes straight to Spoolman with no local-DB read; local mode does the local update and returns 404 (not "fallback to Spoolman") on a local miss. Matches [[feedback_inventory_modes_parity]] — the two inventory modes must behave identically from the user's perspective, including which row gets written. The docstring now spells out the routing contract so the next reader doesn't reintroduce the local-first read. **Tests**: 1 new regression test in `TestUpdateSpoolWeightSpoolman.test_stale_local_row_does_not_shadow_spoolman` — creates a local `Spool` with the same numeric id as a mocked Spoolman spool, posts the sync, asserts (a) Spoolman's `update_spool` was called with the correct remaining weight, and (b) the local row's `weight_used` and `last_scale_weight` are unchanged after a `refresh()` against the live DB. The existing 8 tests in that class continue to assert the Spoolman branch math (filament/spool-level tare priority, 404 / 503 mappings, 250g fallback warning). 9/9 green; 126/126 across the spoolbuddy + spoolman-filament-patch integration suites green under `pytest -n 30`. **Cleanup hint for affected users**: anyone in Spoolman mode with leftover local Spool rows from before they switched should delete those rows — they're inert under the new routing, but they were eating sync attempts under the old. Backend ruff clean.
|
||||
- **Paused prints no longer inflate maintenance hours (#1521, reported by @TempleClause)** — The `track_printer_runtime` background task in `backend/app/main.py` counted both `RUNNING` and `PAUSE` states equally toward `runtime_seconds`, which feeds every hours-based maintenance interval (lubricate rods, clean nozzle, check belts, etc.). Maintenance items measure *mechanical wear*, and pause time involves no motion — so a print paused overnight stretched the maintenance clock forward by ~8 h without any actual wear, triggering "lubricate rods" warnings earlier than warranted. Reporter found this by code review (no support bundle), flagged it cleanly with the exact line in `main.py` and three ranked solution options. **Fix**: option 1 (exclude PAUSE entirely) — `state.state in ("RUNNING", "PAUSE")` → `state.state == "RUNNING"`. PAUSE now follows the same path as FINISH / IDLE / PREPARE: the elapsed-time accumulator skips it, and `last_runtime_update` is cleared so a later RUNNING transition starts fresh and doesn't back-bill the pause. No setting / toggle (reporter's option 3 was deliberately the throwaway — this is a wear-tracking semantic, not a user preference); no cap (option 2) — wear during pause is zero, not "reduced". Docstring and field-comment trail updated across `main.py`, `models/printer.py:23`, and the two `api/routes/maintenance.py` route docstrings that all previously described the field as covering "RUNNING and PAUSE states". **Out of scope**: retroactive backfill of existing `runtime_seconds` values — already-accumulated pause time cannot be split out, only future accumulation is fixed. Users with hours-based maintenance intervals already set will see slower accumulation going forward (the correct outcome), so a previously-near-due item may take longer to ring than under the old behaviour. **Tests**: 3 new in `test_runtime_tracking_pause.py` pinning the new contract — PAUSE does NOT accumulate and clears `last_runtime_update`; RUNNING still accumulates and updates the timestamp; a non-active state (FINISH) clears `last_runtime_update` to prevent back-billing the idle time when the printer next goes RUNNING. The tests drive the actual `track_printer_runtime()` coroutine through a single iteration via patched `asyncio.sleep` against an in-memory SQLite DB, so they catch any regression in the predicate at the call site (not just an extracted helper). Backend ruff clean; targeted 24-test rod/runtime subset all green.
|
||||
|
||||
@@ -20,6 +20,47 @@ from backend.app.models.notification_template import NotificationTemplate
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
# Honest User-Agent — matches the convention used by every other outbound
|
||||
# httpx client in the codebase (bambu_cloud, makerworld, firmware_check,
|
||||
# inventory). Previously this client leaked python-httpx/<version>, which
|
||||
# was both inconsistent with the rest of the project and a more obvious
|
||||
# bot signature for upstream WAFs.
|
||||
_USER_AGENT = "Bambuddy/1.0 (+https://github.com/maziggy/bambuddy)"
|
||||
|
||||
|
||||
def _looks_like_cloudflare_challenge(response: httpx.Response) -> bool:
|
||||
"""Return True if ``response`` looks like a Cloudflare mitigation
|
||||
interstitial (JS challenge / managed challenge / block page) rather
|
||||
than a legitimate response passed through Cloudflare.
|
||||
|
||||
Self-hosted servers behind Cloudflare (Tunnel, "Bot Fight Mode", or
|
||||
"Under Attack" mode) intercept non-browser clients at the edge and
|
||||
return a challenge HTML page instead of forwarding to the origin —
|
||||
so we never reach the user's actual ntfy / webhook backend.
|
||||
Cloudflare cannot be defeated from a Python client; the user has to
|
||||
add a security-skip rule on their side. We detect the shape so the
|
||||
UI can tell them that, instead of dumping the raw HTML.
|
||||
|
||||
Detection deliberately does NOT rely on ``Server: cloudflare`` alone
|
||||
— Cloudflare adds that header to every response it proxies (success
|
||||
AND legitimate origin errors), so a real 401 "wrong token" from a
|
||||
CF-fronted ntfy would false-positive into a misleading "your CF is
|
||||
blocking" message. Reliable signals: the ``cf-mitigated`` header
|
||||
(set only when CF actively mitigates) and the challenge body shape.
|
||||
"""
|
||||
if response.headers.get("cf-mitigated"):
|
||||
return True
|
||||
content_type = (response.headers.get("content-type") or "").lower()
|
||||
if "html" not in content_type:
|
||||
return False
|
||||
body = (response.text or "")[:1024].lower()
|
||||
# "Just a moment..." is Cloudflare's universal challenge-page title
|
||||
# (managed challenge, JS challenge, Under Attack mode). Combined with
|
||||
# an HTML content-type this is unambiguous — no legitimate ntfy or
|
||||
# webhook backend returns HTML with that title. ``cf-chl-*`` and
|
||||
# ``challenge-platform`` cover newer / non-default CF templates.
|
||||
return "just a moment" in body or "cf-chl-bypass" in body or "cf-chl-opt" in body or "challenge-platform" in body
|
||||
|
||||
|
||||
class NotificationService:
|
||||
"""Service for sending notifications through various providers."""
|
||||
@@ -33,7 +74,10 @@ class NotificationService:
|
||||
async def _get_client(self) -> httpx.AsyncClient:
|
||||
"""Get or create HTTP client."""
|
||||
if self._http_client is None or self._http_client.is_closed:
|
||||
self._http_client = httpx.AsyncClient(timeout=30.0)
|
||||
self._http_client = httpx.AsyncClient(
|
||||
timeout=30.0,
|
||||
headers={"User-Agent": _USER_AGENT},
|
||||
)
|
||||
return self._http_client
|
||||
|
||||
async def close(self):
|
||||
@@ -264,8 +308,16 @@ class NotificationService:
|
||||
|
||||
if response.status_code in (200, 204):
|
||||
return True, "Message sent successfully"
|
||||
else:
|
||||
return False, f"HTTP {response.status_code}: {response.text[:200]}"
|
||||
if _looks_like_cloudflare_challenge(response):
|
||||
return False, (
|
||||
f"HTTP {response.status_code} — ntfy server is behind a Cloudflare "
|
||||
"challenge. Bambuddy was served the JS challenge page instead of "
|
||||
"reaching ntfy. Cloudflare cannot be solved from a backend; add a "
|
||||
"Cloudflare security-skip rule for this hostname, disable Bot "
|
||||
"Fight Mode, or front the server with Cloudflare Access using a "
|
||||
"service token. (#1534)"
|
||||
)
|
||||
return False, f"HTTP {response.status_code}: {response.text[:200]}"
|
||||
|
||||
async def _send_pushover(
|
||||
self, config: dict, title: str, message: str, image_data: bytes | None = None
|
||||
|
||||
@@ -2057,3 +2057,149 @@ class TestFirstLayerCompleteNotifications:
|
||||
mock_send.assert_called_once()
|
||||
call_kwargs = mock_send.call_args
|
||||
assert call_kwargs.kwargs.get("image_data") == fake_image
|
||||
|
||||
|
||||
class TestNtfyOutbound:
|
||||
"""Regression for #1534 — UA hygiene and Cloudflare-challenge detection."""
|
||||
|
||||
@pytest.fixture
|
||||
def service(self):
|
||||
return NotificationService()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_notification_client_sets_honest_user_agent(self, service):
|
||||
"""Default httpx UA leaks `python-httpx/<version>` — every other
|
||||
outbound client in the codebase identifies as Bambuddy. The
|
||||
notification client must too."""
|
||||
client = await service._get_client()
|
||||
try:
|
||||
assert client.headers.get("user-agent") == "Bambuddy/1.0 (+https://github.com/maziggy/bambuddy)"
|
||||
finally:
|
||||
await service.close()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_ntfy_cloudflare_challenge_returns_actionable_error(self, service):
|
||||
"""When ntfy is fronted by Cloudflare and CF returns its JS
|
||||
challenge, the user must see a message that points at the actual
|
||||
fix (CF security skip), not the raw HTML."""
|
||||
import httpx
|
||||
|
||||
challenge_html = (
|
||||
'<!DOCTYPE html><html lang="en-US"><head><title>Just a moment...</title>'
|
||||
'<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">'
|
||||
)
|
||||
mock_response = httpx.Response(
|
||||
403,
|
||||
content=challenge_html.encode(),
|
||||
headers={"server": "cloudflare", "content-type": "text/html; charset=UTF-8"},
|
||||
)
|
||||
|
||||
mock_client = AsyncMock()
|
||||
mock_client.post = AsyncMock(return_value=mock_response)
|
||||
|
||||
with patch.object(service, "_get_client", AsyncMock(return_value=mock_client)):
|
||||
ok, detail = await service._send_ntfy(
|
||||
{"server": "https://ntfy.example", "topic": "alerts", "auth_token": "tk_xxx"},
|
||||
title="t",
|
||||
message="m",
|
||||
)
|
||||
|
||||
assert ok is False
|
||||
assert "Cloudflare" in detail
|
||||
assert "security-skip" in detail or "Bot Fight Mode" in detail
|
||||
# The raw HTML must not be the dominant content shown to the user.
|
||||
assert "<!DOCTYPE" not in detail
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_ntfy_normal_403_still_surfaces_body(self, service):
|
||||
"""A non-Cloudflare 403 (e.g. ntfy auth fail) must keep showing
|
||||
the original body so the user can debug the real error — we
|
||||
only intercept the Cloudflare-challenge shape."""
|
||||
import httpx
|
||||
|
||||
mock_response = httpx.Response(
|
||||
403,
|
||||
content=b"forbidden: invalid auth token",
|
||||
headers={"content-type": "text/plain"},
|
||||
)
|
||||
|
||||
mock_client = AsyncMock()
|
||||
mock_client.post = AsyncMock(return_value=mock_response)
|
||||
|
||||
with patch.object(service, "_get_client", AsyncMock(return_value=mock_client)):
|
||||
ok, detail = await service._send_ntfy(
|
||||
{"server": "https://ntfy.sh", "topic": "alerts", "auth_token": "bad"},
|
||||
title="t",
|
||||
message="m",
|
||||
)
|
||||
|
||||
assert ok is False
|
||||
assert "Cloudflare" not in detail
|
||||
assert "invalid auth token" in detail
|
||||
assert detail.startswith("HTTP 403:")
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_ntfy_origin_error_through_cloudflare_is_not_misclassified(self, service):
|
||||
"""Cloudflare adds Server: cloudflare to EVERY proxied response,
|
||||
including legitimate origin errors. A real 401 "wrong token"
|
||||
from an ntfy server that happens to sit behind Cloudflare must
|
||||
still surface the origin's actual error body — we must not flip
|
||||
every CF-fronted 4xx into a "your Cloudflare is blocking" message.
|
||||
"""
|
||||
import httpx
|
||||
|
||||
mock_response = httpx.Response(
|
||||
401,
|
||||
content=b'{"code":40101,"http":401,"error":"unauthorized"}',
|
||||
headers={
|
||||
"server": "cloudflare",
|
||||
"cf-ray": "abc123-FRA",
|
||||
"content-type": "application/json",
|
||||
# No cf-mitigated — CF just proxied the origin response.
|
||||
},
|
||||
)
|
||||
|
||||
mock_client = AsyncMock()
|
||||
mock_client.post = AsyncMock(return_value=mock_response)
|
||||
|
||||
with patch.object(service, "_get_client", AsyncMock(return_value=mock_client)):
|
||||
ok, detail = await service._send_ntfy(
|
||||
{"server": "https://ntfy.example", "topic": "alerts", "auth_token": "wrong"},
|
||||
title="t",
|
||||
message="m",
|
||||
)
|
||||
|
||||
assert ok is False
|
||||
assert "Cloudflare" not in detail
|
||||
assert "unauthorized" in detail
|
||||
assert detail.startswith("HTTP 401:")
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_ntfy_cloudflare_cf_mitigated_header_alone_triggers(self, service):
|
||||
"""The cf-mitigated header on its own is enough — that's the
|
||||
canonical CF "I actively blocked this" signal, even if the
|
||||
response body shape changes between CF challenge generations."""
|
||||
import httpx
|
||||
|
||||
mock_response = httpx.Response(
|
||||
403,
|
||||
content=b"<html>some future CF block page</html>",
|
||||
headers={
|
||||
"server": "cloudflare",
|
||||
"cf-mitigated": "challenge",
|
||||
"content-type": "text/html",
|
||||
},
|
||||
)
|
||||
|
||||
mock_client = AsyncMock()
|
||||
mock_client.post = AsyncMock(return_value=mock_response)
|
||||
|
||||
with patch.object(service, "_get_client", AsyncMock(return_value=mock_client)):
|
||||
ok, detail = await service._send_ntfy(
|
||||
{"server": "https://ntfy.example", "topic": "alerts"},
|
||||
title="t",
|
||||
message="m",
|
||||
)
|
||||
|
||||
assert ok is False
|
||||
assert "Cloudflare" in detail
|
||||
|
||||
Reference in New Issue
Block a user