From af85acfa4e9afa57c613fde75f5e1f3ca2132cb3 Mon Sep 17 00:00:00 2001 From: Sergio Ricardo Zerbetto Masson Date: Wed, 15 Jul 2026 15:13:21 -0300 Subject: [PATCH 1/2] Add regression tests for array-typed member validation during deserialization Cover node children, node weights and mesh weights: a present member of the wrong container type, or an array element of the wrong type, must throw InvalidGLTFException. Includes positive coverage that valid arrays still deserialize. Tests currently fail against the unguarded parser. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 039828de-4db2-48f8-a5de-c9f7d33baffa --- GLTFSDK.Test/Source/DeserializeTests.cpp | 137 +++++++++++++++++++++++ 1 file changed, 137 insertions(+) diff --git a/GLTFSDK.Test/Source/DeserializeTests.cpp b/GLTFSDK.Test/Source/DeserializeTests.cpp index f316fb3..ad0973e 100644 --- a/GLTFSDK.Test/Source/DeserializeTests.cpp +++ b/GLTFSDK.Test/Source/DeserializeTests.cpp @@ -301,6 +301,56 @@ namespace ], "asset": {"version": "2.0"} })"; + + // Regression inputs for variable-length array members that the deserializer + // reads with rapidjson array accessors: node "children" (an array of node + // indices) and node/mesh "weights" (arrays of numbers, via the shared + // RapidJsonUtils::ToFloatArray helper). A present member of the wrong + // container type - or an array containing an element of the wrong type - + // must be rejected with InvalidGLTFException rather than read with accessors + // that are only well-defined on a JSON array of the expected element type. + // These pass SchemaFlags::DisableSchemaRoot so the malformed value reaches + // the semantic deserializers (a consumer that disables JSON-schema + // validation must still get a clean exception). + const char* c_validNodeChildren = R"({ + "nodes": [{ "children": [1, 2] }, {}, {}], + "asset": {"version": "2.0"} +})"; + + const char* c_nodeChildrenIsString = R"({ + "nodes": [{ "children": "not-an-array" }], + "asset": {"version": "2.0"} +})"; + + const char* c_nodeChildrenNonNumericElement = R"({ + "nodes": [{ "children": ["not-a-node-index"] }], + "asset": {"version": "2.0"} +})"; + + const char* c_validNodeWeights = R"({ + "nodes": [{ "weights": [0.25, 0.75] }], + "asset": {"version": "2.0"} +})"; + + const char* c_nodeWeightsIsString = R"({ + "nodes": [{ "weights": "not-an-array" }], + "asset": {"version": "2.0"} +})"; + + const char* c_nodeWeightsNonNumericElement = R"({ + "nodes": [{ "weights": ["not-a-number"] }], + "asset": {"version": "2.0"} +})"; + + const char* c_meshWeightsIsString = R"({ + "meshes": [{ "weights": "not-an-array" }], + "asset": {"version": "2.0"} +})"; + + const char* c_meshWeightsNonNumericElement = R"({ + "meshes": [{ "weights": ["not-a-number"] }], + "asset": {"version": "2.0"} +})"; } namespace Microsoft @@ -586,6 +636,93 @@ namespace Microsoft Deserialize(c_missingDependentPropertyBufferView); }); } + + // Positive regression: a well-formed node "children" array of + // node indices must still deserialize to the expected ids. + GLTFSDK_TEST_METHOD(DeserializeTests, DeserializeSuccess_ValidNodeChildren) + { + auto doc = Deserialize(c_validNodeChildren); + Assert::AreEqual(size_t(3), doc.nodes.Size()); + const auto& node = doc.nodes.Front(); + Assert::AreEqual(size_t(2), node.children.size()); + Assert::AreEqual(std::string("1"), node.children[0]); + Assert::AreEqual(std::string("2"), node.children[1]); + } + + // Negative regression: a node "children" member that is a JSON + // string (or any non-array value) must throw rather than be read + // with array accessors that are only defined on arrays. + GLTFSDK_TEST_METHOD(DeserializeTests, DeserializeFail_NodeChildrenIsString) + { + Assert::ExpectException([]() + { + Deserialize(c_nodeChildrenIsString, DeserializeFlags::None, SchemaFlags::DisableSchemaRoot); + }); + } + + // Negative regression: a "children" array whose elements are not + // node indices must throw rather than read each element as a uint. + GLTFSDK_TEST_METHOD(DeserializeTests, DeserializeFail_NodeChildrenNonNumericElement) + { + Assert::ExpectException([]() + { + Deserialize(c_nodeChildrenNonNumericElement, DeserializeFlags::None, SchemaFlags::DisableSchemaRoot); + }); + } + + // Positive regression: a well-formed node "weights" array of + // numbers must still deserialize. Uses SchemaFlags::DisableSchemaRoot + // because node "weights" has a JSON-schema dependency on "mesh"; + // disabling schema validation exercises the parser's happy path + // (RapidJsonUtils::ToFloatArray) directly. + GLTFSDK_TEST_METHOD(DeserializeTests, DeserializeSuccess_ValidNodeWeights) + { + auto doc = Deserialize(c_validNodeWeights, DeserializeFlags::None, SchemaFlags::DisableSchemaRoot); + Assert::AreEqual(size_t(1), doc.nodes.Size()); + const auto& node = doc.nodes.Front(); + Assert::AreEqual(size_t(2), node.weights.size()); + } + + // Negative regression: a node "weights" member that is a JSON + // string must throw (shared RapidJsonUtils::ToFloatArray helper). + GLTFSDK_TEST_METHOD(DeserializeTests, DeserializeFail_NodeWeightsIsString) + { + Assert::ExpectException([]() + { + Deserialize(c_nodeWeightsIsString, DeserializeFlags::None, SchemaFlags::DisableSchemaRoot); + }); + } + + // Negative regression: a node "weights" array whose elements are + // not numbers must throw rather than read each element as a number. + GLTFSDK_TEST_METHOD(DeserializeTests, DeserializeFail_NodeWeightsNonNumericElement) + { + Assert::ExpectException([]() + { + Deserialize(c_nodeWeightsNonNumericElement, DeserializeFlags::None, SchemaFlags::DisableSchemaRoot); + }); + } + + // Negative regression: a mesh "weights" member that is a JSON + // string must throw. Mesh weights share the same ToFloatArray + // helper as node weights, so the guard must live in the helper. + GLTFSDK_TEST_METHOD(DeserializeTests, DeserializeFail_MeshWeightsIsString) + { + Assert::ExpectException([]() + { + Deserialize(c_meshWeightsIsString, DeserializeFlags::None, SchemaFlags::DisableSchemaRoot); + }); + } + + // Negative regression: a mesh "weights" array whose elements are + // not numbers must throw. + GLTFSDK_TEST_METHOD(DeserializeTests, DeserializeFail_MeshWeightsNonNumericElement) + { + Assert::ExpectException([]() + { + Deserialize(c_meshWeightsNonNumericElement, DeserializeFlags::None, SchemaFlags::DisableSchemaRoot); + }); + } }; } } From 500630e8d0369a786e7588bcd2f60b4bdcb6b772 Mon Sep 17 00:00:00 2001 From: Sergio Ricardo Zerbetto Masson Date: Wed, 15 Jul 2026 15:13:21 -0300 Subject: [PATCH 2/2] Validate array-typed members before array access during deserialization Node children and node/mesh weights were read with rapidjson array and element accessors without first confirming the member is a JSON array of the expected element type. A present member of a different JSON type - or an array element of the wrong type - is now rejected with InvalidGLTFException before any array/number accessor is used. Node weights and mesh weights share RapidJsonUtils::ToFloatArray, so the element-type guard is centralized in that helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 039828de-4db2-48f8-a5de-c9f7d33baffa --- GLTFSDK/Inc/GLTFSDK/RapidJsonUtils.h | 12 ++++++++++++ GLTFSDK/Source/Deserialize.cpp | 13 ++++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/GLTFSDK/Inc/GLTFSDK/RapidJsonUtils.h b/GLTFSDK/Inc/GLTFSDK/RapidJsonUtils.h index 861499c..d952a53 100644 --- a/GLTFSDK/Inc/GLTFSDK/RapidJsonUtils.h +++ b/GLTFSDK/Inc/GLTFSDK/RapidJsonUtils.h @@ -189,8 +189,20 @@ namespace Microsoft const auto& it = v.FindMember(key.c_str()); if (it != v.MemberEnd()) { + // The member's storage is only well-defined as an array of + // numbers when it actually is one. Reject any other type + // before reading array/number accessors, otherwise a value + // of the wrong JSON type would be walked as a forged array. + if (!it->value.IsArray()) + { + throw InvalidGLTFException("The " + key + " member must be a JSON array"); + } for (rapidjson::Value::ConstValueIterator ait = it->value.Begin(); ait != it->value.End(); ++ait) { + if (!ait->IsNumber()) + { + throw InvalidGLTFException("The " + key + " array elements must be JSON numbers"); + } result.push_back(static_cast(ait->GetDouble())); } } diff --git a/GLTFSDK/Source/Deserialize.cpp b/GLTFSDK/Source/Deserialize.cpp index 19afa72..1da2b45 100644 --- a/GLTFSDK/Source/Deserialize.cpp +++ b/GLTFSDK/Source/Deserialize.cpp @@ -393,9 +393,20 @@ namespace if (it != v.MemberEnd()) { const rapidjson::Value& a = it->value; - node.children.reserve(a.Capacity()); + // children is an array of node indices; anything else must be + // rejected before reading array accessors, which are only + // well-defined on a JSON array. + if (!a.IsArray()) + { + throw InvalidGLTFException("Node children must be a JSON array"); + } + node.children.reserve(a.Size()); for (rapidjson::Value::ConstValueIterator ait = a.Begin(); ait != a.End(); ++ait) { + if (!ait->IsUint()) + { + throw InvalidGLTFException("Node children array elements must be unsigned integers"); + } node.children.push_back(std::to_string(ait->GetUint())); } }