From 8bb2446165b16e86b02fa9df12b58d1eb3b376b5 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Sun, 23 Aug 2026 21:15:16 -0400 Subject: [PATCH 1/2] feat(web): static asset serving and the full security header set (#535) Adds keel/web/staticfiles.py, a directory-traversal-safe resolver for files under keel/web/static/ (containment via resolve()+relative_to(), not string matching -- see the module docstring for why Path's `/` operator alone is not a safe join), plus one placeholder asset so the path is exercised end to end. #536's real client does not exist yet. Wires /static/* into keel/web/server.py behind the SAME admission the rendered pages already require (host header, session cookie) -- never weakened just because today's placeholder holds no secrets. Serves it with a new header set distinct from the rendered pages' _SECURITY_HEADERS: CSP (default-src 'self'; connect-src 'self') only on text/html, per the design spec ("CSP belongs on text/html responses only; it is invalid and discouraged on other content types"); nosniff and Referrer-Policy: no-referrer unconditionally; Strict-Transport-Security deliberately never sent (loopback HTTP is not a TLS downgrade target -- reasoning recorded in a comment above the header sets so nobody adds it back). Adds a third CSRF layer: every POST now requires X-Keel-Client: 1 (a custom header forces a CORS preflight a hostile origin cannot satisfy -- the attack this closes is the HTML form POST, which is never preflighted, unlike SameSite/CSRF-token checks that assume it is), paired with a Sec-Fetch-Site: same-origin check where the header is present (missing is accepted for older browsers; a wrong value is refused, since page JS cannot forge it). Extends pyproject.toml's `artifacts` glob to keel/web/static/**, but the comment beside it records what was actually measured: the pinned uv_build (0.11.31) ships the whole module tree by default regardless of this key, so a new packaging test builds a real wheel with `uv build` and inspects the zip directly rather than trusting the glob or the source tree. All five existing layers in keel/web/security.py are untouched (0-line diff). Every prior test in tests/web/test_server.py passes unmodified -- the shared _request() helper gained optional, backward-compatible parameters instead. tests/commands/test_console_thinness.py's Rule 5 import allowlist gained one entry (staticfiles, urllib.parse) for the same no-I/O reason server.py already has one, since that rule scans the whole keel/web/ package. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NyeggYtojNXCTHeD3JHxb6 --- keel/web/server.py | 125 +++++++++++- keel/web/static/index.html | 17 ++ keel/web/staticfiles.py | 99 ++++++++++ pyproject.toml | 18 +- tests/commands/test_console_thinness.py | 16 +- tests/test_packaging.py | 60 ++++++ tests/web/test_server.py | 251 ++++++++++++++++++++++++ tests/web/test_staticfiles.py | 129 ++++++++++++ 8 files changed, 705 insertions(+), 10 deletions(-) create mode 100644 keel/web/static/index.html create mode 100644 keel/web/staticfiles.py create mode 100644 tests/web/test_staticfiles.py diff --git a/keel/web/server.py b/keel/web/server.py index 582254b..cb1a9fa 100644 --- a/keel/web/server.py +++ b/keel/web/server.py @@ -45,7 +45,7 @@ from typing import Any from urllib.parse import parse_qs, quote, urlsplit -from keel.web import render +from keel.web import render, staticfiles from keel.web.security import ( SESSION_COOKIE, HostPolicy, @@ -365,6 +365,51 @@ def _close(repo: Any) -> None: ) +#: The header set for `/static/*` (#535), separate from `_SECURITY_HEADERS` above because the +#: two routes need different values for the SAME header, not merely an additional one. +#: `_SECURITY_HEADERS`'s `default-src 'none'` is correct for the rendered pages -- they ship no +#: script, no style file, no image, nothing to permit -- but #536's client is exactly the thing +#: `'none'` forbids: its own JS, its own CSS, its own icons, all same-origin. `'self'` is the +#: tightest policy that still allows that, and `connect-src 'self'` on top of it is the specific +#: guarantee the design spec asks for: the interface is provably incapable of sending positions, +#: equity or trade history anywhere but this local process, checkable in the response headers +#: rather than merely promised. +#: +#: `Referrer-Policy` and `X-Content-Type-Options` are unconditional -- both are meaningful (and +#: harmless) on any content type. CSP is NOT: RFC-wise it is a response header with no defined +#: meaning outside a browsing context, so it is applied only where the content type is +#: `text/html` (see `_static_headers` below), matching the design spec's "CSP belongs on +#: `text/html` responses only; it is invalid and discouraged on other content types." +_STATIC_BASE_HEADERS: tuple[tuple[str, str], ...] = ( + ("X-Content-Type-Options", "nosniff"), + ("Referrer-Policy", "no-referrer"), +) + +# `Strict-Transport-Security` is deliberately absent from both header sets above, on every +# response this server ever sends -- recorded here so nobody adds it back reading only the +# acceptance checklist. `keel serve` binds loopback HTTP by design +# (`docs/superpowers/specs/2026-08-23-web-ui-rewrite-design.md`'s secure-context argument: +# `http://127.0.0.1` is a secure context by specification, so the service worker, manifest and +# browser install all work with no TLS decision required). HSTS exists to upgrade a site that +# COULD be intercepted on the way to a plaintext connection; there is no "on the way" here, and +# pinning `max-age` on loopback would only ever matter if this process later bound a +# non-loopback address (an operator's own choice, already warned about loudly in `serve()` +# below) -- at which point a stale HSTS pin from an earlier loopback run would be actively +# wrong: forcing HTTPS at an address that was never issued a certificate. + + +def _static_headers(content_type: str) -> tuple[tuple[str, str], ...]: + """`_STATIC_BASE_HEADERS` plus CSP, but ONLY when `content_type` is `text/html` -- see the + comment on `_STATIC_BASE_HEADERS` for why the other static content types get no CSP at all, + not a looser one.""" + if content_type.startswith("text/html"): + return ( + ("Content-Security-Policy", "default-src 'self'; connect-src 'self'"), + *_STATIC_BASE_HEADERS, + ) + return _STATIC_BASE_HEADERS + + class KeelHandler(BaseHTTPRequestHandler): """GET and HEAD. No other verb is implemented, so no other verb reaches keel.""" @@ -443,6 +488,70 @@ def _admitted(self) -> bool: return False return True + def _client_header_ok(self) -> bool: + """The third CSRF layer (#535), checked only for `POST` -- `SameSite=Strict` plus the + HMAC CSRF token already assume a hostile write is PREFLIGHTED, and both `SameSite` + parsing and CSRF-token exfiltration have had real bypasses over the years. A plain HTML + `
` is never preflighted, in any browser, which is exactly the gap: a + custom header (`X-Keel-Client: 1`) forces the preflight a form cannot trigger, and this + server answers no `Access-Control-*` header to any origin (there is no CORS + configuration at all), so a hostile origin's preflight cannot succeed either. + + `Sec-Fetch-Site` is checked second and differently: page JavaScript can set an arbitrary + `X-Keel-Client` value, but it cannot set or override `Sec-Fetch-Site` -- the browser + supplies it. So a WRONG value here is treated as real evidence of a cross-origin request + and refused, while a MISSING value is not: the header is a Fetch Metadata addition + (Safari shipped it later than Chrome/Firefox), and refusing every browser that predates + it would turn a defence-in-depth layer into an availability bug for exactly the users + who most need every OTHER layer to hold.""" + if self.headers.get("X-Keel-Client") != "1": + self._refuse( + 403, + "Refused", + "This request is missing the header keel's own client always sends. A plain " + "HTML form cannot add it, which is the point.", + ) + return False + sec_fetch_site = self.headers.get("Sec-Fetch-Site") + if sec_fetch_site is not None and sec_fetch_site != "same-origin": + self._refuse( + 403, + "Refused", + "This request's Sec-Fetch-Site header says it did not originate on this page.", + ) + return False + return True + + def _serve_static(self, url_path: str) -> None: + """One file under `staticfiles.STATIC_PREFIX` (#535), or the same 404 an unmapped + `ROUTES` path gets -- containment and the Content-Type table are `staticfiles`'s job + (`tests/web/test_staticfiles.py` pins the resolver in isolation); this method's only + responsibility is refusing anything it returns `None` for, uniformly, so a missing + static file and a missing page look identical to a client probing the server.""" + resolved = staticfiles.resolve_static_asset(staticfiles.STATIC_ROOT, url_path) + content_type = staticfiles.content_type_for(resolved) if resolved is not None else None + if resolved is None or content_type is None: + self._refuse(404, "No such page", f"Nothing is served at {url_path}.") + return + + try: + # A second filesystem race between `resolve_static_asset`'s existence check and this + # read (the file removed, a permission change) must be a clean 500 like any other + # broken page (`test_a_broken_page_does_not_take_the_server_down`'s guarantee), + # never an uncaught exception that leaves the connection hanging. + payload = resolved.read_bytes() + except OSError as exc: + self._refuse(500, "That file could not be read", f"{type(exc).__name__}: {exc}") + return + self.send_response(200) + self.send_header("Content-Type", content_type) + self.send_header("Content-Length", str(len(payload))) + for name, value in _static_headers(content_type): + self.send_header(name, value) + self.end_headers() + if self.command != "HEAD": + self.wfile.write(payload) + # -- the request -- def do_HEAD(self) -> None: # noqa: N802 - stdlib's naming, not ours self.do_GET() @@ -450,7 +559,7 @@ def do_HEAD(self) -> None: # noqa: N802 - stdlib's naming, not ours def do_POST(self) -> None: # noqa: N802 - stdlib's naming, not ours """The ENTIRE write surface (#437). Read `keel/web/__init__.py` before extending it. - Four refusals before anything is performed, and then a lookup in a closed set. There is + Five refusals before anything is performed, and then a lookup in a closed set. There is no dynamic dispatch here, no getattr on a user-supplied name, and no path that reaches keel other than `keel.commands.setup.ACTIONS` -- which contains three idempotent, non-destructive, `MECHANICAL` steps and cannot contain anything else without failing a @@ -459,6 +568,9 @@ def do_POST(self) -> None: # noqa: N802 - stdlib's naming, not ours if not self._admitted(): return + if not self._client_header_ok(): + return + if not parsed.path.startswith(SETUP_ACTION_PREFIX): # Not "method not allowed" -- there is no write surface at this path at all, and # saying so is both true and less informative to someone probing. @@ -548,6 +660,15 @@ def do_GET(self) -> None: # noqa: N802 - stdlib's naming, not ours if not self._admitted(): return + if parsed.path.startswith(staticfiles.STATIC_PREFIX): + # Same admission as every rendered page (never weakened): a static asset is not + # exempted from the loopback-plus-session model just because it holds no secrets + # today. #536's client is what actually reads these, and it authenticates the same + # way any other fetch from this origin does -- the session cookie already on the + # request. + self._serve_static(parsed.path) + return + handler = ROUTES.get(parsed.path) if handler is None: self._refuse(404, "No such page", f"Nothing is served at {parsed.path}.") diff --git a/keel/web/static/index.html b/keel/web/static/index.html new file mode 100644 index 0000000..7d621df --- /dev/null +++ b/keel/web/static/index.html @@ -0,0 +1,17 @@ + + + + +keel + + + +

keel's static asset serving is live. The client that uses it (#536) is not built yet.

+ + diff --git a/keel/web/staticfiles.py b/keel/web/staticfiles.py new file mode 100644 index 0000000..5fc86c4 --- /dev/null +++ b/keel/web/staticfiles.py @@ -0,0 +1,99 @@ +"""Static asset serving for `keel serve` (#535). + +The client that consumes this -- plain ES modules, a stylesheet, icons -- is #536 and does not +exist yet. What ships here is the serving capability itself, on the same origin as the existing +rendered routes (§"Static assets and the API share ONE origin" in the design spec), plus one +placeholder asset so the path is exercised end to end rather than merely reachable in theory. + +**Why a hand-written resolver and not `http.server.SimpleHTTPRequestHandler`.** The stdlib +handler roots itself at the process's current working directory, which is wherever `keel serve` +happened to be launched from -- not a fixed location inside the package. Wiring it up safely +would mean overriding `translate_path` anyway, at which point there is nothing left to reuse. + +**The traversal defence is `resolve()` then `relative_to()`, not string matching.** A check for +the literal substring `".."` is a losing game against encoding (`%2e%2e`), redundant separators, +and symlinks, and it also has a sharper failure mode that is easy to miss entirely: `Path`'s `/` +operator DISCARDS the left side when the right side is absolute -- +`Path("/srv/static") / "/etc/passwd"` is `Path("/etc/passwd")`, not an error and not a joined +path. A resolver that only rejected strings containing `".."` would serve `/etc/passwd` to a +request for `/static//etc/passwd` and never notice. Resolving the candidate against the +filesystem and then asking whether it is still `relative_to` the static root catches both +failure modes with one check, because it asks the only question that matters: where does this +path ACTUALLY point, not what does its spelling suggest. +""" + +from __future__ import annotations + +from pathlib import Path +from urllib.parse import unquote + +#: Where the shipped assets live, inside the installed package. A wheel build ships this +#: directory because `pyproject.toml`'s `artifacts` glob (`keel/web/static/**`) says to; without +#: that glob the directory exists in a checkout and nowhere else, which is the exact bug #535 +#: exists to close (see the comment beside that glob). +STATIC_ROOT = Path(__file__).resolve().parent / "static" + +#: The URL prefix static requests are served under. Kept off `/` and the rest of `server.py`'s +#: `ROUTES` table on purpose: the rendered pages still own the root paths today, and #536's +#: client does not exist yet, so nothing here can collide with current routing. When the client +#: lands, its entry point is served from under this prefix like everything else it ships. +STATIC_PREFIX = "/static/" + +#: Content-Type by extension, spelled out rather than left to `mimetypes.guess_type`. The stdlib +#: table is OS-dependent -- some platforms still answer a `.js` file with `text/plain` -- and a +#: wrong type paired with `X-Content-Type-Options: nosniff` (added below) is a script the +#: browser refuses to RUN rather than one it sniffs its way around the mistake for. So the type +#: has to be correct, not merely present, and an extension absent from this table is refused +#: (404) rather than guessed: a "correct" 404 is safer than a wrong Content-Type served with +#: confidence. +_CONTENT_TYPES: dict[str, str] = { + ".html": "text/html; charset=utf-8", + ".css": "text/css; charset=utf-8", + ".js": "text/javascript; charset=utf-8", + ".mjs": "text/javascript; charset=utf-8", + ".json": "application/json; charset=utf-8", + ".webmanifest": "application/manifest+json", + ".svg": "image/svg+xml", + ".png": "image/png", + ".ico": "image/x-icon", + ".txt": "text/plain; charset=utf-8", +} + + +def content_type_for(path: Path) -> str | None: + """The Content-Type for a static file, by its extension. `None` for an extension this server + does not know how to label -- the caller's job is to refuse, not to guess.""" + return _CONTENT_TYPES.get(path.suffix.lower()) + + +def resolve_static_asset(root: Path, url_path: str) -> Path | None: + """A file inside `root` for a request path under `STATIC_PREFIX`, or `None`. + + `None` covers three cases the caller must treat identically -- as a 404, never as a 500 or + as "serve something close enough": the path does not start with the static prefix, the + computed target does not exist or is not a plain file, or the target would land outside + `root` once symlinks and `..` segments are resolved. Decoded ONCE (`unquote`), matching + `http.server`'s own behaviour and the browser's: a doubly-encoded `%252e%252e` decodes here + to the literal, inert string `%2e%2e`, not to `..`. + """ + if not url_path.startswith(STATIC_PREFIX): + return None + rel = unquote(url_path[len(STATIC_PREFIX) :]) + if not rel or "\x00" in rel: + # A null byte truncates a C string in some filesystem APIs beneath Python's own; refused + # here rather than let `resolve()` raise it as an uncaught `ValueError` a layer up. + return None + + root_resolved = root.resolve() + try: + # See the module docstring: `root / rel` alone is not a safe join when `rel` can be + # absolute. `resolve()` then `relative_to()` is what actually enforces containment, + # regardless of how `rel` got here. + candidate = (root / rel).resolve() + candidate.relative_to(root_resolved) + except ValueError: + return None + + if not candidate.is_file(): + return None + return candidate diff --git a/pyproject.toml b/pyproject.toml index 5e00242..6a44aae 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -44,8 +44,22 @@ module-root = "" # `keel_trader/` from the distribution name. module-name = "keel" # Ship the default config template inside the wheel so `keel init-config` can write it out on a -# fresh install (the wheel otherwise contains only .py files). -artifacts = ["keel/templates/*.yaml"] +# fresh install, and (#535) the web UI's static assets so `keel serve` has something to serve +# from an installed wheel rather than a checkout. +# +# ⚠️ Measured against the pinned uv_build (`uv_build>=0.10.4,<0.13.0`, resolving to 0.11.31 as +# of #535): this key has NO observable effect. `artifacts` does not appear in uv_build's +# documented `[tool.uv.build-backend]` schema (`module-root`, `module-name`, `namespace`, `data`, +# `source-include`, `source-exclude`, `wheel-exclude`, `default-excludes`), and removing this +# line entirely, or replacing its value with garbage, still ships both the templates and +# `keel/web/static/**` in a rebuilt wheel with no warning either way -- uv_build's actual default +# is "the whole module tree except `default-excludes`" (`__pycache__`, `*.pyc`, `*.pyo`), not +# "importable source only". `tests/test_packaging.py::test_static_assets_survive_being_built_ +# into_a_wheel` therefore checks the built wheel directly rather than trusting this line, and +# would catch it if a future uv_build (or a `wheel-exclude` added here later) narrowed that +# default. Left in place as a statement of intent and because it is the field this project's +# prior art used for the same purpose -- but it is not, today, load-bearing. +artifacts = ["keel/templates/*.yaml", "keel/web/static/**"] [tool.uv.workspace] members = ["packages/*"] diff --git a/tests/commands/test_console_thinness.py b/tests/commands/test_console_thinness.py index 2133aaf..7433f0c 100644 --- a/tests/commands/test_console_thinness.py +++ b/tests/commands/test_console_thinness.py @@ -76,18 +76,22 @@ def _console_module_paths() -> list[str]: return sorted(set(paths)) -#: Rule 5's one entry-scoped exception, in the same shape as every other allowance here: +#: Rule 5's entry-scoped exceptions, in the same shape as every other allowance here: #: (module stem, imported module). #: #: Rule 5 bans `urllib` by ROOT, which is the right coarseness for a rule about network egress -- #: `urllib.request.urlopen` is the thing it exists to stop. But `urllib.parse` performs no I/O at #: all: it is string manipulation, and it is how `keel/web/server.py` splits a request path from -#: its query string. The alternative was hand-rolling percent-decoding on attacker-influenced -#: input, which is a strictly worse trade than one named, scoped allowance. +#: its query string, and (#535) how `keel/web/staticfiles.py` decodes one static-asset path +#: segment before checking it stays inside the static root. The alternative in both cases was +#: hand-rolling percent-decoding on attacker-influenced input, which is a strictly worse trade +#: than one named, scoped allowance. #: -#: Scoped to the module and the exact import, so it cannot widen: `urllib.request` in the same -#: file still fails, and `urllib.parse` anywhere else still fails. -RULE5_IMPORT_ALLOWLIST: frozenset[tuple[str, str]] = frozenset({("server", "urllib.parse")}) +#: Scoped to the module and the exact import, so it cannot widen: `urllib.request` in either file +#: still fails, and `urllib.parse` anywhere else still fails. +RULE5_IMPORT_ALLOWLIST: frozenset[tuple[str, str]] = frozenset( + {("server", "urllib.parse"), ("staticfiles", "urllib.parse")} +) #: The compute trees -- where sizing/backtest/gate/screening/reporting math lives. The diff --git a/tests/test_packaging.py b/tests/test_packaging.py index 463908e..1d2679e 100644 --- a/tests/test_packaging.py +++ b/tests/test_packaging.py @@ -75,6 +75,66 @@ def test_the_dev_only_fake_venue_is_not_a_runtime_dependency_of_anything(): assert "keel-broker-fake" not in deps, f"{name} must not depend on keel-broker-fake" +# -- what actually survives packaging, not what the source tree merely holds -------------------- + + +def test_static_assets_survive_being_built_into_a_wheel(tmp_path: Path) -> None: + """Whether `keel/web/static/` ships is a fact about the BUILT wheel, never about the source + tree, and only inspecting a real build can state it (#535). + + Asserting the source tree holds these files (a bare `Path.exists`) would prove nothing: they + are on disk in every checkout regardless of what the packaging step does with them. + `pyproject.toml`'s `artifacts` glob states the intent to ship them, but -- measured directly, + see the comment beside that glob -- it turns out to have no effect on the currently pinned + `uv_build`, which already ships the whole module tree by default. That measurement is exactly + why this test builds a REAL wheel with the project's own build backend (`uv build`, the same + command `release.yml` runs) and inspects the zip it actually produces, rather than trusting + either the glob or the source tree: it is a regression guard against uv_build narrowing that + default, or a future `wheel-exclude` catching these files, not a check on the glob's syntax. + """ + import subprocess + import zipfile + + out_dir = tmp_path / "dist" + subprocess.run( + [ + "uv", + "build", + "--wheel", + "--package", + "keel-trader", + "--out-dir", + str(out_dir), + "--no-build-logs", + ], + cwd=_ROOT, + check=True, + capture_output=True, + text=True, + timeout=120, + ) + wheels = sorted(out_dir.glob("keel_trader-*.whl")) + assert wheels, f"`uv build` produced no wheel in {out_dir}" + + with zipfile.ZipFile(wheels[0]) as archive: + shipped = set(archive.namelist()) + + static_dir = _ROOT / "keel" / "web" / "static" + on_disk = sorted( + p.relative_to(_ROOT).as_posix() for p in static_dir.rglob("*") if p.is_file() + ) + assert on_disk, ( + f"no files under {static_dir} on disk -- this test would prove nothing about packaging" + ) + for source in on_disk: + assert source in shipped, ( + f"{source} exists in the source tree but is absent from the built wheel " + f"({wheels[0].name}) -- keel/web/static is no longer surviving packaging, whatever " + "the reason (uv_build's default module-tree inclusion narrowed, a wheel-exclude " + "pattern now catches it, or the file moved outside the module root)" + ) + + #: The flag that identifies a strict override. `strict = true` cannot be used in a per-module #: section -- mypy applies it GLOBALLY whatever `module` the section names -- so the strict #: packages spell the bundle out flag by flag instead, and this is the one that best marks the diff --git a/tests/web/test_server.py b/tests/web/test_server.py index 1aae28a..0d16ae1 100644 --- a/tests/web/test_server.py +++ b/tests/web/test_server.py @@ -81,11 +81,23 @@ def _request( cookie: str | None = None, host: str | None = None, form: dict[str, str] | None = None, + client_header: str | None = "1", + sec_fetch_site: str | None = None, ) -> tuple[int, dict[str, str], str]: + """`client_header` defaults to `"1"` and is only sent on `POST`: every pre-existing POST test + in this module goes through here without knowing #535's `X-Keel-Client` layer exists, and + `"1"` is what a real browser's `fetch` wrapper will always send (per the design spec's + `api.js`), so the happy-path tests stay a faithful stand-in for that client without editing + each one. Tests of the new layer itself pass `client_header=None` (omitted entirely) or an + explicit wrong value, and `sec_fetch_site=...` to exercise that half of the check.""" conn = http.client.HTTPConnection(cfg.host, cfg.port, timeout=10) headers = {"Host": host if host is not None else f"{cfg.host}:{cfg.port}"} if cookie: headers["Cookie"] = cookie + if method == "POST" and client_header is not None: + headers["X-Keel-Client"] = client_header + if sec_fetch_site is not None: + headers["Sec-Fetch-Site"] = sec_fetch_site body = None if form is not None: body = urlencode(form) @@ -401,6 +413,115 @@ def test_the_csrf_token_is_not_the_session_token( assert csrf_token(running.token) in body +# -- the client header (#535's third CSRF layer) ----------------------------------------------- +# +# `SameSite=Strict` and the HMAC CSRF token both assume a hostile cross-origin write is +# PREFLIGHTED -- but a plain `` is not, in any browser, ever. A custom request +# header forces the preflight a form cannot trigger; a hostile origin's script could still try to +# add the header, but doing so makes the request cross-origin-with-a-custom-header, which is +# exactly the shape CORS preflights and which this server never answers with a matching +# `Access-Control-Allow-*` (there is no CORS configuration at all -- see server.py's module +# docstring). `Sec-Fetch-Site` is the second half: it cannot be set by page JavaScript at all, so +# a wrong value (not merely a missing one -- older browsers omit the header) is real evidence of +# a cross-origin request and is refused; a missing value is not evidence of anything. + + +def test_a_write_without_the_client_header_is_refused( + empty_machine: web_server.ServeConfig, +) -> None: + status, _headers, _body = _request( + empty_machine, + "/setup/config", + method="POST", + cookie=_session(empty_machine), + form={"csrf": _csrf(empty_machine)}, + client_header=None, + ) + assert status == 403 + assert not Path(empty_machine.config_path).exists() + + +@pytest.mark.parametrize("value", ["0", "true", "yes", "2", ""]) +def test_a_wrong_client_header_value_is_refused( + empty_machine: web_server.ServeConfig, value: str +) -> None: + status, _headers, _body = _request( + empty_machine, + "/setup/config", + method="POST", + cookie=_session(empty_machine), + form={"csrf": _csrf(empty_machine)}, + client_header=value, + ) + assert status == 403, value + assert not Path(empty_machine.config_path).exists() + + +def test_a_write_missing_sec_fetch_site_is_accepted( + empty_machine: web_server.ServeConfig, +) -> None: + """Older browsers never send `Sec-Fetch-Site` at all; its absence must not be a refusal, or + every one of them would be locked out of the one write surface that exists.""" + status, _headers, _body = _request( + empty_machine, + "/setup/config", + method="POST", + cookie=_session(empty_machine), + form={"csrf": _csrf(empty_machine)}, + sec_fetch_site=None, + ) + assert status == 303 + + +def test_a_write_with_sec_fetch_site_same_origin_is_accepted( + empty_machine: web_server.ServeConfig, +) -> None: + status, _headers, _body = _request( + empty_machine, + "/setup/config", + method="POST", + cookie=_session(empty_machine), + form={"csrf": _csrf(empty_machine)}, + sec_fetch_site="same-origin", + ) + assert status == 303 + + +@pytest.mark.parametrize("value", ["cross-site", "same-site", "none"]) +def test_a_write_with_a_wrong_sec_fetch_site_is_refused( + empty_machine: web_server.ServeConfig, value: str +) -> None: + """Page JavaScript cannot forge this header -- the browser sets it. A value present and + wrong is therefore real evidence of a cross-origin request, unlike a missing one.""" + status, _headers, _body = _request( + empty_machine, + "/setup/config", + method="POST", + cookie=_session(empty_machine), + form={"csrf": _csrf(empty_machine)}, + sec_fetch_site=value, + ) + assert status == 403, value + assert not Path(empty_machine.config_path).exists() + + +def test_the_client_header_check_runs_before_the_csrf_check( + empty_machine: web_server.ServeConfig, +) -> None: + """Admission (host, cookie), then this layer, then CSRF -- so a probe cannot distinguish + "wrong CSRF" from "missing client header" by anything other than both being 403, and the + cheaper check runs first.""" + status, _headers, _body = _request( + empty_machine, + "/setup/config", + method="POST", + cookie=_session(empty_machine), + form={"csrf": "not-the-token"}, + client_header=None, + ) + assert status == 403 + + # -- the actions themselves, over the wire ------------------------------------------------------ @@ -624,6 +745,136 @@ def test_head_returns_the_headers_without_a_body(running: web_server.ServeConfig assert int(headers["Content-Length"]) > 0 +# -- static assets (#535) ------------------------------------------------------------------- +# +# `keel/web/static/index.html` is the one placeholder asset the package ships (#536's real +# client does not exist yet). These drive it over the real wire -- a real bound server, a real +# `Path` on disk -- for the same reason `tests/web/test_staticfiles.py`'s module docstring +# gives: the properties worth pinning are properties of the served bytes, not of a resolver +# called directly. + + +def test_a_shipped_static_asset_is_served(running: web_server.ServeConfig) -> None: + status, headers, body = _request(running, "/static/index.html", cookie=_session(running)) + assert status == 200 + assert headers["Content-Type"] == "text/html; charset=utf-8" + assert "keel" in body.lower() + + +def test_static_assets_are_behind_the_same_admission_as_every_other_page( + running: web_server.ServeConfig, +) -> None: + """Never weakened: a static file is not exempted from the loopback-plus-session model just + because it holds no secrets today. The same guard that protects `/rules` protects this.""" + status, _headers, _body = _request(running, "/static/index.html") # no cookie + assert status == 403 + + +def test_a_missing_static_asset_is_a_404(running: web_server.ServeConfig) -> None: + status, _headers, _body = _request( + running, "/static/does-not-exist.html", cookie=_session(running) + ) + assert status == 404 + + +@pytest.mark.parametrize( + "path", + [ + "/static/../keel/web/security.py", + "/static/../../pyproject.toml", + "/static/%2e%2e/%2e%2e/pyproject.toml", + "/static//etc/passwd", + "/static/..%2f..%2fpyproject.toml", + ], +) +def test_directory_traversal_through_the_wire_is_refused( + running: web_server.ServeConfig, path: str +) -> None: + """The unit-level payloads live in `tests/web/test_staticfiles.py`; these are the same shape + of attack sent as an actual HTTP request line, over a real socket, to prove nothing between + the wire and the resolver -- URL parsing, `http.server`'s own request handling -- reopens + what the resolver closes.""" + status, _headers, body = _request(running, path, cookie=_session(running)) + assert status == 404, (path, body[:200]) + + +def test_a_static_html_response_carries_the_new_header_set( + running: web_server.ServeConfig, +) -> None: + """The full set from #535, on the one content type the design spec puts it on.""" + status, headers, _body = _request(running, "/static/index.html", cookie=_session(running)) + assert status == 200 + csp = headers["Content-Security-Policy"] + assert "default-src 'self'" in csp + assert "connect-src 'self'" in csp + assert headers["X-Content-Type-Options"] == "nosniff" + assert headers["Referrer-Policy"] == "no-referrer" + assert "Strict-Transport-Security" not in headers + + +def test_a_non_html_static_asset_carries_no_csp( + running: web_server.ServeConfig, monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """Per the design spec: CSP is invalid and discouraged on anything but `text/html`. A CSS or + JS asset still gets `nosniff` and `Referrer-Policy` -- those are meaningful on any content + type -- but no `Content-Security-Policy` header at all.""" + from keel.web import staticfiles + + (tmp_path / "style.css").write_text("body { color: black; }") + monkeypatch.setattr(staticfiles, "STATIC_ROOT", tmp_path) + + status, headers, _body = _request(running, "/static/style.css", cookie=_session(running)) + assert status == 200 + assert headers["Content-Type"] == "text/css; charset=utf-8" + assert "Content-Security-Policy" not in headers + assert headers["X-Content-Type-Options"] == "nosniff" + assert headers["Referrer-Policy"] == "no-referrer" + assert "Strict-Transport-Security" not in headers + + +def test_an_unrecognised_static_extension_is_refused( + running: web_server.ServeConfig, monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """A file with no entry in the Content-Type table is refused rather than guessed at -- see + `staticfiles.content_type_for`'s docstring.""" + from keel.web import staticfiles + + (tmp_path / "payload.exe").write_bytes(b"MZ") + monkeypatch.setattr(staticfiles, "STATIC_ROOT", tmp_path) + + status, _headers, _body = _request( + running, "/static/payload.exe", cookie=_session(running) + ) + assert status == 404 + + +def test_a_static_file_removed_between_resolve_and_read_is_a_500_not_a_hang( + running: web_server.ServeConfig, monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """`resolve_static_asset` proves the file existed at check time; a `read_bytes()` failure + afterwards (deleted, permission change) must land as a clean page, matching the guarantee + `test_a_broken_page_does_not_take_the_server_down` already pins for rendered routes.""" + from keel.web import staticfiles + + target = tmp_path / "flaky.html" + target.write_text("") + monkeypatch.setattr(staticfiles, "STATIC_ROOT", tmp_path) + + real_resolve = staticfiles.resolve_static_asset + + def _resolve_then_delete(root: Path, url_path: str) -> Path | None: + resolved = real_resolve(root, url_path) + if resolved is not None: + resolved.unlink() + return resolved + + monkeypatch.setattr(staticfiles, "resolve_static_asset", _resolve_then_delete) + + status, _headers, body = _request(running, "/static/flaky.html", cookie=_session(running)) + assert status == 500 + assert "OSError" in body or "FileNotFoundError" in body + + def test_the_activity_scope_comes_from_the_query_and_hostile_input_collapses_safely( running: web_server.ServeConfig, ) -> None: diff --git a/tests/web/test_staticfiles.py b/tests/web/test_staticfiles.py new file mode 100644 index 0000000..d761d54 --- /dev/null +++ b/tests/web/test_staticfiles.py @@ -0,0 +1,129 @@ +"""`keel/web/staticfiles.py` -- the resolver in isolation, against a synthetic root. + +The wire-level exercise of the same code lives in `tests/web/test_server.py`, against the one +placeholder asset the package actually ships. These tests are the ones that can try payloads a +real filesystem would not conveniently hold still for -- an escape through a directory that may +not even exist on the test machine -- without needing one to. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from keel.web import staticfiles + + +@pytest.fixture +def root(tmp_path: Path) -> Path: + (tmp_path / "index.html").write_text("hi") + (tmp_path / "style.css").write_text("body {}") + (tmp_path / "app.js").write_text("console.log(1)") + (tmp_path / "icon.svg").write_text("") + (tmp_path / "unknown.exe").write_bytes(b"MZ") + sub = tmp_path / "sub" + sub.mkdir() + (sub / "nested.html").write_text("nested") + # A sibling of the static root -- the thing every traversal payload below is reaching for. + (tmp_path.parent / "secret.txt").write_text("do not serve me") + return tmp_path + + +def test_a_plain_file_resolves(root: Path) -> None: + resolved = staticfiles.resolve_static_asset(root, "/static/index.html") + assert resolved == root / "index.html" + + +def test_a_nested_file_resolves(root: Path) -> None: + resolved = staticfiles.resolve_static_asset(root, "/static/sub/nested.html") + assert resolved == root / "sub" / "nested.html" + + +def test_a_path_missing_the_prefix_does_not_resolve(root: Path) -> None: + assert staticfiles.resolve_static_asset(root, "/index.html") is None + + +def test_a_missing_file_does_not_resolve(root: Path) -> None: + assert staticfiles.resolve_static_asset(root, "/static/nope.html") is None + + +def test_a_directory_is_not_a_file(root: Path) -> None: + assert staticfiles.resolve_static_asset(root, "/static/sub") is None + assert staticfiles.resolve_static_asset(root, "/static/sub/") is None + + +def test_the_bare_prefix_does_not_resolve(root: Path) -> None: + assert staticfiles.resolve_static_asset(root, "/static/") is None + + +# -- the entire point of the module ------------------------------------------------------------ + + +@pytest.mark.parametrize( + "payload", + [ + "/static/../secret.txt", + "/static/../../secret.txt", + "/static/sub/../../secret.txt", + "/static/%2e%2e/secret.txt", + "/static/%2e%2e%2f%2e%2e/secret.txt", + "/static//../secret.txt", + "/static/./../secret.txt", + # The specific footgun the module docstring names: `Path("/root") / "/etc/passwd"` + # discards the root entirely rather than raising. Both a raw absolute path and its + # percent-encoded spelling must be caught by the same containment check. + "/static//etc/passwd", + "/static/%2fetc%2fpasswd", + ], +) +def test_directory_traversal_payloads_never_escape_the_root(root: Path, payload: str) -> None: + assert staticfiles.resolve_static_asset(root, payload) is None + + +def test_a_null_byte_is_refused_not_raised(root: Path) -> None: + assert staticfiles.resolve_static_asset(root, "/static/index.html\x00.png") is None + + +def test_a_doubly_encoded_traversal_is_inert_not_decoded_twice(root: Path) -> None: + """`%252e%252e` decodes ONCE, here, to the literal string `%2e%2e` -- not to `..`. It must + therefore fail to resolve to any real file, exactly like any other nonsense path, rather + than be walked a second time into an escape.""" + assert staticfiles.resolve_static_asset(root, "/static/%252e%252e/secret.txt") is None + + +# -- content types ------------------------------------------------------------------------------ + + +@pytest.mark.parametrize( + ("name", "expected"), + [ + ("index.html", "text/html; charset=utf-8"), + ("style.css", "text/css; charset=utf-8"), + ("app.js", "text/javascript; charset=utf-8"), + ("worker.mjs", "text/javascript; charset=utf-8"), + ("manifest.webmanifest", "application/manifest+json"), + ("data.json", "application/json; charset=utf-8"), + ("icon.svg", "image/svg+xml"), + ("icon.png", "image/png"), + ("favicon.ico", "image/x-icon"), + ("notes.txt", "text/plain; charset=utf-8"), + ], +) +def test_known_extensions_map_to_the_right_content_type(name: str, expected: str) -> None: + assert staticfiles.content_type_for(Path(name)) == expected + + +def test_an_unknown_extension_has_no_content_type() -> None: + """No entry, not a guess. `mimetypes.guess_type` would happily return something for `.exe`; + the caller must refuse instead of serving a file it cannot correctly label.""" + assert staticfiles.content_type_for(Path("payload.exe")) is None + + +def test_an_unknown_extension_does_not_resolve_even_though_the_file_exists(root: Path) -> None: + """`resolve_static_asset` only proves the path is safe; `server.py` is expected to also + check `content_type_for` before serving. This pins that the file existing is not, by + itself, enough -- the type table is where an unrecognised kind gets refused.""" + resolved = staticfiles.resolve_static_asset(root, "/static/unknown.exe") + assert resolved == root / "unknown.exe" # resolves fine -- the type check is a SEPARATE gate + assert staticfiles.content_type_for(resolved) is None From f2a9c57cfd8e35b6c190d7b1003ffdd3044a760f Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Sun, 23 Aug 2026 22:01:34 -0400 Subject: [PATCH 2/2] fix(web): scope X-Keel-Client to /api/*, fix static CSP, ship desktop bundle assets Independent review found the previous commit gated EVERY POST -- including /setup/* -- behind X-Keel-Client. The shipped UI's only write path is render.py's plain , and _SECURITY_HEADERS ships no script-src at all: there is no code path by which that form can set a custom header. Every setup action (config, database, rules, credentials) returned 403, with no terminal to fall back to on the desktop bundle. My brief said "every state-changing POST"; the design spec says "every POST /api/*" and was right. Fix: X-Keel-Client is now checked only under the new API_PREFIX ("/api/", reserved for #533/534's still-unbuilt JSON API and #536's fetch()-based client). /setup/* keeps its existing defences (host validation, SameSite=Strict cookie, HMAC CSRF token, the closed action set) plus Sec-Fetch-Site: same-origin, which a real form POST DOES carry and which page JavaScript cannot forge -- unlike a custom header. A new test, test_a_browser_form_post_succeeds_without_the_api_client_header, posts to /setup/* exactly as a browser does (cookie, CSRF token, Origin, Sec-Fetch-Site: same-origin, no X-Keel-Client) and asserts success; verified it fails 403 against the prior commit and passes after this one. Also, from the same review: - The /static/* CSP was missing form-action, base-uri and frame-ancestors -- none of which inherit from default-src under CSP3 -- plus X-Frame-Options. A hostile was not a "connection" so connect-src 'self' did not stop it, and a missing base-uri let an injected retarget every relative URL. Now sends default-src 'self'; connect-src 'self'; form-action 'self'; base-uri 'none'; frame-ancestors 'none', plus X-Frame-Options: DENY, and the same CSP now also covers image/svg+xml (SVG is active content: opened directly, an inline