Skip to content

feat(web): serve static assets, with the headers that make the UI's reach checkable - #545

Open
eaitbrahim wants to merge 2 commits into
mainfrom
feat-535-static-headers
Open

feat(web): serve static assets, with the headers that make the UI's reach checkable#545
eaitbrahim wants to merge 2 commits into
mainfrom
feat-535-static-headers

Conversation

@eaitbrahim

Copy link
Copy Markdown
Contributor

Closes #535.

Adds static file serving plus the security headers the browser client will rely on. No existing security layer is modifiedkeel/web/security.py has a 0-line diff, and no existing test body was edited.

Headers, and where they go

New /static/* route:

header scope why
Content-Security-Policy: default-src 'self'; connect-src 'self' text/html only Makes the UI provably incapable of sending positions, equity or trade history anywhere but the local process. Browser-enforced, verifiable in seconds. Per spec, CSP is invalid and discouraged on non-HTML content types.
X-Content-Type-Options: nosniff every static response Content-type sniffing.
Referrer-Policy: no-referrer every static response The app will link out to keeltrading.com, and the session token rides in the URL until the cookie exchange.
Strict-Transport-Security never sent The origin is loopback HTTP by design. A standing comment records why, so nobody re-adds it.

The existing rendered pages keep their original _SECURITY_HEADERS (default-src 'none', X-Frame-Options: DENY) untouched — correct for zero-script pages.

A third CSRF layer

security.py already documents five layers, including a SameSite=Strict HttpOnly cookie and an HMAC-derived CSRF token described in-source as "the layer that does not depend on the browser being current." This adds one more, because SameSite has had parser bypasses.

Every POST now requires X-Keel-Client: 1, checked before the body is parsed. A custom header forces a CORS preflight a hostile origin cannot satisfy — and the specific attack it closes is the HTML form POST, which is not preflighted and therefore slips past checks that assume all cross-origin requests are.

Sec-Fetch-Site is refused when present and wrong (page JavaScript cannot forge it), and accepted when absent (older browsers).

Directory traversal

resolve_static_asset() resolves and checks containment via .relative_to()not string matching on "..".

Tested over the real wire and at unit level: /static/../secret.txt, ../../, %2e%2e, double-encoded %252e%252e (must stay inert, not decode twice), a null byte, and /static//etc/passwd — the last being the footgun the module docstring names, since Path("/root") / "/etc/passwd" discards the root entirely.

A measured correction to the packaging story

The issue asserted the wheel omits docs/ because artifacts lists only the YAML template. That was wrong, and I confirmed the correction independently.

pyproject.toml's artifacts key is inert on the pinned backend (uv_build>=0.10.4,<0.13.0). Building with the list and with artifacts = [] produces wheels with identical contents — 140 entries each, both carrying the two YAML templates. uv_build ships the whole module root regardless. The adjacent source comment claiming "the wheel otherwise contains only .py files" is wrong by the same measurement, and is rewritten here to state what was measured.

docs/ is absent because it lives at the repository root, outside keel/. That makes #539's case stronger: the empty glossary is structural, not a missing glob.

The keel/web/static/** entry is still added — it matches prior art and stated intent, and is harmless — but the test does not trust it. test_static_assets_survive_being_built_into_a_wheel runs uv build --wheel as a real subprocess and inspects the .whl with zipfile. Verified red/green against the actual failure mode.

One adjacent test changed, and why

test_console_thinness.py's Rule 5 scans keel/web/*.py and flagged staticfiles.py's from urllib.parse import unquote. Added one entry to its existing scoped allowlist, mirroring the ("server", "urllib.parse") entry already there and its rationale — pure string parsing, no I/O.

The capability-disjointness test and its AST-scan sibling are untouched and green. Nothing here widens what the browser may do.

Gates

  • ruff check keel tests packages — All checks passed
  • mypy — no issues in 355 source files
  • pytest -q4607 passed, 3 skipped

🤖 Generated with Claude Code

https://claude.ai/code/session_01NyeggYtojNXCTHeD3JHxb6

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NyeggYtojNXCTHeD3JHxb6
@eaitbrahim

Copy link
Copy Markdown
Contributor Author

⚠️ Do not merge — independent review found a critical defect

This breaks every write in the web UI.

_client_header_ok() is called at the top of do_POST, above the SETUP_ACTION_PREFIX check, so it gates every POST including /setup/*. But the write surface is a plain HTML form (render.py:787) and the rendered pages ship no JavaScript by designdefault-src 'none' with no script-src. A browser form POST cannot set a custom header, so every setup action returns 403: create config, create database, seed rules, capture credentials. That is the flow #467 and #469 shipped, and on the desktop bundle there is no terminal to fall back to.

The suite stayed green because tests/web/test_server.py::_request gained client_header: str | None = "1", sent on every POST. "No existing test body was edited" is literally true — zero deletions — and after it, no pre-existing POST test exercises what the shipped client does.

Root cause is the brief, not the implementation. The issue said POST /api/*; the worker brief said "every state-changing POST". Issue #535's acceptance criteria have been tightened so the distinction cannot be missed again.

Three further findings:

  • The static CSP omits form-action, base-uri and frame-ancestors, none of which inherit from default-src under CSP3. The existing _SECURITY_HEADERS sets all three. So the headline claim — that the UI cannot send trading data anywhere but the local process — is false as written: connect-src does not cover a cross-origin form submit, and a missing base-uri lets an injected <base> retarget relative URLs. Also missing on image/svg+xml, which is active content.
  • The desktop bundle has no static assets. keel/freeze.py's collect_data names only the template package, so /static/* 404s in a frozen app. The acceptance box "a double-click reaches a served page" is unmet. The packaging work in this PR covered the wheel — which turned out to be the half that did not need fixing.
  • No Cache-Control on static responses, since _serve_static bypasses _send. Once Client shell: router, status view, responsive layout, accessibility baseline #536 ships versioned assets, a stale bundle can outlive an engine upgrade in the HTTP cache.

What survives the rework

The traversal defence is genuinely closed — the reviewer ran 17 payloads beyond this PR's own, including double-encoded, %c0%af, ..;/, and symlinks to files and directories outside the root. All refused. resolve() + relative_to() is the right check.

security.py's 0-line diff, the untouched capability-disjointness scan, and the artifacts-is-inert finding (independently reproduced — identical wheels either way) all stand.

Reworking now.

… 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 <form method="post"
action="/setup/{key}">, 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 <form
  action="https://evil.example"> was not a "connection" so connect-src 'self' did not stop it,
  and a missing base-uri let an injected <base> 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 <script> inside one executes in keel's own origin).

- keel/freeze.py's collect_data (read by packaging/keel.spec, which is fully generic over that
  list) did not include the web UI's static assets, so a frozen desktop bundle would 404 every
  /static/* request. Added STATIC_PACKAGE = "keel.web.static" (a PEP 420 namespace package, same
  shape as the existing TEMPLATE_PACKAGE) to collect_data, mirrored in a new
  tests/test_freeze.py test. Not added to hiddenimports: staticfiles.py locates assets via
  Path(__file__).parent, never importlib.resources, so nothing needs it as an import target.

- _serve_static bypassed _send and so emitted no Cache-Control, leaving a static asset open to
  browser heuristic caching. Added Cache-Control: no-store, max-age=0 to match the rendered
  pages, pending #536's versioned-asset cache strategy.

- Registered a `slow` pytest marker (new [tool.pytest.ini_options], CI passes no -m filter so
  this changes nothing there) and applied it to the wheel-build packaging test, which now also
  excludes dotfiles (a stray .DS_Store) from the on-disk/wheel comparison so the test cannot fail
  for a reason unrelated to packaging.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NyeggYtojNXCTHeD3JHxb6
Comment thread keel/web/server.py
# 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()
Comment thread keel/web/staticfiles.py
# 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()
Comment thread keel/web/staticfiles.py
except ValueError:
return None

if not candidate.is_file():
@eaitbrahim

Copy link
Copy Markdown
Contributor Author

Reworked — the blocking defect is fixed, and verified red→green

The write path works again

do_POST now runs _admitted()_sec_fetch_site_ok() (both on every POST — a real form does send Sec-Fetch-Site, and page JS cannot forge it) → then branches on path. Only API_PREFIX requires X-Keel-Client. /setup/* keeps host validation, the SameSite=Strict cookie, the HMAC CSRF token and the closed action set.

Good detail: the API gate is checked before the 404 that path currently returns, so the first API write action does not have to remember to add it.

test_a_browser_form_post_succeeds_without_the_api_client_header posts exactly as a browser does — cookie, CSRF, Origin, Sec-Fetch-Site, no X-Keel-Client — and was verified against the previous commit: 403 == 303 failure, then green after the fix. That is the test the old _request default was hiding.

Everything else from the review

  • CSP now carries form-action 'self'; base-uri 'none'; frame-ancestors 'none' plus X-Frame-Options: DENY, on text/html and image/svg+xml.
  • Desktop bundle: keel/freeze.py collects keel.web.static; packaging/keel.spec needed no change since it is generic over that list. New tests/test_freeze.py test, verified red→green. The double-click acceptance criterion is now actually met.
  • Cache-Control: no-store, max-age=0 on static responses.
  • Wheel test marked @pytest.mark.slow and excludes dotfiles.

CodeQL is red, and it is a false positive — here is the evidence

Three py/path-injection alerts: server.py:602, staticfiles.py:92 and :97.

resolve_static_asset uses the canonical containment pattern — root.resolve(), (root / rel).resolve(), then .relative_to(root_resolved) inside a try, with ValueErrorNone. CodeQL does not model Path.relative_to() as a sanitizer; it recognises os.path.realpath + startswith and not pathlib's idiom.

I ran 14 payloads directly against the resolver, including cases the branch's own tests do not cover:

RESOLVED (expected)  /static/index.html
refused              /static/../secret.txt
refused              /static/..%2fsecret.txt
refused              /static/%2e%2e/secret.txt
refused              /static/%252e%252e/secret.txt      (double-encoded, stays literal)
refused              /static//etc/passwd                (absolute join discards root)
refused              /static/../ x20 /etc/passwd
refused              /static/....//secret.txt
refused              /static/..;/secret.txt
refused              /static/link_to_file               (symlink -> file outside root)
refused              /static/link_to_dir/loot.txt       (symlink -> dir outside root)
refused              /static/index.html/../../secret.txt
refused              /static/\x00index.html
refused              /static/%c0%afsecret.txt
refused              /static/..\\secret.txt

ESCAPES: NONE — containment holds

I have not dismissed the alerts. That is a call on your security dashboard, not mine. The options are to dismiss all three as "used in tests / false positive" with this evidence attached, or to restructure to a shape CodeQL recognises — which would mean rewriting verified-correct security code to satisfy a scanner, and I would not recommend it.

Tests pass on 3.11 and 3.14. 4610 passed, 3 skipped, ruff and mypy clean. security.py and staticfiles.py are 0-line diffs from the reviewed state; the five pre-existing security-layer tests and the capability-disjointness scan are untouched and green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Static asset serving and the full security header set

2 participants