fix: validate edit integers before narrowing

CHANGELOG.md

@@ -16,6 +16,10 @@ The release run heads these entries with the version and opens a fresh
 
 ## Unreleased
 
+- JSON edits reject fractional and out-of-range indices before changing a
+  document, and preserve the full width of positive element identifiers.
+  Text and document edits validate the version and operations array.
+
 - Empty file and HTML handles throw instead of dereferencing null.
   Missing master pages and table rows/columns return empty element handles.
 

src/odr/document.cpp

@@ -105,6 +105,21 @@ std::optional<std::string> Document::locale() const { return m_impl->locale(); }
 
 namespace {
 
+template <typename T> T integer(const nlohmann::json &value) {
+  if (value.is_number_unsigned()) {
+    const auto number = value.get<std::uint64_t>();
+    if (std::in_range<T>(number)) {
+      return static_cast<T>(number);
+    }
+  } else if (value.is_number_integer()) {
+    const auto number = value.get<std::int64_t>();
+    if (std::in_range<T>(number)) {
+      return static_cast<T>(number);
+    }
+  }
+  throw std::invalid_argument("edit integer is out of range");
+}
+
 /// `{"type": "number", "number": …, "text": …}`, `"date"` and `"time"` the
 /// same with days since 1899-12-30, or `"string"` with the text alone, or
 /// `"empty"` for a cell stating nothing.
@@ -266,33 +281,36 @@ Sheet sheet_at(const Element root, const std::uint32_t ordinal) {
 void Document::edit(const std::string_view operations,
                     const Logger & /*logger*/) const {
   const nlohmann::json json = nlohmann::json::parse(operations);
-  if (json.value("version", 0) != 2) {
+  if (json.value("version", nlohmann::json{}) != 2) {
     throw std::invalid_argument("unsupported edit version");
   }
 
+  if (!json.at("ops").is_array()) {
+    throw std::invalid_argument("edit operations must be an array");
+  }
+
   // an operation that creates an element states a negative id for it, and a
   // later one names it by that number (`docs/design/document-editing.md`)
   std::unordered_map<std::int64_t, ElementIdentifier> minted;
 
   // the element @p field names, checked to be one this document holds
   const auto element_of = [&](const nlohmann::json &operation,
                               const char *field) {
-    const auto address = operation.at(field).get<std::int64_t>();
+    const auto &address = operation.at(field);
     ElementIdentifier identifier{};
-    if (address < 0) {
-      const auto entry = minted.find(address);
+    if (address.is_number_integer() && address < 0) {
+      const auto entry = minted.find(integer<std::int64_t>(address));
       if (entry == std::end(minted)) {
-        throw std::invalid_argument("element " + std::to_string(address) +
+        throw std::invalid_argument("element " + address.dump() +
                                     " has not been created");
       }
       identifier = entry->second;
     } else {
-      identifier = static_cast<ElementIdentifier>(address);
+      identifier = integer<ElementIdentifier>(address);
     }
     const Element element = element_by_id(identifier);
     if (!element) {
-      throw std::invalid_argument("element " + std::to_string(address) +
-                                  " not found");
+      throw std::invalid_argument("element " + address.dump() + " not found");
     }
     return element;
   };
@@ -325,7 +343,7 @@ void Document::edit(const std::string_view operations,
   // the negative id an operation reserves, checked before anything is created
   // so that a refusal changes nothing
   const auto reserve = [&](const nlohmann::json &operation) {
-    const auto address = operation.at("id").get<std::int64_t>();
+    const auto address = integer<std::int64_t>(operation.at("id"));
     if (address >= 0) {
       throw std::invalid_argument("a created element needs a negative id");
     }
@@ -340,56 +358,56 @@ void Document::edit(const std::string_view operations,
     const auto name = operation.at("op").get<std::string>();
 
     if (name == "setCell") {
-      sheet_at(root_element(), operation.at("sheet").get<std::uint32_t>())
-          .set_cell(operation.at("column").get<std::uint32_t>(),
-                    operation.at("row").get<std::uint32_t>(),
+      sheet_at(root_element(), integer<std::uint32_t>(operation.at("sheet")))
+          .set_cell(integer<std::uint32_t>(operation.at("column")),
+                    integer<std::uint32_t>(operation.at("row")),
                     parse_cell_value(operation.at("value")));
       continue;
     }
 
     if (name == "setCellStyle") {
       const auto [cell_style, text_style] =
           parse_cell_style(operation.at("style"));
-      sheet_at(root_element(), operation.at("sheet").get<std::uint32_t>())
-          .set_cell_style(operation.at("column").get<std::uint32_t>(),
-                          operation.at("row").get<std::uint32_t>(), cell_style,
-                          text_style);
+      sheet_at(root_element(), integer<std::uint32_t>(operation.at("sheet")))
+          .set_cell_style(integer<std::uint32_t>(operation.at("column")),
+                          integer<std::uint32_t>(operation.at("row")),
+                          cell_style, text_style);
       continue;
     }
 
     if (name == "setRowStyle") {
       const auto [cell_style, text_style] =
           parse_cell_style(operation.at("style"));
-      sheet_at(root_element(), operation.at("sheet").get<std::uint32_t>())
-          .set_row_style(operation.at("row").get<std::uint32_t>(), cell_style,
-                         text_style);
+      sheet_at(root_element(), integer<std::uint32_t>(operation.at("sheet")))
+          .set_row_style(integer<std::uint32_t>(operation.at("row")),
+                         cell_style, text_style);
       continue;
     }
 
     if (name == "setColumnStyle") {
       const auto [cell_style, text_style] =
           parse_cell_style(operation.at("style"));
-      sheet_at(root_element(), operation.at("sheet").get<std::uint32_t>())
-          .set_column_style(operation.at("column").get<std::uint32_t>(),
+      sheet_at(root_element(), integer<std::uint32_t>(operation.at("sheet")))
+          .set_column_style(integer<std::uint32_t>(operation.at("column")),
                             cell_style, text_style);
       continue;
     }
 
     if (name == "insertRows" || name == "deleteRows") {
-      const Sheet sheet =
-          sheet_at(root_element(), operation.at("sheet").get<std::uint32_t>());
-      const auto row = operation.at("row").get<std::uint32_t>();
-      const auto count = operation.at("count").get<std::uint32_t>();
+      const Sheet sheet = sheet_at(
+          root_element(), integer<std::uint32_t>(operation.at("sheet")));
+      const auto row = integer<std::uint32_t>(operation.at("row"));
+      const auto count = integer<std::uint32_t>(operation.at("count"));
       name == "insertRows" ? sheet.insert_rows(row, count)
                            : sheet.delete_rows(row, count);
       continue;
     }
 
     if (name == "insertColumns" || name == "deleteColumns") {
-      const Sheet sheet =
-          sheet_at(root_element(), operation.at("sheet").get<std::uint32_t>());
-      const auto column = operation.at("column").get<std::uint32_t>();
-      const auto count = operation.at("count").get<std::uint32_t>();
+      const Sheet sheet = sheet_at(
+          root_element(), integer<std::uint32_t>(operation.at("sheet")));
+      const auto column = integer<std::uint32_t>(operation.at("column"));
+      const auto count = integer<std::uint32_t>(operation.at("count"));
       name == "insertColumns" ? sheet.insert_columns(column, count)
                               : sheet.delete_columns(column, count);
       continue;

src/odr/file.cpp

@@ -308,10 +308,14 @@ namespace {
 /// The text @p operations set, or none where they state no operation.
 std::optional<std::string> content_of(const std::string_view operations) {
   const nlohmann::json json = nlohmann::json::parse(operations);
-  if (json.value("version", 0) != 2) {
+  if (json.value("version", nlohmann::json{}) != 2) {
     throw std::invalid_argument("unsupported edit version");
   }
 
+  if (!json.at("ops").is_array()) {
+    throw std::invalid_argument("edit operations must be an array");
+  }
+
   std::optional<std::string> content;
   for (const nlohmann::json &operation : json.at("ops")) {
     const auto name = operation.at("op").get<std::string>();

test/src/document_edit_test.cpp

@@ -16,6 +16,7 @@
 #include <odr/internal/zip/zip_file.hpp>
 
 #include <gtest/gtest.h>
+#include <nlohmann/json.hpp>
 
 #include <cstdint>
 #include <memory>
@@ -149,6 +150,33 @@ TEST(DocumentEdit, an_unknown_version_refuses) {
   EXPECT_THROW(document.edit(R"({"ops":[]})"), std::invalid_argument);
 }
 
+TEST(DocumentEdit, invalid_integer_arguments_do_not_target_another_cell) {
+  const Document document = three_cell_sheet();
+  nlohmann::json operation{{"op", "setCell"},
+                           {"sheet", 0},
+                           {"column", 0},
+                           {"row", 0},
+                           {"value", {{"type", "string"}, {"text", "wrong"}}}};
+  for (const char *field : {"sheet", "column", "row"}) {
+    for (const auto &value :
+         nlohmann::json::array({-4294967296LL, 4294967296ULL, 0.5, true})) {
+      operation[field] = value;
+      EXPECT_THROW(
+          document.edit(
+              nlohmann::json{{"version", 2}, {"ops", {operation}}}.dump()),
+          std::invalid_argument);
+      EXPECT_EQ(first_sheet(document).cell(0, 0).value().text(), "a");
+    }
+    operation[field] = 0;
+  }
+  EXPECT_THROW(document.edit(R"({"version":4294967298,"ops":[]})"),
+               std::invalid_argument);
+  EXPECT_THROW(document.edit(R"({"version":2.5,"ops":[]})"),
+               std::invalid_argument);
+  EXPECT_THROW(document.edit(R"({"version":2,"ops":null})"),
+               std::invalid_argument);
+}
+
 TEST(DocumentEdit, an_unknown_op_refuses) {
   const Document document = three_cell_sheet();
 
@@ -329,6 +357,17 @@ TEST(DocumentEdit, an_op_naming_a_created_element_that_is_not_there_refuses) {
                std::invalid_argument);
 }
 
+TEST(DocumentEdit, an_unsigned_id_does_not_alias_a_created_element) {
+  const Document document = two_paragraph_text();
+  EXPECT_THROW(
+      document.edit(
+          ops(R"({"op":"insertText","after":)" + id_of(run_at(document, 0, 2)) +
+              R"(,"text":"four","id":-1},)" +
+              R"({"op":"setText","id":18446744073709551615,"text":"wrong"})")),
+      std::invalid_argument);
+  EXPECT_EQ(text_of(paragraph_at(document, 0)), "one two threefour");
+}
+
 TEST(DocumentEdit, creating_two_elements_under_one_id_refuses) {
   const Document document = two_paragraph_text();
   const std::string anchor = id_of(run_at(document, 0, 0));

test/src/internal/text/text_file_test.cpp

@@ -150,6 +150,12 @@ TEST(TextFile, an_unknown_version_or_op_refuses) {
 
   EXPECT_THROW((void)edited(file, R"({"version":1,"ops":[]})"),
                std::invalid_argument);
+  EXPECT_THROW((void)edited(file, R"({"version":4294967298,"ops":[]})"),
+               std::invalid_argument);
+  EXPECT_THROW((void)edited(file, R"({"version":2.5,"ops":[]})"),
+               std::invalid_argument);
+  EXPECT_THROW((void)edited(file, R"({"version":2,"ops":null})"),
+               std::invalid_argument);
   EXPECT_THROW((void)edited(file, R"({"version":2,"ops":[{"op":"setText"}]})"),
                std::invalid_argument);
 }