Copilot commented on code in PR #8104:
URL: https://github.com/apache/incubator-seata/pull/8104#discussion_r3296792041
##########
saga/seata-saga-engine/src/main/java/org/apache/seata/saga/engine/pcext/handlers/ScriptTaskStateHandler.java:
##########
@@ -104,6 +120,8 @@ public void process(ProcessContext context) throws
EngineExecutionException {
}
}
}
+ validateScriptSecurity(scriptType, scriptContent);
+
Review Comment:
`validateScriptSecurity` runs after `getScriptEngineFromCache(...)` has
already been called. For disallowed/invalid `scriptType`, this can still
create/cache a `ScriptEngine` (and for `null` scriptType it can NPE inside
`computeIfAbsent`). Validate `scriptType`/`scriptContent` before interacting
with the cache/engine manager so failures are deterministic and side-effect
free.
##########
saga/seata-saga-engine/src/main/java/org/apache/seata/saga/engine/serializer/impl/ExceptionSerializer.java:
##########
@@ -34,6 +39,31 @@ public class ExceptionSerializer implements
Serializer<Exception, byte[]> {
private static final Logger LOGGER =
LoggerFactory.getLogger(ExceptionSerializer.class);
+ private static final List<String> ALLOWED_CLASS_PREFIXES =
+ Arrays.asList("java.", "javax.", "org.apache.seata.", "[B",
"io.seata.", "org.springframework.");
+
+ static boolean isClassAllowed(String className) {
+ if (className.startsWith("[")) {
+ String stripped = className;
+ while (stripped.startsWith("[")) {
+ stripped = stripped.substring(1);
+ }
+ if (stripped.length() == 1) {
+ return true;
+ }
+ if (stripped.startsWith("L") && stripped.endsWith(";")) {
+ return isClassAllowed(stripped.substring(1, stripped.length()
- 1));
+ }
+ return false;
+ }
+ for (String prefix : ALLOWED_CLASS_PREFIXES) {
+ if (className.startsWith(prefix)) {
+ return true;
+ }
+ }
+ return false;
+ }
Review Comment:
The hard-coded `ALLOWED_CLASS_PREFIXES` only permits a small set of package
prefixes. Saga persists whatever `Exception` is thrown during execution (often
business exceptions from application packages), so this change can cause
state-log recovery/query to start failing with `SeataRuntimeException` when
encountering previously-serialized user exceptions. Consider making the
allowlist configurable/extendable (or deserializing into a safe placeholder
exception when a class is blocked) to avoid breaking persisted data while still
enforcing security.
##########
saga/seata-saga-spring/src/main/java/org/apache/seata/saga/engine/expression/spel/SpringELExpression.java:
##########
@@ -25,18 +27,34 @@
public class SpringELExpression implements ELExpression {
private org.springframework.expression.Expression expression;
+ private EvaluationContext evaluationContext;
public SpringELExpression(org.springframework.expression.Expression
expression) {
this.expression = expression;
}
+ public SpringELExpression(
+ org.springframework.expression.Expression expression,
EvaluationContext evaluationContext) {
+ this.expression = expression;
+ this.evaluationContext = evaluationContext;
+ }
+
@Override
public Object getValue(Object elContext) {
+ if (evaluationContext instanceof StandardEvaluationContext) {
+ ((StandardEvaluationContext)
evaluationContext).setRootObject(elContext);
+ return expression.getValue(evaluationContext);
+ }
return expression.getValue(elContext);
}
@Override
public void setValue(Object value, Object elContext) {
+ if (evaluationContext instanceof StandardEvaluationContext) {
+ ((StandardEvaluationContext)
evaluationContext).setRootObject(elContext);
+ expression.setValue(evaluationContext, value);
+ return;
Review Comment:
`SpringELExpression` mutates the shared `StandardEvaluationContext` by
calling `setRootObject` before every evaluation. These `Expression` instances
are cached and reused (e.g., `ChoiceStateHandler` stores evaluators in the
state definition), so concurrent executions can race and read/write the wrong
root object. Prefer using the SpEL overloads that accept a root object (e.g.,
`expression.getValue(evaluationContext, elContext)` /
`setValue(evaluationContext, elContext, value)`) or create/clone a context per
evaluation to keep this thread-safe.
##########
saga/seata-saga-engine/src/main/java/org/apache/seata/saga/engine/serializer/impl/ExceptionSerializer.java:
##########
@@ -66,7 +96,17 @@ public static Object deserializeByObjectInputStream(byte[]
bytes) {
Object result = null;
if (bytes != null) {
ByteArrayInputStream bais = new ByteArrayInputStream(bytes);
- try (ObjectInputStream ois = new ObjectInputStream(bais)) {
+ try (ObjectInputStream ois = new ObjectInputStream(bais) {
+ @Override
+ protected Class<?> resolveClass(ObjectStreamClass desc) throws
IOException, ClassNotFoundException {
+ if (!isClassAllowed(desc.getName())) {
+ throw new SeataRuntimeException(
+ ErrorCode.ERR_DESERIALIZATION_SECURITY,
+ "Failed to deserialize object: " +
desc.getName() + " is not permitted");
+ }
+ return super.resolveClass(desc);
+ }
+ }) {
Review Comment:
Only `resolveClass` is overridden for the allowlist check. Java
deserialization of dynamic proxies uses `resolveProxyClass(String[]
interfaces)` (which loads interface classes directly) and can bypass
`resolveClass`-only filtering. Add an override of `resolveProxyClass` that
validates each interface name with `isClassAllowed` (or switch to
`ObjectInputFilter`) to close this gap.
##########
saga/seata-saga-engine/src/main/java/org/apache/seata/saga/engine/pcext/handlers/ScriptTaskStateHandler.java:
##########
@@ -137,6 +155,17 @@ public void process(ProcessContext context) throws
EngineExecutionException {
}
}
+ static void validateScriptSecurity(String scriptType, String
scriptContent) {
+ if (scriptType != null &&
!ALLOWED_SCRIPT_TYPES.contains(scriptType.toLowerCase())) {
+ throw new EngineExecutionException(
+ "Disallowed script type: " + scriptType,
FrameworkErrorCode.ParameterRequired);
+ }
+ if (scriptContent != null &&
DANGEROUS_PATTERN.matcher(scriptContent).find()) {
+ throw new EngineExecutionException(
+ "Script content contains disallowed dangerous code
pattern", FrameworkErrorCode.ParameterRequired);
+ }
Review Comment:
`validateScriptSecurity` throws `FrameworkErrorCode.ParameterRequired`, but
these failures are “invalid parameter” cases (see
`FrameworkErrorCode.InvalidParameter`). Also, `scriptType.toLowerCase()` should
use `Locale.ROOT` to avoid locale-sensitive casing bugs. Consider treating
null/blank `scriptType` as invalid here to avoid later NPEs in the
script-engine cache.
##########
saga/seata-saga-spring/src/main/java/org/apache/seata/saga/engine/expression/spel/SpringELExpressionFactory.java:
##########
@@ -43,9 +46,22 @@ public SpringELExpressionFactory(ApplicationContext
applicationContext) {
@Override
public Expression createExpression(String expression) {
org.springframework.expression.Expression defaultExpression =
parser.parseExpression(expression);
- EvaluationContext evaluationContext = ((SpelExpression)
defaultExpression).getEvaluationContext();
- ((StandardEvaluationContext) evaluationContext).setBeanResolver(new
AppContextBeanResolver());
- return new SpringELExpression(defaultExpression);
+ StandardEvaluationContext evaluationContext =
createRestrictedEvaluationContext();
+ return new SpringELExpression(defaultExpression, evaluationContext);
+ }
+
+ private StandardEvaluationContext createRestrictedEvaluationContext() {
+ StandardEvaluationContext context = new StandardEvaluationContext();
+ context.setTypeLocator(new DenyAllTypeLocator());
+ context.setBeanResolver(new AppContextBeanResolver());
+ return context;
Review Comment:
PR description/title only mentions “optimize Saga-SpringEL”, but this PR
also changes Java deserialization security (`ExceptionSerializer`), script
execution security (`ScriptTaskStateHandler`), and adds a Hessian deserializer
helper. Please update the PR description/title to reflect the full scope, or
split into focused PRs to simplify review and rollout.
##########
saga/seata-saga-engine/src/main/java/org/apache/seata/saga/engine/pcext/handlers/ScriptTaskStateHandler.java:
##########
@@ -49,6 +53,18 @@ public class ScriptTaskStateHandler implements StateHandler,
InterceptableStateH
private static final Logger LOGGER =
LoggerFactory.getLogger(ScriptTaskStateHandler.class);
+ private static final Set<String> ALLOWED_SCRIPT_TYPES = new
HashSet<>(Arrays.asList("groovy", "js", "javascript"));
+
+ private static final Pattern DANGEROUS_PATTERN = Pattern.compile("(?i)"
+ + "Runtime|ProcessBuilder|\\.execute\\s*\\("
+ + "|System\\s*\\.\\s*(exit|getRuntime|setSecurityManager)"
+ + "|Class\\s*\\.\\s*forName|ClassLoader"
+ + "|java\\.lang\\.reflect"
+ + "|Thread\\s*\\.|\\.getClass\\s*\\("
+ + "|java\\.io\\.File|java\\.net\\."
+ + "|javax\\.script\\.ScriptEngine"
+ + "|GroovyShell|GroovyClassLoader");
Review Comment:
The `DANGEROUS_PATTERN` blacklist is easy to bypass because several tokens
require exact dot formatting (e.g., `java\.io\.File` won’t match `java
.io.File` or commented/concatenated variants). If this is meant as a security
boundary, consider using a stricter whitelist/sandbox for the scripting engine
or normalizing/parsing the script content rather than relying on a simple regex
blacklist.
##########
serializer/seata-serializer-hessian/src/main/java/org/apache/seata/serializer/hessian/FieldCountCappingDeserializer.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.seata.serializer.hessian;
+
+import com.caucho.hessian.io.AbstractHessianInput;
+import com.caucho.hessian.io.Deserializer;
+
+import java.io.IOException;
+
+/**
+ * Delegates to a Hessian {@link Deserializer} but rejects absurd
+ * {@code createFields(len)} values before the delegate allocates proportional
+ * to {@code len} (mitigates tiny-payload OOM on malicious Hessian class defs).
+ */
+final class FieldCountCappingDeserializer implements Deserializer {
+
+ private final Deserializer delegate;
+ private final int maxClassFields;
+
+ FieldCountCappingDeserializer(Deserializer delegate, int maxClassFields) {
+ this.delegate = delegate;
+ this.maxClassFields = maxClassFields;
+ }
+
+ @Override
+ public Class<?> getType() {
+ return delegate.getType();
+ }
+
+ @Override
+ public boolean isReadResolve() {
+ return delegate.isReadResolve();
+ }
+
+ @Override
+ public Object readObject(AbstractHessianInput in) throws IOException {
+ return delegate.readObject(in);
+ }
+
+ @Override
+ public Object readList(AbstractHessianInput in, int length) throws
IOException {
+ return delegate.readList(in, length);
+ }
+
+ @Override
+ public Object readLengthList(AbstractHessianInput in, int length) throws
IOException {
+ return delegate.readLengthList(in, length);
+ }
+
+ @Override
+ public Object readMap(AbstractHessianInput in) throws IOException {
+ return delegate.readMap(in);
+ }
+
+ @Override
+ public Object[] createFields(int len) {
+ if (len < 0 || len > maxClassFields) {
+ throw new IllegalStateException(
+ "Hessian class definition field count " + len + " exceeds
maximum " + maxClassFields);
+ }
+ return delegate.createFields(len);
+ }
+
+ @Override
+ public Object createField(String name) {
+ return delegate.createField(name);
+ }
+
+ @Override
+ public Object readObject(AbstractHessianInput in, Object[] fields) throws
IOException {
+ return delegate.readObject(in, fields);
+ }
+
+ @Override
+ public Object readObject(AbstractHessianInput in, String[] fieldNames)
throws IOException {
+ return delegate.readObject(in, fieldNames);
+ }
+}
Review Comment:
`FieldCountCappingDeserializer` is added but not referenced anywhere in the
Hessian serializer path (no usages in
`HessianSerializer`/`HessianSerializerFactory`). As-is it provides no
protection and adds dead code. Either wire it into the Hessian
`SerializerFactory`/deserializer creation flow (wrapping delegates) or remove
it until it’s used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
