[bot] Fast-forward for 26.3.17

src/org/labkey/test/tests/PackageLockJsonTest.java

@@ -28,8 +28,7 @@
 public class PackageLockJsonTest
 {
     private static final Set<String> ALLOWED_DEPENDENCY_HOSTS = Set.of("registry.npmjs.org", "labkey.jfrog.io");
-    // Allow-list of '@isaacs/cliui' dependencies
-    private static final Set<String> ALLOWED_NONSTANDARD_VERSIONS = Set.of("npm:string-width@^4.2.0", "npm:strip-ansi@^6.0.1", "npm:wrap-ansi@^7.0.0");
+    private static final File SERVER_MODULES_DIR = new File(TestFileUtils.getLabKeyRoot(), "server/modules");
 
     private final List<String> errors = new ArrayList<>();
     private final File moduleDir;
@@ -44,11 +43,10 @@ public static Collection<Object[]> data()
     {
         List<File> allModules = new ArrayList<>();
 
-        File modulesDir = new File(TestFileUtils.getLabKeyRoot(), "server/modules");
-        File[] files = modulesDir.listFiles();
+        File[] files = SERVER_MODULES_DIR.listFiles();
         if (files == null)
         {
-            throw new RuntimeException("No files found in modules directory: " + modulesDir.getAbsolutePath());
+            throw new RuntimeException("No files found in modules directory: " + SERVER_MODULES_DIR.getAbsolutePath());
         }
         for (File file : files)
         {
@@ -96,17 +94,19 @@ public void testPackageLock() throws Exception
             }
         }
 
-        Assert.assertTrue("Bad sources: " + errors, errors.isEmpty());
+        Assert.assertTrue("Untrusted package sources:\n" + String.join("\n", errors), errors.isEmpty());
     }
 
     /// Verify that a package reference in a package-lock.json file only resolves to known hosts and has a valid version
     /// Also checks sub-dependencies
     private void verifyPackage(String packageName, JSONObject packageJson, File packageLockFile)
     {
+        String relPath = SERVER_MODULES_DIR.toPath().relativize(packageLockFile.toPath()).toString();
+
         String resolved = packageJson.optString("resolved");
         if (resolved.isBlank())
         {
-            TestLogger.debug("Resolved field is blank for package " + packageName + " in " + packageLockFile.getAbsolutePath());
+            TestLogger.debug("Resolved field is blank for package " + packageName + " in " + relPath);
         }
         else
         {
@@ -116,14 +116,14 @@ private void verifyPackage(String packageName, JSONObject packageJson, File pack
                 String host = resolvedURL.getHost();
                 if (!ALLOWED_DEPENDENCY_HOSTS.contains(host))
                 {
-                    String message = "Package " + packageName + " resolved to unrecognized host [" + host + "] in " + packageLockFile.getAbsolutePath();
+                    String message = "Package " + packageName + " resolved to unrecognized host [" + host + "] in " + relPath;
                     errors.add(message);
                     TestLogger.error(message);
                 }
             }
             catch (URISyntaxException e)
             {
-                String message = "Package " + packageName + " resolved to an invalid location [" + resolved + "] in " + packageLockFile.getAbsolutePath();
+                String message = "Package " + packageName + " resolved to an invalid location [" + resolved + "] in " + relPath;
                 errors.add(message);
                 TestLogger.error(message);
             }
@@ -132,7 +132,7 @@ private void verifyPackage(String packageName, JSONObject packageJson, File pack
         String version = packageJson.optString("version");
         if (version.isBlank() || !CharUtils.isAsciiNumeric(version.charAt(0)))
         {
-            String message = "Package " + packageName + " has bad version [" + version + "] in " + packageLockFile.getAbsolutePath();
+            String message = "Package " + packageName + " has bad version [" + version + "] in " + relPath;
             errors.add(message);
             TestLogger.error(message);
         }
@@ -148,9 +148,9 @@ private void verifyPackage(String packageName, JSONObject packageJson, File pack
             else
             {
                 String tVer = transitiveDeps.optString(tDep);
-                if (tVer == null || tVer.contains(":") && !ALLOWED_NONSTANDARD_VERSIONS.contains(tVer)) // URL, file, or workspace dependency
+                if (tVer == null || tVer.replaceAll("^npm:", "").contains(":")) // URL, file, or workspace dependency
                 {
-                    String message = "Package " + packageName + " has bad transitive dependency [" + tVer + "] in " + packageLockFile.getAbsolutePath();
+                    String message = "Package " + packageName + " has bad transitive dependency [" + tVer + "] in " + relPath;
                     errors.add(message);
                     TestLogger.error(message);
                 }

src/org/labkey/test/tests/list/ListTest.java

@@ -24,11 +24,15 @@
 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.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;
@@ -54,6 +58,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;
@@ -863,6 +868,95 @@ public void testCustomViews()
         crossContainerLookupTest();
     }
 
+    /**
+     * 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

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();

src/org/labkey/test/util/AuditLogHelper.java

@@ -261,19 +261,29 @@ public void checkAuditEventValuesForTransactionId(String containerPath, AuditEve
 
     public void checkAuditEventValuesForTransactionId(String containerPath, AuditEvent auditEventName, Integer transactionId, List<Map<String, Object>> expectedValues) throws IOException, CommandException
     {
-        List<String> columnNames = expectedValues.get(0).keySet().stream().map(Object::toString).toList();
+        List<String> columnNames = expectedValues.getFirst().keySet().stream().map(Object::toString).toList();
         checkAuditEventValuesForTransactionId(containerPath, auditEventName, columnNames, transactionId, expectedValues);
     }
 
     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)