From 05c5d883575f3b90096b12436579695ee321fa0f Mon Sep 17 00:00:00 2001 From: cnathe Date: Mon, 27 Jul 2026 16:26:58 -0500 Subject: [PATCH 1/4] Add validateScriptLocation() call to make sure that assay transform script path is a child of the container's @scripts dir --- .../api/assay/AbstractAssayProvider.java | 49 +++++++++++++++++++ .../filecontent/FileContentServiceImpl.java | 11 +++++ 2 files changed, 60 insertions(+) diff --git a/api/src/org/labkey/api/assay/AbstractAssayProvider.java b/api/src/org/labkey/api/assay/AbstractAssayProvider.java index b99bf13b203..3340330494c 100644 --- a/api/src/org/labkey/api/assay/AbstractAssayProvider.java +++ b/api/src/org/labkey/api/assay/AbstractAssayProvider.java @@ -102,6 +102,7 @@ import org.labkey.api.security.User; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.settings.AppProps; +import org.labkey.api.settings.OptionalFeatureService; import org.labkey.api.study.Dataset; import org.labkey.api.study.TimepointType; import org.labkey.api.study.assay.ParticipantVisitResolverType; @@ -112,6 +113,7 @@ import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.Pair; import org.labkey.api.util.StringUtilsLabKey; +import org.labkey.api.util.URIUtil; import org.labkey.api.util.logging.LogHelper; import org.labkey.api.view.ActionURL; import org.labkey.api.view.DetailsView; @@ -1271,6 +1273,8 @@ public Pair> setValidationAndAnalysisS String ext = FileUtil.getExtension(scriptFile); if (scriptFile.isFile() && ext != null) { + validateScriptLocation(protocol.getContainer(), scriptFile, validationErrors); + ScriptEngine engine = LabKeyScriptEngineManager.get().getEngineByExtension(protocol.getContainer(), ext, LabKeyScriptEngineManager.EngineContext.pipeline); if (engine != null) { @@ -1326,6 +1330,51 @@ public Pair> setValidationAndAnalysisS /** Property name suffix used when storing transform scripts on assay protocol objects */ public static final String TRANSFORM_SCRIPT_PROPERTY_NAME = "TransformScript"; + /** + * Deprecated feature flag. When enabled, transform scripts may live at arbitrary file system paths instead of + * being confined to the assay design container's @scripts directory. Registered by AssayModule. + */ + public static final String DEPRECATED_ARBITRARY_TRANSFORM_SCRIPT_PATHS = "arbitraryTransformScriptPaths"; + + /** + * GitHub Issue #159: transform scripts configured on an assay design must reside in that design's container's + * {@code @scripts} directory, which only platform developers can write to. Module-provided scripts are supplied + * via {@link Scope#ASSAY_TYPE} and never reach this code path, so every script persisted with a design must be + * under {@code @scripts}. + */ + private void validateScriptLocation(Container container, FileLike scriptFile, ValidationException validationErrors) + { + if (OptionalFeatureService.get().isFeatureEnabled(DEPRECATED_ARBITRARY_TRANSFORM_SCRIPT_PATHS)) + return; + + FileContentService fileContentService = FileContentService.get(); + if (fileContentService == null) + { + LOG.warn("FileContentService is not available; unable to validate the location of transform script '{}'.", scriptFile.toNioPathForRead()); + return; + } + + // Returns null when there's no usable file root and for cloud roots, which have no @scripts directory at all + // (see ScriptsResourceProvider). + java.nio.file.Path scriptsDir = fileContentService.getFileRootPath(container, FileContentService.ContentType.scripts); + if (scriptsDir == null) + { + LOG.debug("Skipping the {} location check for transform script '{}': unable to resolve the directory for container '{}'.", + FileContentService.SCRIPTS_LINK, scriptFile.toNioPathForRead(), container.getPath()); + return; + } + + // Normalize both sides (strip "." and "..", correct case on case-insensitive file systems) before comparing + File scriptsDirFile = FileUtil.getAbsoluteCaseSensitiveFile(scriptsDir.toFile()); + File normalizedScriptFile = FileUtil.getAbsoluteCaseSensitiveFile(scriptFile.toNioPathForRead().toFile()); + + if (!URIUtil.isDescendant(scriptsDirFile.toURI(), normalizedScriptFile.toURI())) + { + validationErrors.addError(new SimpleValidationError("The transform script must exist within this folder's " + + FileContentService.SCRIPTS_LINK + " directory.", null, SEVERITY.ERROR, new HelpTopic("programmaticQC"))); + } + } + @NotNull @Override public List getValidationAndAnalysisScripts(ExpProtocol protocol, Scope scope) diff --git a/filecontent/src/org/labkey/filecontent/FileContentServiceImpl.java b/filecontent/src/org/labkey/filecontent/FileContentServiceImpl.java index d39c70821b1..2fc3f5dd8f5 100644 --- a/filecontent/src/org/labkey/filecontent/FileContentServiceImpl.java +++ b/filecontent/src/org/labkey/filecontent/FileContentServiceImpl.java @@ -259,6 +259,10 @@ public List getContainersForFilePath(java.nio.file.Path path) java.nio.file.Path dir = getFileRootPath(c); return dir != null ? dir.resolve(folderName).toFile() : null; + case scripts: + java.nio.file.Path scriptsRoot = getFileRootPath(c, type); + return scriptsRoot != null ? scriptsRoot.toFile() : null; + case pipeline: PipeRoot root = PipelineService.get().findPipelineRoot(c); return root != null ? root.getRootPath() : null; @@ -278,6 +282,13 @@ public List getContainersForFilePath(java.nio.file.Path path) fileRootPath = fileRootPath.resolve(getFolderName(type)); return fileRootPath; + case scripts: + // Return null rather than the bare file root so callers can't mistake the root itself for the @scripts directory. + java.nio.file.Path scriptsRootPath = getFileRootPath(c); + if (null == scriptsRootPath || FileUtil.hasCloudScheme(scriptsRootPath)) + return null; + return scriptsRootPath.resolve(getFolderName(type)); + case pipeline: PipeRoot root = PipelineService.get().findPipelineRoot(c); return root != null ? root.getRootNioPath() : null; From db9af9a6faa249aea4c48d90e26c101592c1fe0e Mon Sep 17 00:00:00 2001 From: cnathe Date: Mon, 27 Jul 2026 16:27:35 -0500 Subject: [PATCH 2/4] Add optional feature flag to "turn off" the @scripts dir path validation check --- assay/src/org/labkey/assay/AssayModule.java | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/assay/src/org/labkey/assay/AssayModule.java b/assay/src/org/labkey/assay/AssayModule.java index 7bd5b425b9f..64adeea58f8 100644 --- a/assay/src/org/labkey/assay/AssayModule.java +++ b/assay/src/org/labkey/assay/AssayModule.java @@ -19,6 +19,7 @@ import jakarta.servlet.ServletContext; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.labkey.api.assay.AbstractAssayProvider; import org.labkey.api.assay.AbstractTsvAssayProvider; import org.labkey.api.assay.AssayBatchDomainKind; import org.labkey.api.assay.AssayProvider; @@ -68,6 +69,8 @@ import org.labkey.api.security.User; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.roles.RoleManager; +import org.labkey.api.settings.OptionalFeatureFlag; +import org.labkey.api.settings.OptionalFeatureService; import org.labkey.api.usageMetrics.UsageMetricsService; import org.labkey.api.util.ContextListener; import org.labkey.api.util.GUID; @@ -190,6 +193,14 @@ protected void init() PropertyService.get().registerDomainKind(new PlateMetadataDomainKind()); CacheManager.addListener(PlateCache::clearCache); + + OptionalFeatureService.get().addFeatureFlag(new OptionalFeatureFlag( + AbstractAssayProvider.DEPRECATED_ARBITRARY_TRANSFORM_SCRIPT_PATHS, + "Allow assay transform scripts at arbitrary file system paths", + "Transform scripts configured on an assay design are normally required to reside in that design's container's @scripts directory, which only platform developers can write to. Enable this to allow assay designs to reference scripts anywhere on the server's file system. This option will be removed in a future release of LabKey Server.", + false, + false, + OptionalFeatureService.FeatureType.Deprecated)); } @Override From c4bde42921d9232514fb7f88e1f9161f0e203839 Mon Sep 17 00:00:00 2001 From: cnathe Date: Mon, 27 Jul 2026 16:28:03 -0500 Subject: [PATCH 3/4] Metric for approximate count of assay transform script paths that are not in an @scripts dir path --- .../src/org/labkey/experiment/ExperimentModule.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/experiment/src/org/labkey/experiment/ExperimentModule.java b/experiment/src/org/labkey/experiment/ExperimentModule.java index b81ced637c0..5ea2011db80 100644 --- a/experiment/src/org/labkey/experiment/ExperimentModule.java +++ b/experiment/src/org/labkey/experiment/ExperimentModule.java @@ -660,6 +660,18 @@ public void containerDeleted(Container c, User user) "%\"" + DataTransformService.TransformOperation.INSERT + "\"%" ).getObject(Long.class)); + // GitHub Issue #159: approximate count of assay designs whose transform scripts aren't in an + // @scripts directory. This is done in SQL instead of parsing each script so there are some caveats + // for cases that aren't counted: + // 1. Any path containing "@scripts" matches, so a script in a sibling container's @scripts looks compliant here but is rejected on save. + // 2. Undercounts designs with a MIX of compliant and non-compliant scripts + assayMetrics.put("protocolsWithTransformScriptNotInScriptsDirCount", new SqlSelector(schema, + "SELECT COUNT(*) FROM exp.protocol EP JOIN exp.objectPropertiesView OP ON EP.lsid = OP.objecturi WHERE OP.name = ? AND status = ? AND OP.stringvalue NOT LIKE ?", + AbstractAssayProvider.TRANSFORM_SCRIPT_PROPERTY_NAME, + ExpProtocol.Status.Active.toString(), + "%" + FileContentService.SCRIPTS_LINK + "%" + ).getObject(Long.class)); + assayMetrics.put("standardAssayWithPlateSupportCount", new SqlSelector(schema, "SELECT COUNT(*) FROM exp.protocol EP JOIN exp.objectPropertiesView OP ON EP.lsid = OP.objecturi WHERE OP.name = 'PlateMetadata' AND floatValue = 1").getObject(Long.class)); SQLFragment runsWithPlateSQL = new SQLFragment(""" SELECT COUNT(*) FROM exp.experimentrun r From b457891e2a81d2a10be450fe2dfa3e96356d3f14 Mon Sep 17 00:00:00 2001 From: cnathe Date: Thu, 30 Jul 2026 11:30:23 -0500 Subject: [PATCH 4/4] claude CR feedback --- api/src/org/labkey/api/assay/AbstractAssayProvider.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/org/labkey/api/assay/AbstractAssayProvider.java b/api/src/org/labkey/api/assay/AbstractAssayProvider.java index 3340330494c..e244c6aff74 100644 --- a/api/src/org/labkey/api/assay/AbstractAssayProvider.java +++ b/api/src/org/labkey/api/assay/AbstractAssayProvider.java @@ -1359,7 +1359,7 @@ private void validateScriptLocation(Container container, FileLike scriptFile, Va java.nio.file.Path scriptsDir = fileContentService.getFileRootPath(container, FileContentService.ContentType.scripts); if (scriptsDir == null) { - LOG.debug("Skipping the {} location check for transform script '{}': unable to resolve the directory for container '{}'.", + LOG.warn("Skipping the {} location check for transform script '{}': unable to resolve the directory for container '{}'.", FileContentService.SCRIPTS_LINK, scriptFile.toNioPathForRead(), container.getPath()); return; }