Repository navigation
fix(webui): security hardening + launch theme crash; add CI - #249
Conversation
- Default bind 127.0.0.1 (was 0.0.0.0); warn to stderr on a non-localhost bind. - Move gradio theme onto gr.Blocks (fixes a launch crash: launch() has no theme param). - Guard empty out_dir in scan_outputs. - Add .github/workflows/ci.yml (test py 3.10/3.11/3.12 + docs mkdocs --strict).
|
Thanks — the localhost default, empty-output guard, and CI setup are useful. One supported-version compatibility issue blocks this as written.
Please either make theme placement runtime-compatible with each supported Gradio API ( |
Address maintainer review on microsoft#249: - Place the gradio theme on Blocks for Gradio <=5 and on launch() for Gradio 6, detected via the installed major, so the WebUI works on any supported version without an ignored-argument warning or a TypeError. - Add a real Gradio build/launch smoke test (skips without the webui extra) asserting the theme is actually applied and no error is raised.
Verified against real Gradio 4.44, 5.50, and 6.25 (built sequentially to avoid conflicting pins): - build_ui() succeeds on all three; theme is placed on Blocks for <6 and on launch() for >=6 (no TypeError / ignored-arg warning). - The launch smoke skips cleanly when a headless/sandboxed environment blocks localhost (not a compatibility bug), and asserts theme application when it launches.
|
Yifan Yang (@Yif-Yang) — thank you for the thorough review and guidance! I've addressed the feedback:
Thanks again for the detailed review! |
|
Thanks — the version-dependent implementation works in local checks, but the requested CI coverage is still not actually exercising that compatibility path. The workflow installs only Please add a reproducible Gradio 4/5/6 WebUI matrix that installs the relevant extra, exercises the production launch-kwargs path, and asserts that |
- Narrow webui extra to gradio>=5,<7 (gradio 4.44 needs huggingface_hub<1, but 6 needs >=1.16; one bound can't satisfy both, so drop the untested major 4 and keep the two the CI matrix validates). - Extract build_launch_kwargs() so tests exercise the production main() path instead of injecting their own theme. - Assert the Soft theme specifically (isinstance/name) rather than is not None (gradio 6 assigns a default theme when none is passed). - Add a CI webui job that installs gradio across majors (5/6) and runs the gradio tests, which the main job's [dev] install silently skipped.
|
Already addressed the review feedback and updated this branch (#249):
|
|
done, please re-review. One note on the dependency choice: I took the "narrow the range" option - webui is now gradio>=5,<7 - because gradio 4/5 need huggingface_hub<1 while gradio 6 needs >=1.16; a single bound cannot satisfy both, so I dropped the untested major 4. |
|
Thanks — the production theme path and Gradio 5/6 CI coverage now address the previous review. One supported-version issue remains: |
- The declared floor gradio>=5.0.0 advertised gradio 5.0-5.49, which either still import HfFolder (removed in modern huggingface_hub) or don't apply the Blocks theme the same way, so a clean install can fail at import. - Raise the webui extra to gradio>=5.50.0,<7 (verified: Blocks theme is Soft, the webui tests pass) and test that actual floor in the CI matrix (5.50 + 6.26).
|
Thanks for the re-review. Addressed the remaining item: the declared floor is now gradio>=5.50.0,<7 (earlier 5.0-5.49 either import HfFolder or don't apply the Blocks theme the same way), and the CI webui matrix tests that actual floor (5.50 + 6.26). Commit d549c86. |
…njection (#264) * fix: harden security beyond PR #249 — command injection, deps, 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. * fix(webui): fail closed on incomplete auth credentials 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. * 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. * refactor: split dependency changes out of the security PR 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). * fix(webui): contain config and launch paths under PROJECT_ROOT 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. * fix(webui): fail closed on blank or ambiguous auth configuration `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. * test(webui): assert invalid auth stops before the UI is even built 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. * test(webui): make the auth decision table exhaustive 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. * fix(spreadsheetbench): 校验任务标识符并约束输出目录 此前只清洗了 mkdtemp 前缀，带路径分隔符的 id 因此不再在那里报错，而是 继续走到用原始 id 拼出的持久输出目录，进而触及提示词写入、agent 调用与 代码执行路径。现在在两个生产入口最先校验 id（限定单一路径段字符集，拒绝 .. 与分隔符），再对 predictions 下的目标做 realpath 包含检查；正常 id 的 流程不变，mkdtemp 前缀不再单独清洗。 新增生产入口回归：恶意 id 在读取数据根目录之前即被拒绝且不触发 agent， 符号链接目标被拒，正常 id 仍进入 LLM / agent 阶段。 --------- Co-authored-by: WODE25500 <WODE25500@users.noreply.github.com> Co-authored-by: WODE25500 <318555974+WODE25500@users.noreply.github.com>
2 participants
Security/robustness hardening for the Gradio WebUI, plus a first CI workflow.
Note: the pre-existing WebUI scan_outputs path traversal (arbitrary typed path) is intentionally left as-is; the localhost default is the mitigation.