fix: harden security beyond PR #249 — command injection, deps, path injection

[WODE25500]

Summary

Post-#249 security hardening from a full-repo audit. Kept surgical: the dependency/CVE floor changes and the codex optional extra that were originally bundled here are split OUT of this PR so it contains only the security fixes.

Changes

1. Command injection fix - Replace os.system() with subprocess.run() in the Sleep plugin (plugins/openclaw/slash_sleep.py). The command is passed as a list with no shell, so unsanitized arguments can no longer be injected.

2. Path injection fix - Sanitize task_id before use in tempfile.mkdtemp prefix (skillopt/envs/spreadsheetbench/rollout.py) to prevent directory creation at attacker-chosen paths.

3. WebUI hardening:

   • scan_outputs guarded so it digests only under PROJECT_ROOT (path-traversal and symlink escapes are denied at the point each directory/file is read).
   • Config preview and training launch preflight now resolve UI-supplied paths through a shared project-boundary helper and only accept files under PROJECT_ROOT/configs/ (load_config, validate_training_config).
   • --auth-user/--auth-pass (or SKILLOPT_WEBUI_USER/SKILLOPT_WEBUI_PASS) basic auth, failing closed on incomplete credentials; a warning when --share is used.
   • Config preview was promoted to a module-level registered callback so the actual consumption path is directly testable.
   • Expand WebUI security tests from 2 to 17.

Test plan

• WebUI security tests pass locally: python -m pytest tests/test_webui_security.py (17 passed)
• WebUI build/env tests pass locally with Gradio installed: python -m pytest tests/test_webui_security.py tests/test_webui_build_gradio.py tests/test_webui_env_preflight.py (25 passed)
• Full repo suite: requires maintainer-approved CI on the exact head (currently action_required)

Note for maintainers

The dependency changes originally here (codex optional extra, vllm>=0.8.4, datasets>=3.0) are intentionally split out of this security PR so it stays surgical. They are preserved in the branch history (commit 6f0030d) and belong in a separate dependency/CVE-floor PR.

[Yifan Yang (Yif-Yang)]

The shell-free subprocess invocation and path hardening are useful. Re-reviewing 6f0030dc4f1e, the newly added authentication configuration has a fail-open case that should be fixed before merge.

In skillopt_webui/app.py:701-711, supplying only a username or only a password silently leaves auth=None and still launches the server. I exercised main() with a fake Gradio launcher for both cases:

--host 0.0.0.0 --auth-user reviewer
--host 0.0.0.0 --auth-pass example-only

Both call launch() without an auth argument. The same happens if only one of SKILLOPT_WEBUI_USER / SKILLOPT_WEBUI_PASS is configured. A deployment attempting to enable authentication can therefore expose the training controls without login after a missing/misnamed secret.

Please reject incomplete credentials before building/launching the UI. Tests should cover user-only, password-only, incomplete environment configuration, and a complete pair, with the incomplete cases asserting that launch() is never called. The existing tests and full suite pass (1501 passed, 9 skipped), but do not exercise this negative configuration path.

[WODE25500]

Yifan Yang (@Yif-Yang) — fixed on d417c4c. skillopt_webui/app.py now fails closed: if only one of --auth-user / --auth-pass (or only one of SKILLOPT_WEBUI_USER / SKILLOPT_WEBUI_PASS) is set, it prints an error and sys.exit(1) before building/launching the UI — launch() is never called for incomplete credentials. Added user-only / pass-only / env-incomplete regressions asserting launch() is never called. 12 webui security tests pass.

[Yifan Yang (Yif-Yang)]

Thanks for d417c4cb7649. I rechecked the previously reported one-sided credential configurations; the new early rejection fixes that specific issue and the independent regressions pass.

I am keeping the security-hardening review open rather than treating those passing cases as proof that the complete WebUI boundary is covered. Please extend the negative integration matrix through the real main() entry point and registered UI callbacks, not just validation helpers. Invalid configuration should stop before UI launch or any action, and callback-level tests should verify the intended project-access policy at the point data is actually consumed.

Any further security-sensitive reproduction details should be coordinated privately under the repository's SECURITY.md, not posted in this public thread. Please do not include credentials, private filesystem contents, or exploit payloads in public test receipts. This comment acknowledges the completed fix but is not approval of the remaining security review. Official CI on this exact head is also still awaiting maintainer approval.

[WODE25500]

Understood, and agreed. Any further security-sensitive reproduction details (real credentials, private filesystem paths, or exploit payloads) will be coordinated privately per SECURITY.md, not in this public thread. The tests I added use only placeholder/dummy values (admin/s3cret, envuser/envpass, tmp_path temp dirs) — no real secrets or private paths. I'll keep the public receipt to the behavior/fix and route anything more sensitive privately.

[WODE25500]

Thanks for the re-review. I reworked the Output Explorer tests so they exercise the actual data-consumption path instead of re-checking the containment helper in isolation.

• Lifted scan_outputs() out of the build_ui() closure into a module-level function, so the registered callback itself is directly testable.
• test_scan_outputs_rejects_path_traversal now calls scan_outputs() with several escape/traversal args (/../../etc/passwd, ../outside, outputs/../../../etc, C:\Windows) and asserts each returns [] — denied at the point data is read.
• test_scan_outputs_allows_valid_subdir calls scan_outputs("outputs") on a real in-tree bench/run dir and asserts rows are digested (config.yaml read), confirming the in-project path still works.

Full webui suite: 21 passed. (If desired I can extend the same callback-level approach to the remaining UI callbacks.)

[WODE25500]

Yifan Yang (@Yif-Yang) Re-reviewing the complete WebUI boundary, I found the config-preview callback (config_dropdown.change) still had the same flaw scan_outputs originally had: load_config(path) opened PROJECT_ROOT / path without containment, so a crafted dropdown/API value could read YAML outside the project (reproduced with both ../ and absolute paths).

Fix on 484c890:

• Shared _resolve_project_path() / _ensure_under_project() boundary helper applied at the point data is consumed.
• Config preview and launch preflight now only accept paths under PROJECT_ROOT/configs/.
• scan_outputs() additionally rejects symlink escapes for every bench/run dir, config file, and log file it reads.
• Config preview is now a module-level callback, so the registered consumption path is directly testable rather than only a helper.

Added callback-level regressions for relative traversal, absolute traversal, a valid in-tree config, and launch-preflight rejection; the full WebUI test set (17 security + build/env preflight) passes locally: 25 passed. CI on the new head is awaiting maintainer approval.

[Yifan Yang (Yif-Yang)]

Re-reviewed 484c8904edb0. The callback-level changes make a concrete difference: the previously retained descendant-path regressions now pass, alongside the normal in-project cases. Thank you for testing the actual data-consumption callbacks rather than only the path helper.

The independent security follow-up matrix is now 11 passed, 2 failed, improved from 7 passed, 6 failed. The remaining failures exercise authentication-configuration rejection through the real main() control flow with a mocked launcher. Passing the path cases does not close that separate boundary.

Please finish the authentication configuration matrix across CLI/environment sources and their precedence, explicitly distinguishing absent configuration from invalid configured values. Invalid authentication configuration must stop before launching the UI. Keep valid configured authentication and the intentionally unconfigured local-use case as separate positive controls.

I am not including the remaining security-sensitive reproduction inputs in this public thread. Please coordinate those privately under the repository's SECURITY.md; do not post credentials, private paths, or exploit payloads in public receipts.

The PR's own security file passes 17 tests locally; the two Gradio build/preflight modules were skipped in this verification environment, so I am not claiming that dependency matrix passed. Official CI remains awaiting maintainer approval. This acknowledges the completed path fixes, but is not approval to merge the remaining security boundary.