Deprecate function support in core.[de]serialize

builtin/common/misc_helpers.lua

@@ -14,11 +14,6 @@ local function basic_dump(o)
 		return tostring(o)
 	elseif tp == "nil" then
 		return "nil"
-	-- Uncomment for full function dumping support.
-	-- Not currently enabled because bytecode isn't very human-readable and
-	-- dump's output is intended for humans.
-	--elseif tp == "function" then
-	--	return string.format("loadstring(%q)", string.dump(o))
 	elseif tp == "userdata" then
 		return tostring(o)
 	else

builtin/common/serialize.lua

@@ -190,7 +190,33 @@ local function serialize(value, write)
 	dump(value)
 end
 
+-- Whether `value` recursively contains a function
+local function contains_function(value)
+	local seen = {}
+	local function check(val)
+		if type(val) == "function" then
+			return true
+		end
+		if type(val) == "table" then
+			if seen[val] then
+				return false
+			end
+			seen[val] = true
+			for k, v in pairs(val) do
+				if check(k) or check(v) then
+					return true
+				end
+			end
+		end
+		return false
+	end
+	return check(value)
+end
+
 function core.serialize(value)
+	if contains_function(value) then
+		core.log("deprecated", "Support for dumping functions in `core.serialize` is deprecated.")
+	end
 	local rope = {}
 	serialize(value, function(text)
 		 -- Faster than table.insert(rope, text) on PUC Lua 5.1

builtin/common/tests/serialize_spec.lua

@@ -93,7 +93,16 @@ describe("serialize", function()
 		assert_preserves(test_in)
 	end)
 
-	it("strips functions in safe mode", function()
+	describe("safe mode", function()
+		setup(function()
+			assert(not core.log)
+			-- logging a deprecation warning will be attempted
+			function core.log() end
+		end)
+		teardown(function()
+			core.log = nil
+		end)
+		it("functions are stripped", function()
 			local test_in = {
 				func = function(a, b)
 					error("test")
@@ -109,6 +118,25 @@ describe("serialize", function()
 			assert.is_nil(test_out.func)
 			assert.equals(test_out.foo, "bar")
 		end)
+	end)
+
+	describe("deprecation warnings", function()
+		before_each(function()
+			assert(not core.log)
+			core.log = spy.new(function(level)
+				assert(level == "deprecated")
+			end)
+		end)
+		after_each(function()
+			core.log = nil
+		end)
+		it("dumping functions", function()
+			local t = {f = function() end, g = function() end}
+			t.t = t
+			core.serialize(t)
+			assert.spy(core.log).was.called(1) -- should have been called exactly *once*
+		end)
+	end)
 
 	it("vectors work", function()
 		local v = vector.new(1, 2, 3)

doc/breakages.md

@@ -23,3 +23,5 @@ This list is largely advisory and items may be reevaluated once the time comes.
 * stop reading initial properties from bare entity def
 * change particle default blend mode to `clip`
 * remove built-in knockback and related functions entirely
+* remove `safe` parameter from `core.serialize`, always enforce `safe = true`.
+  possibly error when `loadstring` calls are encountered in `core.deserialize`.

doc/lua_api.md

@@ -7611,14 +7611,19 @@ Misc.
 * `core.serialize(table)`: returns a string
     * Convert a table containing tables, strings, numbers, booleans and `nil`s
       into string form readable by `core.deserialize`
+    * Support for dumping function bytecode is **deprecated**.
     * Example: `serialize({foo="bar"})`, returns `'return { ["foo"] = "bar" }'`
 * `core.deserialize(string[, safe])`: returns a table
     * Convert a string returned by `core.serialize` into a table
     * `string` is loaded in an empty sandbox environment.
-    * Will load functions if safe is false or omitted. Although these functions
+    * Will load functions if `safe` is `false` or omitted.
-      cannot directly access the global environment, they could bypass this
+      Although these functions cannot directly access the global environment,
-      restriction with maliciously crafted Lua bytecode if mod security is
+      they could bypass this restriction with maliciously crafted Lua bytecode
-      disabled.
+      if mod security is disabled.
+    * Will silently strip functions embedded via calls to `loadstring`
+      (typically bytecode dumped by `core.serialize`) if `safe` is `true`.
+      You should not rely on this if possible.
+      * Example: `core.deserialize("return loadstring('')", true)` will be `nil`.
     * This function should not be used on untrusted data, regardless of the
      value of `safe`. It is fine to serialize then deserialize user-provided
      data, but directly providing user input to deserialize is always unsafe.