Groovy sandbox whitelist support#3817
Conversation
WalkthroughAdds Groovy script sandboxing support with a new constructor and engine setup in GroovyScriptExecutor, introduces whitelist configuration and optional Grape auto-download toggle, updates Spring wiring for sandbox/whitelist and classpath, and expands Groovy sandbox whitelist while adjusting blacklist comments and adding new Studio config properties. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Executor as GroovyScriptExecutor
participant Sandbox as SandboxInterceptor
participant Engine as GroovyScriptEngineImpl
Caller->>Executor: executeScriptString(siteId, script, model)
alt enableScriptSandbox == true
Executor->>Sandbox: register()
end
Executor->>Executor: getScriptEngine(siteId, model)
activate Executor
note over Executor: Build CompilerConfiguration<br/>+ optional RejectASTTransformsCustomizer<br/>+ SandboxTransformer when enabled
Executor->>Engine: instantiate with configured GroovyClassLoader
deactivate Executor
Executor->>Engine: eval(script, bindings)
Engine-->>Executor: result or exception
alt enableScriptSandbox == true
Executor->>Sandbox: unregister()
end
Executor-->>Caller: return result or propagate error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. 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: 0
🧹 Nitpick comments (3)
src/main/resources/crafter/studio/studio-config.yaml (1)
1074-1077: Consider default-enabling sandbox whitelist for stronger security posture.The whitelist is disabled by default (
false) while the blacklist is enabled. For a more secure-by-default configuration, consider enabling the whitelist by default, especially since the whitelist file is already provided.-studio.scripting.sandbox.whitelist.enable: true +studio.scripting.sandbox.whitelist.enable: truesrc/main/java/org/craftercms/studio/impl/v1/script/GroovyScriptExecutor.java (1)
61-76: Consider adding null checks for sandboxInterceptor.While the constructor accepts
sandboxInterceptorwhich could be null (based on Spring wiring), the code inexecuteScriptStringalready handles null checking. However, consider documenting this behavior or adding validation.protected ScriptEngine getScriptEngine(String siteId, Map<String, Object> model) { ScriptEngineManager factory = new ScriptEngineManager(); factory.setBindings(new SimpleBindings(model)); GroovyScriptEngineImpl scriptEngine = (GroovyScriptEngineImpl) factory.getEngineByName(GROOVY_ENGINE_NAME); CompilerConfiguration config = new CompilerConfiguration(); if (enableScriptSandbox) { + // Configure sandbox transformers for secure script execution config.addCompilationCustomizers(new RejectASTTransformsCustomizer(), new SandboxTransformer()); }src/main/resources/crafter/studio/extension/services-overlay-context.xml (1)
87-98: Grape auto-download controlled by config — property present; add debug logging (optional)
studio.scripting.grapes.download.enabled is defined at src/main/resources/crafter/studio/studio-config.yaml:1095 (false); the MethodInvokingFactoryBean in src/main/resources/crafter/studio/extension/services-overlay-context.xml uses studioConfiguration.getProperty(...) so the setting will be applied. Add brief debug logging when calling Grape.setEnableAutoDownload to aid troubleshooting Groovy dependency issues.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/org/craftercms/studio/impl/v1/script/GroovyScriptExecutor.java(3 hunks)src/main/resources/crafter/studio/extension/services-overlay-context.xml(2 hunks)src/main/resources/crafter/studio/groovy/blacklist(0 hunks)src/main/resources/crafter/studio/groovy/whitelist(1 hunks)src/main/resources/crafter/studio/studio-config.yaml(2 hunks)src/main/resources/crafter/studio/studio-services-context.xml(2 hunks)
💤 Files with no reviewable changes (1)
- src/main/resources/crafter/studio/groovy/blacklist
🧰 Additional context used
🧠 Learnings (2)
📚 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/extension/services-overlay-context.xml
📚 Learning: 2025-09-22T21:12:13.850Z
Learnt from: alhambrav
PR: craftercms/studio#3814
File: src/main/api/studio-api.yaml:7-7
Timestamp: 2025-09-22T21:12:13.850Z
Learning: Repo: craftercms/studio — Versioning workflow: maintainers update pom.xml, docs, and runtime version endpoints close to release; PRs that only bump OpenAPI info.version (src/main/api/studio-api.yaml) are acceptable without immediate pom/docs changes.
Applied to files:
src/main/resources/crafter/studio/studio-services-context.xmlsrc/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: Codacy Static Code Analysis
- GitHub Check: Travis CI - Pull Request
🔇 Additional comments (8)
src/main/resources/crafter/studio/studio-config.yaml (2)
1067-1068: LGTM! Good separation of sandbox configuration.The lifecycle scripts classpath property is appropriately configured with the default value.
1093-1096: Good security practice disabling Grapes auto-download.Disabling automatic dependency downloads by default prevents potential security vulnerabilities from untrusted or compromised dependencies being pulled into the runtime environment.
src/main/resources/crafter/studio/studio-services-context.xml (2)
1317-1325: Dynamic classpath configuration looks good.The
scriptsClassPathis now dynamically constructed using thestudio.scripting.lifecycle.classpathproperty, providing flexibility for different deployment scenarios.
1530-1533: LGTM! Whitelist configuration properly wired.The
sandboxInterceptorfactory bean now correctly accepts whitelist configuration from Studio properties.src/main/java/org/craftercms/studio/impl/v1/script/GroovyScriptExecutor.java (1)
78-91: Good sandbox lifecycle management.The sandbox interceptor is properly registered and unregistered in a try-finally block, ensuring cleanup even if script execution throws an exception.
src/main/resources/crafter/studio/groovy/whitelist (3)
1-14: Comprehensive Groovy core type whitelisting.Good coverage of essential Groovy JSON handling, bindings, and closure operations needed for typical scripting scenarios.
303-309: Security consideration for BigDecimal/BigInteger operations.While whitelisting
multiplyandnegateoperations on BigDecimal/BigInteger is necessary for calculations, be aware that unbounded precision arithmetic operations could potentially be exploited for denial of service through resource exhaustion.Consider monitoring script execution time and resource usage when these operations are used extensively.
1402-1446: Good Crafter Studio API exposure for lifecycle scripts.The whitelisted CommonLifecycleApi and DmContentLifeCycleService operations provide appropriate access to lifecycle hooks while maintaining security boundaries. The SLF4J logger exposure is also properly scoped.
|
Please notice this PR depends on craftercms/script-security-plugin#27 |
2 participants
https://github.com/craftersoftware/craftercms/issues/1156
Groovy sandbox whitelist support
Summary by CodeRabbit
New Features
Configuration