Add additional input validation

This makes it harder to crash mods that aren't expecting malicious
formspec input.

I don't like the large amount of unit tests but I don't see how to avoid
them and arguably those tests should have been there beforehand anyway.

init.lua

@@ -23,7 +23,7 @@ local S = minetest.get_translator("flow")
 
 local Form = {}
 
-local min, max = math.min, math.max
+local ceil, floor, min, max = math.ceil, math.floor, math.min, math.max
 
 -- Estimates the width of a valid UTF-8 string, ignoring any escape sequences.
 -- This function hopefully works with most (but not all) scripts, maybe it
@@ -606,34 +606,84 @@ local function chain_cb(f1, f2)
     end
 end
 
-local function safe_tonumber(str)
-    local num = tonumber(str)
-    if num and num == num and num >= 0 then
-        return num
+local function range_check_transformer(items_length)
+    return function(value)
+        local num = tonumber(value)
+        if num and num == num then
+            num = floor(num)
+            if num >= 1 and num <= items_length then
+                return num
+            end
+        end
     end
-    return 0
 end
 
+local function simple_transformer(func)
+    return function() return func end
+end
+
+-- Functions that transform field values into the easiest to use type
 local C1_CHARS = "\194[\128-\159]"
 local field_value_transformers = {
-    field = function(value)
+    field = simple_transformer(function(value)
         -- Remove control characters and newlines
         return value:gsub("[%z\1-\8\10-\31\127]", ""):gsub(C1_CHARS, "")
-    end,
-    tabheader = safe_tonumber,
-    dropdown = safe_tonumber,
-    checkbox = minetest.is_yes,
-    table = function(value)
-        return minetest.explode_table_event(value).row
-    end,
-    textlist = function(value)
-        return minetest.explode_textlist_event(value).index
-    end,
-    scrollbar = function(value)
+    end),
+    checkbox = simple_transformer(minetest.is_yes),
+
+    -- Scrollbars do have min/max values but scrollbars are only really used by
+    -- ScrollableVBox which doesn't need the extra checks
+    scrollbar = simple_transformer(function(value)
         return minetest.explode_scrollbar_event(value).value
-    end,
+    end),
 }
 
+-- Field value transformers that depend on some property of the element
+function field_value_transformers.tabheader(node)
+    return range_check_transformer(node.captions and #node.captions or 0)
+end
+
+function field_value_transformers.dropdown(node)
+    local items = node.items or {}
+    if node.index_event then
+        return range_check_transformer(#items)
+    end
+
+    -- Make sure that the value sent by the client is in the list of items
+    return function(value)
+        if table.indexof(items, value) > 0 then
+            return value
+        end
+    end
+end
+
+function field_value_transformers.table(node, tablecolumn_count)
+    -- Figure out how many rows the table has
+    local cells = node.cells and #node.cells or 0
+    local rows = ceil(cells / tablecolumn_count)
+
+    return function(value)
+        local row = floor(minetest.explode_table_event(value).row)
+        -- Tables and textlists can have values of 0 (nothing selected) but I
+        -- don't think the client can un-select a row so it should be safe to
+        -- ignore any 0 sent by the client to guarantee that the row will be
+        -- valid if the default value is valid
+        if row >= 1 and row <= rows then
+            return row
+        end
+    end
+end
+
+function field_value_transformers.textlist(node)
+    local rows = node.listelems and #node.listelems or 0
+    return function(value)
+        local index = floor(minetest.explode_textlist_event(value).index)
+        if index >= 1 and index <= rows then
+            return index
+        end
+    end
+end
+
 local function default_field_value_transformer(value)
     -- Remove control characters (but preserve newlines)
     -- Pattern by https://github.com/appgurueu
@@ -661,6 +711,7 @@ local function parse_callbacks(tree, ctx_form, auto_name_id,
         replace_backgrounds)
     local callbacks = {}
     local saved_fields = {}
+    local tablecolumn_count = 1
     for node in formspec_ast.walk(tree) do
         if node.type == "container" then
             if node.bgcolor then
@@ -689,6 +740,9 @@ local function parse_callbacks(tree, ctx_form, auto_name_id,
                 end
             end
             replace_backgrounds = replace_backgrounds or node._enable_bgimg_hack
+        elseif node.type == "tablecolumns" and node.tablecolumns then
+            -- Store the amount of columns for input validation
+            tablecolumn_count = max(#node.tablecolumns, 1)
         elseif replace_backgrounds then
             if (node.type == "background" or node.type == "background9") and
                     not node.auto_clip then
@@ -727,15 +781,6 @@ local function parse_callbacks(tree, ctx_form, auto_name_id,
                     end
 
                     node.selected_idx = node.selected_idx or 1
-
-                    -- Add a custom value transformer so that only values that
-                    -- were sent to the player are accepted
-                    saved_fields[node_name] = function(new_val)
-                        if table.indexof(items, new_val) > 0 then
-                            return new_val
-                        end
-                        return ctx_form[node_name]
-                    end
                 elseif value == nil then
                     -- If ctx.form[node_name] doesn't exist, then check whether
                     -- a default value is specified.
@@ -755,6 +800,11 @@ local function parse_callbacks(tree, ctx_form, auto_name_id,
                     -- Set the node's value to the one saved in ctx.form
                     node[value_field] = value
                 end
+
+                local get_transformer = field_value_transformers[node.type]
+                saved_fields[node_name] = get_transformer and
+                    get_transformer(node, tablecolumn_count) or
+                    default_field_value_transformer
             end
         end
 
@@ -1064,15 +1114,17 @@ function fs_process_events(player, form_info, fields)
                     " submitting a large field value (>60 kB), ignoring.")
             else
                 local new_value = transformer(raw_value)
-                if ctx_form[field] ~= new_value then
-                    if redraw_if_changed[field] then
-                        redraw_fs = true
-                    elseif form_info.formname == "" then
-                        -- Update the inventory when the player closes it next
-                        form_info.ctx_form_modified = true
+                if new_value ~= nil then
+                    if ctx_form[field] ~= new_value then
+                        if redraw_if_changed[field] then
+                            redraw_fs = true
+                        elseif form_info.formname == "" then
+                            -- Update the inventory when the player closes it
+                            form_info.ctx_form_modified = true
+                        end
                     end
+                    ctx_form[field] = new_value
                 end
-                ctx_form[field] = new_value
             end
         end
     end

test.lua

@@ -46,6 +46,10 @@ local function stub_player(name)
             return formspec
         end
     end
+    function minetest.get_player_by_name(name)
+        assert(name == "test_player")
+        return self
+    end
     return self
 end
 
@@ -69,6 +73,16 @@ string.split = string.split or function(str, chr)
     return r
 end
 
+function minetest.explode_textlist_event(event)
+    local event_type, number = event:match("^([A-Z]+):(%d+)$")
+    return {type = event_type, index = tonumber(number) or 0}
+end
+
+function minetest.explode_table_event(event)
+    local event_type, row, column = event:match("^([A-Z]+):(%d+):(%d+)$")
+    return {type = event_type, row = tonumber(row) or 0, column = tonumber(column) or 0}
+end
+
 -- Load flow
 local f = assert(io.open("init.lua"))
 local code = f:read("*a") .. "\nreturn naive_str_width"
@@ -102,6 +116,14 @@ local function test_render(build_func, output)
     assert.same(normalise_tree(expected_tree), normalise_tree(tree))
 end
 
+local function render_to_string(tree)
+    local player = stub_player("test_player")
+    local form = flow.make_gui(function() return table.copy(tree) end)
+    local ctx = {}
+    local _, event = form:render_to_formspec_string(player, ctx)
+    return ctx, event
+end
+
 describe("Flow", function()
     describe("bgcolor settings", function ()
         it("renders bgcolor only correctly", function ()
@@ -427,10 +449,6 @@ describe("Flow", function()
                 return gui.Button(buttonargs)
             end)
             local player = stub_player("test_player")
-            function minetest.get_player_by_name(name)
-                assert(name == "test_player")
-                return player
-            end
             local ctx = {a = 1}
             local _, trigger_event = form:render_to_formspec_string(player, ctx, true)
 
@@ -484,4 +502,206 @@ describe("Flow", function()
             assert.equals(h, 2)
         end)
     end)
+
+    describe("field validation for", function()
+        describe("Field", function()
+            it("passes correct input through", function()
+                local ctx, event = render_to_string(gui.Field{
+                    name = "a", default = "(default)"
+                })
+                assert.equals(ctx.form.a, "(default)")
+                event({a = "Hello world!"})
+                assert.equals(ctx.form.a, "Hello world!")
+            end)
+
+            it("strips escape characters", function()
+                local ctx, event = render_to_string(gui.Field{name = "a"})
+                assert.equals(ctx.form.a, "")
+                event({a = "\1\2Hello \3\4world!\n"})
+                assert.equals(ctx.form.a, "Hello world!")
+            end)
+
+            it("ignores other fields", function()
+                local ctx, event = render_to_string(gui.Field{name = "a"})
+                assert.equals(ctx.form.a, "")
+                event({b = "Hello world!"})
+                assert.equals(ctx.form.a, "")
+            end)
+        end)
+
+        describe("Textarea", function()
+            it("strips escape characters", function()
+                local ctx, event = render_to_string(gui.Textarea{name = "a"})
+                assert.equals(ctx.form.a, "")
+                event({a = "\1\2Hello \3\4world!\n"})
+                assert.equals(ctx.form.a, "Hello world!\n")
+            end)
+        end)
+
+        describe("Checkbox", function()
+            it("converts the result to a boolean", function()
+                local ctx, event = render_to_string(gui.Checkbox{name = "a"})
+                assert.equals(ctx.form.a, false)
+                event({a = "true"})
+                assert.equals(ctx.form.a, true)
+            end)
+        end)
+
+        describe("Dropdown", function()
+            describe("{index_event=false}", function()
+                it("passes correct input through", function()
+                    local ctx, event = render_to_string(gui.Dropdown{
+                        name = "a", items = {"hello", "world"},
+                    })
+                    assert.equals(ctx.form.a, "hello")
+                    event({a = "world"})
+                    assert.equals(ctx.form.a, "world")
+                end)
+
+                it("ignores malicious input", function()
+                    local ctx, event = render_to_string(gui.Dropdown{
+                        name = "a", items = {"hello", "world"},
+                    })
+                    assert.equals(ctx.form.a, "hello")
+                    event({a = "there"})
+                    assert.equals(ctx.form.a, "hello")
+                end)
+            end)
+
+            describe("{index_event=true}", function()
+                it("passes correct input through", function()
+                    local ctx, event = render_to_string(gui.Dropdown{
+                        name = "a", items = {"hello", "world"},
+                        index_event = true,
+                    })
+                    assert.equals(ctx.form.a, 1)
+                    event({a = "2"})
+                    assert.equals(ctx.form.a, 2)
+                end)
+
+                it("ignores malicious input", function()
+                    local ctx, event = render_to_string(gui.Dropdown{
+                        name = "a", items = {"hello", "world"},
+                        index_event = true,
+                    })
+                    assert.equals(ctx.form.a, 1)
+                    event({a = "nan"})
+                    assert.equals(ctx.form.a, 1)
+                end)
+
+                it("converts numbers to integers", function()
+                    local ctx, event = render_to_string(gui.Dropdown{
+                        name = "a", items = {"hello", "world"},
+                        index_event = true,
+                    })
+                    assert.equals(ctx.form.a, 1)
+                    event({a = "2.1"})
+                    assert.equals(ctx.form.a, 2)
+                end)
+
+                it("ignores out of bounds input", function()
+                    local ctx, event = render_to_string(gui.Dropdown{
+                        name = "a", items = {"hello", "world"},
+                        index_event = true,
+                    })
+                    assert.equals(ctx.form.a, 1)
+                    event({a = "3"})
+                    assert.equals(ctx.form.a, 1)
+                end)
+            end)
+        end)
+
+        describe("Textlist", function()
+            it("converts the result to a number", function()
+                local ctx, event = render_to_string(gui.Textlist{
+                    name = "a", listelems = {"hello", "world"},
+                    selected_idx = 2
+                })
+                assert.equals(ctx.form.a, 2)
+                event({a = "CHG:1"})
+                assert.equals(ctx.form.a, 1)
+            end)
+
+            it("ignores out of bounds values", function()
+                local ctx, event = render_to_string(gui.Textlist{
+                    name = "a", listelems = {"hello", "world"},
+                    selected_idx = 2
+                })
+                assert.equals(ctx.form.a, 2)
+                event({a = "CHG:3"})
+                assert.equals(ctx.form.a, 2)
+            end)
+        end)
+
+        describe("Table", function()
+            it("converts the result to a number", function()
+                local ctx, event = render_to_string(gui.Table{
+                    name = "a", cells = {"hello", "world"},
+                    selected_idx = 2
+                })
+                assert.equals(ctx.form.a, 2)
+                event({a = "CHG:1:0"})
+                assert.equals(ctx.form.a, 1)
+            end)
+
+            it("ignores out of bounds values", function()
+                local ctx, event = render_to_string(gui.Table{
+                    name = "a", cells = {"hello", "world"}
+                })
+                assert.equals(ctx.form.a, 1)
+                event({a = "CHG:3:0"})
+                assert.equals(ctx.form.a, 1)
+            end)
+
+            it("does not replace zero values", function()
+                local ctx, event = render_to_string(gui.Table{
+                    name = "a", cells = {"hello", "world"}, selected_idx = 0
+                })
+                assert.equals(ctx.form.a, 0)
+                event({a = "INV"})
+                assert.equals(ctx.form.a, 0)
+            end)
+
+            it("understands tablecolumns", function()
+                local ctx, event = render_to_string(gui.VBox{
+                    gui.TableColumns{
+                        tablecolumns = {
+                            {type = "text", opts = {}},
+                            {type = "text", opts = {}},
+                        }
+                    },
+                    gui.Table{
+                        name = "a", cells = {"1", "2", "3", "4", "5", "6"},
+                    }
+                })
+                assert.equals(ctx.form.a, 1)
+                event({a = "CHG:3:0"})
+                assert.equals(ctx.form.a, 3)
+            end)
+
+            it("ignores out-of-bounds values with tablecolumns", function()
+                local ctx, event = render_to_string(gui.VBox{
+                    gui.TableColumns{
+                        tablecolumns = {
+                            {type = "text", opts = {}},
+                            {type = "text", opts = {}},
+                        }
+                    },
+                    gui.Table{
+                        name = "a", cells = {"1", "2", "3", "4", "5", "6"},
+                    }
+                })
+                assert.equals(ctx.form.a, 1)
+                event({a = "CHG:4:0"})
+                assert.equals(ctx.form.a, 1)
+            end)
+        end)
+
+        it("does not save form input for Button", function()
+            local ctx, event = render_to_string(gui.Button{name = "a"})
+            assert.equals(ctx.form.a, nil)
+            event({a = "test"})
+            assert.equals(ctx.form.a, nil)
+        end)
+    end)
 end)