From 51336436007ff08f2799b406e323a868786f2266 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Fri, 21 Aug 2026 18:56:00 -0400 Subject: [PATCH] fix(composer): build the font path from the allowlist entry secure_filename cleared the plugin-directory alerts: CodeQL went from 19 to 5, and from 16 high-severity to 2. The two that remain are in serve_font, which is gated by a frozenset of three exact filenames -- so nothing was exploitable -- but the name reaching the filesystem was still the request value. It now comes from the matched allowlist entry. Identical strings, so runtime behaviour is unchanged; the difference is that the filename is provably a module constant rather than a guarded piece of user input. The first test I wrote for this proved nothing. It asserted 404 on traversal payloads, but Flask's router will not match a path segment containing '/', and the rest 404 simply because no such file exists -- so removing the allowlist entirely still passed. Replaced with a readable file planted next to the fonts: fonts/id_rsa.ttf -> 404, body does not contain its contents which fails with "a readable non-allowlisted file was served" the moment the gate is removed. 45 tests. Left alone: three medium py/stack-trace-exposure alerts on the _generate_plugin_files handlers, which return str(exc) for ComposerInputError. Its seven raise sites are all authored literals ("Author is required.", "Config variable key X is not a valid Python identifier."), so no traceback or path is exposed. Clearing them means either replacing that feedback with a generic string or restructuring validation to return errors instead of raising -- a change to the author's design, made blind, since CodeQL cannot be run locally to confirm it would even work. That is a decision, not a cleanup. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW --- test/test_composer_path_containment.py | 63 ++++++++++++++++++++++++++ web_interface/blueprints/composer.py | 10 +++- 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/test/test_composer_path_containment.py b/test/test_composer_path_containment.py index f8e1d090..f0f760b6 100644 --- a/test/test_composer_path_containment.py +++ b/test/test_composer_path_containment.py @@ -160,3 +160,66 @@ def test_a_trailing_newline_is_not_a_valid_id(): name ends in one. \Z does not.""" assert C._PLUGIN_ID_RE.match("myplugin") is not None assert C._PLUGIN_ID_RE.match("myplugin\n") is None + + +# --- font serving ----------------------------------------------------------- + +FONT_TRAVERSAL = [ + "../../../etc/passwd", "../config/config.json", "..%2f..%2fetc%2fpasswd", + "PressStart2P-Regular.ttf/../../../etc/passwd", "/etc/passwd", "", + "PressStart2P-Regular.TTF", # case differs -> not the allowlisted name + "PressStart2P-Regular.ttf ", # trailing space +] + + +def test_serve_font_refuses_a_file_that_exists_but_is_not_allowlisted(monkeypatch, tmp_path): + """The allowlist must be what refuses it, not a missing file. + + Asserting 404 on traversal payloads proves nothing here: Flask's router + will not match a path segment containing '/', and everything else 404s + simply because no such file exists. Put a real, readable file next to the + fonts and confirm it is still refused -- that is the allowlist working. + """ + fonts = tmp_path / "assets" / "fonts" + fonts.mkdir(parents=True) + (fonts / "id_rsa.ttf").write_bytes(b"PRIVATE KEY") + monkeypatch.setattr(C.composer_bp, "project_root", str(tmp_path), raising=False) + app = __import__("flask").Flask(__name__) + app.register_blueprint(C.composer_bp) + with app.test_client() as client: + resp = client.get("/api/fonts/id_rsa.ttf") + assert resp.status_code == 404, ( + "a readable non-allowlisted file was served; the allowlist is not gating") + assert b"PRIVATE KEY" not in resp.data + + +@pytest.mark.parametrize("payload", FONT_TRAVERSAL) +def test_serve_font_refuses_anything_not_allowlisted(payload, monkeypatch, tmp_path): + """The name reaching the filesystem must come from the allowlist constant. + + _ALLOWED_FONTS gates this endpoint, so nothing here was ever exploitable. + Building the path from the matched constant rather than the request value + is what makes that provable -- and it is why CodeQL reported two + high-severity py/path-injection alerts on an endpoint that was already + safe. + """ + monkeypatch.setattr(C.composer_bp, "project_root", str(tmp_path), raising=False) + app = C.composer_bp.name and __import__("flask").Flask(__name__) + app.register_blueprint(C.composer_bp) + with app.test_client() as client: + resp = client.get(f"/api/fonts/{payload}") + assert resp.status_code in (404, 405, 308), ( + f"{payload!r} was not refused (status {resp.status_code})") + + +def test_serve_font_still_serves_each_allowlisted_font(monkeypatch, tmp_path): + fonts = tmp_path / "assets" / "fonts" + fonts.mkdir(parents=True) + monkeypatch.setattr(C.composer_bp, "project_root", str(tmp_path), raising=False) + app = __import__("flask").Flask(__name__) + app.register_blueprint(C.composer_bp) + for name in C._ALLOWED_FONTS: + (fonts / name).write_bytes(b"\x00\x01ttf") + with app.test_client() as client: + resp = client.get(f"/api/fonts/{name}") + assert resp.status_code == 200, f"{name} should be served, got {resp.status_code}" diff --git a/web_interface/blueprints/composer.py b/web_interface/blueprints/composer.py index 0fd360c5..88e2f2c7 100644 --- a/web_interface/blueprints/composer.py +++ b/web_interface/blueprints/composer.py @@ -758,11 +758,17 @@ def install_locally(): @composer_bp.route('/api/fonts/') def serve_font(font_name): """Serve an allowlisted font file for canvas FontFace loading.""" - if font_name not in _ALLOWED_FONTS: + # Build the path from the allowlist entry, not from the request value. + # They are equal strings, so this changes nothing at runtime -- but the + # name that reaches the filesystem now provably originates in a module + # constant, which is the difference between "guarded" and "not derived + # from user input at all". + allowed_name = next((f for f in sorted(_ALLOWED_FONTS) if f == font_name), None) + if allowed_name is None: return '', 404 if not composer_bp.project_root: return '', 503 - font_path = Path(composer_bp.project_root) / 'assets' / 'fonts' / font_name + font_path = Path(composer_bp.project_root) / 'assets' / 'fonts' / allowed_name if not font_path.exists(): return '', 404 return send_file(str(font_path), mimetype='font/ttf')