feat: memory-only sessions on HTTP, streamed exports, UI refresh
Session - COOKIE_SECURE=false no longer persists the owner key for ten years. services/session.cookie_kwargs() drops max_age in that mode, so the browser holds the key in memory and the session ends with the window. Everything still persists server-side; only the browser link is temporary. base.html shows a warning banner (FR/EN) and an extra paragraph in the onboarding modal, and the README explains the trade-off and the backup-key escape hatch. - Both cookie writers (middleware, POST /session/restore) go through cookie_kwargs() so the policy cannot drift between them. - The CSRF guard on /session/restore compared request.url.scheme against the Origin header. Behind a TLS-terminating proxy uvicorn sees http while the browser sends https, so every legitimate restore was rejected with 403. It now compares hosts only, including X-Forwarded-Host. - /static/*, /favicon.ico and /robots.txt skip the middleware. Each cookieless hit was inserting an Owner row no browser could ever use. Reliability - Malformed printer-form FK fields no longer escape as HTTP 500: a non-numeric client_id/driver_id raised ValueError and an unknown driver_id hit a FOREIGN KEY constraint. Both are now 400/404 HTMX fragments, and the duplicated field checks moved into _validate_fields(). - Package exports stream. build_intunewin() encrypts the inner ZIP in 1 MB chunks against temp files with a streaming HMAC and SHA256, and both endpoints serve the result with FileResponse plus a background cleanup task. A 100 MB driver used to be held in memory three or four times over per concurrent download. The byte layout is unchanged. - FileResponse also escapes the download filename, which was previously interpolated raw into Content-Disposition. - python-multipart >= 0.0.18 (CVE-2024-53981, reachable from /drivers/upload) and Pillow >= 10.3 (CVE-2024-28219, reachable from icon upload). - icons.py reads cfg.ICONS_DIR instead of re-deriving the path from DATA_DIR, matching the .intunewin export. UI - Sidebar/topbar shell, inline SVG icon macros (partials/icons.html), card and data-table components, grouped printer list, and the dedicated /printers/new page replacing partials/printer_form.html. Tests - 194 pass with a bare `pytest tests/`: tests/conftest.py now forces cfg.COOKIE_SECURE = False like the e2e conftest already did, so the Secure cookie is no longer dropped over http://testserver. - New coverage for the malformed-FK guards, the chunk-boundary cases in the encrypt loop (every residue mod _CHUNK plus a multi-megabyte payload), temp-dir cleanup after both exports, and the whole COOKIE_SECURE matrix. - test_printer_edit.py located the Edit button by its translated label, so it only passed on English-locale machines. It now targets the showModal() hook, which also cuts the e2e run from 84s to 15s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -40,6 +40,10 @@ def tmp_data_dir(tmp_path, monkeypatch):
|
||||
cfg.DB_PATH = str(data_dir / "imptune.db")
|
||||
cfg.DRIVERS_DIR = str(data_dir / "drivers")
|
||||
cfg.ICONS_DIR = str(data_dir / "icons")
|
||||
# TestClient talks plain HTTP to http://testserver, so a Secure cookie is
|
||||
# dropped and every request lands on a fresh Owner (~41 spurious 404s).
|
||||
# Tests that care about the Secure branch monkeypatch this back to True.
|
||||
cfg.COOKIE_SECURE = False
|
||||
|
||||
yield data_dir
|
||||
|
||||
|
||||
@@ -3,6 +3,14 @@ from __future__ import annotations
|
||||
|
||||
import pytest
|
||||
|
||||
# The Edit button's label goes through the i18n store, which follows
|
||||
# navigator.language — on a French-locale machine it reads "Modifier", so a
|
||||
# has-text('Edit') locator only ever matched on English hosts. Target the
|
||||
# showModal() hook instead: it is the same in every language.
|
||||
# showModal(), not just edit-modal: the dialog's own close buttons reference
|
||||
# the same element id via .close().
|
||||
EDIT_BUTTON = "button[onclick*='showModal']"
|
||||
|
||||
|
||||
def test_printer_edit_modal_open_and_prefill(page, live_server: str, _e2e_owner_key: str) -> None:
|
||||
"""Edit button opens modal with printer's current name pre-filled."""
|
||||
@@ -24,8 +32,8 @@ def test_printer_edit_modal_open_and_prefill(page, live_server: str, _e2e_owner_
|
||||
)
|
||||
|
||||
page.goto(f"{live_server}/printers", wait_until="domcontentloaded")
|
||||
page.wait_for_selector("button:has-text('Edit')")
|
||||
page.click("button:has-text('Edit')")
|
||||
page.wait_for_selector(EDIT_BUTTON)
|
||||
page.click(EDIT_BUTTON)
|
||||
|
||||
# Dialog should be open
|
||||
page.wait_for_selector("dialog[open]")
|
||||
@@ -57,7 +65,7 @@ def test_printer_edit_submit_updates_list(page, live_server: str, _e2e_owner_key
|
||||
# Find the row containing "OriginalName" and click its Edit button
|
||||
row = page.locator("tr", has=page.locator("a", has_text="OriginalName"))
|
||||
row.wait_for()
|
||||
row.locator("button:has-text('Edit')").click()
|
||||
row.locator(EDIT_BUTTON).click()
|
||||
page.wait_for_selector("dialog[open]")
|
||||
|
||||
# Clear and update the name field
|
||||
|
||||
@@ -93,15 +93,21 @@ def test_upload_no_inf(client: TestClient) -> None:
|
||||
assert resp.status_code == 400
|
||||
|
||||
|
||||
def test_upload_returns_select(client: TestClient) -> None:
|
||||
"""POST /drivers/upload with valid ZIP returns HTML containing a <select> element."""
|
||||
def test_upload_returns_driver_list_with_parsed_names(client: TestClient) -> None:
|
||||
"""POST /drivers/upload with a valid ZIP returns the driver-list fragment
|
||||
showing the names parsed out of the .inf.
|
||||
|
||||
The names used to render inside a display-only <select>; the UI overhaul
|
||||
renders them as pills, so this asserts the swap target and the names
|
||||
themselves rather than the widget that carries them.
|
||||
"""
|
||||
zip_bytes = _make_driver_zip()
|
||||
resp = client.post(
|
||||
"/drivers/upload",
|
||||
files={"file": ("driver.zip", zip_bytes, "application/zip")},
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
assert "<select" in resp.text
|
||||
assert 'id="driver-list"' in resp.text
|
||||
assert "Test LaserJet Pro" in resp.text
|
||||
|
||||
|
||||
|
||||
@@ -281,3 +281,71 @@ class TestCryptographicVerification:
|
||||
assert xml_size == len(plaintext), (
|
||||
f"UnencryptedContentSize {xml_size} does not match actual plaintext size {len(plaintext)}"
|
||||
)
|
||||
|
||||
|
||||
class TestStreamingChunkBoundaries:
|
||||
"""The builder encrypts the inner ZIP in _CHUNK-sized pieces rather than in
|
||||
one buffer. Only the final (short) read carries PKCS7 padding, so a payload
|
||||
landing exactly on a chunk boundary is the case that breaks first."""
|
||||
|
||||
@staticmethod
|
||||
def _decrypt(built_package):
|
||||
with zipfile.ZipFile(built_package, "r") as outer:
|
||||
blob = outer.read("IntuneWinPackage/Contents/IntunePackage.intunewin")
|
||||
tree = ET.fromstring(
|
||||
outer.read("IntuneWinPackage/Metadata/Detection.xml").decode("utf-8")
|
||||
)
|
||||
aes_key = base64.b64decode(_get_enc_text(tree, "EncryptionKey"))
|
||||
iv = base64.b64decode(_get_enc_text(tree, "InitializationVector"))
|
||||
cipher = AES.new(aes_key, AES.MODE_CBC, iv)
|
||||
plaintext = unpad(cipher.decrypt(blob[48:]), AES.block_size)
|
||||
|
||||
# MAC covers IV || ciphertext and must survive chunked hashing
|
||||
mac_key = base64.b64decode(_get_enc_text(tree, "MacKey"))
|
||||
assert hmac.new(mac_key, blob[32:], hashlib.sha256).digest() == blob[:32]
|
||||
# Metadata is computed while streaming, not from a buffered plaintext
|
||||
assert base64.b64decode(_get_enc_text(tree, "FileDigest")) == hashlib.sha256(plaintext).digest()
|
||||
assert int(_get_xml_text(tree, "UnencryptedContentSize")) == len(plaintext)
|
||||
return plaintext
|
||||
|
||||
@pytest.mark.parametrize("payload_size", range(96, 128))
|
||||
def test_every_residue_of_chunk_size(self, tmp_path, monkeypatch, payload_size):
|
||||
"""Sweep 32 consecutive sizes with a 16-byte chunk so every offset mod
|
||||
_CHUNK is exercised, including the exact-multiple case where the last
|
||||
read returns b"" and padding is a whole standalone block."""
|
||||
monkeypatch.setattr("imptune.generators.intunewin_builder._CHUNK", 16)
|
||||
|
||||
src = tmp_path / "src"
|
||||
src.mkdir()
|
||||
# Incompressible, so inner-ZIP size tracks payload size 1:1
|
||||
(src / "install.ps1").write_bytes(os.urandom(payload_size))
|
||||
output = str(tmp_path / "package.intunewin")
|
||||
|
||||
build_intunewin(str(src), "install.ps1", output)
|
||||
plaintext = self._decrypt(output)
|
||||
assert zipfile.is_zipfile(io.BytesIO(plaintext))
|
||||
|
||||
def test_multi_megabyte_payload_roundtrips(self, tmp_path):
|
||||
"""Several full 1 MB chunks through the default code path."""
|
||||
src = tmp_path / "src"
|
||||
src.mkdir()
|
||||
blob = os.urandom(2_500_000)
|
||||
(src / "install.ps1").write_text("Write-Host 'go'")
|
||||
(src / "driver.bin").write_bytes(blob)
|
||||
output = str(tmp_path / "package.intunewin")
|
||||
|
||||
build_intunewin(str(src), "install.ps1", output)
|
||||
plaintext = self._decrypt(output)
|
||||
|
||||
with zipfile.ZipFile(io.BytesIO(plaintext)) as inner:
|
||||
assert inner.read("driver.bin") == blob
|
||||
|
||||
|
||||
class TestArgumentValidation:
|
||||
def test_missing_source_dir_raises(self, tmp_path):
|
||||
with pytest.raises(FileNotFoundError):
|
||||
build_intunewin(str(tmp_path / "nope"), "install.ps1", str(tmp_path / "o.intunewin"))
|
||||
|
||||
def test_empty_setup_file_raises(self, source_dir, tmp_path):
|
||||
with pytest.raises(ValueError):
|
||||
build_intunewin(source_dir, "", str(tmp_path / "o.intunewin"))
|
||||
|
||||
@@ -256,3 +256,70 @@ class TestIntunewinIconInclusion:
|
||||
printer, _ = setup_printer_with_driver
|
||||
resp = client.get(f"/printers/{printer.id}/packages/intunewin")
|
||||
assert resp.status_code == 200
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Streamed exports — packages are assembled on disk, not buffered in RAM
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestStreamedExportCleanup:
|
||||
@pytest.fixture
|
||||
def bulky_printer(self, tmp_data_dir, owner):
|
||||
"""Printer whose driver ZIP is large enough to prove the body streamed."""
|
||||
import hashlib
|
||||
import os
|
||||
|
||||
import imptune.config as cfg
|
||||
from imptune.db.models import Driver, Printer
|
||||
|
||||
buf = io.BytesIO()
|
||||
with zipfile.ZipFile(buf, "w") as zf:
|
||||
zf.writestr("printer.inf", "[Version]\nSignature=$WINDOWS NT$\n")
|
||||
zf.writestr("sub/payload.bin", os.urandom(200_000))
|
||||
data = buf.getvalue()
|
||||
|
||||
sha = hashlib.sha256(data).hexdigest()
|
||||
os.makedirs(cfg.DRIVERS_DIR, exist_ok=True)
|
||||
with open(os.path.join(cfg.DRIVERS_DIR, f"{sha}.zip"), "wb") as f:
|
||||
f.write(data)
|
||||
|
||||
driver = Driver.create(
|
||||
sha256=sha,
|
||||
original_filename="bulky.zip",
|
||||
size_bytes=len(data),
|
||||
driver_desc=json.dumps(["Acme SuperPrint"]),
|
||||
inf_filename="printer.inf",
|
||||
)
|
||||
return Printer.create(
|
||||
name="Bulky Printer",
|
||||
ip_address="10.1.1.1",
|
||||
port_name="IP_10.1.1.1",
|
||||
owner=owner,
|
||||
driver=driver,
|
||||
)
|
||||
|
||||
@pytest.mark.parametrize("kind", ["ninja", "intunewin"])
|
||||
def test_export_streams_and_removes_its_temp_dir(self, client, bulky_printer, kind):
|
||||
"""The response outlives the handler's temp dir, so a background task
|
||||
deletes it — a leak here fills the container's disk one download at a time."""
|
||||
import glob
|
||||
import os
|
||||
import tempfile
|
||||
|
||||
pattern = os.path.join(tempfile.gettempdir(), "imptune_*")
|
||||
before = set(glob.glob(pattern))
|
||||
|
||||
resp = client.get(f"/printers/{bulky_printer.id}/packages/{kind}")
|
||||
|
||||
assert resp.status_code == 200
|
||||
assert len(resp.content) > 150_000, "payload looks truncated"
|
||||
assert set(glob.glob(pattern)) == before
|
||||
|
||||
def test_ninja_zip_preserves_nested_driver_paths(self, client, bulky_printer):
|
||||
"""Members are copied chunk-wise via ZipFile.open — nested paths must survive."""
|
||||
resp = client.get(f"/printers/{bulky_printer.id}/packages/ninja")
|
||||
with zipfile.ZipFile(io.BytesIO(resp.content)) as zf:
|
||||
names = zf.namelist()
|
||||
assert "Bulky_Printer/drivers/sub/payload.bin" in names
|
||||
assert zf.getinfo("Bulky_Printer/drivers/sub/payload.bin").file_size == 200_000
|
||||
|
||||
@@ -392,3 +392,91 @@ def test_client_links_in_printer_list(client: TestClient, owner) -> None:
|
||||
resp = client.get("/printers")
|
||||
assert resp.status_code == 200
|
||||
assert f'href="/clients/{cl.id}"' in resp.text
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Malformed FK form fields — these used to escape as HTTP 500
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_create_rejects_non_numeric_client_id(client: TestClient, owner) -> None:
|
||||
"""A non-numeric client_id is a 400 fragment, not a ValueError traceback."""
|
||||
resp = client.post(
|
||||
"/printers",
|
||||
data={
|
||||
"name": "Bad FK",
|
||||
"ip_address": "10.0.5.1",
|
||||
"port_name": "IP_10_0_5_1",
|
||||
"client_id": "abc",
|
||||
},
|
||||
follow_redirects=False,
|
||||
)
|
||||
assert resp.status_code == 400
|
||||
assert "Invalid client id" in resp.text
|
||||
|
||||
|
||||
def test_create_rejects_non_numeric_driver_id(client: TestClient, owner) -> None:
|
||||
resp = client.post(
|
||||
"/printers",
|
||||
data={
|
||||
"name": "Bad FK",
|
||||
"ip_address": "10.0.5.2",
|
||||
"port_name": "IP_10_0_5_2",
|
||||
"driver_id": "not-an-id",
|
||||
},
|
||||
follow_redirects=False,
|
||||
)
|
||||
assert resp.status_code == 400
|
||||
assert "Invalid driver id" in resp.text
|
||||
|
||||
|
||||
def test_create_rejects_unknown_driver_id(client: TestClient, owner) -> None:
|
||||
"""An unknown driver id is a 404 fragment, not a FOREIGN KEY IntegrityError."""
|
||||
resp = client.post(
|
||||
"/printers",
|
||||
data={
|
||||
"name": "Ghost Driver",
|
||||
"ip_address": "10.0.5.3",
|
||||
"port_name": "IP_10_0_5_3",
|
||||
"driver_id": "9999",
|
||||
},
|
||||
follow_redirects=False,
|
||||
)
|
||||
assert resp.status_code == 404
|
||||
assert "Driver 9999 not found" in resp.text
|
||||
|
||||
|
||||
def test_update_rejects_unknown_driver_id(client: TestClient, owner) -> None:
|
||||
from imptune.db.models import Printer
|
||||
|
||||
printer = Printer.create(
|
||||
name="Patch Me",
|
||||
ip_address="10.0.5.4",
|
||||
port_name="IP_10_0_5_4",
|
||||
owner=owner,
|
||||
)
|
||||
resp = client.patch(
|
||||
f"/printers/{printer.id}",
|
||||
data={"name": "Patch Me", "driver_id": "9999"},
|
||||
)
|
||||
assert resp.status_code == 404
|
||||
assert "Driver 9999 not found" in resp.text
|
||||
# The printer must be untouched
|
||||
assert Printer.get_by_id(printer.id).driver is None
|
||||
|
||||
|
||||
def test_update_rejects_non_numeric_driver_id(client: TestClient, owner) -> None:
|
||||
from imptune.db.models import Printer
|
||||
|
||||
printer = Printer.create(
|
||||
name="Patch Me Too",
|
||||
ip_address="10.0.5.5",
|
||||
port_name="IP_10_0_5_5",
|
||||
owner=owner,
|
||||
)
|
||||
resp = client.patch(
|
||||
f"/printers/{printer.id}",
|
||||
data={"name": "Patch Me Too", "driver_id": "??"},
|
||||
)
|
||||
assert resp.status_code == 400
|
||||
assert "Invalid driver id" in resp.text
|
||||
|
||||
@@ -125,3 +125,146 @@ def test_health_endpoint_does_not_create_owner_rows(client):
|
||||
client.get("/health")
|
||||
after = Owner.select().count()
|
||||
assert after == before
|
||||
|
||||
|
||||
def test_static_assets_do_not_create_owner_rows(client):
|
||||
"""Asset fetches are unscoped — they must not mint an Owner row each time."""
|
||||
from imptune.db.models import Owner
|
||||
|
||||
client.get("/") # first visit provisions exactly one Owner
|
||||
before = Owner.select().count()
|
||||
for _ in range(5):
|
||||
assert client.get("/static/app.css").status_code == 200
|
||||
client.get("/favicon.ico")
|
||||
assert Owner.select().count() == before
|
||||
|
||||
|
||||
def test_restore_accepts_https_origin_behind_tls_proxy(tmp_data_dir):
|
||||
"""A TLS-terminating proxy leaves uvicorn seeing http:// while the browser
|
||||
sends Origin: https://host. Comparing schemes rejected every real restore."""
|
||||
from imptune.main import app
|
||||
|
||||
with TestClient(app) as client_a:
|
||||
client_a.get("/")
|
||||
client_a.post(
|
||||
"/printers",
|
||||
data={"name": "Proxied Printer", "ip_address": "10.0.0.9", "port_name": "IP_P"},
|
||||
follow_redirects=False,
|
||||
)
|
||||
key = client_a.get("/session/key/download").text
|
||||
|
||||
with TestClient(app) as client_new:
|
||||
resp = client_new.post(
|
||||
"/session/restore",
|
||||
data={"key": key},
|
||||
headers={"origin": "https://testserver"},
|
||||
follow_redirects=False,
|
||||
)
|
||||
assert resp.status_code == 303
|
||||
assert "Proxied Printer" in client_new.get("/printers").text
|
||||
|
||||
|
||||
def test_restore_still_rejects_foreign_host_origin(client):
|
||||
"""Host is what's checked — a same-scheme attacker host must still fail."""
|
||||
resp = client.post(
|
||||
"/session/restore",
|
||||
data={"key": "irrelevant"},
|
||||
headers={"origin": "http://attacker.example"},
|
||||
)
|
||||
assert resp.status_code == 403
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# COOKIE_SECURE=false — usable, remembered, but memory-only + warned about
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _set_cookie_header(resp):
|
||||
return next(
|
||||
v for k, v in resp.headers.multi_items() if k.lower() == "set-cookie"
|
||||
)
|
||||
|
||||
|
||||
def test_insecure_mode_issues_a_browser_session_cookie(client, monkeypatch):
|
||||
"""No Max-Age/Expires: the browser keeps the key in memory and drops it on
|
||||
exit, instead of writing a 10-year plaintext bearer credential to disk."""
|
||||
import imptune.config as cfg
|
||||
|
||||
monkeypatch.setattr(cfg, "COOKIE_SECURE", False)
|
||||
resp = client.get("/")
|
||||
|
||||
header = _set_cookie_header(resp)
|
||||
assert COOKIE_NAME in header
|
||||
assert "Max-Age" not in header
|
||||
assert "Expires" not in header
|
||||
assert "Secure" not in header
|
||||
|
||||
|
||||
def test_secure_mode_still_issues_a_persistent_cookie(tmp_data_dir, monkeypatch):
|
||||
from imptune.main import app
|
||||
|
||||
import imptune.config as cfg
|
||||
|
||||
monkeypatch.setattr(cfg, "COOKIE_SECURE", True)
|
||||
with TestClient(app) as fresh:
|
||||
header = _set_cookie_header(fresh.get("/"))
|
||||
|
||||
assert "Max-Age=" in header
|
||||
assert "Secure" in header
|
||||
|
||||
|
||||
def test_insecure_mode_still_remembers_configs_within_the_session(client, monkeypatch):
|
||||
"""The whole point: memory-only ≠ stateless. Same browser, same data."""
|
||||
import imptune.config as cfg
|
||||
|
||||
monkeypatch.setattr(cfg, "COOKIE_SECURE", False)
|
||||
client.post(
|
||||
"/printers",
|
||||
data={"name": "Ephemeral Printer", "ip_address": "10.0.6.1", "port_name": "IP_E"},
|
||||
follow_redirects=False,
|
||||
)
|
||||
assert "Ephemeral Printer" in client.get("/printers").text
|
||||
|
||||
|
||||
def test_insecure_mode_warns_the_user(client, monkeypatch):
|
||||
import imptune.config as cfg
|
||||
|
||||
monkeypatch.setattr(cfg, "COOKIE_SECURE", False)
|
||||
html = client.get("/printers").text
|
||||
|
||||
assert "ephemeral-session-warning" in html
|
||||
assert "only while this browser stays open" in html
|
||||
assert "/session/key/download" in html
|
||||
|
||||
|
||||
def test_secure_mode_shows_no_warning(tmp_data_dir, monkeypatch):
|
||||
from imptune.main import app
|
||||
|
||||
import imptune.config as cfg
|
||||
|
||||
monkeypatch.setattr(cfg, "COOKIE_SECURE", True)
|
||||
with TestClient(app) as fresh:
|
||||
assert "ephemeral-session-warning" not in fresh.get("/").text
|
||||
|
||||
|
||||
def test_restore_also_issues_a_session_cookie_when_insecure(tmp_data_dir, monkeypatch):
|
||||
"""The restore route sets its own cookie — it must honour the same policy."""
|
||||
from imptune.main import app
|
||||
|
||||
import imptune.config as cfg
|
||||
|
||||
monkeypatch.setattr(cfg, "COOKIE_SECURE", False)
|
||||
with TestClient(app) as client_a:
|
||||
client_a.get("/")
|
||||
key = client_a.get("/session/key/download").text
|
||||
|
||||
with TestClient(app) as client_new:
|
||||
resp = client_new.post(
|
||||
"/session/restore",
|
||||
data={"key": key},
|
||||
headers={"origin": "http://testserver"},
|
||||
follow_redirects=False,
|
||||
)
|
||||
assert resp.status_code == 303
|
||||
header = _set_cookie_header(resp)
|
||||
assert "Max-Age" not in header and "Expires" not in header
|
||||
|
||||
Reference in New Issue
Block a user