From 176cda4cb87694ef8be940653d9468bf7e7bc3fa Mon Sep 17 00:00:00 2001 From: alanv Date: Wed, 30 Sep 2026 10:22:26 -0500 Subject: [PATCH 01/11] Add JUnit tests for Issues 1524 and 1626 - Tests fail, as intended --- .../labkey/pipeline/PipelineController.java | 300 +++++++++++++++++- 1 file changed, 298 insertions(+), 2 deletions(-) diff --git a/pipeline/src/org/labkey/pipeline/PipelineController.java b/pipeline/src/org/labkey/pipeline/PipelineController.java index cd98a0ca7eb..5b7a72d25c1 100644 --- a/pipeline/src/org/labkey/pipeline/PipelineController.java +++ b/pipeline/src/org/labkey/pipeline/PipelineController.java @@ -42,13 +42,17 @@ import org.labkey.api.admin.AdminUrls; import org.labkey.api.admin.ImportException; import org.labkey.api.admin.ImportOptions; +import org.labkey.api.collections.CaseInsensitiveHashMap; import org.labkey.api.collections.IntHashMap; import org.labkey.api.compliance.ComplianceService; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerManager; import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.Table; +import org.labkey.api.data.TableInfo; import org.labkey.api.data.TableSelector; +import org.labkey.api.dataiterator.DataIteratorContext; +import org.labkey.api.dataiterator.MapDataIterator; import org.labkey.api.exp.property.DomainUtil; import org.labkey.api.files.FileContentService; import org.labkey.api.files.FilesAdminOptions; @@ -67,8 +71,12 @@ import org.labkey.api.pipeline.browse.PipelinePathForm; import org.labkey.api.pipeline.file.FileAnalysisTaskPipeline; import org.labkey.api.pipeline.view.SetupForm; +import org.labkey.api.query.BatchValidationException; import org.labkey.api.query.FieldKey; +import org.labkey.api.query.QueryService; +import org.labkey.api.query.QueryUpdateService; import org.labkey.api.query.QueryUrls; +import org.labkey.api.query.UserSchema; import org.labkey.api.security.Group; import org.labkey.api.security.MutableSecurityPolicy; import org.labkey.api.security.RequiresPermission; @@ -86,6 +94,7 @@ import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.permissions.UserManagementPermission; import org.labkey.api.security.roles.FolderAdminRole; +import org.labkey.api.security.roles.PlatformDeveloperRole; import org.labkey.api.security.roles.Role; import org.labkey.api.security.roles.RoleManager; import org.labkey.api.settings.AdminConsole; @@ -116,13 +125,13 @@ import org.labkey.pipeline.api.PipeRootImpl; import org.labkey.pipeline.api.PipelineEmailPreferences; import org.labkey.pipeline.api.PipelineManager; +import org.labkey.pipeline.api.PipelineQuerySchema; import org.labkey.pipeline.api.PipelineSchema; import org.labkey.pipeline.api.PipelineServiceImpl; -import org.labkey.pipeline.api.PipelineStatusManager; +import org.labkey.pipeline.api.PipelineStatusManager;Do import org.labkey.pipeline.status.StatusController; import org.labkey.vfs.FileLike; import org.springframework.beans.MutablePropertyValues; -import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.validation.BindException; import org.springframework.validation.Errors; import org.springframework.web.servlet.ModelAndView; @@ -132,6 +141,7 @@ import java.nio.file.Files; import java.text.ParseException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -1789,5 +1799,291 @@ public void testSavePipelineTriggerContainerScoping() throws Exception JSONObject ownEdit = new JSONObject().put("rowId", rowId); assertStatus(HttpServletResponse.SC_BAD_REQUEST, postJson(ownUrl, admin, ownEdit)); } + + // An unregistered type keeps the update service from starting a listener for these rows. + private static final String TEST_TRIGGER_TYPE = "scoping-test-type"; + private static final String PARAMETER_FUNCTION = "parameterFunction"; + private static final String FUNCTION = "var x = 1;"; + + @Test + public void testFolderAdminCanManageTriggerWithoutParameterFunction() throws Exception + { + Container c = createContainer("NoFunction"); + User folderAdmin = createUserInRole(c, FolderAdminRole.class); + assertFalse(folderAdmin.isTrustedAnalyst()); + + Map row = triggerRow(configJson(null)); + assertNoErrors(insert(folderAdmin, c, row)); + int rowId = rowIdByName(c, (String) row.get("Name")); + + Map edit = keyRow(rowId); + edit.put("Description", "edited"); + edit.put("Configuration", new JSONObject(configJson(null)).put("location", "./moved").toString()); + assertNoErrors(update(folderAdmin, c, edit)); + assertEquals("edited", storedRow(rowId).get("Description")); + } + + @Test + public void testFolderAdminCannotInsertParameterFunction() throws Exception + { + Container c = createContainer("InsertFunction"); + User folderAdmin = createUserInRole(c, FolderAdminRole.class); + + List configurations = List.of( + configJson(FUNCTION), + new JSONObject(configJson(FUNCTION)), + configJson(new JSONArray().put(FUNCTION)) + ); + for (Object configuration : configurations) + { + Map row = triggerRow(configuration); + assertTrue("Insert must be rejected: " + configuration, insert(folderAdmin, c, row).hasErrors()); + assertFalse("No row may be written: " + configuration, triggerExists(c, (String) row.get("Name"))); + } + + // Table.insert may ignore an alias key rather than reject it; either way no function may be stored. + Map aliased = triggerRow(null); + aliased.remove("Configuration"); + aliased.put(configurationPropertyURI(folderAdmin, c), configJson(FUNCTION)); + insert(folderAdmin, c, aliased); + if (triggerExists(c, (String) aliased.get("Name"))) + assertNull(storedFunction(rowIdByName(c, (String) aliased.get("Name")))); + } + + @Test + public void testFolderAdminCannotChangeParameterFunction() throws Exception + { + Container c = createContainer("ChangeFunction"); + User folderAdmin = createUserInRole(c, FolderAdminRole.class); + int rowId = seedTrigger(c, FUNCTION); + + List configurations = Arrays.asList( + configJson("var y = 2;"), + configJson(null), + configJson(""), + null, + new JSONObject(configJson("var y = 2;")), + configJson(new JSONArray().put(FUNCTION)) + ); + for (Object configuration : configurations) + { + Map edit = keyRow(rowId); + edit.put("Configuration", configuration); + assertTrue("Update must be rejected: " + configuration, update(folderAdmin, c, edit).hasErrors()); + assertEquals("Function must be unchanged: " + configuration, FUNCTION, storedFunction(rowId)); + } + + Map aliased = keyRow(rowId); + aliased.put(configurationPropertyURI(folderAdmin, c), configJson("var y = 2;")); + assertTrue("Update via propertyURI must be rejected", update(folderAdmin, c, aliased).hasErrors()); + assertEquals(FUNCTION, storedFunction(rowId)); + + int noFunctionRowId = seedTrigger(c, null); + Map add = keyRow(noFunctionRowId); + add.put("Configuration", configJson(FUNCTION)); + assertTrue("Adding a function must be rejected", update(folderAdmin, c, add).hasErrors()); + assertNull(storedFunction(noFunctionRowId)); + } + + @Test + public void testFolderAdminCanEditTriggerWithUnchangedParameterFunction() throws Exception + { + Container c = createContainer("UnchangedFunction"); + User folderAdmin = createUserInRole(c, FolderAdminRole.class); + int rowId = seedTrigger(c, FUNCTION); + + // The wizard regenerates the whole Configuration, so other keys change around the unchanged function. + Map edit = keyRow(rowId); + edit.put("Description", "edited"); + edit.put("Configuration", new JSONObject(configJson(FUNCTION)).put("location", "./moved").put("quiet", 5000).toString()); + assertNoErrors(update(folderAdmin, c, edit)); + assertEquals(FUNCTION, storedFunction(rowId)); + assertEquals("./moved", new JSONObject((String) storedRow(rowId).get("Configuration")).getString("location")); + + Map descriptionOnly = keyRow(rowId); + descriptionOnly.put("Description", "edited again"); + assertNoErrors(update(folderAdmin, c, descriptionOnly)); + assertEquals("edited again", storedRow(rowId).get("Description")); + assertEquals(FUNCTION, storedFunction(rowId)); + } + + @Test + public void testUpdateRowsContainerScoping() throws Exception + { + Container folderA = createContainer("UpdateA"); + Container folderB = createContainer("UpdateB"); + User adminA = createUserInRole(folderA, FolderAdminRole.class); + int rowId = seedTrigger(folderB, null); + String name = (String) storedRow(rowId).get("Name"); + + Map rename = keyRow(rowId); + rename.put("Name", "hacked"); + Map rehome = keyRow(rowId); + rehome.put("Name", "hacked"); + rehome.put("Container", folderA.getId()); + + for (User user : List.of(adminA, getAdmin())) + { + for (Map row : List.of(rename, rehome)) + { + assertTrue("Cross-container update must be rejected", update(user, folderA, new CaseInsensitiveHashMap<>(row)).hasErrors()); + assertUnchanged(rowId, name, folderB); + } + } + + // Key supplied via oldKeys rather than the row + BatchValidationException errors = new BatchValidationException(); + Map keyless = new CaseInsensitiveHashMap<>(Map.of("Name", "hacked")); + updateService(adminA, folderA).updateRows(adminA, folderA, List.of(keyless), List.of(keyRow(rowId)), errors, null, null); + assertTrue("Cross-container update via oldKeys must be rejected", errors.hasErrors()); + assertUnchanged(rowId, name, folderB); + } + + @Test + public void testTrustedAnalystCanManageParameterFunction() throws Exception + { + Container c = createContainer("TrustedFunction"); + User developer = createUserInRole(c, FolderAdminRole.class); + grantRole(developer, ContainerManager.getRoot(), PlatformDeveloperRole.class); + assertTrue(developer.isTrustedAnalyst()); + + Map row = triggerRow(configJson(FUNCTION)); + assertNoErrors(insert(developer, c, row)); + int rowId = rowIdByName(c, (String) row.get("Name")); + assertEquals(FUNCTION, storedFunction(rowId)); + + Map change = keyRow(rowId); + change.put("Configuration", configJson("var y = 2;")); + assertNoErrors(update(developer, c, change)); + assertEquals("var y = 2;", storedFunction(rowId)); + + Map clear = keyRow(rowId); + clear.put("Configuration", configJson(null)); + assertNoErrors(update(developer, c, clear)); + assertNull(storedFunction(rowId)); + } + + @Test + public void testLoadRowsRejected() throws Exception + { + Container c = createContainer("LoadRows"); + User folderAdmin = createUserInRole(c, FolderAdminRole.class); + + Map row = triggerRow(configJson(FUNCTION)); + DataIteratorContext context = new DataIteratorContext(); + context.setInsertOption(QueryUpdateService.InsertOption.IMPORT); + updateService(folderAdmin, c).loadRows(folderAdmin, c, MapDataIterator.of(List.of(row)), context, null); + assertTrue("loadRows must be rejected", context.getErrors().hasErrors()); + assertFalse(triggerExists(c, (String) row.get("Name"))); + } + + private static TableInfo triggerTable() + { + return PipelineSchema.getInstance().getTableInfoTriggerConfigurations(); + } + + private static QueryUpdateService updateService(User user, Container c) + { + UserSchema schema = QueryService.get().getUserSchema(user, c, PipelineQuerySchema.SCHEMA_NAME); + return schema.getTable(PipelineQuerySchema.TRIGGER_CONFIGURATIONS_TABLE_NAME).getUpdateService(); + } + + private static String configurationPropertyURI(User user, Container c) + { + UserSchema schema = QueryService.get().getUserSchema(user, c, PipelineQuerySchema.SCHEMA_NAME); + return schema.getTable(PipelineQuerySchema.TRIGGER_CONFIGURATIONS_TABLE_NAME).getColumn("Configuration").getPropertyURI(); + } + + private static String configJson(@Nullable Object parameterFunction) + { + JSONObject json = new JSONObject().put("location", "./"); + if (parameterFunction != null) + json.put(PARAMETER_FUNCTION, parameterFunction); + return json.toString(); + } + + private static Map triggerRow(@Nullable Object configuration) + { + Map row = new CaseInsensitiveHashMap<>(); + row.put("Name", "trigger-" + GUID.makeGUID()); + row.put("Type", TEST_TRIGGER_TYPE); + row.put("PipelineId", "scoping-test-pipeline"); + row.put("Enabled", false); + row.put("Configuration", configuration); + return row; + } + + private static Map keyRow(int rowId) + { + return new CaseInsensitiveHashMap<>(Map.of("RowId", rowId)); + } + + /** Writes directly to the table, bypassing the update service under test. */ + private int seedTrigger(Container c, @Nullable String parameterFunction) + { + TriggerConfiguration config = new TriggerConfiguration(); + config.beforeInsert(getAdmin(), c.getId()); + config.setName("trigger-" + GUID.makeGUID()); + config.setType(TEST_TRIGGER_TYPE); + config.setPipelineId("scoping-test-pipeline"); + config.setConfiguration(configJson(parameterFunction)); + return Table.insert(getAdmin(), triggerTable(), config).getRowId(); + } + + private static BatchValidationException insert(User user, Container c, Map row) throws Exception + { + BatchValidationException errors = new BatchValidationException(); + updateService(user, c).insertRows(user, c, List.of(row), errors, null, null); + return errors; + } + + private static BatchValidationException update(User user, Container c, Map row) throws Exception + { + BatchValidationException errors = new BatchValidationException(); + updateService(user, c).updateRows(user, c, List.of(row), null, errors, null, null); + return errors; + } + + private static void assertNoErrors(BatchValidationException errors) + { + assertFalse(errors.getMessage(), errors.hasErrors()); + } + + private static boolean triggerExists(Container c, String name) + { + SimpleFilter filter = SimpleFilter.createContainerFilter(c).addCondition(FieldKey.fromParts("Name"), name); + return new TableSelector(triggerTable(), filter, null).exists(); + } + + private static int rowIdByName(Container c, String name) + { + SimpleFilter filter = SimpleFilter.createContainerFilter(c).addCondition(FieldKey.fromParts("Name"), name); + Integer rowId = new TableSelector(triggerTable().getColumn("RowId"), filter, null).getObject(Integer.class); + assertNotNull("Trigger " + name + " must exist", rowId); + return rowId; + } + + private static Map storedRow(int rowId) + { + Map row = new TableSelector(triggerTable(), new SimpleFilter(FieldKey.fromParts("RowId"), rowId), null).getMap(); + assertNotNull("Trigger " + rowId + " must exist", row); + return new CaseInsensitiveHashMap<>(row); + } + + /** Extracts the function the way FileWatcherPipelineTriggerConfig does. */ + private static @Nullable String storedFunction(int rowId) + { + Object configuration = storedRow(rowId).get("Configuration"); + if (configuration == null) + return null; + return Objects.toString(new JSONObject(configuration.toString()).toMap().get(PARAMETER_FUNCTION), null); + } + + private static void assertUnchanged(int rowId, String name, Container container) + { + Map row = storedRow(rowId); + assertEquals("Name must be unchanged", name, row.get("Name")); + assertEquals("Container must be unchanged", container.getId(), row.get("Container")); + } } } From 886e791b1124294026b5bc8ab10cff8cf34f3275 Mon Sep 17 00:00:00 2001 From: alanv Date: Wed, 30 Sep 2026 11:35:32 -0500 Subject: [PATCH 02/11] TriggerConfigurationsTable::updateRows - Actually pass oldRow when updating, properly handle validationErrors --- .../labkey/pipeline/PipelineController.java | 2 +- .../query/TriggerConfigurationsTable.java | 18 ++++++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/pipeline/src/org/labkey/pipeline/PipelineController.java b/pipeline/src/org/labkey/pipeline/PipelineController.java index 5b7a72d25c1..93579e3f627 100644 --- a/pipeline/src/org/labkey/pipeline/PipelineController.java +++ b/pipeline/src/org/labkey/pipeline/PipelineController.java @@ -128,7 +128,7 @@ import org.labkey.pipeline.api.PipelineQuerySchema; import org.labkey.pipeline.api.PipelineSchema; import org.labkey.pipeline.api.PipelineServiceImpl; -import org.labkey.pipeline.api.PipelineStatusManager;Do +import org.labkey.pipeline.api.PipelineStatusManager; import org.labkey.pipeline.status.StatusController; import org.labkey.vfs.FileLike; import org.springframework.beans.MutablePropertyValues; diff --git a/pipeline/src/org/labkey/pipeline/query/TriggerConfigurationsTable.java b/pipeline/src/org/labkey/pipeline/query/TriggerConfigurationsTable.java index 1c33ffb0c81..1aa7fe60587 100644 --- a/pipeline/src/org/labkey/pipeline/query/TriggerConfigurationsTable.java +++ b/pipeline/src/org/labkey/pipeline/query/TriggerConfigurationsTable.java @@ -66,6 +66,7 @@ import java.util.LinkedList; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; public class TriggerConfigurationsTable extends SimpleUserSchema.SimpleTable @@ -253,16 +254,25 @@ public List> insertRows(User user, Container container, List @Override public List> updateRows(User user, Container container, List> rows, List> oldKeys, BatchValidationException errors, @Nullable Map configParameters, Map extraScriptContext) throws InvalidKeyException, QueryUpdateServiceException, SQLException { - List> ret = new LinkedList<>(); - for (Map row : rows) + if (oldKeys != null && rows.size() != oldKeys.size()) + throw new IllegalArgumentException("rows and oldKeys are required to be the same length, but were " + rows.size() + " and " + oldKeys.size() + " in length, respectively"); + + List> ret = new ArrayList<>(rows.size()); + for (int i = 0; i < rows.size(); i++) { + Map row = rows.get(i); try { - ret.add(updateRow(user, container, row, row, false, true)); + Map oldRow = getRow(user, container, oldKeys == null ? row : oldKeys.get(i)); + // getRow() selects by RowId alone, so the row may belong to another folder + if (oldRow == null || !container.getId().equals(Objects.toString(oldRow.get("Container"), null))) + throw new ValidationException("Pipeline trigger configuration not found in this folder."); + + ret.add(updateRow(user, container, row, oldRow, false, true)); } catch (ValidationException e) { - ret.remove(row); + errors.addRowError(e); } } return ret; From f63bcd9c6a72370304a5e6a523a0069981298f71 Mon Sep 17 00:00:00 2001 From: alanv Date: Wed, 30 Sep 2026 11:40:36 -0500 Subject: [PATCH 03/11] TriggerConfigurationsTable::updateRows - throw when errors.hasErrors() is true --- .../labkey/pipeline/PipelineController.java | 18 ++++++++++++++---- .../query/TriggerConfigurationsTable.java | 6 +++++- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/pipeline/src/org/labkey/pipeline/PipelineController.java b/pipeline/src/org/labkey/pipeline/PipelineController.java index 93579e3f627..b974709127e 100644 --- a/pipeline/src/org/labkey/pipeline/PipelineController.java +++ b/pipeline/src/org/labkey/pipeline/PipelineController.java @@ -1932,10 +1932,8 @@ public void testUpdateRowsContainerScoping() throws Exception } // Key supplied via oldKeys rather than the row - BatchValidationException errors = new BatchValidationException(); Map keyless = new CaseInsensitiveHashMap<>(Map.of("Name", "hacked")); - updateService(adminA, folderA).updateRows(adminA, folderA, List.of(keyless), List.of(keyRow(rowId)), errors, null, null); - assertTrue("Cross-container update via oldKeys must be rejected", errors.hasErrors()); + assertTrue("Cross-container update via oldKeys must be rejected", update(adminA, folderA, keyless, keyRow(rowId)).hasErrors()); assertUnchanged(rowId, name, folderB); } @@ -2038,9 +2036,21 @@ private static BatchValidationException insert(User user, Container c, Map row) throws Exception + { + return update(user, c, row, null); + } + + private static BatchValidationException update(User user, Container c, Map row, @Nullable Map oldKey) throws Exception { BatchValidationException errors = new BatchValidationException(); - updateService(user, c).updateRows(user, c, List.of(row), null, errors, null, null); + try + { + updateService(user, c).updateRows(user, c, List.of(row), oldKey == null ? null : List.of(oldKey), errors, null, null); + } + catch (BatchValidationException e) + { + return e; + } return errors; } diff --git a/pipeline/src/org/labkey/pipeline/query/TriggerConfigurationsTable.java b/pipeline/src/org/labkey/pipeline/query/TriggerConfigurationsTable.java index 1aa7fe60587..c96ec506cd9 100644 --- a/pipeline/src/org/labkey/pipeline/query/TriggerConfigurationsTable.java +++ b/pipeline/src/org/labkey/pipeline/query/TriggerConfigurationsTable.java @@ -252,7 +252,7 @@ public List> insertRows(User user, Container container, List } @Override - public List> updateRows(User user, Container container, List> rows, List> oldKeys, BatchValidationException errors, @Nullable Map configParameters, Map extraScriptContext) throws InvalidKeyException, QueryUpdateServiceException, SQLException + public List> updateRows(User user, Container container, List> rows, List> oldKeys, BatchValidationException errors, @Nullable Map configParameters, Map extraScriptContext) throws InvalidKeyException, BatchValidationException, QueryUpdateServiceException, SQLException { if (oldKeys != null && rows.size() != oldKeys.size()) throw new IllegalArgumentException("rows and oldKeys are required to be the same length, but were " + rows.size() + " and " + oldKeys.size() + " in length, respectively"); @@ -275,6 +275,10 @@ public List> updateRows(User user, Container container, List errors.addRowError(e); } } + + if (errors.hasErrors()) + throw errors; + return ret; } From c81934e179168b765a2e0afc42a9b725b4dbf3ae Mon Sep 17 00:00:00 2001 From: alanv Date: Wed, 30 Sep 2026 15:01:20 -0500 Subject: [PATCH 04/11] TriggerConfigurationsTable: validate configuration during _insert and _update - Only allow valid JSON to be inserted - Only allow trusted analysts to alter Parameter Function --- .../labkey/pipeline/PipelineController.java | 73 ++++++++++++-- .../query/TriggerConfigurationsTable.java | 94 +++++++++++++++++++ 2 files changed, 159 insertions(+), 8 deletions(-) diff --git a/pipeline/src/org/labkey/pipeline/PipelineController.java b/pipeline/src/org/labkey/pipeline/PipelineController.java index b974709127e..aa2a7b01cb3 100644 --- a/pipeline/src/org/labkey/pipeline/PipelineController.java +++ b/pipeline/src/org/labkey/pipeline/PipelineController.java @@ -1804,6 +1804,7 @@ public void testSavePipelineTriggerContainerScoping() throws Exception private static final String TEST_TRIGGER_TYPE = "scoping-test-type"; private static final String PARAMETER_FUNCTION = "parameterFunction"; private static final String FUNCTION = "var x = 1;"; + private static final String INVALID_JSON = "{not json"; @Test public void testFolderAdminCanManageTriggerWithoutParameterFunction() throws Exception @@ -1961,18 +1962,74 @@ public void testTrustedAnalystCanManageParameterFunction() throws Exception assertNull(storedFunction(rowId)); } + /** Trigger lookups parse both JSON columns of every matching row, so one invalid row breaks listener startup and management for all of them. */ @Test - public void testLoadRowsRejected() throws Exception + public void testInvalidJsonRejectedForAllUsers() throws Exception { - Container c = createContainer("LoadRows"); + Container c = createContainer("InvalidJson"); User folderAdmin = createUserInRole(c, FolderAdminRole.class); + User developer = createUserInRole(c, FolderAdminRole.class); + grantRole(developer, ContainerManager.getRoot(), PlatformDeveloperRole.class); - Map row = triggerRow(configJson(FUNCTION)); - DataIteratorContext context = new DataIteratorContext(); - context.setInsertOption(QueryUpdateService.InsertOption.IMPORT); - updateService(folderAdmin, c).loadRows(folderAdmin, c, MapDataIterator.of(List.of(row)), context, null); - assertTrue("loadRows must be rejected", context.getErrors().hasErrors()); - assertFalse(triggerExists(c, (String) row.get("Name"))); + for (User user : List.of(folderAdmin, developer)) + { + for (String column : List.of("Configuration", "CustomConfiguration")) + { + Map row = triggerRow(configJson(null)); + row.put(column, INVALID_JSON); + assertTrue(column + " must be valid JSON on insert", insert(user, c, row).hasErrors()); + assertFalse(triggerExists(c, (String) row.get("Name"))); + + int rowId = seedTrigger(c, null); + Map change = keyRow(rowId); + change.put(column, INVALID_JSON); + assertTrue(column + " must be valid JSON on update", update(user, c, change).hasErrors()); + assertNotEquals(INVALID_JSON, storedRow(rowId).get(column)); + } + } + } + + /** A stored Configuration that isn't valid JSON can't run a function, so a folder admin may replace it with one that has none. */ + @Test + public void testFolderAdminCanRepairInvalidConfiguration() throws Exception + { + Container c = createContainer("RepairJson"); + User folderAdmin = createUserInRole(c, FolderAdminRole.class); + int rowId = seedTrigger(c, null); + Table.update(getAdmin(), triggerTable(), new CaseInsensitiveHashMap<>(Map.of("Configuration", INVALID_JSON)), rowId); + + Map withFunction = keyRow(rowId); + withFunction.put("Configuration", configJson(FUNCTION)); + assertTrue("Repair must not add a function", update(folderAdmin, c, withFunction).hasErrors()); + assertEquals(INVALID_JSON, storedRow(rowId).get("Configuration")); + + Map repair = keyRow(rowId); + repair.put("Configuration", configJson(null)); + assertNoErrors(update(folderAdmin, c, repair)); + assertNull(storedFunction(rowId)); + } + + /** loadRows skips insertRow/updateRow, and with them the Parameter Function check and listener startup, so it's rejected for every caller. */ + @Test + public void testLoadRowsRejectedInFavorOfInsertRows() throws Exception + { + Container c = createContainer("LoadRows"); + User developer = createUserInRole(c, FolderAdminRole.class); + grantRole(developer, ContainerManager.getRoot(), PlatformDeveloperRole.class); + Map row = triggerRow(configJson(null)); + + // IMPORT backs the import action; MERGE backs ETL targets + for (QueryUpdateService.InsertOption option : List.of(QueryUpdateService.InsertOption.IMPORT, QueryUpdateService.InsertOption.MERGE)) + { + DataIteratorContext context = new DataIteratorContext(); + context.setInsertOption(option); + updateService(developer, c).loadRows(developer, c, MapDataIterator.of(List.of(row)), context, null); + assertTrue(option + " via loadRows must be rejected", context.getErrors().hasErrors()); + assertFalse(triggerExists(c, (String) row.get("Name"))); + } + + assertNoErrors(insert(developer, c, row)); + assertTrue(triggerExists(c, (String) row.get("Name"))); } private static TableInfo triggerTable() diff --git a/pipeline/src/org/labkey/pipeline/query/TriggerConfigurationsTable.java b/pipeline/src/org/labkey/pipeline/query/TriggerConfigurationsTable.java index c96ec506cd9..4a27cc56673 100644 --- a/pipeline/src/org/labkey/pipeline/query/TriggerConfigurationsTable.java +++ b/pipeline/src/org/labkey/pipeline/query/TriggerConfigurationsTable.java @@ -15,8 +15,12 @@ */ package org.labkey.pipeline.query; +import org.apache.commons.lang3.StringUtils; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.json.JSONException; +import org.json.JSONObject; +import org.labkey.api.collections.CaseInsensitiveHashMap; import org.labkey.api.collections.NamedObjectList; import org.labkey.api.data.AbstractForeignKey; import org.labkey.api.data.AbstractTableInfo; @@ -27,6 +31,8 @@ import org.labkey.api.data.RenderContext; import org.labkey.api.data.TableInfo; import org.labkey.api.data.TableSelector; +import org.labkey.api.dataiterator.DataIteratorBuilder; +import org.labkey.api.dataiterator.DataIteratorContext; import org.labkey.api.pipeline.PipelineJobService; import org.labkey.api.pipeline.TaskPipeline; import org.labkey.api.pipeline.file.FileAnalysisTaskPipeline; @@ -71,6 +77,10 @@ public class TriggerConfigurationsTable extends SimpleUserSchema.SimpleTable { + private static final String CONFIGURATION = "Configuration"; + private static final String CUSTOM_CONFIGURATION = "CustomConfiguration"; + private static final String PARAMETER_FUNCTION = "parameterFunction"; + public TriggerConfigurationsTable(PipelineQuerySchema schema, ContainerFilter cf) { super(schema, PipelineSchema.getInstance().getTableInfoTriggerConfigurations(), cf); @@ -251,6 +261,18 @@ public List> insertRows(User user, Container container, List return ret; } + /** + * The data iterator skips insertRow()/updateRow(), so it would bypass validateConfiguration() and never start or stop listeners. + * Reports an error instead of throwing UnsupportedOperationException because the import action shows context errors + * to the user but lets runtime exceptions escape as a 500. + */ + @Override + public int loadRows(User user, Container container, DataIteratorBuilder rows, @Nullable ArrayList> outputRows, DataIteratorContext context, @Nullable Map extraScriptContext) + { + context.getErrors().addRowError(new ValidationException("Bulk loading pipeline trigger configurations is not supported.")); + return 0; + } + @Override public List> updateRows(User user, Container container, List> rows, List> oldKeys, BatchValidationException errors, @Nullable Map configParameters, Map extraScriptContext) throws InvalidKeyException, BatchValidationException, QueryUpdateServiceException, SQLException { @@ -308,6 +330,78 @@ protected Map updateRow(User user, Container container, Map _insert(User user, Container c, Map row) throws SQLException, ValidationException + { + validateConfiguration(user, null, row); + return super._insert(user, c, row); + } + + @Override + protected Map _update(User user, Container c, Map row, Map oldRow, Object[] keys) throws SQLException, ValidationException + { + validateConfiguration(user, oldRow, row); + return super._update(user, c, row, oldRow, keys); + } + + private void validateConfiguration(User user, @Nullable Map oldRow, Map newRow) throws ValidationException + { + Map row = new CaseInsensitiveHashMap<>(newRow); + parseJson(row, CUSTOM_CONFIGURATION); + if (oldRow != null && !row.containsKey(CONFIGURATION)) + return; + + JSONObject configuration = parseJson(row, CONFIGURATION); + + // GH Issue 1524: the Parameter Function runs as server-side script, so only script authors may add, change, or clear it + if (user.isTrustedAnalyst()) + return; + + JSONObject oldConfiguration = null; + if (oldRow != null) + { + try + { + oldConfiguration = parseJson(new CaseInsensitiveHashMap<>(oldRow), CONFIGURATION); + } + catch (ValidationException ignored) + { + // A stored value that isn't valid JSON can't run, so it has no function to preserve + } + } + + if (!Objects.equals(getParameterFunction(oldConfiguration), getParameterFunction(configuration))) + throw new ValidationException("You must be either a PlatformDeveloper or TrustedAnalyst to set a Parameter Function."); + } + + /** Empty is allowed because FileWatcherPipelineTriggerConfig reads it as {}; anything else must parse there too */ + private static @Nullable JSONObject parseJson(Map row, String column) throws ValidationException + { + Object value = row.get(column); + if (value == null || StringUtils.isEmpty(value.toString())) + return null; + + try + { + return new JSONObject(value.toString()); + } + catch (JSONException e) + { + throw new ValidationException("Invalid JSON for " + column + ": " + e.getMessage(), column); + } + } + + /** Extracts the function as FileWatcherPipelineTriggerConfig does, where any non-null value runs via toString() */ + private static @Nullable String getParameterFunction(@Nullable JSONObject configuration) + { + if (configuration == null) + return null; + + String function = Objects.toString(configuration.toMap().get(PARAMETER_FUNCTION), null); + return StringUtils.isBlank(function) ? null : function; + } + /** Implement to make sure the listener gets unregistered */ @Override public int truncateRows(User user, Container container) throws QueryUpdateServiceException, SQLException From 31d2fe94d2a2b455c66e30a35bc93e9aba3660d4 Mon Sep 17 00:00:00 2001 From: alanv Date: Wed, 30 Sep 2026 15:06:00 -0500 Subject: [PATCH 05/11] PipelineController: Don't prefill parameter function from URL params --- .../labkey/pipeline/PipelineController.java | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/pipeline/src/org/labkey/pipeline/PipelineController.java b/pipeline/src/org/labkey/pipeline/PipelineController.java index aa2a7b01cb3..1c61b7f9a27 100644 --- a/pipeline/src/org/labkey/pipeline/PipelineController.java +++ b/pipeline/src/org/labkey/pipeline/PipelineController.java @@ -132,6 +132,7 @@ import org.labkey.pipeline.status.StatusController; import org.labkey.vfs.FileLike; import org.springframework.beans.MutablePropertyValues; +import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.validation.BindException; import org.springframework.validation.Errors; import org.springframework.web.servlet.ModelAndView; @@ -1452,6 +1453,13 @@ public ModelAndView getView(PipelineTriggerForm form, BindException errors) throw new NotFoundException("Pipeline trigger with id " + rowId + " could not be found"); } } + else + { + // GH Issue 1524: don't let a crafted link plant a function in the collapsed Advanced Settings. The + // reset regenerates a bound raw "configuration" from the fields, now without the function. + form.setParameterFunction(null); + form.resetConfiguration(); + } if (form.getReturnUrl() == null) form.setReturnUrl(getContainer().getStartURL(getUser()).toString()); @@ -2032,6 +2040,33 @@ public void testLoadRowsRejectedInFavorOfInsertRows() throws Exception assertTrue(triggerExists(c, (String) row.get("Name"))); } + /** + * On create the wizard is pre-filled from URL parameters, so a crafted link could plant a function that a + * trusted user saves without seeing it. + * */ + @Test + public void testCreateTriggerIgnoresParameterFunctionFromUrl() throws Exception + { + Container c = createContainer("CreateFromUrl"); + String location = "prefilledLocation" + GUID.makeHash(); + String function = "plantedFunction" + GUID.makeHash(); + + ActionURL viaFields = new ActionURL(CreatePipelineTriggerAction.class, c) + .addParameter("location", location) + .addParameter(PARAMETER_FUNCTION, function); + ActionURL viaConfiguration = new ActionURL(CreatePipelineTriggerAction.class, c) + .addParameter("configuration", new JSONObject().put("location", location).put(PARAMETER_FUNCTION, function).toString()); + + for (ActionURL url : List.of(viaFields, viaConfiguration)) + { + MockHttpServletResponse response = get(url, getAdmin()); + assertStatus(HttpServletResponse.SC_OK, response); + String content = response.getContentAsString(); + assertTrue("Other URL values must still pre-fill the wizard", content.contains(location)); + assertFalse("A function from the URL must not pre-fill the wizard", content.contains(function)); + } + } + private static TableInfo triggerTable() { return PipelineSchema.getInstance().getTableInfoTriggerConfigurations(); From 6f49bee54ff3fca890d0275ee12cc9e588fcbd63 Mon Sep 17 00:00:00 2001 From: alanv Date: Wed, 30 Sep 2026 15:14:23 -0500 Subject: [PATCH 06/11] createPipelineTrigger.jsp - Hide Parameter Function for anyone that isn't a trusted analyst --- .../CreatePipelineTrigger.tsx | 37 ++++++++++++------- .../labkey/pipeline/createPipelineTrigger.jsp | 1 + 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/pipeline/src/client/CreatePipelineTrigger/CreatePipelineTrigger.tsx b/pipeline/src/client/CreatePipelineTrigger/CreatePipelineTrigger.tsx index 818caa8cb85..09ad7fcf334 100644 --- a/pipeline/src/client/CreatePipelineTrigger/CreatePipelineTrigger.tsx +++ b/pipeline/src/client/CreatePipelineTrigger/CreatePipelineTrigger.tsx @@ -180,7 +180,6 @@ const formStateReducer = (state: FormState, action: FormStateAction): FormState if (field === 'pipelineId') { // Set default values on the customConfig based on the appropriate FormSchema. resetCustomConfig = {}; - // eslint-disable-next-line no-unused-expressions customFieldFormSchemas[value]?.fields.forEach(f => { if (f.defaultValue !== null) { resetCustomConfig[f.name] = f.defaultValue; @@ -193,6 +192,10 @@ const formStateReducer = (state: FormState, action: FormStateAction): FormState if (taskFormSchema) { Object.keys(resetTriggerConfig).forEach(key => { + // parameterFunction isn't task-specific, and users without permission must save it back + // unchanged + if (key === 'parameterFunction') return; + if (taskFormSchema.fields.find(f => f.name === key) === undefined) { delete resetTriggerConfig[key]; } @@ -514,6 +517,7 @@ const CustomParameters: FC = ({ customParameters, dispatc }; interface ConfigurationFormProps { + canEditParameterFunction: boolean; dispatch: Dispatch; formState: FormState; onBack: () => void; @@ -522,7 +526,7 @@ interface ConfigurationFormProps { } const ConfigurationForm: FC = props => { - const { formState, dispatch, onBack, onSubmit, returnUrl } = props; + const { canEditParameterFunction, formState, dispatch, onBack, onSubmit, returnUrl } = props; const { customConfig, customConfigValid, @@ -569,19 +573,21 @@ const ConfigurationForm: FC = props => { {showAdvanced && (
-
- -
-