Copilot commented on code in PR #7973:
URL: https://github.com/apache/incubator-seata/pull/7973#discussion_r2969521977
##########
json-common/src/main/java/org/apache/seata/common/json/impl/FastjsonJsonSerializer.java:
##########
@@ -132,13 +140,57 @@ public <T> T parseObject(String text, Class<T> type,
boolean ignoreAutoType) {
if ("[]".equals(text)) {
return (T) new java.util.ArrayList<>();
}
+
+ // Check allowlist when AutoType is enabled
+ if (!ignoreAutoType && useAutoType(text)) {
+ checkAutoTypeClasses(text);
+ }
+
if (ignoreAutoType) {
return JSON.parseObject(text, type,
READER_FEATURES_IGNORE_AUTO_TYPE);
} else {
return JSON.parseObject(text, type,
READER_FEATURES_SUPPORT_AUTO_TYPE);
Review Comment:
Same issue as above: allowlist validation is skipped unless
useAutoType(text) finds the literal "\"@type\"" substring, which is not a safe
detector for AutoType metadata and can be bypassed (e.g. via escaped field
names). For security, run the allowlist scan whenever ignoreAutoType=false (or
determine presence of `@type` via parsing) before calling JSON.parseObject with
SupportAutoType enabled.
##########
json-common/src/main/java/org/apache/seata/common/json/impl/FastjsonJsonSerializer.java:
##########
@@ -132,13 +140,57 @@ public <T> T parseObject(String text, Class<T> type,
boolean ignoreAutoType) {
if ("[]".equals(text)) {
return (T) new java.util.ArrayList<>();
}
+
+ // Check allowlist when AutoType is enabled
+ if (!ignoreAutoType && useAutoType(text)) {
+ checkAutoTypeClasses(text);
+ }
+
if (ignoreAutoType) {
return JSON.parseObject(text, type,
READER_FEATURES_IGNORE_AUTO_TYPE);
} else {
return JSON.parseObject(text, type,
READER_FEATURES_SUPPORT_AUTO_TYPE);
}
+ } catch (SecurityException e) {
+ throw e;
} catch (Exception e) {
throw new JsonParseException("FastJSON deserialize error", e);
}
}
+
+ /**
+ * Parse JSON in safe mode (IgnoreAutoType) and check all real @type
fields against allowlist
+ */
+ private void checkAutoTypeClasses(String json) {
+ Object parsed = JSON.parse(json, Feature.DisableSpecialKeyDetect,
Feature.OrderedField);
+ if (parsed instanceof JSONObject) {
+ checkJsonObject((JSONObject) parsed);
+ } else if (parsed instanceof JSONArray) {
+ checkJsonArray((JSONArray) parsed);
+ }
Review Comment:
The Javadoc says the JSON is parsed in safe mode (IgnoreAutoType), but the
implementation uses DisableSpecialKeyDetect (and does not set
Feature.IgnoreAutoType). Update the comment to reflect the actual safety
mechanism, or include Feature.IgnoreAutoType if that is the intended behavior.
##########
json-common/src/main/java/org/apache/seata/common/json/JsonAllowlistManager.java:
##########
@@ -0,0 +1,277 @@
+/*
+ * 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.common.json;
+
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * JSON deserialization allowlist manager.
+ */
+public class JsonAllowlistManager {
+
+ private static final JsonAllowlistManager INSTANCE = new
JsonAllowlistManager();
+
+ /**
+ * Built-in exact match allowlist
+ */
+ private final Set<String> builtinClasses = ConcurrentHashMap.newKeySet();
+
+ /**
+ * Built-in prefix match allowlist
+ */
+ private final Set<String> builtinPrefixes = ConcurrentHashMap.newKeySet();
+
+ /**
+ * User-defined exact match allowlist
+ */
+ private final Set<String> userClasses = ConcurrentHashMap.newKeySet();
+
+ /**
+ * User-defined prefix match allowlist
+ */
+ private final Set<String> userPrefixes = ConcurrentHashMap.newKeySet();
+
+ /**
+ * Check result cache (className -> allowed)
+ */
+ private final Map<String, Boolean> cache = new ConcurrentHashMap<>();
+
+ private JsonAllowlistManager() {
+ initBuiltinAllowlist();
+ }
+
+ public static JsonAllowlistManager getInstance() {
+ return INSTANCE;
+ }
+
+ /**
+ * Load user allowlist from configuration string
+ */
+ public void loadUserAllowlist(String config) {
+ userClasses.clear();
+ userPrefixes.clear();
+ cache.clear();
+ if (config == null || config.isEmpty()) {
+ return;
+ }
+ for (String item : config.split(",")) {
+ String trimmed = item.trim();
+ if (trimmed.isEmpty()) {
+ continue;
+ }
+ if (trimmed.endsWith(".")) {
+ userPrefixes.add(trimmed);
+ } else {
+ userClasses.add(trimmed);
+ }
+ }
+ }
+
+ /**
+ * Add a class to user allowlist programmatically
+ */
+ public void addUserClass(String className) {
+ if (className != null && !className.isEmpty()) {
+ userClasses.add(className);
+ cache.remove(className);
+ }
+ }
+
+ /**
+ * Add a prefix to user allowlist programmatically
+ */
+ public void addUserPrefix(String prefix) {
+ if (prefix != null && !prefix.isEmpty()) {
+ userPrefixes.add(prefix);
+ cache.clear();
+ }
+ }
+
+ /**
+ * Check if a class is allowed for deserialization
+ */
+ public boolean isAllowed(String className) {
+ if (className == null) {
+ return false;
+ }
+ return cache.computeIfAbsent(className, this::doCheck);
+ }
+
+ /**
+ * Check if a class is allowed, throw SecurityException if not
+ */
+ public void checkClass(String className) {
+ if (!isAllowed(className)) {
+ throw new SecurityException("Class not in JSON deserialization
allowlist: " + className
+ + ". Please add it to seata.json.allowlist
configuration.");
+ }
+ }
+
+ /**
+ * Clear user allowlist and cache.
+ */
+ public void clearUserAllowlist() {
+ userClasses.clear();
+ userPrefixes.clear();
+ cache.clear();
+ }
+
+ private boolean doCheck(String className) {
+ if (isExactOrPrefixAllowed(className)) {
+ return true;
+ }
+ String componentClassName = extractArrayComponentClassName(className);
+ return componentClassName != null &&
isExactOrPrefixAllowed(componentClassName);
+ }
+
+ private boolean isExactOrPrefixAllowed(String className) {
+ if (builtinClasses.contains(className)) {
+ return true;
+ }
+ for (String prefix : builtinPrefixes) {
+ if (className.startsWith(prefix)) {
+ return true;
+ }
+ }
+ if (userClasses.contains(className)) {
+ return true;
+ }
+ for (String prefix : userPrefixes) {
+ if (className.startsWith(prefix)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ private String extractArrayComponentClassName(String className) {
+ if (className == null || className.isEmpty()) {
+ return null;
+ }
+
+ String normalized = className;
+ while (normalized.endsWith("[]")) {
+ normalized = normalized.substring(0, normalized.length() - 2);
+ }
+ if (!normalized.equals(className)) {
+ return normalized;
+ }
+
+ if (!normalized.startsWith("[")) {
+ return null;
+ }
+ while (normalized.startsWith("[")) {
+ normalized = normalized.substring(1);
+ }
+ if (normalized.length() == 1) {
+ return null;
+ }
+ if (normalized.startsWith("L") && normalized.endsWith(";")) {
+ return normalized.substring(1, normalized.length() - 1);
+ }
+ return null;
Review Comment:
extractArrayComponentClassName returns null for primitive array descriptors
with multiple dimensions (e.g. "[[I", "[[Z"). With Jackson Id.CLASS default
typing, multi-dimensional primitive arrays serialize to these descriptors, so
they will be rejected even though 1D primitive arrays are explicitly
allowlisted. Consider treating any array of primitives as allowed (or enhancing
extraction to recognize primitive descriptors) and add coverage for
multi-dimensional primitive arrays.
##########
json-common/src/main/java/org/apache/seata/common/json/impl/FastjsonJsonSerializer.java:
##########
@@ -85,7 +88,12 @@ public <T> T parseObjectWithType(String text, Type type) {
return null;
}
try {
+ if (useAutoType(text)) {
+ checkAutoTypeClasses(text);
+ }
Review Comment:
Fastjson AutoType allowlist enforcement is gated by useAutoType(text) which
only checks for the literal substring "\"@type\"". This can be bypassed with
valid JSON that encodes the key using unicode escapes (e.g. "\u0040type"),
letting AutoType run without allowlist validation. Consider performing
checkAutoTypeClasses whenever ignoreAutoType is false (or detect `@type` by
parsing with DisableSpecialKeyDetect) instead of a raw substring check.
```suggestion
// Always perform AutoType allowlist validation before parsing.
checkAutoTypeClasses(text);
```
--
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]
