Skip to content

fix(webui): security hardening + launch theme crash; add CI - #249

Open
WODE25500 wants to merge 4 commits into
microsoft:mainfrom
WODE25500:fix/webui-security
Open

fix(webui): security hardening + launch theme crash; add CI#249
WODE25500 wants to merge 4 commits into
microsoft:mainfrom
WODE25500:fix/webui-security

Conversation

@WODE25500

Copy link
Copy Markdown
Contributor

Security/robustness hardening for the Gradio WebUI, plus a first CI workflow.

  • Default bind 127.0.0.1 (was 0.0.0.0); warn to stderr on a non-localhost bind.
  • Move the gradio theme onto gr.Blocks (fixes a startup crash: launch() has no theme param on Gradio 4/5).
  • Guard empty out_dir in scan_outputs.
  • Add .github/workflows/ci.yml (test py 3.10/3.11/3.12 + docs mkdocs --strict).

Note: the pre-existing WebUI scan_outputs path traversal (arbitrary typed path) is intentionally left as-is; the localhost default is the mitigation.

- 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).
@Yif-Yang

Copy link
Copy Markdown
Contributor

Thanks — the localhost default, empty-output guard, and CI setup are useful. One supported-version compatibility issue blocks this as written.

pyproject.toml declares gradio>=4.0.0 with no upper bound, but build_ui() now always passes theme to gr.Blocks. With the currently supported Gradio 6.25, Gradio warns that theme moved to launch(), ignores the constructor argument, and the resulting app has app.theme is None. The new CI installs only .[dev], not the WebUI extra, and the new tests replace Gradio with a fake module, so this regression is not exercised.

Please either make theme placement runtime-compatible with each supported Gradio API (Blocks(theme=...) for versions that support it and launch(theme=...) where required), or explicitly constrain the dependency range. Please also add a real WebUI build/launch smoke test for every supported Gradio range and ensure no ignored-argument warning or TypeError occurs and the selected theme is actually applied. Once that compatibility path is covered, this should be straightforward to re-review.

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.
@WODE25500

WODE25500 commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Yifan Yang (@Yif-Yang) — thank you for the thorough review and guidance! I've addressed the feedback:

  • The Gradio theme is now placed at runtime based on the installed version: on gr.Blocks for <=5, on launch() for >=6 (added _GRADIO_MAJOR detection), with no ignored-argument warning or TypeError.
  • Added a real Gradio build/launch smoke test (tests/test_webui_build_gradio.py, version-robust: it skips when a headless environment blocks localhost), verified against Gradio 4.44 / 5.50 / 6.25 — build_ui() succeeds on all three and the theme lands on the correct object (6.25 launch passes too).

Thanks again for the detailed review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants