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

Stop setting HTTP_VERSION to SERVER_PROTOCOL #970

Closed
mattrobenolt opened this issue Nov 6, 2015 · 4 comments · Fixed by #1691
Closed

Stop setting HTTP_VERSION to SERVER_PROTOCOL #970

mattrobenolt opened this issue Nov 6, 2015 · 4 comments · Fixed by #1691
Assignees
@ioquatix
Milestone
v3.0.0

Comments

@mattrobenolt
Copy link
Contributor

I'm opening this to discuss the merits and reasonings for setting HTTP_VERSION inside of the different rack handlers.

See my "Notes" under #969 for a little bit of context.

tl;dr is in all of the handlers, rack does this little bit:

env[HTTP_VERSION] ||= env[SERVER_PROTOCOL]

This causes an environment variable to get set to for HTTP_VERSION, when according to the CGI spec, HTTP_* is explicitly reserved for client headers. This makes it confusing if something downstream is attempting to collect all client headers, this becomes ambiguous. Did the client sent a Version header? Or is this the Version header that Rack is declaring?

This behavior is wrong, but I'm not sure of the ramification of simply... stopping doing this. I can't seem to find things that reference this, except the internals that were fixed up in #969, but no idea how common this is downstream to actually use this value.
This was referenced Nov 6, 2015
Closed
Merged
@nateberkopec nateberkopec mentioned this issue Jan 27, 2017
Merged
@dannyfallon dannyfallon mentioned this issue Apr 19, 2018
Closed
@ioquatix
Copy link
Member

ioquatix commented May 21, 2019 •
edited
Loading

but no idea how common this is downstream to actually use this value.

I don't do this in falcon and I haven't had any complaints.

@jeremyevans
Copy link
Contributor

Rack is still setting HTTP_VERSION in the environment in all handlers. Unicorn sets it and ignores any Version request header submitted. I'm guessing there are some middleware that are relying on HTTP_VERSION instead of using SERVER_PROTOCOL. So this becomes a case of whether the backwards compatibility breakage is worth adding support for the Version request header. I don't have a strong opinion on that, but I would lean toward keeping backwards compatibility.

@ioquatix ioquatix self-assigned this Jan 17, 2020
@ioquatix ioquatix added this to the v2.2.0 milestone Jan 17, 2020
@ioquatix ioquatix modified the milestones: v2.2.0, v3.0.0 Feb 3, 2020
@ioquatix
Copy link
Member

ioquatix commented Feb 3, 2020

Moving this to v3.0.0 milestone.

@tenderlove
Copy link
Member

IMO it's fine to remove this for v3.0.0

jeremyevans added a commit to jeremyevans/rack that referenced this issue Jul 14, 2020
@jeremyevans
Remove Rack::HTTP_VERSION and don't set HTTP_VERSION in env in cgi an…
9794789 
…d webrick

Fixes rack#970
@jeremyevans jeremyevans mentioned this issue Jul 14, 2020
Merged
jeremyevans added a commit that referenced this issue Jul 14, 2020
@jeremyevans
Remove Rack::HTTP_VERSION and don't set HTTP_VERSION in env in cgi an…
5791ef6 
…d webrick

Fixes #970
This was referenced Mar 8, 2021
Open
Open
Open
Open
Open
Open
Open
Open
Merged
Open
Open
Open
This was referenced Mar 15, 2021
Open
Open
This was referenced Mar 15, 2021
Open
Closed
Open
Open
Open
Open
Open
Open
Open
Open
Open
Closed
Open
Open
Open
Open
Open
Closed
@amuta amuta mentioned this issue May 3, 2022
Closed
@lloeki lloeki mentioned this issue Jan 2, 2024
Closed
lloeki added a commit to lloeki/thin that referenced this issue Jan 2, 2024
@lloeki
Remove HTTP_VERSION
b4d7946 
This spec failed:

    Thin::Request parser should not fuck up on stupid fucked IE6 headers

with:

  should validate with Rack Lint: env[HTTP_VERSION] does not equal env[SERVER_PROTOCOL]

where:

  "HTTP_VERSION"=>"HTTP/1.0",
  "SERVER_PROTOCOL"=>"HTTP/1.1",

Indeed:

> Rack::HTTP_VERSION has been removed and the HTTP_VERSION env setting is no longer set in the CGI and Webrick handlers

See:

- rack/rack#970
- rack/rack#969
- rack/rack#1691

The version is necessary to negotiate `persistent?`, so here we punt on
a further refactoring to find a good way to get the request HTTP version
from the first line, instead shoving it in a Thin-specific
`thin.request_http_header`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ioquatix ioquatix

Labels
None yet
Projects
None yet
Milestone
v3.0.0
Development

Successfully merging a pull request may close this issue.

Remove Rack::HTTP_VERSION and don't set HTTP_VERSION in env in cgi and webrick jeremyevans/rack
4 participants
@tenderlove @jeremyevans @ioquatix @mattrobenolt