merge branch 'develop' into the branch

ankurjuneja · ankurjuneja · commit 691c82bb505e · 2026-07-10T10:28:36.000-07:00
'fb_calendar_based_grouping_1209'
diff --git a/pull_request_template.md b/pull_request_template.md
diff --git a/resources/schemas/dbscripts/postgresql/targetedms-26.008-26.009.sql b/resources/schemas/dbscripts/postgresql/targetedms-26.008-26.009.sql
@@ -0,0 +1,2 @@
+-- Drop the AuditLog view so targetedms-create.sql (runs after this script) rebuilds it with the fixed recursive CTE.
+SELECT core.fn_dropifexists('AuditLog', 'targetedms', 'VIEW', NULL);
diff --git a/resources/schemas/dbscripts/postgresql/targetedms-create.sql b/resources/schemas/dbscripts/postgresql/targetedms-create.sql
@@ -19,7 +19,7 @@ WITH RECURSIVE logTree as (
          , nxt.entryHash
          , nxt.parentEntryHash
     FROM targetedms.AuditLogEntry nxt
-             JOIN logTree prev ON prev.parentEntryHash = nxt.entryHash
+             JOIN logTree prev ON prev.entryHash = nxt.parentEntryHash
 )
 SELECT t.entryId
      , e.documentguid
diff --git a/src/org/labkey/targetedms/SkylineAuditLogManager.java b/src/org/labkey/targetedms/SkylineAuditLogManager.java
@@ -593,7 +593,7 @@ public void testEntryRetrieval() throws AuditLogException
         {
             AuditLogTree node = new SkylineAuditLogManager(_container, null).buildLogTree(_docGUID);
             while(node.iterator().hasNext()){
-                AuditLogEntry ent = AuditLogEntry.retrieve(node.getEntryId());
+                AuditLogEntry ent = AuditLogEntry.retrieve(node.getEntryId(), _container);
                 assertNotNull(ent);
                 node = node.iterator().next();
             }
diff --git a/src/org/labkey/targetedms/TargetedMSContainerScopingTest.java b/src/org/labkey/targetedms/TargetedMSContainerScopingTest.java
@@ -0,0 +1,143 @@
+/*
+ * Copyright (c) 2026 LabKey Corporation
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.labkey.targetedms;
+
+import jakarta.servlet.http.HttpServletResponse;
+import org.apache.logging.log4j.Logger;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.labkey.api.data.Container;
+import org.labkey.api.data.SimpleFilter;
+import org.labkey.api.data.Table;
+import org.labkey.api.data.TableSelector;
+import org.labkey.api.exp.api.ExpRun;
+import org.labkey.api.exp.api.ExperimentService;
+import org.labkey.api.pipeline.PipeRoot;
+import org.labkey.api.pipeline.PipelineService;
+import org.labkey.api.query.FieldKey;
+import org.labkey.api.security.permissions.AbstractContainerScopingTest;
+import org.labkey.api.util.GUID;
+import org.labkey.api.util.logging.LogHelper;
+import org.labkey.api.view.ActionURL;
+import org.labkey.api.view.ViewBackgroundInfo;
+import org.labkey.targetedms.parser.skyaudit.UnitTestUtil;
+
+import java.io.File;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * Container-scoping integration tests for {@link TargetedMSController} actions that resolve an object by a
+ * global id. Each test sets up the same object in one folder and confirms the action rejects a request that addresses
+ * via the wrong container
+ */
+public class TargetedMSContainerScopingTest extends AbstractContainerScopingTest
+{
+    private static final Logger LOG = LogHelper.getLogger(TargetedMSContainerScopingTest.class, "TargetedMS container scoping tests");
+    private static final GUID DOC_GUID = new GUID("8f1c2a44-3c5e-4f0a-9d2b-6e7a1b3c5d9e");
+
+    @Before
+    @After
+    public void cleanupAuditLog()
+    {
+        UnitTestUtil.cleanupDatabase(DOC_GUID);
+    }
+
+    /**
+     * RemoveLinkVersionAction relinks a Skyline document-version chain by run rowId. It must reject a run that lives in
+     * a different container (mirroring its sibling SaveLinkVersionsAction), otherwise an editor in one folder could
+     * rewrite the version chain of documents they can't see.
+     */
+    @Test
+    public void removeLinkVersionIsContainerScoped() throws Exception
+    {
+        Container owner = createContainer("LinkOwner");
+        Container other = createContainer("LinkOther");
+
+        // Negative: a run that lives in 'owner' cannot be relinked through the 'other' folder
+        ExpRun foreignRun = createRun(owner, "foreign-version");
+        ActionURL foreign = new ActionURL(TargetedMSController.RemoveLinkVersionAction.class, other)
+                .addParameter("rowId", foreignRun.getRowId());
+        assertStatus(HttpServletResponse.SC_BAD_REQUEST, post(foreign, getAdmin()));
+
+        // Positive: a run that lives in the acting folder is accepted
+        ExpRun localRun = createRun(other, "local-version");
+        ActionURL local = new ActionURL(TargetedMSController.RemoveLinkVersionAction.class, other)
+                .addParameter("rowId", localRun.getRowId());
+        assertStatus(HttpServletResponse.SC_OK, post(local, getAdmin()));
+    }
+
+    /**
+     * ShowSkylineAuditLogExtraInfoAJAXAction renders the full Skyline audit detail (user, timestamp, extra info) for an
+     * audit entry resolved by global EntryId. It must only render entries whose owning run is in the requested folder,
+     * otherwise any reader could read another folder's document audit history by guessing entry ids.
+     */
+    @Test
+    public void auditLogExtraInfoIsContainerScoped() throws Exception
+    {
+        Container owner = createContainer("AuditOwner");
+        Container other = createContainer("AuditOther");
+
+        int entryId = importAuditLogEntry(owner);
+
+        // Negative: the entry's run lives in 'owner', so requesting it via 'other' must 404 rather than leak detail
+        ActionURL foreign = new ActionURL(TargetedMSController.ShowSkylineAuditLogExtraInfoAJAXAction.class, other)
+                .addParameter("entryId", entryId);
+        assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreign, getAdmin()));
+
+        // Positive: requesting the entry through its owning folder renders normally
+        ActionURL owned = new ActionURL(TargetedMSController.ShowSkylineAuditLogExtraInfoAJAXAction.class, owner)
+                .addParameter("entryId", entryId);
+        assertStatus(HttpServletResponse.SC_OK, get(owned, getAdmin()));
+    }
+
+    /** Persist a minimal, saved {@link ExpRun} in the given container so the controller can resolve it by rowId. */
+    private ExpRun createRun(Container c, String name) throws Exception
+    {
+        ExperimentService svc = ExperimentService.get();
+        ExpRun run = svc.createExperimentRun(c, name);
+        PipeRoot pipeRoot = PipelineService.get().findPipelineRoot(c);
+        assertNotNull("No pipeline root for " + c.getPath(), pipeRoot);
+        run.setFilePathRoot(pipeRoot.getRootPath());
+        run.setProtocol(svc.ensureSampleDerivationProtocol(getAdmin()));
+        ViewBackgroundInfo info = new ViewBackgroundInfo(c, getAdmin(), null);
+        return svc.saveSimpleExperimentRun(run, Collections.emptyMap(), Collections.emptyMap(),
+                Collections.emptyMap(), Collections.emptyMap(), Collections.emptyMap(), info, null, false);
+    }
+
+    /** Insert a TargetedMS run in the given container, import a sample Skyline audit log for it, and return an EntryId. */
+    private int importAuditLogEntry(Container c) throws Exception
+    {
+        TargetedMSRun run = new TargetedMSRun();
+        run.setContainer(c);
+        run.setDocumentGUID(DOC_GUID);
+        Table.insert(getAdmin(), TargetedMSManager.getTableInfoRuns(), run);
+
+        File zip = UnitTestUtil.getSampleDataFile("AuditLogFiles/MethodEdit_v1.zip");
+        File logFile = UnitTestUtil.extractLogFromZip(zip, LOG);
+        new SkylineAuditLogManager(c, null).importAuditLogFile(getAdmin(), logFile, DOC_GUID, run);
+
+        List<Integer> entryIds = new TableSelector(
+                TargetedMSManager.getTableInfoSkylineRunAuditLogEntry(),
+                Collections.singleton("AuditLogEntryId"),
+                new SimpleFilter(FieldKey.fromParts("VersionId"), run.getId()), null)
+                .getArrayList(Integer.class);
+        assertFalse("No audit log entries were imported", entryIds.isEmpty());
+        return entryIds.get(0);
+    }
+}
diff --git a/src/org/labkey/targetedms/TargetedMSController.java b/src/org/labkey/targetedms/TargetedMSController.java
@@ -5125,7 +5125,9 @@ public static class ShowSkylineAuditLogExtraInfoAJAXAction extends SimpleViewAct
         @Override
         public ModelAndView getView(SkylineAuditLogExtraInfoForm form, BindException errors)
         {
-            AuditLogEntry ent = AuditLogEntry.retrieve(form.getEntryId());
+            AuditLogEntry ent = AuditLogEntry.retrieve(form.getEntryId(), getContainer());
+            if (ent == null)
+                throw new NotFoundException("No audit log entry found for id " + form.getEntryId() + " in this folder.");
             getPageConfig().setTemplate(PageConfig.Template.None);
             return new JspView<>("/org/labkey/targetedms/view/skylineAuditLogExtraInfoView.jsp", ent);
         }
@@ -7821,7 +7823,7 @@ public void validateForm(RowIdForm form, Errors errors)
                 // verify that the run rowId is valid and matches an existing run
                 // and if the run replaces any other runs, it should only replace one
                 ExpRun run = ExperimentService.get().getExpRun(form.getRowId());
-                if (run == null)
+                if (run == null || !run.getContainer().equals(getContainer()))
                     errors.reject(ERROR_MSG, "No run found for id " + form.getRowId() + ".");
                 else if (!run.getReplacesRuns().isEmpty() && run.getReplacesRuns().size() > 1)
                     errors.reject(ERROR_MSG, "Run " + form.getRowId() + " replaces more than one run.");
diff --git a/src/org/labkey/targetedms/TargetedMSModule.java b/src/org/labkey/targetedms/TargetedMSModule.java
@@ -228,7 +228,7 @@ public String getName()
     @Override
     public Double getSchemaVersion()
     {
-        return 26.008;
+        return 26.009;
     }
 
     @Override
@@ -693,7 +693,8 @@ protected void startupAfterSpringConfig(ModuleContext moduleContext)
     {
         return Set.of(
             MsDataSourceUtil.TestCase.class,
-            SkylineAuditLogManager.TestCase.class
+            SkylineAuditLogManager.TestCase.class,
+            TargetedMSContainerScopingTest.class
         );
     }
 
diff --git a/src/org/labkey/targetedms/outliers/OutlierGenerator.java b/src/org/labkey/targetedms/outliers/OutlierGenerator.java
@@ -29,6 +29,7 @@
 import org.labkey.api.data.TableSelector;
 import org.labkey.api.query.QueryService;
 import org.labkey.api.security.User;
+import org.labkey.api.sql.LabKeySql;
 import org.labkey.api.targetedms.model.SampleFileInfo;
 import org.labkey.api.util.Pair;
 import org.labkey.targetedms.TargetedMSManager;
@@ -130,7 +131,7 @@ else if (configuration.getAnnotationName() != null)
             sql.append("(SELECT PrecursorChromInfoId, SampleFileId, ");
             sql.append(" CAST(IFDEFINED(SeriesLabel) AS VARCHAR) AS SeriesLabel, ");
             sql.append("\nMetricValue, ").append(configuration.getId()).append(" AS MetricId");
-            sql.append("\n FROM ").append(schemaName).append('.').append(queryName);
+            sql.append("\n FROM ").append(schemaName).append('.').append(LabKeySql.quoteIdentifier(queryName));
             sql.append(")");
         }
         return sql.toString();
diff --git a/src/org/labkey/targetedms/parser/skyaudit/AuditLogEntry.java b/src/org/labkey/targetedms/parser/skyaudit/AuditLogEntry.java
@@ -15,13 +15,15 @@
  */
 package org.labkey.targetedms.parser.skyaudit;
 
+import org.labkey.api.data.Container;
 import org.labkey.api.data.SimpleFilter;
 import org.labkey.api.data.Table;
 import org.labkey.api.data.TableSelector;
 import org.labkey.api.query.FieldKey;
 import org.labkey.api.security.User;
 import org.labkey.api.util.GUID;
 import org.labkey.targetedms.TargetedMSManager;
+import org.labkey.targetedms.TargetedMSRun;
 
 import java.math.BigDecimal;
 import java.math.BigInteger;
@@ -70,13 +72,25 @@ public AuditLogEntry(BigDecimal documentFormatVersion)
         _documentFormatVersion = documentFormatVersion;
     }
 
-    public static AuditLogEntry retrieve(int pEntryId)
+    /**
+     * Container-scoped retrieve. Returns the entry only if its owning Skyline run lives in the supplied container,
+     * preventing callers from reading audit detail for documents in folders the user can't access. An entryId can
+     * map to more than one run when documents share an audit history, so check every match for one in the container.
+     */
+    public static AuditLogEntry retrieve(int pEntryId, Container container)
     {
         TableSelector sel = new TableSelector(TargetedMSManager.getTableInfoSkylineAuditLog(), new SimpleFilter(FieldKey.fromParts("EntryId"), pEntryId), null);
-        List<AuditLogEntry> results = sel.getArrayList(AuditLogEntry.class);
-        // Possible to get more than one match if two documents share an audit history. In this case, we don't care
-        // which we use
-        return results.isEmpty() ? null : results.getFirst();
+        for (AuditLogEntry entry : sel.getArrayList(AuditLogEntry.class))
+        {
+            Long runId = entry.getRunId();
+            if (runId != null)
+            {
+                TargetedMSRun run = TargetedMSManager.getRun(runId);
+                if (run != null && container.equals(run.getContainer()))
+                    return entry;
+            }
+        }
+        return null;
     }
 
     public AuditLogTree getTreeEntry(){
diff --git a/src/org/labkey/targetedms/view/pharmacokinetics.jsp b/src/org/labkey/targetedms/view/pharmacokinetics.jsp
@@ -51,7 +51,7 @@
     %>
     <labkey:panel title="<%=subgroupTitle%>" type="portal">
         <div id="targetedms-fom-export" class="export-icon" data-toggle="tooltip" title="Export to Excel">
-            <%=iconLink("fa fa-file-excel-o", "", null).onClick("exportExcel('" + subgroup + "')") %>
+            <%=iconLink("fa fa-file-excel-o", "", null).onClick("exportExcel(" + q(subgroup) + ")") %>
         </div>
 
         <labkey:panel title="Statistics">
@@ -81,7 +81,7 @@
                 <span id="nonIVC0Controls-Warn-<%=h(subgroup)%>" class="labkey-error"><h4>WARNING: Please enter a non-IV C0 and recalculate.</h4></span>
                 non-IV C0
                 <input type="number" id="nonIVC0-<%=h(subgroup)%>" label="non-IV C0"/>
-                <% addHandler("btnNonIVC0-"+h(subgroup), "click", "updateStatsForNonIVC0('"+ subgroup + "')"); %>
+                <% addHandler("btnNonIVC0-"+h(subgroup), "click", "updateStatsForNonIVC0("+ q(subgroup) + ")"); %>
                 <button id="btnNonIVC0-<%=h(subgroup)%>">Recalculate</button>
             </div>
         </labkey:panel>
diff --git a/test/src/org/labkey/test/tests/targetedms/passport/PassportTest.java b/test/src/org/labkey/test/tests/targetedms/passport/PassportTest.java
@@ -24,6 +24,8 @@
 
 import java.util.List;
 
+import static org.junit.Assert.assertTrue;
+
 /**
  * Tests Panorama Passport integration end-to-end, covering user management, permission setup, and project
  * configuration for both normal and admin users.
@@ -95,13 +97,20 @@ private void testNormalStuff()
         waitForElement(Locator.tagWithId("span", "filteredPeptideCount").childTag("green"));
         assertElementContains(Locator.xpath("//span[@id='filteredPeptideCount']//green"), "19");
 
-        //features
-        waitForElements(Locator.tagWithClass("td", "feature-sequencevariant"), 5);
-        waitForElements(Locator.tagWithClass("td", "feature-glycosylationsite"), 4);
-        waitForElements(Locator.tagWithClass("td", "feature-helix"), 7);
-        waitForElements(Locator.tagWithClass("td", "feature-turn"), 6);
+        // Features come from a live query against Uniprot, so be tolerant of any number above our baseline
+        waitForElementsAtLeast(Locator.tagWithClass("td", "feature-sequencevariant"), 3);
+        waitForElementsAtLeast(Locator.tagWithClass("td", "feature-glycosylationsite"), 3);
+        waitForElementsAtLeast(Locator.tagWithClass("td", "feature-helix"), 3);
+        waitForElementsAtLeast(Locator.tagWithClass("td", "feature-turn"), 3);
     }
 
+    public void waitForElementsAtLeast(final Locator loc, final int count)
+    {
+        waitFor(() -> loc.findElements(getDriver()).size() >= count, WAIT_FOR_JAVASCRIPT);
+        assertTrue("Element not present at least the expected number of times", loc.findElements(getDriver()).size() >= count);
+    }
+
+
     @LogMethod
     protected void testAsNormalUser()
     {
diff --git a/webapp/TargetedMS/js/QCPlotHelperBase.js b/webapp/TargetedMS/js/QCPlotHelperBase.js
diff --git a/webapp/TargetedMS/js/QCTrendPlotPanel.js b/webapp/TargetedMS/js/QCTrendPlotPanel.js

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+-- Drop the AuditLog view so targetedms-create.sql (runs after this script) rebuilds it with the fixed recursive CTE.`
	`2`	`+SELECT core.fn_dropifexists('AuditLog', 'targetedms', 'VIEW', NULL);`
Original file line number	Diff line number	Diff line change
`@@ -19,7 +19,7 @@ WITH RECURSIVE logTree as (`
`19`	`19`	`, nxt.entryHash`
`20`	`20`	`, nxt.parentEntryHash`
`21`	`21`	`FROM targetedms.AuditLogEntry nxt`
`22`		`- JOIN logTree prev ON prev.parentEntryHash = nxt.entryHash`
	`22`	`+ JOIN logTree prev ON prev.entryHash = nxt.parentEntryHash`
`23`	`23`	`)`
`24`	`24`	`SELECT t.entryId`
`25`	`25`	`, e.documentguid`
Original file line number	Diff line number	Diff line change
`@@ -593,7 +593,7 @@ public void testEntryRetrieval() throws AuditLogException`
`593`	`593`	`{`
`594`	`594`	`AuditLogTree node = new SkylineAuditLogManager(_container, null).buildLogTree(_docGUID);`
`595`	`595`	`while(node.iterator().hasNext()){`
`596`		`- AuditLogEntry ent = AuditLogEntry.retrieve(node.getEntryId());`
	`596`	`+ AuditLogEntry ent = AuditLogEntry.retrieve(node.getEntryId(), _container);`
`597`	`597`	`assertNotNull(ent);`
`598`	`598`	`node = node.iterator().next();`
`599`	`599`	`}`
Original file line number	Diff line number	Diff line change
`@@ -228,7 +228,7 @@ public String getName()`
`228`	`228`	`@Override`
`229`	`229`	`public Double getSchemaVersion()`
`230`	`230`	`{`
`231`		`- return 26.008;`
	`231`	`+ return 26.009;`
`232`	`232`	`}`
`233`	`233`
`234`	`234`	`@Override`
`@@ -693,7 +693,8 @@ protected void startupAfterSpringConfig(ModuleContext moduleContext)`
`693`	`693`	`{`
`694`	`694`	`return Set.of(`
`695`	`695`	`MsDataSourceUtil.TestCase.class,`
`696`		`- SkylineAuditLogManager.TestCase.class`
	`696`	`+ SkylineAuditLogManager.TestCase.class,`
	`697`	`+ TargetedMSContainerScopingTest.class`
`697`	`698`	`);`
`698`	`699`	`}`
`699`	`700`