From b4d794625601a5b4368ed3222bf51849e9406c5c Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Tue, 2 Jan 2024 14:04:24 +0100 Subject: [PATCH] Remove HTTP_VERSION 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: - https://github.com/rack/rack/issues/970 - https://github.com/rack/rack/pull/969 - https://github.com/rack/rack/pull/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`. --- ext/thin_parser/common.rl | 2 +- ext/thin_parser/parser.c | 4 ++-- ext/thin_parser/parser.h | 2 +- ext/thin_parser/parser.rl | 6 +++--- ext/thin_parser/thin.c | 10 +++++----- lib/thin/request.rb | 4 ++-- spec/request/parser_spec.rb | 5 ++--- spec/request/persistent_spec.rb | 12 ++++++------ 8 files changed, 22 insertions(+), 23 deletions(-) diff --git a/ext/thin_parser/common.rl b/ext/thin_parser/common.rl index 08877036..ad9ba529 100644 --- a/ext/thin_parser/common.rl +++ b/ext/thin_parser/common.rl @@ -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; diff --git a/ext/thin_parser/parser.c b/ext/thin_parser/parser.c index 42832de4..7005d083 100644 --- a/ext/thin_parser/parser.c +++ b/ext/thin_parser/parser.c @@ -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; diff --git a/ext/thin_parser/parser.h b/ext/thin_parser/parser.h index f80d40f6..6a587e3a 100644 --- a/ext/thin_parser/parser.h +++ b/ext/thin_parser/parser.h @@ -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; diff --git a/ext/thin_parser/parser.rl b/ext/thin_parser/parser.rl index 05c8eb62..eb0db63c 100644 --- a/ext/thin_parser/parser.rl +++ b/ext/thin_parser/parser.rl @@ -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)); } } diff --git a/ext/thin_parser/thin.c b/ext/thin_parser/thin.c index b3e777cc..98a9fdfa 100644 --- a/ext/thin_parser/thin.c +++ b/ext/thin_parser/thin.c @@ -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; @@ -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 @@ -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); @@ -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"); diff --git a/lib/thin/request.rb b/lib/thin/request.rb index b5efe24b..4b3fe4df 100644 --- a/lib/thin/request.rb +++ b/lib/thin/request.rb @@ -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 @@ -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 diff --git a/spec/request/parser_spec.rb b/spec/request/parser_spec.rb index 2e11e2a0..7be5cac6 100644 --- a/spec/request/parser_spec.rb +++ b/spec/request/parser_spec.rb @@ -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') @@ -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") @@ -276,4 +275,4 @@ expect(request.env['REQUEST_URI']).to eq("/H%uFFFDhnchenbrustfilet") end -end \ No newline at end of file +end diff --git a/spec/request/persistent_spec.rb b/spec/request/persistent_spec.rb index 2419c082..f86a46d4 100644 --- a/spec/request/persistent_spec.rb +++ b/spec/request/persistent_spec.rb @@ -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 \ No newline at end of file +end