From 57b13f19ba4c10a288c9c45374919941a606391c Mon Sep 17 00:00:00 2001
From: vagisha <vagisha@gmail.com>
Date: Wed, 1 Jul 2026 18:34:57 -0700
Subject: [PATCH 1/2] Address reported security issues in the panoramapublic
 module (#655)

#### Changes
**Test actions globally swap the NcbiPublicationSearchService singleton with no audit event (Informational)**
* The three Selenium-test support actions (`SetupMockNcbiServiceAction`, `RestoreNcbiServiceAction`, `RegisterMockPublicationAction`) swap a process-wide singleton. They are @RequiresSiteAdmin but otherwise reachable in production, and extended ReadOnlyApiAction, which does no CSRF check and is reachable by a plain GET.
* Added `requireDevModeForMockNcbiService()`: the actions now return 404 on a non-dev-mode server. Dev machines and TeamCity run in dev mode, so tests are unaffected.
* Changed the three actions from `ReadOnlyApiAction` to `MutatingApiAction`, since they mutate global state, which also requires a POST with a CSRF token.
* The report suggested adding an audit event or compiling these out of production builds. On review we found that gating them to dev mode is a better fix, since it removes the production attack surface entirely rather than just recording the swap.

**Uses System.out instead of Log4J2 (Informational)**
* `ExperimentModificationGetter`: removed System.out helpers printStructuralMod and printIsotopicMod and their call sites.

**Tests**
* `PublicationSearchTest`: invoke the three mock-NCBI actions with `SimplePostCommand `instead of `SimpleGetCommand`.
---
 .../PanoramaPublicController.java             | 20 ++++++-
 .../ExperimentModificationGetter.java         | 57 -------------------
 .../panoramapublic/PublicationSearchTest.java |  8 +--
 3 files changed, 21 insertions(+), 64 deletions(-)

diff --git a/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java b/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java
index e04e7e79..3dc17ea5 100644
--- a/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java
+++ b/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java
@@ -32,6 +32,7 @@
 import org.labkey.api.action.FormHandlerAction;
 import org.labkey.api.action.FormViewAction;
 import org.labkey.api.action.LabKeyError;
+import org.labkey.api.action.MutatingApiAction;
 import org.labkey.api.action.ReadOnlyApiAction;
 import org.labkey.api.action.ReturnUrlForm;
 import org.labkey.api.action.SimpleErrorView;
@@ -11233,34 +11234,47 @@ public static ActionURL getCatalogImageDownloadUrl(ExperimentAnnotations expAnno
 
     // ======================== Support actions for Selenium tests ========================
 
+    // These actions swap the process-wide NCBI publication search service to a mock so that Selenium tests
+    // run without calling the live NCBI API. They must never be reachable on a production server (non-dev-mode)
+    private static void requireDevModeForMockNcbiService()
+    {
+        if (!AppProps.getInstance().isDevMode())
+        {
+            throw new NotFoundException("Mock NCBI publication search service actions are only available on a server running in dev mode.");
+        }
+    }
+
     @RequiresSiteAdmin
-    public static class SetupMockNcbiServiceAction extends ReadOnlyApiAction<Object>
+    public static class SetupMockNcbiServiceAction extends MutatingApiAction<Object>
     {
         @Override
         public Object execute(Object form, BindException errors)
         {
+            requireDevModeForMockNcbiService();
             NcbiPublicationSearchServiceImpl.setInstance(new MockNcbiPublicationSearchService());
             return new ApiSimpleResponse("mock", true);
         }
     }
 
     @RequiresSiteAdmin
-    public static class RestoreNcbiServiceAction extends ReadOnlyApiAction<Object>
+    public static class RestoreNcbiServiceAction extends MutatingApiAction<Object>
     {
         @Override
         public Object execute(Object form, BindException errors)
         {
+            requireDevModeForMockNcbiService();
             NcbiPublicationSearchServiceImpl.setInstance(new NcbiPublicationSearchServiceImpl());
             return new ApiSimpleResponse("restored", true);
         }
     }
 
     @RequiresSiteAdmin
-    public static class RegisterMockPublicationAction extends ReadOnlyApiAction<RegisterMockPublicationForm>
+    public static class RegisterMockPublicationAction extends MutatingApiAction<RegisterMockPublicationForm>
     {
         @Override
         public Object execute(RegisterMockPublicationForm form, BindException errors)
         {
+            requireDevModeForMockNcbiService();
             NcbiPublicationSearchService service = NcbiPublicationSearchServiceImpl.getInstance();
             if (!(service instanceof MockNcbiPublicationSearchService mock))
             {
diff --git a/panoramapublic/src/org/labkey/panoramapublic/proteomexchange/ExperimentModificationGetter.java b/panoramapublic/src/org/labkey/panoramapublic/proteomexchange/ExperimentModificationGetter.java
index 19253a17..4cde5676 100644
--- a/panoramapublic/src/org/labkey/panoramapublic/proteomexchange/ExperimentModificationGetter.java
+++ b/panoramapublic/src/org/labkey/panoramapublic/proteomexchange/ExperimentModificationGetter.java
@@ -345,8 +345,6 @@ public PxIsotopicMod(String skylineName, long dbModId, UnimodModification uMod,
 
     public static class TestCase extends Assert
     {
-        private static final boolean debug = false;
-
         @Test
         public void testBuildIsotopeModFormula()
         {
@@ -701,8 +699,6 @@ public void testStructuralMods() throws IOException
             {
                 PxModification pxMod = getStructuralUnimodMod(mod, uMods);
 
-                printStructuralMod(mod, pxMod);
-
                 List<UnimodModification> matches = unimodMatches.get(pxMod.getSkylineName());
                 if (matches.isEmpty())
                 {
@@ -727,32 +723,6 @@ else if (matches.size() == 1)
             }
         }
 
-        private void printStructuralMod(Modification mod, PxModification pxMod)
-        {
-            if (!debug)
-            {
-                return;
-            }
-
-            if (pxMod.hasUnimodId())
-            {
-                String term = mod.getTerminus() == null ? "" : (mod.getTerminus().equals("N") ? "N-term" : "C-term");
-                String modInfo = pxMod.getSkylineName() + ", " + Formula.normalizeFormula(mod.getFormula()) + ", " + mod.getAminoAcid() + ", TERM: " + term;
-                System.out.print("Skyline: " + modInfo);
-                System.out.println(" --- " + pxMod.getUnimodId() + ", " + pxMod.getName());
-            }
-            if (pxMod.hasPossibleUnimods())
-            {
-                String term = mod.getTerminus() == null ? "" : (mod.getTerminus().equals("N") ? "N-term" : "C-term");
-                String modInfo = pxMod.getSkylineName() + ", " + Formula.normalizeFormula(mod.getFormula()) + ", " + mod.getAminoAcid() + ", TERM: " + term;
-                System.out.println("Skyline: " + modInfo);
-                for (UnimodModification umod: pxMod.getPossibleUnimodMatches())
-                {
-                    System.out.println(" --- List.of(new UnimodModification(" + umod.getId() + ", \"" + umod.getName() + "\", null))");
-                }
-            }
-        }
-
         @Test
         public void testIsotopicMods() throws IOException
         {
@@ -899,8 +869,6 @@ public void testIsotopicMods() throws IOException
                 }
                 PxModification pxMod = getIsotopicUnimodMod(mod, uMods);
 
-                printIsotopicMod(mod, pxMod);
-
                 List<UnimodModification> expectedMatches = matches.get(pxMod.getSkylineName());
 
                 if (expectedMatches.isEmpty())
@@ -926,31 +894,6 @@ else if (expectedMatches.size() > 1)
             }
         }
 
-        private void printIsotopicMod(IsotopeModification mod, PxModification pxMod)
-        {
-            if (!debug)
-            {
-                return;
-            }
-            if (pxMod.hasUnimodId())
-            {
-                String term = mod.getTerminus() == null ? "" : (mod.getTerminus().equals("N") ? "N-term" : "C-term");
-                String modInfo = pxMod.getSkylineName() + ", " + Formula.normalizeFormula(mod.getFormula()) + ", " + mod.getAminoAcid() + ", TERM: " + term;
-                System.out.print("Skyline: " + modInfo);
-                System.out.println(" --- " + pxMod.getUnimodId() + ", " + pxMod.getName());
-            }
-            if (pxMod.hasPossibleUnimods())
-            {
-                String term = mod.getTerminus() == null ? "" : (mod.getTerminus().equals("N") ? "N-term" : "C-term");
-                String modInfo = pxMod.getSkylineName() + ", " + Formula.normalizeFormula(mod.getFormula()) + ", " + mod.getAminoAcid() + ", TERM: " + term;
-                System.out.println("Skyline: " + modInfo);
-                for (UnimodModification umod: pxMod.getPossibleUnimodMatches())
-                {
-                    System.out.println(" --- List.of(new UnimodModification(" + umod.getId() + ", \"" + umod.getName() + "\", null))");
-                }
-            }
-        }
-
         private Modification createMod(String name, String formula, String sites, String terminus)
         {
             Modification mod = new Modification();
diff --git a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PublicationSearchTest.java b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PublicationSearchTest.java
index 93e7b3bd..fcd7314c 100644
--- a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PublicationSearchTest.java
+++ b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PublicationSearchTest.java
@@ -5,7 +5,7 @@
 import org.junit.experimental.categories.Category;
 import org.labkey.remoteapi.CommandException;
 import org.labkey.remoteapi.Connection;
-import org.labkey.remoteapi.SimpleGetCommand;
+import org.labkey.remoteapi.SimplePostCommand;
 import org.labkey.remoteapi.query.Filter;
 import org.labkey.remoteapi.query.SelectRowsCommand;
 import org.labkey.remoteapi.query.SelectRowsResponse;
@@ -341,7 +341,7 @@ private void initMockNcbiService()
         try
         {
             Connection connection = createDefaultConnection();
-            SimpleGetCommand command = new SimpleGetCommand("panoramapublic", "setupMockNcbiService");
+            SimplePostCommand command = new SimplePostCommand("panoramapublic", "setupMockNcbiService");
             command.execute(connection, "/");
             _useMockNcbi = true;
             log("Using mock NCBI service");
@@ -417,7 +417,7 @@ private void registerMockPublication(String database, String id, String searchKe
         try
         {
             Connection connection = createDefaultConnection();
-            SimpleGetCommand command = new SimpleGetCommand("panoramapublic", "registerMockPublication");
+            SimplePostCommand command = new SimplePostCommand("panoramapublic", "registerMockPublication");
             Map<String, Object> params = new HashMap<>();
             params.put("database", database);
             params.put("id", id);
@@ -447,7 +447,7 @@ private void restoreNcbiService()
         try
         {
             Connection connection = createDefaultConnection();
-            SimpleGetCommand command = new SimpleGetCommand("panoramapublic", "restoreNcbiService");
+            SimplePostCommand command = new SimplePostCommand("panoramapublic", "restoreNcbiService");
             command.execute(connection, "/");
             log("Restored real NCBI service");
         }

From 25e2a0076bf9f4721927b0dbddbc7e083840888d Mon Sep 17 00:00:00 2001
From: vagisha <vagisha@gmail.com>
Date: Wed, 1 Jul 2026 18:36:28 -0700
Subject: [PATCH 2/2] Address reported security issues in the testresults
 module (#654)

#### Changes
***Stored XSS (High) - result views and email preview**
* failureDetail.jsp: HTML-encode the failure language and stack trace
* runDetail.jsp: render the run log/xml popup via a `<pre>` textContent node instead of document.write of concatenated HTML.
* trainingdata.jsp: render the daily-email preview in a sandboxed iframe (scripts disabled, opaque origin) instead of document.write of the raw email HTML.

**Transaction and SQL handling (Medium)**
* ChangeBoundaries.execute: wrapped the GlobalSettings update transaction in try-with-resources so it cannot leak on an exception before commit, and parameterized the INSERT with bind parameters.

**Daily email output encoding (Low)**
* SendTestResultsEmail.getHTMLEmail: HTML-encode the git hash, missing-user name,  problem-table computer name, and test name.

**Logging hygiene (Low)**
* Replaced all five e.printStackTrace() calls with Log4J2 logging that includes the exception.

**Tests**
* Added testViewLogPopup, which opens the run-log popup and asserts the stored log is displayed (the existing tests only exercised the ViewLog API).
---
 .../testresults/SendTestResultsEmail.java     |  8 +--
 .../testresults/TestResultsController.java    | 49 ++++++++++---------
 .../labkey/testresults/TestResultsModule.java |  6 ++-
 .../testresults/TestResultsWebPart.java       |  6 ++-
 .../labkey/testresults/model/RunDetail.java   |  6 ++-
 .../labkey/testresults/view/failureDetail.jsp |  4 +-
 .../org/labkey/testresults/view/runDetail.jsp |  6 ++-
 .../labkey/testresults/view/trainingdata.jsp  | 11 ++++-
 .../tests/testresults/TestResultsTest.java    | 22 +++++++++
 9 files changed, 85 insertions(+), 33 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/TestResultsController.java b/testresults/src/org/labkey/testresults/TestResultsController.java
index 5b99149a..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;
@@ -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.";
diff --git a/testresults/src/org/labkey/testresults/TestResultsModule.java b/testresults/src/org/labkey/testresults/TestResultsModule.java
index bdd474ec..50af20ad 100644
--- a/testresults/src/org/labkey/testresults/TestResultsModule.java
+++ b/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);
         }
 
     }
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();
     }
diff --git a/testresults/src/org/labkey/testresults/view/failureDetail.jsp b/testresults/src/org/labkey/testresults/view/failureDetail.jsp
index b334fd3d..508f4b40 100644
--- a/testresults/src/org/labkey/testresults/view/failureDetail.jsp
+++ b/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";
diff --git a/testresults/src/org/labkey/testresults/view/runDetail.jsp b/testresults/src/org/labkey/testresults/view/runDetail.jsp
index c4895a38..b1c39187 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. 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..86390b17 100644
--- a/testresults/src/org/labkey/testresults/view/trainingdata.jsp
+++ b/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")
     });
 
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()
     {