Report JSON that is not an Ink story instead of aborting

inkcpp_compiler/json_compiler.cpp

@@ -30,8 +30,25 @@ void json_compiler::compile(
     const nlohmann::json& input, emitter* output, compilation_results* results
 )
 {
+	/* Look the top-level keys up rather than reaching for them with operator[].
+	   On a const json, operator[] with a missing key is an assert(), which
+	   aborts the process instead of reporting an error, and any JSON at all can
+	   be handed to a compiler that accepts "a .json file". Every other failure
+	   here is an ink_exception the caller can catch. */
+	if (! input.is_object()) {
+		throw ink_exception("this JSON is not an Ink story: its top level is not an object");
+	}
+	const auto version_itr = input.find("inkVersion");
+	if (version_itr == input.end()) {
+		throw ink_exception("this JSON is not an Ink story: it has no \"inkVersion\"");
+	}
+	const auto root_itr = input.find("root");
+	if (root_itr == input.end()) {
+		throw ink_exception("this JSON is not an Ink story: it has no \"root\"");
+	}
+
 	// Get the runtime version
-	_ink_version = input["inkVersion"];
+	_ink_version = *version_itr;
 
 	// Start the output
 	set_results(results);
@@ -45,7 +62,7 @@ void json_compiler::compile(
 		_emitter->set_list_meta(_list_meta);
 	}
 	// Compile the root container
-	compile_container(input["root"], 0, 0);
+	compile_container(*root_itr, 0, 0);
 
 	// finalize
 	_emitter->finish(_next_container_index);
@@ -133,6 +150,14 @@ void json_compiler::compile_container(
     const std::string& name_override
 )
 {
+	/* Every container is an array whose last element is its metadata object, so
+	   rbegin() and the end() - 1 below are only meaningful for a non-empty
+	   array. Reaching them with anything else dereferences an end iterator.
+	   This guard covers the root and every nested container in one place. */
+	if (! container.is_array() || container.empty()) {
+		throw ink_exception("malformed Ink story: a container must be a non-empty array");
+	}
+
 	// Grab metadata from the last object in this container
 	container_meta meta;
 	bool           is_knot = name_override != "" && index_in_parent == -1;

inkcpp_test/CMakeLists.txt

@@ -33,6 +33,7 @@ add_executable(
 	Fixes.cpp
 	Migration.cpp
 	MultiRunner.cpp
+	NotAnInkStory.cpp
 )
 
 target_link_libraries(inkcpp_test PUBLIC inkcpp inkcpp_compiler inkcpp_shared)

inkcpp_test/NotAnInkStory.cpp

@@ -0,0 +1,88 @@
+#include "catch.hpp"
+
+#include <compiler.h>
+#include <system.h>
+
+#include <sstream>
+#include <string>
+
+/* Anything at all can be handed to a compiler that accepts "a .json file": a
+ * settings file, an API response, a package.json. Before the guards in
+ * json_compiler::compile() and compile_container(), reaching for the top-level
+ * keys with operator[] made that fatal rather than reportable - on a const
+ * nlohmann::json a missing key is an assert(), so the process aborted with
+ * assertions on and dereferenced an end iterator with them off.
+ *
+ * These cases are the ones that were measured to abort, segfault, or silently
+ * emit a bogus binary. They are all cheap to state, and the failure they guard
+ * against is the kind that only shows up in someone else's crash report. */
+
+using namespace ink::compiler;
+
+namespace
+{
+	std::string compile_or_throw(const char* json)
+	{
+		std::istringstream in(json);
+		std::ostringstream out;
+		run(in, out);
+		return out.str();
+	}
+} // namespace
+
+SCENARIO("JSON that is not an Ink story is reported, not fatal", "[compiler]")
+{
+	GIVEN("a document whose top level is not an object")
+	{
+		auto source = GENERATE("[1, 2, 3]", "\"just a string\"", "42", "null");
+		THEN("compiling it raises rather than aborting")
+		{
+			REQUIRE_THROWS_AS(compile_or_throw(source), ink::ink_exception);
+		}
+	}
+
+	GIVEN("an object without the keys a story must have")
+	{
+		// Previously an assert() on the missing key, so an abort() with
+		// assertions enabled.
+		auto source = GENERATE(
+		    "{}",                                        // neither key
+		    "{\"inkVersion\": 21}",                      // no root
+		    "{\"root\": [[\"done\"], null]}",            // no inkVersion
+		    "{\"name\": \"not a story\", \"value\": 42}" // something else entirely
+		);
+		THEN("compiling it raises rather than aborting")
+		{
+			REQUIRE_THROWS_AS(compile_or_throw(source), ink::ink_exception);
+		}
+	}
+
+	GIVEN("a root that is not a non-empty array")
+	{
+		// compile_container() takes rbegin() and end() - 1, which are only
+		// meaningful for a non-empty array. An empty array or an object used to
+		// dereference an end iterator; a scalar root silently produced a small
+		// binary that was not a story.
+		auto source = GENERATE(
+		    "{\"inkVersion\": 21, \"root\": []}", "{\"inkVersion\": 21, \"root\": {}}",
+		    "{\"inkVersion\": 21, \"root\": \"nope\"}", "{\"inkVersion\": 21, \"root\": 42}"
+		);
+		THEN("compiling it raises rather than aborting")
+		{
+			REQUIRE_THROWS_AS(compile_or_throw(source), ink::ink_exception);
+		}
+	}
+
+	GIVEN("a minimal but valid story")
+	{
+		THEN("it still compiles, so the guards have not become too strict")
+		{
+			std::string binary;
+			REQUIRE_NOTHROW(
+			    binary = compile_or_throw("{\"inkVersion\": 21, \"root\": [[\"done\"], null], "
+			                              "\"listDefs\": {}}")
+			);
+			REQUIRE(binary.size() > 0);
+		}
+	}
+}