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.

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

Add test for invalid integer headers. #15

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

ioquatix
Copy link
Member

@ioquatix ioquatix commented Jun 3, 2023

When investigating socketry/protocol-rack#2 I found that puma will accept integer header values (and probably anything that has #to_s). This is a superset of the spec (non-string/non-array-of-string header values are rejected by Rack::Lint), but it has caused users of less strict (e.g. basically every server according to this test) to create apps that are incompatible with servers that follow the spec more strictly.

Possible outcomes:

  • We don't do anything. We might consider adding documentation about the situation, i.e. that servers may accept non-string header values, but it's not necessarily compatible across servers/middlewares.
  • We relax the Rack specification to convert header values to_s where it makes sense.
  • We make Puma (and other servers) more strict and reject non-string header values.

Being more strict will likely break people's applications. Therefore, my initial conclusion is to make falcon/rack-protocol more compatible... but if we decide a different outcome, I'll implement that.

cc @MSP-Greg @nateberkopec @jeremyevans @tenderlove

Types of Changes

  • New feature.

Contribution

@nateberkopec
Copy link

Header values "must respond to to_s"? 🤔

@ioquatix
Copy link
Member Author

ioquatix commented Jun 5, 2023

@nateberkopec I think that's the implicit contract supported by all servers thus far, but I'm not certain middlewares are expecting that. I just updated Falcon (actually protocol-rack to assume that "super-spec").

The actual spec is:

Header values must be either a String instance, or an Array of String instances, such that each String instance must not contain characters below 037.

I think it's okay the spec remains strict. But the reality is, if servers assume the value is according to the spec, they will not be compatible with apps which don't follow this spec (which appears to be at least a non-zero number of apps).

@ioquatix
Copy link
Member Author

ioquatix commented Jun 6, 2023

I was just thinking about this and was reminded that there was actually was a reported bug in Rack 3 / Rails / Puma, because Puma probably just implicitly just calls #to_s on header values, it was writing out array of strings. Basically somewhere in Puma is probably the following code:

socket.write("#{key}: #{value}")

But no validation is done on value, so any object value, e.g. a hash, array, etc, will go out as whatever value.to_s returns. In the case of the bug, set-cookie is often an array of strings now... so it was generating Set-Cookie: ["key=value", "key2=value2"] etc.

@dentarg
Copy link

dentarg commented Jun 20, 2023

I was just thinking about this and was reminded that there was actually was a reported bug in Rack 3 / Rails / Puma, because Puma probably just implicitly just calls #to_s on header values, it was writing out array of strings

Since Puma 6.0.0, it has been fixed: puma/puma@e2ef83b

@ioquatix
Copy link
Member Author

I ended up relaxing Falcon to handle this case.

@ioquatix ioquatix force-pushed the main branch 4 times, most recently from 1353780 to 9be2b9e Compare March 1, 2025 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants