CAMEL-24202: camel-tui accessible from a web browser via --web - #25317
CAMEL-24202: camel-tui accessible from a web browser via --web#25317ammachado wants to merge 7 commits into
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 3 tested, 1 compile-only — current: 2 all testedMaveniverse Scalpel detected 4 affected modules (current approach: 2).
|
There was a problem hiding this comment.
Pull request overview
Adds an opt-in --web mode to the Camel JBang TUI so the live monitoring UI can be driven from a web browser via a local (127.0.0.1) WebSocket terminal bridge, alongside the existing local terminal and --mcp server posture.
Changes:
- Introduces
--web/--web-portoptions and aTuiWebServerthat serves a browser terminal page and accepts WebSocket terminal sessions. - Ensures browser sessions cannot terminate the shared TUI process (e.g., disables quit hints / quit keys for web sessions).
- Vendors xterm.js assets and updates documentation (TUI docs + security model) and adds a server-focused test.
Reviewed changes
Copilot reviewed 12 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| parent/pom.xml | Adds aesh terminal-http version property for the new web terminal dependency. |
| dsl/camel-jbang/camel-jbang-plugin-tui/pom.xml | Adds tamboui-aesh-backend + org.aesh:terminal-http dependencies to enable web sessions. |
| dsl/camel-jbang/camel-jbang-plugin-tui/src/main/java/org/apache/camel/dsl/jbang/core/commands/tui/TuiCommand.java | Wires --web / --web-port through the CLI command arguments. |
| dsl/camel-jbang/camel-jbang-plugin-tui/src/main/java/org/apache/camel/dsl/jbang/core/commands/tui/TuiBackendHelper.java | Forces explicit JLine backend for local sessions; adds overload to build a runner from a provided backend. |
| dsl/camel-jbang/camel-jbang-plugin-tui/src/main/java/org/apache/camel/dsl/jbang/core/commands/tui/OverviewTab.java | Hides the “quit” footer hint for web sessions. |
| dsl/camel-jbang/camel-jbang-plugin-tui/src/main/java/org/apache/camel/dsl/jbang/core/commands/tui/MonitorContext.java | Adds webSession flag to adapt UI behavior based on session type. |
| dsl/camel-jbang/camel-jbang-plugin-tui/src/main/java/org/apache/camel/dsl/jbang/core/commands/tui/CamelMonitor.java | Implements web-server startup, session behavior differences, and header link/badge for local sessions. |
| dsl/camel-jbang/camel-jbang-plugin-tui/src/main/java/org/apache/camel/dsl/jbang/core/commands/tui/TuiWebServer.java | New web server using Aesh terminal-http WebSocket bootstrap; creates per-connection TUI sessions. |
| dsl/camel-jbang/camel-jbang-plugin-tui/src/test/java/org/apache/camel/dsl/jbang/core/commands/tui/TuiWebServerTest.java | New tests validating loopback bind, socket close on stop, WebSocket handshake, and static asset serving. |
| dsl/camel-jbang/camel-jbang-plugin-tui/src/main/resources/tui/web/index.html | New Camel-branded browser page wiring xterm.js to the /ws endpoint. |
| dsl/camel-jbang/camel-jbang-plugin-tui/src/main/resources/tui/web/vendor/xterm.css | Vendored xterm.js stylesheet. |
| dsl/camel-jbang/camel-jbang-plugin-tui/src/main/resources/tui/web/vendor/xterm-addon-fit.js | Vendored xterm fit addon bundle. |
| dsl/camel-jbang/camel-jbang-plugin-tui/src/main/resources/tui/web/vendor/LICENSE-xterm.txt | Adds licensing note for vendored xterm.js assets. |
| docs/user-manual/modules/ROOT/pages/security-model.adoc | Documents --mcp/--web as local, opt-in management surfaces (threat model framing). |
| docs/user-manual/modules/ROOT/pages/camel-jbang-tui.adoc | Documents --web usage and adds CLI option docs for --web/--web-port. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Nice feature — the WebSocket bridge to xterm.js is cleanly structured and follows the existing MCP server pattern well. Security posture is solid (loopback-only, origin checking, session isolation). Tests are comprehensive.
A few items worth considering:
License headers on vendored files — The vendored xterm.js, xterm.css, and xterm-addon-fit.js have Apache 2.0 license headers prepended, but these are MIT-licensed third-party files (© the xterm.js authors). The LICENSE-xterm.txt with MIT attribution is excellent, but the ASF headers on the files themselves misrepresent copyright. The standard ASF approach is to exclude vendored files from the RAT plugin (e.g., add **/tui/web/vendor/** exclusions in the root pom.xml RAT config) and keep only the original license notice.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of @gnodet
|
Fixed the vendored xterm.js license header issue in 8ee58d6: stripped the ASF headers from Claude Code on behalf of ammachado |
|
@atiaomar1978-hub, could you review this PR? |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Well-structured PR with comprehensive tests, proper security documentation, and all prior review feedback addressed.
Non-blocking suggestions
-
Direct Netty API usage via transitive dependency (Low) —
TuiWebServer.javadirectly imports ~18io.netty.*classes, but the pom.xml only declaresorg.aesh:terminal-http(Netty arrives transitively). Sinceterminal-httpexposes Netty types in its own API (TtyWebSocketFrameHandler,HttpRequestHandler), this is largely unavoidable and acceptable for now, but an explicit Netty dependency in the pom.xml would make the contract clearer and prevent breakage ifterminal-httpever adjusts its transitive scope. -
Origin null allowed in WebSocket check (Very low) —
isAllowedOrigin()returnstruewhen noOriginheader is present, which means non-browser tools (curl, etc.) can connect. This is fine given loopback-only binding and matches the documented "no authentication beyond loopback" posture, but worth noting as an intentional choice.
Positive observations
- Security model updated (
security-model.adocand operator hardening checklist) —--webframing matches the existing--mcpposture. - 7 tests in
TuiWebServerTestcovering TCP binding, shutdown, origin rejection/acceptance, port conflict, event loop termination, WebSocket handshake, and static asset serving. Uses@Isolated,AvailablePortFinder, AssertJ — all following project conventions. - All gnodet review comments (bounded thread pool,
awaitTerminationcoveringsessionExecutor, FQCN import) resolved. - Correctly forces JLine backend for local sessions to prevent ServiceLoader picking AeshBackend.
- Browser sessions correctly blocked from quitting the host TUI process.
- Vendored xterm.js properly excluded from RAT/license checks with LICENSE included.
This review checks the PR against project rules and conventions. It does not replace specialized tools (CodeRabbit, Sourcery, SonarCloud) or a deep static analysis pass.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
Addressed both non-blocking suggestions in b42b60b:
Claude Code on behalf of ammachado |
|
sorry you need to rebase as tui code changed |
atiaomar1978-hub
left a comment
atiaomar1978-hub
left a comment
There was a problem hiding this comment.
Code review — BugBot + Grok
Reviewed PR #25317 (CAMEL-24202: camel tui --web). Overall the loopback bind, security-model documentation, origin rejection for foreign hosts, and test coverage for static assets are solid.
Summary: 3 medium issues worth addressing before merge (session shutdown race, clickjaming on loopback, unbounded session admission). Several low-severity items on quit UX consistency, test isolation, and client cleanup.
Cursor (BugBot + Grok) on behalf of atiaomar1978-hub
| channels.close().syncUninterruptibly(); | ||
| bossGroup.shutdownGracefully(0, 5, TimeUnit.SECONDS).syncUninterruptibly(); | ||
| workerGroup.shutdownGracefully(0, 5, TimeUnit.SECONDS).syncUninterruptibly(); | ||
| sessionExecutor.shutdownNow(); |
There was a problem hiding this comment.
[Medium — BugBot] stop() calls sessionExecutor.shutdownNow() immediately after closing Netty channels, without waiting for in-flight accept() tasks to finish. 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: Close channels first, then shutdown() + awaitTermination on sessionExecutor (with a timeout) before shutting down Netty groups; only then shutdownNow() as a last resort.
|
|
||
| @Override | ||
| protected void channelRead0(ChannelHandlerContext context, FullHttpRequest request) { | ||
| if ("/ws".equalsIgnoreCase(request.uri()) && !isAllowedOrigin(request)) { |
There was a problem hiding this comment.
[Medium — Grok] Origin check uses exact "/ws".equalsIgnoreCase(request.uri()). If the upgrade URI includes a query string (e.g. /ws?token=…), this check is skipped and a foreign Origin may reach the WebSocket handler.
Suggestion: Parse the path only (QueryStringDecoder or strip ?…) before comparing, or use startsWith("/ws") with a boundary check.
| String origin = request.headers().get(HttpHeaderNames.ORIGIN); | ||
| // A missing Origin header is allowed on purpose: non-browser clients (curl, custom | ||
| // terminal clients) don't send one, and the loopback-only bind is the actual boundary here. | ||
| return origin == null || origin.equals("http://127.0.0.1:" + port) || origin.equals("http://localhost:" + port); |
There was a problem hiding this comment.
[Info] Missing Origin is intentionally allowed for non-browser clients (curl, tests). This is consistent with the security-model doc (loopback bind is the trust boundary), but worth noting: any local process can open a WebSocket session without origin validation.
| private final ChannelGroup channels = new DefaultChannelGroup(ImmediateEventExecutor.INSTANCE); | ||
| private final EventLoopGroup bossGroup = new MultiThreadIoEventLoopGroup(1, NioIoHandler.newFactory()); | ||
| private final EventLoopGroup workerGroup = new MultiThreadIoEventLoopGroup(NioIoHandler.newFactory()); | ||
| private final ExecutorService sessionExecutor = Executors.newFixedThreadPool( |
There was a problem hiding this comment.
[Medium — Grok] Fixed-size pool but 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) run concurrently.
Suggestion: Consider a bounded queue with a rejection policy, connection limit, or semaphore on admissions.
| } | ||
|
|
||
| private void accept(Connection connection) { | ||
| sessionExecutor.submit(() -> { |
There was a problem hiding this comment.
[Low — Grok] Bare sessionExecutor.submit(...) with no RejectedExecutionException handling. After stop() calls shutdownNow(), a late WebSocket accept on a Netty thread can throw on the event loop.
Suggestion: Catch RejectedExecutionException in accept() and close the connection cleanly.
| } | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
[Low — coverage gap] Tests cover 127.0.0.1 origin and foreign origin, but not http://localhost:<port> (also allowed by isAllowedOrigin). Worth adding for symmetry.
| } | ||
|
|
||
| window.addEventListener('resize', handleResize); | ||
| if (window.ResizeObserver) { |
There was a problem hiding this comment.
[Low — Grok] On disconnect, window.removeEventListener('resize', …) runs but the ResizeObserver is never disconnected and the Terminal instance is never disposed. Reloading is required to GC; long-lived tabs that disconnect/reconnect would leak listeners.
Suggestion: Store observer reference and call disconnect() in onclose; call term.dispose() if available.
|
|
||
| Like the MCP server, the web server is bound to `127.0.0.1` only -- it never listens on | ||
| external interfaces -- and there is no authentication beyond that. Each browser connection | ||
| gets its own independent TUI session (its own process discovery and navigation state), the |
There was a problem hiding this comment.
[Low — docs] States browser sessions have "the same … keyboard shortcuts" as local terminal, but web sessions hide the q/quit footer hint (OverviewTab) and swallow q/Ctrl+C (CamelMonitor L795–798). Consider noting that quit behaves differently in browser sessions.
| // Build the JLine backend explicitly rather than leaving backend selection to | ||
| // TamboUI's ServiceLoader-based auto-discovery: with tamboui-aesh-backend also on the | ||
| // classpath (for --web), auto-discovery can pick AeshBackend for the local session too, | ||
| // which drives a native PosixSysTerminal that doesn't shut down cleanly here. |
There was a problem hiding this comment.
[Info — looks good] Explicit JLine backend avoids ServiceLoader picking AeshBackend for the local session when tamboui-aesh-backend is on the classpath. Good fix for clean shutdown.
| "MBean operation X executes code or sends to endpoint Y when invoked | ||
| from a JMX or Jolokia connection" describes the documented contract, | ||
| not a framework vulnerability. | ||
| * *The Camel TUI's `--mcp` and `--web` servers.* Camel TUI |
There was a problem hiding this comment.
[Info — looks good] Clear framing of --web alongside --mcp as opt-in loopback management surfaces. Matches Camel's documented trust model.
Full review — BugBot + Grok (CAMEL-24202:
|
| 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, openhttp://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
localhostorigin handshake test - Automated: Avoid spawning full
CamelMonitorin lightweight handshake tests
Inline comments
Detailed line-level comments are attached in the review thread:
#25317 (review)
Thanks for the contribution — --web is a nice complement to --mcp for local development workflows.
Re-review — follow-up on prior BugBot + Grok commentsRe-checked PR head Cursor on behalf of atiaomar1978-hub SummaryMost of the original medium/low findings are still open. The author did land some improvements (notably in Fixed / improved since original review
Still open (original review items)
RecommendationGood progress on thread-pool bounds, termination waiting, docs, and dependency hygiene. Before merge, I'd still want at least:
Happy to re-review once these land. |
Adds an opt-in --web/--web-port flag that serves the TUI dashboard to a browser over WebSocket, using TamboUI's Aesh backend and Aesh's terminal-http Netty bootstrap. Each browser connection gets its own independently-scanning CamelMonitor session (same as running a second local terminal session), driven through an AeshBackend instead of the JLine backend used for local terminals. Matches the existing TuiMcpServer security posture: loopback-only bind, opt-in, no authentication for v1. Closes the corresponding documentation gap in security-model.adoc for both --mcp and --web. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ble link, quit gating - Vendor xterm.js/xterm-addon-fit instead of loading from a CDN, and serve a Camel-branded page (logo, disconnect overlay) instead of Aesh's default. - Show the "web :port" badge as an underlined OSC 8 hyperlink in the local terminal session only (hidden once a browser session is attached). - A browser session can no longer quit the shared TUI process via q/Ctrl+C, and the footer hint no longer advertises quit for web sessions. - Build the JLine backend explicitly in TuiBackendHelper so tamboui's ServiceLoader auto-discovery doesn't pick AeshBackend for local sessions now that tamboui-aesh-backend is also on the classpath. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
- Bound the session executor thread pool instead of an unbounded cached pool. - Wait for the session executor to terminate in awaitTermination(), not just the Netty event loops. - Replace FQCN java.net.BindException with an import in CamelMonitor. - Strip the incorrectly-added ASF headers from the vendored MIT-licensed xterm.js/xterm.css/xterm-addon-fit.js assets, and exclude them from both the RAT and license-maven-plugin checks in the module's own pom.xml. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Declares the Netty artifacts TuiWebServer imports directly (netty-common, netty-transport, netty-handler, netty-codec-http) instead of relying on them arriving transitively via org.aesh:terminal-http, and documents why isAllowedOrigin() intentionally allows a missing Origin header. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Fixes 11 issues flagged by an automated review of the web TUI transport layer: - Wait for in-flight sessions to drain (with a timeout) before forcing shutdown, instead of interrupting them immediately in stop(). - Compare the request path (not the raw URI) when deciding whether to enforce the Origin check, closing a bypass via query strings on /ws. - Bound the session executor's queue and reject new sessions once full, instead of queueing unbounded work. - Add X-Frame-Options and Content-Security-Policy response headers to deny framing of the served pages. - Handle RejectedExecutionException when submitting a session so a connection is closed cleanly instead of being silently dropped. - Track a single shared deadline across all three shutdown waits in awaitTermination(), instead of allowing up to 2x the requested timeout. - Dispose the ResizeObserver and Terminal instance on disconnect in the browser client, instead of leaking them until a page reload. - Let tests substitute a lightweight session handler instead of always spawning a full CamelMonitor for WebSocket handshake checks, and add origin coverage for the http://localhost:<port> case. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed all 11 actionable items from @atiaomar1978-hub's automated review (Cursor BugBot + Grok):
All 861 tests in Claude Code on behalf of ammachado |
Re-review #3 — automated review findings addressed ✅Re-checked PR head Cursor on behalf of atiaomar1978-hub Verdict: Ready to approve (with optional nits below)Commit Issue tracker — prior review vs current head
Optional nits (non-blocking)
These are polish items only; the production code looks solid. Code quality notes (looks good)
LGTM from the automated review perspective. Thanks for the quick turnaround on |
atiaomar1978-hub
left a comment
atiaomar1978-hub
left a comment
There was a problem hiding this comment.
Approved ✅
Reviewed head 51bce14e after the automated-review fixes. All prior medium/low findings are addressed:
- Graceful session shutdown with timeout before force-stop
- Clickjacking mitigated (
X-Frame-Options+ CSPframe-ancestors 'none') - Bounded session executor queue (64) with clean rejection handling
- Origin check uses request path (no
/ws?…bypass) - Browser sessions can quit independently; docs and behavior aligned
- Lightweight no-op session handler for transport tests
- Client-side cleanup on disconnect (
ResizeObserver,term.dispose())
Loopback bind, security-model documentation, and origin validation remain sound. LGTM.
Cursor on behalf of atiaomar1978-hub
Description
Adds
--webtocamel tui monitor, serving the live TamboUI dashboard over a loopback WebSocket so it can be used from a browser on the same host.The implementation uses TamboUI's Aesh backend and Aesh terminal bridge. Each browser connection has an independent
CamelMonitor/TuiRunnersession. It is opt-in and binds only to127.0.0.1.This PR also:
web :portlink in the terminal TUI and prevents a browser session from quitting the local TUI.localhostand127.0.0.1browser page origins.Validation
mvn test -Dtest=TuiWebServerTest -DfailIfNoTests=falsemvn testindsl/camel-jbang/camel-jbang-plugin-tui(804 tests)Codex on behalf of ammachado