Skip unknown fields while decoding FieldValue proto objects. (#1354)

4 files changed, 95 insertions, 27 deletions

diff --git a/Firestore/core/src/firebase/firestore/nanopb/reader.cc b/Firestore/core/src/firebase/firestore/nanopb/reader.cc
index ec4282d..69e3d83 100644
--- a/Firestore/core/src/firebase/firestore/nanopb/reader.cc
+++ b/Firestore/core/src/firebase/firestore/nanopb/reader.cc
@@ -136,6 +136,14 @@ std::string Reader::ReadString() {
   return result;
 }
 
+void Reader::SkipField(const Tag& tag) {
+  if (!status_.ok()) return;
+
+  if (!pb_skip_field(&stream_, tag.wire_type)) {
+    status_ = Status(FirestoreErrorCode::DataLoss, PB_GET_ERROR(&stream_));
+  }
+}
+
 }  // namespace nanopb
 }  // namespace firestore
 }  // namespace firebase
diff --git a/Firestore/core/src/firebase/firestore/nanopb/reader.h b/Firestore/core/src/firebase/firestore/nanopb/reader.h
index 930211a..2c16ec4 100644
--- a/Firestore/core/src/firebase/firestore/nanopb/reader.h
+++ b/Firestore/core/src/firebase/firestore/nanopb/reader.h
@@ -89,6 +89,15 @@ class Reader {
   template <typename T>
   T ReadNestedMessage(const std::function<T(Reader*)>& read_message_fn);
 
+  /**
+   * Discards the bytes associated with the given tag.
+   *
+   * @param tag The tag associated with the field that is otherwise about to be
+   * read. This method uses the tag to determine how many bytes should be
+   * discarded.
+   */
+  void SkipField(const Tag& tag);
+
   size_t bytes_left() const {
     return stream_.bytes_left;
   }
diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc
index 0839efc..eee56f8 100644
--- a/Firestore/core/src/firebase/firestore/remote/serializer.cc
+++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc
@@ -204,20 +204,20 @@ FieldValue DecodeFieldValueImpl(Reader* reader) {
         }
         break;
 
-      default:
-        // We could get here for one of two reasons; either because the input
-        // bytes are corrupt, or because we're attempting to parse a tag that we
-        // haven't implemented yet. Long term, the latter reason should become
-        // less likely (especially in production), so we'll assume former.
-
-        // TODO(rsgowman): While still in development, we'll contradict the
-        // above and assume the latter. Remove the following assertion when
-        // we're confident that we're handling all the tags in the protos.
+      case google_firestore_v1beta1_Value_double_value_tag:
+      case google_firestore_v1beta1_Value_bytes_value_tag:
+      case google_firestore_v1beta1_Value_reference_value_tag:
+      case google_firestore_v1beta1_Value_geo_point_value_tag:
+      case google_firestore_v1beta1_Value_array_value_tag:
+        // TODO(b/74243929): Implement remaining types.
         HARD_FAIL("Unhandled message field number (tag): %i.",
                   tag.field_number);
-        reader->set_status(Status(
-            FirestoreErrorCode::DataLoss,
-            "Input proto bytes cannot be parsed (invalid field number (tag))"));
+
+      default:
+        // Unknown tag. According to the proto spec, we need to ignore these. No
+        // action required here, though we'll need to skip the relevant bytes
+        // below.
+        break;
     }
 
     if (!reader->status().ok()) return FieldValue::NullValue();
@@ -247,12 +247,18 @@ FieldValue DecodeFieldValueImpl(Reader* reader) {
             reader->ReadNestedMessage<ObjectValue::Map>(DecodeMapValue));
         break;
 
+      case google_firestore_v1beta1_Value_double_value_tag:
+      case google_firestore_v1beta1_Value_bytes_value_tag:
+      case google_firestore_v1beta1_Value_reference_value_tag:
+      case google_firestore_v1beta1_Value_geo_point_value_tag:
+      case google_firestore_v1beta1_Value_array_value_tag:
+        // TODO(b/74243929): Implement remaining types.
+        HARD_FAIL("Unhandled message field number (tag): %i.",
+                  tag.field_number);
+
       default:
-        // This indicates an internal error as we've already ensured that this
-        // is a valid field_number.
-        HARD_FAIL(
-            "Somehow got an unexpected field number (tag) after verifying that "
-            "the field number was expected.");
+        // Unknown tag. According to the proto spec, we need to ignore these.
+        reader->SkipField(tag);
     }
   }
 
diff --git a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc
index 96ffa9e..7dcdf6e 100644
--- a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc
+++ b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc
@@ -605,7 +605,12 @@ TEST_F(SerializerTest, BadTimestampValue_TooSmall) {
       Status(FirestoreErrorCode::DataLoss, "ignored"), bytes);
 }
 
-TEST_F(SerializerTest, BadTag) {
+TEST_F(SerializerTest, BadFieldValueTagAndNoOtherTagPresent) {
+  // A bad tag should be ignored. But if there are *no* valid tags, then we
+  // don't know the type of the FieldValue. Although it might be reasonable to
+  // assume some sort of default type in this situation, we've decided to fail
+  // the deserialization process in this case instead.
+
   std::vector<uint8_t> bytes =
       EncodeFieldValue(&serializer, FieldValue::NullValue());
 
@@ -615,15 +620,55 @@ TEST_F(SerializerTest, BadTag) {
   // Specifically 31. 0xf8 represents field number 31 encoded as a varint.
   Mutate(&bytes[0], /*expected_initial_value=*/0x58, /*new_value=*/0xf8);
 
-  // TODO(rsgowman): The behaviour is *temporarily* slightly different during
-  // development; this will cause a failed assertion rather than a failed
-  // status. Remove this EXPECT_ANY_THROW statement (and reenable the
-  // following commented out statement) once the corresponding assert has been
-  // removed from serializer.cc.
-  EXPECT_ANY_THROW(ExpectFailedStatusDuringFieldValueDecode(
-      Status(FirestoreErrorCode::DataLoss, "ignored"), bytes));
-  // ExpectFailedStatusDuringFieldValueDecode(
-  //    Status(FirestoreErrorCode::DataLoss, "ignored"), bytes);
+  ExpectFailedStatusDuringFieldValueDecode(
+      Status(FirestoreErrorCode::DataLoss, "ignored"), bytes);
+}
+
+TEST_F(SerializerTest, BadFieldValueTagWithOtherValidTagsPresent) {
+  // A bad tag should be ignored, in which case, we should successfully
+  // deserialize the rest of the bytes as if it wasn't there. To craft these
+  // bytes, we'll use the same technique as
+  // EncodesFieldValuesWithRepeatedEntries (so go read the comments there
+  // first).
+
+  // Copy of the real one (from the nanopb generated document.pb.h), but with
+  // only boolean_value and integer_value.
+  struct google_firestore_v1beta1_Value_Fake {
+    bool boolean_value;
+    int64_t integer_value;
+  };
+
+  // Copy of the real one (from the nanopb generated document.pb.c), but with
+  // only boolean_value and integer_value. Also modified such that integer_value
+  // now has an invalid tag (instead of 2).
+  const int invalid_tag = 31;
+  const pb_field_t google_firestore_v1beta1_Value_fields_Fake[2] = {
+      PB_FIELD(1, BOOL, SINGULAR, STATIC, FIRST,
+               google_firestore_v1beta1_Value_Fake, boolean_value,
+               boolean_value, 0),
+      PB_FIELD(invalid_tag, INT64, SINGULAR, STATIC, OTHER,
+               google_firestore_v1beta1_Value_Fake, integer_value,
+               boolean_value, 0),
+  };
+
+  // Craft the bytes. boolean_value has a smaller tag, so it'll get encoded
+  // first, normally implying integer_value should "win". Except that
+  // integer_value isn't a valid tag, so it should be ignored here.
+  google_firestore_v1beta1_Value_Fake crafty_value{false, int64_t{42}};
+  std::vector<uint8_t> bytes(128);
+  pb_ostream_t stream = pb_ostream_from_buffer(bytes.data(), bytes.size());
+  pb_encode(&stream, google_firestore_v1beta1_Value_fields_Fake, &crafty_value);
+  bytes.resize(stream.bytes_written);
+
+  // Decode the bytes into the model
+  StatusOr<FieldValue> actual_model_status = serializer.DecodeFieldValue(bytes);
+  EXPECT_OK(actual_model_status);
+  FieldValue actual_model = actual_model_status.ValueOrDie();
+
+  // Ensure the decoded model is as expected.
+  FieldValue expected_model = FieldValue::BooleanValue(false);
+  EXPECT_EQ(FieldValue::Type::Boolean, actual_model.type());
+  EXPECT_EQ(expected_model, actual_model);
 }
 
 TEST_F(SerializerTest, TagVarintWiretypeStringMismatch) {