From ad17f12e7308f2ee477ee109c6cc19d3fd93c75f Mon Sep 17 00:00:00 2001 From: vagisha Date: Tue, 23 Jun 2026 17:30:32 -0700 Subject: [PATCH 1/3] TestResults module bugfixes (#646) Fixes several bugs in the testresults module found while writing Selenium tests, plus a folder-scoping gap in how runs are looked up by id: - Training stats: extracted `recomputeUserData`(scope, userid, container), which recomputes a user's stats from their remaining training runs and deletes the `UserData `row when none remain. Called by `TrainRunAction `and `DeleteRunAction`. - `DeleteRunAction`: delete all child rows (now including handleleaks and trainruns) before the parent run, then refresh the owner's training stats. - `TrainRunAction`: verify the run exists before recompute on the force path too. - Folder scoping: added `getRunInContainer(runId, container)` - `trainRun`, `deleteRun`, `flagRun`, `viewLog`, `viewXml `and the flagged-runs list now reject or omit runs from other folders. - `showRun` and `TrainingDataViewAction `were already scoped. - `failureDetail.jsp`: set the chart's date-axis bounds only when dates exist. - Date parsing: parse MM/dd/yyyy strictly and reject invalid dates; use a fresh formatter per call instead of one shared static instance. - Added new tests for strict dates, deleting a run with child rows, the recompute update branch (mean and stddev), the trainRun force-path error, and cross-folder run access (testRunAccessIsContainerScoped). --------- Co-authored-by: Claude Co-authored-by: Ankur Juneja --- .../testresults/TestResultsController.java | 172 +++++++--- .../labkey/testresults/view/failureDetail.jsp | 21 +- .../tests/testresults/TestResultsTest.java | 316 +++++++++++++++++- 3 files changed, 451 insertions(+), 58 deletions(-) diff --git a/testresults/src/org/labkey/testresults/TestResultsController.java b/testresults/src/org/labkey/testresults/TestResultsController.java index 17faeb9f..5b99149a 100644 --- a/testresults/src/org/labkey/testresults/TestResultsController.java +++ b/testresults/src/org/labkey/testresults/TestResultsController.java @@ -142,7 +142,18 @@ public class TestResultsController extends SpringActionController { private static final Logger _log = LogManager.getLogger(TestResultsController.class); - private static final SimpleDateFormat MDYFormat = new SimpleDateFormat("MM/dd/yyyy"); + private static final String MDY_PATTERN = "MM/dd/yyyy"; + + // SimpleDateFormat is not thread-safe and is lenient by default, so we never share a + // single static instance. Each caller gets a fresh, strict formatter: strict parsing + // makes nonsense dates like 13/45/2026 throw ParseException instead of silently rolling + // over to a valid-but-wrong date (month 13 -> +1 year, day 45 -> spill into next month). + private static SimpleDateFormat mdyFormat() + { + SimpleDateFormat format = new SimpleDateFormat(MDY_PATTERN); + format.setLenient(false); + return format; + } private static final String KEY_SUCCESS = "Success"; @@ -175,7 +186,76 @@ public static String getTabClass(String tabName, String activeTab) private static Date parseDate(String dateStr) throws ParseException { - return StringUtils.isNotBlank(dateStr) ? MDYFormat.parse(dateStr) : null; + return StringUtils.isNotBlank(dateStr) ? mdyFormat().parse(dateStr) : null; + } + + /** + * Loads the run with the given id only if it belongs to the given container, otherwise returns + * null. Run ids are global and the raw testruns table has no query-layer container filter, so + * actions that look a run up by id must scope by container or a user could read or modify a run + * in another folder by guessing its id. + */ + private static RunDetail getRunInContainer(int runId, Container container) + { + SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("id"), runId); + filter.addCondition(FieldKey.fromParts("container"), container.getEntityId()); + return new TableSelector(TestResultsSchema.getTableInfoTestRuns(), filter, null).getObject(RunDetail.class); + } + + /** + * Recomputes one user's training statistics (mean/stddev of tests-run and memory) in the + * given container from their remaining training runs. If the user has no training runs + * left, deletes their UserData row instead. + * + * The delete branch matters: the recompute groups by (userid, container), so an empty + * join produces zero output rows, ON CONFLICT never fires, and a stale UserData row would + * otherwise survive with its old mean/stddev values (leaving the user listed on the + * training-data page as if they still had training data). Callers must invoke this inside + * an open transaction. + */ + private static void recomputeUserData(DbScope scope, int userid, Container container) + { + SQLFragment remainingFragment = new SQLFragment(); + remainingFragment.append( + "SELECT COUNT(*) FROM " + TestResultsSchema.getTableInfoTestRuns() + " " + + "JOIN " + TestResultsSchema.getTableInfoTrain() + + " ON " + TestResultsSchema.getTableInfoTestRuns() + ".id = " + TestResultsSchema.getTableInfoTrain() + ".runid " + + "WHERE userid = ? AND container = ?"); + remainingFragment.add(userid); + remainingFragment.add(container.getEntityId()); + long remainingTrainRuns = new SqlSelector(TestResultsSchema.getSchema(), remainingFragment).getObject(Long.class); + + if (remainingTrainRuns == 0) + { + SQLFragment deleteFragment = new SQLFragment(); + deleteFragment.append("DELETE FROM " + TestResultsSchema.getTableInfoUserData() + " WHERE userid = ? AND container = ?"); + deleteFragment.add(userid); + deleteFragment.add(container.getEntityId()); + new SqlExecutor(scope).execute(deleteFragment); + } + else + { + SQLFragment sqlFragmentUpdate = new SQLFragment(); + sqlFragmentUpdate.append( + "INSERT INTO " + TestResultsSchema.getTableInfoUserData() + " " + + " (userid, container, meantestsrun, meanmemory, stddevtestsrun, stddevmemory) " + + "SELECT ?, ?, avg(passedtests), avg(averagemem), stddev_pop(passedtests), stddev_pop(averagemem) " + + "FROM " + TestResultsSchema.getTableInfoTestRuns() + " " + + "JOIN " + TestResultsSchema.getTableInfoTrain() + + " ON " + TestResultsSchema.getTableInfoTestRuns() + ".id = " + TestResultsSchema.getTableInfoTrain() + ".runid " + + "WHERE userid = ? AND container = ? " + + "GROUP BY userid, container " + + "ON CONFLICT(userid, container) DO UPDATE SET " + + " meantestsrun = excluded.meantestsrun, " + + " meanmemory = excluded.meanmemory, " + + " stddevtestsrun = excluded.stddevtestsrun, " + + " stddevmemory = excluded.stddevmemory"); + sqlFragmentUpdate.add(userid); + sqlFragmentUpdate.add(container.getEntityId()); + sqlFragmentUpdate.add(userid); + sqlFragmentUpdate.add(container.getEntityId()); + new SqlExecutor(scope).execute(sqlFragmentUpdate); + } } // Form class for RetrainAllAction @@ -529,16 +609,14 @@ public Object execute(TrainRunForm form, BindException errors) SqlSelector sqlSelector = new SqlSelector(TestResultsSchema.getSchema(), sqlFragment); List foundRuns = new ArrayList<>(); sqlSelector.forEach(rs -> foundRuns.add(rs.getInt("runid"))); - SimpleFilter filter = new SimpleFilter(); - filter.addCondition(FieldKey.fromParts("id"), runId); - RunDetail[] details = new TableSelector(TestResultsSchema.getTableInfoTestRuns(), filter, null).getArray(RunDetail.class); - if (!force) - { - if (details.length == 0) - return new ApiSimpleResponse(Map.of(KEY_SUCCESS, false, "error", "run does not exist: " + runId)); - else if ((train && !foundRuns.isEmpty()) || (!train && foundRuns.isEmpty())) - return new ApiSimpleResponse(Map.of(KEY_SUCCESS, false, "error", "no action necessary")); - } + // The run must exist (in this folder) even on the force path: recomputeUserData below + // uses run.getUserid(), so a missing run would otherwise throw. Scoping by container + // also stops a run in another folder from being trained from here. + RunDetail run = getRunInContainer(runId, getContainer()); + if (run == null) + return new ApiSimpleResponse(Map.of(KEY_SUCCESS, false, "error", "run does not exist: " + runId)); + if (!force && ((train && !foundRuns.isEmpty()) || (!train && foundRuns.isEmpty()))) + return new ApiSimpleResponse(Map.of(KEY_SUCCESS, false, "error", "no action necessary")); DbScope scope = TestResultsSchema.getSchema().getScope(); try (DbScope.Transaction transaction = scope.ensureTransaction()) { @@ -551,27 +629,8 @@ else if ((train && !foundRuns.isEmpty()) || (!train && foundRuns.isEmpty())) fragment.add(runId); new SqlExecutor(scope).execute(fragment); } - // update user table calculations - SQLFragment sqlFragmentUpdate = new SQLFragment(); - sqlFragmentUpdate.append( - "INSERT INTO " + TestResultsSchema.getTableInfoUserData() + " " + - " (userid, container, meantestsrun, meanmemory, stddevtestsrun, stddevmemory) " + - "SELECT ?, ?, avg(passedtests), avg(averagemem), stddev_pop(passedtests), stddev_pop(averagemem) " + - "FROM " + TestResultsSchema.getTableInfoTestRuns() + " " + - "JOIN " + TestResultsSchema.getTableInfoTrain() + - " ON " + TestResultsSchema.getTableInfoTestRuns() + ".id = " + TestResultsSchema.getTableInfoTrain() + ".runid " + - "WHERE userid = ? AND container = ? " + - "GROUP BY userid, container " + - "ON CONFLICT(userid, container) DO UPDATE SET " + - " meantestsrun = excluded.meantestsrun, " + - " meanmemory = excluded.meanmemory, " + - " stddevtestsrun = excluded.stddevtestsrun, " + - " stddevmemory = excluded.stddevmemory"); - sqlFragmentUpdate.add(details[0].getUserid()); - sqlFragmentUpdate.add(getContainer().getEntityId()); - sqlFragmentUpdate.add(details[0].getUserid()); - sqlFragmentUpdate.add(getContainer().getEntityId()); - new SqlExecutor(scope).execute(sqlFragmentUpdate); + // Refresh this user's training statistics from their remaining training runs. + recomputeUserData(scope, run.getUserid(), getContainer()); transaction.commit(); } return new ApiSimpleResponse(KEY_SUCCESS, true); @@ -990,14 +1049,39 @@ public Object execute(RunIdForm form, BindException errors) } int rowId = form.getRunId(); + // Look up the run (scoped to this folder) before deleting, both to reject a run in + // another folder and to refresh the owner's training stats afterward: the run may + // have been part of their training set, so their baseline can change (or disappear) + // once it is gone. + RunDetail run = getRunInContainer(rowId, getContainer()); + if (run == null) + { + response.put(KEY_SUCCESS, false); + response.put("error", "run does not exist: " + rowId); + return response; + } + // Child tables keyed by testrunid -> testruns(id). SimpleFilter filter = new SimpleFilter(); filter.addCondition(FieldKey.fromParts("testrunid"), rowId); - try (DbScope.Transaction transaction = TestResultsSchema.getSchema().getScope().ensureTransaction()) { + // trainruns keys the run via its "runid" column, not "testrunid". + SimpleFilter trainFilter = new SimpleFilter(); + trainFilter.addCondition(FieldKey.fromParts("runid"), rowId); + DbScope scope = TestResultsSchema.getSchema().getScope(); + try (DbScope.Transaction transaction = scope.ensureTransaction()) { + // Delete every child row that references this run before the parent, or the + // testruns delete fails on a foreign key constraint. handleleaks and trainruns + // were previously missed: deleting a run that had handle-leak records or was in + // the training set failed (e.g. fk_memoryleaks_testruns on table handleleaks). Table.delete(TestResultsSchema.getTableInfoTestFails(), filter); Table.delete(TestResultsSchema.getTableInfoTestPasses(), filter); Table.delete(TestResultsSchema.getTableInfoMemoryLeaks(), filter); + Table.delete(TestResultsSchema.getTableInfoHandleLeaks(), filter); Table.delete(TestResultsSchema.getTableInfoHangs(), filter); + Table.delete(TestResultsSchema.getTableInfoTrain(), trainFilter); Table.delete(TestResultsSchema.getTableInfoTestRuns(), rowId); // delete run last because of foreign key + // Refresh the owner's training stats now that the run (and any trainruns row for + // it) is gone, so a removed training run does not leave a stale UserData row behind. + recomputeUserData(scope, run.getUserid(), getContainer()); transaction.commit(); } catch (Exception x) { response.put(KEY_SUCCESS, false); @@ -1039,10 +1123,9 @@ public Object execute(FlagRunForm form, BindException errors) int rowId = form.getRunId(); boolean flag = form.getFlag() != null ? form.getFlag() : false; - SimpleFilter filter = new SimpleFilter(); - filter.addCondition(FieldKey.fromParts("id"), rowId); try (DbScope.Transaction transaction = TestResultsSchema.getSchema().getScope().ensureTransaction()) { - RunDetail detail = new TableSelector(TestResultsSchema.getTableInfoTestRuns(), filter, null).getObject(RunDetail.class); + // Scoped to this folder so a run in another folder cannot be flagged from here. + RunDetail detail = getRunInContainer(rowId, getContainer()); if (detail == null) { response.put(KEY_SUCCESS, false); @@ -1096,8 +1179,11 @@ public static class ShowFlaggedAction extends SimpleViewAction @Override public ModelAndView getView(Object o, BindException errors) throws Exception { + // Scoped to this folder so the Flags page lists only flagged runs in the current folder, + // not flagged runs across every folder. SimpleFilter filter = new SimpleFilter(); filter.addCondition(FieldKey.fromParts("flagged"), true); + filter.addCondition(FieldKey.fromParts("container"), getContainer().getEntityId()); RunDetail[] details = new TableSelector(TestResultsSchema.getTableInfoTestRuns(), filter, null).getArray(RunDetail.class); JspView view = new JspView<>("/org/labkey/testresults/view/flagged.jsp", new TestsDataBean(details, new User[0])); view.setFrame(WebPartView.FrameType.PORTAL); @@ -1196,9 +1282,11 @@ public Object execute(RunIdForm form, BindException errors) if (form.getRunId() == null) return new ApiSimpleResponse("log", null); int runId = form.getRunId(); + // Scoped to this folder so a run's log in another folder cannot be read by id. SQLFragment sqlFragment = new SQLFragment(); - sqlFragment.append("SELECT log FROM testresults.testruns WHERE id = ?"); + sqlFragment.append("SELECT log FROM testresults.testruns WHERE id = ? AND container = ?"); sqlFragment.add(runId); + sqlFragment.add(getContainer().getEntityId()); List logs = new ArrayList<>(); SqlSelector sqlSelector = new SqlSelector(TestResultsSchema.getSchema(), sqlFragment); sqlSelector.forEach(rs -> logs.add(rs.getBytes("log"))); @@ -1217,9 +1305,11 @@ public Object execute(RunIdForm form, BindException errors) if (form.getRunId() == null) return new ApiSimpleResponse("xml", null); int runId = form.getRunId(); + // Scoped to this folder so a run's XML in another folder cannot be read by id. SQLFragment sqlFragment = new SQLFragment(); - sqlFragment.append("SELECT xml FROM testresults.testruns WHERE id = ?"); + sqlFragment.append("SELECT xml FROM testresults.testruns WHERE id = ? AND container = ?"); sqlFragment.add(runId); + sqlFragment.add(getContainer().getEntityId()); List xmls = new ArrayList<>(); SqlSelector sqlSelector = new SqlSelector(TestResultsSchema.getSchema(), sqlFragment); sqlSelector.forEach(rs -> xmls.add(rs.getBytes("xml"))); @@ -1963,7 +2053,7 @@ private static void ParseAndStoreXML(String xml, Container c) throws Exception timestampDay = addDays(timestampDay, 1); } lastHour = hour; - String originalDate = MDYFormat.format(timestampDay); + String originalDate = mdyFormat().format(timestampDay); try { timestamp = new SimpleDateFormat("MM/dd/yyyy HH:mm").parse(originalDate + " " + ts); } catch (IllegalArgumentException e) { @@ -2015,7 +2105,7 @@ private static void ParseAndStoreXML(String xml, Container c) throws Exception timestampDay = addDays(timestampDay, 1); } lastHour = hour; - String originalDate = MDYFormat.format(timestampDay); + String originalDate = mdyFormat().format(timestampDay); try { timestamp = new SimpleDateFormat("MM/dd/yyyy HH:mm").parse(originalDate + " " + ts); } catch (IllegalArgumentException e) { diff --git a/testresults/src/org/labkey/testresults/view/failureDetail.jsp b/testresults/src/org/labkey/testresults/view/failureDetail.jsp index 3d675525..b334fd3d 100644 --- a/testresults/src/org/labkey/testresults/view/failureDetail.jsp +++ b/testresults/src/org/labkey/testresults/view/failureDetail.jsp @@ -266,6 +266,19 @@ $(document).ready(function() { const problemData = <%=json(problemData, 0)%>; // Generate date chart. + // Only set explicit timeseries min/max when there are dates to plot. With an empty + // dates array, dates[0] and dates[length - 1] are undefined and c3/d3 throws while + // building the x-axis scale (e.g. when the active date range excludes all sample runs). + const chartDates = problemData?.graphData?.dates; + const xAxis = { + type: 'timeseries', + localtime: false, + tick: { fit: true, format: '%m/%d' } + }; + if (chartDates.length > 0) { + xAxis.min = chartDates[0]; + xAxis.max = chartDates[chartDates.length - 1]; + } let dateChart = c3.generate({ bindto: '#failGraph', data: { @@ -277,13 +290,7 @@ $(document).ready(function() { bar: { width: { ratio: 0.3 } }, subchart: { show: false, size: { height: 20 } }, axis: { - x: { - min: problemData.graphData.dates[0], - max: problemData.graphData.dates[problemData.graphData.dates.length - 1], - type: 'timeseries', - localtime: false, - tick: { fit: true, format: '%m/%d' } - }, + x: xAxis, // y: { tick: { values: %=graphYTicks.getJavaScriptFragment(0)% } } } }); diff --git a/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java b/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java index 49ee3345..1b1c37b9 100644 --- a/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java +++ b/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java @@ -28,6 +28,7 @@ import org.junit.experimental.categories.Category; import org.labkey.remoteapi.CommandException; import org.labkey.remoteapi.Connection; +import org.labkey.remoteapi.query.Filter; import org.labkey.remoteapi.query.SelectRowsCommand; import org.labkey.remoteapi.query.SelectRowsResponse; import org.labkey.remoteapi.query.Sort; @@ -50,10 +51,13 @@ import java.time.format.DateTimeFormatter; import java.util.List; import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; @Category({External.class, MacCossLabModules.class}) @@ -64,6 +68,10 @@ public class TestResultsTest extends BaseWebDriverTest implements PostgresOnlyTe static final String COMPUTER_NAME_1 = "TEST-PC-1"; static final String COMPUTER_NAME_2 = "TEST-PC-2"; + // Training stats are exact (avg/stddev of integer averagemem), so this only absorbs the + // floating round-trip through the query API. It is not a real tolerance. + private static final double EPSILON = 1e-6; + private static final Locator SUBMIT_BUTTON = Locator.css("input[type='submit'][value='Submit']"); // XPath for the problems matrix table (header cell contains "Fail: | Leak: | Hang:") @@ -466,19 +474,18 @@ public void testTrainingDataPage() toggleTrainingSet(); assertTextPresent("Add to training set"); - // After removal: the Remove link and the run's data row are gone, and - // the stats row now shows "RunCount:0". TEST-PC-1's section is still - // rendered in the training table though, because of a known bug in - // TrainRunAction: when the user's last training run is removed, the - // the stale UserData row (non-zero meanmemory / meantestsrun) is - // never cleared. trainingdata.jsp:158 only moves users to the - // "No Training Data --" list when both fields are 0, so the section - // stays. See TODO-LK-20260403_testresults-bugs.md. + // After removing the user's last training run, TrainRunAction deletes their + // UserData row, so TEST-PC-1 no longer has any training data: its section + // disappears from and the user moves to the + // "No Training Data --" list (it now behaves like TEST-PC-2). Previously a + // stale UserData row left a lingering RunCount:0 section here. clickAndWait(Locator.linkWithText("Training Data")); assertElementNotPresent(removeLink); assertElementNotPresent(trainingTable.containing("2026-01-16 06:00")); - assertElementPresent(statsRow.containing("RunCount:0")); - assertTextPresent(COMPUTER_NAME_2, "No Training Data --"); + assertElementNotPresent(trainingTable.descendant( + Locator.tagWithId("tr", "user-anchor-" + COMPUTER_NAME_1))); + assertElementNotPresent(statsRow); + assertTextPresent(COMPUTER_NAME_1, COMPUTER_NAME_2, "No Training Data --"); } @Test @@ -598,6 +605,14 @@ public void testApiErrorResponses() assertFalse(missingRun.optBoolean("Success", true)); assertEquals("run does not exist: 999999", missingRun.optString("error")); + // TrainRunAction: force path also requires the run to exist. The existence check runs + // before the recompute regardless of force, so a bad runId reports not-found instead of + // throwing on an empty lookup. + JSONObject forceMissingRun = postApi("trainRun", + Map.of("runId", "999999", "train", "force")); + assertFalse(forceMissingRun.optBoolean("Success", true)); + assertEquals("run does not exist: 999999", forceMissingRun.optString("error")); + // SetUserActive: missing userId JSONObject noUserId = postApi("setUserActive", Map.of("active", "true")); assertEquals("userId is required", noUserId.optString("Message")); @@ -640,6 +655,13 @@ public void testInvalidDateParameters() beginAt(WebTestHelper.buildRelativeUrl("testresults", PROJECT_NAME, "showFailures", Map.of("end", "03-24-2026"))); assertTextPresent("Invalid date format: 03-24-2026 (expected MM/dd/yyyy)"); + + // Strict parsing: an out-of-range but MM/dd/yyyy-shaped date like 13/45/2026 used to + // silently roll over to a valid date (02/14/2027) because the shared SimpleDateFormat + // was lenient. It must now be rejected like any other invalid date. + beginAt(WebTestHelper.buildRelativeUrl("testresults", PROJECT_NAME, "begin", + Map.of("end", "13/45/2026"))); + assertTextPresent("Invalid date format: 13/45/2026 (expected MM/dd/yyyy)"); } @Test @@ -666,6 +688,144 @@ public void testDeleteRun() assertTextNotPresent("TestDisposableOne"); } + @Test + public void testDeleteRunWithChildRecordsRecomputesTraining() + { + // Post a fresh run that has both handle-leak and memory-leak child rows, then add it + // to the training set. Deleting it must (1) succeed despite the handleleaks and + // trainruns child rows that previously caused a foreign key violation, and (2) refresh + // the user's training stats so no stale UserData row is left behind. + int baselineUserData = userDataRowCount(); + + int runId = postAndGetNewRunId("testresults/pc1-run-0117-leaks.xml", COMPUTER_NAME_1); + assertTrue("Posted run should have handle-leak child rows", childRowCount("handleleaks", runId) > 0); + assertTrue("Posted run should have memory-leak child rows", childRowCount("memoryleaks", runId) > 0); + + JSONObject trainResp = postApi("trainRun", Map.of("runId", String.valueOf(runId), "train", "true")); + assertTrue("trainRun should succeed: " + trainResp, trainResp.optBoolean("Success", false)); + assertTrue("A UserData row should exist after adding a training run", userDataRowCount() >= 1); + + // Without the fix this returned Success=false with a foreign key violation on + // handleleaks (and would also fail for the trainruns row). + JSONObject deleteResp = postApi("deleteRun", Map.of("runId", String.valueOf(runId))); + assertTrue("deleteRun should succeed without a foreign key violation: " + deleteResp, + deleteResp.optBoolean("Success", false)); + + // Child rows are cleaned up, and the training stats are refreshed: this was the run's + // only training run, so its UserData row is removed rather than left stale. + assertEquals("handleleaks child rows should be deleted", 0, childRowCount("handleleaks", runId)); + assertEquals("memoryleaks child rows should be deleted", 0, childRowCount("memoryleaks", runId)); + assertEquals("testpasses child rows should be deleted", 0, childRowCount("testpasses", runId)); + assertEquals("Deleting the only training run should remove its UserData row", + baselineUserData, userDataRowCount()); + } + + @Test + public void testRemoveTrainingRunRecomputesStatsWhenRunsRemain() + { + // Covers the recomputeUserData "update" branch: when a user still has training runs + // after one is removed, their UserData row must survive (not be deleted) and its stats + // must be recomputed from the remaining runs. The other tests only exercise the + // "delete" branch (removing a user's only/last training run). + int userId = getUserId(COMPUTER_NAME_1); + int baselineUserData = userDataRowCount(); + double cleanMem = runAverageMem(_cleanRunId); + double leakMem = runAverageMem(_leakRunId); + + try + { + // Add two TEST-PC-1 runs to the training set; both share one UserData row whose + // mean memory is the average of the two runs. + assertTrainRun(_cleanRunId, "true"); + assertTrainRun(_leakRunId, "true"); + assertEquals("Both training runs should share one UserData row", + baselineUserData + 1, userDataRowCount()); + assertEquals("Mean memory should be the average of both training runs", + (cleanMem + leakMem) / 2, userDataMeanMemory(userId), EPSILON); + // Population stddev of two values {a, b} is |a - b| / 2. + assertEquals("Stddev memory should reflect both training runs", + Math.abs(cleanMem - leakMem) / 2, userDataStdDevMemory(userId), EPSILON); + + // Remove one run: the row must remain (update branch, not delete) and its stats + // must be recomputed from the single remaining run. + assertTrainRun(_leakRunId, "false"); + assertEquals("Removing one of two training runs must not delete the UserData row", + baselineUserData + 1, userDataRowCount()); + assertEquals("Mean memory should be recomputed from the remaining run", + cleanMem, userDataMeanMemory(userId), EPSILON); + assertEquals("Stddev memory of a single remaining run should be zero", + 0.0, userDataStdDevMemory(userId), EPSILON); + } + finally + { + // Leave the training set empty for other tests (no-op if already removed). + postApi("trainRun", Map.of("runId", String.valueOf(_cleanRunId), "train", "false")); + postApi("trainRun", Map.of("runId", String.valueOf(_leakRunId), "train", "false")); + } + } + + @Test + public void testRunAccessIsContainerScoped() throws IOException, CommandException + { + // Run ids are global, so the actions that look a run up by id must reject a run that lives + // in another folder. Put a run in a subfolder, then from the PARENT folder confirm that + // every run-by-id action refuses it, and that the run is still reachable from its own folder. + final String subFolder = "CrossFolderAccessTest"; + final String subFolderPath = "/" + PROJECT_NAME + "/" + subFolder; + _containerHelper.createSubfolder(PROJECT_NAME, subFolder); + _containerHelper.enableModule(subFolderPath, "TestResults"); + postSampleXml("testresults/pc1-run-0116-failures.xml", subFolderPath); + + Connection conn = WebTestHelper.getRemoteApiConnection(); + SelectRowsCommand runsCmd = new SelectRowsCommand("testresults", "testruns"); + runsCmd.setColumns(List.of("id")); + SelectRowsResponse subRuns = runsCmd.execute(conn, subFolderPath); + assertEquals("Subfolder should have exactly 1 run", 1, subRuns.getRows().size()); + int subRunId = ((Number) subRuns.getRows().getFirst().get("id")).intValue(); + + // --- Negative: act on the subfolder's run by id from the PARENT folder (PROJECT_NAME) --- + + JSONObject del = postApi("deleteRun", Map.of("runId", String.valueOf(subRunId))); + assertFalse("Cross-folder deleteRun must fail: " + del, del.optBoolean("Success", true)); + assertEquals("run does not exist: " + subRunId, del.optString("error")); + + JSONObject train = postApi("trainRun", Map.of("runId", String.valueOf(subRunId), "train", "true")); + assertFalse("Cross-folder trainRun must fail: " + train, train.optBoolean("Success", true)); + assertEquals("run does not exist: " + subRunId, train.optString("error")); + + JSONObject flag = postApi("flagRun", Map.of("runId", String.valueOf(subRunId), "flag", "true")); + assertFalse("Cross-folder flagRun must fail: " + flag, flag.optBoolean("Success", true)); + assertEquals("run not found: " + subRunId, flag.optString("error")); + + assertNull("Cross-folder viewLog must not return content", + getApiString("viewLog", subRunId, "log")); + assertNull("Cross-folder viewXml must not return content", + getApiString("viewXml", subRunId, "xml")); + + // showRun in the parent folder shows the "enter run ID" prompt, not the subfolder run. + beginAt(WebTestHelper.buildRelativeUrl("testresults", PROJECT_NAME, "showRun", + Map.of("runId", String.valueOf(subRunId)))); + assertElementPresent(Locator.css("input[name='runId']")); + + // The run is untouched in its own folder (the cross-folder delete was a no-op there). + assertEquals("Subfolder run must be untouched", 1, + runsCmd.execute(conn, subFolderPath).getRows().size()); + + // --- Positive: the same run IS reachable from its own folder --- + // (deleteRun/trainRun/viewLog/viewXml in-folder are covered by the other tests, which all + // run in PROJECT_NAME and would fail if the container guard rejected same-folder access.) + JSONObject flagOwn = postApi("flagRun", + Map.of("runId", String.valueOf(subRunId), "flag", "true"), subFolderPath); + assertTrue("In-folder flagRun should succeed: " + flagOwn, flagOwn.optBoolean("Success", false)); + + // ShowFlaggedAction is folder-scoped: the flagged subfolder run shows on the subfolder's + // Flags page but not the parent's. + beginAt(WebTestHelper.buildRelativeUrl("testresults", subFolderPath, "showFlagged")); + assertElementPresent(Locator.tag("a").startsWith("id: " + subRunId + " /")); + beginAt(WebTestHelper.buildRelativeUrl("testresults", PROJECT_NAME, "showFlagged")); + assertElementNotPresent(Locator.tag("a").startsWith("id: " + subRunId + " /")); + } + /** * Verifies that child tables in the testresults schema are container-filtered * via a join to testruns. Creates a subfolder, posts a single run into it, @@ -1001,6 +1161,142 @@ private JSONObject postApi(String action, Map params, String con } } + /** + * Posts a sample XML run and returns the id of the run it created, identified as the + * single new testruns row for the given computer (compared against the runs present + * before the post). + */ + private int postAndGetNewRunId(String sampleDataRelativePath, String computerName) + { + Set before = runIdsForComputer(computerName); + postSampleXml(sampleDataRelativePath); + Set after = runIdsForComputer(computerName); + after.removeAll(before); + assertEquals("Expected exactly one new run for " + computerName, 1, after.size()); + return after.iterator().next(); + } + + /** + * Returns the set of testruns ids posted by the given computer in this container. + */ + private Set runIdsForComputer(String computerName) + { + return queryRuns().stream() + .filter(r -> computerName.equals(r.get("userid/username"))) + .map(r -> (Integer) r.get("id")) + .collect(Collectors.toSet()); + } + + /** + * Counts rows in a testresults child table that reference the given run via testrunid. + */ + private int childRowCount(String table, int runId) + { + try + { + Connection connection = WebTestHelper.getRemoteApiConnection(); + SelectRowsCommand cmd = new SelectRowsCommand("testresults", table); + cmd.addFilter(new Filter("testrunid", runId)); + return cmd.execute(connection, PROJECT_NAME).getRows().size(); + } + catch (Exception e) + { + throw new RuntimeException("Failed to count rows in " + table + " for run " + runId, e); + } + } + + /** + * Counts UserData (training stats) rows in this container. + */ + private int userDataRowCount() + { + try + { + Connection connection = WebTestHelper.getRemoteApiConnection(); + SelectRowsCommand cmd = new SelectRowsCommand("testresults", "userdata"); + return cmd.execute(connection, PROJECT_NAME).getRows().size(); + } + catch (Exception e) + { + throw new RuntimeException("Failed to count userdata rows", e); + } + } + + /** + * Posts trainRun for the given run and asserts it succeeded. {@code train} is "true" to add + * to the training set or "false" to remove. + */ + private void assertTrainRun(int runId, String train) + { + JSONObject resp = postApi("trainRun", Map.of("runId", String.valueOf(runId), "train", train)); + assertTrue("trainRun(" + runId + ", " + train + ") should succeed: " + resp, + resp.optBoolean("Success", false)); + } + + /** + * Returns the testresults.user id for the given computer name. + */ + private int getUserId(String computerName) + { + List> rows = selectRows("user", + new Filter("username", computerName), "id"); + assertEquals("Expected exactly one user row for " + computerName, 1, rows.size()); + return ((Number) rows.get(0).get("id")).intValue(); + } + + /** + * Returns the stored average managed memory for a run. + */ + private double runAverageMem(int runId) + { + List> rows = selectRows("testruns", + new Filter("id", runId), "averagemem"); + assertEquals("Expected exactly one testruns row for run " + runId, 1, rows.size()); + return ((Number) rows.get(0).get("averagemem")).doubleValue(); + } + + /** + * Returns the recomputed mean memory from the single UserData row for the given user. + */ + private double userDataMeanMemory(int userId) + { + List> rows = selectRows("userdata", + new Filter("userid", userId), "meanmemory"); + assertEquals("Expected exactly one userdata row for user " + userId, 1, rows.size()); + return ((Number) rows.get(0).get("meanmemory")).doubleValue(); + } + + /** + * Returns the recomputed population stddev of memory from the single UserData row for the + * given user. + */ + private double userDataStdDevMemory(int userId) + { + List> rows = selectRows("userdata", + new Filter("userid", userId), "stddevmemory"); + assertEquals("Expected exactly one userdata row for user " + userId, 1, rows.size()); + return ((Number) rows.get(0).get("stddevmemory")).doubleValue(); + } + + /** + * Runs a filtered SelectRows against a testresults query, returning the selected columns. + */ + private List> selectRows(String queryName, Filter filter, String... columns) + { + try + { + Connection connection = WebTestHelper.getRemoteApiConnection(); + SelectRowsCommand cmd = new SelectRowsCommand("testresults", queryName); + cmd.addFilter(filter); + cmd.setColumns(List.of(columns)); + return cmd.execute(connection, PROJECT_NAME).getRows(); + } + catch (Exception e) + { + throw new RuntimeException("Failed to query " + queryName, e); + } + } + // ------------------------------------------------------------------------- // Infrastructure // ------------------------------------------------------------------------- From a84efa0a87144bb9c6430f5c9393432f13d282f1 Mon Sep 17 00:00:00 2001 From: vagisha Date: Mon, 29 Jun 2026 10:38:11 -0700 Subject: [PATCH 2/3] Address reported security issues in the LINCS module (#652) - Path traversal (High) - DownloadCustomGCTReportAction: Validate the requested `fileName` with `FileUtil.isAllowedFileName` and confirm the resolved path stays inside the container's GCT directory. - Cross-container access (Medium / Low) - PSP job lookups: `LincsManager.getLincsPspJob` / `getLincsPspJobForRun` now require a `Container` and filter on it. The PSP-job detail/status/update actions can no longer reach jobs in other folders. All callers updated (3 controller actions, SubmitPspJobAction, and 2 LincsDataTable display columns). - Cleartext credential transmission (Low, CWE-319) - Clue/PSP server URI: Require `https` for the Clue/PSP server URI. Rejected at save time in `ManageLincsClueCredentials.validateCommand`, and as defense in depth in `LincsPspUtil.getPspEndpoint` before the API key is sent as a request header. The point-of-use check also blocks endpoints saved as `http://` before this fix. Added `` to `manageClueCredentials.jsp` so the save-time rejection is actually shown. - *Missing audit trail (Low) - credential / config changes: ManageLincsClueCredentials.handlePost` and `CromwellConfigAction.handlePost` now write an audit event (container and acting user, no secret values) when the PSP/Clue credentials or Cromwell config change. --------- Co-authored-by: Claude --- .../org/labkey/lincs/DocImportListener.java | 4 +- .../src/org/labkey/lincs/LincsController.java | 50 ++++++++++++++++--- .../src/org/labkey/lincs/LincsDataTable.java | 9 ++-- lincs/src/org/labkey/lincs/LincsManager.java | 18 ++++--- .../org/labkey/lincs/psp/LincsPspUtil.java | 7 +++ .../lincs/view/manageClueCredentials.jsp | 1 + 6 files changed, 69 insertions(+), 20 deletions(-) diff --git a/lincs/src/org/labkey/lincs/DocImportListener.java b/lincs/src/org/labkey/lincs/DocImportListener.java index 94721938..5701efbd 100644 --- a/lincs/src/org/labkey/lincs/DocImportListener.java +++ b/lincs/src/org/labkey/lincs/DocImportListener.java @@ -41,11 +41,11 @@ public void beforeRunDelete(ExpProtocol protocol, ExpRun run, User user) return; } - ITargetedMSRun tRun = TargetedMSService.get().getRunByFileName(run.getName(), run.getContainer()); + ITargetedMSRun tRun = TargetedMSService.get().getRunByFileName(run.getName(), c); if(tRun != null) { // Delete saved entries in lincs.lincspspjob table for this runId - LincsManager.get().deleteLincsPspJobsForRun(tRun.getId()); + LincsManager.get().deleteLincsPspJobsForRun(tRun.getId(), c); } // Get the file root for the container diff --git a/lincs/src/org/labkey/lincs/LincsController.java b/lincs/src/org/labkey/lincs/LincsController.java index e35d4007..453c484c 100644 --- a/lincs/src/org/labkey/lincs/LincsController.java +++ b/lincs/src/org/labkey/lincs/LincsController.java @@ -30,6 +30,8 @@ import org.labkey.api.action.SimpleErrorView; import org.labkey.api.action.SimpleViewAction; import org.labkey.api.action.SpringActionController; +import org.labkey.api.audit.AuditLogService; +import org.labkey.api.audit.ClientApiAuditProvider; import org.labkey.api.data.ActionButton; import org.labkey.api.data.ButtonBar; import org.labkey.api.data.Container; @@ -108,6 +110,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Set; import java.util.stream.Collectors; @@ -720,8 +723,22 @@ public ModelAndView getView(DownloadCustomGCTReportForm form, BindException erro return new SimpleErrorView(errors, false); } - Path gctDir = getGCTDir(getContainer()); - Path downloadFile = gctDir.resolve(form.getFileName()); + // isAllowedFileName returns null when the name is acceptable, an error message otherwise. + String fileNameError = FileUtil.isAllowedFileName(form.getFileName(), false); + if(fileNameError != null) + { + errors.reject(ERROR_MSG, "Invalid fileName '" + form.getFileName() + "': " + fileNameError); + return new SimpleErrorView(errors, false); + } + + Path gctDir = getGCTDir(getContainer()).normalize(); + Path downloadFile = gctDir.resolve(form.getFileName()).normalize(); + // Ensure the resolved path is still inside the GCT directory + if(!downloadFile.startsWith(gctDir)) + { + errors.reject(ERROR_MSG, "Invalid fileName '" + form.getFileName() + "'."); + return new SimpleErrorView(errors, false); + } if(!Files.exists(downloadFile)) { errors.reject(ERROR_MSG, "File does not exist '" + form.getFileName() + "'."); @@ -1011,15 +1028,28 @@ private static boolean isOrHasAncestor(Container container, String name) public static class ManageLincsClueCredentials extends FormViewAction { @Override - public void validateCommand(ClueCredentialsForm target, Errors errors) {} + public void validateCommand(ClueCredentialsForm form, Errors errors) + { + // The API key entered on this form will later be sent to this server URI as a request header, + // so reject a non-https URI at save time to keep the key out of clear text (CWE-319). + String uri = form.getServerUri(); + if (StringUtils.isNotBlank(uri) && !uri.trim().toLowerCase(Locale.ROOT).startsWith("https://")) + { + errors.reject(ERROR_MSG, "Clue/PSP Server URI must use https so the API key is not transmitted in clear text."); + } + } @Override public boolean handlePost(ClueCredentialsForm form, BindException errors) { WritablePropertyMap map = PropertyManager.getEncryptedStore().getWritableProperties(getContainer(), LINCS_CLUE_CREDENTIALS, true); - map.put(CLUE_SERVER_URI, form.getServerUri()); + map.put(CLUE_SERVER_URI, StringUtils.trim(form.getServerUri())); map.put(CLUE_API_KEY, form.getApiKey()); map.save(); + + // Record an audit event noting the container and the user who changed the credentials. + AuditLogService.get().addEvent(getUser(), + new ClientApiAuditProvider.ClientApiAuditEvent(getContainer(), "LINCS Clue/PSP server credentials updated.")); return true; } @@ -1106,6 +1136,10 @@ public boolean handlePost(CromwellConfigForm form, BindException errors) return false; } config.save(getContainer()); + + // Record an audit event noting the container and the user who changed the Cromwell configuration. + AuditLogService.get().addEvent(getUser(), + new ClientApiAuditProvider.ClientApiAuditEvent(getContainer(), "LINCS Cromwell configuration updated.")); return true; } @@ -1188,7 +1222,7 @@ public ModelAndView getView(LincsPspJobForm form, BindException errors) { int runId = form.getRunId(); - LincsPspJob pspJob = LincsManager.get().getLincsPspJobForRun(runId); + LincsPspJob pspJob = LincsManager.get().getLincsPspJobForRun(runId, getContainer()); if(pspJob == null) { errors.addError(new LabKeyError("Could not find a PSP job for runId: " + runId)); @@ -1333,7 +1367,7 @@ public ModelAndView getView(LincsPspJobForm form, BindException errors) { int jobId = form.getJobId(); - LincsPspJob pspJob = LincsManager.get().getLincsPspJob(jobId); + LincsPspJob pspJob = LincsManager.get().getLincsPspJob(jobId, getContainer()); if(pspJob == null) { errors.addError(new LabKeyError("Could not find a PSP job for id: " + jobId)); @@ -1411,7 +1445,7 @@ public boolean handlePost(LincsPspJobForm form, BindException errors) int jobId = form.getJobId(); _runId = form.getRunId(); - LincsPspJob pspJob = LincsManager.get().getLincsPspJob(jobId); + LincsPspJob pspJob = LincsManager.get().getLincsPspJob(jobId, getContainer()); if(pspJob == null) { errors.reject(ERROR_MSG, "Could not find a PSP job for id: " + jobId); @@ -1537,7 +1571,7 @@ public boolean handlePost(LincsPspJobForm form, BindException errors) LincsManager lincsManager = LincsManager.get(); - LincsPspJob oldPspJob = lincsManager.getLincsPspJobForRun(runId); + LincsPspJob oldPspJob = lincsManager.getLincsPspJobForRun(runId, container); LincsPspJob newPspJob = lincsManager.saveNewLincsPspJob(skylineRun, getUser()); ViewBackgroundInfo info = new ViewBackgroundInfo(container, getUser(), null); diff --git a/lincs/src/org/labkey/lincs/LincsDataTable.java b/lincs/src/org/labkey/lincs/LincsDataTable.java index aa9dfb88..108a8714 100644 --- a/lincs/src/org/labkey/lincs/LincsDataTable.java +++ b/lincs/src/org/labkey/lincs/LincsDataTable.java @@ -20,6 +20,7 @@ import org.jetbrains.annotations.Nullable; import org.labkey.api.analytics.AnalyticsService; import org.labkey.api.data.ColumnInfo; +import org.labkey.api.data.Container; import org.labkey.api.data.DataColumn; import org.labkey.api.data.RenderContext; import org.labkey.api.data.TableInfo; @@ -154,7 +155,7 @@ public void renderGridCellContents(RenderContext ctx, HtmlWriter out) out.write("NO_RUN_ID"); return; } - LincsPspJob pspJob = LincsManager.get().getLincsPspJobForRun(runId); + LincsPspJob pspJob = LincsManager.get().getLincsPspJobForRun(runId, getContainer()); if(pspJob == null) { out.write("PSP job not found for runId: " + runId); @@ -374,7 +375,7 @@ public void renderGridCellContents(RenderContext ctx, HtmlWriter out) String downloadFileName = getBaseName(fileName); String extension = LincsModule.getExt(getLevel()); downloadFileName = downloadFileName + extension; - if(!fileAvailable(runId, downloadFileName)) + if(!fileAvailable(runId, downloadFileName, ctx.getContainer())) { out.write("NOT AVAILABLE"); return; @@ -403,9 +404,9 @@ private void renderGridCell(HtmlWriter out, String analyticsScript, String downl ).appendTo(out); } - private boolean fileAvailable(Integer runId, String downloadFileName) + private boolean fileAvailable(Integer runId, String downloadFileName, Container container) { - LincsPspJob job = LincsManager.get().getLincsPspJobForRun(runId); + LincsPspJob job = LincsManager.get().getLincsPspJobForRun(runId, container); if(job != null) { if(getLevel() == LincsModule.LincsLevel.Two && job.isLevel2Done()) diff --git a/lincs/src/org/labkey/lincs/LincsManager.java b/lincs/src/org/labkey/lincs/LincsManager.java index 62926ccc..b971772c 100644 --- a/lincs/src/org/labkey/lincs/LincsManager.java +++ b/lincs/src/org/labkey/lincs/LincsManager.java @@ -164,24 +164,30 @@ public void updatePipelineJobId(LincsPspJob job) job.getPipelineJobId(), job.getId()); } - public LincsPspJob getLincsPspJobForRun(int runId) + public LincsPspJob getLincsPspJobForRun(int runId, Container container) { // Get the most recent PSP job details Sort sort = new Sort(); sort.appendSortColumn(FieldKey.fromParts("Modified"), Sort.SortDirection.DESC, true); - TableSelector ts = new TableSelector(getTableInfoLincsPspJob(), new SimpleFilter(FieldKey.fromParts("RunId"), runId), sort); + SimpleFilter filter = SimpleFilter.createContainerFilter(container); + filter.addCondition(FieldKey.fromParts("RunId"), runId); + TableSelector ts = new TableSelector(getTableInfoLincsPspJob(), filter, sort); ts.setMaxRows(1); return ts.getObject(LincsPspJob.class); } - public LincsPspJob getLincsPspJob(int id) + public LincsPspJob getLincsPspJob(int id, Container container) { - return new TableSelector(getTableInfoLincsPspJob(), new SimpleFilter(FieldKey.fromParts("Id"), id), null).getObject(LincsPspJob.class); + SimpleFilter filter = SimpleFilter.createContainerFilter(container); + filter.addCondition(FieldKey.fromParts("Id"), id); + return new TableSelector(getTableInfoLincsPspJob(), filter, null).getObject(LincsPspJob.class); } - public void deleteLincsPspJobsForRun(long runId) + public void deleteLincsPspJobsForRun(long runId, Container container) { - Table.delete(getTableInfoLincsPspJob(), new SimpleFilter(FieldKey.fromParts("runId"), runId)); + SimpleFilter filter = SimpleFilter.createContainerFilter(container); + filter.addCondition(FieldKey.fromParts("runId"), runId); + Table.delete(getTableInfoLincsPspJob(), filter); } public void deleteLincsPspJob(LincsPspJob job) diff --git a/lincs/src/org/labkey/lincs/psp/LincsPspUtil.java b/lincs/src/org/labkey/lincs/psp/LincsPspUtil.java index 2737df4f..d1d3fe5b 100644 --- a/lincs/src/org/labkey/lincs/psp/LincsPspUtil.java +++ b/lincs/src/org/labkey/lincs/psp/LincsPspUtil.java @@ -26,6 +26,7 @@ import java.net.HttpURLConnection; import java.net.URL; import java.nio.charset.StandardCharsets; +import java.util.Locale; public class LincsPspUtil { @@ -55,6 +56,12 @@ public static PspEndpoint getPspEndpoint(Container container) throws LincsPspExc { throw new LincsPspException("Could not find PSP API Key in the saved properties."); } + // The API key retrieved here is later sent to the endpoint URL as a request header by the callers. + // Refuse to return a non-https endpoint so the key is never transmitted in clear text (CWE-319). + if(!pspUrl.trim().toLowerCase(Locale.ROOT).startsWith("https://")) + { + throw new LincsPspException("PSP endpoint URL must use https so the API key is not transmitted in clear text."); + } return new PspEndpoint(pspUrl, pspApiKey); } diff --git a/lincs/src/org/labkey/lincs/view/manageClueCredentials.jsp b/lincs/src/org/labkey/lincs/view/manageClueCredentials.jsp index 905af218..9ec4360f 100644 --- a/lincs/src/org/labkey/lincs/view/manageClueCredentials.jsp +++ b/lincs/src/org/labkey/lincs/view/manageClueCredentials.jsp @@ -11,6 +11,7 @@ LincsController.ClueCredentialsForm form = ((JspView) HttpView.currentView()).getModelBean(); %> +
From d3d8590874801b065b20ce92391be6a5b272bdb2 Mon Sep 17 00:00:00 2001 From: Ankur Juneja Date: Mon, 29 Jun 2026 15:22:48 -0700 Subject: [PATCH 3/3] nextflow security fixes (#653) --- .../labkey/nextflow/NextFlowController.java | 33 +++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/nextflow/src/org/labkey/nextflow/NextFlowController.java b/nextflow/src/org/labkey/nextflow/NextFlowController.java index f3adaef1..25075ab2 100644 --- a/nextflow/src/org/labkey/nextflow/NextFlowController.java +++ b/nextflow/src/org/labkey/nextflow/NextFlowController.java @@ -45,6 +45,7 @@ import org.springframework.web.servlet.ModelAndView; import java.io.File; +import java.nio.file.InvalidPathException; import java.util.Arrays; import java.util.List; @@ -223,6 +224,10 @@ public ModelAndView getView(EnabledForm form, boolean reshow, BindException erro @Override public boolean handlePost(EnabledForm form, BindException errors) { + if (!getUser().hasSiteAdminPermission()) + { + throw new UnauthorizedException(); + } NextFlowManager.get().saveEnabledState(getContainer(), form.getEnabled()); return true; } @@ -257,6 +262,10 @@ public void validateCommand(AnalyzeForm o, Errors errors) { errors.reject(ERROR_MSG, "NextFlow is not enabled"); } + else if (NextFlowManager.get().getConfiguration() == null) + { + errors.reject(ERROR_MSG, "NextFlow has not been configured"); + } } @Override @@ -278,7 +287,7 @@ public ModelAndView getView(AnalyzeForm o, boolean b, BindException errors) } NextFlowConfiguration config = NextFlowManager.get().getConfiguration(); - if (config.getNextFlowConfigFilePath() != null) + if (config != null && config.getNextFlowConfigFilePath() != null) { File configDir = new File(config.getNextFlowConfigFilePath()); if (configDir.isDirectory()) @@ -311,8 +320,28 @@ public boolean handlePost(AnalyzeForm form, BindException errors) throws Excepti } NextFlowConfiguration config = NextFlowManager.get().getConfiguration(); + if (config == null || config.getNextFlowConfigFilePath() == null) + { + errors.reject(ERROR_MSG, "NextFlow has not been configured"); + return false; + } + if (StringUtils.isBlank(form.getConfigFile())) + { + errors.reject(ERROR_MSG, "No config file specified"); + return false; + } File configDir = new File(config.getNextFlowConfigFilePath()); - File configFile = FileUtil.appendPath(configDir, Path.parse(form.getConfigFile())); + File configFile; + try + { + // appendPath normalizes and enforces that the resolved path stays within configDir, rejecting traversal + configFile = FileUtil.appendPath(configDir, Path.parse(form.getConfigFile())); + } + catch (InvalidPathException e) + { + errors.reject(ERROR_MSG, "Invalid config file"); + return false; + } if (!configFile.exists()) { errors.reject(ERROR_MSG, "Config file does not exist");