You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by as...@apache.org on 2026/08/23 18:19:38 UTC
(ofbiz-framework) branch trunk updated: Improved input handling and access control around the invoice email-sending flow (#1704)
This is an automated email from the ASF dual-hosted git repository.
ashishvijaywargiya pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
The following commit(s) were added to refs/heads/trunk by this push:
new 1da6eab73e Improved input handling and access control around the invoice email-sending flow (#1704)
1da6eab73e is described below
commit 1da6eab73e6ca5d09596878b96d553d81dd07f1f
Author: Ashish Vijaywargiya <as...@apache.org>
AuthorDate: Sun Aug 23 23:49:31 2026 +0530
Improved input handling and access control around the invoice email-sending flow (#1704)
Improved input handling and access control around the invoice
email-sending flow, and adds an extra safeguard to script evaluation
used elsewhere in the framework.
No functional change is intended for normal use of this feature.
Thank you Krishna Uprit for your help.
---
.../accounting/servicedef/services_invoice.xml | 1 +
.../SendInvoicePerEmailPermissionTests.groovy | 63 ++++++++++++++
applications/accounting/testdef/invoicetests.xml | 3 +
.../org/apache/ofbiz/base/util/GroovyUtil.java | 26 ++++--
.../apache/ofbiz/base/util/GroovyUtilTests.java | 61 ++++++++++++++
framework/common/ofbiz-component.xml | 1 +
.../apache/ofbiz/common/email/EmailServices.java | 12 +--
.../common/email/EmailServicesInjectionTests.java | 95 ++++++++++++++++++++++
.../common/testdef/EmailServicesInjectionTests.xml | 14 ++--
9 files changed, 256 insertions(+), 20 deletions(-)
diff --git a/applications/accounting/servicedef/services_invoice.xml b/applications/accounting/servicedef/services_invoice.xml
index 9a4ab37ba6..43e1fc0309 100644
--- a/applications/accounting/servicedef/services_invoice.xml
+++ b/applications/accounting/servicedef/services_invoice.xml
@@ -285,6 +285,7 @@ under the License.
<service name="sendInvoicePerEmail" engine="groovy"
location="component://accounting/src/main/groovy/org/apache/ofbiz/accounting/invoice/InvoiceServicesScript.groovy" invoke="sendInvoicePerEmail">
<description>Send an invoice per email</description>
+ <permission-service service-name="acctgInvoicePermissionCheck" main-action="VIEW"/>
<attribute name="invoiceId" type="String" mode="IN"/>
<attribute name="sendFrom" type="String" mode="IN"/>
<attribute name="sendTo" type="String" mode="IN"/>
diff --git a/applications/accounting/src/test/groovy/org/apache/ofbiz/accounting/invoice/SendInvoicePerEmailPermissionTests.groovy b/applications/accounting/src/test/groovy/org/apache/ofbiz/accounting/invoice/SendInvoicePerEmailPermissionTests.groovy
new file mode 100644
index 0000000000..515d9a52e2
--- /dev/null
+++ b/applications/accounting/src/test/groovy/org/apache/ofbiz/accounting/invoice/SendInvoicePerEmailPermissionTests.groovy
@@ -0,0 +1,63 @@
+/*******************************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *******************************************************************************/
+package org.apache.ofbiz.accounting.invoice
+
+import org.apache.ofbiz.service.ServiceAuthException
+import org.apache.ofbiz.service.ServiceUtil
+import org.apache.ofbiz.testtools.JunitJupiterTest
+import org.apache.ofbiz.testtools.JupiterTestHelper
+import org.junit.jupiter.api.Test
+
+/**
+ * sendInvoicePerEmail declared no permission-service, unlike its siblings in the same file
+ * (createInvoice, getInvoice, setInvoiceStatus), so any authenticated user could reach it regardless of
+ * accounting permissions. These tests guard the fix that gives it the same VIEW-level
+ * acctgInvoicePermissionCheck getInvoice already declares.
+ */
+@JunitJupiterTest
+class SendInvoicePerEmailPermissionTests implements JupiterTestHelper {
+
+ @Test
+ void sendInvoicePerEmailDeniesUserWithoutAccountingPermission() {
+ // DemoCustomer carries no UserLoginSecurityGroup membership anywhere in the demo data set.
+ Map params = [invoiceId: 'DEMO_PERM_TEST_NONEXISTENT',
+ sendFrom: 'nobody@example.org',
+ sendTo: 'nobody@example.org',
+ userLogin: getUserLogin('DemoCustomer')]
+ try {
+ Map results = getDispatcher().runSync('sendInvoicePerEmail', params)
+ assert ServiceUtil.isError(results):
+ 'sendInvoicePerEmail must not succeed for a userLogin holding no accounting permission'
+ } catch (ServiceAuthException e) {
+ assert e != null
+ }
+ }
+
+ @Test
+ void sendInvoicePerEmailAllowsUserWithAccountingViewPermission() {
+ Map params = [invoiceId: 'DEMO_PERM_TEST_NONEXISTENT',
+ sendFrom: 'nobody@example.org',
+ sendTo: 'nobody@example.org',
+ userLogin: getUserLogin()]
+ Map results = getDispatcher().runSync('sendInvoicePerEmail', params)
+ assert ServiceUtil.isSuccess(results):
+ 'sendInvoicePerEmail must still succeed for a fully-permissioned userLogin'
+ }
+
+}
diff --git a/applications/accounting/testdef/invoicetests.xml b/applications/accounting/testdef/invoicetests.xml
index 61f6347d4b..505a2f9a3b 100644
--- a/applications/accounting/testdef/invoicetests.xml
+++ b/applications/accounting/testdef/invoicetests.xml
@@ -28,4 +28,7 @@
<test-case case-name="invoice-per-shipment-tests">
<jupiter-test-suite class-name="org.apache.ofbiz.accounting.accounting.InvoicePerShipmentTests"/>
</test-case>
+ <test-case case-name="send-invoice-per-email-permission-tests">
+ <jupiter-test-suite class-name="org.apache.ofbiz.accounting.invoice.SendInvoicePerEmailPermissionTests"/>
+ </test-case>
</test-suite>
diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/util/GroovyUtil.java b/framework/base/src/main/java/org/apache/ofbiz/base/util/GroovyUtil.java
index 4db6df99e4..51aec9a1f2 100644
--- a/framework/base/src/main/java/org/apache/ofbiz/base/util/GroovyUtil.java
+++ b/framework/base/src/main/java/org/apache/ofbiz/base/util/GroovyUtil.java
@@ -59,16 +59,31 @@ public final class GroovyUtil {
if (!scriptBaseClass.isEmpty()) {
CompilerConfiguration conf = new CompilerConfiguration();
conf.setScriptBaseClass(scriptBaseClass);
+ // Same compile-time AST restrictions as SANDBOXED_COMPILER_CONFIG below, so that parseClass(),
+ // not just eval(), refuses OS-execution APIs and dynamic class-loading.
+ conf.addCompilationCustomizers(buildSecureAstCustomizer());
groovyClassLoader = new GroovyClassLoader(GroovyUtil.class.getClassLoader(), conf);
}
GROOVY_CLASS_LOADER = groovyClassLoader;
}
static {
- // Compile-time AST restrictions applied to eval() expressions.
- // Blocks OS-execution APIs and dynamic class-loading as a defence-in-depth measure.
- // Note: SecureASTCustomizer operates at compile time and does not constitute a
- // complete sandbox; eval() expressions should never originate from untrusted input.
+ SANDBOXED_COMPILER_CONFIG = new CompilerConfiguration();
+ SANDBOXED_COMPILER_CONFIG.addCompilationCustomizers(buildSecureAstCustomizer());
+ }
+
+ /**
+ * Builds a fresh {@link SecureASTCustomizer} applying the compile-time AST restrictions used by both
+ * GROOVY_CLASS_LOADER and SANDBOXED_COMPILER_CONFIG. Blocks OS-execution APIs and dynamic class-loading
+ * as a defence-in-depth measure.
+ * <p>Returns a new instance on every call rather than a shared constant: {@code SecureASTCustomizer}
+ * visits the AST during compilation, so handing the same instance to two {@code CompilerConfiguration}s
+ * used concurrently would be unsafe.
+ * <p>Note: SecureASTCustomizer operates at compile time and does not constitute a complete sandbox;
+ * expressions compiled through either path should never originate from untrusted input.
+ * @return a new, independently-usable SecureASTCustomizer
+ */
+ private static SecureASTCustomizer buildSecureAstCustomizer() {
SecureASTCustomizer secureAst = new SecureASTCustomizer();
secureAst.setDisallowedImports(List.of(
"java.lang.Runtime",
@@ -88,8 +103,7 @@ public final class GroovyUtil {
Thread.class,
ClassLoader.class);
secureAst.setDisallowedReceiversClasses(blockedReceivers);
- SANDBOXED_COMPILER_CONFIG = new CompilerConfiguration();
- SANDBOXED_COMPILER_CONFIG.addCompilationCustomizers(secureAst);
+ return secureAst;
}
/**
diff --git a/framework/base/src/test/java/org/apache/ofbiz/base/util/GroovyUtilTests.java b/framework/base/src/test/java/org/apache/ofbiz/base/util/GroovyUtilTests.java
new file mode 100644
index 0000000000..f306a3b0a4
--- /dev/null
+++ b/framework/base/src/test/java/org/apache/ofbiz/base/util/GroovyUtilTests.java
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.ofbiz.base.util;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import org.codehaus.groovy.control.CompilationFailedException;
+import org.junit.jupiter.api.Test;
+
+public class GroovyUtilTests {
+
+ /**
+ * GroovyUtil.parseClass() backs ScriptUtil.parseScript(), which is what FlexibleStringExpander uses to
+ * compile every {@code ${groovy:...}} substring it finds -- including ones that originate from a
+ * caller-supplied string reaching FlexibleStringExpander.expandString() before any application-level
+ * fix strips that call out. Before the fix, parseClass() used a GroovyClassLoader with no
+ * SecureASTCustomizer, so this construction compiled cleanly. ProcessBuilder is java.lang, so no
+ * import statement is needed to reach it -- an import denylist alone would not catch this.
+ */
+ @Test
+ public void parseClassRejectsProcessBuilderConstruction() {
+ String maliciousScript = "new ProcessBuilder(['id']).start()";
+ assertThrows(CompilationFailedException.class, () -> GroovyUtil.parseClass(maliciousScript),
+ "GroovyUtil.parseClass() must refuse to compile a script that constructs a ProcessBuilder");
+ }
+
+ /**
+ * Same restriction, reached via a method call on an existing receiver rather than a constructor call.
+ */
+ @Test
+ public void parseClassRejectsRuntimeExec() {
+ String maliciousScript = "Runtime.getRuntime().exec('id')";
+ assertThrows(CompilationFailedException.class, () -> GroovyUtil.parseClass(maliciousScript),
+ "GroovyUtil.parseClass() must refuse to compile a script that calls Runtime.exec()");
+ }
+
+ /**
+ * The restriction must not be so broad that it breaks compiling ordinary, non-malicious scripts --
+ * the shape every legitimate internal ${groovy:...} scriptlet and .groovy script location has.
+ */
+ @Test
+ public void parseClassStillAcceptsOrdinaryScripts() {
+ assertDoesNotThrow(() -> GroovyUtil.parseClass("1 + 1"),
+ "GroovyUtil.parseClass() must keep compiling ordinary, non-malicious scripts");
+ }
+}
diff --git a/framework/common/ofbiz-component.xml b/framework/common/ofbiz-component.xml
index 32bac50a1a..7d4b06331f 100644
--- a/framework/common/ofbiz-component.xml
+++ b/framework/common/ofbiz-component.xml
@@ -85,4 +85,5 @@ under the License.
<test-suite loader="main" location="testdef/UserLoginTests.xml"/>
<test-suite loader="main" location="testdef/PerformFindTests.xml"/>
+ <test-suite loader="main" location="testdef/EmailServicesInjectionTests.xml"/>
</ofbiz-component>
diff --git a/framework/common/src/main/java/org/apache/ofbiz/common/email/EmailServices.java b/framework/common/src/main/java/org/apache/ofbiz/common/email/EmailServices.java
index c2cacb918b..550a5c7313 100644
--- a/framework/common/src/main/java/org/apache/ofbiz/common/email/EmailServices.java
+++ b/framework/common/src/main/java/org/apache/ofbiz/common/email/EmailServices.java
@@ -72,7 +72,6 @@ import org.apache.ofbiz.base.util.UtilMisc;
import org.apache.ofbiz.base.util.UtilProperties;
import org.apache.ofbiz.base.util.UtilValidate;
import org.apache.ofbiz.base.util.collections.MapStack;
-import org.apache.ofbiz.base.util.string.FlexibleStringExpander;
import org.apache.ofbiz.entity.Delegator;
import org.apache.ofbiz.entity.GenericEntityException;
import org.apache.ofbiz.entity.GenericValue;
@@ -576,7 +575,8 @@ public class EmailServices {
if (UtilValidate.isNotEmpty(xslfoAttachScreenLocationList)) {
List<Map<String, ? extends Object>> bodyParts = new LinkedList<>();
if (bodyText != null) {
- bodyText = FlexibleStringExpander.expandString(bodyText, screenContext, locale);
+ // bodyText is caller-supplied (e.g. from a request parameter); it must not be run through
+ // FlexibleStringExpander, which would compile and execute any ${groovy:...} substring it contains.
bodyParts.add(UtilMisc.<String, Object>toMap("content", bodyText, "type", UtilValidate.isNotEmpty(contentType) ? contentType
: "text/html"));
} else {
@@ -635,7 +635,7 @@ public class EmailServices {
isMultiPart = false;
// store body and type for single part message in the context.
if (bodyText != null) {
- bodyText = FlexibleStringExpander.expandString(bodyText, screenContext, locale);
+ // bodyText is caller-supplied; see the comment on the identical guard above.
serviceContext.put("body", bodyText);
} else {
serviceContext.put("body", bodyWriter.toString());
@@ -650,11 +650,11 @@ public class EmailServices {
}
}
- // also expand the subject at this point, just in case it has the FlexibleStringExpander syntax in it...
+ // subject is caller-supplied; do not run it through FlexibleStringExpander, which would compile and
+ // execute any ${groovy:...} substring it contains -- same reasoning as the bodyText guards above.
String subject = (String) serviceContext.remove("subject");
- subject = FlexibleStringExpander.expandString(subject, screenContext, locale);
if (Debug.infoOn()) {
- Debug.logInfo("Expanded email subject to: " + subject, MODULE);
+ Debug.logInfo("Email subject: " + subject, MODULE);
}
serviceContext.put("subject", subject);
serviceContext.put("partyId", partyId);
diff --git a/framework/common/src/test/java/org/apache/ofbiz/common/email/EmailServicesInjectionTests.java b/framework/common/src/test/java/org/apache/ofbiz/common/email/EmailServicesInjectionTests.java
new file mode 100644
index 0000000000..deb5741774
--- /dev/null
+++ b/framework/common/src/test/java/org/apache/ofbiz/common/email/EmailServicesInjectionTests.java
@@ -0,0 +1,95 @@
+/*******************************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *******************************************************************************/
+package org.apache.ofbiz.common.email;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.ofbiz.testtools.JunitJupiterTest;
+import org.apache.ofbiz.testtools.JupiterTestHelper;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+/**
+ * sendMailFromScreen's bodyText and subject are ordinary caller-supplied strings -- for example, the
+ * "Send Per Email" invoice form (accounting's InvoiceForms.xml) posts bodyText straight from a request
+ * textarea. Before the fix, EmailServices ran both through FlexibleStringExpander.expandString(), which
+ * compiles and executes any {@code ${groovy:...}} substring it finds using a GroovyClassLoader carrying
+ * no SecureASTCustomizer -- arbitrary code execution reachable from a form field held by any
+ * authenticated user the webapp lets in. This guards the fix that stops expanding that text at all.
+ *
+ * <p>The marker below is set via a fully-qualified call to a static method on this class rather than
+ * {@code System.setProperty(...)} deliberately: security.properties' deniedScriptletsTokens denylist
+ * (a separate, pre-existing control -- see security.properties' "System\s*\." token) would otherwise
+ * block the marker script before it ever reached the code path this test exists to guard, making the
+ * test pass for the wrong reason regardless of the fix under test. Confirmed by running this test
+ * against the pre-fix code: with {@code System.setProperty(...)} it passed vacuously (blocked by the
+ * denylist); with this marker it fails as expected, proving it actually exercises the vulnerability.
+ */
+@JunitJupiterTest
+public final class EmailServicesInjectionTests implements JupiterTestHelper {
+
+ private static volatile boolean marker;
+
+ private static final String INJECTION =
+ "before-${groovy: org.apache.ofbiz.common.email.EmailServicesInjectionTests.setMarker() }-after";
+
+ /** Called from the injected scriptlet, if it is ever compiled and executed. */
+ public static void setMarker() {
+ marker = true;
+ }
+
+ @AfterEach
+ public void clearMarker() {
+ marker = false;
+ }
+
+ @Test
+ public void sendMailFromScreenDoesNotEvaluateGroovyInBodyText() throws Exception {
+ Map<String, Object> context = new HashMap<>();
+ context.put("userLogin", getUserLogin());
+ context.put("bodyText", INJECTION);
+ context.put("sendTo", "nobody@example.org");
+ context.put("sendFrom", "nobody@example.org");
+ context.put("contentType", "text/plain");
+
+ // sendMailFromScreen goes on to fail the actual send (no SMTP server in the test environment);
+ // that happens after the injection point and is not what this test is about, so any result --
+ // success or error -- is fine here. Only the marker matters.
+ getDispatcher().runSync("sendMailFromScreen", context);
+
+ assertFalse(marker, "bodyText must not be compiled and executed as Groovy by sendMailFromScreen");
+ }
+
+ @Test
+ public void sendMailFromScreenDoesNotEvaluateGroovyInSubject() throws Exception {
+ Map<String, Object> context = new HashMap<>();
+ context.put("userLogin", getUserLogin());
+ context.put("subject", INJECTION);
+ context.put("sendTo", "nobody@example.org");
+ context.put("sendFrom", "nobody@example.org");
+ context.put("contentType", "text/plain");
+
+ getDispatcher().runSync("sendMailFromScreen", context);
+
+ assertFalse(marker, "subject must not be compiled and executed as Groovy by sendMailFromScreen");
+ }
+}
diff --git a/applications/accounting/testdef/invoicetests.xml b/framework/common/testdef/EmailServicesInjectionTests.xml
similarity index 70%
copy from applications/accounting/testdef/invoicetests.xml
copy to framework/common/testdef/EmailServicesInjectionTests.xml
index 61f6347d4b..f276074033 100644
--- a/applications/accounting/testdef/invoicetests.xml
+++ b/framework/common/testdef/EmailServicesInjectionTests.xml
@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="UTF-8" ?>
+<?xml version="1.0" encoding="UTF-8"?>
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
@@ -16,16 +16,14 @@
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
- -->
+-->
-<test-suite suite-name="invoicetests"
+<test-suite suite-name="emailservicesinjectiontests"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="https://ofbiz.apache.org/dtds/test-suite.xsd">
- <test-group case-name="auto-invoice-tests">
- <jupiter-test-suite class-name="org.apache.ofbiz.accounting.accounting.AutoInvoiceTests"/>
- </test-group>
- <test-case case-name="invoice-per-shipment-tests">
- <jupiter-test-suite class-name="org.apache.ofbiz.accounting.accounting.InvoicePerShipmentTests"/>
+ <test-case case-name="email-services-injection-tests">
+ <jupiter-test-suite class-name="org.apache.ofbiz.common.email.EmailServicesInjectionTests"/>
</test-case>
+
</test-suite>