fix: Prefer accessible interface methods over internal implementation classes in method resolution#485
Merged
lukaszlenart merged 12 commits intomainfrom Nov 29, 2025
Conversation
…mentation classes (#286) This fix addresses issue #286 where OGNL was selecting methods from internal JDK implementation classes (like sun.security.x509.X509CertImpl) instead of their public interfaces (like java.security.cert.X509Certificate), causing IllegalAccessException due to Java module system restrictions. Changes: - Added isLikelyAccessible() helper method to check if a class is likely accessible considering the Java module system - Enhanced findBestMethod() tie-breaking logic to prefer methods in this order: 1. Accessible classes over inaccessible ones 2. Interface methods over class methods (better encapsulation) 3. Public classes over non-public classes - Added comprehensive test cases in Issue286Test.java The fix uses the Java Module API to check if packages are exported and includes heuristics for known internal packages (sun.*, jdk.internal.*, etc). This ensures OGNL selects the most accessible method variant when multiple methods with the same signature exist in the class hierarchy. Fixes #286
- Added 8 new test cases covering edge cases and different scenarios - Tests now include: varargs, overloaded methods, abstract classes, generics, collections, interface inheritance, factory patterns, null handling, and primitives - Added assertions on actual values, not just non-null checks - Comprehensive coverage of method resolution tie-breaking logic - Tests exercise different code paths in isLikelyAccessible() and findBestMethod()
- Focus on the core issue: prefer accessible over inaccessible methods - Remove aggressive interface-over-class preference that could break existing tests - Keep original tie-breaking logic (prefer public classes) when both methods have the same accessibility level - Add null/empty package name check for safety - This should fix #286 while maintaining backward compatibility
- Removed interfaceMethodWithVarargs test that was failing due to OGNL varargs syntax limitations - Removed unused VarargsInterface and VarargsImplementation classes - Cleaned up unused imports (SSLContext, certificates, etc.) - Simplified x509CertificateMethodResolution test comment - Still have 14 comprehensive tests covering the fix
- Added tests with actual JDK classes (HashMap, ArrayList, StringBuilder) - Added test for package-private class to test accessibility logic - Added tests for multiple interface inheritance - Added tests with java.util classes to exercise different code paths - Tests now cover Map vs HashMap, List vs ArrayList scenarios - Total of 19 test cases covering various method resolution scenarios
- Added tests for CharSequence and Comparable interfaces - Added test to verify methods from both interfaces and classes are found - Added HashMap/Map test to verify interface vs concrete class handling - Tests cover various JDK interface scenarios - Now 22 test cases total providing comprehensive coverage of accessible code paths Note: Tests for inaccessible internal classes (sun.*, com.sun.*, etc.) cannot be easily added as we cannot instantiate those classes in unit tests. The inaccessibility detection logic is designed to handle real-world scenarios where OGNL encounters actual internal JDK implementation classes.
- Changed isLikelyAccessible() from private to package-private for testing - Added OgnlRuntimeAccessibilityTest with comprehensive coverage - Tests verify interface detection, standard JDK classes, and internal package detection - Tests attempt to load actual sun.* and com.sun.* classes to verify inaccessibility detection - Handles ClassNotFoundException gracefully for classes that may not be available - Significantly improves code coverage by testing the method directly
- Created sun.test.SimulatedInternalClass to simulate sun.* internal classes - Created sun.test.PublicTestInterface for testing interface preference - Created com.sun.test.AnotherInternalClass to test com.sun.* detection - Added tests that verify classes in sun.* and com.sun.* are detected as inaccessible - Added simulatedInternalClassVsInterface test to exercise the key code path - Added simulatedInternalClassIsDetectedAsInaccessible for direct verification - These simulated classes allow coverage of the (!currentAccessible && newAccessible) branch - Updated OgnlRuntimeAccessibilityTest with additional test cases This significantly improves coverage by allowing us to test the actual accessibility preference logic without needing real internal JDK classes.
Issue286Test is in package ognl.test and cannot access the package-private isLikelyAccessible() method in package ognl. Direct testing of that method is already covered in OgnlRuntimeAccessibilityTest which is in the correct package. The simulatedInternalClassVsInterface() test still provides end-to-end coverage of the fix by using classes in sun.test package.
- Removed defaultPackageClassIsAccessible() which had no assertions - Removed jdkInternalPackageWouldBeInaccessible() which only tested string operations - These tests provided no actual coverage or value - All meaningful test cases are already covered by other tests
|
lukaszlenart
added a commit
that referenced
this pull request
Nov 29, 2025
…backport #485 to ognl-3-4-x) Fixes #286 - OGNL choosing method on unexported class rather than exported interface This backport adapts PR #485 from main branch for Java 8 compatibility: - Added isLikelyAccessible() helper method (Java 8 compatible) - Enhanced findBestMethod() tie-breaking logic to prefer accessible methods - Added comprehensive test coverage (36 new tests: 10 unit + 26 integration) Java 8 adaptations: - Use clazz.getPackage().getName() instead of getPackageName() - Remove Module API checks (Java 9+ only) - Use Arrays.asList() instead of List.of() in tests - Convert from JUnit 5 to JUnit 4 - Swap assertion parameter order (message first in JUnit 4) - Replace assertDoesNotThrow() with try-catch pattern Test results: 993 tests passed (957 existing + 36 new), 0 failures 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
8 tasks
lukaszlenart
added a commit
that referenced
this pull request
Nov 29, 2025
…backport #485 to ognl-3-4-x) Fixes #286 - OGNL choosing method on unexported class rather than exported interface This backport adapts PR #485 from main branch for Java 8 compatibility: - Added isLikelyAccessible() helper method (Java 8 compatible) - Enhanced findBestMethod() tie-breaking logic to prefer accessible methods - Added comprehensive test coverage (36 new tests: 10 unit + 26 integration) Java 8 adaptations: - Use clazz.getPackage().getName() instead of getPackageName() - Remove Module API checks (Java 9+ only) - Use Arrays.asList() instead of List.of() in tests - Convert from JUnit 5 to JUnit 4 - Swap assertion parameter order (message first in JUnit 4) - Replace assertDoesNotThrow() with try-catch pattern Test results: 993 tests passed (957 existing + 36 new), 0 failures 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
lukaszlenart
added a commit
that referenced
this pull request
Nov 29, 2025
…backport #485 to ognl-3-4-x) Fixes #286 - OGNL choosing method on unexported class rather than exported interface This backport adapts PR #485 from main branch for Java 8 compatibility: - Added isLikelyAccessible() helper method (Java 8 compatible) - Enhanced findBestMethod() tie-breaking logic to prefer accessible methods - Added comprehensive test coverage (36 new tests: 10 unit + 26 integration) Java 8 adaptations: - Use clazz.getPackage().getName() instead of getPackageName() - Remove Module API checks (Java 9+ only) - Use Arrays.asList() instead of List.of() in tests - Convert from JUnit 5 to JUnit 4 - Swap assertion parameter order (message first in JUnit 4) - Replace assertDoesNotThrow() with try-catch pattern Test results: 993 tests passed (957 existing + 36 new), 0 failures 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
lukaszlenart
added a commit
that referenced
this pull request
Nov 29, 2025
…backport #485 to ognl-3-4-x) (#503) Fixes #286 - OGNL choosing method on unexported class rather than exported interface This backport adapts PR #485 from main branch for Java 8 compatibility: - Added isLikelyAccessible() helper method (Java 8 compatible) - Enhanced findBestMethod() tie-breaking logic to prefer accessible methods - Added comprehensive test coverage (36 new tests: 10 unit + 26 integration) Java 8 adaptations: - Use clazz.getPackage().getName() instead of getPackageName() - Remove Module API checks (Java 9+ only) - Use Arrays.asList() instead of List.of() in tests - Convert from JUnit 5 to JUnit 4 - Swap assertion parameter order (message first in JUnit 4) - Replace assertDoesNotThrow() with try-catch pattern Test results: 993 tests passed (957 existing + 36 new), 0 failures 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Description
Fixes #286 - OGNL choosing method on unexported class rather than exported interface
This PR enhances OGNL's method resolution logic to prefer accessible methods from public interfaces over internal implementation classes, preventing
IllegalAccessExceptionerrors when working with Java 9+ module system restrictions.Problem
When calling methods on objects like
X509Certificatearray elements, OGNL was selecting methods from internal JDK implementation classes (e.g.,sun.security.x509.X509CertImpl) instead of the public interface (java.security.cert.X509Certificate). This causedIllegalAccessExceptionbecause the module system doesn't export internal packages: