Add way to bundle libraries without exposing them to plugins#26600
Add way to bundle libraries without exposing them to plugins#26600daniel-beck wants to merge 11 commits into
Conversation
comment-ops-bot
Bot
added
the
major-rfe
There was a problem hiding this comment.
Pull request overview
Adds a new mechanism for bundling “core-only” libraries inside Jenkins so core can use third-party dependencies without exposing them on plugin classpaths, and demonstrates the mechanism by sanitizing update-site plugin excerpts using OWASP Java HTML Sanitizer.
Changes:
- Introduce
CoreLibClassLoader+Jenkins#getCoreLibrary(...)to load internal services fromWEB-INF/core-lib/viaServiceLoader. - Add a new
core-libsMaven module (jenkins-core-libs) providing internal service implementations (initiallyPluginExcerptSanitizer). - Update WAR packaging to copy core-libs artifacts into
WEB-INF/core-lib/and prevent duplication intoWEB-INF/lib, plus add tests validating classloader isolation and sanitization behavior.
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
core/src/main/java/jenkins/model/Jenkins.java |
Adds core-libs classloader initialization, service lookup API, and shutdown cleanup. |
core/src/main/java/jenkins/core/CoreLibClassLoader.java |
Implements isolated classloader loading jars from WEB-INF/core-lib/. |
core/src/main/java/jenkins/core/PluginExcerptSanitizer.java |
Defines internal service interface for excerpt sanitization. |
core/src/main/java/hudson/model/UpdateSite.java |
Sanitizes plugin excerpt field via core-libs service (fallback to escaping). |
core-libs/pom.xml |
New module that bundles OWASP sanitizer and hosts service implementations. |
core-libs/src/main/java/jenkins/security/OwaspPluginExcerptSanitizer.java |
OWASP-based sanitizer implementation exposed via META-INF services. |
core-libs/src/main/resources/META-INF/bundled-libraries.properties |
Declares artifactIds expected to be copied into WEB-INF/core-lib/. |
core-libs/validate-dependencies.groovy |
Build-time validation that declared bundled libs match actual dependencies. |
war/pom.xml |
Packages core-libs + transitives into WEB-INF/core-lib/, excludes from WEB-INF/lib, adds Groovy helper + dev copy-to-src. |
war/extract-core-lib-artifact-ids.groovy |
Extracts bundled library artifactIds from the core-libs JAR to drive WAR packaging config. |
pom.xml |
Adds core-libs module + cleans generated war/.../WEB-INF/core-lib artifacts. |
test/pom.xml |
Excludes jenkins-core-libs from test classpath to validate isolation. |
test/src/test/java/jenkins/core/CoreLibClassLoaderTest.java |
Verifies sanitizer availability + excerpt sanitization behavior. |
test/src/test/java/jenkins/core/CoreLibIsolationTest.java |
Tests core access vs plugin isolation for core-libs. |
test/src/test/java/jenkins/core/CoreLibIsolationRealTest.java |
RealJenkinsExtension test ensuring synthetic plugin cannot load OWASP classes. |
test/src/test/java/jenkins/core/corelib_test_plugin/ClassLoaderProbe.java |
Helper class for the synthetic plugin isolation test. |
.gitignore |
Ignores generated war/src/main/webapp/WEB-INF/core-lib. |
.idea/encodings.xml |
Adds IntelliJ encoding entries for the new core-libs module. |
Files not reviewed (1)
- .idea/encodings.xml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| project.properties['core-lib-artifact-ids'] = artifactIds | ||
|
|
||
| // Generate regex pattern for packaging excludes (include jenkins-core-libs itself + all artifact IDs) | ||
| def allIds = (['jenkins-core-libs'] + props.keySet()).join('|') |
There was a problem hiding this comment.
core-lib-packaging-excludes is built by concatenating artifactIds into a regex alternation, but the artifactIds are not regex-escaped. If a future bundled artifactId contains regex metacharacters (e.g., ., +), the pattern could misbehave and accidentally include/exclude the wrong JARs. Escape each artifactId (or generate non-regex excludes) before building the pattern.
| def allIds = (['jenkins-core-libs'] + props.keySet()).join('|') | |
| def allIds = (['jenkins-core-libs'] + props.keySet()) | |
| .collect { java.util.regex.Pattern.quote(it.toString()) } | |
| .join('|') |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 18 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- .idea/encodings.xml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <dependencies> | ||
| <dependency> | ||
| <groupId>com.googlecode.owasp-java-html-sanitizer</groupId> | ||
| <artifactId>owasp-java-html-sanitizer</artifactId> | ||
| <version>20260313.1</version> | ||
| </dependency> | ||
|
|
||
| <dependency> | ||
| <groupId>org.jenkins-ci.main</groupId> | ||
| <artifactId>jenkins-core</artifactId> | ||
| <version>${project.version}</version> | ||
| <scope>provided</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.kohsuke.metainf-services</groupId> | ||
| <artifactId>metainf-services</artifactId> | ||
| <version>1.11</version> | ||
| <scope>provided</scope> | ||
| </dependency> | ||
| </dependencies> | ||
|
|
Copilot
AI
Apr 14, 2026
•
There was a problem hiding this comment.
Unlike other Maven modules in this repo (e.g., core/pom.xml, cli/pom.xml), this new module does not import the jenkins-bom in <dependencyManagement>. That can lead to inconsistent/transitively-selected dependency versions versus the rest of the reactor. Consider adding a dependencyManagement section importing org.jenkins-ci.main:jenkins-bom:${project.version} for consistency with the established module pattern.
There was a problem hiding this comment.
Unsure about this one. Seems like something that maven-enforcer-plugin would identify.
A problem for when we get there?
There was a problem hiding this comment.
Using jenkins-bom as above would make sense to me.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 18 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- .idea/encodings.xml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 19 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- .idea/encodings.xml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (String resourcePath : resourcePaths) { | ||
| if (resourcePath.endsWith(".jar")) { | ||
| try { | ||
| urls.add(context.getResource(resourcePath)); |
There was a problem hiding this comment.
ServletContext#getResource(String) can return null when the resource cannot be resolved. Right now a null would be added to urls and later cause an NPE inside URLClassLoader. Consider checking for a null URL and throwing an IllegalStateException that includes the resourcePath to make startup failures diagnosable.
| urls.add(context.getResource(resourcePath)); | |
| URL url = context.getResource(resourcePath); | |
| if (url == null) { | |
| throw new IllegalStateException("Failed to resolve resource: " + resourcePath); | |
| } | |
| urls.add(url); |
| <dependencies> | ||
| <dependency> | ||
| <groupId>com.googlecode.owasp-java-html-sanitizer</groupId> | ||
| <artifactId>owasp-java-html-sanitizer</artifactId> | ||
| <version>20260313.1</version> | ||
| </dependency> | ||
|
|
||
| <dependency> | ||
| <groupId>org.jenkins-ci.main</groupId> | ||
| <artifactId>jenkins-core</artifactId> | ||
| <version>${project.version}</version> | ||
| <scope>provided</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.kohsuke.metainf-services</groupId> | ||
| <artifactId>metainf-services</artifactId> | ||
| <version>1.11</version> |
There was a problem hiding this comment.
This module goes straight from the <parent> block to hardcoded dependency versions without importing org.jenkins-ci.main:jenkins-bom in <dependencyManagement>, unlike other reactor modules (e.g., core/pom.xml:48-65, war/pom.xml:55-65, cli/pom.xml:23-33). Consider adding the BOM import here and dropping explicit <version> tags that are managed there to keep versions aligned across the reactor.
| <dependencies> | |
| <dependency> | |
| <groupId>com.googlecode.owasp-java-html-sanitizer</groupId> | |
| <artifactId>owasp-java-html-sanitizer</artifactId> | |
| <version>20260313.1</version> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.jenkins-ci.main</groupId> | |
| <artifactId>jenkins-core</artifactId> | |
| <version>${project.version}</version> | |
| <scope>provided</scope> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.kohsuke.metainf-services</groupId> | |
| <artifactId>metainf-services</artifactId> | |
| <version>1.11</version> | |
| <dependencyManagement> | |
| <dependencies> | |
| <dependency> | |
| <groupId>org.jenkins-ci.main</groupId> | |
| <artifactId>jenkins-bom</artifactId> | |
| <version>${project.version}</version> | |
| <type>pom</type> | |
| <scope>import</scope> | |
| </dependency> | |
| </dependencies> | |
| </dependencyManagement> | |
| <dependencies> | |
| <dependency> | |
| <groupId>com.googlecode.owasp-java-html-sanitizer</groupId> | |
| <artifactId>owasp-java-html-sanitizer</artifactId> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.jenkins-ci.main</groupId> | |
| <artifactId>jenkins-core</artifactId> | |
| <scope>provided</scope> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.kohsuke.metainf-services</groupId> | |
| <artifactId>metainf-services</artifactId> |
| ServiceLoader<T> loader = ServiceLoader.load(clazz, coreLibClassLoader); | ||
| result = loader.findFirst().or(Optional::empty); | ||
| if (result.isEmpty()) { | ||
| LOGGER.log(Level.WARNING, "No service implementation found for: {0}", clazz.getName()); |
There was a problem hiding this comment.
getCoreLibrary logs WARNING when no implementation is found. Since this API returns null by design (and callers like UpdateSite.Plugin already handle null), missing services may be an expected condition and the warning can become noisy in normal operation. Consider logging at FINE/CONFIG for the empty-result case and reserving WARNING for ServiceConfigurationError (similar to jenkins/websocket/WebSockets.java:55-63, which logs service misconfiguration at FINE).
| LOGGER.log(Level.WARNING, "No service implementation found for: {0}", clazz.getName()); | |
| LOGGER.log(FINE, "No service implementation found for: {0}", clazz.getName()); |
There was a problem hiding this comment.
Agreed, I think. IIUC this would only happen in a unit test?
| final Set<String> resourcePaths = context.getResourcePaths("/WEB-INF/core-lib/"); | ||
|
|
||
| if (resourcePaths == null || resourcePaths.isEmpty()) { | ||
| throw new IllegalStateException("No WEB-INF/core-lib/ resources found"); | ||
| } |
There was a problem hiding this comment.
resourcePaths being non-empty doesn’t guarantee there are any JARs to load (e.g., only subdirectories/markers). After filtering to *.jar, urls can still be empty and the classloader will initialize without core-lib content. Consider validating that at least one JAR URL was found (and failing fast with a clear message) before returning the classloader.
Kevin-CB
left a comment
Kevin-CB
left a comment
There was a problem hiding this comment.
Looks great!
Changes to various pom.xml, including the addition of some Groovy scripts, that ensure we're bundling the libraries in the correct location, outside the usual directory. This is the most awkward part of the PR, but I haven't found a way to make this nicer while keeping the functionality as intended (works with java -jar jenkins.war, mvn -pl war jetty:run, mvn hpi:run, and core + plugin tests).
I tried to think about a simpler approach. And didn't mange to have a real simplification. However, we could maybe try to have core-libs to create a ZIP file with the validated core-lib JARs (maven-assembly?), after checking bundled-libraries.properties against the resolved dependencies. Then the war could just unpack this ZIP into WEB-INF/core-lib with a single dependency:unpack.
This would make things clearer between the modules:
core-libsmanages and validates the list of bundled librarieswarjust uses the ZIP and doesn’t need to figure out artifact IDs fromjenkins-core-libs.jar
That said, I’m not sure this would work in all cases (java -jar jenkins.war, mvn -pl war jetty:run, etc.).
Feel free to do that and report back how well that works.
To clarify, you'd remove the |
timja
left a comment
timja
left a comment
There was a problem hiding this comment.
LGTM
/label ready-for-merge
This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.
Thanks!
comment-ops-bot
Bot
added
the
ready-for-merge
| war/src/main/webapp/jsbundles/ | ||
|
|
||
| # Generated core-libs to ensure correct classpath in jetty:run, see `copy-core-lib-to-src` in `war/pom.xml`. | ||
| war/src/main/webapp/WEB-INF/core-lib |
There was a problem hiding this comment.
This feels really wrong. A Maven build should never be creating files under src/. Did you mean to edit
Lines 805 to 810 in 8a2cb98
| </goals> | ||
| <phase>generate-resources</phase> | ||
| <configuration> | ||
| <outputDirectory>${project.basedir}/src/main/webapp/WEB-INF/core-lib</outputDirectory> |
There was a problem hiding this comment.
As above: please avoid doing this if at all possible.
| * THE SOFTWARE. | ||
| */ | ||
|
|
||
| package jenkins.security; |
There was a problem hiding this comment.
Different JAR so you should use a distinct package name to avoid IllegalAccessErrors.
| if (excerpt == null) { | ||
| return null; | ||
| } | ||
| final Jenkins jenkins = Jenkins.getInstanceOrNull(); |
There was a problem hiding this comment.
Why not get()? This should not be called when Jenkins.instance == null.
timja
removed
the
ready-for-merge
| } | ||
| final Jenkins jenkins = Jenkins.getInstanceOrNull(); | ||
| final PluginExcerptSanitizer sanitizer = jenkins != null ? jenkins.getCoreLibrary(PluginExcerptSanitizer.class) : null; | ||
| if (sanitizer == null) { |
There was a problem hiding this comment.
Why would this be null? Unit tests?
| } | ||
| } | ||
| } | ||
| return new CoreLibClassLoader(urls.toArray(new URL[0]), parent); |
There was a problem hiding this comment.
or stylistically
| return new CoreLibClassLoader(urls.toArray(new URL[0]), parent); | |
| return new CoreLibClassLoader(urls.toArray(URL[]::new), parent); |
| @CheckForNull | ||
| String sanitize(@CheckForNull String html); |
There was a problem hiding this comment.
Both should be non-null I think; caller should do any required null check.
| */ | ||
| private final transient CoreLibClassLoader coreLibClassLoader; | ||
|
|
||
| private final transient HashMap<Class<?>, Optional<?>> coreLibs = new HashMap<>(); |
There was a problem hiding this comment.
Document the key/value types, which are not apparent from reading.
| @Restricted(NoExternalUse.class) | ||
| public <T> T getCoreLibrary(Class<T> clazz) { | ||
| synchronized (coreLibs) { | ||
| if (!coreLibs.containsKey(clazz)) { |
There was a problem hiding this comment.
Use compute. (And then you can use a ConcurrentHashMap if you like.)
| <followSymlinks>false</followSymlinks> | ||
| </fileset> | ||
| <fileset> | ||
| <directory>war/src/main/webapp/WEB-INF/core-lib</directory> |
| <type>executable-war</type> | ||
| <scope>test</scope> | ||
| <exclusions> | ||
| <!-- Remove from test classpath --> |
There was a problem hiding this comment.
Why is that? And would this not require a matching change to https://github.com/jenkinsci/plugin-pom/blob/7eccfd2bd8c0d431e80ffd87d8cb9bab70317082/pom.xml#L252-L258 ?
| private static class VerifyIsolationStep implements RealJenkinsExtension.Step { | ||
| @Override | ||
| public void run(JenkinsRule r) throws Exception { |
There was a problem hiding this comment.
More idiomatic to use a static method handle.
| ClassLoader pluginClassLoader = plugin.classLoader; | ||
|
|
||
| // The following assertion is fairly brittle, as plugin classloaders have access to dependencies in the local Maven repo that don't match actual behavior during Jenkins runtime. | ||
| // Once https://github.com/jenkinsci/antisamy-markup-formatter-plugin/pull/174, this assertion will probably fail and should be removed. |
There was a problem hiding this comment.
There was a problem hiding this comment.
It is not clear from this comment which assertion would fail, how, or what we should be asserting instead.
| PluginWrapper plugin = j.jenkins.getPluginManager().getPlugins().stream().filter(p -> !"antisamy-markup-formatter".equals(p.getShortName())).findFirst().orElseThrow(); | ||
| ClassLoader pluginClassLoader = plugin.classLoader; | ||
|
|
||
| // The following assertion is fairly brittle, as plugin classloaders have access to dependencies in the local Maven repo that don't match actual behavior during Jenkins runtime. |
There was a problem hiding this comment.
So what is the purpose of this test, if the RealJenkinsRule version covers what we care about?
There was a problem hiding this comment.
Most tests are written as JenkinsRule, and this test asserts reasonable behavior for those. I guess we could use another synthetic plugin here to eliminate the risk of ending up with a (transitive) dependency that causes the test to fall over.
|
|
||
| // Generate regex pattern for packaging excludes (include jenkins-core-libs itself + all artifact IDs) | ||
| def allIds = (['jenkins-core-libs'] + props.keySet()).join('|') | ||
| project.properties['core-lib-packaging-excludes'] = "%regex[WEB-INF/lib/(" + allIds + ")-.*\\.jar]" |
There was a problem hiding this comment.
| project.properties['core-lib-packaging-excludes'] = "%regex[WEB-INF/lib/(" + allIds + ")-.*\\.jar]" | |
| project.properties['core-lib-packaging-excludes'] = "%regex[WEB-INF/lib/(" + allIds + ")-.*[.]jar]" |
arguably more legible
| <!-- Exclude everything already in WEB-INF/core-lib from being duplicated in WEB-INF/lib --> | ||
| <packagingExcludes>${core-lib-packaging-excludes}</packagingExcludes> |
There was a problem hiding this comment.
This seems messy. Maybe rather than a dependency on jenkins-core-libs you wanted to copy jenkins-core-libs.jar itself to WEB-INF/core-lib/? Which IIUC would avoid the need for extract-core-lib-artifact-ids.groovy.
There was a problem hiding this comment.
Actually you are already doing that copy? So why is it a dep also? I am not following.
There was a problem hiding this comment.
Unsure what you're asking for here. I need a way to get the jars into the war's core-lib/ directory. Kevin suggested bundling them up as a zip and then extracting them here, which to me means having a dependency on a packaging/classifier artifact. Is that where you're going, or a different approach?
There was a problem hiding this comment.
…which would I think also address #26600 (comment) by allowing
Lines 169 to 175 in 8a2cb98
Fixes #18156
This adds a way to bundle libraries with Jenkins that does not make them available, even shaded, on plugin classpaths. This allows us to add them, replace them, or remove them at will.
At a high level, this PR includes the following components:
JenkinsandCoreLibClassLoader)pom.xml, including the addition of some Groovy scripts, that ensure we're bundling the libraries in the correct location, outside the usual directory. This is the most awkward part of the PR, but I haven't found a way to make this nicer while keeping the functionality as intended (works withjava -jar jenkins.war,mvn -pl war jetty:run,mvn hpi:run, and core + plugin tests).jenkins-core-libsthat holds implementations of dedicatedMETA-INFservices.core/) and its implementation (incore-libs/) usingowasp-java-html-sanitizer, to sanitize the plugin excerpt provided by update sites.The HTML sanitizer use case nicely demonstrates the usefulness of this system: We want to be able to sanitize HTML, but not with the configured "markup formatter" (different use case), don't want to implement this ourselves, and do not want to make the library used available to plugins.
(And, just in case: This does not fix a vulnerability. You need to trust the update sites you're using anyway. This is an improvement to protect from improper sanitization in the update site generator providing those JSON files. The rules we're applying here are identical to those in
update-center2, so makes no difference to the vast majority of users.)Testing done
Autotests and interactive.
Tested with
antisamy-markup-formatterto see whether it sees classes new in the newer OWASP Sanitizer library dependency, it doesn't, as expected: jenkinsci/antisamy-markup-formatter-plugin#177About Jenkins correctly lists the libraries.
Screenshots (UI changes only)
n/a
Proposed changelog entries
Proposed changelog category
/label major-rfe
Proposed upgrade guidelines
N/A
Submitter checklist
@Restrictedor have@since TODOJavadocs, as appropriate.@Deprecated(since = "TODO")or@Deprecated(forRemoval = true, since = "TODO"), if applicable.evalto ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@mention
Before the changes are marked as
ready-for-merge:Maintainer checklist
upgrade-guide-neededlabel is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidateto be considered.