atiaomar1978-hub commented on PR #25317: URL: https://github.com/apache/camel/pull/25317#issuecomment-5181960391
## Full review — BugBot + Grok (CAMEL-24202: `camel tui --web`) Reviewed https://github.com/apache/camel/pull/25317 on branch `CAMEL-24202`. _Cursor (BugBot + Grok) on behalf of atiaomar1978-hub_ --- ### Overall assessment This PR adds a useful, opt-in `--web` mode for the Camel TUI: loopback-only WebSocket terminal via Aesh/TamboUI, vendored xterm.js UI, per-browser-session `CamelMonitor` instances, and good documentation updates in both the user manual and security model. **Recommendation:** Approve after addressing the **medium** items below (especially session shutdown and clickjaming). The design direction is sound and aligns with how `--mcp` is already framed in the security model. --- ### What looks good 1. **Loopback bind** — `TuiWebServer` binds explicitly to `127.0.0.1` (not `0.0.0.0`). 2. **Security-model documentation** — Clear framing of `--web` alongside `--mcp` as opt-in management surfaces with no auth beyond loopback. 3. **Origin rejection** — Foreign origins get `403` on WebSocket upgrade (tested in `TuiWebServerTest`). 4. **JLine backend fix** — `TuiBackendHelper` explicitly creates JLine for local sessions, avoiding Aesh auto-discovery when `tamboui-aesh-backend` is on the classpath. 5. **Signal handler scoping** — `Signal.handle(INT)` is registered only for the local terminal session, not browser sessions. 6. **Browser session isolation** — Each WebSocket connection gets its own `CamelMonitor` / `TuiRunner` in `TuiWebServer.accept()`. 7. **Static asset tests** — Index page, xterm.js/css, fit addon, and logo are covered. 8. **License handling** — Vendored xterm assets excluded from RAT with MIT license file. 9. **Port conflict UX** — Friendly `BindException` message with `--web-port` hint, mirroring MCP. --- ### Issues to address (medium) #### 1. Session shutdown race — `TuiWebServer.java:121-133` `stop()` closes Netty channels then immediately calls `sessionExecutor.shutdownNow()` without waiting for in-flight `accept()` tasks. When the local terminal exits while browser tabs are connected, `CamelMonitor.call()` `finally` blocks can race with event-loop teardown and be interrupted mid-cleanup. **Suggestion:** After closing channels, call `sessionExecutor.shutdown()` + `awaitTermination(timeout)` before `shutdownNow()` as a last resort. #### 2. Clickjacking on loopback — `TuiWebServer.java:173` + `index.html` The served page has no `X-Frame-Options: DENY` or `Content-Security-Policy: frame-ancestors 'none'`. A remote site can iframe `http://127.0.0.1:<port>/`; the browser then sends a loopback `Origin` on WebSocket upgrade (which passes `isAllowedOrigin`), enabling clickjacking of the full TUI on the loopback trust boundary. **Suggestion:** Add frame-denial headers via a small Netty handler or in the static response path. #### 3. Unbounded session admission — `TuiWebServer.java:76-81`, `142-161` `Executors.newFixedThreadPool` uses an **unbounded** task queue. Each WebSocket accept enqueues a full `CamelMonitor.call()`. A localhost client opening many connections can queue unbounded work and exhaust memory/CPU even though only `max(4, 2×cpus)` sessions run concurrently. **Suggestion:** Bounded queue + rejection policy, connection limit, or admission semaphore. #### 4. Origin check URI bypass — `TuiWebServer.java:183-196` Origin validation uses exact `"/ws".equalsIgnoreCase(request.uri())`. If the upgrade URI includes a query string (e.g. `/ws?token=…`), the check is skipped. **Suggestion:** Compare path only (`QueryStringDecoder` or strip query before compare). #### 5. Inconsistent quit behavior — `CamelMonitor.java:793-798` Comment says browser session "views the shared monitor" and must not quit the process others use — but `TuiWebServer.accept()` creates an **independent** `CamelMonitor` per connection. Swallowing `q`/Ctrl+C prevents the browser user from closing their own session, while AI `/quit` (`AiSlashCommandRegistry` → `requestExit()` → `tui::quit()`) still exits that session. **Suggestion:** Allow `q`/Ctrl+C to call `runner.quit()` for `webBackend != null` (only that session), or also block `/quit` in web sessions — and fix the misleading comment. #### 6. Test isolation — `TuiWebServerTest.java:98-107`, `133-147` Successful `101` handshake triggers `TtyWebSocketFrameHandler` → `accept()` → full `CamelMonitor.call()` on a background thread. Tests only assert the status line but may spawn heavyweight monitor sessions with no guaranteed teardown before `@AfterEach stop()`. **Suggestion:** Stub accept callback in unit tests, or abort connection immediately after handshake assertion. --- ### Low severity | Location | Finding | |----------|---------| | `TuiWebServer.java:143` | Bare `submit()` — `RejectedExecutionException` possible after `stop()`; close connection cleanly on reject | | `TuiWebServer.java:135-139` | `awaitTermination(timeout)` can block ~2× requested timeout (boss + worker each get full timeout) | | `index.html:247-255` | On disconnect, `ResizeObserver` never disconnected and `Terminal` never disposed — listener leak until reload | | `TuiWebServerTest.java` | Missing test for `http://localhost:<port>` origin (allowed by `isAllowedOrigin`) | | `camel-jbang-tui.adoc:779` | Docs say "same keyboard shortcuts" but web sessions hide quit hint and swallow `q`/Ctrl+C | --- ### Informational (by design / looks correct) | Location | Note | |----------|------| | `TuiWebServer.java:192-197` | Missing `Origin` allowed for non-browser clients; any local process can open WS — consistent with loopback trust model | | `CamelMonitor.java:578` | Process-wide signal handler correctly scoped to local session only | | `TuiBackendHelper.java:36` | Explicit JLine backend prevents Aesh auto-discovery shutdown issues | | `security-model.adoc:644` | Security framing for `--web` / `--mcp` is accurate and helpful | --- ### Test plan suggestions - [ ] Manual: `camel tui --web`, open `http://127.0.0.1:8090`, verify TUI renders and keyboard works - [ ] Manual: Open two browser tabs — confirm independent sessions (separate process discovery state) - [ ] Manual: Quit local terminal while browser tab open — verify browser session fails gracefully - [ ] Manual: Attempt iframe embed from external origin — verify frame-denial after fix - [ ] Automated: Add `localhost` origin handshake test - [ ] Automated: Avoid spawning full `CamelMonitor` in lightweight handshake tests --- ### Inline comments Detailed line-level comments are attached in the review thread: https://github.com/apache/camel/pull/25317#pullrequestreview-4856817799 Thanks for the contribution — `--web` is a nice complement to `--mcp` for local development workflows. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
