Apply Opus pre-release SHOULD-FIX (absorbed in stage-299)
Per Opus advisor on stage-299:
1. Bounded WIKI_PATH walk + forbidden-root guard (api/routes.py)
- _LLM_WIKI_MAX_FILES = 10000 caps rglob iteration (prevents hangs on
symlink loops or pathologically-large trees)
- _LLM_WIKI_FORBIDDEN_ROOTS blocklist refuses '/' '/etc' '/usr' '/var'
'/opt' '/sys' '/proc' even if WIKI_PATH is misconfigured to point
at them
- Self-DoS prevention: /api/wiki/status fires on every Insights tab
open via Promise.all, and unbounded rglob would block the endpoint
2. URL-scheme guard for docs_url interpolation (static/panels.js)
- rawDocsUrl is regex-validated against /^https?:\/\//i before being
interpolated into the <a href=> attribute
- esc() HTML-escapes but doesn't validate URL scheme; docs_url is
server-controlled today but the contributor scaffolded it for
potential config-driven use, so future-proof against javascript:
scheme XSS
6 regression tests in tests/test_stage299_opus_fixes.py pin both fixes.
This commit is contained in:
@@ -1804,6 +1804,15 @@ def _llm_wiki_config_path() -> str | None:
|
||||
)
|
||||
|
||||
|
||||
# Cap WIKI walks to prevent self-DoS if WIKI_PATH points at /, /etc, /home, etc.
|
||||
# Real LLM wikis have under a few thousand files; 10k is generous and catches misconfig.
|
||||
_LLM_WIKI_MAX_FILES = 10000
|
||||
# Refuse to walk these system roots even if explicitly configured.
|
||||
_LLM_WIKI_FORBIDDEN_ROOTS = frozenset(
|
||||
str(Path(p).expanduser().resolve()) for p in ("/", "/etc", "/usr", "/var", "/opt", "/sys", "/proc")
|
||||
)
|
||||
|
||||
|
||||
def _llm_wiki_resolve_path() -> tuple[Path, str, bool]:
|
||||
hermes_home = _llm_wiki_active_hermes_home()
|
||||
raw = os.getenv("WIKI_PATH") or _llm_wiki_env_file_path(hermes_home)
|
||||
@@ -1832,8 +1841,20 @@ def _llm_wiki_safe_iso(ts: float | None) -> str | None:
|
||||
def _llm_wiki_count_files(root: Path) -> int:
|
||||
if not root.exists() or not root.is_dir():
|
||||
return 0
|
||||
# Defense in depth: refuse to walk forbidden system roots even if WIKI_PATH
|
||||
# was set to one. The endpoint is auth-gated but a misconfigured server
|
||||
# shouldn't self-DoS by rglob'ing all of /etc on every Insights load.
|
||||
try:
|
||||
if str(root.resolve()) in _LLM_WIKI_FORBIDDEN_ROOTS:
|
||||
return 0
|
||||
except Exception:
|
||||
return 0
|
||||
count = 0
|
||||
iterated = 0
|
||||
for item in root.rglob("*"):
|
||||
iterated += 1
|
||||
if iterated > _LLM_WIKI_MAX_FILES:
|
||||
break # bounded — prevents hangs on symlink loops or huge trees
|
||||
try:
|
||||
if item.is_file() and not any(part.startswith(".") for part in item.relative_to(root).parts):
|
||||
count += 1
|
||||
@@ -1844,11 +1865,21 @@ def _llm_wiki_count_files(root: Path) -> int:
|
||||
|
||||
def _llm_wiki_page_files(wiki_path: Path) -> list[Path]:
|
||||
pages: list[Path] = []
|
||||
# Defense in depth: refuse forbidden system roots.
|
||||
try:
|
||||
if str(wiki_path.resolve()) in _LLM_WIKI_FORBIDDEN_ROOTS:
|
||||
return pages
|
||||
except Exception:
|
||||
return pages
|
||||
iterated = 0
|
||||
for dirname in _LLM_WIKI_PAGE_DIRS:
|
||||
section = wiki_path / dirname
|
||||
if not section.exists() or not section.is_dir():
|
||||
continue
|
||||
for item in section.rglob("*.md"):
|
||||
iterated += 1
|
||||
if iterated > _LLM_WIKI_MAX_FILES:
|
||||
return pages # bounded
|
||||
try:
|
||||
rel = item.relative_to(section)
|
||||
if item.is_file() and not any(part.startswith(".") for part in rel.parts):
|
||||
|
||||
@@ -2141,7 +2141,10 @@ function _renderLlmWikiStatus(d) {
|
||||
const isError = status.status === 'error';
|
||||
const badgeClass = isReady ? 'ok' : isError ? 'err' : isEmpty ? 'warn' : 'muted';
|
||||
const badgeText = isReady ? 'Available' : isError ? 'Error' : isEmpty ? 'Empty' : 'Unavailable';
|
||||
const docsUrl = status.docs_url || 'https://hermes-agent.nousresearch.com/docs/user-guide/skills/bundled/research/research-llm-wiki';
|
||||
const rawDocsUrl = status.docs_url || 'https://hermes-agent.nousresearch.com/docs/user-guide/skills/bundled/research/research-llm-wiki';
|
||||
// Guard against unsafe URL schemes (e.g. js: / data:) if docs_url ever
|
||||
// becomes config-driven. esc() HTML-escapes but doesn't validate URL scheme.
|
||||
const docsUrl = /^https?:\/\//i.test(rawDocsUrl) ? rawDocsUrl : '#';
|
||||
const toggleNote = status.toggle_available
|
||||
? 'Toggle available from configured Hermes Agent setting.'
|
||||
: (status.toggle_reason || 'No stable LLM Wiki on/off config flag was detected, so this panel is read-only.');
|
||||
|
||||
85
tests/test_stage299_opus_fixes.py
Normal file
85
tests/test_stage299_opus_fixes.py
Normal file
@@ -0,0 +1,85 @@
|
||||
"""Regression test for the Opus SHOULD-FIX bounds applied in stage-299.
|
||||
|
||||
PR #1664 introduced /api/wiki/status with `_llm_wiki_count_files` and
|
||||
`_llm_wiki_page_files` that walk WIKI_PATH via `rglob`. Without bounds,
|
||||
a misconfigured WIKI_PATH=/ or symlink loop would hang the endpoint.
|
||||
|
||||
These tests pin the defenses applied per Opus advisor on stage-299:
|
||||
- A constant cap on iteration (_LLM_WIKI_MAX_FILES) for both functions
|
||||
- A forbidden-roots blocklist (_LLM_WIKI_FORBIDDEN_ROOTS) that includes
|
||||
'/' / '/etc' / '/usr' / '/var' / '/opt' / '/sys' / '/proc' (resolved
|
||||
to absolute strings)
|
||||
- Bounded behavior: if WIKI_PATH points at a forbidden root, both
|
||||
functions return 0/empty without iterating
|
||||
"""
|
||||
from pathlib import Path
|
||||
|
||||
ROUTES_PY = Path(__file__).parent.parent / "api" / "routes.py"
|
||||
|
||||
|
||||
def _read_source():
|
||||
return ROUTES_PY.read_text()
|
||||
|
||||
|
||||
def test_wiki_max_files_constant_present():
|
||||
src = _read_source()
|
||||
assert "_LLM_WIKI_MAX_FILES" in src
|
||||
assert "_LLM_WIKI_FORBIDDEN_ROOTS" in src
|
||||
# Make sure cap is reasonable (≥ a few thousand, ≤ 100k)
|
||||
assert "10000" in src or "_LLM_WIKI_MAX_FILES = 10" in src
|
||||
|
||||
|
||||
def test_count_files_has_iteration_cap():
|
||||
src = _read_source()
|
||||
# Locate _llm_wiki_count_files body
|
||||
start = src.find("def _llm_wiki_count_files(")
|
||||
end = src.find("\ndef ", start + 1)
|
||||
body = src[start:end]
|
||||
assert "_LLM_WIKI_MAX_FILES" in body
|
||||
assert "_LLM_WIKI_FORBIDDEN_ROOTS" in body
|
||||
assert "iterated > _LLM_WIKI_MAX_FILES" in body or "iterated >= _LLM_WIKI_MAX_FILES" in body
|
||||
|
||||
|
||||
def test_page_files_has_iteration_cap():
|
||||
src = _read_source()
|
||||
start = src.find("def _llm_wiki_page_files(")
|
||||
end = src.find("\ndef ", start + 1)
|
||||
body = src[start:end]
|
||||
assert "_LLM_WIKI_MAX_FILES" in body
|
||||
assert "_LLM_WIKI_FORBIDDEN_ROOTS" in body
|
||||
|
||||
|
||||
def test_forbidden_roots_includes_system_paths():
|
||||
src = _read_source()
|
||||
# Find the constant definition
|
||||
start = src.find("_LLM_WIKI_FORBIDDEN_ROOTS = ")
|
||||
end = src.find(")\n", start) + 1
|
||||
decl = src[start:end + 1]
|
||||
for forbidden in ("/", "/etc", "/usr", "/var"):
|
||||
assert f'"{forbidden}"' in decl, f"Forbidden root {forbidden!r} not in _LLM_WIKI_FORBIDDEN_ROOTS"
|
||||
|
||||
|
||||
def test_count_files_returns_zero_for_forbidden_root(tmp_path, monkeypatch):
|
||||
"""Behavioral test: walking a forbidden root returns 0 without iterating."""
|
||||
import importlib
|
||||
routes = importlib.import_module("api.routes")
|
||||
|
||||
forbidden_root = Path("/etc")
|
||||
if forbidden_root.exists(): # skip on systems without /etc (Windows)
|
||||
result = routes._llm_wiki_count_files(forbidden_root)
|
||||
assert result == 0, "Walking /etc should return 0 (forbidden root guard)"
|
||||
|
||||
|
||||
def test_render_llm_wiki_status_uses_url_scheme_guard():
|
||||
"""Opus SHOULD-FIX #1: docs_url interpolated into href must be scheme-guarded."""
|
||||
panels_js = (Path(__file__).parent.parent / "static" / "panels.js").read_text()
|
||||
# Find the _renderLlmWikiStatus function body
|
||||
start = panels_js.find("function _renderLlmWikiStatus")
|
||||
end = panels_js.find("\nfunction ", start + 1)
|
||||
body = panels_js[start:end]
|
||||
# Must use a scheme-guarded form, not raw esc()
|
||||
assert "/^https?:" in body or "test(rawDocsUrl)" in body or "test(docsUrl)" in body, (
|
||||
"Expected URL scheme guard (e.g. /^https?:\\/\\//.test(...)) before "
|
||||
"interpolating docsUrl into href to prevent javascript: scheme XSS "
|
||||
"if docs_url ever becomes config-driven."
|
||||
)
|
||||
Reference in New Issue
Block a user