From 9bb5cf1c461a395005a530d886a2fcfc68339f37 Mon Sep 17 00:00:00 2001
From: Roman Rizzi <rizziromanalejandro@gmail.com>
Date: Wed, 30 Nov 2022 14:29:07 -0300
Subject: [PATCH] FIX: Validate unsubscribe key has an associated user (#19262)

* FIX: Validate unsubscribe key has an associated user

* Improve error messages
---
 app/controllers/email_controller.rb    |  5 +++--
 app/views/email/unsubscribe.html.erb   |  6 ++++--
 config/locales/server.en.yml           |  3 ++-
 spec/requests/email_controller_spec.rb | 10 ++++++++++
 4 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/app/controllers/email_controller.rb b/app/controllers/email_controller.rb
index de0c1cc2413..b7fd4c83806 100644
--- a/app/controllers/email_controller.rb
+++ b/app/controllers/email_controller.rb
@@ -6,10 +6,11 @@ class EmailController < ApplicationController
   skip_before_action :check_xhr, :preload_json, :redirect_to_login_if_required
 
   def unsubscribe
-    key = UnsubscribeKey.find_by(key: params[:key])
+    key = UnsubscribeKey.includes(:user).find_by(key: params[:key])
     @found = key.present?
+    @key_owner_found = key&.user.present?
 
-    if @found
+    if @found && @key_owner_found
       UnsubscribeKey
         .get_unsubscribe_strategy_for(key)
         &.prepare_unsubscribe_options(self)
diff --git a/app/views/email/unsubscribe.html.erb b/app/views/email/unsubscribe.html.erb
index 522b27d59a1..f36a5e497ee 100644
--- a/app/views/email/unsubscribe.html.erb
+++ b/app/views/email/unsubscribe.html.erb
@@ -1,14 +1,16 @@
 <div class='container unsubscribe'>
-  <%- if !@found || @different_user %>
+  <%- if !@found || !@key_owner_found || @different_user %>
 
     <%if !@found %>
       <p><%= t "unsubscribe.not_found_description" %></p>
+    <%- elsif !@key_owner_found %>
+      <p><%= t "unsubscribe.user_not_found_description" %></p>
     <%- else %>
       <p><%= t("unsubscribe.different_user_description").html_safe %></p>
       <%= form_tag(session_path(id: current_user.username_lower), method: :delete) do %>
         <%= hidden_field_tag(:return_url, @return_url) %>
         <%= submit_tag t('unsubscribe.log_out'), class: 'btn btn-danger' %>
-      <%- end%>
+      <%- end %>
     <%- end %>
   <%- else %>
     <br/>
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index fedb1ac76a2..8d29f5af5fe 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -1048,7 +1048,8 @@ en:
     mailing_list_mode: "Turn off mailing list mode"
     all: "Don’t send me any mail from %{sitename}"
     different_user_description: "You are currently logged in as a different user than the one we emailed. Please log out, or enter anonymous mode, and try again."
-    not_found_description: "Sorry, we couldn't find this unsubscribe. It’s possible the link in your email is too old and has expired?"
+    not_found_description: Sorry, we couldn't find that subscription. It's possible the link in your email is too old and has expired?"
+    user_not_found_description: "Sorry, we couldn't find a user for this subscription. You are probably attempting to unsubscribe an account that no longer exists."
     log_out: "Log Out"
     submit: "Save preferences"
     digest_frequency:
diff --git a/spec/requests/email_controller_spec.rb b/spec/requests/email_controller_spec.rb
index 051573e3609..6cdd86267a9 100644
--- a/spec/requests/email_controller_spec.rb
+++ b/spec/requests/email_controller_spec.rb
@@ -160,6 +160,16 @@ RSpec.describe EmailController do
     end
 
     fab!(:user) { Fabricate(:user) }
+
+    it "displays an error when the key has no associated user" do
+      key_without_owner = UnsubscribeKey.create_key_for(user, UnsubscribeKey::DIGEST_TYPE)
+      user.destroy!
+
+      navigate_to_unsubscribe(key_without_owner)
+
+      expect(response.body).to include(CGI.escapeHTML(I18n.t("unsubscribe.user_not_found_description")))
+    end
+
     let(:unsubscribe_key) { UnsubscribeKey.create_key_for(user, key_type, post: post) }
 
     context 'when unsubscribing from digest' do