-
-
Notifications
You must be signed in to change notification settings - Fork 40
validate target_remote at the install API boundary: hostile strings flow into backend daemon URLs across ALL resolve_*_url helpers #2467
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| ### Fixed | ||
| - `POST /api/store/install-v2` now validates `target_remote` at the API boundary before it is interpolated into backend daemon URLs (`resolve_rkllama_url`, LXC remote addressing). Hostile strings containing `:`, `/`, `?`, `#`, or `@` are rejected with HTTP 400 and a named `invalid_target_remote` reason, preventing SSRF-shaped installs or silent mis-routing to unregistered workers. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
|
|
||
| import asyncio | ||
| import logging | ||
| import re | ||
| from dataclasses import asdict | ||
| from pathlib import Path | ||
| from urllib.parse import urlparse | ||
|
|
@@ -46,6 +47,13 @@ | |
| "hailo-ollama", | ||
| } | ||
|
|
||
| # Strict hostname charset for target_remote fallback. Rejects host/path/port | ||
| # injection (":", "/", "?", "#", "@") so an authenticated caller cannot steer | ||
| # a pull at an arbitrary daemon URL (SSRF-shaped) or route an install to a | ||
| # typo'd host silently. A bare hostname still passes so registry-less | ||
| # installs (e.g. ad-hoc worker hostnames) keep working. | ||
| _HOSTNAME_RE = re.compile(r"^[A-Za-z0-9._-]+$") | ||
|
|
||
| # Backend ID → install method known to get_installer(). | ||
| # rkllama has a purpose-built installer (calls /api/pull, manages symlinks, | ||
| # restarts systemd units). rk-llama-cpp models are downloaded to disk and | ||
|
|
@@ -735,9 +743,49 @@ async def install_app(request: Request): | |
| body = await request.json() | ||
| manifest_id = body.get("manifest_id") or body.get("app_id") | ||
| variant_id = body.get("variant_id", "auto") | ||
| target_remote = body.get("target_remote") or None | ||
| raw_target_remote = body.get("target_remote") | ||
| if raw_target_remote is not None and not isinstance(raw_target_remote, str): | ||
| # JSON allows any type here; a truthy non-string (123, true, [...]) | ||
| # would crash the regex below with TypeError → 500. Reject cleanly. | ||
| return JSONResponse( | ||
| { | ||
| "error": ( | ||
| "target_remote must be a string, 'local', or omitted; " | ||
| f"got {type(raw_target_remote).__name__}" | ||
| ), | ||
| "reason": "invalid_target_remote", | ||
| }, | ||
| status_code=400, | ||
| ) | ||
| target_remote = raw_target_remote or None | ||
| force = bool(body.get("force", False)) | ||
|
|
||
| # Boundary validation — reject hostile target_remote before it is | ||
| # interpolated into any backend daemon URL by resolve_rkllama_url or the | ||
| # lxc <target_remote>:<name> addressing. An authenticated caller must not | ||
| # be able to inject an arbitrary host/path/port (SSRF-shaped) or silently | ||
| # route an install to an unregistered worker via a degenerate stub | ||
| # capability. Accepted values: | ||
| # * None / empty / "local" -> local install | ||
| # * a registered cluster worker id (cluster.get_worker() non-None) | ||
| # * a bare hostname matching _HOSTNAME_RE (registry-less installs) | ||
| if target_remote and target_remote != "local": | ||
| cluster = getattr(request.app.state, "cluster_manager", None) | ||
| is_known_worker = False | ||
| if cluster is not None and hasattr(cluster, "get_worker"): | ||
| is_known_worker = cluster.get_worker(target_remote) is not None | ||
| if not is_known_worker and not _HOSTNAME_RE.match(target_remote): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
python - <<'PY'
import re
pattern = re.compile(r"^[A-Za-z0-9._-]+$")
assert pattern.match("edge-host\n")
assert not pattern.fullmatch("edge-host\n")
PYRepository: jaylfc/taOS Length of output: 149 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- target validation and regex ---'
sed -n '1,120p' tinyagentos/routes/store_install.py
sed -n '730,805p' tinyagentos/routes/store_install.py
printf '%s\n' '--- related tests and route behavior ---'
rg -n -C 3 '_HOSTNAME_RE|target_remote|is_known_worker|HTTPException|status_code.*400|store_install' tinyagentos tests 2>/dev/null | head -n 300Repository: jaylfc/taOS Length of output: 28347 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- install route continuation ---'
sed -n '805,930p' tinyagentos/routes/store_install.py
printf '%s\n' '--- worker lookup definition and callers ---'
rg -n -C 5 'def get_worker|async def get_worker|get_worker\(' tinyagentos tests
printf '%s\n' '--- store-install tests ---'
git ls-files | rg '(^|/)(test.*store|store.*test|test.*install|install.*test)'
rg -n -C 4 'install-v2|target_remote.*invalid|invalid_target_remote|edge-host|target_remote' tests | head -n 350Repository: jaylfc/taOS Length of output: 50368 Reject target names with a trailing newline. When 🤖 Prompt for AI Agents |
||
| return JSONResponse( | ||
| { | ||
| "error": ( | ||
| "target_remote must be 'local' or a registered worker id; " | ||
| f"got {target_remote!r}" | ||
| ), | ||
| "reason": "invalid_target_remote", | ||
| }, | ||
| status_code=400, | ||
| ) | ||
|
|
||
| # Progress store — opened up here so both legacy and v2 paths can | ||
| # tag work-in-flight that the frontend polls for. Importing here | ||
| # avoids a circular import at module load. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover
Noneand empty local targets.The docstring states that
"local",None, and""bypass validation. This test sends only"local". Parametrize the request with all three values so JSONnulland an empty string remain accepted.Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents