diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e062c565..c00e5322d 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/src/odr/document.cpp b/src/odr/document.cpp index 33003293c..96aebe43a 100644 --- a/src/odr/document.cpp +++ b/src/odr/document.cpp @@ -105,6 +105,21 @@ std::optional Document::locale() const { return m_impl->locale(); } namespace { +template T integer(const nlohmann::json &value) { + if (value.is_number_unsigned()) { + const auto number = value.get(); + if (std::in_range(number)) { + return static_cast(number); + } + } else if (value.is_number_integer()) { + const auto number = value.get(); + if (std::in_range(number)) { + return static_cast(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,10 +281,14 @@ 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 minted; @@ -277,22 +296,21 @@ void Document::edit(const std::string_view operations, // 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(); + 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(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(address); + identifier = integer(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(); + const auto address = integer(operation.at("id")); if (address >= 0) { throw std::invalid_argument("a created element needs a negative id"); } @@ -340,9 +358,9 @@ void Document::edit(const std::string_view operations, const auto name = operation.at("op").get(); if (name == "setCell") { - sheet_at(root_element(), operation.at("sheet").get()) - .set_cell(operation.at("column").get(), - operation.at("row").get(), + sheet_at(root_element(), integer(operation.at("sheet"))) + .set_cell(integer(operation.at("column")), + integer(operation.at("row")), parse_cell_value(operation.at("value"))); continue; } @@ -350,46 +368,46 @@ void Document::edit(const std::string_view operations, if (name == "setCellStyle") { const auto [cell_style, text_style] = parse_cell_style(operation.at("style")); - sheet_at(root_element(), operation.at("sheet").get()) - .set_cell_style(operation.at("column").get(), - operation.at("row").get(), cell_style, - text_style); + sheet_at(root_element(), integer(operation.at("sheet"))) + .set_cell_style(integer(operation.at("column")), + integer(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()) - .set_row_style(operation.at("row").get(), cell_style, - text_style); + sheet_at(root_element(), integer(operation.at("sheet"))) + .set_row_style(integer(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()) - .set_column_style(operation.at("column").get(), + sheet_at(root_element(), integer(operation.at("sheet"))) + .set_column_style(integer(operation.at("column")), cell_style, text_style); continue; } if (name == "insertRows" || name == "deleteRows") { - const Sheet sheet = - sheet_at(root_element(), operation.at("sheet").get()); - const auto row = operation.at("row").get(); - const auto count = operation.at("count").get(); + const Sheet sheet = sheet_at( + root_element(), integer(operation.at("sheet"))); + const auto row = integer(operation.at("row")); + const auto count = integer(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()); - const auto column = operation.at("column").get(); - const auto count = operation.at("count").get(); + const Sheet sheet = sheet_at( + root_element(), integer(operation.at("sheet"))); + const auto column = integer(operation.at("column")); + const auto count = integer(operation.at("count")); name == "insertColumns" ? sheet.insert_columns(column, count) : sheet.delete_columns(column, count); continue; diff --git a/src/odr/file.cpp b/src/odr/file.cpp index 8ce90e278..6d0d8acba 100644 --- a/src/odr/file.cpp +++ b/src/odr/file.cpp @@ -308,10 +308,14 @@ namespace { /// The text @p operations set, or none where they state no operation. std::optional 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 content; for (const nlohmann::json &operation : json.at("ops")) { const auto name = operation.at("op").get(); diff --git a/test/src/document_edit_test.cpp b/test/src/document_edit_test.cpp index cdf552dde..04baf3d52 100644 --- a/test/src/document_edit_test.cpp +++ b/test/src/document_edit_test.cpp @@ -16,6 +16,7 @@ #include #include +#include #include #include @@ -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)); diff --git a/test/src/internal/text/text_file_test.cpp b/test/src/internal/text/text_file_test.cpp index a6b91311e..e08cfbbd0 100644 --- a/test/src/internal/text/text_file_test.cpp +++ b/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); }