fix(security): close icons path-prefix bypass, log swallowed exceptions

/icons/image used a bare string startswith() against allowed roots, so
a sibling dir like /usr/share/icons_evil would pass as if it were under
/usr/share/icons. Switched to the pathlib parents-based check already
used correctly in icons/compositor.py, plus a regression test.

Also stopped three bare `except Exception: pass` blocks (auto model
select fallback, conversation titling) from swallowing errors silently
- they now log to the existing chat trace helper. Behavior unchanged,
just visible when something's actually failing.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
2026-08-25 19:26:08 -05:00
co-authored by Claude Sonnet 5
parent 9bc1c51734
commit 50738b139a
2 changed files with 43 additions and 7 deletions
+30
View File
@@ -216,6 +216,36 @@ def test_icon_source_requires_real_allowed_file_boundary(tmp_path):
module._ALLOWED_ROOTS[:] = old_roots
def test_icons_image_endpoint_requires_real_allowed_root_boundary(tmp_path):
# Sibling directories that merely share a string prefix with an allowed
# root (e.g. "icons-other" vs "icons") must not pass the check.
from synapse import main
allowed = tmp_path / "icons"
allowed.mkdir()
source = allowed / "app.svg"
source.write_text("<svg />")
sibling = tmp_path / "icons-other"
sibling.mkdir()
evil = sibling / "app.svg"
evil.write_text("<svg />")
old_roots = list(main._ALLOWED_ICON_ROOTS)
main._ALLOWED_ICON_ROOTS[:] = [str(allowed)]
try:
client = TestClient(app)
ok = client.get("/icons/image", params={"path": str(source)})
assert ok.status_code == 200
blocked = client.get("/icons/image", params={"path": str(evil)})
assert blocked.status_code == 403
missing = client.get("/icons/image", params={"path": str(allowed / "missing.svg")})
assert missing.status_code in (403, 404)
finally:
main._ALLOWED_ICON_ROOTS[:] = old_roots
def test_ollama_stream_propagates_transport_errors(monkeypatch):
"""A failing stream must surface, not be swallowed into an empty reply —
and it must carry Ollama's own explanation, since that is the only part the