Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| return jsonify({"path": path, "parent": parent, "entries": entries}) | ||
| except PermissionError as exc: | ||
| return jsonify({"error": str(exc)}), 403 |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 13 days ago
To fix the issue, the exception message should no longer be returned directly to the client. Instead, log the detailed error (including exc) on the server, and return a generic, non-sensitive error string in the JSON response, such as "Permission denied" or "Access to this path is not allowed". This preserves debuggability for developers while preventing exposure of any filesystem paths or other internal details contained in str(exc).
Concretely, in itkit/web/app.py, within the /api/browse route, update the except PermissionError as exc: block at lines 123–124. Replace {"error": str(exc)} with a generic message and add a call to a logging facility using the existing app.logger from Flask (no new imports required). For example, log the exception with app.logger.warning(...) or app.logger.error(...) including the candidate path, then return jsonify({"error": "Permission denied accessing requested path"}) with the same 403 status code, preserving the external behavior (status and JSON structure) without leaking exception details.
| @@ -121,7 +121,9 @@ | ||
|
|
||
| return jsonify({"path": current_path, "parent": parent, "entries": entries}) | ||
| except PermissionError as exc: | ||
| return jsonify({"error": str(exc)}), 403 | ||
| # Log the detailed permission error on the server but return a generic message to the client. | ||
| app.logger.warning("Permission error while browsing '%s': %s", candidate, exc) | ||
| return jsonify({"error": "Permission denied accessing requested path"}), 403 | ||
|
|
||
|
|
||
| @app.route("/api/run/<tool>", methods=["POST"]) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #82 +/- ##
=======================================
Coverage ? 72.19%
=======================================
Files ? 108
Lines ? 12244
Branches ? 1099
=======================================
Hits ? 8839
Misses ? 3147
Partials ? 258 ☔ View full report in Codecov by Sentry. |
…n path expression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…n path expression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new browser-based web UI for running ITKIT preprocessing tools, including packaging entry points and documentation updates so the web interface can be installed and launched as itkit-web.
Changes:
- Introduces a Flask backend (
itkit.web.app) with REST endpoints for browsing directories, launching whitelisted CLI tools, streaming logs/progress via SSE, and terminating jobs. - Adds a single-page HTML frontend template implementing tool parameter forms, log/progress display, and a directory picker.
- Updates packaging (
itkit-webscript +webextra) and MkDocs documentation/navigation for the new web interface.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Adds web optional dependencies and a new itkit-web console entry point. |
| mkdocs.yml | Adds the Web Interface page to the documentation nav. |
| itkit/web/templates/index.html | New SPA-style frontend for running tools, browsing paths, and viewing streaming output. |
| itkit/web/app.py | New Flask backend implementing browse/run/stream/kill endpoints and CLI entry point. |
| itkit/web/init.py | Introduces the itkit.web package. |
| docs/web_interface.md | New documentation describing installation, usage, API, and security notes for the web interface. |
| docs/installation.md | Documents the new web optional dependency group. |
| docs/index.md | Adds a homepage link/summary for the Web Interface docs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| # ── Routes ──────────────────────────────────────────────────────────────────── |
There was a problem hiding this comment.
render_template("index.html") depends on itkit/web/templates/index.html being included in the installed wheel/sdist. The current pyproject.toml doesn’t define any package-data/include-package-data, so the template is likely to be missing at runtime and this route will 500 after pip install. Please add setuptools package-data configuration (or a MANIFEST.in + include-package-data) to ship the templates directory.
| else: | ||
| parent = os.path.relpath(str(parent_path), _BROWSE_ROOT) | ||
|
|
||
| # Expose the current directory path relative to the browse root. | ||
| current_path = os.path.relpath(candidate, _BROWSE_ROOT) |
There was a problem hiding this comment.
request.get_json(force=True) will raise a BadRequest exception (returning an HTML 400) if the client sends invalid JSON. Since this is a public API endpoint, consider using silent=True (and returning your JSON error payload) or catching the exception to keep error responses consistently JSON.
| def run_tool(tool: str): | ||
| """Launch an itkit CLI tool as a background job. | ||
|
|
||
| Request body (JSON) | ||
| ------------------- | ||
| args : list[str] CLI arguments to append after the tool name. | ||
|
|
||
| Returns | ||
| ------- | ||
| {"job_id": "<uuid>"} | ||
| """ | ||
| if tool not in _ALLOWED_TOOLS: | ||
| return jsonify({"error": f"Tool '{tool}' is not in the allow-list"}), 400 |
There was a problem hiding this comment.
The server currently reads subprocess output line-by-line (for line in proc.stdout). Many of ITKIT’s tools use tqdm-style progress bars that update with carriage returns and may not emit newlines frequently, so output/progress may appear “stuck” until the process finishes. To match the existing PyQt GUI behavior (itkit/gui/runner.py reads raw chunks), consider reading stdout in chunks (or handling \r) so incremental tqdm updates are streamed promptly.
|
|
||
| Events | ||
| ------ | ||
| data Standard log line (stdout/stderr) | ||
| progress Integer percentage parsed from tqdm-style output | ||
| heartbeat Keep-alive (sent every 30 s of silence) | ||
| done Emitted once when the process finishes | ||
| """ |
There was a problem hiding this comment.
SSE data: fields must not contain embedded newlines; this code only strips \n and will forward any \r (common in progress bars) directly into the event stream. Consider normalizing \r and/or splitting into multiple data: lines to avoid malformed SSE frames and client-side parsing issues.
| q.put(line) | ||
| proc.wait() | ||
| q.put(f"\n[Process exited with code {proc.returncode}]\n") | ||
| except FileNotFoundError: | ||
| q.put(f"[Error] Command not found: '{tool}'\n") | ||
| except Exception as exc: # pylint: disable=broad-except | ||
| q.put(f"[Error] {exc}\n") | ||
| finally: | ||
| q.put(None) # sentinel → signals the SSE generator to close | ||
|
|
||
| with _jobs_lock: | ||
| _jobs[job_id] = {"proc": None, "queue": q} | ||
| threading.Thread(target=_run, daemon=True).start() | ||
| return jsonify({"job_id": job_id}) | ||
|
|
||
|
|
||
| @app.route("/api/stream/<job_id>") | ||
| def stream_job(job_id: str): | ||
| """Server-Sent Events stream for a running job. |
There was a problem hiding this comment.
Jobs are never removed from the in-memory _jobs registry after completion/streaming. Over time (or under repeated use) this will leak memory/queues and keep job metadata around indefinitely. Please delete the job entry (under lock) when the process finishes and/or when the done event is emitted, and consider cleaning up if the client never connects to /api/stream/<job_id>.
| <header id="header"> | ||
| <button class="btn-icon" id="sidebar-toggle" title="Toggle file browser">☰</button> | ||
| <span class="logo">ITKIT</span> | ||
| <span class="tagline">Medical Image Preprocessing — Web Interface</span> | ||
| </header> | ||
|
|
||
| <div id="body-row"> | ||
|
|
||
| <!-- ── Sidebar: File Browser ───────────────────────── --> | ||
| <aside id="sidebar"> | ||
| <div id="sidebar-header"> | ||
| <span class="sb-title">File Browser</span> | ||
| <button class="btn-icon" id="btn-home" title="Go to home directory">⌂</button> | ||
| <button class="btn-icon" id="btn-refresh" title="Refresh">↺</button> | ||
| </div> |
There was a problem hiding this comment.
Several controls are icon-only (e.g., sidebar toggle, home, refresh, clear) and rely on title + glyph characters. For accessibility, please add an aria-label (or visually-hidden text) so screen readers announce meaningful button names consistently.
| - Click any directory to expand it. | ||
| - Click a file or folder to copy its absolute path to the currently focused path field in the parameters panel. | ||
| - The current directory path is shown at the top of the sidebar and can be edited directly. |
There was a problem hiding this comment.
The documentation describes sidebar behaviors that aren’t implemented in the current frontend: (1) clicking a file/folder copies its path into the currently focused parameter field, and (2) the current directory path at the top of the sidebar can be edited directly. The current index.html only updates a non-editable path label and uses a separate modal picker for form fields. Please update the docs or implement the described behavior to avoid confusing users.
| - Click any directory to expand it. | |
| - Click a file or folder to copy its absolute path to the currently focused path field in the parameters panel. | |
| - The current directory path is shown at the top of the sidebar and can be edited directly. | |
| - Click any directory to expand it and update the current path label. | |
| - Click a file or folder to select it in the sidebar; use the parameter panel’s “Browse…” buttons to choose paths for specific fields via a modal dialog. | |
| - The current directory path is shown at the top of the sidebar as a read-only label. |
| | Convert | `itk_convert` | | ||
| | Evaluate | `itk_evaluate` | | ||
| | Combine | `itk_combine` | | ||
|
|
||
| ### Log Panel | ||
|
|
||
| - Displays real-time stdout / stderr from the running tool. | ||
| - Progress bars (tqdm-style percentages) are parsed and shown in a progress indicator. | ||
| - A **Kill** button terminates the running job immediately. |
There was a problem hiding this comment.
Docs claim the web UI exposes tabs for 9 tools and that the Web Interface supports “All 9 processing tools”, but the current frontend only has tabs/forms for 6 tools (check/resample/orient/patch/aug/extract). Either add tabs for convert/evaluate/combine or adjust the docs/comparison table to reflect the actual UI capabilities. Also, the Log Panel section mentions a Kill button, but the UI label is Stop.
| | Convert | `itk_convert` | | |
| | Evaluate | `itk_evaluate` | | |
| | Combine | `itk_combine` | | |
| ### Log Panel | |
| - Displays real-time stdout / stderr from the running tool. | |
| - Progress bars (tqdm-style percentages) are parsed and shown in a progress indicator. | |
| - A **Kill** button terminates the running job immediately. | |
| ### Log Panel | |
| - Displays real-time stdout / stderr from the running tool. | |
| - Progress bars (tqdm-style percentages) are parsed and shown in a progress indicator. | |
| - A **Stop** button terminates the running job immediately. |
| @app.route("/") | ||
| def index() -> str: | ||
| return render_template("index.html") | ||
|
|
||
|
|
||
| @app.route("/api/browse") | ||
| def browse(): | ||
| """Return the contents of a directory as JSON. | ||
|
|
||
| Query params | ||
| ------------ | ||
| path : str | ||
| Directory path to list. ``~`` is expanded to the user's home. | ||
| """ | ||
| raw = request.args.get("path", "~") | ||
| requested = os.path.normpath(os.path.expanduser(raw)) | ||
|
|
||
| # Resolve the requested path under the configured browse root and ensure | ||
| # that it does not escape this root directory. | ||
| candidate = os.path.realpath(os.path.join(_BROWSE_ROOT, requested.lstrip(os.sep))) | ||
| try: | ||
| common = os.path.commonpath([_BROWSE_ROOT, candidate]) | ||
| except ValueError: | ||
| return jsonify({"error": "Invalid path"}), 400 | ||
|
|
||
| if common != _BROWSE_ROOT: | ||
| return jsonify({"error": "Path not allowed"}), 400 | ||
|
|
||
| if not os.path.isdir(candidate): | ||
| return jsonify({"error": f"Not a directory: {requested}"}), 400 | ||
|
|
||
| try: | ||
| def _sort_key(name: str) -> tuple: | ||
| full = os.path.join(candidate, name) | ||
| try: | ||
| return (not os.path.isdir(full), name.lower()) | ||
| except OSError: | ||
| return (1, name.lower()) | ||
|
|
||
| entries = [] | ||
| for name in sorted(os.listdir(candidate), key=_sort_key): | ||
| full = os.path.join(candidate, name) | ||
| try: | ||
| is_dir = os.path.isdir(full) | ||
| except OSError: | ||
| is_dir = False | ||
| # Expose a path relative to the browse root instead of the absolute filesystem path. | ||
| rel_path = os.path.relpath(full, _BROWSE_ROOT) | ||
| entries.append({"name": name, "path": rel_path, "is_dir": is_dir}) | ||
|
|
||
| # Compute the parent directory relative to the browse root. Do not expose paths | ||
| # above the browse root; represent the root itself with None. | ||
| parent_path = Path(candidate).parent | ||
| if str(parent_path) == _BROWSE_ROOT or str(parent_path) == candidate: | ||
| parent = None |
There was a problem hiding this comment.
New Flask API behavior (browse restrictions, tool allow-list enforcement, job lifecycle, and SSE stream formatting/progress parsing) isn’t covered by tests. The repo already has extensive pytest coverage for process tooling; adding a small test module for itkit.web.app (using Flask’s test client) would help prevent regressions and catch issues like job cleanup and JSON error handling.
| ### `POST /api/run/<tool>` | ||
|
|
||
| Start a tool as a background job. | ||
|
|
||
| **Request body** | ||
|
|
||
| ```json | ||
| { "args": ["<mode>", "--input", "/data/dataset", "--output", "/data/out"] } | ||
| ``` | ||
|
|
||
| **Response** | ||
|
|
||
| ```json | ||
| { "job_id": "3f2f1a..." } | ||
| ``` | ||
|
|
||
| Only tools in the allow-list (`itk_check`, `itk_resample`, `itk_orient`, `itk_patch`, `itk_aug`, `itk_extract`, `itk_convert`, `itk_evaluate`, `itk_combine`) are accepted. | ||
|
|
There was a problem hiding this comment.
The /api/run/<tool> API, as described here, is currently implemented without CSRF/origin protections and parses JSON with request.get_json(force=True), which allows any website running in the user’s browser to trigger tool executions on the local machine via a cross-origin POST with a text/plain JSON body. This lets a malicious page start allowed CLI tools (itk_check, itk_resample, etc.) whenever itkit-web is running on 127.0.0.1 or exposed on the network, even though the attacker cannot read the responses. The backend should require proper JSON Content-Type (avoid force=True), enforce CSRF/origin checks or an API token for state-changing endpoints, and the documentation should be updated to call out these requirements so the API is not used unauthenticated from the browser.
…in path expression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
This PR introduces an independent entrypoint of
itkit-- WEB.This is an experimental feature and may will be removed in the future.
📚 Documentation preview 📚: https://ITKIT--82.org.readthedocs.build/en/82/