fix: harden security beyond PR #249 — command injection, deps, path injection by WODE25500 · Pull Request #264 · microsoft/SkillOpt

WODE25500 · 2026-09-01T20:06:06Z

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

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.
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.
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, resolved per field with explicit precedence (a flag wins over its variable). "Not supplied" and "supplied but blank" are different states: incomplete or blank configuration, from any mix of sources, stops before the UI is even built. A blank pair previously left both sides falsy and launched with no authentication at all. A warning is emitted 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 100, including a full decision table over CLI/environment x username/password x absent/blank/supplied (81 combinations, 24 of which fail against the previously proposed resolution).

Test plan

python -m pytest tests/test_webui_security.py - 100 passed
Full suite on Linux / Python 3.11 - 1519 passed, 12 skipped (self-run scratch workflow; the workflow file is not part of this PR)
The Gradio build / env-preflight modules (test_webui_build_gradio.py, test_webui_env_preflight.py) need the webui extra and have not been re-run since this change
Upstream CI on the exact head still reports action_required pending maintainer approval

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.

…s, path injection Five additional security hardening changes identified during a full repository security audit: 1. Replace os.system() with subprocess.run() in Sleep plugin (plugins/openclaw/slash_sleep.py) to prevent shell command injection via unsanitized arguments. 2. Raise dependency floors to address known CVEs: - vllm >= 0.8.4 (was 0.4.0; CVE-2025-32433 in transitive deps) - datasets >= 3.0 (was 2.18.0; remote code execution via load_dataset with untrusted configs) - Declare openai-codex-sdk as an explicit optional dep (codex extra) to prevent dependency confusion / undeclared-import attacks. 3. Sanitize task_id before use in tempfile.mkdtemp prefix (skillopt/envs/spreadsheetbench/rollout.py) to prevent directory creation at attacker-chosen paths via crafted task identifiers. 4. Extend WebUI security tests from 2 to 8, covering --share warning, auth via CLI args / env vars, default-no-auth, and path traversal rejection in scan_outputs(). 5. Sync requirements.txt commented versions with pyproject.toml floors. All 1445 existing tests pass; 6 new regression tests added.

Yifan Yang (Yif-Yang) · 2026-09-05T16:42:00Z

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.

Supplying only --auth-user or only --auth-pass (or only one of SKILLOPT_WEBUI_USER / SKILLOPT_WEBUI_PASS) previously left auth=None and still launched the UI — a deployment could expose the training controls without login. Now reject before building/launching (sys.exit 1); launch() is never called for incomplete credentials. Added user-only / pass-only / env-incomplete regressions.

WODE25500 · 2026-09-05T17:05:36Z

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) · 2026-09-05T17:51:37Z

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 · 2026-09-05T21:35:30Z

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.

…oint Lift scan_outputs out of the build_ui closure so the Output Explorer callback is directly testable, and add callback-level tests that call it with traversal args (denied, returns []) and a valid in-tree output area (digested, reads config.yaml). This replaces the prior approximation tests that only re-checked relative_to() in isolation.

WODE25500 · 2026-09-05T21:49:18Z

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.)

The codex optional extra and the vllm/datasets floor bumps are dependency hygiene / CVE-floor changes, not part of the command-injection and path- injection hardening. Keep this PR surgical (the four security fixes + WebUI tests); the dependency changes are preserved in the branch history (commit 6f0030d) for a separate dependency PR. The gradio floor comment stays as-is (already synced to pyproject's 5.50.0 floor).

UI callbacks must not trust Gradio component values: config preview and launch preflight now resolve paths through a shared boundary helper and only accept configs/ files, while scan_outputs also rejects symlink escapes at every directory/file it reads. Config preview was promoted to a module-level callback so the registered consumption path is directly testable.

WODE25500 · 2026-09-08T10:08:23Z

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) · 2026-09-12T19:52:55Z

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.

`resolve_auth()` replaces the `args.auth_user or os.environ.get(...)` pair: - Precedence is explicit and per field: a CLI flag wins over its environment variable. - "Not supplied" and "supplied but blank" are distinct states. Previously `--auth-user "" --auth-pass ""`, or a pair of blank environment variables, left both sides falsy and launched the UI with no authentication at all, and an explicitly blank flag fell back to the environment. - Incomplete configuration still stops before the UI launches, now through a single error path that names the source that was set. `main()` keeps the fail-closed `sys.exit(1)` and its message now separates "configure both to require login" from "omit both to run without". Tests: the `main()` matrix covers CLI/env/field-mixed acceptance, per-field precedence, and the incomplete and blank rejections. Four of the new cases fail against the previous resolution.

WODE25500 · 2026-09-13T00:24:13Z

Yifan Yang (@Yif-Yang) — fixed on 3f86d6f. The remaining gap was that "not supplied" and "supplied but blank" were the same value to args.auth_user or os.environ.get(...), and that shape fails open rather than merely being incomplete.

skillopt_webui/app.py now resolves credentials in resolve_auth():

precedence is per field and explicit: a flag wins over its environment variable;
a field counts as absent only when neither the flag nor the variable was supplied, so --auth-user "" is a configured-but-invalid value and no longer falls back to SKILLOPT_WEBUI_USER;
blank values (empty or whitespace-only) are rejected instead of being read as unset. Previously --auth-user "" --auth-pass "", or a pair of blank variables, left both sides falsy and launched the UI with no authentication at all — a deployment that fills these from an empty secret would come up open;
incomplete configuration still stops before build_ui() / launch(), now through one error path that names the source that was set. main() keeps sys.exit(1), and its message separates "configure both to require login" from "omit both to run without".

tests/test_webui_security.py, all through the real main() with a mocked builder and launcher:

acceptance matrix: CLI-only, env-only and field-mixed (CLI username + env password) each launch with auth, plus per-field precedence when both sources are set;
rejection matrix: five incomplete shapes and five blank shapes each exit 1 without launching;
the four blank cases fail against the previous resolution (assert None == 1), so they are regression tests rather than restatements;
your earlier incomplete CLI/env tests and the unconfigured local-use control are unchanged.

Verification on Linux / Python 3.11 running this repository's own CI commands — scratch branch off this head, deleted afterwards, workflow file not part of this PR:

tests/test_webui_security.py: 29 passed
full suite: 1519 passed, 12 skipped
run: https://github.com/WODE25500/SkillOpt/actions/runs/34727810070

Same caveat you raised, stated plainly: that run installs only .[dev] like the test job, so I am not claiming the two Gradio build/preflight modules passed — they belong to the separate webui job and upstream CI for this head is still awaiting maintainer approval.

On coordinating the rest privately: the configuration shape is what I would need to reproduce it — which source supplied which field, and which of them were empty. That is not credential or payload material. If your matrix still fails after 3f86d6f, send the shape and I will fix it here; anything that is genuinely a live exploitation detail I will take to SECURITY.md as you asked.

The rejection matrix asserted that launch() was never called, which leaves a refactor free to construct the UI and then refuse. The probe now hands back the mocked build_ui so the rejection cases also assert it was never called.

WODE25500 · 2026-09-13T00:40:15Z

Yifan Yang (@Yif-Yang) — one more from a self-review pass, test-side only, on 9adad57.

The rejection matrix asserted that launch() was never called, which still leaves a refactor free to build the UI and only then refuse. The probe now hands back the mocked build_ui as well, and every rejection case asserts it was never called, so "stops before the UI" is checked at both ends rather than only at the launch call.

Re-verified on Linux / Python 3.11 (scratch branch off this head, deleted afterwards; workflow file not part of this PR):

tests/test_webui_security.py: 29 passed
full suite: 1519 passed, 12 skipped
run: https://github.com/WODE25500/SkillOpt/actions/runs/34728495110

The positive controls are unchanged: valid CLI, valid env, and field-mixed configurations all launch with auth, and the intentionally unconfigured local run still launches without it.

The rejection matrix listed ten shapes. The contract is a table over four inputs - CLI and environment, each supplying a username and a password, each either absent, supplied-but-blank or supplied - so enumerate all 81 combinations against a restated decision table instead. Ten shapes covered the families I could think of; the sweep is what covers the boundary. Against the pre-fix resolution this reports 24 failures, every one of them a launch with missing, blank or environment-substituted credentials. Against the current resolution it reports none. Precedence keeps its own test: the sweep uses the same literal on both sources, so it cannot pin which one actually won.

WODE25500 · 2026-09-13T00:53:36Z

Yifan Yang (@Yif-Yang) — this round is about the matrix itself rather than a new input, on f93ddd1.

I replaced the ten-shape rejection list with the whole decision table: four inputs — CLI and environment, each supplying a username and a password — each either absent, supplied-but-blank or supplied, so all 81 combinations go through the real main() and are compared against a restated decision table.

against the pre-fix resolution (484c890) the sweep reports 24 failures
against this head it reports 0

All 24 were launches with missing, blank or environment-substituted credentials — the family you were pointing at. If your remaining two cases are configuration-shape cases rather than something structurally different, they should fall inside those 81. The table now lives in tests/test_webui_security.py, so it is re-checked on every run instead of being a one-off receipt.

Honest caveat: _expected_auth_outcome is my own restatement of the contract, in the same file, so a shared misreading would pass both sides. What rules that out is the pre-fix run — the same table fails 24 times against the previous resolution.

Precedence still has its own test, because the sweep uses the same literal on both sources and cannot tell which one actually won.

Re-verified on Linux / Python 3.11 (scratch branch, deleted afterwards; workflow file not part of this PR):

tests/test_webui_security.py: 100 passed
full suite: 1590 passed, 12 skipped
run: https://github.com/WODE25500/SkillOpt/actions/runs/34729071395

WODE25500 and others added 2 commits September 6, 2026 23:35

Navigation Menu

Uh oh!

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

fix: harden security beyond PR #249 — command injection, deps, path injection#264
WODE25500 wants to merge 8 commits into
microsoft:mainfrom
WODE25500:fix/security-hardening-beyond-pr249

WODE25500 commented Sep 1, 2026 •

edited

Loading

Uh oh!

Yifan Yang (Yif-Yang) commented Sep 5, 2026

Uh oh!

WODE25500 commented Sep 5, 2026

Uh oh!

Yifan Yang (Yif-Yang) commented Sep 5, 2026

Uh oh!

WODE25500 commented Sep 5, 2026

Uh oh!

WODE25500 commented Sep 5, 2026

Uh oh!

WODE25500 commented Sep 8, 2026

Uh oh!

Yifan Yang (Yif-Yang) commented Sep 12, 2026

Uh oh!

WODE25500 commented Sep 13, 2026

Uh oh!

WODE25500 commented Sep 13, 2026

Uh oh!

WODE25500 commented Sep 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

WODE25500 commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Test plan

Note for maintainers

Uh oh!

Yifan Yang (Yif-Yang) commented Sep 5, 2026

Uh oh!

WODE25500 commented Sep 5, 2026

Uh oh!

Yifan Yang (Yif-Yang) commented Sep 5, 2026

Uh oh!

WODE25500 commented Sep 5, 2026

Uh oh!

WODE25500 commented Sep 5, 2026

Uh oh!

WODE25500 commented Sep 8, 2026

Uh oh!

Yifan Yang (Yif-Yang) commented Sep 12, 2026

Uh oh!

WODE25500 commented Sep 13, 2026

Uh oh!

WODE25500 commented Sep 13, 2026

Uh oh!

WODE25500 commented Sep 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

WODE25500 commented Sep 1, 2026 •

edited

Loading