atiaomar1978-hub commented on PR #25317: URL: https://github.com/apache/camel/pull/25317#issuecomment-5182379915
## Re-review — follow-up on prior BugBot + Grok comments Re-checked PR head `265946e1` against the review posted at `b42b60bb`. _Cursor on behalf of atiaomar1978-hub_ --- ### Summary **Most of the original medium/low findings are still open.** The author did land some improvements (notably in `a46d7ed` / `265946e1`), but **`TuiWebServer.java`, `TuiWebServerTest.java`, and `index.html` are unchanged since the prior review commit** — none of the line-level issues called out in the earlier thread have been patched yet. --- ### Fixed / improved since original review | Item | Status | Notes | |------|--------|-------| | Unbounded `CachedThreadPool` | **Fixed** | Now `newFixedThreadPool(max(4, 2×cpus))` (`a46d7ed`) | | `awaitTermination()` ignores session executor | **Fixed** | Now waits on `sessionExecutor` with a shared deadline (`a46d7ed`) | | Missing `Origin` undocumented | **Fixed** | Comment added in `isAllowedOrigin()` (`265946e1`) | | Explicit Netty dependencies | **Fixed** | Declared in `pom.xml` instead of transitive-only (`265946e1`) | | Vendored xterm license headers | **Fixed** | ASF headers removed; RAT/license excludes added (`a46d7ed`) | | Loopback-only docs wording | **Fixed** | `camel-jbang-tui.adoc` clarified same-host access (`91cf90e8`) | | `BindException` FQCN | **Fixed** | Proper import in `CamelMonitor` (`a46d7ed`) | | Custom Netty pipeline + origin check | **Present** | From `e23a572` hardening commit | --- ### Still open (original review items) | Severity | Location | Issue | Status | |----------|----------|-------|--------| | **Medium** | `TuiWebServer.java:121-132` | `stop()` still calls `shutdownNow()` immediately — no graceful drain of in-flight browser sessions before tearing down Netty | **Open** | | **Medium** | `TuiWebServer.java:173` + `index.html` | No `X-Frame-Options` / CSP `frame-ancestors` — clickjacking via iframe on loopback still possible | **Open** | | **Medium** | `TuiWebServer.java:76-81` | Fixed pool but **unbounded queue** — many connections can still queue unbounded `CamelMonitor.call()` tasks | **Open** | | **Medium** | `TuiWebServer.java:183` | Origin check skipped when URI is `/ws?…` (exact match only) | **Open** | | **Medium** | `CamelMonitor.java:793-798` | Misleading comment (“shared monitor”); `q`/Ctrl+C swallowed but AI `/quit` still exits session | **Open** | | **Medium** | `TuiWebServerTest.java:98-147` | Successful handshake still spawns full `CamelMonitor` with weak teardown | **Open** | | **Low** | `TuiWebServer.java:143` | No `RejectedExecutionException` handling after `stop()` | **Open** | | **Low** | `TuiWebServer.java:135-139` | `awaitTermination` still gives boss + worker each the full timeout (only remainder goes to executor) | **Partial** | | **Low** | `index.html:247-255` | `ResizeObserver` / `Terminal` not cleaned up on disconnect | **Open** | | **Low** | `TuiWebServerTest.java` | No test for `http://localhost:<port>` origin | **Open** | | **Low** | `camel-jbang-tui.adoc:779` | Docs still say “same keyboard shortcuts”; quit differs in browser | **Open** | --- ### Recommendation Good progress on thread-pool bounds, termination waiting, docs, and dependency hygiene. Before merge, I'd still want at least: 1. Graceful session shutdown in `stop()` (or document that open browser tabs are force-killed). 2. Frame-denial headers on the HTML response (clickjacking). 3. Fix or document the `/ws?…` origin-check bypass. 4. Align quit behavior + comment in `CamelMonitor` (either allow browser `q` for that session only, or block `/quit` too). Happy to re-review once these land. -- 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]
