FIX: stop adding email to unsubscribe url

Instead of adding email to unsubscribe url store it in redis for 1 hour
rate limit calls to unsubscribe endpoint to ensure there is no risk of
bloating redis

Also move controller to request specs

app/controllers/email_controller.rb

@@ -41,6 +41,8 @@ class EmailController < ApplicationController
   end
 
   def perform_unsubscribe
+    RateLimiter.new(nil, "unsubscribe_#{request.ip}", 10, 1.minute).performed!
+
     key = UnsubscribeKey.find_by(key: params[:key])
     raise Discourse::NotFound unless key && key.user
 
@@ -99,19 +101,24 @@ class EmailController < ApplicationController
     unless updated
       redirect_back fallback_location: path("/")
     else
+
+      key = "unsub_#{SecureRandom.hex}"
+      $redis.setex key, 1.hour, user.email
+
+      url = path("/email/unsubscribed?key=#{key}")
       if topic
-        redirect_to path("/email/unsubscribed?topic_id=#{topic.id}&email=#{user.email}")
-      else
-        redirect_to path("/email/unsubscribed?email=#{user.email}")
+        url += "&topic_id=#{topic.id}"
       end
+
+      redirect_to url
     end
 
   end
 
   def unsubscribed
-    @email = params[:email]
+    @email = $redis.get(params[:key])
     @topic_id = params[:topic_id]
-    user = User.find_by_email(params[:email])
+    user = User.find_by_email(@email)
     raise Discourse::NotFound unless user
     topic = Topic.find_by(id: params[:topic_id].to_i) if @topic_id
     @topic = topic if topic && Guardian.new(nil).can_see?(topic)

spec/controllers/email_controller_spec.rb

@@ -20,135 +20,6 @@ describe EmailController do
 
   end
 
-  context '.perform unsubscribe' do
-    it 'raises not found on invalid key' do
-      post :perform_unsubscribe, params: { key: "123" }, format: :json
-      expect(response.status).to eq(404)
-    end
-
-    it 'can fully unsubscribe' do
-      user = Fabricate(:user)
-      key = UnsubscribeKey.create_key_for(user, "all")
-
-      user.user_option.update_columns(email_always: true,
-                                      email_digests: true,
-                                      email_direct: true,
-                                      email_private_messages: true)
-
-      post :perform_unsubscribe,
-        params: { key: key, unsubscribe_all: "1" },
-        format: :json
-
-      expect(response.status).to eq(302)
-
-      user.user_option.reload
-
-      expect(user.user_option.email_always).to eq(false)
-      expect(user.user_option.email_digests).to eq(false)
-      expect(user.user_option.email_direct).to eq(false)
-      expect(user.user_option.email_private_messages).to eq(false)
-
-    end
-
-    it 'can disable mailing list' do
-      user = Fabricate(:user)
-      key = UnsubscribeKey.create_key_for(user, "all")
-
-      user.user_option.update_columns(mailing_list_mode: true)
-
-      post :perform_unsubscribe,
-        params: { key: key, disable_mailing_list: "1" },
-        format: :json
-
-      expect(response.status).to eq(302)
-
-      user.user_option.reload
-
-      expect(user.user_option.mailing_list_mode).to eq(false)
-    end
-
-    it 'can disable digest' do
-      user = Fabricate(:user)
-      key = UnsubscribeKey.create_key_for(user, "all")
-
-      user.user_option.update_columns(email_digests: true)
-
-      post :perform_unsubscribe,
-        params: { key: key, disable_digest_emails: "1" },
-        format: :json
-
-      expect(response.status).to eq(302)
-
-      user.user_option.reload
-
-      expect(user.user_option.email_digests).to eq(false)
-    end
-
-    it 'can unwatch topic' do
-      p = Fabricate(:post)
-      key = UnsubscribeKey.create_key_for(p.user, p)
-
-      TopicUser.change(p.user_id, p.topic_id, notification_level: TopicUser.notification_levels[:watching])
-
-      post :perform_unsubscribe,
-        params: { key: key, unwatch_topic: "1" },
-        format: :json
-
-      expect(response.status).to eq(302)
-
-      expect(TopicUser.get(p.topic, p.user).notification_level).to eq(TopicUser.notification_levels[:tracking])
-    end
-
-    it 'can mute topic' do
-      p = Fabricate(:post)
-      key = UnsubscribeKey.create_key_for(p.user, p)
-
-      TopicUser.change(p.user_id, p.topic_id, notification_level: TopicUser.notification_levels[:watching])
-
-      post :perform_unsubscribe,
-        params: { key: key, mute_topic: "1" },
-        format: :json
-
-      expect(response.status).to eq(302)
-
-      expect(TopicUser.get(p.topic, p.user).notification_level).to eq(TopicUser.notification_levels[:muted])
-    end
-
-    it 'can unwatch category' do
-      p = Fabricate(:post)
-      key = UnsubscribeKey.create_key_for(p.user, p)
-
-      cu = CategoryUser.create!(user_id: p.user.id,
-                                category_id: p.topic.category_id,
-                                notification_level: CategoryUser.notification_levels[:watching])
-
-      post :perform_unsubscribe,
-        params: { key: key, unwatch_category: "1" },
-        format: :json
-
-      expect(response.status).to eq(302)
-
-      expect(CategoryUser.find_by(id: cu.id)).to eq(nil)
-    end
-
-    it 'can unwatch first post from category' do
-      p = Fabricate(:post)
-      key = UnsubscribeKey.create_key_for(p.user, p)
-
-      cu = CategoryUser.create!(user_id: p.user.id,
-                                category_id: p.topic.category_id,
-                                notification_level: CategoryUser.notification_levels[:watching_first_post])
-
-      post :perform_unsubscribe,
-        params: { key: key, unwatch_category: "1" },
-        format: :json
-
-      expect(response.status).to eq(302)
-
-      expect(CategoryUser.find_by(id: cu.id)).to eq(nil)
-    end
-  end
-
   context '.unsubscribe' do
 
     render_views

spec/requests/email_controller_spec.rb

@@ -5,6 +5,133 @@ RSpec.describe EmailController do
   let(:topic) { Fabricate(:topic) }
   let(:private_topic) { Fabricate(:private_message_topic) }
 
+  context '.perform unsubscribe' do
+    it 'raises not found on invalid key' do
+      post "/email/unsubscribe/123.json"
+      expect(response.status).to eq(404)
+    end
+
+    it 'can fully unsubscribe' do
+      user = Fabricate(:user)
+      key = UnsubscribeKey.create_key_for(user, "all")
+
+      user.user_option.update_columns(email_always: true,
+                                      email_digests: true,
+                                      email_direct: true,
+                                      email_private_messages: true)
+
+      post "/email/unsubscribe/#{key}.json",
+        params: { unsubscribe_all: "1" }
+
+      expect(response.status).to eq(302)
+
+      get response.redirect_url
+
+      # cause it worked ... yay
+      expect(body).to include(user.email)
+
+      user.user_option.reload
+
+      expect(user.user_option.email_always).to eq(false)
+      expect(user.user_option.email_digests).to eq(false)
+      expect(user.user_option.email_direct).to eq(false)
+      expect(user.user_option.email_private_messages).to eq(false)
+
+    end
+
+    it 'can disable mailing list' do
+      user = Fabricate(:user)
+      key = UnsubscribeKey.create_key_for(user, "all")
+
+      user.user_option.update_columns(mailing_list_mode: true)
+
+      post "/email/unsubscribe/#{key}.json",
+        params: { disable_mailing_list: "1" }
+
+      expect(response.status).to eq(302)
+
+      user.user_option.reload
+
+      expect(user.user_option.mailing_list_mode).to eq(false)
+    end
+
+    it 'can disable digest' do
+      user = Fabricate(:user)
+      key = UnsubscribeKey.create_key_for(user, "all")
+
+      user.user_option.update_columns(email_digests: true)
+
+      post "/email/unsubscribe/#{key}.json",
+        params: { disable_digest_emails: "1" }
+
+      expect(response.status).to eq(302)
+
+      user.user_option.reload
+
+      expect(user.user_option.email_digests).to eq(false)
+    end
+
+    it 'can unwatch topic' do
+      p = Fabricate(:post)
+      key = UnsubscribeKey.create_key_for(p.user, p)
+
+      TopicUser.change(p.user_id, p.topic_id, notification_level: TopicUser.notification_levels[:watching])
+
+      post "/email/unsubscribe/#{key}.json",
+        params: { unwatch_topic: "1" }
+
+      expect(response.status).to eq(302)
+
+      expect(TopicUser.get(p.topic, p.user).notification_level).to eq(TopicUser.notification_levels[:tracking])
+    end
+
+    it 'can mute topic' do
+      p = Fabricate(:post)
+      key = UnsubscribeKey.create_key_for(p.user, p)
+
+      TopicUser.change(p.user_id, p.topic_id, notification_level: TopicUser.notification_levels[:watching])
+
+      post "/email/unsubscribe/#{key}.json",
+        params: { mute_topic: "1" }
+
+      expect(response.status).to eq(302)
+
+      expect(TopicUser.get(p.topic, p.user).notification_level).to eq(TopicUser.notification_levels[:muted])
+    end
+
+    it 'can unwatch category' do
+      p = Fabricate(:post)
+      key = UnsubscribeKey.create_key_for(p.user, p)
+
+      cu = CategoryUser.create!(user_id: p.user.id,
+                                category_id: p.topic.category_id,
+                                notification_level: CategoryUser.notification_levels[:watching])
+
+      post "/email/unsubscribe/#{key}.json",
+        params: { unwatch_category: "1" }
+
+      expect(response.status).to eq(302)
+
+      expect(CategoryUser.find_by(id: cu.id)).to eq(nil)
+    end
+
+    it 'can unwatch first post from category' do
+      p = Fabricate(:post)
+      key = UnsubscribeKey.create_key_for(p.user, p)
+
+      cu = CategoryUser.create!(user_id: p.user.id,
+                                category_id: p.topic.category_id,
+                                notification_level: CategoryUser.notification_levels[:watching_first_post])
+
+      post "/email/unsubscribe/#{key}.json",
+        params: { unwatch_category: "1" }
+
+      expect(response.status).to eq(302)
+
+      expect(CategoryUser.find_by(id: cu.id)).to eq(nil)
+    end
+  end
+
   describe '#unsubscribed' do
     describe 'when email is invalid' do
       it 'should return the right response' do
@@ -15,7 +142,9 @@ RSpec.describe EmailController do
 
     describe 'when topic is public' do
       it 'should return the right response' do
-        get '/email/unsubscribed', params: { email: user.email, topic_id: topic.id }
+        key = SecureRandom.hex
+        $redis.set(key, user.email)
+        get '/email/unsubscribed', params: { key: key, topic_id: topic.id }
         expect(response).to be_success
         expect(response.body).to include(topic.title)
       end
@@ -23,7 +152,9 @@ RSpec.describe EmailController do
 
     describe 'when topic is private' do
       it 'should return the right response' do
-        get '/email/unsubscribed', params: { email: user.email, topic_id: private_topic.id }
+        key = SecureRandom.hex
+        $redis.set(key, user.email)
+        get '/email/unsubscribed', params: { key: key, topic_id: private_topic.id }
         expect(response).to be_success
         expect(response.body).to_not include(private_topic.title)
       end