From efbaa378bd061b067a4bf9ba58bf958366ecdbb1 Mon Sep 17 00:00:00 2001
From: Vagisha Sharma <vagisha@gmail.com>
Date: Sun, 28 Jun 2026 19:58:56 -0700
Subject: [PATCH 1/5] Fixed stored XSS in three testresults result views

* failureDetail.jsp: HTML-encode the failure language and stack trace with
  LABKEY.Utils.encodeHtml before building the row markup, rather than
  concatenating them straight into innerHTML.
* runDetail.jsp: render the run log/xml popup via a <pre> textContent node
  instead of document.write of concatenated HTML.
* trainingdata.jsp: render the email preview in a sandboxed iframe (scripts
  disabled, opaque origin) instead of document.write of the raw email HTML.

All three display stored run/failure data that originates from the
unauthenticated SkylineNightly results upload (PostAction), so a crafted post
could otherwise store script that runs in an admin's browser.

Co-Authored-By: Claude <noreply@anthropic.com>
---
 .../src/org/labkey/testresults/view/failureDetail.jsp  |  6 ++++--
 .../src/org/labkey/testresults/view/runDetail.jsp      |  6 +++++-
 .../src/org/labkey/testresults/view/trainingdata.jsp   | 10 +++++++++-
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/testresults/src/org/labkey/testresults/view/failureDetail.jsp b/testresults/src/org/labkey/testresults/view/failureDetail.jsp
index b334fd3d..846a7d60 100644
--- a/testresults/src/org/labkey/testresults/view/failureDetail.jsp
+++ b/testresults/src/org/labkey/testresults/view/failureDetail.jsp
@@ -332,9 +332,11 @@ $(document).ready(function() {
                 filterFunc = run => run.failures.length > 0;
                 headerName = "Failures";
                 displayFunc = run => {
+                    // f.language and f.trace come from stored failure data (originally from the
+                    // unauthenticated results upload), so HTML-encode them before building markup.
                     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";
diff --git a/testresults/src/org/labkey/testresults/view/runDetail.jsp b/testresults/src/org/labkey/testresults/view/runDetail.jsp
index c4895a38..1966c766 100644
--- a/testresults/src/org/labkey/testresults/view/runDetail.jsp
+++ b/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 (originally from the unauthenticated results upload).
+        // 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,
diff --git a/testresults/src/org/labkey/testresults/view/trainingdata.jsp b/testresults/src/org/labkey/testresults/view/trainingdata.jsp
index 69d4d5cd..abfbb513 100644
--- a/testresults/src/org/labkey/testresults/view/trainingdata.jsp
+++ b/testresults/src/org/labkey/testresults/view/trainingdata.jsp
@@ -288,7 +288,15 @@
             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 (originally from the
+            // unauthenticated results upload). Render it in a sandboxed iframe (no allow-scripts, opaque
+            // origin) so any markup it contains is displayed but cannot execute as script in our origin.
+            var iframe = win.document.createElement('iframe');
+            iframe.setAttribute('sandbox', '');
+            iframe.style.cssText = 'border:0;width:100%;height:100%;';
+            iframe.srcdoc = data.HTML;
+            win.document.body.style.margin = '0';
+            win.document.body.appendChild(iframe);
         }, "json")
     });
 

From 901924970dcb414b6d0eae8b74c96700ee20ea9c Mon Sep 17 00:00:00 2001
From: Vagisha Sharma <vagisha@gmail.com>
Date: Sun, 28 Jun 2026 20:24:55 -0700
Subject: [PATCH 2/5] Wrapped the testresults ChangeBoundaries transaction in
 try-with-resources

* ChangeBoundaries.execute: the GlobalSettings update transaction was opened
  with a bare assignment and committed with no try/finally, so it could leak if
  an exception was thrown before commit. Wrapped it in try-with-resources, like
  every other transaction in this controller.
* PostAction / PostErrorFilesAction: documented that the stack trace returned in
  the response is intentional, so SkylineTester can diagnose posting failures
  from the client without server log access. Noted by the security review;
  kept by design.

Co-Authored-By: Claude <noreply@anthropic.com>
---
 .../testresults/TestResultsController.java    | 36 +++++++++++--------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/testresults/src/org/labkey/testresults/TestResultsController.java b/testresults/src/org/labkey/testresults/TestResultsController.java
index 5b99149a..5d979d98 100644
--- a/testresults/src/org/labkey/testresults/TestResultsController.java
+++ b/testresults/src/org/labkey/testresults/TestResultsController.java
@@ -1229,22 +1229,24 @@ public Object execute(BoundariesForm form, BindException errors) throws Exceptio
             }
 
             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 (" + warningB + ", " + 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()));
                 }
             }

From baf1ed7eeb48c613825597f1b8a005eb3be2ec95 Mon Sep 17 00:00:00 2001
From: Vagisha Sharma <vagisha@gmail.com>
Date: Sun, 28 Jun 2026 20:43:56 -0700
Subject: [PATCH 3/5] * Replaced all five e.printStackTrace() calls with Log4J2
 logging that includes   the exception, per the logging convention.
 TestResultsController used its   existing _log; RunDetail, TestResultsModule,
 and TestResultsWebPart got a   static LOG via LogHelper.getLogger. *
 ChangeBoundaries.execute: parameterized the GlobalSettings INSERT with bind  
 parameters (VALUES (?, ?)) instead of concatenating the warning/error integer
   values into the SQL string.

Co-Authored-By: Claude <noreply@anthropic.com>
---
 .../org/labkey/testresults/TestResultsController.java    | 9 +++++----
 .../src/org/labkey/testresults/TestResultsModule.java    | 6 +++++-
 .../src/org/labkey/testresults/TestResultsWebPart.java   | 6 +++++-
 .../src/org/labkey/testresults/model/RunDetail.java      | 6 +++++-
 4 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/testresults/src/org/labkey/testresults/TestResultsController.java b/testresults/src/org/labkey/testresults/TestResultsController.java
index 5d979d98..0de43f26 100644
--- a/testresults/src/org/labkey/testresults/TestResultsController.java
+++ b/testresults/src/org/labkey/testresults/TestResultsController.java
@@ -450,7 +450,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)
@@ -1243,7 +1243,9 @@ public Object execute(BoundariesForm form, BindException errors) throws Exceptio
                     new SqlExecutor(TestResultsSchema.getSchema()).execute(sqlFragmentDelete);
                 }
                 SQLFragment sqlFragmentInsert = new SQLFragment();
-                sqlFragmentInsert.append("INSERT INTO " + TestResultsSchema.getTableInfoGlobalSettings() + " (warningb, errorb) VALUES (" + warningB + ", " + errorB +")");
+                sqlFragmentInsert.append("INSERT INTO " + TestResultsSchema.getTableInfoGlobalSettings() + " (warningb, errorb) VALUES (?, ?)");
+                sqlFragmentInsert.add(warningB);
+                sqlFragmentInsert.add(errorB);
                 new SqlExecutor(TestResultsSchema.getSchema()).execute(sqlFragmentInsert);
                 transaction.commit();
             }
@@ -1903,8 +1905,7 @@ private static String SaveXML(MultipartFile file, Container c) {
             }
             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.";
diff --git a/testresults/src/org/labkey/testresults/TestResultsModule.java b/testresults/src/org/labkey/testresults/TestResultsModule.java
index bdd474ec..f53687a9 100644
--- a/testresults/src/org/labkey/testresults/TestResultsModule.java
+++ b/testresults/src/org/labkey/testresults/TestResultsModule.java
@@ -16,9 +16,11 @@
 
 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;
+import org.labkey.api.util.logging.LogHelper;
 import org.labkey.api.data.ContainerManager;
 import org.labkey.api.module.DefaultModule;
 import org.labkey.api.module.ModuleContext;
@@ -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);
         }
 
     }
diff --git a/testresults/src/org/labkey/testresults/TestResultsWebPart.java b/testresults/src/org/labkey/testresults/TestResultsWebPart.java
index 2a8a9a53..95878a89 100644
--- a/testresults/src/org/labkey/testresults/TestResultsWebPart.java
+++ b/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");
diff --git a/testresults/src/org/labkey/testresults/model/RunDetail.java b/testresults/src/org/labkey/testresults/model/RunDetail.java
index 8202cdac..83c5cbae 100644
--- a/testresults/src/org/labkey/testresults/model/RunDetail.java
+++ b/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();
     }

From dc964494b3b14fac756a7e423fdc688db5425683 Mon Sep 17 00:00:00 2001
From: Vagisha Sharma <vagisha@gmail.com>
Date: Mon, 29 Jun 2026 14:48:33 -0700
Subject: [PATCH 4/5] * SendTestResultsEmail.getHTMLEmail: HTML-encode the git
 hash, missing-user name,   problem-table computer name, and test name. *
 trainingdata.jsp: the email-preview iframe used height:100%, which collapses
 to   zero against the popup body (no defined height) and showed a blank
 popup;   switched to 100vh so the preview renders. * TestResultsTest: added
 testViewLogPopup, which opens the run-log popup and   asserts the stored log
 is displayed.

Co-Authored-By: Claude <noreply@anthropic.com>
---
 .../testresults/SendTestResultsEmail.java     |  8 +++----
 .../labkey/testresults/view/failureDetail.jsp |  2 --
 .../org/labkey/testresults/view/runDetail.jsp |  4 ++--
 .../labkey/testresults/view/trainingdata.jsp  |  9 ++++----
 .../tests/testresults/TestResultsTest.java    | 22 +++++++++++++++++++
 5 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/testresults/src/org/labkey/testresults/SendTestResultsEmail.java b/testresults/src/org/labkey/testresults/SendTestResultsEmail.java
index f406ed23..2fac3463 100644
--- a/testresults/src/org/labkey/testresults/SendTestResultsEmail.java
+++ b/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;'>");
diff --git a/testresults/src/org/labkey/testresults/view/failureDetail.jsp b/testresults/src/org/labkey/testresults/view/failureDetail.jsp
index 846a7d60..508f4b40 100644
--- a/testresults/src/org/labkey/testresults/view/failureDetail.jsp
+++ b/testresults/src/org/labkey/testresults/view/failureDetail.jsp
@@ -332,8 +332,6 @@ $(document).ready(function() {
                 filterFunc = run => run.failures.length > 0;
                 headerName = "Failures";
                 displayFunc = run => {
-                    // f.language and f.trace come from stored failure data (originally from the
-                    // unauthenticated results upload), so HTML-encode them before building markup.
                     return "<ul>" + run.failures.map(f => "<li>" +
                             "Pass " + f.pass.toString() + " (" + LABKEY.Utils.encodeHtml(f.language) + ") ---" +
                             '<pre class="stack-trace">' + LABKEY.Utils.encodeHtml(f.trace) + "</pre>"+
diff --git a/testresults/src/org/labkey/testresults/view/runDetail.jsp b/testresults/src/org/labkey/testresults/view/runDetail.jsp
index 1966c766..b1c39187 100644
--- a/testresults/src/org/labkey/testresults/view/runDetail.jsp
+++ b/testresults/src/org/labkey/testresults/view/runDetail.jsp
@@ -46,8 +46,8 @@
         var win = window.open("", "Log file",
                 "toolbar=no,location=no,directories=no,status=no,menubar=no,scrollbars=yes,resizable=yes," +
                 "width=800,height=600");
-        // The log/xml is stored run content (originally from the unauthenticated results upload).
-        // Set it as text rather than writing it as HTML so it cannot execute as markup in the popup.
+        // 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);
diff --git a/testresults/src/org/labkey/testresults/view/trainingdata.jsp b/testresults/src/org/labkey/testresults/view/trainingdata.jsp
index abfbb513..86390b17 100644
--- a/testresults/src/org/labkey/testresults/view/trainingdata.jsp
+++ b/testresults/src/org/labkey/testresults/view/trainingdata.jsp
@@ -288,12 +288,13 @@
             var win = window.open("", data.subject,
                 "toolbar=no,location=no,directories=no,status=no,menubar=no,scrollbars=yes,resizable=yes," +
                 "width=800,height=600");
-            // data.HTML is a rendered email containing stored run/user/test names (originally from the
-            // unauthenticated results upload). Render it in a sandboxed iframe (no allow-scripts, opaque
-            // origin) so any markup it contains is displayed but cannot execute as script in our origin.
+            // 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', '');
-            iframe.style.cssText = 'border:0;width:100%;height:100%;';
+            // 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);
diff --git a/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java b/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java
index 1b1c37b9..720710a2 100644
--- a/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java
+++ b/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()
     {

From c044eac5a2ca59621f9f7083dafe52ca74b000ce Mon Sep 17 00:00:00 2001
From: Vagisha Sharma <vagisha@gmail.com>
Date: Mon, 29 Jun 2026 15:17:16 -0700
Subject: [PATCH 5/5] * ShowFlaggedAction: require ReadPermission like the
 other view actions. * TestResultsController SaveXML: parameterized the two
 filename log statements * ChangeBoundaries.execute: removed the unused
 GlobalSettings local * TestResultsModule: moved the LogHelper import to its
 correct position.

Co-Authored-By: Claude <noreply@anthropic.com>
---
 .../src/org/labkey/testresults/TestResultsController.java | 8 +++-----
 .../src/org/labkey/testresults/TestResultsModule.java     | 2 +-
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/testresults/src/org/labkey/testresults/TestResultsController.java b/testresults/src/org/labkey/testresults/TestResultsController.java
index 0de43f26..f35bfd32 100644
--- a/testresults/src/org/labkey/testresults/TestResultsController.java
+++ b/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;
@@ -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,7 +1227,6 @@ public Object execute(BoundariesForm form, BindException errors) throws Exceptio
                 return new ApiSimpleResponse(res);
             }
 
-            GlobalSettings settings = new GlobalSettings(warningB, errorB);
             try (DbScope.Transaction transaction = TestResultsSchema.getSchema().getScope().ensureTransaction())
             {
                 SQLFragment sqlFragment = new SQLFragment();
@@ -1898,14 +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);
+                _log.error("Failed to save {}.", fileName, e);
                 return "Failed to save the file.";
             }
             return "File saved to system.";
diff --git a/testresults/src/org/labkey/testresults/TestResultsModule.java b/testresults/src/org/labkey/testresults/TestResultsModule.java
index f53687a9..50af20ad 100644
--- a/testresults/src/org/labkey/testresults/TestResultsModule.java
+++ b/testresults/src/org/labkey/testresults/TestResultsModule.java
@@ -20,12 +20,12 @@
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.labkey.api.data.Container;
-import org.labkey.api.util.logging.LogHelper;
 import org.labkey.api.data.ContainerManager;
 import org.labkey.api.module.DefaultModule;
 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;