[tarantool-patches] [PATCH] httpc: add checking of headers in httpc:request

[tarantool-patches] [PATCH] httpc: add checking of headers in httpc:request

[Roman Khabibov]

Add function that checks the value of each header. It must be 'string' or 'table'
with '__tostring' metamethod.

Closes #3679
---

Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3679-httpc-request
Issue: https://github.com/tarantool/tarantool/issues/3679

 src/lua/httpc.lua                 | 23 +++++++++++++++++++++++
 test/app-tap/http_client.test.lua | 17 +++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/src/lua/httpc.lua b/src/lua/httpc.lua
index cd44b6054..ede976a3a 100644
--- a/src/lua/httpc.lua
+++ b/src/lua/httpc.lua
@@ -216,6 +216,28 @@ local function process_cookies(cookies)
     return result
 end
 
+local function check_headers(opts)
+    if opts ~= nil then
+        local headers = opts["headers"]
+        if headers ~= nil then
+            for header, value in pairs(headers) do
+                if type(value) ~= 'string' then
+                    if  type(value) ~= 'table' then
+                        error('Usage: httpc:request bad \''..header..'\' value')
+                    end
+                    local mt = getmetatable(value)
+                    if mt == nil then
+                        error('Usage: httpc:request \''..header..'\' has not \'__tostring\'')
+                    end
+                    if mt["__tostring"] ~= nil then
+                        headers[header] = tostring(value)
+                    end
+                end
+            end
+        end
+    end
+end
+
 local function process_headers(headers)
     for header, value in pairs(headers) do
         if type(value) == 'table' then
@@ -296,6 +318,7 @@ curl_mt = {
             if not method or not url then
                 error('request(method, url [, options]])')
             end
+            check_headers(opts)
             local resp = self.curl:request(method, url, body, opts or {})
             if resp and resp.headers then
                 if resp.headers['set-cookie'] ~= nil then
diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua
index 10a3728f8..d7e01d60f 100755
--- a/test/app-tap/http_client.test.lua
+++ b/test/app-tap/http_client.test.lua
@@ -205,6 +205,23 @@ local function test_errors(test)
     test:is(r.status, 595, "GET: response on bad url")
 end
 
+--gh-3679 Check that client check that the httpc:request doesn't modify headers.
+
+local function test_request_headers(test)
+    test:plan(5)
+    httpc = require('http.client').new()
+    test:ok(not pcall(httpc:request('GET', 'localhost:4444', nil, {headers = {aaa = true}})),
+            'expected error about bad value')
+    test:ok(not pcall(httpc:request('GET', 'localhost:4444', nil, {headers = {aaa = 10}})),
+            'expected error about bad value')
+    test:ok(not pcall(httpc:request('GET', 'localhost:4444', nil, {headers = {aaa = box.NULL}})),
+            'expected error about bad value')
+    test:ok(not pcall(httpc:request('GET', 'localhost:4444', nil, {headers = {aaa = 10ULL}})),
+            'expected error about bad value')
+    test:ok(not pcall(httpc:request('GET', 'localhost:4444', nil, {headers = {aaa = {}}})),
+            'expected error with no \'__tostring\'')
+end
+
 local function test_headers(test, url, opts)
     test:plan(19)
     local http = client:new()
-- 
2.17.1

[tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request

[Alexander Turenko]

Hi!

See comments below and proposed fixup on the branch
romanhabibov/gh-3679-httpc-request-fixups. Please, amend the patch and
proceed with the next reviewer (Vlad, added to CC).

WBR, Alexander Turenko.

On Fri, Dec 21, 2018 at 09:18:44PM +0300, Roman Khabibov wrote:
> Add function that checks the value of each header. It must be 'string' or 'table'
> with '__tostring' metamethod.
> 
> Closes #3679
> ---
> 
> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3679-httpc-request
> Issue: https://github.com/tarantool/tarantool/issues/3679
> 
>  src/lua/httpc.lua                 | 23 +++++++++++++++++++++++
>  test/app-tap/http_client.test.lua | 17 +++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/src/lua/httpc.lua b/src/lua/httpc.lua
> index cd44b6054..ede976a3a 100644
> --- a/src/lua/httpc.lua
> +++ b/src/lua/httpc.lua
> @@ -216,6 +216,28 @@ local function process_cookies(cookies)
>      return result
>  end
>  
> +local function check_headers(opts)
> +    if opts ~= nil then
> +        local headers = opts["headers"]
> +        if headers ~= nil then
> +            for header, value in pairs(headers) do
> +                if type(value) ~= 'string' then
> +                    if  type(value) ~= 'table' then
> +                        error('Usage: httpc:request bad \''..header..'\' value')
> +                    end
> +                    local mt = getmetatable(value)
> +                    if mt == nil then
> +                        error('Usage: httpc:request \''..header..'\' has not \'__tostring\'')
> +                    end
> +                    if mt["__tostring"] ~= nil then
> +                        headers[header] = tostring(value)
> +                    end
> +                end
> +            end
> +        end
> +    end
> +end
> +

1. Too common name, IMHO.
2. Can be implemented with much less indentation level.
3. Changes user provided options.

>  local function process_headers(headers)
>      for header, value in pairs(headers) do
>          if type(value) == 'table' then
> @@ -296,6 +318,7 @@ curl_mt = {
>              if not method or not url then
>                  error('request(method, url [, options]])')
>              end
> +            check_headers(opts)
>              local resp = self.curl:request(method, url, body, opts or {})
>              if resp and resp.headers then
>                  if resp.headers['set-cookie'] ~= nil then
> diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua
> index 10a3728f8..d7e01d60f 100755
> --- a/test/app-tap/http_client.test.lua
> +++ b/test/app-tap/http_client.test.lua
> @@ -205,6 +205,23 @@ local function test_errors(test)
>      test:is(r.status, 595, "GET: response on bad url")
>  end
>  
> +--gh-3679 Check that client check that the httpc:request doesn't modify headers.
> +
> +local function test_request_headers(test)
> +    test:plan(5)
> +    httpc = require('http.client').new()
> +    test:ok(not pcall(httpc:request('GET', 'localhost:4444', nil, {headers = {aaa = true}})),
> +            'expected error about bad value')
> +    test:ok(not pcall(httpc:request('GET', 'localhost:4444', nil, {headers = {aaa = 10}})),
> +            'expected error about bad value')
> +    test:ok(not pcall(httpc:request('GET', 'localhost:4444', nil, {headers = {aaa = box.NULL}})),
> +            'expected error about bad value')
> +    test:ok(not pcall(httpc:request('GET', 'localhost:4444', nil, {headers = {aaa = 10ULL}})),
> +            'expected error about bad value')
> +    test:ok(not pcall(httpc:request('GET', 'localhost:4444', nil, {headers = {aaa = {}}})),
> +            'expected error with no \'__tostring\'')
> +end
> +

1. Don't called.
2. Don't check error messages.
3. Too long lines.
4. Will not work in strict mode (require('strict').on() or Debug build).
5. pcall is called for the result, so it will not work as you expected;
   see below.
6. The message above the function is not about what the function does
   and is not what the issue states.

Re 5, compare:

pcall(error, 'hello')
pcall(error('hello'))

It is sad that you not even check whether your test was run.

>  local function test_headers(test, url, opts)
>      test:plan(19)
>      local http = client:new()
> -- 
> 2.17.1
> 

[tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request

[Roman]

On 23.12.2018 6:19, Alexander Turenko wrote:
> Hi!
>
> See comments below and proposed fixup on the branch
> romanhabibov/gh-3679-httpc-request-fixups. Please, amend the patch and
> proceed with the next reviewer (Vlad, added to CC).
>
> WBR, Alexander Turenko.

Done. Vlad, review, please.


commit 253be70b02a700ef9936fbab98f80c79de3cf0fe
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date:   Wed Dec 26 17:49:34 2018 +0300

     httpc: add checking of headers in httpc:request

     Add functions that preprocess the request headers. Each header must 
be nil, 'string' or 'table'
     with '__tostring' metamethod.

     Closes #3679

diff --git a/src/lua/httpc.lua b/src/lua/httpc.lua
index cd44b6054..21a7cd2fd 100644
--- a/src/lua/httpc.lua
+++ b/src/lua/httpc.lua
@@ -216,6 +216,42 @@ local function process_cookies(cookies)
      return result
  end

+local function bad_header_error(func_name, header_name)
+    error(('%s: cannot convert header "%s" to a string'):format(
+        func_name, header_name))
+end
+
+-- Verify and preprocess outcoming headers before send a request.
+--
+-- Return the new headers table (with all string values) or nil
+-- when no headers were provided.
+local function preprocess_request_headers(headers)
+    local func_name = 'preprocess_request_headers'
+
+    if headers == nil then
+        return nil
+    end
+
+    local res = {}
+
+    for name, value in pairs(headers) do
+        local lua_type = type(value)
+        if lua_type == 'string' then
+            res[name] = value
+        elseif lua_type == 'table' then
+            local mt = getmetatable(value)
+            if mt == nil or mt.__tostring == nil then
+                bad_header_error(func_name, name)
+            end
+            res[name] = tostring(value)
+        else
+            bad_header_error(func_name, name)
+        end
+    end
+
+    return res
+end
+
  local function process_headers(headers)
      for header, value in pairs(headers) do
          if type(value) == 'table' then
@@ -296,6 +332,12 @@ curl_mt = {
              if not method or not url then
                  error('request(method, url [, options]])')
              end
+            local headers = preprocess_request_headers(opts and 
opts.headers)
+            if headers ~= nil then
+                -- prevent changing of user provided options
+                opts = table.copy(opts)
+                opts.headers = headers
+            end
              local resp = self.curl:request(method, url, body, opts or {})
              if resp and resp.headers then
                  if resp.headers['set-cookie'] ~= nil then
diff --git a/test/app-tap/http_client.test.lua 
b/test/app-tap/http_client.test.lua
index 10a3728f8..4ae382fe7 100755
--- a/test/app-tap/http_client.test.lua
+++ b/test/app-tap/http_client.test.lua
@@ -205,6 +205,81 @@ 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 = 'preprocess_request_headers: cannot convert header ' ..
+        '"aaa" 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<uint64_t> 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 +472,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

[tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request

[Vladislav Shpilevoy]

Hi! Thanks for the patch! See 5 comments below.

On 26/12/2018 18:56, Roman wrote:
> 
> On 23.12.2018 6:19, Alexander Turenko wrote:
>> Hi!
>>
>> See comments below and proposed fixup on the branch
>> romanhabibov/gh-3679-httpc-request-fixups. Please, amend the patch and
>> proceed with the next reviewer (Vlad, added to CC).
>>
>> WBR, Alexander Turenko.
> 
> Done. Vlad, review, please.
> 
> 
> commit 253be70b02a700ef9936fbab98f80c79de3cf0fe
> Author: Roman Khabibov <roman.habibov@tarantool.org>
> Date:   Wed Dec 26 17:49:34 2018 +0300
> 
>      httpc: add checking of headers in httpc:request
> 
>      Add functions that preprocess the request headers. Each header must be nil, 'string' or 'table'
>      with '__tostring' metamethod.
> 
>      Closes #3679

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.

> 
> diff --git a/src/lua/httpc.lua b/src/lua/httpc.lua
> index cd44b6054..21a7cd2fd 100644
> --- a/src/lua/httpc.lua
> +++ b/src/lua/httpc.lua
> @@ -216,6 +216,42 @@ local function process_cookies(cookies)
>       return result
>   end
> 
> +local function bad_header_error(func_name, header_name)
> +    error(('%s: cannot convert header "%s" to a string'):format(
> +        func_name, header_name))

2. Broken indentation. We never wrap a line right after '(',
and when we wrap parameters, they should be aligned by '(' of
the first line.

> +end
> +
> +-- Verify and preprocess outcoming headers before send a request.
> +--
> +-- Return the new headers table (with all string values) or nil
> +-- when no headers were provided.

3. Please, use @retval when describing return value.

> +local function preprocess_request_headers(headers)
> +    local func_name = 'preprocess_request_headers'

4. For a user this function name is useless. He called
:request(), but in logs doesn't see an appropriate message.

> +
> +    if headers == nil then
> +        return nil
> +    end
> +
> +    local res = {}
> +
> +    for name, value in pairs(headers) do
> +        local lua_type = type(value)
> +        if lua_type == 'string' then
> +            res[name] = value
> +        elseif lua_type == 'table' then
> +            local mt = getmetatable(value)
> +            if mt == nil or mt.__tostring == nil then
> +                bad_header_error(func_name, name)
> +            end
> +            res[name] = tostring(value)
> +        else
> +            bad_header_error(func_name, name)
> +        end
> +    end
> +
> +    return res
> +end
> +
>   local function process_headers(headers)
>       for header, value in pairs(headers) do
>           if type(value) == 'table' then
> @@ -296,6 +332,12 @@ curl_mt = {
>               if not method or not url then
>                   error('request(method, url [, options]])')
>               end
> +            local headers = preprocess_request_headers(opts and opts.headers)
> +            if headers ~= nil then
> +                -- prevent changing of user provided options
> +                opts = table.copy(opts)
> +                opts.headers = headers
> +            end

5. Already second table copying. This is why I do not like it in Lua.

>               local resp = self.curl:request(method, url, body, opts or {})
>               if resp and resp.headers then
>                   if resp.headers['set-cookie'] ~= nil then

[tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request

[Roman]

> 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 <roman.habibov@tarantool.org>
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<uint64_t> 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

[tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request

[Roman]

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 <roman.habibov@tarantool.org>
> 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<uint64_t> 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
>
>

[tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request

[Vladislav Shpilevoy]

Hi! Thanks for the patch! See 7 comments below.

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 <roman.habibov@tarantool.org>
> 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);

1. I see, that you didn't use format-patch + send-email, since here we
have white-spaces, but in the real diff there are tabs. Please, use
format-patch + send-email. Or if you used them, then fix you text editor.
Or stop using web client - use a specialized one like Thunderbird.

>           while (lua_next(L, -2) != 0) {
> +            int header_type = lua_type(L, -1);
> +            if (header_type != LUA_TSTRING) {

2. Why so complex instead of just lua_isstring()?

> +                if (header_type != LUA_TTABLE) {
> +                    return luaL_error(L, "cannot convert header to a string");

3. Word 'cannot' does not exist. Please, use "can't" or "can not".

4. In the commit message you said that 'nil' is ok, but here you did not
check for that and return an error. Of course, real nil can not appear in
a table, because it means absence of a value, but what about box.NULL?

5. Out of 80 symbols.

> +                } else if (luaL_getmetafield(L, -1, "__tostring") == LUA_TNIL) {

6. Why do you compare with LUA_TNIL instead of direct checking of 0/1?
I see, that luaL_getmetafield does return exactly 0 or 1, not LUA_TNIL. Maybe
they accidentally matched, but it is not a reason to use LUA_TNIL here.

> +                    return luaL_error(L, "cannot convert header to a string");
> +                }
> +                lua_pop(L, 1);

7. Why do you pop? Now on a stack we have key (L[-2]) and value (L[-1]).
Here you pops one, and we have key (L[-1]). So the function below does a
mess. And maybe this is a reason why the test below fails.

> +            }
>               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

[tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request

[Alexander Turenko]

> >           while (lua_next(L, -2) != 0) {
> > +            int header_type = lua_type(L, -1);
> > +            if (header_type != LUA_TSTRING) {
> 
> 2. Why so complex instead of just lua_isstring()?

It is not quite same. lua_isstring() accepts numbers as well as strings.

[tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request

[Roman]

Hello! Thanks for the review.

On 29.01.2019 22:44, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch! See 7 comments below.
>
> 2. Why so complex instead of just lua_isstring()?

As written by Alexander, it really takes a number.


> 3. Word 'cannot' does not exist. Please, use "can't" or "can not".

Fixed.


> 4. In the commit message you said that 'nil' is ok, but here you did not
> check for that and return an error. Of course, real nil can not appear in
> a table, because it means absence of a value, but what about box.NULL?

Added test for this case, right?


> 5. Out of 80 symbols.
>
>> +                } else if (luaL_getmetafield(L, -1, "__tostring") == 
>> LUA_TNIL) {
>
> 6. Why do you compare with LUA_TNIL instead of direct checking of 0/1?
> I see, that luaL_getmetafield does return exactly 0 or 1, not 
> LUA_TNIL. Maybe
> they accidentally matched, but it is not a reason to use LUA_TNIL here.

Fixed.


>
>> +                    return luaL_error(L, "cannot convert header to a 
>> string");
>> +                }
>> +                lua_pop(L, 1);
>
> 7. Why do you pop? Now on a stack we have key (L[-2]) and value (L[-1]).
> Here you pops one, and we have key (L[-1]). So the function below does a
> mess. And maybe this is a reason why the test below fails.

Because luaL_getmetafield puts on the stack a field from the metatable 
of the object at index -1, if there is one.


commit 5c02d723b8a6552bbd78deef6acd719bddf1e620
Author: Roman Khabibov <roman.habibov@tarantool.org>
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..d2a07c863 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, "can't convert header to a 
string");
+                } else if (!luaL_getmetafield(L, -1, "__tostring")) {
+                    return luaL_error(L, "can't 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..f231c2e90 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 = 'can\'t 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<uint64_t> 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

[tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request

[Vladislav Shpilevoy]

Hi! Thanks for the fixes! See 3 comments below.

>> 4. In the commit message you said that 'nil' is ok, but here you did not
>> check for that and return an error. Of course, real nil can not appear in
>> a table, because it means absence of a value, but what about box.NULL?
> 
> Added test for this case, right?

1. Yes, you did, but the test fails. On my laptop http_client.test.lua
is totally broken for already a year at least, but I run the test from
console and got this:

     	tarantool> client = require('http.client')
	---
	...

	tarantool> http = client:new()
	---
	...

	tarantool> opts = {headers = {aaa = box.NULL}}
	---
	...

	tarantool> http:get('127.0.0.1:12345', opts)
	---
	- error: 'builtin/http.client.lua:299: can''t convert header to a string'

So box.NULL is not converted to string despite the fact that box.NULL == nil
and you said in the commit message

     "... Each header must be nil, 'string' or 'table' ..."

> 
> 
>> 5. Out of 80 symbols.

2. Still out of 80.

>>
>>> +                } else if (luaL_getmetafield(L, -1, "__tostring") == LUA_TNIL) {
> diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua
> index 10a3728f8..f231c2e90 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

3. I am not a linguist, but the sentence looks grammatically
incorrect IMO. 'Allow only headers <something is missing> can be ... '.

> +local function test_request_headers(test, url, opts)
> +    local exp_err = 'can\'t convert header to a string'
> +    local cases = {
> +        {

[tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request

[Roman]

[-- Attachment #1: Type: text/plain, Size: 7273 bytes --]

Hi! Thanks for review.

On Вт, фев 5, 2019 at 2:42 PM, Vladislav Shpilevoy 
<v.shpilevoy@tarantool.org> wrote:
> Hi! Thanks for the fixes! See 3 comments below.
> 
>>> 4. In the commit message you said that 'nil' is ok, but here you 
>>> did not
>>> check for that and return an error. Of course, real nil can not 
>>> appear in
>>> a table, because it means absence of a value, but what about 
>>> box.NULL?
>> 
>> Added test for this case, right?
> 
> 1. Yes, you did, but the test fails. On my laptop http_client.test.lua
> is totally broken for already a year at least, but I run the test from
> console and got this:
> 
>     	tarantool> client = require('http.client')
> 	---
> 	...
> 
> 	tarantool> http = client:new()
> 	---
> 	...
> 
> 	tarantool> opts = {headers = {aaa = box.NULL}}
> 	---
> 	...
> 
> 	tarantool> http:get('127.0.0.1:12345', opts)
> 	---
> 	- error: 'builtin/http.client.lua:299: can''t convert header to a 
> string'
> 
> So box.NULL is not converted to string despite the fact that box.NULL 
> == nil
> and you said in the commit message
> 
>     "... Each header must be nil, 'string' or 'table' ..."
In a private chat, we decided that this result is ok. And I changed the 
commit message.

>>> 5. Out of 80 symbols.
> 
> 2. Still out of 80.
My text editor shows 70 symbols.

>>>> +                } else if (luaL_getmetafield(L, -1, "__tostring") 
>>>> == LUA_TNIL) {
>> diff --git a/test/app-tap/http_client.test.lua 
>> b/test/app-tap/http_client.test.lua
>> index 10a3728f8..f231c2e90 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
> 
> 3. I am not a linguist, but the sentence looks grammatically
> incorrect IMO. 'Allow only headers <something is missing> can be ... 
> '.
> 
>> +local function test_request_headers(test, url, opts)
>> +    local exp_err = 'can\'t convert header to a string'
>> +    local cases = {
>> +        {
+				if (header_type != LUA_TTABLE) {
+					return luaL_error(L,
+						"headers must be string or table with \"__tostring\"");
+				} else if (!luaL_getmetafield(L, -1, "__tostring")) {
+					return luaL_error(L,
+						"headers must be string or table with \"__tostring\"");
+				}
Done like other error messages in this file. But I do not know how best 
to fit code in 80 symbols.


commit 104dbc419aaf3d9ec8f294513c73a98d2f103459
Author: Roman Khabibov <roman.habibov@tarantool.org>
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..bc89a2334 100644
--- a/src/lua/httpc.c
+++ b/src/lua/httpc.c
@@ -173,6 +173,17 @@ 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,
+						"headers must be string or table with \"__tostring\"");
+				} else if (!luaL_getmetafield(L, -1, "__tostring")) {
+					return luaL_error(L,
+						"headers must be string or table with \"__tostring\"");
+				}
+				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<uint64_t> 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



[-- Attachment #2: Type: text/html, Size: 8281 bytes --]

[tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request

[Vladislav Shpilevoy]

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.

[tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request

[Roman]

[-- Attachment #1: Type: text/plain, Size: 5604 bytes --]


Hi! Thanks for review.

On Сб, фев 16, 2019 at 12:17 AM, Vladislav Shpilevoy 
<v.shpilevoy@tarantool.org> wrote:
> Hi!
> 
>>>         5. Out of 80 symbols. \x7f\x7f
>>> 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.

commit c9802592b27cb6986e3e72eb948d8dbfb21baa7e
Author: Roman Khabibov <roman.habibov@tarantool.org>
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\"";
+				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<uint64_t> 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



[-- Attachment #2: Type: text/html, Size: 13142 bytes --]

[tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request

[Vladislav Shpilevoy]

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 <v.shpilevoy@tarantool.org> wrote:
>> Hi!
>>
>>         5. Out of 80 symbols. \x7f\x7f 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.

> 
> commit c9802592b27cb6986e3e72eb948d8dbfb21baa7e
> Author: Roman Khabibov <roman.habibov@tarantool.org>
> 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);
> + }

3. Obviously, there is still something wrong with indentation of your
editor/email agent/whatever else can remove tabs. Please, cope with it.
Before sending a next version into the list, send it to yourself and
check if now the indentation is good.

> if (httpc_set_header(req, "%s: %s",
> lua_tostring(L, -2),
> lua_tostring(L, -1)) < 0) {

[tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request

[Roman Khabibov]

[-- Attachment #1: Type: text/plain, Size: 7370 bytes --]

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 <v.shpilevoy@tarantool.org> wrote:
>>> Hi!
>>>          5. Out of 80 symbols. \x7f\x7f 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 <roman.habibov@tarantool.org>
>> 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.

+				const char *err_msg =
+					"headers must be string or table "\
+					"with \"__tostring\"";


commit 42d25f4d6eef69ae56db3841863afa3373fa9b67
Author: Roman Khabibov <roman.habibov@tarantool.org>
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<uint64_t> 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


[-- Attachment #2: Type: text/html, Size: 9552 bytes --]

[tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request

[Roman Khabibov]

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 
>> <v.shpilevoy@tarantool.org> <mailto:v.shpilevoy@tarantool.org>
>>  wrote: 
>> 
>>> Hi! 
>>>          5. Out of 80 symbols. \x7f\x7f 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 <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 
>> <roman.habibov@tarantool.org> <mailto:roman.habibov@tarantool.org>
>>  
>> 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 <roman.habibov@tarantool.org>
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<uint64_t> 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

[tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request

[Vladislav Shpilevoy]

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
>>> <v.shpilevoy@tarantool.org> <mailto:v.shpilevoy@tarantool.org>
>>>   wrote:
>>>
>>>> Hi!
>>>>           5. Out of 80 symbols. \x7f\x7f 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 <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
>>> <roman.habibov@tarantool.org> <mailto:roman.habibov@tarantool.org>
>>>   
>>> 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 <roman.habibov@tarantool.org>
> 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<uint64_t> 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
> 

[tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request

[Kirill Yukhin]

Hello,

On 21 Dec 21:18, Roman Khabibov wrote:
> Add function that checks the value of each header. It must be 'string' or 'table'
> with '__tostring' metamethod.
> 
> Closes #3679
> ---
> 
> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3679-httpc-request
> Issue: https://github.com/tarantool/tarantool/issues/3679

I've checked your patch into 2.1 and 1.10 branch.

--
Regards, Kirill Yukhin