diff --git a/app/models/topic_link_click.rb b/app/models/topic_link_click.rb index 1cbf444fce5..225d6904393 100644 --- a/app/models/topic_link_click.rb +++ b/app/models/topic_link_click.rb @@ -13,6 +13,8 @@ class TopicLinkClick < ActiveRecord::Base validates_presence_of :topic_link_id validates_presence_of :ip_address + WHITELISTED_REDIRECT_HOSTNAMES = Set.new(%W{www.youtube.com youtu.be}) + # Create a click from a URL and post_id def self.create_from(args={}) url = args[:url] @@ -52,7 +54,10 @@ class TopicLinkClick < ActiveRecord::Base # If we have it somewhere else on the site, just allow the redirect. # This is likely due to a onebox of another topic. link = TopicLink.find_by(url: url) - return link.present? ? link.url : url + return link.url if link.present? + + # Only redirect to whitelisted hostnames + return WHITELISTED_REDIRECT_HOSTNAMES.include?(uri.hostname) ? url : nil end return url if args[:user_id] && link.user_id == args[:user_id] diff --git a/spec/models/topic_link_click_spec.rb b/spec/models/topic_link_click_spec.rb index c698e5f2f38..2330ec3d28a 100644 --- a/spec/models/topic_link_click_spec.rb +++ b/spec/models/topic_link_click_spec.rb @@ -45,10 +45,15 @@ describe TopicLinkClick do context 'create_from' do context 'without a url' do - let(:click) { TopicLinkClick.create_from(url: "url that doesn't exist", post_id: @post.id, ip: '127.0.0.1') } - it "returns the url" do - expect(click).to eq("url that doesn't exist") + it "returns nil to prevent redirect exploit" do + click = TopicLinkClick.create_from(url: "http://url-that-doesnt-exist.com", post_id: @post.id, ip: '127.0.0.1') + expect(click).to eq(nil) + end + + it "redirect to whitelisted hostnames" do + click = TopicLinkClick.create_from(url: "https://www.youtube.com/watch?v=jYd_5aggzd4", post_id: @post.id, ip: '127.0.0.1') + expect(click).to eq("https://www.youtube.com/watch?v=jYd_5aggzd4") end end