From cf34ccb40dbc4545f189abe4aa4e17854e20a689 Mon Sep 17 00:00:00 2001 From: Laszlo Bodor Date: Fri, 18 Sep 2026 13:05:27 +0200 Subject: [PATCH 1/2] TEZ-4755: AMWebController: rendering improvements in StaticAMView Encode the configured history-url value when splicing it into the StaticAMView redirect page so unexpected characters cannot alter the surrounding HTML or JavaScript context. Adds a unit test. Co-Authored-By: Claude Code --- .../tez/dag/app/web/AMWebController.java | 60 ++++++++++++++++++- .../tez/dag/app/web/TestAMWebController.java | 38 ++++++++++++ 2 files changed, 95 insertions(+), 3 deletions(-) diff --git a/tez-dag/src/main/java/org/apache/tez/dag/app/web/AMWebController.java b/tez-dag/src/main/java/org/apache/tez/dag/app/web/AMWebController.java index e547dad520..b1a028fa07 100644 --- a/tez-dag/src/main/java/org/apache/tez/dag/app/web/AMWebController.java +++ b/tez-dag/src/main/java/org/apache/tez/dag/app/web/AMWebController.java @@ -921,17 +921,71 @@ private void render(PrintWriter pw) { "

To enable tracking url pointing to Tez UI, set the config " + TezConfiguration.TEZ_HISTORY_URL_BASE + " in the tez-site.xml.

"); } else { + // historyUrl is derived from a submitter-supplied AM configuration + // property (tez.tez-ui.history-url.base). Escape it before splicing + // into the HTML attribute and the inline JS string literal so a + // value like ' or " cannot break out and run script in the browser + // of whoever opens the AM tracking URL. pw.write("

Redirecting to Tez UI

.

If you are not redirected shortly, click " + - "here

" + "here

" ); pw.write(""); + "window.location.replace('" + escapeJsString(historyUrl) + "');" + + "}, 0); "); } pw.write(""); pw.write(""); pw.flush(); } + + static String escapeHtmlAttribute(String s) { + StringBuilder sb = new StringBuilder(s.length() + 16); + for (int i = 0; i < s.length(); i++) { + char c = s.charAt(i); + switch (c) { + case '&': sb.append("&"); break; + case '<': sb.append("<"); break; + case '>': sb.append(">"); break; + case '"': sb.append("""); break; + case '\'': sb.append("'"); break; + default: sb.append(c); + } + } + return sb.toString(); + } + + static String escapeJsString(String s) { + StringBuilder sb = new StringBuilder(s.length() + 16); + for (int i = 0; i < s.length(); i++) { + char c = s.charAt(i); + switch (c) { + case '\\': sb.append("\\\\"); break; + case '\'': sb.append("\\'"); break; + case '"': sb.append("\\\""); break; + case '\n': sb.append("\\n"); break; + case '\r': sb.append("\\r"); break; + case '\t': sb.append("\\t"); break; + case '\b': sb.append("\\b"); break; + case '\f': sb.append("\\f"); break; + case '<': sb.append("\\u003c"); break; + case '>': sb.append("\\u003e"); break; + case '&': sb.append("\\u0026"); break; + case '/': sb.append("\\/"); break; + // JS line terminators: illegal raw inside a string literal pre-ES2019. + // Written numerically because a \\u2028 source escape would be decoded + // by javac's lexer into a real line break and split this file. + case 0x2028: sb.append("\\u2028"); break; + case 0x2029: sb.append("\\u2029"); break; + default: + if (c < 0x20) { + sb.append(String.format("\\u%04x", (int) c)); + } else { + sb.append(c); + } + } + } + return sb.toString(); + } } @VisibleForTesting diff --git a/tez-dag/src/test/java/org/apache/tez/dag/app/web/TestAMWebController.java b/tez-dag/src/test/java/org/apache/tez/dag/app/web/TestAMWebController.java index e5a8c0e0a7..f6cf853315 100644 --- a/tez-dag/src/test/java/org/apache/tez/dag/app/web/TestAMWebController.java +++ b/tez-dag/src/test/java/org/apache/tez/dag/app/web/TestAMWebController.java @@ -890,4 +890,42 @@ private void verifySingleAttemptResult(TaskAttempt mockTask, Map assertEquals(Float.toString(mockTask.getProgress()), taskResult.get("progress")); } + @Test + public void testStaticAMViewEscapesHistoryUrl() { + // Angle brackets and quotes in the submitter-controlled history url + // must not appear literally in the rendered attribute or JS literal. + String malicious = "http://ui/'\">"; + String htmlEscaped = AMWebController.StaticAMView.escapeHtmlAttribute(malicious); + assertFalse(htmlEscaped.contains(" that would end the enclosing script element + // must be neutralised too. + assertFalse(jsEscaped.contains(""), + "Closing must be neutralised: " + jsEscaped); + } + + @Test + public void testEscapeJsStringEscapesLineTerminators() { + // U+2028/U+2029 are JS line terminators: raw, they break the string + // literal pre-ES2019 and the redirect never runs. Built numerically — + // a source \\u2028 escape is decoded by javac before string parsing. + String ls = String.valueOf((char) 0x2028); + String ps = String.valueOf((char) 0x2029); + String escaped = AMWebController.StaticAMView.escapeJsString("http://ui/" + ls + ps + "x"); + assertFalse(escaped.contains(ls), "U+2028 must be escaped: " + escaped); + assertFalse(escaped.contains(ps), "U+2029 must be escaped: " + escaped); + assertTrue(escaped.contains("\\u2028"), "Expected \\u2028 escape: " + escaped); + assertTrue(escaped.contains("\\u2029"), "Expected \\u2029 escape: " + escaped); + } + } From f41516c32b67f1bdd2410383286ee579a6bf443c Mon Sep 17 00:00:00 2001 From: Laszlo Bodor Date: Mon, 2 Nov 2026 13:22:18 +0100 Subject: [PATCH 2/2] PR comments --- pom.xml | 6 ++ tez-dag/pom.xml | 4 ++ .../tez/dag/app/web/AMWebController.java | 57 ++----------------- .../tez/dag/app/web/TestAMWebController.java | 56 ++++++++++-------- 4 files changed, 46 insertions(+), 77 deletions(-) diff --git a/pom.xml b/pom.xml index dbd8b1c2f3..1b25d56789 100644 --- a/pom.xml +++ b/pom.xml @@ -74,6 +74,7 @@ 4.4 2.22.0 3.19.0 + 1.15.0 5.9.0 3.2.0 4.9.3 @@ -328,6 +329,11 @@ commons-lang3 ${commons-lang3.version} + + org.apache.commons + commons-text + ${commons-text.version} + io.netty netty-all diff --git a/tez-dag/pom.xml b/tez-dag/pom.xml index f4c9ed8867..af9b4235cd 100644 --- a/tez-dag/pom.xml +++ b/tez-dag/pom.xml @@ -111,6 +111,10 @@ org.apache.commons commons-lang3 + + org.apache.commons + commons-text + commons-codec commons-codec diff --git a/tez-dag/src/main/java/org/apache/tez/dag/app/web/AMWebController.java b/tez-dag/src/main/java/org/apache/tez/dag/app/web/AMWebController.java index b1a028fa07..b99bd69986 100644 --- a/tez-dag/src/main/java/org/apache/tez/dag/app/web/AMWebController.java +++ b/tez-dag/src/main/java/org/apache/tez/dag/app/web/AMWebController.java @@ -36,6 +36,7 @@ import javax.servlet.http.HttpServletResponse; +import org.apache.commons.text.StringEscapeUtils; import org.apache.hadoop.security.UserGroupInformation; import org.apache.hadoop.yarn.webapp.Controller; import org.apache.hadoop.yarn.webapp.MimeType; @@ -909,7 +910,8 @@ public void render() { } } - private void render(PrintWriter pw) { + @VisibleForTesting + void render(PrintWriter pw) { pw.write(""); pw.write(""); pw.write(""); @@ -927,65 +929,16 @@ private void render(PrintWriter pw) { // value like ' or " cannot break out and run script in the browser // of whoever opens the AM tracking URL. pw.write("

Redirecting to Tez UI

.

If you are not redirected shortly, click " + - "here

" + "here

" ); pw.write(""); } pw.write(""); pw.write(""); pw.flush(); } - - static String escapeHtmlAttribute(String s) { - StringBuilder sb = new StringBuilder(s.length() + 16); - for (int i = 0; i < s.length(); i++) { - char c = s.charAt(i); - switch (c) { - case '&': sb.append("&"); break; - case '<': sb.append("<"); break; - case '>': sb.append(">"); break; - case '"': sb.append("""); break; - case '\'': sb.append("'"); break; - default: sb.append(c); - } - } - return sb.toString(); - } - - static String escapeJsString(String s) { - StringBuilder sb = new StringBuilder(s.length() + 16); - for (int i = 0; i < s.length(); i++) { - char c = s.charAt(i); - switch (c) { - case '\\': sb.append("\\\\"); break; - case '\'': sb.append("\\'"); break; - case '"': sb.append("\\\""); break; - case '\n': sb.append("\\n"); break; - case '\r': sb.append("\\r"); break; - case '\t': sb.append("\\t"); break; - case '\b': sb.append("\\b"); break; - case '\f': sb.append("\\f"); break; - case '<': sb.append("\\u003c"); break; - case '>': sb.append("\\u003e"); break; - case '&': sb.append("\\u0026"); break; - case '/': sb.append("\\/"); break; - // JS line terminators: illegal raw inside a string literal pre-ES2019. - // Written numerically because a \\u2028 source escape would be decoded - // by javac's lexer into a real line break and split this file. - case 0x2028: sb.append("\\u2028"); break; - case 0x2029: sb.append("\\u2029"); break; - default: - if (c < 0x20) { - sb.append(String.format("\\u%04x", (int) c)); - } else { - sb.append(c); - } - } - } - return sb.toString(); - } } @VisibleForTesting diff --git a/tez-dag/src/test/java/org/apache/tez/dag/app/web/TestAMWebController.java b/tez-dag/src/test/java/org/apache/tez/dag/app/web/TestAMWebController.java index f6cf853315..7acfdf631a 100644 --- a/tez-dag/src/test/java/org/apache/tez/dag/app/web/TestAMWebController.java +++ b/tez-dag/src/test/java/org/apache/tez/dag/app/web/TestAMWebController.java @@ -35,6 +35,8 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import java.io.PrintWriter; +import java.io.StringWriter; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -890,42 +892,46 @@ private void verifySingleAttemptResult(TaskAttempt mockTask, Map assertEquals(Float.toString(mockTask.getProgress()), taskResult.get("progress")); } + private static String renderStaticAMView(String historyUrl) { + AMWebController.StaticAMView view = new AMWebController.StaticAMView(); + view.historyUrl = historyUrl; + StringWriter out = new StringWriter(); + view.render(new PrintWriter(out)); + return out.toString(); + } + @Test public void testStaticAMViewEscapesHistoryUrl() { // Angle brackets and quotes in the submitter-controlled history url - // must not appear literally in the rendered attribute or JS literal. - String malicious = "http://ui/'\">"; - String htmlEscaped = AMWebController.StaticAMView.escapeHtmlAttribute(malicious); - assertFalse(htmlEscaped.contains(" that would end the enclosing script element - // must be neutralised too. - assertFalse(jsEscaped.contains(""), - "Closing must be neutralised: " + jsEscaped); + // must not break out of the rendered attribute or JS literal. + String rendered = renderStaticAMView("http://ui/'\">"); + + // Double-quoted href: " < > & are entity-encoded, a bare ' is harmless. + assertTrue(rendered.contains( + ""), + "Unexpected href encoding: " + rendered); + // Single-quoted JS literal: ' and " are backslash-escaped and / becomes \/, + // so the injected cannot close the enclosing script element. + assertTrue(rendered.contains( + "window.location.replace('http:\\/\\/ui\\/\\'\\\">", -1).length - 1, + "Only the page's own may appear: " + rendered); } @Test - public void testEscapeJsStringEscapesLineTerminators() { + public void testStaticAMViewEscapesJsLineTerminators() { // U+2028/U+2029 are JS line terminators: raw, they break the string // literal pre-ES2019 and the redirect never runs. Built numerically — // a source \\u2028 escape is decoded by javac before string parsing. String ls = String.valueOf((char) 0x2028); String ps = String.valueOf((char) 0x2029); - String escaped = AMWebController.StaticAMView.escapeJsString("http://ui/" + ls + ps + "x"); - assertFalse(escaped.contains(ls), "U+2028 must be escaped: " + escaped); - assertFalse(escaped.contains(ps), "U+2029 must be escaped: " + escaped); - assertTrue(escaped.contains("\\u2028"), "Expected \\u2028 escape: " + escaped); - assertTrue(escaped.contains("\\u2029"), "Expected \\u2029 escape: " + escaped); + String rendered = renderStaticAMView("http://ui/" + ls + ps + "x"); + String script = rendered.substring(rendered.indexOf("