From 6b66fae73f57f009dc5b1f9eeaf00ad505cf1bc7 Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Tue, 9 Jan 2024 12:54:48 -0500 Subject: [PATCH 1/3] Cache GroovySourceFileAllowlist return values GroovySourceFileAllowlist can be consulted many times, but the allowed files are effectively static and just depend on which plugins are installed. Once a file has been allowed once, we don't need to check all of the allowlists again. --- .../cps/GroovySourceFileAllowlist.java | 26 ++++++++++++++----- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovySourceFileAllowlist.java b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovySourceFileAllowlist.java index b9c330256..4d6af3491 100644 --- a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovySourceFileAllowlist.java +++ b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovySourceFileAllowlist.java @@ -37,10 +37,11 @@ import java.net.URL; import java.nio.charset.StandardCharsets; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collections; import java.util.Enumeration; import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; import java.util.logging.Level; import java.util.logging.Logger; import jenkins.util.SystemProperties; @@ -138,12 +139,7 @@ public Enumeration getResources(String name) throws IOException { private static boolean isAllowed(URL url) { String urlString = url.toString(); - for (GroovySourceFileAllowlist allowlist : GroovySourceFileAllowlist.all()) { - if (allowlist.isAllowed(urlString)) { - return true; - } - } - return false; + return ExtensionList.lookupSingleton(AllowedGroovyResourcesCache.class).isAllowed(urlString); } private static boolean endsWithIgnoreCase(String value, String suffix) { @@ -152,6 +148,22 @@ private static boolean endsWithIgnoreCase(String value, String suffix) { } } + @Extension + public static class AllowedGroovyResourcesCache { + private final Map cache = new ConcurrentHashMap<>(); + + public boolean isAllowed(String groovySourceFileUrl) { + return cache.computeIfAbsent(groovySourceFileUrl, url -> { + for (GroovySourceFileAllowlist allowlist : GroovySourceFileAllowlist.all()) { + if (allowlist.isAllowed(url)) { + return true; + } + } + return false; + }); + } + } + /** * Allows Groovy source files used to implement DSLs in plugins that were created before * {@link GroovySourceFileAllowlist} was introduced. From 7ad54131e5395311c7f945aa66e4b77eb4ea6285 Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Tue, 9 Jan 2024 12:58:08 -0500 Subject: [PATCH 2/3] Remove entries from default-allowlist that are unrelated to WithScriptDescriptor.WithScriptAllowlist --- .../cps/GroovySourceFileAllowlist/default-allowlist | 6 ------ 1 file changed, 6 deletions(-) diff --git a/plugin/src/main/resources/org/jenkinsci/plugins/workflow/cps/GroovySourceFileAllowlist/default-allowlist b/plugin/src/main/resources/org/jenkinsci/plugins/workflow/cps/GroovySourceFileAllowlist/default-allowlist index 132684793..78619ea0f 100644 --- a/plugin/src/main/resources/org/jenkinsci/plugins/workflow/cps/GroovySourceFileAllowlist/default-allowlist +++ b/plugin/src/main/resources/org/jenkinsci/plugins/workflow/cps/GroovySourceFileAllowlist/default-allowlist @@ -1,10 +1,8 @@ # This list is ordered from most popular to least popular plugin to minimize performance impact. # pipeline-model-definition -/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy /org/jenkinsci/plugins/pipeline/modeldefinition/agent/impl/AnyScript.groovy /org/jenkinsci/plugins/pipeline/modeldefinition/agent/impl/LabelScript.groovy /org/jenkinsci/plugins/pipeline/modeldefinition/agent/impl/NoneScript.groovy -/org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/AbstractChangelogConditionalScript.groovy /org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/AllOfConditionalScript.groovy /org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/AnyOfConditionalScript.groovy /org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/BranchConditionalScript.groovy @@ -18,11 +16,7 @@ /org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/NotConditionalScript.groovy /org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/TagConditionalScript.groovy /org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/TriggeredByConditionalScript.groovy -# pipeline-model-extensions -/org/jenkinsci/plugins/pipeline/modeldefinition/agent/CheckoutScript.groovy # docker-workflow -/org/jenkinsci/plugins/docker/workflow/Docker.groovy -/org/jenkinsci/plugins/docker/workflow/declarative/AbstractDockerPipelineScript.groovy /org/jenkinsci/plugins/docker/workflow/declarative/DockerPipelineFromDockerfileScript.groovy /org/jenkinsci/plugins/docker/workflow/declarative/DockerPipelineScript.groovy # kubernetes From 777deff649dbd81b45f63bbdd4d341bb7a473824 Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Wed, 17 Jan 2024 16:46:43 -0500 Subject: [PATCH 3/3] Do not cache negative results in GroovySourceFileAllowlist.AllowedGroovyResourcesCache --- .../plugins/workflow/cps/GroovySourceFileAllowlist.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovySourceFileAllowlist.java b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovySourceFileAllowlist.java index 4d6af3491..a19b9b6d8 100644 --- a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovySourceFileAllowlist.java +++ b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovySourceFileAllowlist.java @@ -149,18 +149,23 @@ private static boolean endsWithIgnoreCase(String value, String suffix) { } @Extension + @SuppressFBWarnings(value = "NP_BOOLEAN_RETURN_NULL", justification = "intentionally not caching negative results") public static class AllowedGroovyResourcesCache { private final Map cache = new ConcurrentHashMap<>(); public boolean isAllowed(String groovySourceFileUrl) { - return cache.computeIfAbsent(groovySourceFileUrl, url -> { + Boolean cachedResult = cache.computeIfAbsent(groovySourceFileUrl, url -> { for (GroovySourceFileAllowlist allowlist : GroovySourceFileAllowlist.all()) { if (allowlist.isAllowed(url)) { return true; } } - return false; + // In practice we should only get here with files that are allowed, so we don't cache negative + // results in case it would cause problems with unusual Pipelines that reference Groovy source + // files directly in combination with dynamically installed plugins. + return null; }); + return Boolean.TRUE.equals(cachedResult); } }