wgzhao commented on PR #12775:
URL: https://github.com/apache/seatunnel/pull/12775#issuecomment-6207365128
Thanks @SEZ9 for the detailed pass. Five of the six points are addressed on
the new head `dc92ae6a7`; I kept the current redirect emission style for #4 and
explain why below.
**#1 context-path is now covered end to end.** `WebUiEntryPointIT` starts a
second node with `context-path=/seatunnel` on its own port and asserts `GET
/seatunnel/%23/overview` answers `302` with `Location: /seatunnel/#/overview`,
so the behaviour both docs files promise is checked against Jetty's real
`getRequestURI()`/`getContextPath()` pair instead of only by the helper unit
test. The case asserts the redirect only, not the UI render: under a non-root
context path the packaged `index.html` still references `/assets/...`, which is
a separate pre-existing issue I will report separately rather than fix here.
**#2 the redirect now only re-emits well-formed URI text.** `isUriText`
accepts URI characters with `%` only as a well-formed `%HH` escape that is
neither a control character nor `DEL`; anything else - a raw `<`, an encoded
line break, a truncated escape - falls through to the normal 404. That pins the
invariant in this class rather than leaving it to the HTTP layer, and the unit
test now covers `/%23/<script>alert(1)</script>`, `?x=%0d%0aInjected:%20yes`,
`?x=%2` and `?x=100%`, while
`/%23/%E4%B8%AD%E6%96%87/jobs?tab=metrics&pretty=true` still redirects.
**#3 the symptom and the contract are documented where operators look.**
`web-ui.md` (en/zh) now names what is observable - `302` with `Location:
<context-path>/#<route>`, for `GET`/`HEAD` only - and the `URI: /%23/overview`
page is listed in the "Web UI and Port 8080 Troubleshooting" section of
`rest-api-v2.md` and its zh counterpart, with the `curl -i --path-as-is`
command that shows the redirect.
**#4 I kept `setStatus(SC_FOUND)` + `setHeader("Location", ...)` on
purpose.** I tried both alternatives on Jetty 9.4.56:
`sendRedirect("/#/overview")` is rewritten to an absolute URL (`Location:
http://127.0.0.1:18090/#/overview`), which would send the browser to the node's
internal host and scheme behind a reverse proxy or an HTTPS ingress, and
`setContentLength(0)` + `flushBuffer()` produces headers byte-identical to the
current code (`302`, relative `Location`, `Content-Length: 0`). So the current
form is the one that keeps the documented relative `Location`, and I would
rather not trade that for an explicit commit.
**#5 the filter is registered only when there is a UI to land on.** The
registration moved inside the `uiResource != null` branch. Verified on a node
whose `seatunnel-engine-server` jar had the `ui/` entries removed: `GET
/%23/overview` answers `404` directly, `JettyService` logs "UI resources not
found in classpath", and `/overview` still answers `200`.
**#6 renamed to `ENCODED_FRAGMENT_PATH_PREFIX`.**
Local verification on the new head: `WebUiFragmentRedirectFilterTest` 9/9,
`RestApiHttpBasicTest` 4/4, the whole `seatunnel-engine-server` REST area
113/113, `WebUiEntryPointIT` 7/7, and `./mvnw spotless:apply` is clean. CI is
running again on `dc92ae6a7`.
--
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]
