dongjoon-hyun commented on code in PR #57549:
URL: https://github.com/apache/spark/pull/57549#discussion_r3677325314
##########
core/src/main/scala/org/apache/spark/ui/UIUtils.scala:
##########
@@ -211,7 +211,19 @@ private[spark] object UIUtils extends Logging {
request: HttpServletRequest,
basePath: String = "",
resource: String = ""): String = {
- uiRoot(request) + basePath + resource
+ val root = uiRoot(request).stripSuffix("/")
+ val hasScheme = basePath.startsWith("http://";) ||
basePath.startsWith("https://";)
+ val cleanBase = if (hasScheme || basePath.startsWith("/") ||
basePath.isEmpty) {
+ basePath
+ } else {
+ "/" + basePath
+ }
+ val cleanResource = if (resource.startsWith("/") || resource.isEmpty) {
+ resource
+ } else {
+ "/" + resource
+ }
Review Comment:
This normalization looks out of scope for this fix. Checking all current
call sites,
`basePath` and `resource` are always either empty or already start with `/`,
so the
slash-insertion is a no-op today — and this is a shared path that affects
every UI
(YARN, K8s, History Server), not just Standalone reverse proxy.
The suggested `hasScheme` branch is also buggy as written: no caller passes
an absolute URL as
`basePath`, but if one did while `root` is non-empty, `s"$root$cleanBase"`
would produce
a broken URL like `/proxy/app-1http://...`.
Please recover the original code.
##########
core/src/main/scala/org/apache/spark/ui/HttpSecurityFilter.scala:
##########
@@ -49,9 +49,12 @@ private class HttpSecurityFilter(
val hres = res.asInstanceOf[HttpServletResponse]
hres.setHeader("Cache-Control", "no-cache, no-store, must-revalidate")
+ val isProxyRequest =
Option(hreq.getPathInfo).exists(_.startsWith("/proxy/")) ||
+ Option(hreq.getRequestURI).exists(_.contains("/proxy/"))
Review Comment:
The current check is spoofable: `getRequestURI.contains("/proxy/")` matches
any URL
containing that substring (e.g. `/jobs/proxy/x`, `/history/proxy/...`), so
an attacker-chosen
URL can disable CSP on any UI (driver, Worker, History Server) — not just
the Master's proxy.
Meanwhile `getPathInfo.startsWith("/proxy/")` never matches for the actual
proxy handler,
because it is mounted with `contextPath="/proxy"`, so its `pathInfo` is
`/app-123/...`.
Checking the servlet context path is exact and cannot be forged by crafted
request paths:
```suggestion
val isProxyRequest = hreq.getContextPath == "/proxy"
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
