Don't redirect to arbitrary URLs via link tracker

app/controllers/clicks_controller.rb

@@ -7,15 +7,15 @@ class ClicksController < ApplicationController
 
     if params[:topic_id].present? || params[:post_id].present?
       params.merge!({ user_id: current_user.id }) if current_user.present?
-      TopicLinkClick.create_from(params)
+      @redirect_url = TopicLinkClick.create_from(params)
     end
 
     # Sometimes we want to record a link without a 302. Since XHR has to load the redirected
     # URL we want it to not return a 302 in those cases.
-    if params[:redirect] == 'false'
+    if params[:redirect] == 'false' || @redirect_url.blank?
       render nothing: true
     else
-      redirect_to(params[:url])
+      redirect_to(@redirect_url)
     end
   end
 

spec/controllers/clicks_controller_spec.rb

@@ -6,44 +6,53 @@ describe ClicksController do
 
     context 'missing params' do
       it 'raises an error without the url param' do
-	lambda { xhr :get, :track, post_id: 123 }.should raise_error(ActionController::ParameterMissing)
+        lambda { xhr :get, :track, post_id: 123 }.should raise_error(ActionController::ParameterMissing)
       end
 
       it "redirects to the url even without the topic_id or post_id params" do
         xhr :get, :track, url: 'http://google.com'
-        response.should redirect_to("http://google.com")
+        response.should_not be_redirect
       end
-
     end
 
     context 'correct params' do
+      let(:url) { "http://discourse.org" }
 
       before do
         request.stubs(:remote_ip).returns('192.168.0.1')
       end
 
+      context "with a made up url" do
+        it "doesn't redirect" do
+          TopicLinkClick.expects(:create_from).returns(nil)
+          xhr :get, :track, url: 'http://discourse.org', post_id: 123
+          response.should_not be_redirect
+        end
+
+      end
+
       context 'with a post_id' do
         it 'calls create_from' do
-	  TopicLinkClick.expects(:create_from).with('url' => 'http://discourse.org', 'post_id' => '123', 'ip' => '192.168.0.1')
+          TopicLinkClick.expects(:create_from).with('url' => url, 'post_id' => '123', 'ip' => '192.168.0.1').returns(url)
-          xhr :get, :track, url: 'http://discourse.org', post_id: 123
+          xhr :get, :track, url: url, post_id: 123
-          response.should redirect_to("http://discourse.org")
+          response.should redirect_to(url)
         end
 
         it 'redirects to the url' do
-          TopicLinkClick.stubs(:create_from)
+          TopicLinkClick.stubs(:create_from).returns(url)
-          xhr :get, :track, url: 'http://discourse.org', post_id: 123
+          xhr :get, :track, url: url, post_id: 123
-          response.should redirect_to("http://discourse.org")
+          response.should redirect_to(url)
         end
 
         it 'will pass the user_id to create_from' do
-	  TopicLinkClick.expects(:create_from).with('url' => 'http://discourse.org', 'post_id' => '123', 'ip' => '192.168.0.1')
+          TopicLinkClick.expects(:create_from).with('url' => url, 'post_id' => '123', 'ip' => '192.168.0.1').returns(url)
-          xhr :get, :track, url: 'http://discourse.org', post_id: 123
+          xhr :get, :track, url: url, post_id: 123
-          response.should redirect_to("http://discourse.org")
+          response.should redirect_to(url)
         end
 
         it "doesn't redirect with the redirect=false param" do
-	  TopicLinkClick.expects(:create_from).with('url' => 'http://discourse.org', 'post_id' => '123', 'ip' => '192.168.0.1', 'redirect' => 'false')
+          TopicLinkClick.expects(:create_from).with('url' => url, 'post_id' => '123', 'ip' => '192.168.0.1', 'redirect' => 'false').returns(url)
-          xhr :get, :track, url: 'http://discourse.org', post_id: 123, redirect: 'false'
+          xhr :get, :track, url: url, post_id: 123, redirect: 'false'
           response.should_not be_redirect
         end
 
@@ -51,9 +60,9 @@ describe ClicksController do
 
       context 'with a topic_id' do
         it 'calls create_from' do
-	  TopicLinkClick.expects(:create_from).with('url' => 'http://discourse.org', 'topic_id' => '789', 'ip' => '192.168.0.1')
+          TopicLinkClick.expects(:create_from).with('url' => url, 'topic_id' => '789', 'ip' => '192.168.0.1').returns(url)
-          xhr :get, :track, url: 'http://discourse.org', topic_id: 789
+          xhr :get, :track, url: url, topic_id: 789
-          response.should redirect_to("http://discourse.org")
+          response.should redirect_to(url)
         end
       end
 

spec/models/topic_link_click_spec.rb

@@ -5,7 +5,6 @@ describe TopicLinkClick do
 
   it { should belong_to :topic_link }
   it { should belong_to :user }
-
   it { should validate_presence_of :topic_link_id }
 
   def test_uri
@@ -47,8 +46,10 @@ describe TopicLinkClick do
     context 'create_from' do
 
       context 'without a url' do
-        it "doesn't raise an exception" do
+        let(:click) { TopicLinkClick.create_from(url: "url that doesn't exist", post_id: @post.id, ip: '127.0.0.1') }
-          TopicLinkClick.create_from(url: "url that doesn't exist", post_id: @post.id, ip: '127.0.0.1')
+
+        it "returns nil" do
+          click.should be_nil
         end
       end
 
@@ -57,13 +58,12 @@ describe TopicLinkClick do
           lambda {
             TopicLinkClick.create_from(url: @topic_link.url, post_id: @post.id, ip: '127.0.0.1', user_id: @post.user_id)
           }.should_not change(TopicLinkClick, :count)
-
         end
       end
 
       context 'with a valid url and post_id' do
         before do
-          TopicLinkClick.create_from(url: @topic_link.url, post_id: @post.id, ip: '127.0.0.1')
+          @url = TopicLinkClick.create_from(url: @topic_link.url, post_id: @post.id, ip: '127.0.0.1')
           @click = TopicLinkClick.last
         end
 
@@ -75,6 +75,10 @@ describe TopicLinkClick do
           @click.topic_link.should == @topic_link
         end
 
+        it "returns the url clicked on" do
+          @url.should == @topic_link.url
+        end
+
         context "clicking again" do
           it "should not record the click due to rate limiting" do
             -> { TopicLinkClick.create_from(url: @topic_link.url, post_id: @post.id, ip: '127.0.0.1') }.should_not change(TopicLinkClick, :count)
@@ -84,7 +88,7 @@ describe TopicLinkClick do
 
       context 'with a valid url and topic_id' do
         before do
-          TopicLinkClick.create_from(url: @topic_link.url, topic_id: @topic.id, ip: '127.0.0.1')
+          @url = TopicLinkClick.create_from(url: @topic_link.url, topic_id: @topic.id, ip: '127.0.0.1')
           @click = TopicLinkClick.last
         end
 
@@ -95,6 +99,10 @@ describe TopicLinkClick do
         it 'has the topic_link id' do
           @click.topic_link.should == @topic_link
         end
+
+        it "returns the url linked to" do
+          @url.should == @topic_link.url
+        end
       end