Skip to content

fix(cookbook): validate model_dir segments in /api/model/cached#5529

Open
archdex-art wants to merge 1 commit into
odysseus-dev:devfrom
archdex-art:fix/cookbook-model-dir-validation
Open

fix(cookbook): validate model_dir segments in /api/model/cached#5529
archdex-art wants to merge 1 commit into
odysseus-dev:devfrom
archdex-art:fix/cookbook-model-dir-validation

Conversation

@archdex-art

@archdex-art archdex-art commented Jul 14, 2026

Copy link
Copy Markdown

Summary

GET /api/model/cached validates host and ssh_port (validate_remote_host, validate_ssh_port) but never validated the model_dir query param before splitting it on commas and embedding each segment into the generated cache-scan script — unlike the sibling POST /api/model/download endpoint, which runs its local_dir param through _validate_local_dir. Paths reach the generated script via Python repr() rather than shell interpolation, so this was not a shell-injection RCE, but the inconsistency meant an invalid segment (shell metacharacters, a --prefixed path) could still reach the scanner and trigger an unintended filesystem walk instead of a clean 400. This extracts the split/normalize/validate logic into a new _parse_model_dirs() helper that reuses _validate_local_dir so both endpoints enforce identical rules.

Target branch

  • This PR targets dev, not main.

Linked Issue

Fixes #5508

Type of Change

  • Bug fix (non-breaking — fixes a confirmed issue)
  • New feature (non-breaking — adds new behaviour)
  • Breaking change (changes or removes existing behaviour)
  • Refactor / cleanup (behaviour unchanged)
  • Documentation only
  • CI / tooling / configuration

Checklist

  • I searched open issues and open PRs — this is not a duplicate.
  • This PR targets dev
  • My changes are limited to the scope described above — no unrelated refactors or whitespace changes mixed in.
  • I actually ran the app (docker compose up or uvicorn app:app) and verified the change works end-to-end. Type-checks and unit tests are not enough.

How to Test

  1. python -m pytest tests/test_cookbook_helpers.py — 73 passed (5 new tests added for _parse_model_dirs), 1 pre-existing skip.
  2. python -m pytest tests/ -k cookbook — 223 passed, 1 pre-existing skip, 1 unrelated pre-existing failure (confirmed via git stash against unmodified dev: test_container_opt_in_with_unix_socket_is_allowed fails with OSError: AF_UNIX path too long, a macOS pytest-tmp-path-length artifact in a Docker-socket test, nothing to do with this change).
  3. Live app run: started with AUTH_ENABLED=false ODYSSEUS_DATA_DIR=/tmp/odysseus-data uvicorn app:app --host 127.0.0.1 --port 7001, then hit the real endpoint:
    • curl "http://127.0.0.1:7001/api/model/cached?model_dir=/tmp/odysseus-data"HTTP 200, real scan results returned.
    • curl "http://127.0.0.1:7001/api/model/cached?model_dir=/tmp/x%3Btouch%20/tmp/pwned" (shell metacharacter) → HTTP 400, {"detail":"Invalid local_dir — must be an absolute or ~ path with no shell metacharacters"} — rejected before reaching the scan script.
    • curl "http://127.0.0.1:7001/api/model/cached?model_dir=/models/-rf" (leading-dash segment / option injection) → HTTP 400, {"detail":"Invalid local_dir — path segments cannot start with '-'"}.
    • curl "http://127.0.0.1:7001/api/model/cached?model_dir=/tmp/odysseus-data,/tmp/odysseus-data" (multi-segment) → HTTP 200, confirms comma-splitting still works for valid paths.
  4. python -m py_compile routes/cookbook_helpers.py routes/cookbook_routes.py — passes.

Visual / UI changes — REQUIRED if you touched anything that renders

Not applicable — backend-only change (Python route handler + helper), no rendering code touched.

The /api/model/cached endpoint split model_dir on commas and normalized
bare Linux paths, but never validated segments before embedding them into
the generated scan script — unlike the sibling /api/model/download
endpoint, which runs local_dir through _validate_local_dir.

Extracts the split/normalize/validate logic into a new _parse_model_dirs()
helper (routes/cookbook_helpers.py, alongside _validate_local_dir and
_cached_model_scan_script, which it now composes) so both endpoints share
identical validation: shell metacharacters and leading-dash (option
injection) segments are rejected with a 400 instead of reaching the
generated scanner. Paths were already embedded via Python repr rather than
shell-interpolated, so this was not an RCE — but the inconsistency meant
invalid paths could still trigger unintended filesystem walks.

The route handler is now a one-line call: model_dirs = _parse_model_dirs(model_dir).

Fixes odysseus-dev#5508
@github-actions github-actions Bot added needs work PR description incomplete — please update before review ready for review Description complete — ready for maintainer review and removed needs work PR description incomplete — please update before review labels Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for review Description complete — ready for maintainer review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cookbook /api/model/cached accepts unvalidated model_dir paths

1 participant