diff --git a/builtin/common/misc_helpers.lua b/builtin/common/misc_helpers.lua index 47e0aeabc..04ea9c486 100644 --- a/builtin/common/misc_helpers.lua +++ b/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 diff --git a/builtin/common/serialize.lua b/builtin/common/serialize.lua index 146128e0c..8f463d978 100644 --- a/builtin/common/serialize.lua +++ b/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 diff --git a/builtin/common/tests/serialize_spec.lua b/builtin/common/tests/serialize_spec.lua index d4e501468..2a7a0f3ce 100644 --- a/builtin/common/tests/serialize_spec.lua +++ b/builtin/common/tests/serialize_spec.lua @@ -93,21 +93,49 @@ describe("serialize", function() assert_preserves(test_in) end) - it("strips functions in safe mode", function() - local test_in = { - func = function(a, b) - error("test") - end, - foo = "bar" - } - setfenv(test_in.func, _G) + 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") + end, + foo = "bar" + } + setfenv(test_in.func, _G) - local str = core.serialize(test_in) - assert.not_nil(str:find("loadstring")) + local str = core.serialize(test_in) + assert.not_nil(str:find("loadstring")) - local test_out = core.deserialize(str, true) - assert.is_nil(test_out.func) - assert.equals(test_out.foo, "bar") + local test_out = core.deserialize(str, true) + 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() diff --git a/doc/breakages.md b/doc/breakages.md index 51387e279..412cf2e41 100644 --- a/doc/breakages.md +++ b/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`. diff --git a/doc/lua_api.md b/doc/lua_api.md index dd1670d34..ebc5826c2 100644 --- a/doc/lua_api.md +++ b/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 - cannot directly access the global environment, they could bypass this - restriction with maliciously crafted Lua bytecode if mod security is - disabled. + * Will load functions if `safe` is `false` or omitted. + Although these functions cannot directly access the global environment, + they could bypass this restriction with maliciously crafted Lua bytecode + 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.