CompositeWhitelist to support multiple whitelists #25
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary by CodeRabbit
WalkthroughIntroduces CompositeWhitelist, a new public class that wraps a non-empty, unmodifiable collection of Whitelist delegates and enforces unanimous approval: all permission checks (methods, constructors, static methods, field get/set, static field get/set, and isAllowedGetEnvSystemMethod) return true only if every delegate permits the action. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CompositeWhitelist
participant Delegate as Delegate[i]
Caller->>CompositeWhitelist: permitsX(args)
activate CompositeWhitelist
loop for each delegate i in delegates
CompositeWhitelist->>Delegate: permitsX(args)
Delegate-->>CompositeWhitelist: boolean (perm_i)
end
alt unanimous approval required
CompositeWhitelist-->>Caller: AND(perm_1..perm_N)
end
deactivate CompositeWhitelist
note right of CompositeWhitelist: delegates is non-empty and unmodifiable
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/CompositeWhitelist.java (3)
38-71: Confirm intended semantics (AND vs OR) and documentClass comment states AND semantics. That’s stricter than Jenkins’ common “union” behavior (permit if any delegate allows). If AND is truly desired, great—just ensure callers never pass an empty collection (addressed above). If union was intended, switch
allMatchtoanyMatch.Example union change (only if required):
- return delegates.stream().allMatch(delegate -> delegate.permitsMethod(method, receiver, args)); + return delegates.stream().anyMatch(delegate -> delegate.permitsMethod(method, receiver, args));Apply similarly to the other permit methods if union semantics are desired.
27-31: Clarify Javadoc: explicitly state empty-delegates behaviorOnce you enforce non-empty in the ctor, update the Javadoc to say “A call is permitted only if all delegates permit it. The delegate list must be non-empty.”
31-31: Add focused tests for safety and semanticsPlease add unit tests covering:
- Constructor rejects
null, empty, and collections withnullelements.- AND semantics: if any delegate denies, the composite denies; if all allow, it allows.
- Regression test ensuring permissions are not granted when delegates list is emptied post-construction (immutability).
Do you want me to draft these tests?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/CompositeWhitelist.java(1 hunks)
🔇 Additional comments (1)
src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/CompositeWhitelist.java (1)
18-26: Ensure annotation API dependency is declared
Confirm thatjavax.annotation.Nonnull/CheckForNullare provided at compile- and runtime (they’re not in Java 11+); add an explicit dependency onjavax.annotation:javax.annotation-apiorjakarta.annotation:jakarta.annotation-apiin your POM if it isn’t already.
src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/CompositeWhitelist.java
Outdated
Show resolved
Hide resolved
…od to validate against whitelist delegates
jmendeza
force-pushed
the
bugfix/1156-e
branch
from
09c4e01 to
a00e1d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/CompositeWhitelist.java (1)
35-42: Fix mutability and vacuous-truth bypass: make delegates private final, defensively copy, and forbid null elements.As written, unmodifiableCollection(delegates) is only a read-only view; external code can still mutate the backing collection to empty or inject nulls, turning allMatch(..) into allow-all (vacuous truth) or causing NPEs. Also the field is protected and non-final.
Apply within this range:
-public class CompositeWhitelist extends Whitelist { - protected Collection<? extends Whitelist> delegates; +public class CompositeWhitelist extends Whitelist { + private final java.util.List<Whitelist> delegates; @@ - public CompositeWhitelist(Collection<? extends Whitelist> delegates) { - if (CollectionUtils.isEmpty(delegates)) { - throw new IllegalArgumentException("delegates must not be empty"); - } - this.delegates = unmodifiableCollection(delegates); - } + public CompositeWhitelist(@Nonnull Collection<? extends Whitelist> delegates) { + java.util.Objects.requireNonNull(delegates, "delegates must not be null"); + if (delegates.isEmpty()) { + throw new IllegalArgumentException("delegates must not be empty"); + } + final java.util.ArrayList<Whitelist> copy = new java.util.ArrayList<>(delegates.size()); + for (Whitelist d : delegates) { + copy.add(java.util.Objects.requireNonNull(d, "delegate must not be null")); + } + this.delegates = java.util.Collections.unmodifiableList(copy); + }Also adjust imports outside this range:
// remove: import org.apache.commons.collections4.CollectionUtils; import static java.util.Collections.unmodifiableCollection; // add: import java.util.ArrayList; import java.util.List; import java.util.Objects; import static java.util.Collections.unmodifiableList;
🧹 Nitpick comments (3)
src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/CompositeWhitelist.java (3)
30-33: Clarify Javadoc: AND for general checks, OR for getenv.Current comment says “A call is permitted if all delegates permit it” but isAllowedGetEnvSystemMethod uses OR. Make the exception explicit.
-/** - * Composite of multiple whitelists - * A call is permitted if all delegates permit it - */ +/** + * Composite of multiple whitelists. + * Most checks use consensus (AND): a call is permitted only if all delegates permit it. + * Exception: {@link #isAllowedGetEnvSystemMethod(Method, Object[])} uses OR so that any delegate + * allowing a specific System.getenv call is sufficient. + */
79-82: Confirm OR semantics for getenv are intentional.This allows an env read if any delegate approves it, even if others would deny. If consensus is desired here too, switch to allMatch; otherwise keep OR and ensure docs/tests cover this policy.
Optional change:
- return delegates.stream().anyMatch(delegate -> delegate.isAllowedGetEnvSystemMethod(m, args)); + return delegates.stream().allMatch(delegate -> delegate.isAllowedGetEnvSystemMethod(m, args));
37-42: Optional: add a convenience varargs ctor.Improves ergonomics at call sites without affecting behavior.
Example (outside this range):
public CompositeWhitelist(@Nonnull Whitelist... delegates) { this(Arrays.asList(Objects.requireNonNull(delegates, "delegates must not be null"))); }I can add unit tests for AND vs OR behavior if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/CompositeWhitelist.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/CompositeWhitelist.java (1)
src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/Whitelist.java (1)
Whitelist(41-112)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Travis CI - Pull Request
🔇 Additional comments (1)
src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/CompositeWhitelist.java (1)
45-77: AND aggregation looks good and short-circuits correctly.Use of allMatch(...) across delegates for methods/ctors/fields is correct and efficient.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/CompositeWhitelistTest.java (7)
29-31: Remove unused Mockito runner.No mocks are used; drop the runner and imports to reduce test footprint.
-import org.junit.runner.RunWith; -import org.mockito.junit.MockitoJUnitRunner; - -@RunWith(MockitoJUnitRunner.class)Also applies to: 39-39
61-69: Always unregister the interceptor (use try/finally).Prevents state leakage if the assertion or run fails.
sandboxInterceptorEmptyWhitelist.register(); -assertThrows(INVALID_SCRIPT + " contains unsupported operations", UnsupportedOperationException.class, () -> - scriptEngine.run(INVALID_SCRIPT, new Binding())); -sandboxInterceptorEmptyWhitelist.unregister(); +try { + assertThrows(INVALID_SCRIPT + " contains unsupported operations", UnsupportedOperationException.class, + () -> scriptEngine.run(INVALID_SCRIPT, new Binding())); +} finally { + sandboxInterceptorEmptyWhitelist.unregister(); +}
71-77: Guard unregister with finally.Ensures cleanup even on unexpected exceptions.
sandboxInterceptor.register(); -scriptEngine.run(INVALID_SCRIPT, new Binding()); -sandboxInterceptor.unregister(); +try { + scriptEngine.run(INVALID_SCRIPT, new Binding()); +} finally { + sandboxInterceptor.unregister(); +}
79-91: Apply try/finally around both registrations.Avoids interceptor leakage between tests.
sandboxInterceptorEmptyWhitelist.register(); -assertThrows(MULTIPLE_OPS_SCRIPT + " contains unsupported operations", UnsupportedOperationException.class, () -> - scriptEngine.run(MULTIPLE_OPS_SCRIPT, new Binding())); -sandboxInterceptorEmptyWhitelist.unregister(); +try { + assertThrows(MULTIPLE_OPS_SCRIPT + " contains unsupported operations", UnsupportedOperationException.class, + () -> scriptEngine.run(MULTIPLE_OPS_SCRIPT, new Binding())); +} finally { + sandboxInterceptorEmptyWhitelist.unregister(); +} sandboxInterceptor.register(); -assertThrows(MULTIPLE_OPS_SCRIPT + " contains non-whitelisted operations. They should fail even if accepted" + - " by the blacklist", UnsupportedOperationException.class, () -> - scriptEngine.run(MULTIPLE_OPS_SCRIPT, new Binding())); -sandboxInterceptor.unregister(); +try { + assertThrows(MULTIPLE_OPS_SCRIPT + " contains non-whitelisted operations. They should fail even if accepted by the blacklist", + UnsupportedOperationException.class, () -> scriptEngine.run(MULTIPLE_OPS_SCRIPT, new Binding())); +} finally { + sandboxInterceptor.unregister(); +}
93-98: Prefer classpath-based script roots to file paths.More robust across environments (e.g., IDEs, CI work dirs).
- return new GroovyScriptEngine(SCRIPTS_PATH, - new GroovyClassLoader(getClass().getClassLoader(), compilerConfig)); + ClassLoader cl = getClass().getClassLoader(); + URL scriptsRoot = Objects.requireNonNull(cl.getResource("groovy/scripts/"), "Missing scripts resource"); + return new GroovyScriptEngine(new URL[]{scriptsRoot}, + new GroovyClassLoader(cl, compilerConfig));Add import outside this hunk:
import java.net.URL; import java.util.Objects;
100-107: Null-safe resource loading for clearer failures.Avoids NPE if a resource is missing; yields actionable messages.
- ClassLoader loader = getClass().getClassLoader(); - Whitelist whitelist1 = new Blacklist(new InputStreamReader(loader.getResourceAsStream(BLACKLIST))); - Whitelist whitelist2 = new StaticWhitelist(new InputStreamReader(loader.getResourceAsStream(whitelist))); + ClassLoader loader = getClass().getClassLoader(); + InputStream blIs = Objects.requireNonNull(loader.getResourceAsStream(BLACKLIST), "Missing resource: " + BLACKLIST); + InputStream wlIs = Objects.requireNonNull(loader.getResourceAsStream(whitelist), "Missing resource: " + whitelist); + Whitelist whitelist1 = new Blacklist(new InputStreamReader(blIs)); + Whitelist whitelist2 = new StaticWhitelist(new InputStreamReader(wlIs));Add import outside this hunk if not present:
import java.io.InputStream; import java.util.Objects;
71-77: Optional: assert on failure cause to tighten intent.Consider checking the reject message or method to ensure the composite logic (unanimous allow) is what fails, not a missing method.
Also applies to: 80-91
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
pom.xml(1 hunks)src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/CompositeWhitelist.java(1 hunks)src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/CompositeWhitelistTest.java(1 hunks)src/test/resources/groovy/blacklist(1 hunks)src/test/resources/groovy/scripts/invalid.groovy(1 hunks)src/test/resources/groovy/scripts/multiple-ops.groovy(1 hunks)src/test/resources/groovy/whitelist(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/test/resources/groovy/scripts/multiple-ops.groovy
- src/test/resources/groovy/scripts/invalid.groovy
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/CompositeWhitelist.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/CompositeWhitelistTest.java (3)
src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/Whitelist.java (1)
Whitelist(41-112)src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/blacklists/Blacklist.java (1)
Blacklist(31-72)src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptor.java (1)
SuppressWarnings(57-550)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Travis CI - Pull Request
🔇 Additional comments (3)
src/test/resources/groovy/blacklist (1)
1-1: LGTM: blacklist entry format and target are correct.This precisely blocks String.contains(CharSequence) for the composite tests.
pom.xml (1)
121-131: Pin test dependencies to explicit versions
Add<version>4.13.2</version>to JUnit and<version>5.13.0</version>to Mockito to ensureassertThrowssupport and JDK 21 compatibility. Verify the resolved versions withmvn dependency:tree(or your IDE’s dependency viewer) before merging.src/test/resources/groovy/whitelist (1)
1-2: LGTM: whitelist entries align with test intents.Allows Script.println(Object) and String.contains(CharSequence) for the positive path in CompositeWhitelist tests.
https://github.com/craftersoftware/craftercms/issues/1156