Skip to content

Commit

Permalink
Remove HTTP_VERSION
Browse files Browse the repository at this point in the history
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`.
  • Loading branch information
lloeki committed Jan 2, 2024
1 parent ab9ad02 commit b4d7946
Show file tree
Hide file tree
Showing 8 changed files with 22 additions and 23 deletions.
2 changes: 1 addition & 1 deletion ext/thin_parser/common.rl
Expand Up @@ -43,7 +43,7 @@
Method = ( upper | digit | safe ){1,20} >mark %request_method;

http_number = ( digit+ "." digit+ ) ;
HTTP_Version = ( "HTTP/" http_number ) >mark %http_version ;
HTTP_Version = ( "HTTP/" http_number ) >mark %request_http_version ;
Request_Line = ( Method " " Request_URI ("#" Fragment){0,1} " " HTTP_Version CRLF ) ;

field_name = ( token -- ":" )+ >start_field %write_field;
Expand Down
4 changes: 2 additions & 2 deletions ext/thin_parser/parser.c
Expand Up @@ -295,8 +295,8 @@ case 13:
tr17:
#line 59 "parser.rl"
{
if (parser->http_version != NULL) {
parser->http_version(parser->data, PTR_TO(mark), LEN(mark, p));
if (parser->request_http_version != NULL) {
parser->request_http_version(parser->data, PTR_TO(mark), LEN(mark, p));
}
}
goto st14;
Expand Down
2 changes: 1 addition & 1 deletion ext/thin_parser/parser.h
Expand Up @@ -33,7 +33,7 @@ typedef struct http_parser {
element_cb fragment;
element_cb request_path;
element_cb query_string;
element_cb http_version;
element_cb request_http_version;
element_cb header_done;

} http_parser;
Expand Down
6 changes: 3 additions & 3 deletions ext/thin_parser/parser.rl
Expand Up @@ -56,9 +56,9 @@
}
}

action http_version {
if (parser->http_version != NULL) {
parser->http_version(parser->data, PTR_TO(mark), LEN(mark, fpc));
action request_http_version {
if (parser->request_http_version != NULL) {
parser->request_http_version(parser->data, PTR_TO(mark), LEN(mark, fpc));
}
}

Expand Down
10 changes: 5 additions & 5 deletions ext/thin_parser/thin.c
Expand Up @@ -21,7 +21,7 @@ static VALUE global_request_method;
static VALUE global_request_uri;
static VALUE global_fragment;
static VALUE global_query_string;
static VALUE global_http_version;
static VALUE global_request_http_version;
static VALUE global_content_length;
static VALUE global_http_content_length;
static VALUE global_request_path;
Expand Down Expand Up @@ -150,11 +150,11 @@ static void query_string(void *data, const char *at, size_t length)
rb_hash_aset(req, global_query_string, val);
}

static void http_version(void *data, const char *at, size_t length)
static void request_http_version(void *data, const char *at, size_t length)
{
VALUE req = (VALUE)data;
VALUE val = rb_str_new(at, length);
rb_hash_aset(req, global_http_version, val);
rb_hash_aset(req, global_request_http_version, val);
}

/** Finalizes the request header to have a bunch of stuff that's
Expand Down Expand Up @@ -237,7 +237,7 @@ VALUE Thin_HttpParser_alloc(VALUE klass)
hp->fragment = fragment;
hp->request_path = request_path;
hp->query_string = query_string;
hp->http_version = http_version;
hp->request_http_version = request_http_version;
hp->header_done = header_done;
thin_http_parser_init(hp);

Expand Down Expand Up @@ -401,7 +401,7 @@ void Init_thin_parser()
DEF_GLOBAL(request_uri, "REQUEST_URI");
DEF_GLOBAL(fragment, "FRAGMENT");
DEF_GLOBAL(query_string, "QUERY_STRING");
DEF_GLOBAL(http_version, "HTTP_VERSION");
DEF_GLOBAL(request_http_version, "thin.request_http_version");
DEF_GLOBAL(request_path, "REQUEST_PATH");
DEF_GLOBAL(content_length, "CONTENT_LENGTH");
DEF_GLOBAL(http_content_length, "HTTP_CONTENT_LENGTH");
Expand Down
4 changes: 2 additions & 2 deletions lib/thin/request.rb
Expand Up @@ -22,7 +22,7 @@ class Request
SERVER_NAME = 'SERVER_NAME'.freeze
REQUEST_METHOD = 'REQUEST_METHOD'.freeze
LOCALHOST = 'localhost'.freeze
HTTP_VERSION = 'HTTP_VERSION'.freeze
REQUEST_HTTP_VERSION = 'thin.request_http_version'.freeze
HTTP_1_0 = 'HTTP/1.0'.freeze
REMOTE_ADDR = 'REMOTE_ADDR'.freeze
CONTENT_LENGTH = 'CONTENT_LENGTH'.freeze
Expand Down Expand Up @@ -114,7 +114,7 @@ def persistent?
# Clients and servers SHOULD NOT assume that a persistent connection
# is maintained for HTTP versions less than 1.1 unless it is explicitly
# signaled. (http://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html)
if @env[HTTP_VERSION] == HTTP_1_0
if @env[REQUEST_HTTP_VERSION] == HTTP_1_0
@env[CONNECTION] =~ KEEP_ALIVE_REGEXP

# HTTP/1.1 client intends to maintain a persistent connection unless
Expand Down
5 changes: 2 additions & 3 deletions spec/request/parser_spec.rb
Expand Up @@ -12,7 +12,7 @@
request = R("GET / HTTP/1.1\r\nHost: localhost\r\n\r\n")
expect(request.env['SERVER_PROTOCOL']).to eq('HTTP/1.1')
expect(request.env['REQUEST_PATH']).to eq('/')
expect(request.env['HTTP_VERSION']).to eq('HTTP/1.1')
expect(request.env['thin.request_http_version']).to eq('HTTP/1.1')
expect(request.env['REQUEST_URI']).to eq('/')
expect(request.env['GATEWAY_INTERFACE']).to eq('CGI/1.2')
expect(request.env['REQUEST_METHOD']).to eq('GET')
Expand Down Expand Up @@ -195,7 +195,6 @@
expect(request.env['PATH_INFO']).to eq('/hi')
expect(request.env['REQUEST_PATH']).to eq('/hi')
expect(request.env['REQUEST_URI']).to eq('/hi?qs')
expect(request.env['HTTP_VERSION']).to eq('HTTP/1.1')
expect(request.env['REQUEST_METHOD']).to eq('GET')
expect(request.env["rack.url_scheme"]).to eq('http')
expect(request.env['FRAGMENT'].to_s).to eq("f")
Expand Down Expand Up @@ -276,4 +275,4 @@

expect(request.env['REQUEST_URI']).to eq("/H%uFFFDhnchenbrustfilet")
end
end
end
12 changes: 6 additions & 6 deletions spec/request/persistent_spec.rb
Expand Up @@ -6,30 +6,30 @@
end

it "should not assume that a persistent connection is maintained for HTTP version 1.0" do
@request.env['HTTP_VERSION'] = 'HTTP/1.0'
@request.env['thin.request_http_version'] = 'HTTP/1.0'
expect(@request).not_to be_persistent
end

it "should assume that a persistent connection is maintained for HTTP version 1.0 when specified" do
@request.env['HTTP_VERSION'] = 'HTTP/1.0'
@request.env['thin.request_http_version'] = 'HTTP/1.0'
@request.env['HTTP_CONNECTION'] = 'Keep-Alive'
expect(@request).to be_persistent
end

it "should maintain a persistent connection for HTTP/1.1 client" do
@request.env['HTTP_VERSION'] = 'HTTP/1.1'
@request.env['thin.request_http_version'] = 'HTTP/1.1'
@request.env['HTTP_CONNECTION'] = 'Keep-Alive'
expect(@request).to be_persistent
end

it "should maintain a persistent connection for HTTP/1.1 client by default" do
@request.env['HTTP_VERSION'] = 'HTTP/1.1'
@request.env['thin.request_http_version'] = 'HTTP/1.1'
expect(@request).to be_persistent
end

it "should not maintain a persistent connection for HTTP/1.1 client when Connection header include close" do
@request.env['HTTP_VERSION'] = 'HTTP/1.1'
@request.env['thin.request_http_version'] = 'HTTP/1.1'
@request.env['HTTP_CONNECTION'] = 'close'
expect(@request).not_to be_persistent
end
end
end

0 comments on commit b4d7946

Please sign in to comment.