From f1d83a5dabdb40d6636cc4db305c25474dba0639 Mon Sep 17 00:00:00 2001 From: Kevin Dew Date: Tue, 27 Sep 2022 20:00:43 +0100 Subject: [PATCH] Use Net::HTTPHeaderSyntaxError instead of ArgumentError This change proposes changing the exception class used when invalid syntax is used in a HTTP Header from ArgumentError to Net::HTTPHeaderSyntaxError. The reason I am proposing this change is to make it easier for applications and libraries to catch errors that result as part of HTTP response. For example, if you're calling: `Net::HTTP.get("www.example.com", "/")` it is confusing to receive an ArgumentError exception as this isn't a problem with the arguments provided to `Net::HTTP#get` and instead is a lower-level parsing concern of the work done by `Net::HTTP#get`. This exception isn't caught by common libraries that wrap Net::HTTP, for example Faraday: https://github.com/lostisland/faraday-net_http/blob/616bd1c35c058da01f91af8c4f504141b4f2b427/lib/faraday/adapter/net_http.rb#L15-L31 and is a somewhat delicate error to catch when making a request as a developer likely wants to distinguish between a genuine ArgumentError to usage of `Net::HTTP` and what is an error in the syntax in the HTTP request/response. There was existing precedence for the use of this exception in Net::HTTPHeader, so this seemed like a change that could made and make the class behave consistently. An example of usage is below. Before this change: ``` irb(main):001:0> require 'webmock'; include WebMock::API; WebMock.enable!; stub_request(:any, "www.example.com").to_return (headers: { "Invalid" => "Item with \r character" }) irb(main):002:0> Net::HTTP.get("www.example.com", "/") /Users/kevin.dew/dev/net-http/lib/net/http/header.rb:85:in `set_field': header field value cannot include CR/LF (ArgumentError) ``` After this change: ``` irb(main):001:0> require 'webmock'; include WebMock::API; WebMock.enable!; stub_request(:any, "www.example.com").to_return (headers: { "Invalid" => "Item with \r character" }) irb(main):002:0> Net::HTTP.get("www.example.com", "/") /Users/kevin.dew/dev/net-http/lib/net/http/header.rb:87:in `set_field': header field value cannot include CR/LF (Net::HTTPHeaderSyntaxError) => "Item with \r character" }) ``` --- lib/net/http/header.rb | 10 ++++++---- test/net/http/test_httpheader.rb | 12 ++++++------ 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/lib/net/http/header.rb b/lib/net/http/header.rb index b0ec4b06..ef95e6c6 100644 --- a/lib/net/http/header.rb +++ b/lib/net/http/header.rb @@ -20,10 +20,12 @@ def initialize_http_header(initheader) else value = value.strip # raise error for invalid byte sequences if value.count("\r\n") > 0 - raise ArgumentError, "header #{key} has field value #{value.inspect}, this cannot include CR/LF" + raise Net::HTTPHeaderSyntaxError, "header #{key} has field value #{value.inspect}, this cannot include CR/LF" end @header[key.downcase.to_s] = [value] end + rescue ArgumentError => e + raise Net::HTTPHeaderSyntaxError, e.message end end @@ -82,7 +84,7 @@ def add_field(key, val) else val = val.to_s # for compatibility use to_s instead of to_str if val.b.count("\r\n") > 0 - raise ArgumentError, 'header field value cannot include CR/LF' + raise Net::HTTPHeaderSyntaxError, 'header field value cannot include CR/LF' end @header[key.downcase.to_s] = [val] end @@ -95,7 +97,7 @@ def add_field(key, val) else val = val.to_s if /[\r\n]/n.match?(val.b) - raise ArgumentError, 'header field value cannot include CR/LF' + raise Net::HTTPHeaderSyntaxError, 'header field value cannot include CR/LF' end ary.push val end @@ -481,7 +483,7 @@ def set_form(params, enctype='application/x-www-form-urlencoded', formopt={}) /\Amultipart\/form-data\z/i self.content_type = enctype else - raise ArgumentError, "invalid enctype: #{enctype}" + raise Net::HTTPHeaderSyntaxError, "invalid enctype: #{enctype}" end end diff --git a/test/net/http/test_httpheader.rb b/test/net/http/test_httpheader.rb index b1ca9e82..4505877a 100644 --- a/test/net/http/test_httpheader.rb +++ b/test/net/http/test_httpheader.rb @@ -26,9 +26,9 @@ def test_initialize @c.initialize_http_header([["foo", "abc"], ["bar","xyz"]]) assert_equal "xyz", @c["bar"] assert_raise(NoMethodError){ @c.initialize_http_header("foo"=>[]) } - assert_raise(ArgumentError){ @c.initialize_http_header("foo"=>"a\nb") } - assert_raise(ArgumentError){ @c.initialize_http_header("foo"=>"a\rb") } - assert_raise(ArgumentError){ @c.initialize_http_header("foo"=>"a\xff") } + assert_raise(Net::HTTPHeaderSyntaxError){ @c.initialize_http_header("foo"=>"a\nb") } + assert_raise(Net::HTTPHeaderSyntaxError){ @c.initialize_http_header("foo"=>"a\rb") } + assert_raise(Net::HTTPHeaderSyntaxError){ @c.initialize_http_header("foo"=>"a\xff") } end def test_initialize_with_symbol @@ -68,8 +68,8 @@ def test_ASET @c['aaa'] = "aaa\xff" assert_equal 2, @c.length - assert_raise(ArgumentError){ @c['foo'] = "a\nb" } - assert_raise(ArgumentError){ @c['foo'] = ["a\nb"] } + assert_raise(Net::HTTPHeaderSyntaxError){ @c['foo'] = "a\nb" } + assert_raise(Net::HTTPHeaderSyntaxError){ @c['foo'] = ["a\nb"] } end def test_AREF @@ -95,7 +95,7 @@ def test_add_field @c.add_field 'My-Header', 'd, d' assert_equal 'a, b, c, d, d', @c['My-Header'] assert_equal ['a', 'b', 'c', 'd, d'], @c.get_fields('My-Header') - assert_raise(ArgumentError){ @c.add_field 'My-Header', "d\nd" } + assert_raise(Net::HTTPHeaderSyntaxError){ @c.add_field 'My-Header', "d\nd" } @c.add_field 'My-Header', ['e', ["\xff", 7]] assert_equal "a, b, c, d, d, e, \xff, 7", @c['My-Header'] assert_equal ['a', 'b', 'c', 'd, d', 'e', "\xff", '7'], @c.get_fields('My-Header')