CVE-2022-34778

@@ -10,4 +10,5 @@ nbactions-jenkins.xml
 /.idea
 /*.iml
 /nbproject/
-.DS_Store
\ No newline at end of file
+.DS_Store
+.dccache
\ No newline at end of file

@@ -93,4 +93,16 @@ Results**. This option allows you to configure the following properties:
       step([$class: 'Publisher', reportFilenamePattern: '**/testng-results.xml'])
     }
   }
-```
\ No newline at end of file
+```
+
+### Properties
+
+Some TestNG plugin properties can only be controlled by command line properties set at Jenkins startup.
+
+#### Allow unescaped HTML
+
+[SECURITY-2788](insert-advisory-hyperlink-here) notes that test description and test exception messages allow unescaped HTML, leading to a cross-site scripting vulnerability.
+Current releases of the TestNG plugin always escape test description and test exception messages.
+If test description or test exceptions messages must not be escaped and administrators accept the risk of disabling this security safeguard, set the Java property
+`hudson.plugins.testng.Publisher.allowUnescapedHTML=true`
+from the command line that starts the Jenkins controller.

@@ -6,6 +6,8 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 
 import hudson.EnvVars;
 import hudson.Extension;
@@ -33,6 +35,7 @@
  *
  */
 public class Publisher extends Recorder implements SimpleBuildStep {
+   private static final Logger LOGGER = Logger.getLogger(Publisher.class.getName());
 
    //ant style regex pattern to find report files
    private String reportFilenamePattern= "**/testng-results.xml";
@@ -57,6 +60,26 @@ public class Publisher extends Recorder implements SimpleBuildStep {
    private Integer failedFails = 100;
    private Integer thresholdMode = 2;
 
+   /** SECURITY-2788 - do not allow unescaped HTML in description or exception message */
+   /* Log a warning if the value is set to true */
+   private static final boolean ALLOW_UNESCAPED_HTML = Boolean.valueOf(System.getProperty("hudson.plugins.testng.Publisher.allowUnescapedHTML", "false"));
+   private static boolean allowUnescapedHTML = ALLOW_UNESCAPED_HTML;
+
+   /* Package protected for testability */
+   static void setAllowUnescapedHTML(boolean value) {
+      allowUnescapedHTML = value;
+   }
+
+   /* Package protected for testability */
+   static boolean getAllowUnescapedHTML() {
+      return allowUnescapedHTML;
+   }
+
+   static {
+      if(ALLOW_UNESCAPED_HTML) {
+         LOGGER.log(Level.WARNING, "You are vulnerable to a cross-site scripting attack through the TestNG plugin. Remove the system property hudson.plugins.testng.Publisher.allowUnescapedHTML from your Jenkins controller startup.");
+      }
+   }
 
    @Extension
    public static final DescriptorImpl DESCRIPTOR = new DescriptorImpl();
@@ -74,21 +97,37 @@ public void setReportFilenamePattern(String reportFilenamePattern) {
    }
 
    public boolean getEscapeTestDescp() {
-       return escapeTestDescp;
+      if (allowUnescapedHTML) {
+         return escapeTestDescp;
+      } else {
+         return true;
+      }
    }
 
    @DataBoundSetter
    public void setEscapeTestDescp(boolean escapeTestDescp) {
-       this.escapeTestDescp = escapeTestDescp;
+      if (allowUnescapedHTML) {
+         this.escapeTestDescp = escapeTestDescp;
+      } else {
+         this.escapeTestDescp = true;
+      }
    }
 
    public boolean getEscapeExceptionMsg() {
-       return escapeExceptionMsg;
+      if (allowUnescapedHTML) {
+         return escapeExceptionMsg;
+      } else {
+         return true;
+      }
    }
 
    @DataBoundSetter
    public void setEscapeExceptionMsg(boolean escapeExceptionMsg) {
-       this.escapeExceptionMsg = escapeExceptionMsg;
+      if (allowUnescapedHTML) {
+         this.escapeExceptionMsg = escapeExceptionMsg;
+      } else {
+         this.escapeExceptionMsg = true;
+      }
    }
 
    public boolean getFailureOnFailedTestConfig() {

@@ -35,8 +35,13 @@ public class TestNGProjectAction extends TestResultProjectAction implements Prom
    public TestNGProjectAction(Job<?, ?> project,
          boolean escapeTestDescp, boolean escapeExceptionMsg, boolean showFailedBuilds) {
       super(project);
-      this.escapeExceptionMsg = escapeExceptionMsg;
-      this.escapeTestDescp = escapeTestDescp;
+      if (Publisher.getAllowUnescapedHTML()) {
+          this.escapeExceptionMsg = escapeExceptionMsg;
+          this.escapeTestDescp = escapeTestDescp;
+      } else {
+          this.escapeExceptionMsg = true;
+          this.escapeTestDescp = true;
+      }
       this.showFailedBuilds = showFailedBuilds;
    }
 
@@ -46,12 +51,20 @@ protected Class<TestNGTestResultBuildAction> getBuildActionClass() {
 
    public boolean getEscapeTestDescp()
    {
-      return escapeTestDescp;
+      if (Publisher.getAllowUnescapedHTML()) {
+          return escapeTestDescp;
+      } else {
+          return true;
+      }
    }
 
    public boolean getEscapeExceptionMsg()
    {
-      return escapeExceptionMsg;
+      if (Publisher.getAllowUnescapedHTML()) {
+          return escapeExceptionMsg;
+      } else {
+          return true;
+      }
    }
 
    /**

@@ -6,12 +6,14 @@
       <f:textbox value="${instance.reportFilenamePattern}" default="**/testng-results.xml"/>
    </f:entry>
    <f:advanced>
+      <j:if test="${hudson.plugins.testng.Publisher.allowUnescapedHTML}"> <!-- SECURITY-2788 - do not show configuration switches if ignored -->
       <f:entry title="Escape Test description string?" field="escapeTestDescp">
          <f:checkbox default="true" />
 	   </f:entry>
 	   <f:entry title="Escape exception messages?" field="escapeExceptionMsg">
          <f:checkbox default="true" />
       </f:entry>
+      </j:if> <!-- end of SECURITY-2788 - do not show configuration switches if ignored -->
       <f:entry title="Show failed builds in trend graph?" field="showFailedBuilds">
          <f:checkbox default="false" />
       </f:entry>

@@ -1,4 +1,6 @@
 <div>
    <b>Escape exception messages?</b>
-   <p>If checked, the plug-in escapes the test method's exception messages. Unchecking this allows you to use HTML tags to format the exception message e.g. embed links in the text. (Enabled by default)</p>
+   <p>If checked, the plug-in escapes the test method's exception messages.</p>
+   <p>If unchecked, this allows you to use HTML tags to format the exception message e.g. embed links in the text. (Enabled by default)</p>
+   <p>However, if this field is unchecked, you are vulnerable to a cross-site scripting attack through an HTML exception message.</p>
 </div>
\ No newline at end of file

@@ -1,5 +1,6 @@
 <div>
    <b>Escape Test Description string?</b>
-   <p>If checked, the plug-in escapes the description string associated with the test method while displaying
-            test method details. Unchecking this allows you to use HTML tags to format the description. (Enabled by default)</p>
+   <p>If checked, the plug-in escapes the description string associated with the test method while displaying test method details.</p>
+   <p>If unchecked, this allows you to use HTML tags to format the description. (Enabled by default)</p>
+   <p>However, if this field is unchecked, you are vulnerable to a cross-site scripting attack through an HTML test description.</p>
 </div>
\ No newline at end of file

@@ -14,6 +14,7 @@
 import java.util.TreeMap;
 import org.jenkinsci.plugins.structs.describable.DescribableModel;
 import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.Rule;
 import org.junit.rules.TemporaryFolder;
@@ -36,6 +37,14 @@ public class PublisherTest {
     @Rule
     public TemporaryFolder tmp = new TemporaryFolder();
 
+    /**
+     * Reset SECURITY-2788 escape hatch before each test.
+     */
+    @Before
+    public void disallowUnescapedHTML() {
+        Publisher.setAllowUnescapedHTML(false);
+    }
+
     @WithoutJenkins
     @Test
     public void testLocateReports() throws Exception {
@@ -86,7 +95,7 @@ public void testBuildAborted() throws Exception {
         publisher.setEscapeExceptionMsg(false);
         publisher.setShowFailedBuilds(false);
         Launcher launcherMock = mock(Launcher.class);
-        AbstractBuild<?,?> buildMock = mock(AbstractBuild.class);
+        AbstractBuild<?, ?> buildMock = mock(AbstractBuild.class);
         BuildListener listenerMock = mock(BuildListener.class);
 
         ByteArrayOutputStream os = new ByteArrayOutputStream();
@@ -117,10 +126,17 @@ public void testRoundTrip() throws Exception {
         before.setThresholdMode(1);
         p.getPublishersList().add(before);
 
-        r.submit(r.createWebClient().getPage(p,"configure").getFormByName("config"));
+        /* Even though set to false by earlier calls to setters, setting is ignored */
+        Assert.assertTrue(before.getEscapeTestDescp()); // SECURITY-2788 - prevent XSS from test description
+        Assert.assertTrue(before.getEscapeExceptionMsg()); // SECURITY-2788 - prevent XSS from test exception
+
+        r.submit(r.createWebClient().getPage(p, "configure").getFormByName("config"));
 
         Publisher after = p.getPublishersList().get(Publisher.class);
 
+        Assert.assertTrue(after.getEscapeTestDescp()); // SECURITY-2788 - prevent XSS from test description
+        Assert.assertTrue(after.getEscapeExceptionMsg()); // SECURITY-2788 - prevent XSS from test exception
+
         r.assertEqualBeans(before, after, "reportFilenamePattern,escapeTestDescp,escapeExceptionMsg,showFailedBuilds");
     }
 
@@ -129,17 +145,43 @@ public void testRoundTrip() throws Exception {
     @Test
     public void testDefaultFields() throws Exception {
         DescribableModel<Publisher> model = new DescribableModel<Publisher>(Publisher.class);
-        Map<String,Object> args = new TreeMap<String,Object>();
+        Map<String, Object> args = new TreeMap<String, Object>();
         Publisher p = model.instantiate(args);
         Assert.assertEquals("**/testng-results.xml", p.getReportFilenamePattern());
         Assert.assertTrue(p.getEscapeExceptionMsg());
+        Assert.assertTrue(p.getEscapeTestDescp());
         Assert.assertFalse(p.getShowFailedBuilds());
         Assert.assertEquals(args, model.uninstantiate(model.instantiate(args)));
         args.put("reportFilenamePattern", "results.xml");
         Assert.assertEquals(args, model.uninstantiate(model.instantiate(args)));
-        args.put("escapeExceptionMsg", false);
         args.put("showFailedBuilds", true);
         Assert.assertEquals(args, model.uninstantiate(model.instantiate(args)));
     }
 
-}
\ No newline at end of file
+    @Issue("SECURITY-2788")
+    @WithoutJenkins
+    @Test
+    public void testUnescapedFields() throws Exception {
+        Publisher.setAllowUnescapedHTML(true);
+        DescribableModel<Publisher> model = new DescribableModel<>(Publisher.class);
+        Map<String, Object> args = new TreeMap<>();
+        Publisher p = model.instantiate(args);
+
+        Assert.assertTrue(p.getEscapeExceptionMsg());
+        p.setEscapeExceptionMsg(false);
+        Assert.assertFalse(p.getEscapeExceptionMsg());
+        p.setEscapeExceptionMsg(true);
+        Assert.assertTrue(p.getEscapeExceptionMsg());
+
+        Assert.assertTrue(p.getEscapeTestDescp());
+        p.setEscapeTestDescp(false);
+        Assert.assertFalse(p.getEscapeTestDescp());
+        p.setEscapeTestDescp(true);
+        Assert.assertTrue(p.getEscapeTestDescp());
+    }
+
+    /* Used by other tests to modify allowUnescapedHTML flag */
+    public static void setAllowUnescapedHTML(boolean value) {
+        Publisher.setAllowUnescapedHTML(value);
+    }
+}

@@ -15,9 +15,12 @@
 import hudson.plugins.testng.Constants;
 import hudson.plugins.testng.PluginImpl;
 import hudson.plugins.testng.Publisher;
+import hudson.plugins.testng.PublisherTest; // Special case for SECURITY-2788 method
 import hudson.tasks.test.AbstractTestResultAction;
 import hudson.tasks.test.TestResult;
 import static org.junit.Assert.*;
+import org.junit.After;
+import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.JenkinsRule;
@@ -33,6 +36,18 @@ public class MethodResultTest {
     @Rule
     public JenkinsRule r = new JenkinsRule();
 
+    @Before
+    public void allowUnescapedHTML() {
+        /* Close the SECURITY-2788 escape hatch before starting a test */
+        PublisherTest.setAllowUnescapedHTML(false);
+    }
+
+    @After
+    public void disallowUnescapedHTML() {
+        /* Close the SECURITY-2788 escape hatch after ending a test */
+        PublisherTest.setAllowUnescapedHTML(false);
+    }
+
     @Test
     public void testEscapeExceptionMessageTrue() throws Exception {
         FreeStyleProject p = r.createFreeStyleProject();
@@ -68,6 +83,7 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher,
 
     @Test
     public void testEscapeExceptionMessageFalse() throws Exception {
+        PublisherTest.setAllowUnescapedHTML(true); // Open the SECURITY-2788 escape hatch for this test
         FreeStyleProject p = r.createFreeStyleProject();
         Publisher publisher = new Publisher();
         publisher.setReportFilenamePattern("testng.xml");
@@ -99,6 +115,7 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher,
 
     @Test
     public void testEscapeDescriptionFalse() throws Exception {
+        PublisherTest.setAllowUnescapedHTML(true); // Open the SECURITY-2788 escape hatch for this test
         FreeStyleProject p = r.createFreeStyleProject();
         Publisher publisher = new Publisher();
         publisher.setReportFilenamePattern("testng.xml");
@@ -171,6 +188,7 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher,
      */
     @Test
     public void testMultilineDescriptionAndExceptionMessage() throws Exception {
+        PublisherTest.setAllowUnescapedHTML(true); // Open the SECURITY-2788 escape hatch for this test
         FreeStyleProject p = r.createFreeStyleProject();
         Publisher publisher = new Publisher();
         publisher.setReportFilenamePattern("testng.xml");

@@ -12,12 +12,17 @@
 import hudson.tasks.test.TestResult;
 import hudson.util.ChartUtil;
 import hudson.util.DataSetBuilder;
+import org.junit.After;
 import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.TestBuilder;
 
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
 /**
  * Tests for {@link TestNGProjectAction}
  *
@@ -28,6 +33,18 @@ public class TestNGProjectActionTest {
     @Rule
     public JenkinsRule r = new JenkinsRule();
 
+    @Before
+    public void allowUnescapedHTML() {
+        /* Open the SECURITY-2788 escape hatch */
+        Publisher.setAllowUnescapedHTML(true);
+    }
+
+    @After
+    public void disallowUnescapedHTML() {
+        /* Close the SECURITY-2788 escape hatch */
+        Publisher.setAllowUnescapedHTML(false);
+    }
+
     /**
      * Test:
      *
@@ -45,7 +62,7 @@ public void testSettings() throws Exception {
         FreeStyleProject p = r.createFreeStyleProject();
         Publisher publisher = new Publisher();
         publisher.setReportFilenamePattern("some.xml");
-        publisher.setEscapeTestDescp(false);
+        publisher.setEscapeTestDescp(false); // Relies on SECURITY-2788 escape hatch being open
         publisher.setEscapeExceptionMsg(true);
         p.getPublishersList().add(publisher);
         p.onCreatedFromScratch(); //to setup project action
@@ -73,7 +90,7 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher,
         //assert on project action
         TestNGProjectAction projAction;
         Assert.assertNotNull(projAction = build.getProject().getAction(TestNGProjectAction.class));
-        Assert.assertFalse(projAction.getEscapeTestDescp());
+        Assert.assertFalse(projAction.getEscapeTestDescp()); // Relies on SECURITY-2788 escape hatch being open
         Assert.assertTrue(projAction.getEscapeExceptionMsg());
         Assert.assertSame(testResult, projAction.getLastCompletedBuildAction().getResult());
 
@@ -87,7 +104,6 @@ public void testHistoryRemoval() throws Exception {
         FreeStyleProject p = r.createFreeStyleProject();
         Publisher publisher = new Publisher();
         publisher.setReportFilenamePattern("some.xml");
-        publisher.setEscapeTestDescp(false);
         publisher.setEscapeExceptionMsg(true);
         p.getPublishersList().add(publisher);
         p.onCreatedFromScratch(); //to setup project action
@@ -127,4 +143,64 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher,
 
         Assert.assertEquals((buildNumber - buildsToRemove.length), dataSetBuilder.build().getColumnCount());
     }
+
+    private FreeStyleProject runNewProjectWithTestNGResults(boolean escapeTestDescp, boolean escapeExceptionMsg) throws Exception {
+        FreeStyleProject project = r.createFreeStyleProject();
+        Publisher publisher = new Publisher();
+        publisher.setReportFilenamePattern("some.xml");
+        publisher.setEscapeExceptionMsg(escapeExceptionMsg);
+        publisher.setEscapeTestDescp(escapeTestDescp);
+        project.getPublishersList().add(publisher);
+        project.onCreatedFromScratch(); //to setup project action
+
+        project.getBuildersList().add(new TestBuilder() {
+            @Override
+            public boolean perform(AbstractBuild<?, ?> build, Launcher launcher,
+                    BuildListener listener) throws InterruptedException, IOException {
+                //any testng xml will do
+                String contents = CommonUtil.getContents(Constants.TESTNG_XML_EXP_MSG_XML);
+                build.getWorkspace().child("some.xml").write(contents, "UTF-8");
+                return true;
+            }
+        });
+
+        project.scheduleBuild2(0).get();
+        return project;
+    }
+
+    @Test
+    public void testGetEscapeTestDescp() throws Exception {
+        Publisher.setAllowUnescapedHTML(false); // Close the escape hatch for this test
+        FreeStyleProject project = runNewProjectWithTestNGResults(false, false);
+        /* false ignored because hatch is closed */
+        TestNGProjectAction action = project.getAction(TestNGProjectAction.class);
+        assertTrue(action.getEscapeTestDescp());
+    }
+
+    @Test
+    public void testGetEscapeTestDescpAllowXSS() throws Exception {
+        Publisher.setAllowUnescapedHTML(true); // Open the escape hatch for this test
+        FreeStyleProject project = runNewProjectWithTestNGResults(false, false);
+        /* false honored because hatch is open */
+        TestNGProjectAction action = project.getAction(TestNGProjectAction.class);
+        assertFalse(action.getEscapeTestDescp());
+    }
+
+    @Test
+    public void testGetEscapeExceptionMsg() throws Exception {
+        Publisher.setAllowUnescapedHTML(false); // Close the escape hatch for this test
+        FreeStyleProject project = runNewProjectWithTestNGResults(false, false);
+        /* false ignored because hatch is closed */
+        TestNGProjectAction action = project.getAction(TestNGProjectAction.class);
+        assertTrue(action.getEscapeExceptionMsg());
+    }
+
+    @Test
+    public void testGetEscapeExceptionMsgAllowXSS() throws Exception {
+        Publisher.setAllowUnescapedHTML(true); // Open the escape hatch for this test
+        FreeStyleProject project = runNewProjectWithTestNGResults(false, false);
+        /* false honored because hatch is open */
+        TestNGProjectAction action = project.getAction(TestNGProjectAction.class);
+        assertFalse(action.getEscapeExceptionMsg());
+    }
 }

@@ -390,8 +390,7 @@ public void test_threshold_for_fails_default_pipeline() throws Exception {
       }
       WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p");
       String contents = CommonUtil.getContents(Constants.TESTNG_FAILED_TEST);
-      r.jenkins.getWorkspaceFor(p).child("testng-results.xml").write(contents, "UTF-8");
-      p.setDefinition(new CpsFlowDefinition("node {step([$class: 'Publisher'])}", true));
+      p.setDefinition(new CpsFlowDefinition("node {\n  writeFile(file: 'testng-results.xml', text: '''" + contents + "''')\n  step([$class: 'Publisher'])\n}\n", true));
       WorkflowRun build = p.scheduleBuild2(0).get();
       r.assertBuildStatus(Result.UNSTABLE, build);
       TestNGTestResultBuildAction action = build.getAction(TestNGTestResultBuildAction.class);