scrapinghub / splash

Lightweight, scriptable browser as a service with an HTTP API
BSD 3-Clause "New" or "Revised" License
4.09k stars 513 forks source link

Error when `request:set_header(name, value)` receives integer as `value` #827

Open seagatesoft opened 6 years ago

seagatesoft commented 6 years ago

My Lua script has the following snippet:

local value = 10.0

splash:on_request(function (request)
    request:set_header("Custom-Header", value)
    request:set_header("Another-Custom-Header", value2)
    request:set_proxy(host, port, username, password)
end)

On that case, request:set_header("Custom-Header", value) will causing error because request:set_header only accept string. However, the error will not cause the overall script execution to stop, but only the callback on splash:on_request. This way, unless we inspect Splash log we will never know about this error.

I have discussed this issue with @pawelmhm and he mentioned a related PR: https://github.com/scrapinghub/splash/pull/451

I think we have 2 issues here:

  1. Should request:set_header() accept non-string (and convert non-string argument to string)? Or is it the responsibility of the users to make sure that they always pass string as arguments?
  2. Should Splash stop the overall execution of the Lua script when error happens on splash:on_request (and maybe splash:on_response too) and then send HTTP 400 to the client?
kmike commented 6 years ago

Should request:set_header() accept non-string (and convert non-string argument to string)? Or is it the responsibility of the users to make sure that they always pass string as arguments?

That's a tricky question. Currently, if we blindly convert anything to string, some errors may go unnoticed - now at least they should be visible in logs. I think it is safe to convert numbers to strings, but not other data types.

Should Splash stop the overall execution of the Lua script when error happens on splash:on_request (and maybe splash:on_response too) and then send HTTP 400 to the client?

This would be backwards incompatible, but probably it should. See https://github.com/scrapinghub/splash/issues/208.

pawelmhm commented 6 years ago

Other related PR is this: https://github.com/scrapinghub/splash/pull/644 It seems that intention of attached PR is validating header values and converting ints to strings

The problem is that when user makes mistake and tries to set header value to integer setting all headers might fail, and many users dont have access to splash logs. So there will be silent failure, something will not work as expected and user will not know why. So I think we should probably raise some error if value for header is not string. But this will probably be backwards incompatible behavior.