atiaomar1978-hub commented on PR #25317: URL: https://github.com/apache/camel/pull/25317#issuecomment-5182776452
## Re-review #3 — automated review findings addressed ✅ Re-checked PR head **`51bce14e`** (`CAMEL-24202: address automated review findings on web TUI`). _Cursor on behalf of atiaomar1978-hub_ --- ### Verdict: **Ready to approve** (with optional nits below) Commit `51bce14` directly addresses **all medium items** and **most low items** from the prior BugBot + Grok review. Nice, focused transport-layer hardening. --- ### Issue tracker — prior review vs current head | Prior finding | Status | Evidence | |---------------|--------|----------| | `stop()` immediate `shutdownNow()` race | **Fixed** | `shutdown()` + `awaitTermination(5s)` before `shutdownNow()` fallback | | Clickjacking (no frame denial) | **Fixed** | `SecurityHeadersHandler`: `X-Frame-Options: DENY` + `CSP: frame-ancestors 'none'` | | Unbounded session queue | **Fixed** | `ThreadPoolExecutor` + `ArrayBlockingQueue(64)` + `RejectedExecutionException` handling | | Origin check `/ws?…` bypass | **Fixed** | `QueryStringDecoder(request.uri()).path()` before compare | | Quit gating / wrong comment | **Fixed** | Removed `webSession` flag; browser `q`/Ctrl+C now quits **this session**; comment corrected | | Handshake tests spawn full `CamelMonitor` | **Fixed** | `newServerWithNoOpSession()` + injectable `sessionHandler` constructor | | `RejectedExecutionException` after stop | **Fixed** | Caught in `accept()`, connection closed cleanly | | `awaitTermination` ~2× timeout | **Fixed** | Single shared deadline across boss/worker/session waits | | `index.html` ResizeObserver / Terminal leak | **Fixed** | `disconnect()`, `clearTimeout`, `term.dispose()` on close | | Missing `localhost` origin test | **Fixed** | `acceptsWebSocketUpgradeFromTheLoopbackHostname` | | Docs “same shortcuts” vs quit | **Fixed** | Behavior now matches docs (`q` works in browser sessions) | --- ### Optional nits (non-blocking) 1. **Test coverage gap** — `servesTheCustomIndexPageWithVendoredAssets` could assert `X-Frame-Options` / `Content-Security-Policy` response headers now that `SecurityHeadersHandler` exists. 2. **Test coverage gap** — no explicit test for foreign origin on `/ws?token=…` (the path parsing fix is correct in code; a regression test would lock it in). 3. **Test coverage gap** — no test exercising queue-full / session rejection (would document the 64-slot admission limit). These are polish items only; the production code looks solid. --- ### Code quality notes (looks good) - Injectable `sessionHandler` for tests is clean — no need to mock Netty. - Graceful shutdown ordering (close channels → drain sessions → tear down Netty) is sensible. - Removing the special-case `webSession` plumbing simplifies `CamelMonitor` / `OverviewTab` nicely. **LGTM** from the automated review perspective. Thanks for the quick turnaround on `51bce14`. -- 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]
