Improve FactoryFinder Validation by cshannon · Pull Request #1793 · apache/activemq

cshannon · 2026-03-17T22:45:38Z

Also modernizes with generics and adds improved testing

activemq-stomp/src/test/java/org/apache/activemq/transport/stomp/StompTest.java

jbonofre · 2026-03-18T05:49:20Z

activemq-broker/src/main/java/org/apache/activemq/util/osgi/Activator.java

-    public Object create(String path) throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException {
+    public Object create(String path)
+            throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException {
+        throw new UnsupportedOperationException("Create is not supported without requiredType and allowed impls");


nit: as it's a "breaking change", we should remember to note in the release note.

I would not call this a breaking change because ActiveMQ never calls this method. FactoryFinder now calls the new method in the ObjectFactory interface so this new method is never invoked unless someone extended or implemented the interface themselves or wrote something custom. If someone has their own version of ObjectFactory everything works fine because the new method just delegates to the legacy. This is only breaking if it's expected users would invoke Activator themselves and not just the broker but I don't see why we would expect that.

jeanouii

Great rework @cshannon

We can maybe discuss about the backward compatible part if we have the fix on 6.3.0 maybe. If we don't have the backward compatibility we can't backport to previous versions. Trade off. I'm fine either way.

It looks great.
Only thing to verify is the Windows compatibility. The rest is more discussion/comment for sharing purposes.

jeanouii · 2026-03-18T10:39:58Z

activemq-client/src/main/java/org/apache/activemq/util/FactoryFinder.java

+            throw new InstantiationException("Provided key escapes the FactoryFinder configured directory");
+        }
+
+        return resolvedPath.toString();


With your comment above, this might fail on Windows, because /dir1/dir2/file will probably be in reality \dir1\dir2\file so I think you will probably need to consider doing
return resolvedPath.toString().replace('\', '/');

What do you think?

I don't think we need to do this because forward slashes should work fine with windows in java as this validation check doesn't result in any different output than previously. Previously we just used the path with forward slashes and add on the key, now we do the same but before returning just resolve it and check it first. So the resulting value should be the same as before on both windows/linux if it passes validation.

jeanouii · 2026-03-18T10:43:24Z

activemq-client/src/main/java/org/apache/activemq/util/FactoryFinder.java

-            Class clazz = classMap.get(path);
+        public Object create(final String path)
+                throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException {
+            throw new UnsupportedOperationException("Create is not supported without requiredType and allowed impls");


Same comment as @jbonofre did above for the Activator.

jeanouii · 2026-03-18T10:48:13Z

activemq-client/src/main/java/org/apache/activemq/util/FactoryFinder.java

+        @SuppressWarnings("unchecked")
+        default <T> T create(String path, Class<T> requiredType, Set<Class<? extends T>> allowedImpls)
+                throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException {
+            return (T) create(path);


This is bypassing all the class validation, path validation and allowed impls. Anyone setting a custom ObjectFactory can bypass validations.
Is it a design choice?
Maybe a javadoc comment if that's the case.

This was done by default for backwards compatibility and also I don't know why we would do anything if someone plugs in their own impl. If someone provides their own custom impl then it is on them to do whatever they want in my opinion.

I added a javadoc that makes mention it's up to the implementations to decide if the parameters are used. Maybe in 6.3.0 or another version where we can have breaking changes we could just drop the create(path) method entirely and just have the new method. Ultimately though, as I said, it's just an interface so implementations can ignore or do what they want.

jeanouii · 2026-03-18T11:03:16Z

activemq-client/src/main/java/org/apache/activemq/util/FactoryFinder.java


+    @SuppressWarnings("unchecked")
+    private static <T> Set<Class<? extends T>> loadAllowedImpls(Class<T> requiredType, String allowedImpls) {
+        // If allowedImpls is either null or an asterisk (allow all wild card) then set to null so we don't filter


Useful comment to move to javadoc maybe so it can benefit developers/contributors.

Now, I have one concern. The default 2 args contructor defaults to allow-all. Usually from a security perspective, we tend to deny-all by default. This PR being about improving security, I'm surprised. That being said, I see that for internet facing factories (Stomp and Web) are using a strict list of impl which is great.
For the internal factories (like TransportFactory), the default is applied and we use allow all whereas we know in advance the list, so we could switch to deny all by default and provide the list of known implementations for internal factories as well.
What do you think?

There's not really much point of having a 2 arg constructor if the default is deny all as it's pointless to create a FactoryFinder that can't do load anything. We could just only have the 3 arg constructor and then just update all the uses to provide the known lists

Actually there are a ton of impls for some of them and the impls are not all loaded on the same classpath so it's not easy to build the list. I think we can just get rid of the 2 arg and have the 3 arg and at least make the calling code explicilty set null if not caring to check

I agree with this compromise.

rmannibucau · 2026-03-18T12:45:34Z

activemq-client/src/main/java/org/apache/activemq/util/FactoryFinder.java

+        // Normalize the base path with the given key. This
+        // will resolve/remove any relative ".." sections of the path.
+        // Example: "/dir1/dir2/dir3/../file" becomes "/dir1/dir2/file"
+        final Path resolvedPath = Path.of(path).resolve(key).normalize();


need to move toAbsolutePath somehow (both paths) to be comparable otherwise the startsWith can fail even if "true"

rmannibucau · 2026-03-18T12:45:54Z

activemq-client/src/main/java/org/apache/activemq/util/FactoryFinder.java

+                .map(Class::getName).collect(Collectors.joining(","));
+    }
+
+    public static <T> void validateClass(Class<?> clazz, Class<T> requiredType,


if you want to validate the class you must validate the class name before loading it otherwise you can have side effects and defeat the validation no?

The class is is fine to load, what you want to do is avoid initialization and calling classloader.loadClass() does not initialize

You actually can't assume that with AMQ since it runs in complex classloader envs where it is wrong several (in some OSGi or EE setup for ex)

rmannibucau · 2026-03-18T12:46:31Z

activemq-client/src/main/java/org/apache/activemq/util/FactoryFinder.java

+            }
+        }
+        if (clazz == null) {
+            clazz = FactoryFinder.class.getClassLoader().loadClass(className);


classloader can be null there so a fallback on the system classloader is likely needed

This was existing behavior so could be changed in a future PR if needed

Also modernizes with generics and adds improved testing

Also modernizes with generics and adds improved testing Co-authored-by: Christopher L. Shannon <cshannon@apache.org>

…#1793)

…1801) * Backport: remove parallel annotation on ProtocolConverterTest (#1793) * Cleanup import

cshannon requested a review from jbonofre March 17, 2026 22:45

Improve FactoryFinder Validation

2fe1f53

Also modernizes with generics and adds improved testing

cshannon force-pushed the factory-finder-validation branch from 4adfd2f to 2fe1f53 Compare March 17, 2026 22:52

jbonofre previously approved these changes Mar 18, 2026

View reviewed changes

Remove unused imports

f0901eb

cshannon dismissed jbonofre’s stale review via f0901eb March 18, 2026 11:01

jeanouii reviewed Mar 18, 2026

View reviewed changes

Address comments

f7ce848

rmannibucau reviewed Mar 18, 2026

View reviewed changes

jbonofre approved these changes Mar 18, 2026

View reviewed changes

cshannon merged commit c9d3963 into apache:main Mar 18, 2026
10 checks passed

jbonofre pushed a commit to jbonofre/activemq that referenced this pull request Mar 18, 2026

Improve FactoryFinder Validation (apache#1793)

9e6c27c

Also modernizes with generics and adds improved testing

jbonofre mentioned this pull request Mar 18, 2026

[6.2.x] Backport: Improve FactoryFinder Validation (#1793) #1799

Merged

2 tasks

jbonofre pushed a commit to jbonofre/activemq that referenced this pull request Mar 18, 2026

Improve FactoryFinder Validation (apache#1793)

a3b2eab

Also modernizes with generics and adds improved testing

jbonofre mentioned this pull request Mar 18, 2026

[5.19.x] Backport: Improve FactoryFinder Validation (#1793) #1800

Merged

2 tasks

jbonofre added a commit that referenced this pull request Mar 18, 2026

Improve FactoryFinder Validation (#1793) (#1799)

b0ca555

Also modernizes with generics and adds improved testing Co-authored-by: Christopher L. Shannon <cshannon@apache.org>

jbonofre added a commit that referenced this pull request Mar 18, 2026

Improve FactoryFinder Validation (#1793) (#1800)

171fdfc

Also modernizes with generics and adds improved testing Co-authored-by: Christopher L. Shannon <cshannon@apache.org>

jbonofre added a commit to jbonofre/activemq that referenced this pull request Mar 18, 2026

Backport: add missing test class (apache#1793)

b056d4f

jbonofre added a commit to jbonofre/activemq that referenced this pull request Mar 18, 2026

Backport: remove parallel annotation on ProtocolConverterTest (apache…

a3e667c

…#1793)

jbonofre added a commit that referenced this pull request Mar 18, 2026

Backport: remove the annotation on the ProtocolConverterTest (#1793) (#…

0544699

…1801) * Backport: remove parallel annotation on ProtocolConverterTest (#1793) * Cleanup import

jbonofre added a commit to jbonofre/activemq that referenced this pull request Mar 18, 2026

Backport: fix the imports and dependencies (apache#1793)

c921713

jbonofre added a commit that referenced this pull request Mar 19, 2026

Backport: fix the imports and dependencies (#1793) (#1802)

256f225

Conversation

cshannon commented Mar 17, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeanouii left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jeanouii left a comment •

edited

Loading