From fe76c325b1cfb529480d310ea15fff08c400a7dd Mon Sep 17 00:00:00 2001 From: Sergio Ricardo Zerbetto Masson Date: Wed, 15 Jul 2026 14:59:36 -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 6576d20..4f791d2 100644 --- a/GLTFSDK.Test/Source/DeserializeTests.cpp +++ b/GLTFSDK.Test/Source/DeserializeTests.cpp @@ -392,6 +392,56 @@ namespace "materials": [{ "pbrMetallicRoughness": "not-an-object" }], "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 @@ -829,6 +879,93 @@ namespace Microsoft Deserialize(c_pbrMetallicRoughnessIsString, DeserializeFlags::None, SchemaFlags::DisableSchemaRoot); }); } + + // 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 980b5dd250c8b93fe3913b618a07e598405f98ce Mon Sep 17 00:00:00 2001 From: Sergio Ricardo Zerbetto Masson Date: Wed, 15 Jul 2026 15:04:53 -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 b1e4af6..6695dc0 100644 --- a/GLTFSDK/Inc/GLTFSDK/RapidJsonUtils.h +++ b/GLTFSDK/Inc/GLTFSDK/RapidJsonUtils.h @@ -208,8 +208,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 6276728..2f2bb73 100644 --- a/GLTFSDK/Source/Deserialize.cpp +++ b/GLTFSDK/Source/Deserialize.cpp @@ -435,9 +435,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())); } }