Merge branch 'develop' into fb_fixImportPerfWaitCheck

labkey-danield · labkey-danield · commit 1b452cf85c61 · 2026-07-15T12:29:44.000-07:00
diff --git a/src/org/labkey/test/components/core/ApiKeyDialog.java b/src/org/labkey/test/components/core/ApiKeyDialog.java
@@ -182,7 +182,7 @@ protected class ElementCache extends ModalDialog.ElementCache
         OptionSelect<OptionSelect.SelectOption> restrictionRoleSelect = new OptionSelect<>(Locator.tagWithId("select", "keyRole")
             .findWhenNeeded(this).withTimeout(2000));
         WebElement generateApiKeyButton = Locator.tagWithText("button", "Generate API Key").findWhenNeeded(this);
-        Input inputField = Input.Input(Locator.tagWithClass("input", "api-key__input"), getDriver()).findWhenNeeded(this);
+        Input inputField = Input.Input(Locator.tagWithClass("input", "api-key__display"), getDriver()).findWhenNeeded(this);
         WebElement copyKeyButton = Locator.tagWithName("button", "copy_apikey_token").findWhenNeeded(this);
         WebElement doneButton = Locator.tagWithText("button", "Done").findWhenNeeded(this);
     }
diff --git a/src/org/labkey/test/pages/core/admin/CustomizeSitePage.java b/src/org/labkey/test/pages/core/admin/CustomizeSitePage.java
@@ -188,18 +188,6 @@ public CustomizeSitePage setAdminOnlyMessage(String value)
         return this;
     }
 
-    public CustomizeSitePage setCSRFCheck(CSRFCheck value)
-    {
-        elementCache().CSRFCheck.selectByValue(value.name());
-        return this;
-    }
-
-    public CustomizeSitePage setXFrameOption(XFrameOption value)
-    {
-        elementCache().XFrameOption.selectByValue(value.name());
-        return this;
-    }
-
     public CustomizeSitePage setEnableServerHttpHeader(boolean enable)
     {
         elementCache().enableServerHttpHeader.set(enable);
@@ -271,8 +259,6 @@ protected RadioButton exceptionReportingLevel(ReportingLevel level)
         protected final Input adminOnlyMessage = Input(Locator.id("adminOnlyMessage"), getDriver()).findWhenNeeded(this);
 
         // HTTP Security Settings
-        protected final Select CSRFCheck = Select(Locator.id("CSRFCheck")).findWhenNeeded(this);
-        protected final Select XFrameOption = Select(Locator.id("XFrameOption")).findWhenNeeded(this);
         protected final Checkbox enableServerHttpHeader = Checkbox(Locator.id("includeServerHttpHeader")).findWhenNeeded(this);
     }
 
@@ -281,17 +267,8 @@ public enum ReportingLevel
         NONE, LOW, MEDIUM, HIGH
     }
 
-    public enum CSRFCheck
-    {
-        POST, ADMINONLY
-    }
-
-    public enum XFrameOption
-    {
-        SAMEORIGIN, ALLOW
-    }
-
     private static final int SECONDS_PER_DAY = 60*60*24;
+
     public enum KeyExpirationOptions implements OptionSelect.SelectOption
     {
         UNLIMITED(-1),
diff --git a/src/org/labkey/test/tests/list/ListTest.java b/src/org/labkey/test/tests/list/ListTest.java
@@ -24,12 +24,16 @@
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
 import org.labkey.remoteapi.CommandException;
+import org.labkey.remoteapi.Connection;
 import org.labkey.remoteapi.domain.Domain;
 import org.labkey.remoteapi.domain.DomainResponse;
 import org.labkey.remoteapi.domain.PropertyDescriptor;
 import org.labkey.remoteapi.domain.SaveDomainCommand;
 import org.labkey.remoteapi.query.ContainerFilter;
 import org.labkey.remoteapi.query.Filter;
+import org.labkey.remoteapi.query.SelectRowsCommand;
+import org.labkey.remoteapi.query.SelectRowsResponse;
+import org.labkey.remoteapi.query.Sort;
 import org.labkey.serverapi.reader.TabLoader;
 import org.labkey.test.BaseWebDriverTest;
 import org.labkey.test.Locator;
@@ -55,6 +59,7 @@
 import org.labkey.test.params.FieldDefinition.StringLookup;
 import org.labkey.test.params.FieldInfo;
 import org.labkey.test.params.FieldKey;
+import org.labkey.test.params.list.IntListDefinition;
 import org.labkey.test.params.list.VarListDefinition;
 import org.labkey.test.tests.AuditLogTest;
 import org.labkey.test.util.AbstractDataRegionExportOrSignHelper.ColumnHeaderType;
@@ -867,6 +872,95 @@ public void testCustomViews()
         new PortalHelper(this).removeAllWebParts();
     }
 
+    /**
+     * CWE-639 (IDOR): a list audit event loaded by user-controlled rowId in
+     * ListItemDetailsAction must be tied back to the URL-requested listId before its
+     * old/new record maps are rendered. Without this, listId=X&rowId=N-belonging-to-Y
+     * would render List Y's audit payload inside List X's details page.
+     *
+     * Builds two lists in the same container, generates a modify-audit-event on the
+     * second one, then verifies the action refuses to render it when the URL names the
+     * first list. A positive control confirms the matched-listId path still works, so
+     * the test fails if the predicate is ever inverted/over-rejects.
+     */
+    @Test
+    public void testAuditDetailRejectsRowIdFromOtherList() throws Exception
+    {
+        final String LIST_X = "IDOR_VICTIM_LIST";       // attacker claims to be viewing details for this list
+        final String LIST_Y = "IDOR_SOURCE_LIST";       // audit event actually belongs to this list
+        final String NAME_FIELD = "Name";
+        final String LIST_Y_ROW_VALUE = "y-original";
+        final String LIST_Y_ROW_EDITED = "y-modified-secret";
+
+        log("Set up two lists in the same container, each with a Name field");
+        Connection connection = createDefaultConnection();
+        new IntListDefinition(LIST_X, "Key")
+            .addField(new FieldDefinition(NAME_FIELD, ColumnType.String))
+            .create(connection, getProjectName());
+        new IntListDefinition(LIST_Y, "Key")
+            .addField(new FieldDefinition(NAME_FIELD, ColumnType.String))
+            .create(connection, getProjectName());
+
+        log("Insert and then modify a row in List Y so it generates an audit event with old/new record maps");
+        _listHelper.goToList(LIST_Y);
+        _listHelper.clickImportData()
+            .setText(NAME_FIELD + "\n" + LIST_Y_ROW_VALUE)
+            .submit();
+        DataRegionTable yTable = new DataRegionTable("query", getDriver());
+        yTable.clickEditRow(yTable.getRowIndex(LIST_Y_ROW_VALUE));
+        setFormElement(Locator.name("quf_" + NAME_FIELD), LIST_Y_ROW_EDITED);
+        clickButton("Submit");
+
+        log("Discover List X's listId and the audit rowId for List Y's modification event");
+        Connection cn = createDefaultConnection();
+        int listXId = lookupListId(cn, LIST_X);
+        int listYId = lookupListId(cn, LIST_Y);
+        int listYAuditRowId = lookupListAuditRowId(cn, LIST_Y);
+
+        log("Attack: URL says listId=" + LIST_X + " but rowId points at the audit event for " + LIST_Y);
+        String attackUrl = WebTestHelper.buildURL("list", getProjectName(), "listItemDetails",
+            Map.of("listId", String.valueOf(listXId), "rowId", String.valueOf(listYAuditRowId)));
+        beginAt(attackUrl);
+        assertEquals("Action should render normally (200), not throw", 200, getResponseCode());
+        assertTextPresent("No details available for this event");
+        assertTextNotPresent(LIST_Y_ROW_VALUE);   // proof of non-disclosure: pre-edit value
+        assertTextNotPresent(LIST_Y_ROW_EDITED);  // proof of non-disclosure: post-edit value
+
+        log("Positive control: same rowId but with the matching listId must still render the audit changes");
+        String matchedUrl = WebTestHelper.buildURL("list", getProjectName(), "listItemDetails",
+            Map.of("listId", String.valueOf(listYId), "rowId", String.valueOf(listYAuditRowId)));
+        beginAt(matchedUrl);
+        assertEquals(200, getResponseCode());
+        assertTextPresent(LIST_Y_ROW_VALUE);
+        assertTextPresent(LIST_Y_ROW_EDITED);
+        assertTextNotPresent("No details available for this event");
+    }
+
+    private int lookupListId(Connection cn, String listName) throws Exception
+    {
+        SelectRowsCommand cmd = new SelectRowsCommand("exp", "Lists");
+        cmd.setColumns(List.of("RowId", "Name"));
+        cmd.addFilter(new Filter("Name", listName, Filter.Operator.EQUAL));
+        SelectRowsResponse rs = cmd.execute(cn, getProjectName());
+        if (rs.getRows().isEmpty())
+            throw new AssertionError("No exp.Lists row for " + listName);
+        return ((Number) rs.getRows().get(0).get("RowId")).intValue();
+    }
+
+    private int lookupListAuditRowId(Connection cn, String listName) throws Exception
+    {
+        // Most-recent ListAuditEvent for this list; the row-modify above will be it.
+        SelectRowsCommand cmd = new SelectRowsCommand("auditLog", "ListAuditEvent");
+        cmd.setColumns(List.of("RowId", "ListName", "Comment"));
+        cmd.addFilter(new Filter("ListName", listName, Filter.Operator.EQUAL));
+        cmd.setSorts(List.of(new Sort("RowId", Sort.Direction.DESCENDING)));
+        cmd.setMaxRows(1);
+        SelectRowsResponse rs = cmd.execute(cn, getProjectName());
+        if (rs.getRows().isEmpty())
+            throw new AssertionError("No ListAuditEvent for " + listName);
+        return ((Number) rs.getRows().get(0).get("RowId")).intValue();
+    }
+
     /* Issue 23487: add regression coverage for batch insert into list with multiple errors
     */
     @Test
diff --git a/src/org/labkey/test/tests/remoteapi/BulkUpdateGroupApiTest.java b/src/org/labkey/test/tests/remoteapi/BulkUpdateGroupApiTest.java
@@ -30,6 +30,7 @@
 import org.labkey.test.util.APIUserHelper;
 import org.labkey.test.util.ApiPermissionsHelper;
 import org.labkey.test.util.DataRegionTable;
+import org.labkey.test.util.PermissionsHelper;
 import org.labkey.test.util.PermissionsHelper.PrincipalType;
 
 import java.util.ArrayList;
@@ -62,11 +63,15 @@ public class BulkUpdateGroupApiTest extends BaseWebDriverTest
     private static Integer group1Id;
     private static Integer group2Id;
     private static final String siteGroup = "createdSiteGroup";
+    private static final String OTHER_PROJECT = "BulkUpdateGroupApiTest Other Project";
+    private static final String OTHER_GROUP = "otherProjectGroup";
+    private static Integer otherProjectGroupId;
 
     @Override
     protected void doCleanup(boolean afterTest) throws TestTimeoutException
     {
         _containerHelper.deleteProject(getProjectName(), afterTest);
+        _containerHelper.deleteProject(OTHER_PROJECT, afterTest);
         deleteTestUsers(EMAIL_SUFFIX);
         _permissionsHelper.deleteGroup(siteGroup);
     }
@@ -105,6 +110,9 @@ private void doSetup()
         user2Id = _userHelper.createUser(USER2).getUserId();
         group1Id = _permissionsHelper.createProjectGroup(GROUP1, getProjectName());
         group2Id = _permissionsHelper.createProjectGroup(GROUP2, getProjectName());
+
+        _containerHelper.createProject(OTHER_PROJECT, null);
+        otherProjectGroupId = _permissionsHelper.createProjectGroup(OTHER_GROUP, OTHER_PROJECT);
     }
 
     @Test
@@ -503,6 +511,60 @@ public void testBulkUpdateAuditing() throws Exception
         AuditLogTest.verifyAuditEvent(this, AuditLogTest.GROUP_AUDIT_EVENT, "Comment", GROUP1, 2);
     }
 
+    // A project group may only be modified through the container it belongs to; targeting another project's group must be rejected.
+    @Test
+    public void testGroupFromDifferentContainerError() throws Exception
+    {
+        // otherProjectGroupId belongs to OTHER_PROJECT, but the command targets this test's project
+        BulkUpdateGroupCommand command = new BulkUpdateGroupCommand(otherProjectGroupId);
+        command.addMemberUser(USER1);
+        Connection connection = createDefaultConnection();
+
+        try
+        {
+            String message = command.execute(connection, getProjectName()).getText();
+            fail("Expected CommandException modifying a group from another container\nResponse:\n" + message);
+        }
+        catch (CommandException e)
+        {
+            assertTrue("Expected cross-container error. Actual error: " + e.getMessage(), e.getMessage().contains("does not belong to this project"));
+        }
+
+        _permissionsHelper.assertUserNotInGroup(USER1, OTHER_GROUP, OTHER_PROJECT, PrincipalType.USER);
+    }
+
+    // Adding an existing user to a group requires only AdminPermission, but creating a brand-new user additionally requires AddUserPermission.
+    @Test
+    public void testCreateUserWithoutAddUserPermission() throws Exception
+    {
+        // A Folder Administrator has AdminPermission (so the action's @RequiresPermission passes) but lacks AddUserPermission
+        String folderAdmin = genTestEmail("folderadminnoadduser");
+        String newUser = genTestEmail("shouldnotbecreated");
+        _userHelper.createUser(folderAdmin);
+        _permissionsHelper.addMemberToRole(folderAdmin, PermissionsHelper.FOLDER_ADMIN_ROLE, PermissionsHelper.MemberType.user, getProjectName());
+
+        Connection connection = createDefaultConnection();
+        connection.impersonate(folderAdmin, getProjectName());
+        try
+        {
+            BulkUpdateGroupCommand command = new BulkUpdateGroupCommand(group1Id);
+            command.addMemberUser(USER1);   // pre-existing user: should be added successfully
+            command.addMemberUser(newUser); // new user: folder admin lacks AddUserPermission
+            BulkUpdateGroupResponse response = command.execute(connection, getProjectName());
+
+            List<String> errors = collectErrors(response);
+            assertEquals("Expected exactly one member error:\n" + String.join("\n", errors), 1, errors.size());
+            assertTrue("Expected AddUserPermission error. Actual error: " + errors.get(0), errors.get(0).contains("do not have permission to create new users"));
+        }
+        finally
+        {
+            connection.stopImpersonating();
+        }
+
+        assertNull("New user was created despite the caller lacking AddUserPermission", _userHelper.getUserId(newUser));
+        _permissionsHelper.assertUserInGroup(USER1, GROUP1, getProjectName(), PrincipalType.USER);
+    }
+
     protected List<String> collectErrors(BulkUpdateGroupResponse response)
     {
         Map<String, Object> errors = response.getErrors();
diff --git a/src/org/labkey/test/util/AuditLogHelper.java b/src/org/labkey/test/util/AuditLogHelper.java
@@ -284,12 +284,22 @@ public void checkAuditEventValuesForTransactionId(String containerPath, AuditEve
     public void checkAuditEventValuesForTransactionId(String containerPath, AuditEvent auditEventName, List<String> columnNames, Integer transactionId, List<Map<String, Object>> expectedValues) throws IOException, CommandException
     {
         List<Map<String, Object>> events = getAuditLogsForTransactionId(containerPath, auditEventName, columnNames, transactionId, ContainerFilter.CurrentAndSubfolders);
-        assertEquals("Unexpected number of events for transactionId " + transactionId, expectedValues.size(), events.size());
-        for (int i = 0; i < expectedValues.size(); i++)
-        {
-            for (String key : expectedValues.get(i).keySet())
-                assertEquals("Event " + i  + " value for " + key + " not as expected", expectedValues.get(i).get(key), events.get(i).get(key));
-        }
+
+        List<Map<String, Object>> unmatched = expectedValues.stream()
+                .filter(expectedRow -> {
+                    Set<String> keysOfInterest = expectedRow.keySet();
+                    return events.stream().noneMatch(actualRow -> {
+                        Map<String, Object> actualFiltered = actualRow.entrySet().stream()
+                                .filter(e -> keysOfInterest.contains(e.getKey()))
+                                .collect(HashMap::new,
+                                        (m, e) -> m.put(e.getKey(), e.getValue()),
+                                        HashMap::putAll);
+                        return actualFiltered.equals(expectedRow);
+                    });
+                })
+                .toList();
+
+        assertTrue("Expected rows with no match in actual: " + unmatched, unmatched.isEmpty());
     }
 
     public Map<String, Object> getTransactionAuditLogDetails(Integer transactionAuditId)

Original file line number	Diff line number	Diff line change
`@@ -182,7 +182,7 @@ protected class ElementCache extends ModalDialog.ElementCache`
`182`	`182`	`OptionSelect<OptionSelect.SelectOption> restrictionRoleSelect = new OptionSelect<>(Locator.tagWithId("select", "keyRole")`
`183`	`183`	`.findWhenNeeded(this).withTimeout(2000));`
`184`	`184`	`WebElement generateApiKeyButton = Locator.tagWithText("button", "Generate API Key").findWhenNeeded(this);`
`185`		`- Input inputField = Input.Input(Locator.tagWithClass("input", "api-key__input"), getDriver()).findWhenNeeded(this);`
	`185`	`+ Input inputField = Input.Input(Locator.tagWithClass("input", "api-key__display"), getDriver()).findWhenNeeded(this);`
`186`	`186`	`WebElement copyKeyButton = Locator.tagWithName("button", "copy_apikey_token").findWhenNeeded(this);`
`187`	`187`	`WebElement doneButton = Locator.tagWithText("button", "Done").findWhenNeeded(this);`
`188`	`188`	`}`