From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id B4973250BE for ; Thu, 24 Jan 2019 09:54:05 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id FaTw2MTj2GIk for ; Thu, 24 Jan 2019 09:54:05 -0500 (EST) Received: from smtp5.mail.ru (smtp5.mail.ru [94.100.179.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 6CA4024466 for ; Thu, 24 Jan 2019 09:54:05 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request From: Roman References: <20181221181844.4417-1-roman.habibov@tarantool.org> <20181223031929.hhmxdlsox2aon5ih@tkn_work_nb> <3b2626ce-b887-70dd-38b0-ef38b37a983c@tarantool.org> <72bbdfde-54d7-0e94-c5e7-ca84c2a6078d@tarantool.org> Message-ID: <9c3f8039-7e92-fc5a-aeb0-0ea30fbea699@tarantool.org> Date: Thu, 24 Jan 2019 17:54:02 +0300 MIME-Version: 1.0 In-Reply-To: <72bbdfde-54d7-0e94-c5e7-ca84c2a6078d@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, Alexander Turenko Cc: Vladislav Shpilevoy On 09.01.2019 16:29, Roman wrote: > >> 1. Firstly, I think, that the patch should be in C in >> lua/httpc.c and affect only function luaT_httpc_request. It >> will prevent creation of additional table 'res', maybe >> will look even shorter, and evidently will be more efficient. >> It is not so complex logic so as to write it in Lua, IMO. >> > Done. > > commit beb0562425dd5062af5d18ae202f0f4434cb5108 > Author: Roman Khabibov > Date:   Wed Dec 26 17:49:34 2018 +0300 > >     httpc: add checking of headers in httpc:request > >     Add preprocessing of the request headers. Each header must be nil, > 'string' or 'table' >     with '__tostring' metamethod. > >     Closes #3679 > > diff --git a/src/lua/httpc.c b/src/lua/httpc.c > index 5f4e2e912..16a435ef1 100644 > --- a/src/lua/httpc.c > +++ b/src/lua/httpc.c > @@ -173,6 +173,15 @@ luaT_httpc_request(lua_State *L) >      if (!lua_isnil(L, -1)) { >          lua_pushnil(L); >          while (lua_next(L, -2) != 0) { > +            int header_type = lua_type(L, -1); > +            if (header_type != LUA_TSTRING) { > +                if (header_type != LUA_TTABLE) { > +                    return luaL_error(L, "cannot convert header to a > string"); > +                } else if (luaL_getmetafield(L, -1, "__tostring") == > LUA_TNIL) { > +                    return luaL_error(L, "cannot convert header to a > string"); > +                } > +                lua_pop(L, 1); > +            } >              if (httpc_set_header(req, "%s: %s", >                           lua_tostring(L, -2), >                           lua_tostring(L, -1)) < 0) { > diff --git a/test/app-tap/http_client.test.lua > b/test/app-tap/http_client.test.lua > index 10a3728f8..d1fde5009 100755 > --- a/test/app-tap/http_client.test.lua > +++ b/test/app-tap/http_client.test.lua > @@ -205,6 +205,80 @@ local function test_errors(test) >      test:is(r.status, 595, "GET: response on bad url") >  end > > +-- gh-3679 allow only headers can be converted to string > +local function test_request_headers(test, url, opts) > +    local exp_err = 'cannot convert header to a string' > +    local cases = { > +        { > +            'string header', > +            opts = {headers = {aaa = 'aaa'}}, > +            exp_err = nil, > +        }, > +        { > +            'header with __tostring() metamethod', > +            opts = {headers = {aaa = setmetatable({}, { > +                __tostring = function(self) > +                    return 'aaa' > +                end})}}, > +            exp_err = nil, > +            postrequest_check = function(opts) > +                assert(type(opts.headers.aaa) == 'table', > +                    '"aaa" header was modified in http_client') > +            end, > +        }, > +        { > +            'boolean header', > +            opts = {headers = {aaa = true}}, > +            exp_err = exp_err, > +        }, > +        { > +            'number header', > +            opts = {headers = {aaa = 10}}, > +            exp_err = exp_err, > +        }, > +        { > +            'cdata header (box.NULL)', > +            opts = {headers = {aaa = box.NULL}}, > +            exp_err = exp_err, > +        }, > +        { > +            'cdata header', > +            opts = {headers = {aaa = 10ULL}}, > +            exp_err = exp_err, > +        }, > +        { > +            'table header w/o metatable', > +            opts = {headers = {aaa = {}}}, > +            exp_err = exp_err, > +        }, > +        { > +            'table header w/o __tostring() metamethod', > +            opts = {headers = {aaa = setmetatable({}, {})}}, > +            exp_err = exp_err, > +        }, > +    } > +    test:plan(#cases) > + > +    local http = client:new() > + > +    for _, case in ipairs(cases) do > +        local opts = merge(table.copy(opts), case.opts) > +        local ok, err = pcall(http.get, http, url, opts) > +        if case.postrequest_check ~= nil then > +            case.postrequest_check(opts) > +        end > +        if case.exp_err == nil then > +            -- expect success > +            test:ok(ok, case[1]) > +        else > +            -- expect fail > +            assert(type(err) == 'string') > +            err = err:gsub('^builtin/[a-z._]+.lua:[0-9]+: ', '') > +            test:is_deeply({ok, err}, {false, case.exp_err}, case[1]) > +        end > +    end > +end > + >  local function test_headers(test, url, opts) >      test:plan(19) >      local http = client:new() > @@ -397,12 +471,13 @@ local function test_concurrent(test, url, opts) >  end > >  function run_tests(test, sock_family, sock_addr) > -    test:plan(9) > +    test:plan(10) >      local server, url, opts = start_server(test, sock_family, sock_addr) >      test:test("http.client", test_http_client, url, opts) >      test:test("cancel and errinj", test_cancel_and_errinj, url .. > 'long_query', opts) >      test:test("basic http post/get", test_post_and_get, url, opts) >      test:test("errors", test_errors) > +    test:test("request_headers", test_request_headers, url, opts) >      test:test("headers", test_headers, url, opts) >      test:test("special methods", test_special_methods, url, opts) >      if sock_family == 'AF_UNIX' and jit.os ~= "Linux" then > >