subbudvk commented on code in PR #1021:
URL: https://github.com/apache/opennlp/pull/1021#discussion_r3143410599
##########
opennlp-api/src/main/java/opennlp/tools/util/ext/ExtensionLoader.java:
##########
@@ -98,4 +177,13 @@ public static <T> T instantiateExtension(Class<T> clazz,
String extensionClassNa
clazz.getName() + ", the class or service " + extensionClassName +
" could not be located!");
}
+
+ /**
+ * Resets allowed package prefixes to defaults ({@code opennlp.} plus any
+ * prefixes in {@link #ALLOWED_PACKAGES_PROPERTY}). Package-private — for
tests only.
+ */
+ static void resetAllowedPackages() {
Review Comment:
Yes, supported _unregisterAllowedPackage_
##########
opennlp-tools/src/test/java/opennlp/tools/util/ext/ExtensionLoaderTest.java:
##########
@@ -33,11 +33,152 @@ public String generateTestString() {
}
}
+ @AfterEach
+ void reset() {
+ System.clearProperty(ExtensionLoader.ALLOWED_PACKAGES_PROPERTY);
+ ExtensionLoader.resetAllowedPackages();
+ }
+
+ // --- existing test ---
+
@Test
void testLoadingStringGenerator() {
TestStringGenerator g =
ExtensionLoader.instantiateExtension(TestStringGenerator.class,
TestStringGeneratorImpl.class.getName());
Assertions.assertEquals("test", g.generateTestString());
}
+ // --- allowlist tests ---
+
+ /**
+ * Classes in opennlp.* are allowed by default — no registration needed.
+ */
+ @Test
+ void testBuiltinOpennlpPackageAllowedByDefault() {
+ Assertions.assertDoesNotThrow(() ->
+ ExtensionLoader.instantiateExtension(TestStringGenerator.class,
+ TestStringGeneratorImpl.class.getName()));
+ }
+
+ /**
+ * A class outside opennlp.* is rejected without registration.
+ * This is the core security invariant — untrusted class names from model
+ * manifests must not reach Class.forName() without an explicit allowlist
entry.
+ */
+ @Test
+ void testUnregisteredPackageIsRejected() {
+ ExtensionNotLoadedException ex = Assertions.assertThrows(
+ ExtensionNotLoadedException.class,
+ () -> ExtensionLoader.instantiateExtension(
+ TestStringGenerator.class,
+ "com.example.exploit.MaliciousFactory"));
+
+ Assertions.assertTrue(ex.getMessage().contains("not in an allowed
package"),
+ "exception message should mention allowed package");
+ }
+
+ /**
+ * Allowlist check runs before Class.forName() — even for non-existent
classes
+ * the error must be "not in an allowed package", never "could not be
located".
+ */
+ @Test
+ void testAllowlistGateRunsBeforeClassForName() {
+ ExtensionNotLoadedException ex = Assertions.assertThrows(
+ ExtensionNotLoadedException.class,
+ () -> ExtensionLoader.instantiateExtension(
+ TestStringGenerator.class,
+ "com.example.DoesNotExistOnClasspath$$Probe"));
+
+ Assertions.assertTrue(ex.getMessage().contains("not in an allowed
package"),
+ "allowlist must reject before Class.forName(); got: " +
ex.getMessage());
+ Assertions.assertFalse(ex.getMessage().contains("could not be located"),
+ "Class.forName() must not have been reached; got: " + ex.getMessage());
+ }
+
+ /**
+ * After registerAllowedPackage(), a class from that package is permitted.
+ */
+ @Test
+ void testRegisteredPackageIsAllowed() {
+ ExtensionLoader.registerAllowedPackage("opennlp.tools.util.ext");
+
+ Assertions.assertDoesNotThrow(() ->
+ ExtensionLoader.instantiateExtension(TestStringGenerator.class,
+ TestStringGeneratorImpl.class.getName()));
+ }
+
+ /**
+ * Prefix collision: registering "com.acme" must not permit "com.acmeevil.*".
+ */
+ @Test
+ void testPrefixCollisionPrevented() {
+ ExtensionLoader.registerAllowedPackage("com.acme");
+
+ ExtensionNotLoadedException ex = Assertions.assertThrows(
+ ExtensionNotLoadedException.class,
+ () -> ExtensionLoader.instantiateExtension(
+ TestStringGenerator.class,
+ "com.acmeevil.Exploit"));
+
+ Assertions.assertTrue(ex.getMessage().contains("not in an allowed
package"));
+ }
+
+ /**
+ * registerAllowedPackage() rejects null and blank inputs.
+ */
+ @Test
+ void testRegisterAllowedPackageRejectsNullAndBlank() {
+ Assertions.assertThrows(NullPointerException.class,
+ () -> ExtensionLoader.registerAllowedPackage(null));
+
+ Assertions.assertThrows(IllegalArgumentException.class,
+ () -> ExtensionLoader.registerAllowedPackage(""));
+
+ Assertions.assertThrows(IllegalArgumentException.class,
+ () -> ExtensionLoader.registerAllowedPackage(" "));
+ }
+
+ // --- system property escape hatch tests ---
+
+ /**
+ * OPENNLP_EXT_ALLOWED_PACKAGES system property permits a custom package
+ * without requiring a registerAllowedPackage() call — the operational
escape hatch.
+ */
+ @Test
+ void testSystemPropertyAddsAllowedPackage() {
+ System.setProperty(ExtensionLoader.ALLOWED_PACKAGES_PROPERTY,
"opennlp.tools.util.ext");
+ ExtensionLoader.resetAllowedPackages(); // re-initialize from updated
system property
+
+ Assertions.assertDoesNotThrow(() ->
+ ExtensionLoader.instantiateExtension(TestStringGenerator.class,
Review Comment:
Corrected tests
##########
opennlp-tools/src/test/java/opennlp/tools/util/ext/ExtensionLoaderTest.java:
##########
@@ -33,11 +33,152 @@ public String generateTestString() {
}
}
+ @AfterEach
+ void reset() {
+ System.clearProperty(ExtensionLoader.ALLOWED_PACKAGES_PROPERTY);
+ ExtensionLoader.resetAllowedPackages();
+ }
+
+ // --- existing test ---
+
@Test
void testLoadingStringGenerator() {
TestStringGenerator g =
ExtensionLoader.instantiateExtension(TestStringGenerator.class,
TestStringGeneratorImpl.class.getName());
Assertions.assertEquals("test", g.generateTestString());
}
+ // --- allowlist tests ---
+
+ /**
+ * Classes in opennlp.* are allowed by default — no registration needed.
+ */
+ @Test
+ void testBuiltinOpennlpPackageAllowedByDefault() {
+ Assertions.assertDoesNotThrow(() ->
+ ExtensionLoader.instantiateExtension(TestStringGenerator.class,
+ TestStringGeneratorImpl.class.getName()));
+ }
+
+ /**
+ * A class outside opennlp.* is rejected without registration.
+ * This is the core security invariant — untrusted class names from model
+ * manifests must not reach Class.forName() without an explicit allowlist
entry.
+ */
+ @Test
+ void testUnregisteredPackageIsRejected() {
+ ExtensionNotLoadedException ex = Assertions.assertThrows(
+ ExtensionNotLoadedException.class,
+ () -> ExtensionLoader.instantiateExtension(
+ TestStringGenerator.class,
+ "com.example.exploit.MaliciousFactory"));
+
+ Assertions.assertTrue(ex.getMessage().contains("not in an allowed
package"),
+ "exception message should mention allowed package");
+ }
+
+ /**
+ * Allowlist check runs before Class.forName() — even for non-existent
classes
+ * the error must be "not in an allowed package", never "could not be
located".
+ */
+ @Test
+ void testAllowlistGateRunsBeforeClassForName() {
+ ExtensionNotLoadedException ex = Assertions.assertThrows(
+ ExtensionNotLoadedException.class,
+ () -> ExtensionLoader.instantiateExtension(
+ TestStringGenerator.class,
+ "com.example.DoesNotExistOnClasspath$$Probe"));
+
+ Assertions.assertTrue(ex.getMessage().contains("not in an allowed
package"),
+ "allowlist must reject before Class.forName(); got: " +
ex.getMessage());
+ Assertions.assertFalse(ex.getMessage().contains("could not be located"),
+ "Class.forName() must not have been reached; got: " + ex.getMessage());
+ }
+
+ /**
+ * After registerAllowedPackage(), a class from that package is permitted.
+ */
+ @Test
+ void testRegisteredPackageIsAllowed() {
+ ExtensionLoader.registerAllowedPackage("opennlp.tools.util.ext");
+
+ Assertions.assertDoesNotThrow(() ->
+ ExtensionLoader.instantiateExtension(TestStringGenerator.class,
+ TestStringGeneratorImpl.class.getName()));
+ }
+
+ /**
+ * Prefix collision: registering "com.acme" must not permit "com.acmeevil.*".
+ */
+ @Test
+ void testPrefixCollisionPrevented() {
+ ExtensionLoader.registerAllowedPackage("com.acme");
+
+ ExtensionNotLoadedException ex = Assertions.assertThrows(
+ ExtensionNotLoadedException.class,
+ () -> ExtensionLoader.instantiateExtension(
+ TestStringGenerator.class,
+ "com.acmeevil.Exploit"));
+
+ Assertions.assertTrue(ex.getMessage().contains("not in an allowed
package"));
+ }
+
+ /**
+ * registerAllowedPackage() rejects null and blank inputs.
+ */
+ @Test
+ void testRegisterAllowedPackageRejectsNullAndBlank() {
+ Assertions.assertThrows(NullPointerException.class,
+ () -> ExtensionLoader.registerAllowedPackage(null));
+
+ Assertions.assertThrows(IllegalArgumentException.class,
+ () -> ExtensionLoader.registerAllowedPackage(""));
+
+ Assertions.assertThrows(IllegalArgumentException.class,
+ () -> ExtensionLoader.registerAllowedPackage(" "));
+ }
+
+ // --- system property escape hatch tests ---
+
+ /**
+ * OPENNLP_EXT_ALLOWED_PACKAGES system property permits a custom package
+ * without requiring a registerAllowedPackage() call — the operational
escape hatch.
+ */
+ @Test
+ void testSystemPropertyAddsAllowedPackage() {
+ System.setProperty(ExtensionLoader.ALLOWED_PACKAGES_PROPERTY,
"opennlp.tools.util.ext");
+ ExtensionLoader.resetAllowedPackages(); // re-initialize from updated
system property
+
+ Assertions.assertDoesNotThrow(() ->
+ ExtensionLoader.instantiateExtension(TestStringGenerator.class,
+ TestStringGeneratorImpl.class.getName()));
+ }
+
+ /**
+ * OPENNLP_EXT_ALLOWED_PACKAGES accepts multiple comma-separated prefixes.
+ */
+ @Test
+ void testSystemPropertyMultiplePackages() {
+ System.setProperty(ExtensionLoader.ALLOWED_PACKAGES_PROPERTY,
+ "com.example.nlp, opennlp.tools.util.ext");
+ ExtensionLoader.resetAllowedPackages();
+
+ Assertions.assertDoesNotThrow(() ->
+ ExtensionLoader.instantiateExtension(TestStringGenerator.class,
Review Comment:
Corrected tests
--
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]
