Permalink
Browse files

improved error messages

error message tests are kind of ugly to pass under all ruby versions and skipped under 1.8.6 where nice error messages aren't generated
  • Loading branch information...
1 parent b9e1fb2 commit 02c627e2bd62fb5b063daeb022533bee4e539f22 @matsadler committed with Dec 17, 2011
Showing with 221 additions and 30 deletions.
  1. +27 −8 lib/http_tools/parser.rb
  2. +118 −16 test/parser/request_test.rb
  3. +76 −6 test/parser/response_test.rb
@@ -297,7 +297,7 @@ def add_listener(event, proc=nil, &block)
alias on add_listener
def inspect # :nodoc:
- super.sub(/ .*>$/, " #{posstr} #{state}>")
+ super.sub(/ .*>$/, " #{posstr(false)} #{state}>")
end
private
@@ -313,6 +313,7 @@ def start
elsif @allow_html_without_header && @buffer.check(/\s*</i)
skip_header
else
+ @buffer.skip(/H(T(TP?)?)?/i) || @buffer.skip(/[a-z]+/i)
raise ParseError.new("Protocol or method not recognised at " + posstr)
end
end
@@ -328,6 +329,7 @@ def uri
elsif @buffer.check(/[a-z0-9;\/?:@&=+$,%_.!~*')(-]+\Z/i)
:uri
else
+ @buffer.skip(/[a-z0-9;\/?:@&=+$,%_.!~*')(-]+/i)
raise ParseError.new("URI or path not recognised at " + posstr)
end
end
@@ -343,6 +345,7 @@ def request_http_version
@buffer.check(/ (H(T(T(P(\/(\d+(\.(\d+\r?)?)?)?)?)?)?)?)?\Z/i)
:request_http_version
else
+ @buffer.skip(/ (H(T(T(P(\/(\d+(\.(\d+\r?)?)?)?)?)?)?)?)?/i)
raise ParseError.new("Invalid version specifier at " + posstr)
end
end
@@ -356,6 +359,7 @@ def response_http_version
@buffer.check(/H(T(T(P(\/(\d+(\.(\d+\r?)?)?)?)?)?)?)?\Z/i)
:response_http_version
else
+ @buffer.skip(/H(T(T(P(\/(\d+(\.(\d+\r?)?)?)?)?)?)?)?/i)
raise ParseError.new("Invalid version specifier at " + posstr)
end
end
@@ -379,6 +383,7 @@ def status
@buffer.check(/\d(\d(\d( ([^\x00-\x1f\x7f]+\r?)?)?)?)?\Z/i)
:status
else
+ @buffer.skip(/\d(\d(\d( ([^\x00-\x1f\x7f]+\r?)?)?)?)?/i)
raise ParseError.new("Invalid status line at " + posstr)
end
end
@@ -398,6 +403,7 @@ def key_or_newline
@last_key.chomp!(COLON)
value
elsif @request_method
+ @buffer.skip(/[ -9;-~]+/i)
raise ParseError.new("Illegal character in field name at " + posstr)
else
skip_bad_header
@@ -410,6 +416,7 @@ def skip_bad_header
elsif @buffer.check(/[^\x00\n\x7f]+\Z/)
:skip_bad_header
else
+ @buffer.skip(/[ -9;-~]+/i)
raise ParseError.new("Illegal character in field name at " + posstr)
end
end
@@ -432,6 +439,7 @@ def value
elsif @buffer.eos? || @buffer.check(/[^\x00\n\x7f]+\Z/i)
:value
else
+ @buffer.skip(/[^\x00\n\x7f]+/i)
raise ParseError.new("Illegal character in field body at " + posstr)
end
end
@@ -546,6 +554,7 @@ def trailer_key_or_newline
@last_key.chomp!(COLON)
trailer_value
else
+ @buffer.skip(/[ -9;-~]+/i)
raise ParseError.new("Illegal character in field name at " + posstr)
end
end
@@ -563,6 +572,7 @@ def trailer_value
elsif @buffer.eos? || @buffer.check(/[^\x00\n\x7f]+\Z/i)
:trailer_value
else
+ @buffer.skip(/[^\x00\n\x7f]+/i)
raise ParseError.new("Illegal character in field body at " + posstr)
end
end
@@ -605,19 +615,28 @@ def stream_callback(chunk)
end
def line_char(string, position)
- line_count = 1
+ line_count = 0
char_count = 0
string.each_line do |line|
break if line.length + char_count >= position
line_count += 1
char_count += line.length
end
- [line_count, position + 1 - char_count]
- end
-
- def posstr
- line, char = line_char(@buffer.string, @buffer.pos)
- "line #{line}, char #{char}"
+ [line_count, position - char_count]
+ end
+
+ def posstr(visual=true)
+ line_num, char_num = line_char(@buffer.string, @buffer.pos)
+ out = "line #{line_num + 1}, char #{char_num + 1}"
+ return out unless visual && @buffer.string.respond_to?(:lines)
+ line = ""
+ pointer = nil
+ @buffer.string.lines.to_a[line_num].chars.each_with_index.map do |char, i|
+ line << char.dump.gsub(/(^"|"$)/, "")
+ pointer = "#{" " * (line.length - 1)}^" if i == char_num
+ end
+ pointer ||= " " * line.length + "^"
+ [out, "", line, pointer].join("\n")
end
end
@@ -75,9 +75,17 @@ def test_complicated_path
def test_invalid_path
parser = HTTPTools::Parser.new
- assert_raise(HTTPTools::ParseError) do
+ error = assert_raise(HTTPTools::ParseError) do
parser << "GET \\ HTTP/1.1\r\n\r\n"
end
+
+ return unless "".respond_to?(:lines)
+ assert_equal(<<-MESSAGE.chomp, error.message)
+URI or path not recognised at line 1, char 5
+
+GET \\\\ HTTP/1.1\\r\\n
+ ^
+ MESSAGE
end
def test_uri
@@ -119,25 +127,49 @@ def test_uri_callback_called_with_path
def test_fragment_with_path
parser = HTTPTools::Parser.new
- assert_raise(HTTPTools::ParseError) do
+ error = assert_raise(HTTPTools::ParseError) do
parser << "GET /foo#bar HTTP/1.1\r\n\r\n"
end
+
+ return unless "".respond_to?(:lines)
+ assert_equal(<<-MESSAGE.chomp, error.message)
+URI or path not recognised at line 1, char 9
+
+GET /foo#bar HTTP/1.1\\r\\n
+ ^
+ MESSAGE
end
- def test_fragment_with_unfinished_path
+ def test_fragment_with_uri
parser = HTTPTools::Parser.new
error = assert_raise(HTTPTools::ParseError) do
- parser << "GET /foo#ba"
+ parser << "GET http://example.com/foo#bar HTTP/1.1\r\n\r\n"
end
+
+ return unless "".respond_to?(:lines)
+ assert_equal(<<-MESSAGE.chomp, error.message)
+URI or path not recognised at line 1, char 27
+
+GET http://example.com/foo#bar HTTP/1.1\\r\\n
+ ^
+ MESSAGE
end
- def test_fragment_with_uri
+ def test_fragment_with_unfinished_path
parser = HTTPTools::Parser.new
- assert_raise(HTTPTools::ParseError) do
- parser << "GET http://example.com/foo#bar HTTP/1.1\r\n\r\n"
+ error = assert_raise(HTTPTools::ParseError) do
+ parser << "GET /foo#ba"
end
+
+ return unless "".respond_to?(:lines)
+ assert_equal(<<-MESSAGE.chomp, error.message)
+URI or path not recognised at line 1, char 9
+
+GET /foo#ba
+ ^
+ MESSAGE
end
def test_with_header
@@ -294,7 +326,15 @@ def test_without_http
def test_unknown_protocol
parser = HTTPTools::Parser.new
- assert_raise(HTTPTools::ParseError) {parser << "GET / SPDY/1.1\r\n"}
+ error = assert_raise(HTTPTools::ParseError) {parser << "GET / SPDY/1.1\r\n"}
+
+ return unless "".respond_to?(:lines)
+ assert_equal(<<-MESSAGE.chomp, error.message)
+Invalid version specifier at line 1, char 7
+
+GET / SPDY/1.1\\r\\n
+ ^
+ MESSAGE
end
def test_protocol_version
@@ -311,7 +351,15 @@ def test_protocol_version
def test_protocol_without_version
parser = HTTPTools::Parser.new
- assert_raise(HTTPTools::ParseError) {parser << "GET / HTTP\r\n\r\n"}
+ error = assert_raise(HTTPTools::ParseError) {parser << "GET / HTTP\r\n\r\n"}
+
+ return unless "".respond_to?(:lines)
+ assert_equal(<<-MESSAGE.chomp, error.message)
+Invalid version specifier at line 1, char 11
+
+GET / HTTP\\r\\n
+ ^
+ MESSAGE
end
def test_one_dot_x_protocol_version
@@ -440,7 +488,7 @@ def test_rest_size
def test_not_a_http_request
parser = HTTPTools::Parser.new
- assert_raise(HTTPTools::ParseError) {parser << "not a http request"}
+ assert_raise(HTTPTools::ParseError) {parser << "not a http/1.1 request"}
end
def test_data_past_end
@@ -466,6 +514,23 @@ def test_lowecase_method
assert_equal("get", result)
end
+ def test_invalid_method
+ parser = HTTPTools::Parser.new
+
+ error = assert_raise(HTTPTools::ParseError) do
+ parser << "G3T / HTTP/1.1\r\n\r\n"
+ end
+
+ return unless "".respond_to?(:lines)
+ assert_equal(<<-MESSAGE.chomp, error.message)
+Protocol or method not recognised at line 1, char 2
+
+G3T / HTTP/1.1\\r\\n
+ ^
+ MESSAGE
+ end
+
+
def test_lowercase_http
parser = HTTPTools::Parser.new
version = nil
@@ -480,31 +545,68 @@ def test_lowercase_http
def test_invalid_version
parser = HTTPTools::Parser.new
- assert_raise(HTTPTools::ParseError) {parser << "GET / HTTP/one dot one\r\n"}
+ error = assert_raise(HTTPTools::ParseError) do
+ parser << "GET / HTTP/one dot one\r\n"
+ end
+
+ return unless "".respond_to?(:lines)
+ assert_equal(<<-MESSAGE.chomp, error.message)
+Invalid version specifier at line 1, char 12
+
+GET / HTTP/one dot one\\r\\n
+ ^
+ MESSAGE
end
def test_invalid_header_key_with_control_character
parser = HTTPTools::Parser.new
- assert_raise(HTTPTools::ParseError) do
+ error = assert_raise(HTTPTools::ParseError) do
parser << "GET / HTTP/1.1\r\nx-invalid\0key: text/plain\r\n"
end
+
+ return unless "".respond_to?(:lines)
+ null = "\000".dump.gsub(/"/, "")
+ assert_equal(<<-MESSAGE.chomp, error.message)
+Illegal character in field name at line 2, char 10
+
+x-invalid#{null}key: text/plain\\r\\n
+ ^
+ MESSAGE
end
def test_invalid_header_key_with_non_ascii_character
parser = HTTPTools::Parser.new
- assert_raise(HTTPTools::ParseError) do
- parser << "GET / HTTP/1.1\r\nx-invalid\u2014key: text/plain\r\n"
+ error = assert_raise(HTTPTools::ParseError) do
+ parser << "GET / HTTP/1.1\r\nx-invalid\342\200\224key: text/plain\r\n"
end
+
+ em_dash = "\342\200\224".dump.gsub(/"/, "")
+ return unless "".respond_to?(:lines)
+ assert_equal(<<-MESSAGE.chomp, error.message)
+Illegal character in field name at line 2, char 10
+
+x-invalid#{em_dash}key: text/plain\\r\\n
+ #{" " * (em_dash.length / 4)}^
+ MESSAGE
end
def test_invalid_header_value_with_non_control_character
parser = HTTPTools::Parser.new
- assert_raise(HTTPTools::ParseError) do
- parser << "GET / HTTP/1.1\r\nAccept: \000text/plain\r\n"
+ error = assert_raise(HTTPTools::ParseError) do
+ parser << "GET / HTTP/1.1\r\nAccept: text\000plain\r\n"
end
+
+ return unless "".respond_to?(:lines)
+ null = "\000".dump.gsub(/"/, "")
+ assert_equal(<<-MESSAGE.chomp, error.message)
+Illegal character in field body at line 2, char 13
+
+Accept: text#{null}plain\\r\\n
+ ^
+ MESSAGE
end
def test_error_callback
Oops, something went wrong.

0 comments on commit 02c627e

Please sign in to comment.