grape icon indicating copy to clipboard operation
grape copied to clipboard

Grape allows invalid headers to be set.

Open ioquatix opened this issue 2 years ago • 5 comments

Follow up from https://github.com/socketry/protocol-rack/issues/2#issuecomment-1577206428.

The following implementation allows non-string header values.

https://github.com/ruby-grape/grape/blob/d1dfdccca242e00d4d88064a013f92e89a76dbd2/lib/grape/dsl/headers.rb#L13

Technically, all headers should be strings, according to the rack spec.

header "x-foo", 123

I'm not sure if we should change this. I see several options:

  • Emit a warning if it's not a string.
  • Convert values to a string.
  • Do nothing.
  • Later on, map @header key values to string values, (or array of string values, allowed by Rack 3+).

ioquatix avatar Jun 06 '23 02:06 ioquatix

For Grape users, is the real issue is that it works in Grape with Rack 2.x and not 3.x causing a NoMethodError: undefined method split' for 2:Integer`? If so, I think we should do nothing and document that upgrading to Rack 3 causes this. We could error early with "rack 3 doesn't support non-string values", too.

dblock avatar Jun 06 '23 15:06 dblock

Other breaking changes with Rack 3, #2298.

dblock avatar Jun 06 '23 15:06 dblock

No, it's nothing to do with Rack 3, it's always been invalid, even Rack 2 spec does not allow non-string header values.

ioquatix avatar Jun 06 '23 22:06 ioquatix

But it works for users today, except when otherwise (described in https://github.com/socketry/protocol-rack/issues/2). I think Grape should adhere to spec. I like the option of doing .to_s on header values, because it's backwards-compatible, but we should look at whether any specs break with that.

dblock avatar Jun 07 '23 17:06 dblock

If you put Rack::Lint in front of the apps (e.g. during testing/CI) it will fail.

I think you should encourage people to use Rack::Lint behind Grape in CI. It will report the problem.

ioquatix avatar Jun 07 '23 22:06 ioquatix