gnodet-bot commented on code in PR #13277:
URL: https://github.com/apache/maven/pull/13277#discussion_r4115098817
##########
apache-maven/src/assembly/maven/bin/m2.conf:
##########
@@ -29,3 +29,7 @@ optionally ${user.home}/.m2/ext/*.jar
optionally ${maven.home}/lib/ext/*.jar
load ${maven.home}/lib/maven-*.jar
load ${maven.home}/lib/*.jar
+# CLAPP (Command Line App) support: when maven.clapp.name is set by a launcher
+# script, the tool's private jars in lib/clapp/<toolName>/ are added to the
+# core class-realm so they are available alongside the shared Maven classes.
+optionally ${maven.home}/lib/clapp/${maven.clapp.name}/*.jar
Review Comment:
🔴 **Critical — Architectural contradiction: double class loading.**
This `optionally` directive loads CLAPP JARs into the `plexus.core` realm
(the shared Maven ClassLoader). But `MavenClappCling.launchClapp()` _also_
creates a child `URLClassLoader` that scans the same `lib/clapp/<toolName>/`
directory for `*.jar` files.
This means every CLAPP JAR gets loaded twice:
1. By ClassWorlds into `plexus.core` (via this m2.conf line)
2. By `launchClapp()` into a child `URLClassLoader`
This defeats the stated isolation goal — the CLAPP classes are already
visible in the core realm, so the child classloader's "isolation" is illusory.
Worse, the same class loaded from both classloaders will produce
`ClassCastException` when objects cross the boundary (core realm code sees
`com.example.Foo@core` while CLAPP code sees `com.example.Foo@child`).
**Choose one approach:**
- **Option A (recommended):** Remove this `optionally` line entirely. Let
`launchClapp()` handle all CLAPP class loading via its child `URLClassLoader`.
This gives real isolation.
- **Option B:** Keep this m2.conf line and remove the `URLClassLoader` from
`launchClapp()` — but then there's no isolation at all, which contradicts the
PR's purpose.
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/MavenClappCling.java:
##########
@@ -0,0 +1,252 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.maven.api.annotations.Nullable;
+import org.apache.maven.api.cli.Invoker;
+import org.apache.maven.api.cli.Parser;
+import org.apache.maven.api.cli.ParserRequest;
+import org.apache.maven.cling.invoker.ProtoLookup;
+import org.apache.maven.cling.invoker.mvn.MavenInvoker;
+import org.apache.maven.cling.invoker.mvn.MavenParser;
+import org.codehaus.plexus.classworlds.ClassWorld;
+
+/**
+ * Maven CLAPP (Command Line App) entry point.
+ * <p>
+ * This class acts as the launcher for external Maven CLI tools ("CLAPPs")
that ship their
+ * own dependencies in {@code ${maven.home}/lib/clapp/<toolName>/} instead of
requiring
+ * those jars to be in the shared {@code ${maven.home}/lib/} directory.
+ * <p>
+ * The {@code maven.clapp.name} system property identifies which CLAPP to
launch.
+ * The CLAPP's jar directory is {@code ${maven.home}/lib/clapp/<toolName>/}.
+ * That directory is scanned for {@code *.jar} files which are added to a child
+ * {@link URLClassLoader} that delegates to the core Maven class-loader.
+ * The CLAPP's main entry point class is then looked up via the
+ * {@code maven.clapp.mainClass} system property and invoked.
+ * <p>
+ * This mechanism allows future {@code mvnXxx} tools to package tool-specific
+ * dependencies in isolation without bloating the core Maven classpath.
+ *
+ * @since 4.1.0
+ */
+public class MavenClappCling extends ClingSupport {
+
+ /**
+ * System property that specifies the CLAPP tool name (e.g., {@code
"mvnenc"}).
+ * Used to locate {@code ${maven.home}/lib/clapp/<toolName>/}.
+ */
+ public static final String MAVEN_CLAPP_NAME_PROPERTY = "maven.clapp.name";
+
+ /**
+ * System property that specifies the fully-qualified main class name of
the CLAPP tool.
+ * That class must expose a {@code public static int main(String[],
ClassWorld)} method.
+ */
+ public static final String MAVEN_CLAPP_MAIN_CLASS_PROPERTY =
"maven.clapp.mainClass";
+
+ /**
+ * Relative path under {@code ${maven.home}} where per-CLAPP jar
directories live.
+ */
+ static final String CLAPP_LIB_RELATIVE_PATH = "lib/clapp";
+
+ /**
+ * "Normal" Java entry point. Note: Maven uses ClassWorld Launcher and
this entry point is NOT
+ * used under normal circumstances.
+ */
+ public static void main(String[] args) throws IOException {
+ int exitCode = new MavenClappCling().run(args, null, null, null,
false);
+ System.exit(exitCode);
+ }
+
+ /**
+ * ClassWorld Launcher "enhanced" entry point: returning exitCode and
accepts ClassWorld.
+ * <p>
+ * When {@code maven.clapp.name} and {@code maven.clapp.mainClass} system
properties are set,
+ * this method builds a per-CLAPP child classloader and delegates to the
CLAPP's main class.
+ * Otherwise, it falls back to the standard {@link MavenCling} behaviour.
+ */
+ public static int main(String[] args, ClassWorld world) throws IOException
{
+ String clappName = System.getProperty(MAVEN_CLAPP_NAME_PROPERTY);
+ String clappMainClass =
System.getProperty(MAVEN_CLAPP_MAIN_CLASS_PROPERTY);
+
+ if (clappName != null && !clappName.isBlank() && clappMainClass !=
null && !clappMainClass.isBlank()) {
+ return launchClapp(clappName.trim(), clappMainClass.trim(), args,
world);
+ }
+
+ // Fallback: behave as MavenCling when no CLAPP is configured
+ return MavenCling.main(args, world);
+ }
+
+ /**
+ * ClassWorld Launcher "embedded" entry point: returning exitCode and
accepts ClassWorld and streams.
+ */
+ public static int main(
+ String[] args,
+ ClassWorld world,
+ @Nullable InputStream stdIn,
+ @Nullable OutputStream stdOut,
+ @Nullable OutputStream stdErr)
+ throws IOException {
+ return new MavenClappCling(world).run(args, stdIn, stdOut, stdErr,
true);
+ }
+
+ public MavenClappCling() {
+ super();
+ }
+
+ public MavenClappCling(ClassWorld classWorld) {
+ super(classWorld);
+ }
+
+ //
-------------------------------------------------------------------------
+ // ClingSupport contract – used when invoked as a fallback Maven build
+ //
-------------------------------------------------------------------------
+
+ @Override
+ protected Invoker createInvoker() {
+ return new MavenInvoker(
+ ProtoLookup.builder().addMapping(ClassWorld.class,
classWorld).build(), null);
+ }
+
+ @Override
+ protected Parser createParser() {
+ return new MavenParser();
+ }
+
+ @Override
+ protected ParserRequest.Builder createParserRequestBuilder(String[] args) {
+ return ParserRequest.mvn(args, createMessageBuilderFactory());
+ }
+
+ //
-------------------------------------------------------------------------
+ // CLAPP launch logic
+ //
-------------------------------------------------------------------------
+
+ /**
+ * Constructs a per-CLAPP child class-loader, loads the CLAPP main class
from it,
+ * and invokes its {@code main(String[], ClassWorld)} method.
+ *
+ * @param clappName the CLAPP tool name (e.g., {@code "mvnenc"})
+ * @param clappMainClass fully-qualified name of the CLAPP entry-point
class
+ * @param args command-line arguments
+ * @param world the ClassWorld shared with the Maven core
+ * @return the exit code returned by the CLAPP
+ * @throws IOException if the CLAPP lib directory cannot be read or the
entry-point class
+ * cannot be loaded/invoked
+ */
+ static int launchClapp(String clappName, String clappMainClass, String[]
args, ClassWorld world)
+ throws IOException {
+ String mavenHome = System.getProperty("maven.home");
+ if (mavenHome == null || mavenHome.isBlank()) {
+ throw new IOException(
+ "System property 'maven.home' is not set; cannot locate
CLAPP lib directory for: " + clappName);
+ }
+
+ Path clappLibDir =
Paths.get(mavenHome).resolve(CLAPP_LIB_RELATIVE_PATH).resolve(clappName);
+
+ // Build the list of jar URLs from the CLAPP-specific lib directory
+ List<URL> jarUrls = collectJarUrls(clappLibDir, clappName);
+
+ // Create a child class-loader that sees the core classes + the
CLAPP's own jars
+ ClassLoader parentLoader =
Thread.currentThread().getContextClassLoader();
+ URLClassLoader clappLoader = new URLClassLoader(jarUrls.toArray(new
URL[0]), parentLoader);
+
+ try {
+ Class<?> clazz = clappLoader.loadClass(clappMainClass);
+ Method mainMethod = clazz.getMethod("main", String[].class,
ClassWorld.class);
+ // Publish the CLAPP class-loader as the context class-loader so
that
+ // SPI / ServiceLoader mechanisms work correctly inside the CLAPP.
+ Thread.currentThread().setContextClassLoader(clappLoader);
+ return (int) mainMethod.invoke(null, args, world);
+ } catch (ClassNotFoundException e) {
+ throw new IOException("CLAPP '" + clappName + "': cannot find main
class '" + clappMainClass + "'", e);
+ } catch (NoSuchMethodException e) {
+ throw new IOException(
+ "CLAPP '" + clappName + "': main class '" + clappMainClass
+ + "' does not expose public static int
main(String[], ClassWorld)",
+ e);
+ } catch (InvocationTargetException e) {
+ Throwable cause = e.getCause();
+ if (cause instanceof IOException) {
+ throw (IOException) cause;
+ }
+ if (cause instanceof RuntimeException) {
+ throw (RuntimeException) cause;
+ }
+ if (cause instanceof Error) {
+ throw (Error) cause;
+ }
+ throw new IOException("CLAPP '" + clappName + "': invocation
failed", cause != null ? cause : e);
+ } catch (IllegalAccessException e) {
+ throw new IOException(
+ "CLAPP '" + clappName + "': cannot access main method of
'" + clappMainClass + "'", e);
+ } finally {
+ // Restore class-loader so that the core Maven runtime is
unaffected
+ Thread.currentThread().setContextClassLoader(parentLoader);
+ }
Review Comment:
⚠️ **Resource leak: URLClassLoader is never closed.**
The `URLClassLoader` at line 183 is created but never closed. The `finally`
block only restores the context class loader — it does not close `clappLoader`.
Each invocation leaks file handles to JAR files.
Wrap in try-with-resources:
```suggestion
// Create a child class-loader that sees the core classes + the
CLAPP's own jars
ClassLoader parentLoader =
Thread.currentThread().getContextClassLoader();
try (URLClassLoader clappLoader = new
URLClassLoader(jarUrls.toArray(new URL[0]), parentLoader)) {
try {
Class<?> clazz = clappLoader.loadClass(clappMainClass);
Method mainMethod = clazz.getMethod("main", String[].class,
ClassWorld.class);
// Publish the CLAPP class-loader as the context
class-loader so that
// SPI / ServiceLoader mechanisms work correctly inside the
CLAPP.
Thread.currentThread().setContextClassLoader(clappLoader);
return (int) mainMethod.invoke(null, args, world);
} catch (ClassNotFoundException e) {
throw new IOException("CLAPP '" + clappName + "': cannot
find main class '" + clappMainClass + "'", e);
} catch (NoSuchMethodException e) {
throw new IOException(
"CLAPP '" + clappName + "': main class '" +
clappMainClass
+ "' does not expose public static int
main(String[], ClassWorld)",
e);
} catch (InvocationTargetException e) {
Throwable cause = e.getCause();
if (cause instanceof IOException) {
throw (IOException) cause;
}
if (cause instanceof RuntimeException) {
throw (RuntimeException) cause;
}
if (cause instanceof Error) {
throw (Error) cause;
}
throw new IOException("CLAPP '" + clappName + "': invocation
failed", cause != null ? cause : e);
} catch (IllegalAccessException e) {
throw new IOException(
"CLAPP '" + clappName + "': cannot access main
method of '" + clappMainClass + "'", e);
} finally {
// Restore class-loader so that the core Maven runtime is
unaffected
Thread.currentThread().setContextClassLoader(parentLoader);
}
}
```
##########
apache-maven/src/assembly/maven/bin/mvn:
##########
@@ -499,7 +501,13 @@ if $cygwin ; then
fi
handle_args() {
+ _skip_next=false
while [ $# -gt 0 ]; do
+ if $_skip_next ; then
+ _skip_next=false
+ shift
+ continue
+ fi
Review Comment:
💡 **Dead code: `_skip_next` is initialized but never set to `true`.**
The `--clapp` handler at line 541 uses `shift` directly to consume its
argument, so the iteration naturally advances past the tool name. The
`_skip_next` mechanism is never activated — this entire block is dead code.
Remove it:
```suggestion
while [ $# -gt 0 ]; do
```
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/MavenClappCling.java:
##########
@@ -0,0 +1,252 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.maven.api.annotations.Nullable;
+import org.apache.maven.api.cli.Invoker;
+import org.apache.maven.api.cli.Parser;
+import org.apache.maven.api.cli.ParserRequest;
+import org.apache.maven.cling.invoker.ProtoLookup;
+import org.apache.maven.cling.invoker.mvn.MavenInvoker;
+import org.apache.maven.cling.invoker.mvn.MavenParser;
+import org.codehaus.plexus.classworlds.ClassWorld;
+
+/**
+ * Maven CLAPP (Command Line App) entry point.
+ * <p>
+ * This class acts as the launcher for external Maven CLI tools ("CLAPPs")
that ship their
+ * own dependencies in {@code ${maven.home}/lib/clapp/<toolName>/} instead of
requiring
+ * those jars to be in the shared {@code ${maven.home}/lib/} directory.
+ * <p>
+ * The {@code maven.clapp.name} system property identifies which CLAPP to
launch.
+ * The CLAPP's jar directory is {@code ${maven.home}/lib/clapp/<toolName>/}.
+ * That directory is scanned for {@code *.jar} files which are added to a child
+ * {@link URLClassLoader} that delegates to the core Maven class-loader.
+ * The CLAPP's main entry point class is then looked up via the
+ * {@code maven.clapp.mainClass} system property and invoked.
+ * <p>
+ * This mechanism allows future {@code mvnXxx} tools to package tool-specific
+ * dependencies in isolation without bloating the core Maven classpath.
+ *
+ * @since 4.1.0
+ */
+public class MavenClappCling extends ClingSupport {
+
+ /**
+ * System property that specifies the CLAPP tool name (e.g., {@code
"mvnenc"}).
+ * Used to locate {@code ${maven.home}/lib/clapp/<toolName>/}.
+ */
+ public static final String MAVEN_CLAPP_NAME_PROPERTY = "maven.clapp.name";
+
+ /**
+ * System property that specifies the fully-qualified main class name of
the CLAPP tool.
+ * That class must expose a {@code public static int main(String[],
ClassWorld)} method.
+ */
+ public static final String MAVEN_CLAPP_MAIN_CLASS_PROPERTY =
"maven.clapp.mainClass";
+
+ /**
+ * Relative path under {@code ${maven.home}} where per-CLAPP jar
directories live.
+ */
+ static final String CLAPP_LIB_RELATIVE_PATH = "lib/clapp";
+
+ /**
+ * "Normal" Java entry point. Note: Maven uses ClassWorld Launcher and
this entry point is NOT
+ * used under normal circumstances.
+ */
+ public static void main(String[] args) throws IOException {
+ int exitCode = new MavenClappCling().run(args, null, null, null,
false);
+ System.exit(exitCode);
+ }
+
+ /**
+ * ClassWorld Launcher "enhanced" entry point: returning exitCode and
accepts ClassWorld.
+ * <p>
+ * When {@code maven.clapp.name} and {@code maven.clapp.mainClass} system
properties are set,
+ * this method builds a per-CLAPP child classloader and delegates to the
CLAPP's main class.
+ * Otherwise, it falls back to the standard {@link MavenCling} behaviour.
+ */
+ public static int main(String[] args, ClassWorld world) throws IOException
{
+ String clappName = System.getProperty(MAVEN_CLAPP_NAME_PROPERTY);
+ String clappMainClass =
System.getProperty(MAVEN_CLAPP_MAIN_CLASS_PROPERTY);
+
+ if (clappName != null && !clappName.isBlank() && clappMainClass !=
null && !clappMainClass.isBlank()) {
+ return launchClapp(clappName.trim(), clappMainClass.trim(), args,
world);
+ }
+
+ // Fallback: behave as MavenCling when no CLAPP is configured
+ return MavenCling.main(args, world);
+ }
+
+ /**
+ * ClassWorld Launcher "embedded" entry point: returning exitCode and
accepts ClassWorld and streams.
+ */
+ public static int main(
+ String[] args,
+ ClassWorld world,
+ @Nullable InputStream stdIn,
+ @Nullable OutputStream stdOut,
+ @Nullable OutputStream stdErr)
+ throws IOException {
+ return new MavenClappCling(world).run(args, stdIn, stdOut, stdErr,
true);
+ }
+
+ public MavenClappCling() {
+ super();
+ }
+
+ public MavenClappCling(ClassWorld classWorld) {
+ super(classWorld);
+ }
+
+ //
-------------------------------------------------------------------------
+ // ClingSupport contract – used when invoked as a fallback Maven build
+ //
-------------------------------------------------------------------------
+
+ @Override
+ protected Invoker createInvoker() {
+ return new MavenInvoker(
+ ProtoLookup.builder().addMapping(ClassWorld.class,
classWorld).build(), null);
+ }
+
+ @Override
+ protected Parser createParser() {
+ return new MavenParser();
+ }
+
+ @Override
+ protected ParserRequest.Builder createParserRequestBuilder(String[] args) {
+ return ParserRequest.mvn(args, createMessageBuilderFactory());
+ }
+
+ //
-------------------------------------------------------------------------
+ // CLAPP launch logic
+ //
-------------------------------------------------------------------------
+
+ /**
+ * Constructs a per-CLAPP child class-loader, loads the CLAPP main class
from it,
+ * and invokes its {@code main(String[], ClassWorld)} method.
+ *
+ * @param clappName the CLAPP tool name (e.g., {@code "mvnenc"})
+ * @param clappMainClass fully-qualified name of the CLAPP entry-point
class
+ * @param args command-line arguments
+ * @param world the ClassWorld shared with the Maven core
+ * @return the exit code returned by the CLAPP
+ * @throws IOException if the CLAPP lib directory cannot be read or the
entry-point class
+ * cannot be loaded/invoked
+ */
+ static int launchClapp(String clappName, String clappMainClass, String[]
args, ClassWorld world)
+ throws IOException {
+ String mavenHome = System.getProperty("maven.home");
+ if (mavenHome == null || mavenHome.isBlank()) {
+ throw new IOException(
+ "System property 'maven.home' is not set; cannot locate
CLAPP lib directory for: " + clappName);
+ }
+
+ Path clappLibDir =
Paths.get(mavenHome).resolve(CLAPP_LIB_RELATIVE_PATH).resolve(clappName);
Review Comment:
⚠️ **Path traversal vulnerability: `clappName` is not validated.**
`clappName` originates from user input (the `maven.clapp.name` system
property, set by `--clapp <value>` on the command line). It is used directly in
`Paths.get(mavenHome).resolve(CLAPP_LIB_RELATIVE_PATH).resolve(clappName)`
without validation.
A value like `../../etc` would resolve to a directory outside the intended
`lib/clapp/` tree. While the impact is limited (it only scans for `*.jar` files
and loads them), it could still load arbitrary JARs from unexpected locations.
Validate that the resolved path stays within the intended directory:
```suggestion
String mavenHome = System.getProperty("maven.home");
if (mavenHome == null || mavenHome.isBlank()) {
throw new IOException(
"System property 'maven.home' is not set; cannot locate
CLAPP lib directory for: " + clappName);
}
Path clappBase =
Paths.get(mavenHome).resolve(CLAPP_LIB_RELATIVE_PATH);
Path clappLibDir = clappBase.resolve(clappName).normalize();
if (!clappLibDir.startsWith(clappBase)) {
throw new IOException(
"CLAPP name '" + clappName + "' resolves outside the
CLAPP lib directory: " + clappLibDir);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
