Skip to content

Add config for groovy sandbox whitelist#3806

Merged
sumerjabri merged 12 commits intocraftercms:support/4.xfrom
jmendeza:bugfix/1156-e
Sep 10, 2025
Merged

Add config for groovy sandbox whitelist#3806
sumerjabri merged 12 commits intocraftercms:support/4.xfrom
jmendeza:bugfix/1156-e

Conversation

@jmendeza
Copy link
Copy Markdown
Member

@jmendeza jmendeza commented Sep 3, 2025

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Sep 3, 2025

Summary by CodeRabbit

  • New Features
    • Added sandbox whitelist for Groovy scripts with a bundled default list and enable/disable support.
    • Introduced configurable lifecycle script classpath with a default value.
  • Improvements
    • Expanded allowed APIs for Groovy scripts to reduce false positives while maintaining security.
    • Script execution now supports sandboxed evaluation for safer runtime behavior.
  • Chores
    • Reorganized scripting configuration keys (no functional change).
    • Minor cleanup in blacklist file comments (no functional change).

Walkthrough

Adds a Groovy sandbox whitelist resource and configuration, wires whitelist support into sandbox factory and GroovyScriptExecutor via constructor injection, rewrites GroovyScriptExecutor to be sandbox-aware (init, classloader/config, register/unregister sandbox), and minor cleanup of the blacklist header.

Changes

Cohort / File(s) Summary of Changes
Configuration
src/main/resources/crafter/studio/studio-config.yaml
Added studio.scripting.lifecycle.classpath: default-site, studio.scripting.sandbox.whitelist.enable: true, and studio.scripting.sandbox.whitelist.path: classpath:crafter/studio/groovy/whitelist; reordered some script keys (no value changes).
Spring wiring / Bean signatures
src/main/resources/crafter/studio/studio-services-context.xml
Switched to constructor-based wiring: GroovyScriptExecutor now constructed with sandboxInterceptor, scriptsClassPath, enableScriptSandbox; SandboxInterceptorFactory constructor now accepts whitelistEnabled and whitelist before whitelistGetEnvRegex; studioConfiguration uses constructor/init.
Sandbox whitelist resource
src/main/resources/crafter/studio/groovy/whitelist
Added Jenkins-style comprehensive whitelist enumerating allowed staticMethod, new, staticField, and method entries for Groovy/Java stdlib and selected Crafter Studio APIs (including CommonLifecycleApi-related entries).
Groovy executor implementation
src/main/java/org/craftercms/studio/impl/v1/script/GroovyScriptExecutor.java
Rewritten to be sandbox-aware with constructor GroovyScriptExecutor(SandboxInterceptor, List<String>, boolean), init() to configure ScriptEngine/GroovyClassLoader/CompilerConfiguration, apply sandbox AST transforms when enabled, register/unregister sandbox around evaluation; removed public get/set for scriptsClassPath.
Blacklist minor edit
src/main/resources/crafter/studio/groovy/blacklist
Removed top header comment and a blank line; blacklist entries remain unchanged.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor ScriptAuthor
  participant Studio
  participant Spring as "Spring Context"
  participant SandboxFactory as "SandboxInterceptorFactory"
  participant Sandbox as "SandboxInterceptor"
  participant Executor as "GroovyScriptExecutor"

  ScriptAuthor->>Studio: request script execution
  Studio->>Spring: resolve GroovyScriptExecutor bean
  Spring->>SandboxFactory: provide (sandboxEnabled, blacklist, whitelistEnabled, whitelistPath, getEnvRegex)
  SandboxFactory->>Sandbox: create interceptor (blacklist [+ optional whitelist])
  Studio->>Executor: executeScript(model, script)
  Executor->>Sandbox: register()
  Executor->>Executor: init/configure ScriptEngine (apply sandbox AST transforms if enabled)
  Executor->>Executor: eval(script) using configured engine
  Executor->>Sandbox: unregister()
  alt allowed
    Executor-->>Studio: return result
  else blocked
    Executor-->>Studio: throw SecurityException/error
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • sumerjabri

Pre-merge checks (1 passed, 2 warnings)

❌ Failed Checks (2 warnings)
Check Name Status Explanation Resolution
Description Check ⚠️ Warning The description only references an issue link and lacks any summary of the changes, motivation, impact, or testing instructions, making it difficult for reviewers to understand the purpose and scope of the PR. Please expand the PR description to include a clear summary of the changes and their rationale, any dependencies (such as the commons PR), expected behavior, testing steps, and consider adding a pull request template to the repository to enforce consistent, detailed descriptions going forward.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed Checks (1 passed)
Check Name Status Explanation
Title Check ✅ Passed The title “Add config for groovy sandbox whitelist” succinctly and accurately summarizes the primary change—adding configuration support for the Groovy sandbox whitelist—without unnecessary detail or noise.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
		  - name: "Undocumented Breaking Changes"
			  mode: "warning"
			  instructions: |
				  Flag potential breaking changes that are not documented:
				  1. Identify changes to public APIs/exports, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints (including removed/renamed items and changes to types, required params, return values, defaults, or behavior).
				  2. Ignore purely internal/private changes (e.g., code not exported from package entry points or marked internal).
				  3. Verify documentation exists: a "Breaking Change" section in the PR description and updates to CHANGELOG.md.
✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between fd469ad and db9d732.

📒 Files selected for processing (3)
  • src/main/resources/crafter/studio/studio-config.yaml (1 hunks)
  • src/main/resources/crafter/studio/studio-services-context.xml (1 hunks)
  • src/test/resources/crafter/studio/groovy/whitelist (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T23:38:33.963Z
Learnt from: jmendeza
PR: craftercms/studio#3794
File: src/main/resources/crafter/studio/studio-services-context.xml:801-808
Timestamp: 2025-08-08T23:38:33.963Z
Learning: In CrafterCMS Studio's Spring XML configuration, beans can have partial constructor arguments specified in XML while Spring autowires the remaining arguments by type. The DiskMonitor class uses ConstructorProperties annotation which enables Spring to correctly match and inject the missing dependencies even when only some constructor arguments are explicitly provided in the XML configuration.

Applied to files:

  • src/main/resources/crafter/studio/studio-services-context.xml
⏰ 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: Codacy Static Code Analysis
🔇 Additional comments (1)
src/main/resources/crafter/studio/studio-services-context.xml (1)

1681-1684: Verify SandboxInterceptorFactory constructor binding
In src/main/resources/crafter/studio/studio-services-context.xml (lines 1681–1684) you added two new constructor-args (whitelistEnabled, whitelist). Since SandboxInterceptorFactory isn’t present in this repo, confirm the external class provides a six-arg constructor in this exact order (or carries @ConstructorProperties/@ConstructorParameters for name-based binding), or specify index on each <constructor-arg> to eliminate ambiguity.

Comment thread src/main/resources/crafter/studio/studio-config.yaml
Comment thread src/test/resources/crafter/studio/groovy/whitelist Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (4)
src/main/resources/crafter/studio/groovy/whitelist (4)

1-2: Add explicit license attribution header (copied from Jenkins whitelist).

You reference Jenkins’ generic whitelist. To be safe for license compliance, add SPDX lines acknowledging MIT and original source.

Apply:

+# SPDX-FileCopyrightText: Jenkins project authors
+# SPDX-License-Identifier: MIT
 # Based on https://raw.githubusercontent.com/jenkinsci/script-security-plugin/master/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/generic-whitelist

716-717: Trim deprecated Date APIs (noise only).

toGMTString/toLocaleString are long-deprecated; safe to omit and reduce surface.

-method java.util.Date toGMTString
-method java.util.Date toLocaleString

1404-1409: Drop Jenkins Pipeline-only entries.

CpsScript and the Checker exception are Jenkins migration artifacts. Unless Studio runs legacy serialized CPS scripts, remove to avoid confusion.

-# Needed for normal Pipeline script instantiation. TODO: Add @Whitelisted annotation to the constructor and remove this entry.
-new org.jenkinsci.plugins.workflow.cps.CpsScript
-
-# No longer needed except for Pipelines serialized before groovy-cps changes for SECURITY-2824, so wait for most users to update and then remove it.
-staticMethod org.kohsuke.groovy.sandbox.impl.Checker checkedCast java.lang.Class java.lang.Object boolean boolean boolean

If you believe groovy-sandbox’s Checker.checkedCast is required by your sandbox loader, keep it and just drop CpsScript.


6-70: Overall: solid, conservative surface aligned with Jenkins’ generic list.

Good restraint on I/O, Process, Reflection, System props/env, and Files/Paths. The remaining nits above should make this production-safe.

I can add a tiny integration test that loads this whitelist and executes representative allowed/denied snippets—want me to open a follow-up?

Also applies to: 71-346, 347-583, 584-909, 910-1403

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between db9d732 and ef47147.

📒 Files selected for processing (3)
  • src/main/resources/crafter/studio/groovy/whitelist (1 hunks)
  • src/main/resources/crafter/studio/studio-config.yaml (1 hunks)
  • src/main/resources/crafter/studio/studio-services-context.xml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/main/resources/crafter/studio/studio-services-context.xml
  • src/main/resources/crafter/studio/studio-config.yaml
⏰ 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). (2)
  • GitHub Check: Travis CI - Pull Request
  • GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
src/main/resources/crafter/studio/groovy/whitelist (3)

312-314: SSRF guard note for URL usage (informational).

Allowing new URL(String) is fine; do not whitelist URL.openStream/openConnection or StringGroovyMethods.getText to keep network I/O blocked.


3-4: Retain the CPS-specific whitelist entry
The file already whitelists org.jenkinsci.plugins.workflow.cps.CpsScript (line 1405), showing that Studio ships and supports groovy-cps/workflow-cps—so the CpsDefaultGroovyMethods each entry is required for Pipeline scripts and must stay.

Likely an incorrect or invalid review comment.


789-796: Verify Java compatibility or gate JDK17-only RandomGenerator methods
No <maven-compiler-plugin> or <release> level was found in any POM to confirm JDK17 support. The java.util.random.RandomGenerator entries at src/main/resources/crafter/studio/groovy/whitelist:789-796 are JDK17-only and will fail on Java 11. Confirm the effective Java target or remove/gate these methods.

Comment thread src/main/resources/crafter/studio/groovy/whitelist
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/main/resources/crafter/studio/studio-services-context.xml (1)

1677-1690: Fix SandboxInterceptorFactory bean constructor args
The factory’s constructor only accepts four parameters—sandboxEnabled, blacklistEnabled, blacklist, and whitelistGetEnvRegex—but the bean definition supplies six (whitelistEnabled and whitelist aren’t in @ConstructorProperties). Remove or consolidate the extra args (or extend the factory) so the XML matches the actual constructor signature.

🧹 Nitpick comments (5)
src/main/java/org/craftercms/studio/impl/v1/script/GroovyScriptExecutor.java (3)

58-61: Null-safe classpath handling.

Defensive check avoids NPE if scriptsClassPath is null.

-    for (String classPath : scriptsClassPath) {
-      scriptEngine.getClassLoader().addClasspath(classPath);
-    }
+    if (scriptsClassPath != null) {
+      for (String classPath : scriptsClassPath) {
+        scriptEngine.getClassLoader().addClasspath(classPath);
+      }
+    }

66-68: Only register sandbox when enabled.

Avoids unnecessary register/unregister overhead when sandbox is disabled.

-    if (sandboxInterceptor != null) {
+    if (enableScriptSandbox && sandboxInterceptor != null) {
       sandboxInterceptor.register();
     }

37-37: Nit: constants modifier order.

Prefer “static final” order for constants.

-protected final static String GROOVY_ENGINE_NAME = "groovy";
+protected static final String GROOVY_ENGINE_NAME = "groovy";
src/main/resources/crafter/studio/groovy/whitelist (2)

1406-1414: Tighten the CommonLifecycleApi wildcard.

The entry method scripts.libs.CommonLifecycleApi * java.lang.Object java.lang.Object broadens the surface area. Prefer explicit method names to reduce risk.

If only the listed methods are needed, consider removing the wildcard line entirely.


697-717: Optional: prune deprecated java.util.Date mutators.

Methods like setYear/setMonth/getYear are long-deprecated; consider relying on java.time entries already present.

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between ef47147 and ce473d5.

📒 Files selected for processing (3)
  • src/main/java/org/craftercms/studio/impl/v1/script/GroovyScriptExecutor.java (2 hunks)
  • src/main/resources/crafter/studio/groovy/whitelist (1 hunks)
  • src/main/resources/crafter/studio/studio-services-context.xml (2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-03T18:41:57.465Z
Learnt from: jmendeza
PR: craftercms/studio#3806
File: src/main/resources/crafter/studio/groovy/whitelist:797-808
Timestamp: 2025-09-03T18:41:57.465Z
Learning: The Java 21 MatchResult interface includes default methods for named groups support: group(String), start(String), end(String), namedGroups(), and hasMatch(). These methods were added in Java 20 and are valid in Java 21.

Applied to files:

  • src/main/resources/crafter/studio/groovy/whitelist
📚 Learning: 2025-08-08T23:38:33.963Z
Learnt from: jmendeza
PR: craftercms/studio#3794
File: src/main/resources/crafter/studio/studio-services-context.xml:801-808
Timestamp: 2025-08-08T23:38:33.963Z
Learning: In CrafterCMS Studio's Spring XML configuration, beans can have partial constructor arguments specified in XML while Spring autowires the remaining arguments by type. The DiskMonitor class uses ConstructorProperties annotation which enables Spring to correctly match and inject the missing dependencies even when only some constructor arguments are explicitly provided in the XML configuration.

Applied to files:

  • src/main/resources/crafter/studio/studio-services-context.xml
⏰ 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: Codacy Static Code Analysis
🔇 Additional comments (3)
src/main/java/org/craftercms/studio/impl/v1/script/GroovyScriptExecutor.java (1)

42-47: Constructor-based injection looks good.

Final fields + @ConstructorProperties align with Spring XML wiring and prevent partial initialization.

src/main/resources/crafter/studio/groovy/whitelist (2)

798-809: MatchResult named-group methods are correct for Java 21.

group(String)/start(String)/end(String)/namedGroups()/hasMatch() are valid defaults in modern JDKs; keep them.


313-317: SSRF guard check passed: no URL I/O helper methods (getText, newReader, openStream, withInputStream) are whitelisted in the specified ranges.

Comment thread src/main/java/org/craftercms/studio/impl/v1/script/GroovyScriptExecutor.java Outdated
Comment thread src/main/resources/crafter/studio/studio-services-context.xml Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/main/resources/crafter/studio/groovy/whitelist (1)

1404-1411: Remove wildcard on CommonLifecycleApi; it defeats sandboxing.

method scripts.libs.CommonLifecycleApi * java.lang.Object java.lang.Object is overbroad and can open unintended surface if supported by the whitelist parser. Prefer explicit methods (you already list execute, getContentLifecycleParams, getLogger).

Apply this diff:

- method scripts.libs.CommonLifecycleApi * java.lang.Object java.lang.Object
🧹 Nitpick comments (2)
src/main/resources/crafter/studio/groovy/whitelist (2)

1404-1408: Allow common SLF4J overloads (varargs) for better DX without extra risk.

Current whitelist has only one debug overload. Adding varargs forms for debug/info/warn/error is safe and avoids friction in scripts.

 new scripts.libs.CommonLifecycleApi java.lang.Object
 staticMethod org.slf4j.LoggerFactory getLogger java.lang.Class
-method org.slf4j.Logger debug java.lang.String java.lang.Object java.lang.Object
+method org.slf4j.Logger debug java.lang.String
+method org.slf4j.Logger debug java.lang.String java.lang.Object[]
+method org.slf4j.Logger info java.lang.String
+method org.slf4j.Logger info java.lang.String java.lang.Object[]
+method org.slf4j.Logger warn java.lang.String
+method org.slf4j.Logger warn java.lang.String java.lang.Object[]
+method org.slf4j.Logger error java.lang.String
+method org.slf4j.Logger error java.lang.String java.lang.Object[]
 method scripts.libs.CommonLifecycleApi execute
 method scripts.libs.CommonLifecycleApi getContentLifecycleParams

1-2: Add provenance and update instructions in comments.

Tiny nit: add a short note with the source (commit/URL) and how to regenerate/sync this file to keep it coherent with upstream lists.

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between ce473d5 and 521f967.

📒 Files selected for processing (1)
  • src/main/resources/crafter/studio/groovy/whitelist (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-03T18:41:57.465Z
Learnt from: jmendeza
PR: craftercms/studio#3806
File: src/main/resources/crafter/studio/groovy/whitelist:797-808
Timestamp: 2025-09-03T18:41:57.465Z
Learning: The Java 21 MatchResult interface includes default methods for named groups support: group(String), start(String), end(String), namedGroups(), and hasMatch(). These methods were added in Java 20 and are valid in Java 21.

Applied to files:

  • src/main/resources/crafter/studio/groovy/whitelist
⏰ 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: Codacy Static Code Analysis
🔇 Additional comments (1)
src/main/resources/crafter/studio/groovy/whitelist (1)

795-806: Java 21 MatchResult named-group methods — correct, keep them.

The entries for group(String), start(String), end(String), namedGroups(), and hasMatch() are valid on Java 21.

Using your prior learning about Java 21’s MatchResult updates to confirm this.

Comment thread src/main/resources/crafter/studio/groovy/whitelist
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
src/main/java/org/craftercms/studio/impl/v1/script/GroovyScriptExecutor.java (1)

68-68: Bindings passed via eval: LGTM (matches prior suggestion)

This applies the earlier recommendation to bind the model per-eval instead of relying on manager-level globals.

🧹 Nitpick comments (1)
src/main/java/org/craftercms/studio/impl/v1/script/GroovyScriptExecutor.java (1)

49-60: Fail fast if engine unavailable; guard null/empty classpath

Avoid latent NPEs and tolerate empty classpath lists.

 protected void init() {
   ScriptEngineManager factory = new ScriptEngineManager();
-  this.scriptEngine = (GroovyScriptEngineImpl) factory.getEngineByName(GROOVY_ENGINE_NAME);
+  GroovyScriptEngineImpl engine = (GroovyScriptEngineImpl) factory.getEngineByName(GROOVY_ENGINE_NAME);
+  if (engine == null) {
+    throw new IllegalStateException("Groovy ScriptEngine not found: " + GROOVY_ENGINE_NAME);
+  }
+  this.scriptEngine = engine;
   CompilerConfiguration config = new CompilerConfiguration();
   if (enableScriptSandbox) {
     config.addCompilationCustomizers(new RejectASTTransformsCustomizer(), new SandboxTransformer());
   }
   scriptEngine.setClassLoader(new GroovyClassLoader(scriptEngine.getClassLoader(), config));
-  for (String classPath : scriptsClassPath) {
-    scriptEngine.getClassLoader().addClasspath(classPath);
-  }
+  if (scriptsClassPath != null && !scriptsClassPath.isEmpty()) {
+    GroovyClassLoader cl = scriptEngine.getClassLoader();
+    for (String classPath : scriptsClassPath) {
+      cl.addClasspath(classPath);
+    }
+  }
 }
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 0fdd668 and b350e1b.

📒 Files selected for processing (3)
  • src/main/java/org/craftercms/studio/impl/v1/script/GroovyScriptExecutor.java (2 hunks)
  • src/main/resources/crafter/studio/studio-config.yaml (1 hunks)
  • src/main/resources/crafter/studio/studio-services-context.xml (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/main/resources/crafter/studio/studio-config.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T23:38:33.963Z
Learnt from: jmendeza
PR: craftercms/studio#3794
File: src/main/resources/crafter/studio/studio-services-context.xml:801-808
Timestamp: 2025-08-08T23:38:33.963Z
Learning: In CrafterCMS Studio's Spring XML configuration, beans can have partial constructor arguments specified in XML while Spring autowires the remaining arguments by type. The DiskMonitor class uses ConstructorProperties annotation which enables Spring to correctly match and inject the missing dependencies even when only some constructor arguments are explicitly provided in the XML configuration.

Applied to files:

  • src/main/resources/crafter/studio/studio-services-context.xml
⏰ 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). (2)
  • GitHub Check: Travis CI - Pull Request
  • GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
src/main/java/org/craftercms/studio/impl/v1/script/GroovyScriptExecutor.java (3)

36-47: Constructor-based DI + @ConstructorProperties: LGTM

Clean switch to ctor injection with explicit parameter names. Matches the Spring wiring.


53-56: AST hardening is on point

Rejecting AST transforms and adding SandboxTransformer only when sandbox is enabled is correct.


40-41: Concurrency check: is GroovyScriptEngineImpl used concurrently here?

JSR-223 engines aren’t guaranteed to be thread-safe. If this executor is shared, please confirm concurrent eval is supported by the engine version in use or add a lightweight guard (e.g., synchronization or engine-per-thread) if needed.

src/main/resources/crafter/studio/studio-services-context.xml (1)

1684-1687: Verify SandboxInterceptorFactory constructor and environment config properties

  • Confirm SandboxInterceptorFactory defines a constructor matching the new <constructor-arg> names whitelistEnabled and whitelist in src/main/resources/crafter/studio/studio-services-context.xml (lines 1684–1687).
  • Ensure studio.scripting.sandbox.whitelist.enable and studio.scripting.sandbox.whitelist.path are present in the base studio-config.yaml and all environment-specific override files under src/main/resources/crafter/studio to avoid startup failures.

Comment thread src/main/resources/crafter/studio/studio-services-context.xml
@jmendeza
Copy link
Copy Markdown
Member Author

jmendeza commented Sep 9, 2025

Notice this PR depends on craftercms/commons#413

@jmendeza jmendeza marked this pull request as ready for review September 9, 2025 00:02
@jmendeza jmendeza requested a review from sumerjabri as a code owner September 9, 2025 00:02
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
src/main/resources/crafter/studio/groovy/whitelist (1)

793-804: MatchResult default methods are valid on Java 21 — keep as-is.

These entries (group(String), start(String), end(String), namedGroups, hasMatch) are correct for Java 20+.

🧹 Nitpick comments (7)
src/main/resources/crafter/studio/groovy/whitelist (7)

2-11: Add JsonOutput.toJson(List) to cover common cases.

Many scripts serialize lists; only scalars/Map are covered now.

Apply:

 staticMethod groovy.json.JsonOutput toJson java.util.Map
+staticMethod groovy.json.JsonOutput toJson java.util.List
 staticMethod groovy.json.JsonOutput toJson java.util.UUID

46-50: Allow PrintWriter.println for convenience.

Templates often use println; only print is allowed.

Apply:

 method java.io.PrintWriter print java.lang.Object
 method java.io.PrintWriter print java.lang.String
+method java.io.PrintWriter println java.lang.Object
+method java.io.PrintWriter println java.lang.String

514-516: Whitelist ArrayList no-arg and capacity constructors.

These are ubiquitous; only the collection ctor is allowed.

Apply:

+new java.util.ArrayList
+new java.util.ArrayList int
 new java.util.ArrayList java.util.Collection

831-839: Expose Pattern flag constants to avoid magic integers.

Without these, callers must pass raw ints to compile(..., int).

Apply:

 staticMethod java.util.regex.Pattern compile java.lang.String int
+staticField java.util.regex.Pattern CANON_EQ
+staticField java.util.regex.Pattern CASE_INSENSITIVE
+staticField java.util.regex.Pattern COMMENTS
+staticField java.util.regex.Pattern DOTALL
+staticField java.util.regex.Pattern LITERAL
+staticField java.util.regex.Pattern MULTILINE
+staticField java.util.regex.Pattern UNICODE_CASE
+staticField java.util.regex.Pattern UNICODE_CHARACTER_CLASS
+staticField java.util.regex.Pattern UNIX_LINES

38-41: Remove unreachable constructors (minor cleanup).

groovy.lang.Script is abstract; java.lang.Enum can’t be user-constructed.

Apply:

-new groovy.lang.Script
-new groovy.lang.Script groovy.lang.Binding
 ...
-new java.lang.Enum java.lang.String int

Also applies to: 127-129


777-780: Add UUID.fromString(String).

Commonly needed to parse UUIDs from input.

Apply:

 staticMethod java.util.UUID randomUUID
+staticMethod java.util.UUID fromString java.lang.String

242-245: Add String.getBytes(Charset) overload.

Completes the charset API surface already exposed elsewhere.

Apply:

 method java.lang.String getBytes
 method java.lang.String getBytes java.lang.String
+method java.lang.String getBytes java.nio.charset.Charset
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 715c1b7 and a6a9ad5.

📒 Files selected for processing (1)
  • src/main/resources/crafter/studio/groovy/whitelist (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-03T18:41:57.465Z
Learnt from: jmendeza
PR: craftercms/studio#3806
File: src/main/resources/crafter/studio/groovy/whitelist:797-808
Timestamp: 2025-09-03T18:41:57.465Z
Learning: The Java 21 MatchResult interface includes default methods for named groups support: group(String), start(String), end(String), namedGroups(), and hasMatch(). These methods were added in Java 20 and are valid in Java 21.

Applied to files:

  • src/main/resources/crafter/studio/groovy/whitelist
📚 Learning: 2025-09-05T18:02:53.138Z
Learnt from: jmendeza
PR: craftercms/studio#3806
File: src/main/resources/crafter/studio/groovy/whitelist:122-124
Timestamp: 2025-09-05T18:02:53.138Z
Learning: CrafterCMS 4.x requires Java 21, so Studio projects inheriting from the CrafterCMS parent POM automatically get Java 21 support without needing to declare it locally.

Applied to files:

  • src/main/resources/crafter/studio/groovy/whitelist
📚 Learning: 2025-09-05T18:02:53.138Z
Learnt from: jmendeza
PR: craftercms/studio#3806
File: src/main/resources/crafter/studio/groovy/whitelist:122-124
Timestamp: 2025-09-05T18:02:53.138Z
Learning: In Studio project, Java version baseline is inherited from the parent project rather than being declared locally in the build files.

Applied to files:

  • src/main/resources/crafter/studio/groovy/whitelist

@sumerjabri sumerjabri merged commit 026ce5a into craftercms:support/4.x Sep 10, 2025
2 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants