diff --git a/app/models/topic_link_click.rb b/app/models/topic_link_click.rb index 1f3302988c0..5dd8901fece 100644 --- a/app/models/topic_link_click.rb +++ b/app/models/topic_link_click.rb @@ -54,7 +54,16 @@ class TopicLinkClick < ActiveRecord::Base return nil unless uri # Only redirect to whitelisted hostnames - return WHITELISTED_REDIRECT_HOSTNAMES.include?(uri.hostname) ? url : nil + return url if WHITELISTED_REDIRECT_HOSTNAMES.include?(uri.hostname) + + if Discourse.asset_host.present? + cdn_uri = URI.parse(Discourse.asset_host) rescue nil + if cdn_uri + return url if cdn_uri.hostname == uri.hostname && uri.path.starts_with?(cdn_uri.path) + end + end + + return 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 2330ec3d28a..909e662883a 100644 --- a/spec/models/topic_link_click_spec.rb +++ b/spec/models/topic_link_click_spec.rb @@ -29,41 +29,35 @@ describe TopicLinkClick do it 'creates the forum topic link click' do expect(TopicLinkClick.count).to eq(1) - end - it 'has 0 clicks at first' do @topic_link.reload expect(@topic_link.clicks).to eq(1) - end - it 'serializes and deserializes the IP' do expect(TopicLinkClick.first.ip_address.to_s).to eq('192.168.1.1') end - end context 'create_from' do - context 'without a url' do - 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 "works correctly" do + + # returns nil to prevent exploits + 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) + + # redirects if whitelisted + 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") + + # does not change own link + expect { + TopicLinkClick.create_from(url: @topic_link.url, post_id: @post.id, ip: '127.0.0.0', user_id: @post.user_id) + }.not_to change(TopicLinkClick, :count) - 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 - context 'clicking on your own link' do - it "should not record the click" do - expect { - TopicLinkClick.create_from(url: @topic_link.url, post_id: @post.id, ip: '127.0.0.0', user_id: @post.user_id) - }.not_to change(TopicLinkClick, :count) - end - end + context 'with a valid url and post_id' do before do @@ -75,13 +69,11 @@ describe TopicLinkClick do expect(@click).to be_present expect(@click.topic_link).to eq(@topic_link) expect(@url).to eq(@topic_link.url) + + # second click should not record + expect { TopicLinkClick.create_from(url: @topic_link.url, post_id: @post.id, ip: '127.0.0.1') }.not_to change(TopicLinkClick, :count) end - context "clicking again" do - it "should not record the click due to rate limiting" do - expect { TopicLinkClick.create_from(url: @topic_link.url, post_id: @post.id, ip: '127.0.0.1') }.not_to change(TopicLinkClick, :count) - end - end end context "relative urls" do @@ -103,6 +95,37 @@ describe TopicLinkClick do redirect = TopicLinkClick.create_from(url: url) expect(redirect).to eq(url) end + + context "cdn links" do + + before do + Rails.configuration.action_controller.asset_host = "https://cdn.discourse.org/stuff" + end + + after do + Rails.configuration.action_controller.asset_host = nil + end + + it "correctly handles cdn links" do + + url = TopicLinkClick.create_from( + url: 'https://cdn.discourse.org/stuff/my_link', + topic_id: @topic.id, + ip: '127.0.0.3') + + expect(url).to eq('https://cdn.discourse.org/stuff/my_link') + + # cdn exploit + url = TopicLinkClick.create_from( + url: 'https://cdn.discourse.org/bad/my_link', + topic_id: @topic.id, + ip: '127.0.0.3') + + expect(url).to eq(nil) + end + + end + end context 'with a HTTPS version of the same URL' do