Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Grape allows invalid headers to be set. #2334

Open
ioquatix opened this issue Jun 6, 2023 · 5 comments · May be fixed by #2511
Open

Grape allows invalid headers to be set. #2334

ioquatix opened this issue Jun 6, 2023 · 5 comments · May be fixed by #2511
Labels
bug?

Comments

@ioquatix
Copy link
Contributor

ioquatix commented Jun 6, 2023

Follow up from socketry/protocol-rack#2 (comment).

The following implementation allows non-string header values.

grape/lib/grape/dsl/headers.rb

Line 13 in d1dfdcc

val ? header[key.to_s] = val : header.delete(key.to_s)

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+).
@dblock
Copy link
Member

dblock commented Jun 6, 2023 •
edited
Loading

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 dblock added the bug? label Jun 6, 2023
@dblock
Copy link
Member

dblock commented Jun 6, 2023

Other breaking changes with Rack 3, #2298.

@ioquatix
Copy link
Contributor Author

ioquatix commented Jun 6, 2023

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.

@dblock
Copy link
Member

dblock commented Jun 7, 2023

But it works for users today, except when otherwise (described in socketry/protocol-rack#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.

@ioquatix
Copy link
Contributor Author

ioquatix commented Jun 7, 2023

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.

SlakrHakr added a commit to SlakrHakr/grape that referenced this issue Nov 4, 2024
@SlakrHakr
Fix Grape allowing invalid headers to be set
82bfb3b 
Fixes ruby-grape#2334

Ensure all header values are strings according to the Rack spec.

* Convert header values to strings using `to_s` in the `header` method in `lib/grape/dsl/headers.rb`.
* Emit a warning if the header value is not a string in the `header` method in `lib/grape/dsl/headers.rb`.
* Add tests in `spec/grape/dsl/headers_spec.rb` to verify that non-string header values are converted to strings and warnings are emitted.

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/ruby-grape/grape/issues/2334?shareId=XXXX-XXXX-XXXX-XXXX).
@SlakrHakr SlakrHakr linked a pull request Nov 4, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug?
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix Grape allowing invalid headers to be set SlakrHakr/grape
2 participants
@ioquatix @dblock