From 1ca0d220e584a2ee40726e4291e04368f7fe343b Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Wed, 14 Oct 2026 13:28:22 -0400 Subject: [PATCH 1/4] Clear the open CodeQL alerts: listing-based static lookup, closed-set redirect, flag-named probe refusal - #3/#5/#7 py/path-injection: resolve_static_asset answers by exact name from a listing of the static root (_servable_files: real files whose resolved location stays under the root) instead of joining the request path onto the root. Same containment, stricter on non-canonical spellings, and no request byte reaches a filesystem call. - #2 py/http-response-splitting: the token exchange redirects to a member of shell_request_paths (shell_redirect_target), never to the request's own spelling; also closes a `/\host` protocol-relative redirect. - #1 py/clear-text-logging-sensitive-data: the probe's missing-credential refusal names the missing flag, never the value typed after it. - #8 py/bad-tag-filter: the shell's script-tag test parses with html.parser instead of a regex. Co-Authored-By: Claude Opus 5.5 --- keel/web/server.py | 7 +- keel/web/staticfiles.py | 108 +++++++++++++------- scripts/robinhood_order_probe.py | 9 +- tests/scripts/test_robinhood_order_probe.py | 32 ++++++ tests/web/test_client_assets.py | 47 +++++++-- tests/web/test_server.py | 51 +++++++++ tests/web/test_staticfiles.py | 47 +++++++++ 7 files changed, 253 insertions(+), 48 deletions(-) diff --git a/keel/web/server.py b/keel/web/server.py index e5a43e66..9df95c45 100644 --- a/keel/web/server.py +++ b/keel/web/server.py @@ -1318,12 +1318,15 @@ def do_GET(self) -> None: # noqa: N802 - stdlib's naming, not ours presented = (query.get("token") or [""])[0] if tokens_match(presented, self.cfg.token): # Exchange the token for a cookie and get it out of the URL, so it stops appearing in - # history, in a bookmark, and in anything the user pastes when asking for help. + # history, in a bookmark, and in anything the user pastes when asking for help. The + # target is one of the console's own paths, never the request's spelling of one + # (`staticfiles.shell_redirect_target`, CodeQL alert #2). + location = staticfiles.shell_redirect_target(staticfiles.STATIC_ROOT, parsed.path) self._send( 303, "", extra=( - ("Location", parsed.path or "/"), + ("Location", location), ("Set-Cookie", session_cookie(self.cfg.token)), ), ) diff --git a/keel/web/staticfiles.py b/keel/web/staticfiles.py index 3b41eec5..b536431e 100644 --- a/keel/web/staticfiles.py +++ b/keel/web/staticfiles.py @@ -12,16 +12,25 @@ 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 -- +**The traversal defence is a lookup in a listing, not a join and 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. +request for `/static//etc/passwd` and never notice. + +So the request path is never joined onto the root at all. `_servable_files` walks the root -- the +package's own directory, a fixed location -- and lists every regular file whose REAL location +(`resolve()`, symlinks followed) is still under the root; `resolve_static_asset` then answers by +exact name from that listing. A request path is only ever COMPARED with names the package +produced, never handed to a filesystem call, so there is no spelling of it -- `..`, an absolute +path, `%2e%2e`, a null byte, a symlink planted inside the root that points out of it -- that can +name a file the listing does not already hold. Until the CodeQL alerts #3/#5/#7 this was +`(root / rel).resolve()` then `relative_to(root)`, which enforced the same containment but walked +a user-supplied path to get there; the listing is at least as strict (it also refuses +non-canonical spellings such as `sub/../index.html`, which no browser sends) and puts no request +byte near the filesystem. The walk is per call, over a directory of a few dozen files. """ from __future__ import annotations @@ -48,7 +57,7 @@ #: **The consequence worth stating: this prefix no longer NARROWS anything.** While it was #: `/static/`, `url_path.startswith(STATIC_PREFIX)` was itself a filter, and `resolve_static_asset` #: could not be reached by a request for `/api/pulse`. At `/` every path passes that test, so the -#: containment check in `resolve_static_asset` -- `resolve()` then `relative_to()` -- is the only +#: containment in `resolve_static_asset` -- an exact-name lookup in `_servable_files` -- is the only #: thing keeping a request inside the static root, and `server.do_GET` orders `/api/` ahead of #: static resolution rather than relying on the prefix to separate them. STATIC_PREFIX = "/" @@ -80,37 +89,38 @@ def content_type_for(path: Path) -> str | None: return _CONTENT_TYPES.get(path.suffix.lower()) +def _servable_files(root: Path) -> dict[str, Path]: + """Every regular file under `root`, by its request name (`relpath`, POSIX separators), mapped + to its real location. An entry whose real location is outside `root` -- a symlink, or a file + under a symlinked directory, that points out of it -- is left out, so the listing can never + name a file the root does not hold. Built from the filesystem alone; no request path reaches + it (see the module docstring).""" + root_resolved = root.resolve() + files: dict[str, Path] = {} + for path in root.rglob("*"): + real = path.resolve() # non-strict: a symlink loop resolves to a non-file, not an error + if not real.is_relative_to(root_resolved) or not real.is_file(): + continue + files[path.relative_to(root).as_posix()] = real + return files + + 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 `..`. + as "serve something close enough": the path does not start with the static prefix, no file + of that exact name is in `_servable_files`'s listing (missing, a directory, a non-canonical + spelling of a real file), or the only file of that name lives outside `root` once symlinks + are followed. 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 `..` -- and then, like a null byte or `..` itself, simply names nothing in the listing. + The returned `Path` is the listing's own value, never one built from the request. """ 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 + return _servable_files(root).get(rel) # -- the client's own routes (#536) --------------------------------------------------------------- @@ -227,15 +237,37 @@ def shell_request_paths(root: Path) -> frozenset[str]: **Why enumerate rather than resolve.** `server.KeelHandler._is_a_sessioned_shell_read` asks "is this request path one the shell answers" and compares against this set, so the request path is only ever COMPARED, never handed to a filesystem call: what is built here comes from - the package's own directory listing. Resolving the path instead would be a second - user-controlled flow into `resolve_static_asset`. The walk is per call, which is cheap - enough because its one caller is reached only once the remote session lifetime has expired. + the package's own directory listing (`_servable_files`, the same one `resolve_static_asset` + answers from). Its callers are that session check, reached only once the remote session + lifetime has expired, and `shell_redirect_target`, reached only by a token exchange; the walk + is per call, as `resolve_static_asset`'s is. """ names = { - STATIC_PREFIX + path.relative_to(root).as_posix() - for path in root.rglob("*") - if path.is_file() and content_type_for(path) is not None + STATIC_PREFIX + name + for name, path in _servable_files(root).items() + if content_type_for(path) is not None } names.add(STATIC_PREFIX) names.update(STATIC_PREFIX + name for name in (*CLIENT_ROUTES, *RETIRED_CLIENT_ROUTES)) return frozenset(names) + + +def shell_redirect_target(root: Path, url_path: str) -> str: + """The path the token exchange redirects to: the member of `shell_request_paths` that + `url_path` names once decoded, else `STATIC_PREFIX` (CodeQL alert #2). + + **The answer is always one of the set's own strings, never the request's.** The redirect's + `Location` used to be the request path as sent, which put request bytes into a response + header; `http.server` splits the request line on whitespace, so a raw CR or LF never reached + it, but a backslash did (`/\\host` is the protocol-relative `//host` to a browser), and a + header built from what the client sent is one safe only for as long as every layer beneath + it stays that way. Comparing against the closed set and returning the matched member means no + byte of the request can be in the header at all. A `?token=` link to any path the console + does not answer -- an `/api/` route, a typo -- lands on the home view, which is where the + printed address sends everyone anyway. + """ + wanted = unquote(url_path) + for known in shell_request_paths(root): + if known == wanted: + return known + return STATIC_PREFIX diff --git a/scripts/robinhood_order_probe.py b/scripts/robinhood_order_probe.py index 13793c83..41dd9124 100644 --- a/scripts/robinhood_order_probe.py +++ b/scripts/robinhood_order_probe.py @@ -497,7 +497,14 @@ def main(argv: list[str] | None = None) -> int: api_key = (values.get(args.api_key_var) or "").strip() private_key = (values.get(args.private_key_var) or "").strip() if not api_key or not private_key: - print(f"missing {args.api_key_var} or {args.private_key_var} in {args.env}") + # Named by FLAG, never by the value after it (CodeQL alert #1): that value is whatever the + # operator typed, and a key pasted where its variable name belongs would be echoed here. + missing = [ + flag + for flag, value in (("--api-key-var", api_key), ("--private-key-var", private_key)) + if not value + ] + print(f"no value in {args.env} for the variable named by {' or '.join(missing)}") return 2 transport = RobinhoodTransport(api_key=api_key, private_key_b64=private_key) diff --git a/tests/scripts/test_robinhood_order_probe.py b/tests/scripts/test_robinhood_order_probe.py index eb230042..2fa90894 100644 --- a/tests/scripts/test_robinhood_order_probe.py +++ b/tests/scripts/test_robinhood_order_probe.py @@ -373,3 +373,35 @@ def test_the_float_repr_trap_stays_closed() -> None: the original text is bypassed and `0.00000001` becomes `1e-08`. This is the regression test for that having actually happened here.""" assert "e-" not in dumps_venue_json({"tiny": Decimal("0.00000001")}) + + +# --- the credential check ----------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("env_text", "missing_flag", "present_flag"), + [ + ("ROBINHOOD_API_KEY=rh-api-key-value\n", "--private-key-var", "--api-key-var"), + ("ROBINHOOD_PRIVATE_KEY=cHJpdmF0ZS1rZXktdmFsdWU=\n", "--api-key-var", "--private-key-var"), + ], +) +def test_a_missing_credential_is_named_by_its_flag_and_echoes_no_value( + tmp_path: Any, capsys: Any, env_text: str, missing_flag: str, present_flag: str +) -> None: + """CodeQL alert #1. The refusal used to print `args.api_key_var` and `args.private_key_var` + back -- variable NAMES by default, but whatever the operator typed after the flag, so a key + pasted where its variable name belongs was echoed straight to the terminal. The refusal now + names the FLAG that is missing and repeats nothing the operator or the `.env` supplied.""" + env = tmp_path / "probe.env" + env.write_text(env_text) + pasted = "PASTED-INTO-THE-NAME-FLAG-not-a-variable-name" + + code = probe.main(["--env", str(env), missing_flag, pasted]) + + out = capsys.readouterr().out + assert code == 2 + assert pasted not in out + assert "rh-api-key-value" not in out + assert "cHJpdmF0ZS1rZXktdmFsdWU=" not in out + assert missing_flag in out + assert present_flag not in out diff --git a/tests/web/test_client_assets.py b/tests/web/test_client_assets.py index 8530d151..d36517f3 100644 --- a/tests/web/test_client_assets.py +++ b/tests/web/test_client_assets.py @@ -29,6 +29,7 @@ import json import re +from html.parser import HTMLParser from pathlib import Path import pytest @@ -176,6 +177,27 @@ def _markup_only(html: str) -> str: return re.sub(r"", " ", html, flags=re.DOTALL) +class _StartTags(HTMLParser): + """Every start tag in document order, as `(name, {attribute: value})`.""" + + def __init__(self) -> None: + super().__init__(convert_charrefs=True) + self.tags: list[tuple[str, dict[str, str]]] = [] + + def handle_starttag(self, tag: str, attrs: list[tuple[str, str | None]]) -> None: + self.tags.append((tag, {name: value or "" for name, value in attrs})) + + def handle_startendtag(self, tag: str, attrs: list[tuple[str, str | None]]) -> None: + self.handle_starttag(tag, attrs) + + +def _start_tags(html: str) -> list[tuple[str, dict[str, str]]]: + parser = _StartTags() + parser.feed(html) + parser.close() + return parser.tags + + #: Every character that can only be arithmetic once comments and strings are gone. `/` is in the #: list because `_code_only` has already consumed both comment forms, so a surviving `/` is #: division. @@ -938,18 +960,29 @@ def test_the_shell_ships_no_inline_script_or_style() -> None: html = _markup_only(_INDEX.read_text(encoding="utf-8")).lower() assert "`, + # attribute order and whitespace inside a tag. + tags = _start_tags(html) + assert not [tag for tag, _attrs in tags if tag in {"style", "base"}] # A `