diff --git a/lib/oneboxer.rb b/lib/oneboxer.rb index a9bdabdddf5..3bf526a57f4 100644 --- a/lib/oneboxer.rb +++ b/lib/oneboxer.rb @@ -170,7 +170,7 @@ module Oneboxer end def self.onebox_raw(url, opts = {}) - url = URI(url).to_s + url = UrlHelper.escape_uri(url).to_s local_onebox(url, opts) || external_onebox(url) rescue => e # no point warning here, just cause we have an issue oneboxing a url @@ -191,7 +191,7 @@ module Oneboxer when "list" then local_category_html(url, route) end - html = html.presence || "#{url}" + html = html.presence || "#{URI(url).to_s}" { onebox: html, preview: html } end diff --git a/lib/url_helper.rb b/lib/url_helper.rb index 4b2ad9106c8..885c6ee7675 100644 --- a/lib/url_helper.rb +++ b/lib/url_helper.rb @@ -60,11 +60,9 @@ class UrlHelper self.absolute(Upload.secure_media_url_from_upload_url(url), nil) end - # Prevents double URL encode - # https://stackoverflow.com/a/37599235 def self.escape_uri(uri) return uri if s3_presigned_url?(uri) - UrlHelper.encode_component(CGI.unescapeHTML(UrlHelper.unencode(uri))) + Addressable::URI.normalized_encode(uri) end def self.rails_route_from_url(url) diff --git a/spec/components/final_destination_spec.rb b/spec/components/final_destination_spec.rb index ff61289e8af..585ca610976 100644 --- a/spec/components/final_destination_spec.rb +++ b/spec/components/final_destination_spec.rb @@ -463,7 +463,7 @@ describe FinalDestination do expect(fd(fragment_url).escape_url.to_s).to eq(fragment_url) expect(fd("https://eviltrout.com?s=180&d=mm&r=g").escape_url.to_s) - .to eq("https://eviltrout.com?s=180&d=mm&r=g") + .to eq("https://eviltrout.com?s=180&d=mm&%23038;r=g") expect(fd("http://example.com/?a=\11\15").escape_url.to_s).to eq("http://example.com/?a=%09%0D") diff --git a/spec/components/url_helper_spec.rb b/spec/components/url_helper_spec.rb index 8328cec9b96..0d8afc95981 100644 --- a/spec/components/url_helper_spec.rb +++ b/spec/components/url_helper_spec.rb @@ -114,11 +114,16 @@ describe UrlHelper do expect(url).to eq('http://example.com/%D9%85%D8%A7%D9%87%DB%8C') end - it "doesn't escape already escaped chars" do + it "doesn't escape already escaped chars (space)" do url = UrlHelper.escape_uri('http://example.com/foo%20bar/foo bar/') expect(url).to eq('http://example.com/foo%20bar/foo%20bar/') end + it "doesn't escape already escaped chars (hash)" do + url = UrlHelper.escape_uri('https://calendar.google.com/calendar/embed?src=en.uk%23holiday%40group.v.calendar.google.com&ctz=Europe%2FLondon') + expect(url).to eq('https://calendar.google.com/calendar/embed?src=en.uk%23holiday@group.v.calendar.google.com&ctz=Europe/London') + end + it "doesn't escape S3 presigned URLs" do # both of these were originally real presigned URLs and have had all # sensitive information stripped diff --git a/spec/models/topic_embed_spec.rb b/spec/models/topic_embed_spec.rb index d3dc1567b1a..9cf293a9c83 100644 --- a/spec/models/topic_embed_spec.rb +++ b/spec/models/topic_embed_spec.rb @@ -349,9 +349,9 @@ describe TopicEmbed do let(:invalid_url) { 'http://source.com/#double#anchor' } let(:contents) { "hello world new post hello" } - it "does not attempt absolutizing on a bad URI" do + it "handles badly formed URIs" do raw = TopicEmbed.absolutize_urls(invalid_url, contents) - expect(raw).to eq(contents) + expect(raw).to eq("hello world new post hello") end end