Copilot commented on code in PR #7973: URL: https://github.com/apache/incubator-seata/pull/7973#discussion_r2937816599
########## json-common/src/test/java/org/apache/seata/common/json/FastjsonAllowlistTest.java: ########## @@ -0,0 +1,244 @@ +/* + * 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 org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * Tests for FastJSON serializer with allowlist security check + */ +public class FastjsonAllowlistTest { + + private JsonSerializer jsonSerializer; + + @BeforeEach + void setUp() { + jsonSerializer = JsonSerializerFactory.getSerializer("fastjson"); + } + + @AfterEach + void tearDown() { + JsonAllowlistManager.getInstance().clearUserAllowlist(); + } + + @Test + public void testParseObject_allowedSeataClass() { + + String json = + "{\"@type\":\"org.apache.seata.common.json.FastjsonAllowlistTest$AllowedTestClass\",\"name\":\"test\"}"; + + AllowedTestClass result = jsonSerializer.parseObject(json, AllowedTestClass.class, false); + + assertThat(result).isNotNull(); + assertThat(result.getName()).isEqualTo("test"); + } + + @Test + public void testParseObject_allowedJavaClass() { + + String json = "{\"@type\":\"java.util.HashMap\"}"; + + Object result = jsonSerializer.parseObject(json, Object.class, false); + + assertThat(result).isNotNull(); + } + + @Test + public void testParseObject_notAllowedClass() { + + String json = "{\"@type\":\"com.malicious.EvilClass\",\"command\":\"rm -rf /\"}"; + + assertThatThrownBy(() -> jsonSerializer.parseObject(json, Object.class, false)) + .isInstanceOf(SecurityException.class) + .hasMessageContaining("not in JSON deserialization allowlist") + .hasMessageContaining("com.malicious.EvilClass"); + } + + @Test + public void testParseObject_userAllowedClass() { + + JsonAllowlistManager.getInstance().addUserClass("com.example.UserClass"); + + String json = "{\"@type\":\"com.example.UserClass\",\"data\":\"test\"}"; + + try { + jsonSerializer.parseObject(json, Object.class, false); + } catch (SecurityException e) { + throw e; + } catch (Exception e) { + assertThat(e).isNotInstanceOf(SecurityException.class); + } + } Review Comment: This test intentionally swallows all non-SecurityException failures, which can let the test pass even if parsing is broken for unrelated reasons. Consider making the assertion explicit (e.g., assert that the thrown exception, if any, is not a SecurityException) and/or introduce simple test classes under src/test/java (matching the allowed package names) so you can assert successful deserialization rather than ignoring exceptions. ########## json-common/src/main/java/org/apache/seata/common/json/impl/JacksonJsonSerializer.java: ########## @@ -95,7 +97,12 @@ public <T> T parseObjectWithType(String text, Type type) { return null; } try { + if (useAutoType(text)) { + checkAutoTypeClasses(text); + } return objectMapperWithAutoType.readValue(text, objectMapperWithAutoType.constructType(type)); Review Comment: The allowlist enforcement added here for Jackson AutoType deserialization is security-critical, but there are no unit tests validating that JacksonJsonSerializer actually throws SecurityException for non-allowed `@type` values (including nested `@type` occurrences) and allows user-configured allowlist entries. Adding a dedicated Jackson allowlist test (mirroring FastjsonAllowlistTest) would prevent regressions and ensure both serializers behave consistently. ########## json-common/src/test/java/org/apache/seata/common/json/FastjsonAllowlistTest.java: ########## @@ -0,0 +1,244 @@ +/* + * 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 org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * Tests for FastJSON serializer with allowlist security check + */ +public class FastjsonAllowlistTest { + + private JsonSerializer jsonSerializer; + + @BeforeEach + void setUp() { + jsonSerializer = JsonSerializerFactory.getSerializer("fastjson"); + } + + @AfterEach + void tearDown() { + JsonAllowlistManager.getInstance().clearUserAllowlist(); + } + + @Test + public void testParseObject_allowedSeataClass() { + + String json = + "{\"@type\":\"org.apache.seata.common.json.FastjsonAllowlistTest$AllowedTestClass\",\"name\":\"test\"}"; + + AllowedTestClass result = jsonSerializer.parseObject(json, AllowedTestClass.class, false); + + assertThat(result).isNotNull(); + assertThat(result.getName()).isEqualTo("test"); + } + + @Test + public void testParseObject_allowedJavaClass() { + + String json = "{\"@type\":\"java.util.HashMap\"}"; + + Object result = jsonSerializer.parseObject(json, Object.class, false); + + assertThat(result).isNotNull(); + } + + @Test + public void testParseObject_notAllowedClass() { + + String json = "{\"@type\":\"com.malicious.EvilClass\",\"command\":\"rm -rf /\"}"; + + assertThatThrownBy(() -> jsonSerializer.parseObject(json, Object.class, false)) + .isInstanceOf(SecurityException.class) + .hasMessageContaining("not in JSON deserialization allowlist") + .hasMessageContaining("com.malicious.EvilClass"); + } + + @Test + public void testParseObject_userAllowedClass() { + + JsonAllowlistManager.getInstance().addUserClass("com.example.UserClass"); + + String json = "{\"@type\":\"com.example.UserClass\",\"data\":\"test\"}"; + + try { + jsonSerializer.parseObject(json, Object.class, false); + } catch (SecurityException e) { + throw e; + } catch (Exception e) { + assertThat(e).isNotInstanceOf(SecurityException.class); + } + } + + @Test + public void testParseObject_userAllowedPrefix() { + JsonAllowlistManager.getInstance().addUserPrefix("com.mycompany.model."); + + String json = "{\"@type\":\"com.mycompany.model.User\",\"id\":1}"; + + try { + jsonSerializer.parseObject(json, Object.class, false); + } catch (SecurityException e) { + throw e; + } catch (Exception e) { + + assertThat(e).isNotInstanceOf(SecurityException.class); + } + } + + @Test + public void testParseObject_ignoreAutoType_bypasses_check() { + + String json = "{\"@type\":\"com.malicious.EvilClass\",\"command\":\"rm -rf /\"}"; + + try { + jsonSerializer.parseObject(json, Object.class, true); + } catch (SecurityException e) { + throw new AssertionError("Should not throw SecurityException when ignoreAutoType=true", e); + } catch (Exception e) { + + assertThat(e).isNotInstanceOf(SecurityException.class); + } + } + + @Test + public void testParseObject_noAutoType_bypasses_check() { + + String json = "{\"name\":\"test\",\"value\":123}"; + + TestObject result = jsonSerializer.parseObject(json, TestObject.class, false); + + assertThat(result).isNotNull(); + assertThat(result.getName()).isEqualTo("test"); + } + + @Test + public void testParseObject_multipleAutoTypes() { + + String json = "{\"@type\":\"org.apache.seata.common.json.FastjsonAllowlistTest$ContainerClass\"," + + "\"inner\":{\"@type\":\"org.apache.seata.common.json.FastjsonAllowlistTest$AllowedTestClass\",\"name\":\"nested\"}}"; + + ContainerClass result = jsonSerializer.parseObject(json, ContainerClass.class, false); + + assertThat(result).isNotNull(); + } + + @Test + public void testParseObject_multipleAutoTypes_oneNotAllowed() { + + String json = "{\"@type\":\"org.apache.seata.common.json.FastjsonAllowlistTest$ContainerClass\"," + + "\"inner\":{\"@type\":\"com.malicious.EvilClass\",\"name\":\"evil\"}}"; + + assertThatThrownBy(() -> jsonSerializer.parseObject(json, ContainerClass.class, false)) + .isInstanceOf(SecurityException.class) + .hasMessageContaining("com.malicious.EvilClass"); + } + + @Test + public void testParseObject_atTypeInStringValue_notBlocked() { + // @type appearing inside a string value should not be treated as AutoType metadata + String json = "{\"description\":\"the \\\"@type\\\" field is important\",\"name\":\"test\"}"; + + TestObject result = jsonSerializer.parseObject(json, TestObject.class, false); + + assertThat(result).isNotNull(); + assertThat(result.getName()).isEqualTo("test"); + } + + @Test + public void testLoadUserAllowlist_thenParse() { + JsonAllowlistManager.getInstance().loadUserAllowlist("com.trusted.model.,com.trusted.dto.SpecificDTO"); + + String json1 = "{\"@type\":\"com.trusted.model.User\",\"id\":1}"; + try { + jsonSerializer.parseObject(json1, Object.class, false); + } catch (SecurityException e) { + throw e; + } catch (Exception e) { + + } + + String json2 = "{\"@type\":\"com.trusted.dto.SpecificDTO\",\"data\":\"test\"}"; + try { + jsonSerializer.parseObject(json2, Object.class, false); + } catch (SecurityException e) { + throw e; + } catch (Exception e) { + + } + + String json3 = "{\"@type\":\"com.untrusted.EvilClass\",\"data\":\"evil\"}"; + assertThatThrownBy(() -> jsonSerializer.parseObject(json3, Object.class, false)) + .isInstanceOf(SecurityException.class); + } Review Comment: In testLoadUserAllowlist_thenParse the catch blocks for non-SecurityException are empty, so the test can pass even if deserialization fails due to unrelated errors (and it doesn't assert success for the allowed cases). It would be more robust to either assert successful parsing using real classes available on the test classpath, or at least assert that any exception thrown is not a SecurityException. -- 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]
