From 9bac0b8b0d46db0facc5a4840b876e89a30b9794 Mon Sep 17 00:00:00 2001 From: Caspian Baska Date: Sun, 30 Oct 2022 20:18:27 +0800 Subject: [PATCH 1/6] Use `.in?` in more places --- src/base64.cr | 4 ++-- src/http/cookie.cr | 8 ++++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/base64.cr b/src/base64.cr index 2457768daa00..ddb783dfe49e 100644 --- a/src/base64.cr +++ b/src/base64.cr @@ -262,7 +262,7 @@ module Base64 break if bytes > fin # Move the pointer by one byte until there is a valid base64 character - while bytes.value == NL || bytes.value == NR + while bytes.value.in?(NL, NR) bytes += 1 end break if bytes > fin @@ -272,7 +272,7 @@ module Base64 end # Move the pointer by one byte until there is a valid base64 character or the end of `bytes` was reached - while (bytes < fin + 4) && (bytes.value == NL || bytes.value == NR) + while (bytes < fin + 4) && bytes.value.in?(NL, NR) bytes += 1 end diff --git a/src/http/cookie.cr b/src/http/cookie.cr index d22fdf1419d0..a987100fe353 100644 --- a/src/http/cookie.cr +++ b/src/http/cookie.cr @@ -58,7 +58,7 @@ module HTTP # valid characters for cookie-name per https://tools.ietf.org/html/rfc6265#section-4.1.1 # and https://tools.ietf.org/html/rfc2616#section-2.2 # "!#$%&'*+-.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ^_`abcdefghijklmnopqrstuvwxyz|~" - unless (0x21...0x7f).includes?(byte) && byte != 0x22 && byte != 0x28 && byte != 0x29 && byte != 0x2c && byte != 0x2f && !(0x3a..0x40).includes?(byte) && !(0x5b..0x5d).includes?(byte) && byte != 0x7b && byte != 0x7d + if !valid_cookie_value_byte?(byte) || byte.in?(0x3a..0x40, 0x5b..0x5d, 0x7b, 0x7d) raise IO::Error.new("Invalid cookie name") end end @@ -76,12 +76,16 @@ module HTTP value.each_byte do |byte| # valid characters for cookie-value per https://tools.ietf.org/html/rfc6265#section-4.1.1 # all printable ASCII characters except ' ', ',', '"', ';' and '\\' - unless (0x21...0x7f).includes?(byte) && byte != 0x22 && byte != 0x2c && byte != 0x3b && byte != 0x5c + unless valid_cookie_value_byte?(byte) raise IO::Error.new("Invalid cookie value") end end end + private def valid_cookie_value_byte?(byte) + byte.in?(0x21...0x7f) && !byte.in?(0x22, 0x2c, 0x3b, 0x5c) + end + def to_set_cookie_header : String path = @path expires = @expires From e6f2feb190796f5c04e9c7357404865ded55060c Mon Sep 17 00:00:00 2001 From: Caspian Baska Date: Mon, 7 Nov 2022 18:48:27 +0800 Subject: [PATCH 2/6] Add more extensive specs for `HTTP::Cookie#name=` --- spec/std/http/cookie_spec.cr | 60 +++++++++++++++--------------------- 1 file changed, 24 insertions(+), 36 deletions(-) diff --git a/spec/std/http/cookie_spec.cr b/spec/std/http/cookie_spec.cr index 28174021f908..a1a891e4c9a1 100644 --- a/spec/std/http/cookie_spec.cr +++ b/spec/std/http/cookie_spec.cr @@ -38,34 +38,33 @@ module HTTP expect_raises IO::Error, "Invalid cookie name" do HTTP::Cookie.new("\t", "") end - # more extensive specs on #name= end it "raises on invalid value" do expect_raises IO::Error, "Invalid cookie value" do HTTP::Cookie.new("x", %(foo\rbar)) end - # more extensive specs on #value= end end describe "#name=" do it "raises on invalid name" do cookie = HTTP::Cookie.new("x", "") - expect_raises IO::Error, "Invalid cookie name" do - cookie.name = "" - end - expect_raises IO::Error, "Invalid cookie name" do - cookie.name = "\t" - end - expect_raises IO::Error, "Invalid cookie name" do - cookie.name = "\r" - end - expect_raises IO::Error, "Invalid cookie name" do - cookie.name = "a\nb" - end - expect_raises IO::Error, "Invalid cookie name" do - cookie.name = "a\rb" + invalid_names = [ + '"', ',', ';', '\\', + ' ', '\r', '\t', '\n', + '{', '}', + (':'..'@').to_a, + ('['..']').to_a, + ].flatten.map { |c| "a#{c}b" } + + # name cannot be empty + invalid_names << "" + + invalid_names.each do |invalid_name| + expect_raises IO::Error, "Invalid cookie name" do + cookie.name = invalid_name + end end end end @@ -73,26 +72,15 @@ module HTTP describe "#value=" do it "raises on invalid value" do cookie = HTTP::Cookie.new("x", "") - expect_raises IO::Error, "Invalid cookie value" do - cookie.value = %(foo\rbar) - end - expect_raises IO::Error, "Invalid cookie value" do - cookie.value = %(foo"bar) - end - expect_raises IO::Error, "Invalid cookie value" do - cookie.value = "foo;bar" - end - expect_raises IO::Error, "Invalid cookie value" do - cookie.value = "foo\\bar" - end - expect_raises IO::Error, "Invalid cookie value" do - cookie.value = "foo\\bar" - end - expect_raises IO::Error, "Invalid cookie value" do - cookie.value = "foo bar" - end - expect_raises IO::Error, "Invalid cookie value" do - cookie.value = "foo,bar" + invalid_values = { + '"', ',', ';', '\\', # invalid printable ascii characters + ' ', '\r', '\t', '\n', # non-printable ascii characters + }.map { |c| "foo#{c}bar" } + + invalid_values.each do |invalid_value| + expect_raises IO::Error, "Invalid cookie value" do + cookie.value = invalid_value + end end end end From 9cc6bbb2deac5c836b399c0ac9934f683a76720d Mon Sep 17 00:00:00 2001 From: Caspian Baska Date: Mon, 7 Nov 2022 18:49:55 +0800 Subject: [PATCH 3/6] Fix `.in?` in `HTTP::Cookie#name=` --- src/http/cookie.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/http/cookie.cr b/src/http/cookie.cr index a987100fe353..ec69101bbb2d 100644 --- a/src/http/cookie.cr +++ b/src/http/cookie.cr @@ -58,7 +58,7 @@ module HTTP # valid characters for cookie-name per https://tools.ietf.org/html/rfc6265#section-4.1.1 # and https://tools.ietf.org/html/rfc2616#section-2.2 # "!#$%&'*+-.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ^_`abcdefghijklmnopqrstuvwxyz|~" - if !valid_cookie_value_byte?(byte) || byte.in?(0x3a..0x40, 0x5b..0x5d, 0x7b, 0x7d) + if !valid_cookie_value_byte?(byte) || byte.in?(0x3a..0x40) || byte.in?(0x5b..0x5d) || byte.in?(0x7b, 0x7d) raise IO::Error.new("Invalid cookie name") end end From c5ce7c5740d17fdfadd6effe61b90f4eb9b596bf Mon Sep 17 00:00:00 2001 From: Caspian Baska Date: Tue, 8 Nov 2022 22:39:46 +0700 Subject: [PATCH 4/6] Apply suggestions from review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Johannes Müller --- spec/std/http/cookie_spec.cr | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/std/http/cookie_spec.cr b/spec/std/http/cookie_spec.cr index a1a891e4c9a1..7e73a7a6ee51 100644 --- a/spec/std/http/cookie_spec.cr +++ b/spec/std/http/cookie_spec.cr @@ -54,9 +54,9 @@ module HTTP '"', ',', ';', '\\', ' ', '\r', '\t', '\n', '{', '}', - (':'..'@').to_a, - ('['..']').to_a, - ].flatten.map { |c| "a#{c}b" } + (':'..'@').each, + ('['..']').each, + ].flat_map { |c| "a#{c}b" } # name cannot be empty invalid_names << "" From d0d31c54be938279c55431cec43203f60d74c587 Mon Sep 17 00:00:00 2001 From: Caspian Baska Date: Wed, 9 Nov 2022 00:09:06 +0800 Subject: [PATCH 5/6] Remove redundant range check, return missing invalid chars --- src/http/cookie.cr | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/http/cookie.cr b/src/http/cookie.cr index ec69101bbb2d..9a4d972464a3 100644 --- a/src/http/cookie.cr +++ b/src/http/cookie.cr @@ -58,7 +58,11 @@ module HTTP # valid characters for cookie-name per https://tools.ietf.org/html/rfc6265#section-4.1.1 # and https://tools.ietf.org/html/rfc2616#section-2.2 # "!#$%&'*+-.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ^_`abcdefghijklmnopqrstuvwxyz|~" - if !valid_cookie_value_byte?(byte) || byte.in?(0x3a..0x40) || byte.in?(0x5b..0x5d) || byte.in?(0x7b, 0x7d) + if !byte.in?(0x21...0x7f) || # Non-printable ASCII character + byte.in?(0x22, 0x28, 0x29, 0x2c) || # '"', '(', ')', ',' + byte.in?(0x3a..0x40) || # ':', ';', '<', '=', '>', '?', '@' + byte.in?(0x5b..0x5d) || # '[', '\\', ']' + byte.in?(0x7b, 0x7d) # '{', '}' raise IO::Error.new("Invalid cookie name") end end @@ -76,16 +80,12 @@ module HTTP value.each_byte do |byte| # valid characters for cookie-value per https://tools.ietf.org/html/rfc6265#section-4.1.1 # all printable ASCII characters except ' ', ',', '"', ';' and '\\' - unless valid_cookie_value_byte?(byte) + if !byte.in?(0x21...0x7f) || byte.in?(0x22, 0x2c, 0x3b, 0x5c) raise IO::Error.new("Invalid cookie value") end end end - private def valid_cookie_value_byte?(byte) - byte.in?(0x21...0x7f) && !byte.in?(0x22, 0x2c, 0x3b, 0x5c) - end - def to_set_cookie_header : String path = @path expires = @expires From ed9a5572e2f0303c010ac44f497a350efc44f678 Mon Sep 17 00:00:00 2001 From: Caspian Baska Date: Thu, 10 Nov 2022 14:26:24 +0800 Subject: [PATCH 6/6] Another missing character :facepalm: --- spec/std/http/cookie_spec.cr | 2 +- src/http/cookie.cr | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/std/http/cookie_spec.cr b/spec/std/http/cookie_spec.cr index 7e73a7a6ee51..08d2e91c7b16 100644 --- a/spec/std/http/cookie_spec.cr +++ b/spec/std/http/cookie_spec.cr @@ -51,7 +51,7 @@ module HTTP it "raises on invalid name" do cookie = HTTP::Cookie.new("x", "") invalid_names = [ - '"', ',', ';', '\\', + '"', '(', ')', ',', '/', ' ', '\r', '\t', '\n', '{', '}', (':'..'@').each, diff --git a/src/http/cookie.cr b/src/http/cookie.cr index 9a4d972464a3..9dc5517fa05d 100644 --- a/src/http/cookie.cr +++ b/src/http/cookie.cr @@ -58,11 +58,11 @@ module HTTP # valid characters for cookie-name per https://tools.ietf.org/html/rfc6265#section-4.1.1 # and https://tools.ietf.org/html/rfc2616#section-2.2 # "!#$%&'*+-.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ^_`abcdefghijklmnopqrstuvwxyz|~" - if !byte.in?(0x21...0x7f) || # Non-printable ASCII character - byte.in?(0x22, 0x28, 0x29, 0x2c) || # '"', '(', ')', ',' - byte.in?(0x3a..0x40) || # ':', ';', '<', '=', '>', '?', '@' - byte.in?(0x5b..0x5d) || # '[', '\\', ']' - byte.in?(0x7b, 0x7d) # '{', '}' + if !byte.in?(0x21...0x7f) || # Non-printable ASCII character + byte.in?(0x22, 0x28, 0x29, 0x2c, 0x2f) || # '"', '(', ')', ',', '/' + byte.in?(0x3a..0x40) || # ':', ';', '<', '=', '>', '?', '@' + byte.in?(0x5b..0x5d) || # '[', '\\', ']' + byte.in?(0x7b, 0x7d) # '{', '}' raise IO::Error.new("Invalid cookie name") end end