mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-22 10:58:15 +00:00
Previous attempt got the count from 22 down to 19 but left the 16
path-injection alerts untouched: CodeQL carries taint through
_plugin_dir's return value and does not treat an internal realpath /
commonpath guard as a sanitiser.
secure_filename is one it does model. It is also a no-op on every id the
regex accepts -- verified across the accepted alphabet, 4000 generated
ids, zero altered -- so it cannot rewrite a caller's id into a different
plugin's directory. The equality check makes that explicit: if it changes
anything, the id was not one we accept, and we refuse rather than
silently redirect.
Found a real bug while testing the layers separately: '.' resolved to the
plugins root, and install() calls shutil.rmtree(target) when force is
set, so an id of '.' would have deleted every installed plugin. The regex
blocks it today, but the containment layer was allowing candidate == base
on the grounds that the base is not "outside" itself. A plugin directory
must be a child, never the root.
That came out of writing the isolated tests. Removing containment did not
fail anything, because secure_filename rejects traversal first -- which
made a redundant layer look load-bearing. Each layer is now neutralised
in turn so the one under test is the only thing standing:
containment removed -> FAIL (13 payloads reach the base or past it)
candidate == base allowed -> FAIL ('.' resolves to the plugins root)
commonpath -> startswith -> FAIL (sibling "plugins-evil" accepted)
secure_filename bypassed -> pass, containment covers it
The last is honest rather than a gap: with containment in place the
sanitiser has nothing left to block, and its value here is CodeQL
recognition plus a second barrier if containment is ever weakened.
Also corrected an assertion in the previous commit's test, which counted
any non-None result as an escape. '....', '~' and 'a\..\..' are ordinary
directory names on Linux and resolve safely inside the base; treating
them as escapes made the test fail on correct code.
35 tests. The 5 test_web_api.py failures are pre-existing on this branch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
163 lines
6.8 KiB
Python
163 lines
6.8 KiB
Python
"""A composer plugin id must never resolve outside the plugins directory.
|
|
|
|
CodeQL reported sixteen high-severity py/path-injection alerts against
|
|
web_interface/blueprints/composer.py: a request-supplied plugin_id reaching
|
|
Path(plugins_dir) / plugin_id, which is then created, written to, deleted
|
|
(shutil.rmtree) and read back.
|
|
|
|
The id was already validated by an anchored regex, so every traversal payload
|
|
was in fact rejected. What was missing was the guarantee living *with* the path
|
|
building rather than in a regex several hundred lines away -- loosen that regex
|
|
later and the traversal opens silently, with nothing at the filesystem boundary
|
|
to catch it. _plugin_dir() closes that, and is the form static analysis can see.
|
|
"""
|
|
import sys
|
|
from pathlib import Path
|
|
from unittest.mock import MagicMock
|
|
|
|
import pytest
|
|
|
|
sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
|
|
|
|
from web_interface.blueprints import composer as C # noqa: E402
|
|
|
|
#: Anything that has ever been used to climb out of a directory.
|
|
TRAVERSAL = [
|
|
"../../etc/passwd", "..", ".", "a/../../etc", "good/../../..",
|
|
"/etc/passwd", "//etc/passwd", "a\\..\\..", "a%2f..%2f..",
|
|
"....//....//etc", "a/./../../etc", "~", "~root",
|
|
"plugin/../../../../../../etc/shadow",
|
|
]
|
|
|
|
#: Rejected for shape, not traversal -- but rejected all the same.
|
|
MALFORMED = ["", "A-upper", "1-leading-digit", "-leading-dash", "has_underscore",
|
|
"has space", "has.dot", "a" * 64, "plugin\n", "plugin\n../../etc",
|
|
"\n", "plug\x00in"]
|
|
|
|
|
|
@pytest.fixture
|
|
def plugins_dir(tmp_path, monkeypatch):
|
|
base = tmp_path / "plugin-repos"
|
|
base.mkdir()
|
|
monkeypatch.setattr(C.composer_bp, "plugins_dir", str(base), raising=False)
|
|
return base
|
|
|
|
|
|
@pytest.mark.parametrize("payload", TRAVERSAL)
|
|
def test_traversal_payloads_are_refused(plugins_dir, payload):
|
|
assert C._plugin_dir(payload) is None
|
|
|
|
|
|
@pytest.mark.parametrize("payload", MALFORMED)
|
|
def test_malformed_ids_are_refused(plugins_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}")
|
|
|
|
|
|
def test_no_payload_can_escape_even_if_the_regex_is_loosened(plugins_dir, monkeypatch):
|
|
"""The containment check must stand on its own.
|
|
|
|
This is the whole point of resolving at the filesystem boundary: if the id
|
|
pattern is ever relaxed, traversal must still be impossible. Replace the
|
|
regex with one that permits slashes and dots, then re-run the payloads.
|
|
"""
|
|
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:
|
|
resolved = C._plugin_dir(payload)
|
|
if resolved is None:
|
|
continue
|
|
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
|
|
# Neutralise the two layers in front so this exercises the containment
|
|
# check itself; otherwise secure_filename rejects the payload first and a
|
|
# startswith regression would go unnoticed here.
|
|
monkeypatch.setattr(C, "_PLUGIN_ID_RE", re.compile(r"\A[\w./\\~-]+\Z"))
|
|
monkeypatch.setattr(C, "secure_filename", lambda v: v)
|
|
assert C._plugin_dir("../plugins-evil") is None
|
|
|
|
|
|
def test_containment_still_holds_if_the_sanitiser_is_defeated(plugins_dir, monkeypatch):
|
|
"""Each layer is tested on its own, not just the stack.
|
|
|
|
secure_filename's equality guard rejects every traversal payload before the
|
|
containment check sees it, so removing containment does not fail the other
|
|
tests -- which would make it look load-bearing when it is not. Neutralise
|
|
the regex *and* the sanitiser, and the realpath/commonpath check must still
|
|
refuse everything on its own.
|
|
"""
|
|
import re
|
|
monkeypatch.setattr(C, "_PLUGIN_ID_RE", re.compile(r"\A[\w./\\~-]+\Z"))
|
|
monkeypatch.setattr(C, "secure_filename", lambda v: v)
|
|
import os
|
|
base = os.path.realpath(str(plugins_dir))
|
|
escaped = []
|
|
for payload in TRAVERSAL:
|
|
resolved = C._plugin_dir(payload)
|
|
if resolved is None:
|
|
continue
|
|
real = os.path.realpath(str(resolved))
|
|
# Inside the base is fine -- "...." and "~" are ordinary directory
|
|
# names on Linux, so they are not escapes. What must never happen is
|
|
# landing outside the base, or on the base itself: install() rmtrees
|
|
# its target, so the plugins root resolving to a "plugin" would wipe
|
|
# every installed plugin.
|
|
if real == base or os.path.commonpath([base, real]) != base:
|
|
escaped.append((payload, real))
|
|
assert not escaped, f"containment alone let these through: {escaped}"
|
|
|
|
|
|
def test_secure_filename_never_rewrites_an_accepted_id(plugins_dir):
|
|
"""The sanitiser must be a no-op on everything the regex accepts.
|
|
|
|
If secure_filename ever altered an accepted id, _plugin_dir would resolve
|
|
to a *different* plugin's directory than the caller asked for -- a silent
|
|
redirect, which is worse than a refusal. The guard turns that into a
|
|
refusal; this proves the guard never has to fire in practice.
|
|
"""
|
|
import random
|
|
from werkzeug.utils import secure_filename
|
|
random.seed(1)
|
|
alphabet = "abcdefghijklmnopqrstuvwxyz0123456789-"
|
|
altered = []
|
|
for _ in range(2000):
|
|
n = random.randint(1, 63)
|
|
cand = random.choice("abcdefghijklmnopqrstuvwxyz") + "".join(
|
|
random.choice(alphabet) for _ in range(n - 1))
|
|
if C._PLUGIN_ID_RE.match(cand) and secure_filename(cand) != cand:
|
|
altered.append((cand, secure_filename(cand)))
|
|
assert not altered, f"secure_filename rewrote accepted ids: {altered[:5]}"
|
|
|
|
|
|
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
|
|
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
|