Re: [PR] [SPARK-59334][CORE] Handle invalid application/worker links [spark]

["dongjoon-hyun (via GitHub)" <gi...@apache.org>]

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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org