From f9b504843bbcb10d3bd1f78db4c7897ddf22164c Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Sun, 28 Jun 2026 15:49:49 -0700 Subject: [PATCH 1/3] Addresses reported security issues in the LINCS module * DownloadCustomGCTReportAction.getView: validates the requested fileName and confines the resolved path to the container's GCT directory before streaming and deleting it, closing a path-traversal arbitrary file read/delete reachable by any Reader. * LincsManager.getLincsPspJob/getLincsPspJobForRun: requires a Container and filters on it, so PSP-job detail/status/update actions can no longer reach jobs in other folders by guessing an id (cross-container IDOR). Updated all callers. * Requires https for the Clue/PSP server URI, rejects both at save (ManageLincsClueCredentials) and at use (LincsPspUtil.getPspEndpoint), so the API key is never sent in clear text (CWE-319, cleartext transmission of sensitive data). * ManageLincsClueCredentials and CromwellConfigAction: write an audit event (no secret values) when credentials or config change. Identified by a security review of the MacCoss lab modules. Co-Authored-By: Claude --- .../src/org/labkey/lincs/LincsController.java | 51 ++++++++++++++++--- .../src/org/labkey/lincs/LincsDataTable.java | 9 ++-- lincs/src/org/labkey/lincs/LincsManager.java | 12 +++-- .../org/labkey/lincs/psp/LincsPspUtil.java | 8 +++ 4 files changed, 64 insertions(+), 16 deletions(-) diff --git a/lincs/src/org/labkey/lincs/LincsController.java b/lincs/src/org/labkey/lincs/LincsController.java index e35d4007..eddcf999 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,23 @@ 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(); + // Defense in depth: ensure the resolved path is still inside the GCT directory before + // reading or deleting it, so a traversal sequence cannot escape the container's folder. + 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 +1029,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 is sent to this server as a request header. Require https so the key is + // never transmitted in 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 +1137,10 @@ public boolean handlePost(CromwellConfigForm form, BindException errors) return false; } config.save(getContainer()); + + // Record an audit event noting who changed the Cromwell configuration. + AuditLogService.get().addEvent(getUser(), + new ClientApiAuditProvider.ClientApiAuditEvent(getContainer(), "LINCS Cromwell configuration updated.")); return true; } @@ -1188,7 +1223,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 +1368,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 +1446,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 +1572,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..4a2d8508 100644 --- a/lincs/src/org/labkey/lincs/LincsManager.java +++ b/lincs/src/org/labkey/lincs/LincsManager.java @@ -164,19 +164,23 @@ 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) diff --git a/lincs/src/org/labkey/lincs/psp/LincsPspUtil.java b/lincs/src/org/labkey/lincs/psp/LincsPspUtil.java index 2737df4f..b43189a7 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,13 @@ public static PspEndpoint getPspEndpoint(Container container) throws LincsPspExc { throw new LincsPspException("Could not find PSP API Key in the saved properties."); } + // The API key is sent to this URL as a request header. Refuse to use a non-https endpoint so the + // key is never transmitted in clear text (CWE-319). This also guards configurations that were saved + // with an http:// URL before save-time validation was added. + 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); } From 4267fd7bc26b8000cb1394c7d14cece004492485 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Sun, 28 Jun 2026 17:07:42 -0700 Subject: [PATCH 2/3] * manageClueCredentials.jsp: added so the https-required rejection from ManageLincsClueCredentials.validateCommand is shown. * Clarified the comments on the PSP endpoint https check (LincsPspUtil) and the Cromwell config audit event (LincsController). No behavior change. --- lincs/src/org/labkey/lincs/LincsController.java | 9 ++++----- lincs/src/org/labkey/lincs/psp/LincsPspUtil.java | 5 ++--- .../src/org/labkey/lincs/view/manageClueCredentials.jsp | 1 + 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/lincs/src/org/labkey/lincs/LincsController.java b/lincs/src/org/labkey/lincs/LincsController.java index eddcf999..453c484c 100644 --- a/lincs/src/org/labkey/lincs/LincsController.java +++ b/lincs/src/org/labkey/lincs/LincsController.java @@ -733,8 +733,7 @@ public ModelAndView getView(DownloadCustomGCTReportForm form, BindException erro Path gctDir = getGCTDir(getContainer()).normalize(); Path downloadFile = gctDir.resolve(form.getFileName()).normalize(); - // Defense in depth: ensure the resolved path is still inside the GCT directory before - // reading or deleting it, so a traversal sequence cannot escape the container's folder. + // Ensure the resolved path is still inside the GCT directory if(!downloadFile.startsWith(gctDir)) { errors.reject(ERROR_MSG, "Invalid fileName '" + form.getFileName() + "'."); @@ -1031,8 +1030,8 @@ public static class ManageLincsClueCredentials extends FormViewAction) HttpView.currentView()).getModelBean(); %> + From 289226aeaa3fbec6a514fad292b01a0857362d60 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Sun, 28 Jun 2026 21:05:43 -0700 Subject: [PATCH 3/3] Container-scoped LincsManager.deleteLincsPspJobsForRun --- lincs/src/org/labkey/lincs/DocImportListener.java | 4 ++-- lincs/src/org/labkey/lincs/LincsManager.java | 6 ++++-- 2 files changed, 6 insertions(+), 4 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/LincsManager.java b/lincs/src/org/labkey/lincs/LincsManager.java index 4a2d8508..b971772c 100644 --- a/lincs/src/org/labkey/lincs/LincsManager.java +++ b/lincs/src/org/labkey/lincs/LincsManager.java @@ -183,9 +183,11 @@ public LincsPspJob getLincsPspJob(int id, Container container) 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)