dongjoon-hyun commented on code in PR #58622:
URL: https://github.com/apache/spark/pull/58622#discussion_r3985570510
##########
core/src/test/scala/org/apache/spark/ui/UIUtilsSuite.scala:
##########
@@ -244,4 +244,14 @@ class UIUtilsSuite extends SparkFunSuite {
assert(html.contains("data-toggle-details"), "detailsUINode should use
data-toggle-details")
assert(html.contains("stacktrace-details"), "detailsUINode should contain
stacktrace-details")
}
+
+ test("makeHref only renders http(s) or relative URLs as hyperlinks") {
+ // appUiUrl and webUiAddress come from external registrants.
+ assert(UIUtils.makeHref(proxy = false, "app-1", "http://host:4040";) ===
"http://host:4040";)
+ assert(UIUtils.makeHref(proxy = false, "app-1", "https://host:4040";) ===
"https://host:4040";)
+ assert(UIUtils.makeHref(proxy = false, "app-1", "/relative/path") ===
"/relative/path")
+ assert(UIUtils.makeHref(proxy = false, "app-1", null) === "#")
+ // Reverse-proxy mode is unaffected: the href is generated by the master
itself.
+ assert(UIUtils.makeHref(proxy = true, "app-1",
"badlink").endsWith("/proxy/app-1"))
Review Comment:
`"badlink"` has no scheme, so `isSafeHref` would accept it anyway. As a
result, this assertion doesn't prove that the proxy branch bypasses the filter.
In addition to the reject-path assertions suggested above, shall we use an
unsafe href here?
```scala
assert(UIUtils.makeHref(proxy = false, "app-1", "javascript:alert(1)") ===
"#")
assert(UIUtils.makeHref(proxy = false, "app-1", "http://host:4040/a b") ===
"#")
assert(UIUtils.makeHref(proxy = true, "app-1",
"javascript:alert(1)").endsWith("/proxy/app-1"))
```
##########
core/src/main/scala/org/apache/spark/ui/UIUtils.scala:
##########
@@ -787,8 +787,22 @@ private[spark] object UIUtils extends Logging {
if (proxy) {
val proxyPrefix = sys.props.getOrElse("spark.ui.proxyBase", "")
proxyPrefix + "/proxy/" + id
Review Comment:
The href is generated by the master in reverse-proxy mode, but the proxy
target still comes from the same externally registered `appUiUrl` /
`webUiAddress` via `MasterWebUI.idToUiAddress` and `JettyUtils.createProxyURI`.
So, this PR covers only the non-proxy rendering path. Could you mention it in
the PR description or file a follow-up JIRA if you want to validate the proxy
target too?
--
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]
