mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-10-19 07:33:18 +02:00
fix(vp): stop uvloop from silently truncating VP FTP uploads (#1896)
Native (non-Docker) installs launched uvicorn without --loop asyncio, so uvicorn[standard] auto-selected uvloop. uvloop's SSL layer drops already-received but still-buffered data when the client closes the data connection without a TLS close_notify while the reader is flow-control paused on slow storage. cmd_STOR writes each chunk to disk inside the read loop, so a slow consumer falls behind, the tail is lost, read() returns a clean EOF, and the loop exits with no exception -- the server acked 226 for a file it truncated itself, then archived, queued, and forwarded the corrupt 3MF to the real printer. Fix in two independent layers: 1. Remove the trigger: add --loop asyncio to every native launch path, matching the Dockerfile -- deploy/bambuddy.service, install/install.sh (systemd + launchd), spoolbuddy/install/install.sh, the Windows NSSM service, README, CONTRIBUTING dev command. 2. Defense in depth (loop-independent): cmd_STOR now validates that a received .3mf opens as a ZIP (reads the central directory, no decompression) before replying 226. A truncated/corrupt file is dropped and answered with 426, and on_file_received never runs -- so a broken upload surfaces as an immediate slicer-side send error instead of being archived and pushed to the printer. Scoped to .3mf; other filetypes pass through unchanged.
This commit is contained in:
@@ -13,6 +13,7 @@ import logging
|
||||
import os
|
||||
import random
|
||||
import ssl
|
||||
import zipfile
|
||||
from collections.abc import Callable
|
||||
from pathlib import Path
|
||||
|
||||
@@ -476,6 +477,43 @@ class FTPSession:
|
||||
await self.send(426, f"Transfer failed: {write_failed}")
|
||||
return
|
||||
|
||||
# Defense in depth (#1896): a clean read-loop EOF does NOT prove the
|
||||
# upload arrived intact. Under uvloop, the SSL layer can silently drop
|
||||
# already-received but still-buffered data when the client closes the
|
||||
# data connection without a TLS close_notify (a "ragged EOF") while the
|
||||
# transport is flow-control-paused on slow storage — read() then returns
|
||||
# b"" and we would otherwise reply 226 for a tail-truncated file, archive
|
||||
# it, queue it, and forward the corrupt job to the real printer.
|
||||
#
|
||||
# Bambu 3MF uploads are ZIP containers whose End-Of-Central-Directory
|
||||
# record sits at the very end of the file, so any lost tail makes the
|
||||
# archive impossible to open. Verify that before acknowledging success:
|
||||
# a truncated file is treated exactly like a failed transfer (426 +
|
||||
# drop) so the slicer surfaces an actionable send error instead of the
|
||||
# printer choking on a half-written job later. Only ZIP-based (.3mf)
|
||||
# uploads are validated — other filetypes keep the prior pass-through
|
||||
# behaviour. Reading the central directory is O(dir), not O(file): no
|
||||
# decompression, negligible next to the write loop above.
|
||||
if filename.lower().endswith(".3mf"):
|
||||
try:
|
||||
with zipfile.ZipFile(file_path) as zf:
|
||||
zf.namelist()
|
||||
except Exception as e:
|
||||
logger.error(
|
||||
"FTP upload of %s is a corrupt/truncated 3MF (%s bytes): %s(%s) — "
|
||||
"rejecting with 426 instead of archiving a broken file",
|
||||
filename,
|
||||
total_received,
|
||||
type(e).__name__,
|
||||
e,
|
||||
)
|
||||
try:
|
||||
file_path.unlink(missing_ok=True)
|
||||
except OSError:
|
||||
pass
|
||||
await self.send(426, "Transfer failed: uploaded 3MF is incomplete or corrupt")
|
||||
return
|
||||
|
||||
# Confirm + notify
|
||||
logger.info("FTP saved file: %s (%s bytes)", file_path, total_received)
|
||||
await self.send(226, "Transfer complete")
|
||||
|
||||
@@ -10,7 +10,9 @@ up a real TLS/FTP server.
|
||||
"""
|
||||
|
||||
import asyncio
|
||||
import io
|
||||
import ssl
|
||||
import zipfile
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
import pytest
|
||||
@@ -18,6 +20,22 @@ import pytest
|
||||
from backend.app.services.virtual_printer.ftp_server import MAX_UPLOAD_BYTES, FTPSession
|
||||
|
||||
|
||||
def _valid_3mf_bytes() -> bytes:
|
||||
"""A minimal but structurally valid ZIP (stands in for a .gcode.3mf).
|
||||
|
||||
Bambu 3MF files are ZIP containers; the streaming STOR path validates the
|
||||
received file opens as a ZIP before acking 226 (#1896), so happy-path
|
||||
tests must feed real ZIP bytes rather than arbitrary filler.
|
||||
"""
|
||||
buf = io.BytesIO()
|
||||
with zipfile.ZipFile(buf, "w") as zf:
|
||||
zf.writestr("Metadata/slice_info.config", "<config/>")
|
||||
zf.writestr("3D/3dmodel.model", "<model/>")
|
||||
# Pad an entry so the archive spans several 64 KiB read chunks.
|
||||
zf.writestr("plate_1.gcode", b"G1 X0 Y0\n" * 40000)
|
||||
return buf.getvalue()
|
||||
|
||||
|
||||
def _make_session(tmp_path, *, data_chunks: list[bytes]) -> FTPSession:
|
||||
"""Build an FTPSession primed with a pre-fed StreamReader so cmd_STOR
|
||||
can iterate through the chunks without a real TCP connection.
|
||||
@@ -63,8 +81,9 @@ def _make_session(tmp_path, *, data_chunks: list[bytes]) -> FTPSession:
|
||||
async def test_stor_writes_payload_to_disk(tmp_path):
|
||||
"""Happy path: chunks fed to the data reader land in the upload_dir
|
||||
with the right content + the slicer gets 226."""
|
||||
payload = b"X" * (3 * 64 * 1024 + 123) # 3 chunks + a partial one
|
||||
payload = _valid_3mf_bytes() # spans several 64 KiB chunks, opens as ZIP
|
||||
chunks = [payload[i : i + 65536] for i in range(0, len(payload), 65536)]
|
||||
assert len(chunks) > 3 # exercise the multi-chunk read loop
|
||||
session = _make_session(tmp_path, data_chunks=chunks)
|
||||
session.send = AsyncMock()
|
||||
|
||||
@@ -78,6 +97,54 @@ async def test_stor_writes_payload_to_disk(tmp_path):
|
||||
sent_codes = [args[0][0] for args in session.send.call_args_list]
|
||||
assert 150 in sent_codes # "Opening data connection"
|
||||
assert 226 in sent_codes # "Transfer complete"
|
||||
assert 426 not in sent_codes
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_stor_rejects_truncated_3mf(tmp_path):
|
||||
"""#1896: a .3mf whose tail was lost (uvloop ragged-EOF data loss, or any
|
||||
other silent truncation) must NOT be acked with 226 — the read loop sees a
|
||||
clean EOF and no write error, so only a ZIP-integrity check catches it.
|
||||
Reject with 426, drop the file, and never fire the on_file_received
|
||||
callback that would archive/queue/forward the corrupt job."""
|
||||
payload = _valid_3mf_bytes()
|
||||
truncated = payload[: len(payload) - 4096] # drop the EOCD-bearing tail
|
||||
chunks = [truncated[i : i + 65536] for i in range(0, len(truncated), 65536)]
|
||||
|
||||
callback = AsyncMock()
|
||||
session = _make_session(tmp_path, data_chunks=chunks)
|
||||
session.on_file_received = callback
|
||||
session.send = AsyncMock()
|
||||
|
||||
await session.cmd_STOR("truncated.gcode.3mf")
|
||||
|
||||
# Corrupt file dropped, not left in the upload dir.
|
||||
assert not (session.upload_dir / "truncated.gcode.3mf").exists()
|
||||
sent_codes = [args[0][0] for args in session.send.call_args_list]
|
||||
assert 426 in sent_codes
|
||||
assert 226 not in sent_codes
|
||||
# The archive/queue/forward callback must never run for a corrupt upload.
|
||||
callback.assert_not_called()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_stor_skips_zip_validation_for_non_3mf(tmp_path):
|
||||
"""The ZIP-integrity gate is scoped to .3mf uploads. A non-3MF file (e.g.
|
||||
a plain .gcode some slicers still send) is not a ZIP and must keep the
|
||||
prior pass-through behaviour — 226, not a false-positive 426."""
|
||||
payload = b"G1 X0 Y0\n" * 5000 # plain text, deliberately not a ZIP
|
||||
chunks = [payload[i : i + 65536] for i in range(0, len(payload), 65536)]
|
||||
session = _make_session(tmp_path, data_chunks=chunks)
|
||||
session.send = AsyncMock()
|
||||
|
||||
await session.cmd_STOR("plain.gcode")
|
||||
|
||||
saved = session.upload_dir / "plain.gcode"
|
||||
assert saved.exists()
|
||||
assert saved.read_bytes() == payload
|
||||
sent_codes = [args[0][0] for args in session.send.call_args_list]
|
||||
assert 226 in sent_codes
|
||||
assert 426 not in sent_codes
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
||||
Reference in New Issue
Block a user