diff --git a/test/test_composer_path_containment.py b/test/test_composer_path_containment.py index cc5f067b..c47ca884 100644 --- a/test/test_composer_path_containment.py +++ b/test/test_composer_path_containment.py @@ -45,19 +45,18 @@ def plugins_dir(tmp_path, monkeypatch): @pytest.mark.parametrize("payload", TRAVERSAL) def test_traversal_payloads_are_refused(plugins_dir, payload): - with pytest.raises(C.ComposerInputError): - C._plugin_dir(payload) + assert C._plugin_dir(payload) is None @pytest.mark.parametrize("payload", MALFORMED) def test_malformed_ids_are_refused(plugins_dir, payload): - with pytest.raises(C.ComposerInputError): - C._plugin_dir(payload) + assert C._plugin_dir(payload) is None @pytest.mark.parametrize("payload", ["a", "my-plugin", "x9", "a" * 63]) def test_valid_ids_resolve_inside_the_base(plugins_dir, payload): resolved = C._plugin_dir(payload) + assert resolved is not None, f"{payload!r} was rejected but is valid" assert resolved.parent == plugins_dir.resolve(), ( f"{payload!r} resolved to {resolved}, outside {plugins_dir}") @@ -71,18 +70,34 @@ def test_no_payload_can_escape_even_if_the_regex_is_loosened(plugins_dir, monkey """ import re monkeypatch.setattr(C, "_PLUGIN_ID_RE", re.compile(r"\A[\w./\\~-]+\Z")) + import os escaped = [] + base = os.path.realpath(str(plugins_dir)) for payload in TRAVERSAL: - try: - resolved = C._plugin_dir(payload) - except C.ComposerInputError: + resolved = C._plugin_dir(payload) + if resolved is None: continue - base = plugins_dir.resolve() - if resolved != base and base not in resolved.parents: - escaped.append((payload, str(resolved))) + real = os.path.realpath(str(resolved)) + if real != base and os.path.commonpath([base, real]) != base: + escaped.append((payload, real)) assert not escaped, f"these escaped the base with a loosened regex: {escaped}" +def test_a_sibling_directory_with_a_shared_prefix_is_not_inside(tmp_path, monkeypatch): + """commonpath, not startswith. + + "/x/plugins-evil" starts with "/x/plugins" but is a different directory, so + a prefix test would accept it. + """ + base = tmp_path / "plugins" + base.mkdir() + (tmp_path / "plugins-evil").mkdir() + monkeypatch.setattr(C.composer_bp, "plugins_dir", str(base), raising=False) + import re + monkeypatch.setattr(C, "_PLUGIN_ID_RE", re.compile(r"\A[\w./\\~-]+\Z")) + assert C._plugin_dir("../plugins-evil") is None + + def test_a_trailing_newline_is_not_a_valid_id(): r"""Python's `$` also matches before a trailing newline, so the original `^...$` accepted "myplugin\n" and would have created a directory whose diff --git a/web_interface/blueprints/composer.py b/web_interface/blueprints/composer.py index 3eb0002c..eac307d1 100644 --- a/web_interface/blueprints/composer.py +++ b/web_interface/blueprints/composer.py @@ -12,7 +12,9 @@ import ast import io import json import logging +import os import re +from typing import Optional import zipfile from datetime import datetime from pathlib import Path @@ -598,7 +600,7 @@ class ComposerInputError(ValueError): """ -def _plugin_dir(plugin_id: str) -> Path: +def _plugin_dir(plugin_id: str) -> Optional[Path]: """Resolve a plugin directory, refusing anything outside plugins_dir. _PLUGIN_ID_RE already rejects '/', '.' and '..', so this cannot currently @@ -609,15 +611,21 @@ def _plugin_dir(plugin_id: str) -> Path: recognises, which is why CodeQL reported sixteen path-injection alerts against code that was already safe. - Raises ComposerInputError if the id is malformed or escapes the base. + Returns None for a malformed id or one that escapes the base. It returns + rather than raises so the handlers answer with a fixed literal: routing a + caught exception's text into a response is what py/stack-trace-exposure + flags, and there is nothing here a caller needs beyond "that id is not ok". """ if not _PLUGIN_ID_RE.match(plugin_id or ''): - raise ComposerInputError('Invalid plugin ID') - base = Path(composer_bp.plugins_dir).resolve() - candidate = (base / plugin_id).resolve() - if candidate != base and base not in candidate.parents: - raise ComposerInputError('Invalid plugin ID') - return candidate + return None + base = os.path.realpath(str(composer_bp.plugins_dir)) + candidate = os.path.realpath(os.path.join(base, plugin_id)) + # commonpath, not startswith: "/plugins-evil" starts with "/plugins" but is + # a different directory. This is also the form static analysis recognises + # as a containment check. + if candidate != base and os.path.commonpath([base, candidate]) != base: + return None + return Path(candidate) def _save_composer_state(target_dir: Path, payload: dict) -> None: @@ -697,10 +705,9 @@ def install_locally(): # different function -- re-check here, at the point the path is actually # built, so this route stays safe on its own if that call is ever # reordered or changed. - try: - target = _plugin_dir(plugin_id) - except ComposerInputError as exc: - return jsonify({'status': 'error', 'message': str(exc)}), 400 + target = _plugin_dir(plugin_id) + if target is None: + return jsonify({'status': 'error', 'message': 'Invalid plugin ID'}), 400 force = bool(data.get('_force', False)) if target.exists() and not force: @@ -754,10 +761,10 @@ def validate_id(plugin_id): if not _PLUGIN_ID_RE.match(plugin_id): return jsonify({'valid': False, 'available': False, 'reason': 'Invalid format'}) if composer_bp.plugins_dir: - try: - taken = _plugin_dir(plugin_id).exists() - except ComposerInputError: + resolved = _plugin_dir(plugin_id) + if resolved is None: return jsonify({'valid': False, 'available': False, 'reason': 'Invalid format'}) + taken = resolved.exists() if taken: return jsonify({'valid': True, 'available': False, 'reason': 'Already installed'}) return jsonify({'valid': True, 'available': True}) @@ -826,10 +833,9 @@ def load_plugin(plugin_id): """ if not composer_bp.plugins_dir: return jsonify({'status': 'error', 'message': 'Plugin directory not configured'}), 503 - try: - plugin_dir = _plugin_dir(plugin_id) - except ComposerInputError as exc: - return jsonify({'status': 'error', 'message': str(exc)}), 400 + plugin_dir = _plugin_dir(plugin_id) + if plugin_dir is None: + return jsonify({'status': 'error', 'message': 'Invalid plugin ID'}), 400 if not plugin_dir.exists(): return jsonify({'status': 'error', 'message': 'Plugin not found'}), 404 diff --git a/web_interface/static/v3/app.css b/web_interface/static/v3/app.css index a1342955..fa73108b 100644 --- a/web_interface/static/v3/app.css +++ b/web_interface/static/v3/app.css @@ -421,6 +421,9 @@ a, button, input, select, textarea { .md\:grid-cols-4 { grid-template-columns: repeat(4, minmax(0, 1fr)); } .md\:flex { display: flex; } .md\:hidden { display: none; } + /* composer.html labels its toolbar buttons `hidden md:inline`, so without + this the label is hidden at every width and the buttons stay icon-only. */ + .md\:inline { display: inline; } } @media (min-width: 1024px) {