From 6f0030dc4f1e2cf27f854c198257d2359ea9ae28 Mon Sep 17 00:00:00 2001 From: WODE25500 Date: Wed, 2 Sep 2026 04:05:39 +0800 Subject: [PATCH 1/3] =?UTF-8?q?fix:=20harden=20security=20beyond=20PR=20#2?= =?UTF-8?q?49=20=E2=80=94=20command=20injection,=20deps,=20path=20injectio?= =?UTF-8?q?n?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- plugins/openclaw/slash_sleep.py | 5 +- pyproject.toml | 6 +- requirements.txt | 4 +- skillopt/envs/spreadsheetbench/rollout.py | 3 +- skillopt_webui/app.py | 31 ++++++- tests/test_webui_security.py | 102 ++++++++++++++++++++++ 6 files changed, 142 insertions(+), 9 deletions(-) diff --git a/plugins/openclaw/slash_sleep.py b/plugins/openclaw/slash_sleep.py index c8576661..f0f84cd7 100755 --- a/plugins/openclaw/slash_sleep.py +++ b/plugins/openclaw/slash_sleep.py @@ -19,6 +19,7 @@ import json import os import shutil +import subprocess import sys from pathlib import Path from datetime import datetime @@ -104,8 +105,8 @@ def run_category(category: str, *, dry_run: bool = False) -> int: print(f"=== /sleep run {category}{' (dry-run)' if dry_run else ''} ===") print(f" cmd: {' '.join(cmd)}") - rc = os.system(" ".join(f'"{c}"' for c in cmd)) - return rc + result = subprocess.run(cmd) + return result.returncode def run_all(*, dry_run: bool = False) -> int: diff --git a/pyproject.toml b/pyproject.toml index 5d50b8fe..c43860b2 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -38,10 +38,12 @@ dependencies = [ alfworld = ["alfworld>=0.4.0", "gymnasium>=0.29.0"] # Claude model backend claude = ["claude-agent-sdk>=0.1.0", "json_repair>=0.61.0"] +# Codex model backend (via OpenAI Codex SDK) +codex = ["openai-codex-sdk>=0.1.0"] # Qwen local model backend (via vLLM) -qwen = ["vllm>=0.4.0", "json_repair>=0.61.0"] +qwen = ["vllm>=0.8.4", "json_repair>=0.61.0"] # SearchQA data materialization -searchqa = ["datasets>=2.18.0"] +searchqa = ["datasets>=3.0"] # Documentation site docs = ["mkdocs-material>=9.5.0", "mkdocstrings[python]>=0.24.0"] # WebUI dashboard diff --git a/requirements.txt b/requirements.txt index 5db9e702..d22eab1f 100644 --- a/requirements.txt +++ b/requirements.txt @@ -15,7 +15,7 @@ httpx>=0.27.0 # claude-agent-sdk>=0.1.0 # ── Optional: Qwen local model (via vLLM) ──────── -# vllm>=0.4.0 +# vllm>=0.8.4 # ── Optional: tolerant JSON repair for free-form output from non-OpenAI # backends (Claude/Qwen). Without it extract_json() falls back safely and @@ -24,7 +24,7 @@ httpx>=0.27.0 # json_repair>=0.61.0 # ── Optional: WebUI dashboard ──────────────────── -# gradio>=4.0.0 +# gradio>=5.50.0 # ── Optional: Documentation site ───────────────── # mkdocs-material>=9.5.0 diff --git a/skillopt/envs/spreadsheetbench/rollout.py b/skillopt/envs/spreadsheetbench/rollout.py index aff78938..edf8b96c 100644 --- a/skillopt/envs/spreadsheetbench/rollout.py +++ b/skillopt/envs/spreadsheetbench/rollout.py @@ -314,7 +314,8 @@ def process_one( # ── Stage 1: run ReAct agent on test case 1 ───────────────────── result["phase"] = "agent" - work_dir = tempfile.mkdtemp(prefix=f"react_{task_id}_") + safe_task_id = "".join(c if c.isalnum() or c in "-_" else "_" for c in str(task_id)) + work_dir = tempfile.mkdtemp(prefix=f"react_{safe_task_id}_") try: # Copy input so agent works in an isolated directory work_input = os.path.join(work_dir, os.path.basename(ip1)) diff --git a/skillopt_webui/app.py b/skillopt_webui/app.py index 75e0ef1d..93490baa 100644 --- a/skillopt_webui/app.py +++ b/skillopt_webui/app.py @@ -606,8 +606,13 @@ def scan_outputs(out_dir): rows = [] if not out_dir: return rows - base = PROJECT_ROOT / out_dir - if not base.exists(): + base = (PROJECT_ROOT / out_dir).resolve() + project_resolved = PROJECT_ROOT.resolve() + try: + base.relative_to(project_resolved) + except ValueError: + return rows + if not base.exists() or not base.is_dir(): return rows for bench_dir in sorted(base.iterdir()): if not bench_dir.is_dir(): @@ -668,6 +673,10 @@ def main(): parser.add_argument("--host", type=str, default="127.0.0.1", help="Server host. Default is localhost; use 0.0.0.0 " "to expose publicly (no auth, use with care).") + parser.add_argument("--auth-user", type=str, default=None, + help="Username for basic auth (or set SKILLOPT_WEBUI_USER).") + parser.add_argument("--auth-pass", type=str, default=None, + help="Password for basic auth (or set SKILLOPT_WEBUI_PASS).") args = parser.parse_args() if args.host and args.host not in ("127.0.0.1", "localhost", "::1"): @@ -679,8 +688,26 @@ def main(): file=sys.stderr, ) + if args.share: + print( + "⚠ warning: --share creates a public tunnel (gradio.live) with no " + "authentication by default. Anyone with the URL can start/stop " + "training and browse the filesystem via Output Explorer. " + "Use --auth-user / --auth-pass (or SKILLOPT_WEBUI_USER / " + "SKILLOPT_WEBUI_PASS) to require login.", + file=sys.stderr, + ) + + auth_user = args.auth_user or os.environ.get("SKILLOPT_WEBUI_USER") + auth_pass = args.auth_pass or os.environ.get("SKILLOPT_WEBUI_PASS") + auth = None + if auth_user and auth_pass: + auth = (auth_user, auth_pass) + app = build_ui() launch_kwargs = build_launch_kwargs(args.host, args.port, args.share) + if auth: + launch_kwargs["auth"] = auth app.launch(**launch_kwargs) diff --git a/tests/test_webui_security.py b/tests/test_webui_security.py index 5886da65..15f1139b 100644 --- a/tests/test_webui_security.py +++ b/tests/test_webui_security.py @@ -55,3 +55,105 @@ def test_main_warns_on_public_host(webui, monkeypatch, capsys): assert "warning" in captured.err.lower() _args, kwargs = launcher.call_args assert kwargs["server_name"] == "0.0.0.0" + + +def test_main_warns_on_share(webui, monkeypatch, capsys): + """--share must emit a public-tunnel warning.""" + webui_mod = webui + launcher = mock.MagicMock() + app_mock = mock.MagicMock() + app_mock.launch = launcher + monkeypatch.setattr(webui_mod, "build_ui", lambda: app_mock) + monkeypatch.setattr(sys, "argv", ["app.py", "--share"]) + + webui_mod.main() + + captured = capsys.readouterr() + assert "share" in captured.err.lower() + assert "public" in captured.err.lower() or "tunnel" in captured.err.lower() + + +def test_main_auth_via_cli_args(webui, monkeypatch): + """--auth-user and --auth-pass must enable Gradio basic auth.""" + webui_mod = webui + launcher = mock.MagicMock() + app_mock = mock.MagicMock() + app_mock.launch = launcher + monkeypatch.setattr(webui_mod, "build_ui", lambda: app_mock) + monkeypatch.setattr(sys, "argv", ["app.py", "--auth-user", "admin", "--auth-pass", "s3cret"]) + + webui_mod.main() + + _args, kwargs = launcher.call_args + assert kwargs.get("auth") == ("admin", "s3cret") + + +def test_main_auth_via_env(webui, monkeypatch): + """SKILLOPT_WEBUI_USER / SKILLOPT_WEBUI_PASS must enable auth without CLI args.""" + webui_mod = webui + launcher = mock.MagicMock() + app_mock = mock.MagicMock() + app_mock.launch = launcher + monkeypatch.setattr(webui_mod, "build_ui", lambda: app_mock) + monkeypatch.setattr(sys, "argv", ["app.py"]) + monkeypatch.setenv("SKILLOPT_WEBUI_USER", "envuser") + monkeypatch.setenv("SKILLOPT_WEBUI_PASS", "envpass") + + webui_mod.main() + + _args, kwargs = launcher.call_args + assert kwargs.get("auth") == ("envuser", "envpass") + + +def test_main_no_auth_by_default(webui, monkeypatch): + """Without auth args or env vars, no auth must be configured.""" + webui_mod = webui + launcher = mock.MagicMock() + app_mock = mock.MagicMock() + app_mock.launch = launcher + monkeypatch.setattr(webui_mod, "build_ui", lambda: app_mock) + monkeypatch.setattr(sys, "argv", ["app.py"]) + monkeypatch.delenv("SKILLOPT_WEBUI_USER", raising=False) + monkeypatch.delenv("SKILLOPT_WEBUI_PASS", raising=False) + + webui_mod.main() + + _args, kwargs = launcher.call_args + assert "auth" not in kwargs or kwargs["auth"] is None + + +def test_scan_outputs_rejects_path_traversal(webui, tmp_path, monkeypatch): + """scan_outputs must not enumerate directories outside PROJECT_ROOT.""" + webui_mod = webui + monkeypatch.setattr(webui_mod, "PROJECT_ROOT", tmp_path) + (tmp_path / "outputs").mkdir() + + outside = tmp_path / "outputs" + result = webui_mod.build_ui.__wrapped__ if hasattr(webui_mod.build_ui, "__wrapped__") else None + + from pathlib import Path + base = (tmp_path / "outputs" / "../../etc").resolve() + project_resolved = tmp_path.resolve() + try: + base.relative_to(project_resolved) + escaped = False + except ValueError: + escaped = True + assert escaped, "Path traversal via Output Explorer must be blocked" + + +def test_scan_outputs_allows_valid_subdir(webui, tmp_path, monkeypatch): + """scan_outputs must accept directories within PROJECT_ROOT.""" + from pathlib import Path + project = tmp_path + monkeypatch.setattr(webui, "PROJECT_ROOT", project) + (project / "outputs" / "bench1" / "run1").mkdir(parents=True) + + base = (project / "outputs").resolve() + project_resolved = project.resolve() + try: + base.relative_to(project_resolved) + contained = True + except ValueError: + contained = False + assert contained, "Valid subdirectory must pass containment check" From d417c4cb7649319438e438263ec470e7b7d2c3a6 Mon Sep 17 00:00:00 2001 From: WODE25500 Date: Sun, 6 Sep 2026 01:05:14 +0800 Subject: [PATCH 2/3] fix(webui): fail closed on incomplete auth credentials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- skillopt_webui/app.py | 15 ++++++++-- tests/test_webui_security.py | 56 ++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 3 deletions(-) diff --git a/skillopt_webui/app.py b/skillopt_webui/app.py index 93490baa..f6416203 100644 --- a/skillopt_webui/app.py +++ b/skillopt_webui/app.py @@ -700,9 +700,18 @@ def main(): auth_user = args.auth_user or os.environ.get("SKILLOPT_WEBUI_USER") auth_pass = args.auth_pass or os.environ.get("SKILLOPT_WEBUI_PASS") - auth = None - if auth_user and auth_pass: - auth = (auth_user, auth_pass) + # Fail-closed: authentication requires BOTH credentials. Supplying only a + # username or only a password must not silently launch the UI unauthenticated + # (a deployment could expose the training controls without login). + if bool(auth_user) != bool(auth_pass): + print( + "SKILLOPT_WEBUI authentication requires BOTH --auth-user and " + "--auth-pass (or SKILLOPT_WEBUI_USER and SKILLOPT_WEBUI_PASS). " + "Refusing to start with incomplete credentials.", + file=sys.stderr, + ) + sys.exit(1) + auth = (auth_user, auth_pass) if auth_user else None app = build_ui() launch_kwargs = build_launch_kwargs(args.host, args.port, args.share) diff --git a/tests/test_webui_security.py b/tests/test_webui_security.py index 15f1139b..64eb1549 100644 --- a/tests/test_webui_security.py +++ b/tests/test_webui_security.py @@ -157,3 +157,59 @@ def test_scan_outputs_allows_valid_subdir(webui, tmp_path, monkeypatch): except ValueError: contained = False assert contained, "Valid subdirectory must pass containment check" + + +def test_main_rejects_incomplete_cli_auth_user_only(webui, monkeypatch): + """--auth-user without --auth-pass must fail closed (never launch).""" + webui_mod = webui + launcher = mock.MagicMock() + app_mock = mock.MagicMock() + app_mock.launch = launcher + monkeypatch.setattr(webui_mod, "build_ui", lambda: app_mock) + monkeypatch.setattr(sys, "argv", ["app.py", "--host", "0.0.0.0", "--auth-user", "admin"]) + with pytest.raises(SystemExit): + webui_mod.main() + launcher.assert_not_called() + + +def test_main_rejects_incomplete_cli_auth_pass_only(webui, monkeypatch): + """--auth-pass without --auth-user must fail closed (never launch).""" + webui_mod = webui + launcher = mock.MagicMock() + app_mock = mock.MagicMock() + app_mock.launch = launcher + monkeypatch.setattr(webui_mod, "build_ui", lambda: app_mock) + monkeypatch.setattr(sys, "argv", ["app.py", "--host", "0.0.0.0", "--auth-pass", "s3cret"]) + with pytest.raises(SystemExit): + webui_mod.main() + launcher.assert_not_called() + + +def test_main_rejects_incomplete_env_auth_user_only(webui, monkeypatch): + """Only SKILLOPT_WEBUI_USER set must fail closed (never launch).""" + webui_mod = webui + launcher = mock.MagicMock() + app_mock = mock.MagicMock() + app_mock.launch = launcher + monkeypatch.setattr(webui_mod, "build_ui", lambda: app_mock) + monkeypatch.setattr(sys, "argv", ["app.py", "--host", "0.0.0.0"]) + monkeypatch.setenv("SKILLOPT_WEBUI_USER", "envuser") + monkeypatch.delenv("SKILLOPT_WEBUI_PASS", raising=False) + with pytest.raises(SystemExit): + webui_mod.main() + launcher.assert_not_called() + + +def test_main_rejects_incomplete_env_auth_pass_only(webui, monkeypatch): + """Only SKILLOPT_WEBUI_PASS set must fail closed (never launch).""" + webui_mod = webui + launcher = mock.MagicMock() + app_mock = mock.MagicMock() + app_mock.launch = launcher + monkeypatch.setattr(webui_mod, "build_ui", lambda: app_mock) + monkeypatch.setattr(sys, "argv", ["app.py", "--host", "0.0.0.0"]) + monkeypatch.setenv("SKILLOPT_WEBUI_PASS", "envpass") + monkeypatch.delenv("SKILLOPT_WEBUI_USER", raising=False) + with pytest.raises(SystemExit): + webui_mod.main() + launcher.assert_not_called() From 32a29ad81dc284cfa480d62ecf928ee9bfb457c5 Mon Sep 17 00:00:00 2001 From: WODE25500 Date: Sun, 6 Sep 2026 05:45:25 +0800 Subject: [PATCH 3/3] test(webui): exercise scan_outputs callback at the data-consumption point 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. --- skillopt_webui/app.py | 97 +++++++++++++++++++----------------- tests/test_webui_security.py | 53 +++++++++----------- 2 files changed, 76 insertions(+), 74 deletions(-) diff --git a/skillopt_webui/app.py b/skillopt_webui/app.py index f6416203..eccc93c6 100644 --- a/skillopt_webui/app.py +++ b/skillopt_webui/app.py @@ -46,6 +46,58 @@ def load_config(path: str) -> dict: return yaml.safe_load(f) +def scan_outputs(out_dir: str) -> list: + """Digest experiment results strictly under PROJECT_ROOT. + + The Output Explorer callback. Any path that escapes PROJECT_ROOT is + rejected (empty result) at the point data is read, so a traversal arg + can never read files outside the project. + """ + rows = [] + if not out_dir: + return rows + base = (PROJECT_ROOT / out_dir).resolve() + project_resolved = PROJECT_ROOT.resolve() + try: + base.relative_to(project_resolved) + except ValueError: + return rows + if not base.exists() or not base.is_dir(): + return rows + for bench_dir in sorted(base.iterdir()): + if not bench_dir.is_dir(): + continue + for run_dir in sorted(bench_dir.iterdir()): + if not run_dir.is_dir(): + continue + cfg_file = run_dir / "config.yaml" + score = "—" + steps = "—" + if cfg_file.exists(): + try: + c = yaml.safe_load(cfg_file.read_text()) + steps = str(c.get("train", {}).get("num_steps", "—")) + except Exception: + pass + # Try to find best score from logs + for log_f in run_dir.glob("**/*.jsonl"): + try: + with open(log_f) as f: + for line in f: + d = json.loads(line) + if "score" in d: + score = f"{d['score']:.4f}" + except Exception: + pass + rows.append([ + run_dir.name, + bench_dir.name, + score, + steps, + ]) + return rows + + def config_to_display(cfg: dict) -> str: """Pretty-print config for display.""" return yaml.dump(cfg, default_flow_style=False, sort_keys=False) @@ -602,51 +654,6 @@ def on_refresh(): label="Experiments", ) - def scan_outputs(out_dir): - rows = [] - if not out_dir: - return rows - base = (PROJECT_ROOT / out_dir).resolve() - project_resolved = PROJECT_ROOT.resolve() - try: - base.relative_to(project_resolved) - except ValueError: - return rows - if not base.exists() or not base.is_dir(): - return rows - for bench_dir in sorted(base.iterdir()): - if not bench_dir.is_dir(): - continue - for run_dir in sorted(bench_dir.iterdir()): - if not run_dir.is_dir(): - continue - cfg_file = run_dir / "config.yaml" - score = "—" - steps = "—" - if cfg_file.exists(): - try: - c = yaml.safe_load(cfg_file.read_text()) - steps = str(c.get("train", {}).get("num_steps", "—")) - except Exception: - pass - # Try to find best score from logs - for log_f in run_dir.glob("**/*.jsonl"): - try: - with open(log_f) as f: - for line in f: - d = json.loads(line) - if "score" in d: - score = f"{d['score']:.4f}" - except Exception: - pass - rows.append([ - run_dir.name, - bench_dir.name, - score, - steps, - ]) - return rows - scan_btn.click(scan_outputs, output_dir, results_table) return app diff --git a/tests/test_webui_security.py b/tests/test_webui_security.py index 64eb1549..a5a28fac 100644 --- a/tests/test_webui_security.py +++ b/tests/test_webui_security.py @@ -123,40 +123,35 @@ def test_main_no_auth_by_default(webui, monkeypatch): def test_scan_outputs_rejects_path_traversal(webui, tmp_path, monkeypatch): - """scan_outputs must not enumerate directories outside PROJECT_ROOT.""" - webui_mod = webui - monkeypatch.setattr(webui_mod, "PROJECT_ROOT", tmp_path) + """The scan_outputs callback must not enumerate directories outside PROJECT_ROOT.""" + monkeypatch.setattr(webui, "PROJECT_ROOT", tmp_path) (tmp_path / "outputs").mkdir() - - outside = tmp_path / "outputs" - result = webui_mod.build_ui.__wrapped__ if hasattr(webui_mod.build_ui, "__wrapped__") else None - - from pathlib import Path - base = (tmp_path / "outputs" / "../../etc").resolve() - project_resolved = tmp_path.resolve() - try: - base.relative_to(project_resolved) - escaped = False - except ValueError: - escaped = True - assert escaped, "Path traversal via Output Explorer must be blocked" + # Every traversal / escape form is denied at consumption: no rows, no reads. + for bad in ("/../../etc/passwd", "../outside", "outputs/../../../etc", "C:\\Windows"): + assert webui.scan_outputs(bad) == [], f"traversal {bad!r} must be denied" def test_scan_outputs_allows_valid_subdir(webui, tmp_path, monkeypatch): """scan_outputs must accept directories within PROJECT_ROOT.""" - from pathlib import Path - project = tmp_path - monkeypatch.setattr(webui, "PROJECT_ROOT", project) - (project / "outputs" / "bench1" / "run1").mkdir(parents=True) - - base = (project / "outputs").resolve() - project_resolved = project.resolve() - try: - base.relative_to(project_resolved) - contained = True - except ValueError: - contained = False - assert contained, "Valid subdirectory must pass containment check" + monkeypatch.setattr(webui, "PROJECT_ROOT", tmp_path) + (tmp_path / "outputs" / "bench1" / "run1").mkdir(parents=True) + (tmp_path / "outputs" / "bench1" / "run1" / "config.yaml").write_text("a: 1\n", encoding="utf-8") + rows = webui.scan_outputs("outputs") + assert rows, "valid in-tree output area must be digested" + + +def test_scan_outputs_callback_consumes_within_project(webui, tmp_path, monkeypatch): + """The registered scan_outputs callback must digest data only inside PROJECT_ROOT + at the point data is actually read (traversal denied, in-tree consumed).""" + monkeypatch.setattr(webui, "PROJECT_ROOT", tmp_path) + # A traversal arg must be denied at consumption: no rows, no data read. + assert webui.scan_outputs("/../../etc/passwd") == [] + assert webui.scan_outputs("../outside") == [] + # A valid in-tree output area is digested (config.yaml read per run dir). + (tmp_path / "outputs/bench1/run1").mkdir(parents=True) + (tmp_path / "outputs/bench1/run1/config.yaml").write_text("alpha: 1\n", encoding="utf-8") + rows = webui.scan_outputs("outputs") + assert rows, f"expected rows from a valid in-tree output area, got {rows!r}" def test_main_rejects_incomplete_cli_auth_user_only(webui, monkeypatch):