Mirror upstream elastic/elasticsearch#134454 for AI review (snapshot of HEAD tree) #118
Conversation
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
BASE=41fea9d8a715b1e2ffb668c3cf54c6c9645f0331 HEAD=135d74b33ba9f1ce2b0e61fa5fb2a39a3763c688 Branch=main
WalkthroughExpands desktop entitlements to include ManageThreads. Broadens visibility of constants/fields in PolicyManager. Adds a new unit test instantiating MemoryCacheImageInputStream. Refactors TestScopeResolver from record to class, introduces excluded system package scope computation and lookups, and updates APIs to use PolicyScope. Adjusts TestPolicyManager trust logic to exclude those system packages. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor T as Test/Caller
participant TSR as TestScopeResolver
participant JRT as Java Modules
participant PM as PolicyScope
Note over TSR: Initialization phase
TSR->>JRT: ModuleFinder.ofSystem() / ModuleLayer.boot()
JRT-->>TSR: System modules & packages
TSR->>TSR: Build excludedSystemPackageScopes (from MODULES_EXCLUDED_FROM_SYSTEM_MODULES)
Note over T,TSR: Scope resolution
T->>TSR: getScope(callerClass)
TSR->>TSR: Resolve CodeSource
alt CodeSource is null
TSR->>TSR: getExcludedSystemPackageScope(callerClass)
alt Found excluded system package
TSR-->>T: PolicyScope (excluded system package)
else Not found
TSR-->>T: PolicyScope.unknown(location)
end
else CodeSource present
TSR-->>T: PolicyScope.server(...) or .plugin(...), or .unknown(...)
end
sequenceDiagram
autonumber
actor C as RequestingClass
participant TPM as TestPolicyManager
participant TSR as TestScopeResolver
C->>TPM: isTrustedSystemClass(requestingClass)
TPM->>TSR: getExcludedSystemPackageScope(requestingClass)
alt Scope is non-null (excluded)
TPM-->>C: false (not trusted)
else Scope is null
TPM-->>C: prior trust logic (unchanged)
end
Note over TPM: New guard enforces exclusion for specific system packages
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
libs/entitlement/src/main/java/org/elasticsearch/entitlement/bootstrap/HardcodedEntitlements.java(1 hunks)libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyManager.java(2 hunks)libs/entitlement/src/test/java/org/elasticsearch/entitlement/bootstrap/HardcodedEntitlementsTests.java(1 hunks)test/framework/src/main/java/org/elasticsearch/bootstrap/TestScopeResolver.java(4 hunks)test/framework/src/main/java/org/elasticsearch/entitlement/runtime/policy/TestPolicyManager.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/framework/src/main/java/org/elasticsearch/entitlement/runtime/policy/TestPolicyManager.java (1)
test/framework/src/main/java/org/elasticsearch/bootstrap/TestScopeResolver.java (1)
TestScopeResolver(35-160)
test/framework/src/main/java/org/elasticsearch/bootstrap/TestScopeResolver.java (1)
libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyManager.java (1)
PolicyManager(50-398)
🔇 Additional comments (5)
libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyManager.java (2)
57-58: Public visibility for excluded system modules constant looks goodNeeded for test/framework lookups; aligns with usage in TestScopeResolver.
97-101: Expose ComponentKind.componentNamePublic field enables constructing PolicyScope outside the package. No behavioral risk.
test/framework/src/main/java/org/elasticsearch/bootstrap/TestScopeResolver.java (1)
94-134: Resolver wiring and PolicyScope usage look correctPlugin/server scopes built as expected; jar URL handling preserved.
test/framework/src/main/java/org/elasticsearch/entitlement/runtime/policy/TestPolicyManager.java (1)
101-107: Correctly guard excluded system packages from being trustedAligns trust check with excluded-system scope computation.
libs/entitlement/src/main/java/org/elasticsearch/entitlement/bootstrap/HardcodedEntitlements.java (1)
117-123: Add ManageThreads to java.desktop is appropriateScoped to java.desktop; satisfies Disposer needs alongside native libs.
Comment on lines
+30
to
+32
| public void testTikaPDF() { | ||
| new MemoryCacheImageInputStream(new ByteArrayInputStream("test test".getBytes(UTF_8))); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Close the stream (try-with-resources) to avoid resource leaks
Also consider a minimal assertion to exercise the instance.
Apply:
- public void testTikaPDF() {
- new MemoryCacheImageInputStream(new ByteArrayInputStream("test test".getBytes(UTF_8)));
- }
+ public void testTikaPDF() throws Exception {
+ try (var in = new MemoryCacheImageInputStream(new ByteArrayInputStream("test test".getBytes(UTF_8)))) {
+ assertNotNull(in);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public void testTikaPDF() { | |
| new MemoryCacheImageInputStream(new ByteArrayInputStream("test test".getBytes(UTF_8))); | |
| } | |
| public void testTikaPDF() throws Exception { | |
| try (var in = new MemoryCacheImageInputStream(new ByteArrayInputStream("test test".getBytes(UTF_8)))) { | |
| assertNotNull(in); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
libs/entitlement/src/test/java/org/elasticsearch/entitlement/bootstrap/HardcodedEntitlementsTests.java
around lines 30-32, the MemoryCacheImageInputStream is created without being
closed and there is no assertion; change the test to open the
ByteArrayInputStream and MemoryCacheImageInputStream in a try-with-resources
block to ensure both are closed, read or call a simple method on the
MemoryCacheImageInputStream (e.g., read() or available()) to exercise the
instance, and add a minimal assertion (e.g., assertThat(value,
is(expectedNonNegativeOrNotMinusOne))) to validate the read/available result.
Comment on lines
+45
to
+64
| private static Map<String, PolicyScope> computeExcludedSystemPackageScopes() { | ||
| // Within any one module layer, module names are unique, so we just need the names | ||
| Set<String> systemModuleNames = ModuleFinder.ofSystem() | ||
| .findAll() | ||
| .stream() | ||
| .map(ref -> ref.descriptor().name()) | ||
| .filter(MODULES_EXCLUDED_FROM_SYSTEM_MODULES::contains) | ||
| .collect(toSet()); | ||
|
|
||
| Map<String, PolicyScope> result = new TreeMap<>(); | ||
| ModuleLayer.boot().modules().stream().filter(m -> systemModuleNames.contains(m.getName())).forEach(m -> { | ||
| ModuleDescriptor desc = m.getDescriptor(); | ||
| if (desc != null) { | ||
| desc.packages().forEach(pkg -> | ||
| // Our component identification logic returns SERVER for these | ||
| result.put(pkg, new PolicyScope(SERVER, SERVER.componentName, m.getName()))); | ||
| } | ||
| }); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Return an unmodifiable map to prevent accidental mutation
Compute once, then freeze the result to avoid accidental writes.
Apply:
- private static Map<String, PolicyScope> computeExcludedSystemPackageScopes() {
+ private static Map<String, PolicyScope> computeExcludedSystemPackageScopes() {
...
- return result;
+ return Map.copyOf(result);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private static Map<String, PolicyScope> computeExcludedSystemPackageScopes() { | |
| // Within any one module layer, module names are unique, so we just need the names | |
| Set<String> systemModuleNames = ModuleFinder.ofSystem() | |
| .findAll() | |
| .stream() | |
| .map(ref -> ref.descriptor().name()) | |
| .filter(MODULES_EXCLUDED_FROM_SYSTEM_MODULES::contains) | |
| .collect(toSet()); | |
| Map<String, PolicyScope> result = new TreeMap<>(); | |
| ModuleLayer.boot().modules().stream().filter(m -> systemModuleNames.contains(m.getName())).forEach(m -> { | |
| ModuleDescriptor desc = m.getDescriptor(); | |
| if (desc != null) { | |
| desc.packages().forEach(pkg -> | |
| // Our component identification logic returns SERVER for these | |
| result.put(pkg, new PolicyScope(SERVER, SERVER.componentName, m.getName()))); | |
| } | |
| }); | |
| return result; | |
| } | |
| private static Map<String, PolicyScope> computeExcludedSystemPackageScopes() { | |
| // Within any one module layer, module names are unique, so we just need the names | |
| Set<String> systemModuleNames = ModuleFinder.ofSystem() | |
| .findAll() | |
| .stream() | |
| .map(ref -> ref.descriptor().name()) | |
| .filter(MODULES_EXCLUDED_FROM_SYSTEM_MODULES::contains) | |
| .collect(toSet()); | |
| Map<String, PolicyScope> result = new TreeMap<>(); | |
| ModuleLayer.boot().modules().stream() | |
| .filter(m -> systemModuleNames.contains(m.getName())) | |
| .forEach(m -> { | |
| ModuleDescriptor desc = m.getDescriptor(); | |
| if (desc != null) { | |
| desc.packages().forEach(pkg -> | |
| // Our component identification logic returns SERVER for these | |
| result.put(pkg, new PolicyScope(SERVER, SERVER.componentName, m.getName())) | |
| ); | |
| } | |
| }); | |
| return Map.copyOf(result); | |
| } |
🤖 Prompt for AI Agents
In
test/framework/src/main/java/org/elasticsearch/bootstrap/TestScopeResolver.java
around lines 45-64, the method currently returns a mutable TreeMap; change it to
return an unmodifiable map to prevent accidental mutation by wrapping the result
before returning (e.g., Collections.unmodifiableMap(result) or
Map.copyOf(result)) so the computed map is frozen when returned.
Comment on lines
+70
to
+76
| PolicyScope getScope(Class<?> callerClass) { | ||
| var callerCodeSource = callerClass.getProtectionDomain().getCodeSource(); | ||
| assert callerCodeSource != null; | ||
| if (callerCodeSource == null) { | ||
| // This only happens for JDK classes. Furthermore, for trivially allowed modules, we shouldn't even get here. | ||
| // Hence, this must be an excluded system module, so check for that. | ||
| return requireNonNull(getExcludedSystemPackageScope(callerClass)); | ||
| } |
There was a problem hiding this comment.
Avoid potential NPE on non-excluded JDK classes
Edge-case: if this is ever called for a JDK class not in the excluded set, requireNonNull will throw. Prefer a safe fallback.
Apply:
- return requireNonNull(getExcludedSystemPackageScope(callerClass));
+ var scope = getExcludedSystemPackageScope(callerClass);
+ return scope != null ? scope : PolicyScope.unknown(callerClass.getModule().getName());Alternatively, throw with a clear message:
- return requireNonNull(getExcludedSystemPackageScope(callerClass));
+ return java.util.Objects.requireNonNull(
+ getExcludedSystemPackageScope(callerClass),
+ "Missing excluded-system package scope for " + callerClass.getName()
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| PolicyScope getScope(Class<?> callerClass) { | |
| var callerCodeSource = callerClass.getProtectionDomain().getCodeSource(); | |
| assert callerCodeSource != null; | |
| if (callerCodeSource == null) { | |
| // This only happens for JDK classes. Furthermore, for trivially allowed modules, we shouldn't even get here. | |
| // Hence, this must be an excluded system module, so check for that. | |
| return requireNonNull(getExcludedSystemPackageScope(callerClass)); | |
| } | |
| PolicyScope getScope(Class<?> callerClass) { | |
| var callerCodeSource = callerClass.getProtectionDomain().getCodeSource(); | |
| if (callerCodeSource == null) { | |
| // This only happens for JDK classes. Furthermore, for trivially allowed modules, we shouldn't even get here. | |
| // Hence, this must be an excluded system module, so check for that. | |
| var scope = getExcludedSystemPackageScope(callerClass); | |
| return scope != null | |
| ? scope | |
| : PolicyScope.unknown(callerClass.getModule().getName()); | |
| } | |
| … | |
| } |
🤖 Prompt for AI Agents
In
test/framework/src/main/java/org/elasticsearch/bootstrap/TestScopeResolver.java
around lines 70 to 76, avoid using
requireNonNull(getExcludedSystemPackageScope(callerClass)) because if a JDK
class that is not in the excluded set reaches this point it will cause an NPE;
instead capture the result into a local variable, check it for null, and if null
throw an IllegalStateException with a clear message that includes the
callerClass name (or alternatively return an explicit safe default PolicyScope
if that is more appropriate for your design).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Single commit with tree=135d74b33ba9f1ce2b0e61fa5fb2a39a3763c688^{tree}, parent=41fea9d8a715b1e2ffb668c3cf54c6c9645f0331. Exact snapshot of upstream PR head. No conflict resolution attempted.
Summary by CodeRabbit