Address reported security issues in the testresults module

testresults/src/org/labkey/testresults/SendTestResultsEmail.java

@@ -223,7 +223,7 @@ public Pair<String, String> getHTMLEmail(org.labkey.api.security.User from)
                             .append("\n<td style='padding: 6px; " + (highlightDuration ? style : "") + "'>" + run.getDuration() + (run.getHang() != null ? " (hang)" : "") + "</td>")
                             .append("\n<td style='padding: 6px; " + (run.getFailedtests() > 0 ? getBackgroundStyle(BackgroundColor.error) : "") + "'>" + run.getFailedtests() + "</td>")
                             .append("\n<td style='padding: 6px; " + (run.getLeaks().length > 0 ? getBackgroundStyle(BackgroundColor.error) : "") + "'>" + run.getLeaks().length + "</td>")
-                            .append("\n<td style='padding: 6px;'> " + run.getGitHash() + "</td>")
+                            .append("\n<td style='padding: 6px;'> " + PageFlowUtil.filter(run.getGitHash()) + "</td>")
                             .append("</tr>");
                     }
                 }
@@ -237,7 +237,7 @@ public Pair<String, String> getHTMLEmail(org.labkey.api.security.User from)
             for (User u : missingUsers)
             {
                 message.append("<tr style='border-bottom: 1px solid grey;'>")
-                    .append("\n<td style='" + getBackgroundStyle(BackgroundColor.error) + " padding: 6px;' colspan='7'>Missing " + u.getUsername() + "</td>")
+                    .append("\n<td style='" + getBackgroundStyle(BackgroundColor.error) + " padding: 6px;' colspan='7'>Missing " + PageFlowUtil.filter(u.getUsername()) + "</td>")
                     .append("</tr>");
             }
             message.append("<tr>")
@@ -267,13 +267,13 @@ public Pair<String, String> getHTMLEmail(org.labkey.api.security.User from)
                             "</td>");
                 RunDetail[] problemRuns = problems.getRuns();
                 for (RunDetail run : problemRuns)
-                    message.append("\n<td style='max-width: 60px; width: 60px; overflow: hidden; text-overflow: ellipsis; padding: 3px; border: 1px solid #ccc;'>" + run.getUserName() + "</td>");
+                    message.append("\n<td style='max-width: 60px; width: 60px; overflow: hidden; text-overflow: ellipsis; padding: 3px; border: 1px solid #ccc;'>" + PageFlowUtil.filter(run.getUserName()) + "</td>");
                 message.append("\n</tr>");
 
                 for (String test : problems.getTestNames())
                 {
                     message.append("\n<tr>")
-                        .append("\n<td style='overflow: hidden; text-overflow: ellipsis; padding: 3px; border: 1px solid #ccc;'>" + test + "</td>");
+                        .append("\n<td style='overflow: hidden; text-overflow: ellipsis; padding: 3px; border: 1px solid #ccc;'>" + PageFlowUtil.filter(test) + "</td>");
                     for (RunDetail run : problemRuns)
                     {
                         message.append("\n<td style='width: 60px; overflow: hidden; padding: 3px; border: 1px solid #ccc;'>");

testresults/src/org/labkey/testresults/TestResultsController.java

@@ -68,7 +68,6 @@
 import org.labkey.api.view.JspView;
 import org.labkey.api.view.NavTree;
 import org.labkey.api.view.WebPartView;
-import org.labkey.testresults.model.GlobalSettings;
 import org.labkey.testresults.model.RunDetail;
 import org.labkey.testresults.model.TestFailDetail;
 import org.labkey.testresults.model.TestHandleLeakDetail;
@@ -450,7 +449,7 @@ public static void ensureRunDataCached(RunDetail[] runs, boolean keepObjData) {
                     }
                     catch (IOException e)
                     {
-                        e.printStackTrace();
+                        _log.error("Failed to encode run pass summary", e);
                     }
                     int avgMem = 0;
                     if (passes.length != 0)
@@ -1173,7 +1172,7 @@ public void setFlag(Boolean flag)
     /**
      * action to show all flagged runs flagged.jsp
      */
-    @RequiresNoPermission
+    @RequiresPermission(ReadPermission.class)
     public static class ShowFlaggedAction extends SimpleViewAction<Object>
     {
         @Override
@@ -1228,23 +1227,26 @@ public Object execute(BoundariesForm form, BindException errors) throws Exceptio
                 return new ApiSimpleResponse(res);
             }
 
-            GlobalSettings settings = new GlobalSettings(warningB, errorB);
-            DbScope.Transaction transaction = TestResultsSchema.getSchema().getScope().ensureTransaction();
-            SQLFragment sqlFragment = new SQLFragment();
-            sqlFragment.append("select exists(select 1 from " + TestResultsSchema.getTableInfoGlobalSettings() + ") ");
-            SqlSelector sqlSelector = new SqlSelector(TestResultsSchema.getSchema(), sqlFragment);
-            List<Boolean> values = new ArrayList<>();
-            sqlSelector.forEach(rs -> values.add(rs.getBoolean(1)));
-
-            if (values.get(0)) {
-                SQLFragment sqlFragmentDelete = new SQLFragment();
-                sqlFragmentDelete.append("DELETE FROM " + TestResultsSchema.getTableInfoGlobalSettings());
-                new SqlExecutor(TestResultsSchema.getSchema()).execute(sqlFragmentDelete);
+            try (DbScope.Transaction transaction = TestResultsSchema.getSchema().getScope().ensureTransaction())
+            {
+                SQLFragment sqlFragment = new SQLFragment();
+                sqlFragment.append("select exists(select 1 from " + TestResultsSchema.getTableInfoGlobalSettings() + ") ");
+                SqlSelector sqlSelector = new SqlSelector(TestResultsSchema.getSchema(), sqlFragment);
+                List<Boolean> values = new ArrayList<>();
+                sqlSelector.forEach(rs -> values.add(rs.getBoolean(1)));
+
+                if (values.get(0)) {
+                    SQLFragment sqlFragmentDelete = new SQLFragment();
+                    sqlFragmentDelete.append("DELETE FROM " + TestResultsSchema.getTableInfoGlobalSettings());
+                    new SqlExecutor(TestResultsSchema.getSchema()).execute(sqlFragmentDelete);
+                }
+                SQLFragment sqlFragmentInsert = new SQLFragment();
+                sqlFragmentInsert.append("INSERT INTO " + TestResultsSchema.getTableInfoGlobalSettings() + " (warningb, errorb) VALUES (?, ?)");
+                sqlFragmentInsert.add(warningB);
+                sqlFragmentInsert.add(errorB);
+                new SqlExecutor(TestResultsSchema.getSchema()).execute(sqlFragmentInsert);
+                transaction.commit();
             }
-            SQLFragment sqlFragmentInsert = new SQLFragment();
-            sqlFragmentInsert.append("INSERT INTO " + TestResultsSchema.getTableInfoGlobalSettings() + " (warningb, errorb) VALUES (" + warningB + ", " + errorB +")");
-            new SqlExecutor(TestResultsSchema.getSchema()).execute(sqlFragmentInsert);
-            transaction.commit();
             res.put("Message", "success!");
             return new ApiSimpleResponse(res);
         }
@@ -1779,6 +1781,8 @@ else if (xml.isEmpty())
                 _log.info("Attempting to save file for a future post attempt");
                 res.put(KEY_SUCCESS, false);
                 res.put("Message", "Error Parsing XML attempting to save the XML file...   " + NIGHTLY_POSTER.SaveXML(file, getContainer()));
+                // The stack trace is intentionally returned to the caller (SkylineTester) so posting
+                // failures can be diagnosed from the client without server log access.
                 res.put("Exception", e + NIGHTLY_POSTER.getStackTraceText(e));
                 return new ApiSimpleResponse(res);
             }
@@ -1848,6 +1852,8 @@ public Object execute(Object o, BindException errors)
                     f.delete();
                     res.put(f.getName(), "Success!");
                 } catch (Exception e) {
+                    // The stack trace is intentionally returned to the caller so a failed re-post can be
+                    // diagnosed from the client without server log access.
                     res.put(f.getName(), Arrays.toString(e.getStackTrace()));
                 }
             }
@@ -1890,15 +1896,14 @@ private static String SaveXML(MultipartFile file, Container c) {
             {
                 File f = makeFile(c, fileName);
                 if(f.exists()) {
-                    _log.info("A file by the name " + fileName + " is already stored.");
+                    _log.info("A file by the name {} is already stored.", fileName);
                     return "File not saved - file already exists in file system.";
                 }
                 file.transferTo(f);
             }
             catch (IOException e)
             {
-                _log.error("Failed to save " + fileName + ".");
-                e.printStackTrace();
+                _log.error("Failed to save {}.", fileName, e);
                 return "Failed to save the file.";
             }
             return "File saved to system.";

testresults/src/org/labkey/testresults/TestResultsModule.java

@@ -16,6 +16,7 @@
 
 package org.labkey.testresults;
 
+import org.apache.logging.log4j.Logger;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.labkey.api.data.Container;
@@ -24,6 +25,7 @@
 import org.labkey.api.module.ModuleContext;
 import org.labkey.api.security.Directive;
 import org.labkey.api.security.SecurityManager;
+import org.labkey.api.util.logging.LogHelper;
 import org.labkey.filters.ContentSecurityPolicyFilter;
 import org.labkey.api.view.WebPartFactory;
 import org.quartz.JobKey;
@@ -42,6 +44,8 @@
 
 public class TestResultsModule extends DefaultModule
 {
+    private static final Logger LOG = LogHelper.getLogger(TestResultsModule.class, "Test results module");
+
     public static final WebPartFactory _testResultsFactory = new TestResultsWebPart();
     public static final String JOB_NAME = "TestResultsEmailTrigger";
     public static final String JOB_GROUP = "TestResultsGroup";
@@ -127,7 +131,7 @@ public void startBackgroundThreads()
         }
         catch (SchedulerException e)
         {
-            e.printStackTrace();
+            LOG.error("Failed to start the test results email scheduler", e);
         }
 
     }

testresults/src/org/labkey/testresults/TestResultsWebPart.java

@@ -1,7 +1,9 @@
 package org.labkey.testresults;
 
+import org.apache.logging.log4j.Logger;
 import org.jetbrains.annotations.NotNull;
 import org.labkey.api.data.Container;
+import org.labkey.api.util.logging.LogHelper;
 import org.labkey.api.view.BaseWebPartFactory;
 import org.labkey.api.view.JspView;
 import org.labkey.api.view.Portal;
@@ -16,6 +18,8 @@
 
 public class TestResultsWebPart extends BaseWebPartFactory
 {
+    private static final Logger LOG = LogHelper.getLogger(TestResultsWebPart.class, "Test results web part");
+
     public TestResultsWebPart()
     {
         super("Test Results", true, false, WebPartFactory.LOCATION_BODY);
@@ -32,7 +36,7 @@ public WebPartView<?> getWebPartView(@NotNull ViewContext portalCtx, Portal.@Not
         }
         catch (ParseException | IOException e)
         {
-            e.printStackTrace();
+            LOG.error("Failed to build the test results web part data", e);
         }
         JspView<TestsDataBean> view = new JspView<>("/org/labkey/testresults/view/rundown.jsp", bean);
         view.setTitle("Test Results");

testresults/src/org/labkey/testresults/model/RunDetail.java

@@ -15,8 +15,10 @@
  */
 package org.labkey.testresults.model;
 
+import org.apache.logging.log4j.Logger;
 import org.labkey.api.data.Container;
 import org.labkey.api.reader.Readers;
+import org.labkey.api.util.logging.LogHelper;
 
 import java.io.BufferedReader;
 import java.io.ByteArrayInputStream;
@@ -34,6 +36,8 @@
  */
 public class RunDetail implements Comparable<RunDetail>
 {
+    private static final Logger LOG = LogHelper.getLogger(RunDetail.class, "Test results run detail");
+
     public final static int HANG_MILLISECONDS = 30*60*1000; // 30 minutes
 
     private int id;
@@ -275,7 +279,7 @@ public static String decode(byte[] bytes) {
         }
         catch (IOException e1)
         {
-            e1.printStackTrace();
+            LOG.error("Error reading gzipped run log", e1);
         }
         return out.toString();
     }

testresults/src/org/labkey/testresults/view/failureDetail.jsp

@@ -333,8 +333,8 @@ $(document).ready(function() {
                 headerName = "Failures";
                 displayFunc = run => {
                     return "<ul>" + run.failures.map(f => "<li>" +
-                            "Pass " + f.pass.toString() + " (" + f.language + ") ---" +
-                            '<pre class="stack-trace">' + f.trace + "</pre>"+
+                            "Pass " + f.pass.toString() + " (" + LABKEY.Utils.encodeHtml(f.language) + ") ---" +
+                            '<pre class="stack-trace">' + LABKEY.Utils.encodeHtml(f.trace) + "</pre>"+
                             "</li>").join() + "</ul>";
                 };
                 jsonKey = "failures";

testresults/src/org/labkey/testresults/view/runDetail.jsp

@@ -46,7 +46,11 @@
         var win = window.open("", "Log file",
                 "toolbar=no,location=no,directories=no,status=no,menubar=no,scrollbars=yes,resizable=yes," +
                 "width=800,height=600");
-        win.document.write('<pre>' + data + '</pre>');
+        // The log/xml is stored run content. Set it as text rather than writing it as HTML so it cannot execute as
+        // markup in the popup.
+        var pre = win.document.createElement('pre');
+        pre.textContent = data;
+        win.document.body.appendChild(pre);
     };
     var showLog = function() {
         $.get('<%=h(new ActionURL(TestResultsController.ViewLogAction.class, c).addParameter("runId", runId))%>', csrf_header,

testresults/src/org/labkey/testresults/view/trainingdata.jsp

@@ -288,7 +288,16 @@
             var win = window.open("", data.subject,
                 "toolbar=no,location=no,directories=no,status=no,menubar=no,scrollbars=yes,resizable=yes," +
                 "width=800,height=600");
-            win.document.write(data.HTML);
+            // data.HTML is a rendered email containing stored run/user/test names. Render it in a sandboxed iframe
+            // (no allow-scripts, opaque origin) so any markup it contains is displayed but cannot execute as script.
+            var iframe = win.document.createElement('iframe');
+            iframe.setAttribute('sandbox', '');
+            // Use viewport height: a percentage height would resolve against the popup body
+            // (which has no defined height) and collapse the iframe to zero, showing nothing.
+            iframe.style.cssText = 'border:0;width:100%;height:100vh;display:block;';
+            iframe.srcdoc = data.HTML;
+            win.document.body.style.margin = '0';
+            win.document.body.appendChild(iframe);
         }, "json")
     });
 

testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java

@@ -516,6 +516,28 @@ public void testViewXml()
         assertFalse("XML should not contain Log element (stripped before storage)", xmlContent.contains("<Log>"));
     }
 
+    @Test
+    public void testViewLogPopup()
+    {
+        // Verify the popup actually displays the stored log.
+        navigateToRunById(_cleanRunId);
+        click(Locator.lkButton("View Log"));
+        switchToWindow(1);
+        try
+        {
+            waitForElement(Locator.tag("pre"));
+            String popupText = getText(Locator.tag("pre"));
+            assertTrue("Log popup should show the nightly header, was: " + popupText,
+                    popupText.contains("# Nightly started Thursday, January 15, 2026 9:00 PM"));
+            assertTrue("Log popup should show test entries", popupText.contains("TestAlpha"));
+        }
+        finally
+        {
+            closeExtraWindows();
+            switchToMainWindow();
+        }
+    }
+
     @Test
     public void testChangeBoundaries()
     {