[SPARK-59334][CORE][MINOR] Handle invalid application/worker links - #58622
[SPARK-59334][CORE][MINOR] Handle invalid application/worker links#58622holdenk wants to merge 1 commit into
Conversation
…eploy UI pages Application and worker UI addresses come from external registrants and could contain bad/invalid info. Filter it out to prevent the UI from breaking. Co-authored-by: Cursor <cursoragent@cursor.com> Fix links
HyukjinKwon
left a comment
HyukjinKwon
left a comment
There was a problem hiding this comment.
0 blocking, 1 non-blocking, 0 nits.
The implementation is correct and low-risk. The single point raised is a non-blocking test-coverage suggestion: the added unit test asserts the pass-through cases and the null -> "#" case but never exercises the reject path for a non-null disallowed-scheme or malformed input, so the PR's core new behavior is not directly asserted.
Suggestions (1)
- core/src/test/scala/org/apache/spark/ui/UIUtilsSuite.scala:253: Test does not assert the reject path (disallowed-scheme / malformed href). -- see inline
Verification
Ran no builds/tests as part of this review. The relevant suite to run locally is build/sbt "core/testOnly org.apache.spark.ui.UIUtilsSuite".
| 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) === "#") |
There was a problem hiding this comment.
The null input short-circuits at href != null before the URI parse, so this test never exercises the two branches that implement the PR's core behavior: a non-null value with a disallowed scheme, and a value that trips URISyntaxException. Since the test is named "only renders http(s) or relative URLs as hyperlinks", consider adding assertions for the reject path, e.g.:
assert(UIUtils.makeHref(proxy = false, "app-1", "javascript:alert(1)") === "#")
assert(UIUtils.makeHref(proxy = false, "app-1", "ht tp://bad url") === "#") // malformed -> URISyntaxExceptionNon-blocking.
3 participants
What changes were proposed in this pull request?
Drop invalid links rather than break rendering
Why are the changes needed?
Application and worker UI addresses come from external registrants and could contain bad/invalid info. Filter it out to prevent the UI from breaking.
Does this PR introduce any user-facing change?
No
How was this patch tested?
New UIUtilSuite test
Was this patch authored or co-authored using generative AI tooling?
Yes