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 A4D6E26FDB for ; Mon, 25 Feb 2019 08:04:34 -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 xnhx630icMIG for ; Mon, 25 Feb 2019 08:04:34 -0500 (EST) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 2DBF22691F for ; Mon, 25 Feb 2019 08:04:34 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request 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> <7f31300f-2e4a-f0b1-a40c-90d631fb9778@tarantool.org> <2e5f83a6-2885-96f8-6fd6-a5fd34e32644@tarantool.org> <1549927450.14514.0@smtp.mail.ru> <1550573375.20050.0@smtp.mail.ru> From: Vladislav Shpilevoy Message-ID: Date: Mon, 25 Feb 2019 16:04:31 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: Roman Khabibov , tarantool-patches@freelists.org, Kirill Yukhin LGTM. On 24/02/2019 01:43, Roman Khabibov wrote: > Hi! Thanks for review. > > On 2/22/19 7:01 PM, Vladislav Shpilevoy wrote: > > >> Hi! Thanks for the fixes! See 3 comments below. >> On 19/02/2019 13:49, Roman wrote: >> >>> Hi! Thanks for review. >>> On Сб, фев 16, 2019 at 12:17 AM, Vladislav Shpilevoy >>> >>> wrote: >>> >>>> Hi! >>>> 5. Out of 80 symbols.  2. Still out of 80. >>>> >>>> My text editor shows 70 symbols. >>>> >>>> Then there is something wrong with your editor, sorry. >>>> https://github.com/tarantool/tarantool/blob/romanhabibov/gh-3679-httpc-request/src/lua/httpc.c#L180 >>>> Lets count. Here you have 6 tabs - 6 * 8 = 48 symbols. Then you have 55 symbols of "headers must be string or table with \"__tostring\""); 48 + 55 = 103. Also, the error string is not aligned by opening parenthesis. In your email I see, that you have 4 symbols tab, what gives you 79 symbols, but tab width in this file should be 8. So probably you mistakenly set tab width to 4 and the editor shows you < 80 width. Please, fix. >>>> >>> Ok. Done. >>> >> 1. Now you have leading whitespace on line 182. It is clearly visible in >> git diff as a bright red point. Please, do self-review before sending >> a patch. >> > I'm sorry to waste your time. I was in a hurry and did't notice. > > > >>> commit c9802592b27cb6986e3e72eb948d8dbfb21baa7e >>> 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 'string' or 'table' >>> with '__tostring' metamethod. >>> Closes #3679 >>> diff --git a/src/lua/httpc.c b/src/lua/httpc.c >>> index 5f4e2e912..fb012f58b 100644 >>> --- a/src/lua/httpc.c >>> +++ b/src/lua/httpc.c >>> @@ -173,6 +173,18 @@ 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) { >>> + char *err_msg = "headers must be " \ >>> + "string or table with \"__tostring\""; >>> >> 2. Please, do not assign a const char string to >> a 'char *' variable. Use 'const char *'. Also, >> please, properly align strings. This part should >> look like this: >> if (header_type != LUA_TSTRING) { >> const char *err_msg = >> "headers must be string or table with "\ >> "\"__tostring\""; >> if (header_type != LUA_TTABLE) { >> Wrapped string is aligned by the beginning. >> >>> + if (header_type != LUA_TTABLE) { >>> + return luaL_error(L, err_msg); >>> + } else if (!luaL_getmetafield(L, -1, >>> + "__tostring")) { >>> + return luaL_error(L, err_msg); >>> + } >>> + lua_pop(L, 1); >>> + } >>> > Done. > > > commit 42d25f4d6eef69ae56db3841863afa3373fa9b67 > 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 'string' or 'table' > with '__tostring' metamethod. > > Closes #3679 > > diff --git a/src/lua/httpc.c b/src/lua/httpc.c > index 5f4e2e912..db8634949 100644 > --- a/src/lua/httpc.c > +++ b/src/lua/httpc.c > @@ -173,6 +173,19 @@ 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) { > + const char *err_msg = > + "headers must be string or table "\ > + "with \"__tostring\""; > + if (header_type != LUA_TTABLE) { > + return luaL_error(L, err_msg); > + } else if (!luaL_getmetafield(L, -1, > + "__tostring")) { > + return luaL_error(L, err_msg); > + } > + 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..8a98d1e92 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 = 'headers must be string or table with "__tostring"' > + 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 >