From 2e08cefa2a31283589373675f4bcd9ac15321735 Mon Sep 17 00:00:00 2001 From: Alex Bullen Date: Wed, 17 Apr 2019 16:19:32 -0700 Subject: [PATCH 1/5] Check to see if @normalized_* variables are already UTF-8 before calling force_encoding on them This prevents unnecessary mutation of these variables when already set and appropriately encoded. Necessary for our use case as we freeze our Addressable::URI objects to ensure they don't get changed when passed around --- lib/addressable/uri.rb | 38 ++++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/lib/addressable/uri.rb b/lib/addressable/uri.rb index 2550d9d1..60c07967 100644 --- a/lib/addressable/uri.rb +++ b/lib/addressable/uri.rb @@ -900,7 +900,9 @@ def normalized_scheme end end # All normalized values should be UTF-8 - @normalized_scheme.force_encoding(Encoding::UTF_8) if @normalized_scheme + if @normalized_scheme && @normalized_scheme.encoding != Encoding::UTF_8 + @normalized_scheme.force_encoding(Encoding::UTF_8) + end @normalized_scheme end @@ -955,7 +957,9 @@ def normalized_user end end # All normalized values should be UTF-8 - @normalized_user.force_encoding(Encoding::UTF_8) if @normalized_user + if @normalized_user && @normalized_user.encoding != Encoding::UTF_8 + @normalized_user.force_encoding(Encoding::UTF_8) + end @normalized_user end @@ -1012,7 +1016,9 @@ def normalized_password end end # All normalized values should be UTF-8 - if @normalized_password + if @normalized_password && ( + @normalized_password.encoding != Encoding::UTF_8 + ) @normalized_password.force_encoding(Encoding::UTF_8) end @normalized_password @@ -1082,7 +1088,9 @@ def normalized_userinfo end end # All normalized values should be UTF-8 - if @normalized_userinfo + if @normalized_userinfo && ( + @normalized_userinfo.encoding != Encoding::UTF_8 + ) @normalized_userinfo.force_encoding(Encoding::UTF_8) end @normalized_userinfo @@ -1151,7 +1159,7 @@ def normalized_host end end # All normalized values should be UTF-8 - if @normalized_host && !@normalized_host.empty? + if @normalized_host && @normalized_host.encoding != Encoding::UTF_8 @normalized_host.force_encoding(Encoding::UTF_8) end @normalized_host @@ -1271,7 +1279,9 @@ def normalized_authority authority end # All normalized values should be UTF-8 - if @normalized_authority + if @normalized_authority && ( + @normalized_authority.encoding != Encoding::UTF_8 + ) @normalized_authority.force_encoding(Encoding::UTF_8) end @normalized_authority @@ -1507,7 +1517,9 @@ def normalized_site site_string end # All normalized values should be UTF-8 - @normalized_site.force_encoding(Encoding::UTF_8) if @normalized_site + if @normalized_site && @normalized_site.encoding != Encoding::UTF_8 + @normalized_site.force_encoding(Encoding::UTF_8) + end @normalized_site end @@ -1570,7 +1582,9 @@ def normalized_path result end # All normalized values should be UTF-8 - @normalized_path.force_encoding(Encoding::UTF_8) if @normalized_path + if @normalized_path && @normalized_path.encoding != Encoding::UTF_8 + @normalized_path.force_encoding(Encoding::UTF_8) + end @normalized_path end @@ -1646,7 +1660,9 @@ def normalized_query(*flags) component == "" ? nil : component end # All normalized values should be UTF-8 - @normalized_query.force_encoding(Encoding::UTF_8) if @normalized_query + if @normalized_query && @normalized_query.encoding != Encoding::UTF_8 + @normalized_query.force_encoding(Encoding::UTF_8) + end @normalized_query end @@ -1842,7 +1858,9 @@ def normalized_fragment component == "" ? nil : component end # All normalized values should be UTF-8 - if @normalized_fragment + if @normalized_fragment && ( + @normalized_fragment.encoding != Encoding::UTF_8 + ) @normalized_fragment.force_encoding(Encoding::UTF_8) end @normalized_fragment From 52c524b677796409035f8717d1ab9fcbe9321e60 Mon Sep 17 00:00:00 2001 From: Alex Bullen Date: Wed, 24 Apr 2019 15:12:26 -0700 Subject: [PATCH 2/5] Implement tests to verify a deeply frozen URI doesn't mutate instance variables unnecessarily --- spec/addressable/uri_spec.rb | 66 ++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/spec/addressable/uri_spec.rb b/spec/addressable/uri_spec.rb index 00baaacf..9ab8038e 100644 --- a/spec/addressable/uri_spec.rb +++ b/spec/addressable/uri_spec.rb @@ -999,6 +999,72 @@ def to_s end end +describe Addressable::URI, "when normalized and then deeply frozen" do + before do + @uri = Addressable::URI.parse( + "http://user:password@example.com:8080/path?query=value#fragment" + ).normalize! + + @uri.instance_variables.each do |var| + @uri.instance_variable_set(var, @uri.instance_variable_get(var).freeze) + end + + @uri.freeze + end + + it "#normalized_scheme should not error" do + expect { @uri.normalized_scheme }.not_to raise_error + end + + it "#normalized_user should not error" do + expect { @uri.normalized_user }.not_to raise_error + end + + it "#normalized_password should not error" do + expect { @uri.normalized_password }.not_to raise_error + end + + it "#normalized_userinfo should not error" do + expect { @uri.normalized_userinfo }.not_to raise_error + end + + it "#normalized_host should not error" do + expect { @uri.normalized_host }.not_to raise_error + end + + it "#normalized_authority should not error" do + expect { @uri.normalized_authority }.not_to raise_error + end + + it "#normalized_port should not error" do + expect { @uri.normalized_port }.not_to raise_error + end + + it "#normalized_site should not error" do + expect { @uri.normalized_site }.not_to raise_error + end + + it "#normalized_path should not error" do + expect { @uri.normalized_path }.not_to raise_error + end + + it "#normalized_query should not error" do + expect { @uri.normalized_query }.not_to raise_error + end + + it "#normalized_fragment should not error" do + expect { @uri.normalized_fragment }.not_to raise_error + end + + it "should be frozen" do + expect(@uri).to be_frozen + end + + it "should not allow destructive operations" do + expect { @uri.normalize! }.to raise_error(FrozenError) + end +end + describe Addressable::URI, "when created from string components" do before do @uri = Addressable::URI.new( From 8194318fce539b4c58d37c57eaf903c3f8a76024 Mon Sep 17 00:00:00 2001 From: Philip Ross Date: Mon, 30 Aug 2021 18:51:09 -0700 Subject: [PATCH 3/5] move forcing utf8 encoding into a helper function --- lib/addressable/uri.rb | 58 +++++++++++++++--------------------------- 1 file changed, 20 insertions(+), 38 deletions(-) diff --git a/lib/addressable/uri.rb b/lib/addressable/uri.rb index 60c07967..4ebc6649 100644 --- a/lib/addressable/uri.rb +++ b/lib/addressable/uri.rb @@ -900,9 +900,7 @@ def normalized_scheme end end # All normalized values should be UTF-8 - if @normalized_scheme && @normalized_scheme.encoding != Encoding::UTF_8 - @normalized_scheme.force_encoding(Encoding::UTF_8) - end + force_utf8_encoding_if_needed(@normalized_scheme) @normalized_scheme end @@ -957,9 +955,7 @@ def normalized_user end end # All normalized values should be UTF-8 - if @normalized_user && @normalized_user.encoding != Encoding::UTF_8 - @normalized_user.force_encoding(Encoding::UTF_8) - end + force_utf8_encoding_if_needed(@normalized_user) @normalized_user end @@ -1016,11 +1012,7 @@ def normalized_password end end # All normalized values should be UTF-8 - if @normalized_password && ( - @normalized_password.encoding != Encoding::UTF_8 - ) - @normalized_password.force_encoding(Encoding::UTF_8) - end + force_utf8_encoding_if_needed(@normalized_password) @normalized_password end @@ -1088,11 +1080,7 @@ def normalized_userinfo end end # All normalized values should be UTF-8 - if @normalized_userinfo && ( - @normalized_userinfo.encoding != Encoding::UTF_8 - ) - @normalized_userinfo.force_encoding(Encoding::UTF_8) - end + force_utf8_encoding_if_needed(@normalized_userinfo) @normalized_userinfo end @@ -1159,9 +1147,7 @@ def normalized_host end end # All normalized values should be UTF-8 - if @normalized_host && @normalized_host.encoding != Encoding::UTF_8 - @normalized_host.force_encoding(Encoding::UTF_8) - end + force_utf8_encoding_if_needed(@normalized_host) @normalized_host end @@ -1279,11 +1265,7 @@ def normalized_authority authority end # All normalized values should be UTF-8 - if @normalized_authority && ( - @normalized_authority.encoding != Encoding::UTF_8 - ) - @normalized_authority.force_encoding(Encoding::UTF_8) - end + force_utf8_encoding_if_needed(@normalized_authority) @normalized_authority end @@ -1517,9 +1499,7 @@ def normalized_site site_string end # All normalized values should be UTF-8 - if @normalized_site && @normalized_site.encoding != Encoding::UTF_8 - @normalized_site.force_encoding(Encoding::UTF_8) - end + force_utf8_encoding_if_needed(@normalized_site) @normalized_site end @@ -1582,9 +1562,7 @@ def normalized_path result end # All normalized values should be UTF-8 - if @normalized_path && @normalized_path.encoding != Encoding::UTF_8 - @normalized_path.force_encoding(Encoding::UTF_8) - end + force_utf8_encoding_if_needed(@normalized_path) @normalized_path end @@ -1660,9 +1638,7 @@ def normalized_query(*flags) component == "" ? nil : component end # All normalized values should be UTF-8 - if @normalized_query && @normalized_query.encoding != Encoding::UTF_8 - @normalized_query.force_encoding(Encoding::UTF_8) - end + force_utf8_encoding_if_needed(@normalized_query) @normalized_query end @@ -1858,11 +1834,7 @@ def normalized_fragment component == "" ? nil : component end # All normalized values should be UTF-8 - if @normalized_fragment && ( - @normalized_fragment.encoding != Encoding::UTF_8 - ) - @normalized_fragment.force_encoding(Encoding::UTF_8) - end + force_utf8_encoding_if_needed(@normalized_fragment) @normalized_fragment end @@ -2570,5 +2542,15 @@ def remove_composite_values remove_instance_variable(:@uri_string) if defined?(@uri_string) remove_instance_variable(:@hash) if defined?(@hash) end + + ## + # Converts the string to be UTF-8 if it is not already UTF-8 + # + # @api private + def force_utf8_encoding_if_needed(str) + if str && str.encoding != Encoding::UTF_8 + str.force_encoding(Encoding::UTF_8) + end + end end end From 02c691271346b238d4bf8b584c85339052760580 Mon Sep 17 00:00:00 2001 From: Philip Ross Date: Tue, 31 Aug 2021 18:38:28 -0700 Subject: [PATCH 4/5] check against RuntimeError for Ruby < 2.5 --- spec/addressable/uri_spec.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/spec/addressable/uri_spec.rb b/spec/addressable/uri_spec.rb index 9ab8038e..f1121502 100644 --- a/spec/addressable/uri_spec.rb +++ b/spec/addressable/uri_spec.rb @@ -1061,7 +1061,9 @@ def to_s end it "should not allow destructive operations" do - expect { @uri.normalize! }.to raise_error(FrozenError) + ruby_check = Gem::Version.new(RUBY_VERSION) < Gem::Version.new("2.5.0") + error = ruby_check ? RuntimeError : FrozenError + expect { @uri.normalize! }.to raise_error(error) end end From b5e929bafa934df7a58e219e0d2bb7e2693a8c30 Mon Sep 17 00:00:00 2001 From: Philip Ross Date: Sun, 31 Jul 2022 19:06:59 -0700 Subject: [PATCH 5/5] update spec --- spec/addressable/uri_spec.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/spec/addressable/uri_spec.rb b/spec/addressable/uri_spec.rb index 4e3aced9..b1f95417 100644 --- a/spec/addressable/uri_spec.rb +++ b/spec/addressable/uri_spec.rb @@ -1060,9 +1060,7 @@ def to_s end it "should not allow destructive operations" do - ruby_check = Gem::Version.new(RUBY_VERSION) < Gem::Version.new("2.5.0") - error = ruby_check ? RuntimeError : FrozenError - expect { @uri.normalize! }.to raise_error(error) + expect { @uri.normalize! }.to raise_error(RuntimeError) end end