THRIFT-5802: Validate set uniqueness in Node.js library

compiler/cpp/src/thrift/generate/t_js_generator.cc

@@ -193,7 +193,10 @@ class t_js_generator : public t_oop_generator {
 
   void generate_deserialize_container(std::ostream& out, t_type* ttype, std::string prefix = "");
 
-  void generate_deserialize_set_element(std::ostream& out, t_set* tset, std::string prefix = "");
+  void generate_deserialize_set_element(std::ostream& out,
+                                        t_set* tset,
+                                        std::string prefix,
+                                        std::string seen);
 
   void generate_deserialize_map_element(std::ostream& out, t_map* tmap, std::string prefix = "");
 
@@ -2397,6 +2400,7 @@ void t_js_generator::generate_deserialize_struct(ostream& out, t_struct* tstruct
 void t_js_generator::generate_deserialize_container(ostream& out, t_type* ttype, string prefix) {
   string size = tmp("_size");
   string rtmp3 = tmp("_rtmp3");
+  string seen; // populated only for sets; gives O(1) per-element dedup
 
   t_field fsize(g_type_i32, size);
 
@@ -2408,8 +2412,10 @@ void t_js_generator::generate_deserialize_container(ostream& out, t_type* ttype,
     out << indent() << js_const_type_ << size << " = " << rtmp3 << ".size || 0;" << '\n';
 
   } else if (ttype->is_set()) {
+    seen = tmp("_seen");
 
     out << indent() << prefix << " = [];" << '\n'
+        << indent() << js_const_type_ << seen << " = new Set();" << '\n'
         << indent() << js_const_type_ << rtmp3 << " = input.readSetBegin();" << '\n'
         << indent() << js_const_type_ << size << " = " << rtmp3 << ".size || 0;" << '\n';
 
@@ -2436,7 +2442,7 @@ void t_js_generator::generate_deserialize_container(ostream& out, t_type* ttype,
 
     generate_deserialize_map_element(out, (t_map*)ttype, prefix);
   } else if (ttype->is_set()) {
-    generate_deserialize_set_element(out, (t_set*)ttype, prefix);
+    generate_deserialize_set_element(out, (t_set*)ttype, prefix, seen);
   } else if (ttype->is_list()) {
     generate_deserialize_list_element(out, (t_list*)ttype, prefix);
   }
@@ -2471,15 +2477,26 @@ void t_js_generator::generate_deserialize_map_element(ostream& out, t_map* tmap,
   indent(out) << prefix << "[" << key << "] = " << val << ";" << '\n';
 }
 
-void t_js_generator::generate_deserialize_set_element(ostream& out, t_set* tset, string prefix) {
+void t_js_generator::generate_deserialize_set_element(ostream& out,
+                                                      t_set* tset,
+                                                      string prefix,
+                                                      string seen) {
   string elem = tmp("elem");
   t_field felem(tset->get_elem_type(), elem);
 
   indent(out) << js_let_type_ << elem << " = null;" << '\n';
 
   generate_deserialize_field(out, &felem);
 
+  // O(1) dedup against a parallel Set, allocated once in the container header.
+  // SameValueZero on Set matches indexOf-with-=== for primitives and is
+  // equivalent (reference equality) for complex element types.
+  indent(out) << "if (!" << seen << ".has(" << elem << ")) {" << '\n';
+  indent_up();
+  indent(out) << seen << ".add(" << elem << ");" << '\n';
   indent(out) << prefix << ".push(" << elem << ");" << '\n';
+  indent_down();
+  indent(out) << "}" << '\n';
 }
 
 void t_js_generator::generate_deserialize_list_element(ostream& out,
@@ -2589,6 +2606,7 @@ void t_js_generator::generate_serialize_container(ostream& out, t_type* ttype, s
                 << type_to_enum(((t_map*)ttype)->get_val_type()) << ", "
                 << "Thrift.objectLength(" << prefix << "));" << '\n';
   } else if (ttype->is_set()) {
+    indent(out) << "Thrift.checkSetUniqueness(" << prefix << ");" << '\n';
     indent(out) << "output.writeSetBegin(" << type_to_enum(((t_set*)ttype)->get_elem_type()) << ", "
                 << prefix << ".length);" << '\n';
 

lib/js/src/thrift.js

@@ -1624,3 +1624,16 @@ copyMap = function(obj, types) {
 
 Thrift.copyMap = copyMap;
 Thrift.copyList = copyList;
+
+Thrift.checkSetUniqueness = function(arr) {
+  var seen = new Set();
+  for (var i = 0; i < arr.length; i++) {
+    var val = arr[i];
+    if (seen.has(val)) {
+      throw new Thrift.TProtocolException(
+        Thrift.TProtocolExceptionType.INVALID_DATA,
+        "encoding set: duplicate element");
+    }
+    seen.add(val);
+  }
+};

lib/nodejs/lib/thrift/thrift.js

@@ -238,3 +238,16 @@ copyMap = function (obj, types) {
 
 module.exports.copyMap = copyMap;
 module.exports.copyList = copyList;
+
+function checkSetUniqueness(arr) {
+  var seen = new Set();
+  for (var i = 0; i < arr.length; i++) {
+    var val = arr[i];
+    if (seen.has(val)) {
+      throw new TProtocolException(TProtocolExceptionType.INVALID_DATA,
+        "encoding set: duplicate element");
+    }
+    seen.add(val);
+  }
+}
+module.exports.checkSetUniqueness = checkSetUniqueness;

lib/nodejs/test/check_set_uniqueness.test.js

@@ -0,0 +1,96 @@
+/*
+ * 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.
+ */
+
+const test = require("tape");
+const Thrift = require("thrift/lib/nodejs/lib/thrift/thrift");
+
+const cases = {
+  "Should accept a unique numeric set": function (assert) {
+    Thrift.checkSetUniqueness([10, 43, 38, -2]);
+    assert.pass("no exception thrown");
+    assert.end();
+  },
+  "Should accept an empty set": function (assert) {
+    Thrift.checkSetUniqueness([]);
+    assert.pass("no exception thrown");
+    assert.end();
+  },
+  "Should accept a single-element set": function (assert) {
+    Thrift.checkSetUniqueness([42]);
+    assert.pass("no exception thrown");
+    assert.end();
+  },
+  "Should accept a unique string set": function (assert) {
+    Thrift.checkSetUniqueness(["a", "b", "c"]);
+    assert.pass("no exception thrown");
+    assert.end();
+  },
+  "Should accept a unique boolean set": function (assert) {
+    Thrift.checkSetUniqueness([true, false]);
+    assert.pass("no exception thrown");
+    assert.end();
+  },
+  "Should reject duplicate i8 elements": function (assert) {
+    assert.throws(function () {
+      Thrift.checkSetUniqueness([10, 43, 38, 38, -2]);
+    }, /duplicate element/);
+    assert.end();
+  },
+  "Should reject duplicate i32 elements": function (assert) {
+    assert.throws(function () {
+      Thrift.checkSetUniqueness([1, 2, 2, 3]);
+    }, /duplicate element/);
+    assert.end();
+  },
+  "Should reject all-same elements": function (assert) {
+    assert.throws(function () {
+      Thrift.checkSetUniqueness([1, 1, 1]);
+    }, /duplicate element/);
+    assert.end();
+  },
+  "Should reject duplicate string elements": function (assert) {
+    assert.throws(function () {
+      Thrift.checkSetUniqueness(["a", "b", "a"]);
+    }, /duplicate element/);
+    assert.end();
+  },
+  "Should reject duplicate boolean elements": function (assert) {
+    assert.throws(function () {
+      Thrift.checkSetUniqueness([true, true]);
+    }, /duplicate element/);
+    assert.end();
+  },
+  "Should throw TProtocolException with INVALID_DATA type": function (assert) {
+    try {
+      Thrift.checkSetUniqueness([1, 1]);
+      assert.fail("expected exception was not thrown");
+    } catch (e) {
+      assert.equal(
+        e.type,
+        Thrift.TProtocolExceptionType.INVALID_DATA,
+        "exception type is INVALID_DATA",
+      );
+    }
+    assert.end();
+  },
+};
+
+Object.keys(cases).forEach(function (caseName) {
+  test(caseName, cases[caseName]);
+});

lib/nodejs/test/testAll.sh

@@ -130,6 +130,7 @@ fi
 # unit tests
 
 node ${DIR}/binary.test.js || TESTOK=1
+node ${DIR}/check_set_uniqueness.test.js || TESTOK=1
 node ${DIR}/header.test.js || TESTOK=1
 node ${DIR}/int64.test.js || TESTOK=1
 node ${DIR}/deep-constructor.test.js || TESTOK=1