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 <[email protected]>
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: '[email protected]',
+ sendTo: '[email protected]',
+ 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: '[email protected]',
+ sendTo: '[email protected]',
+ 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", "[email protected]");
+ context.put("sendFrom", "[email protected]");
+ 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", "[email protected]");
+ context.put("sendFrom", "[email protected]");
+ 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>
