confirm_email_change: Use redirect-to-POST trick.

Just like with signup confirmation links, we shouldn't trigger email
change based on a GET to the confirmation URL - POST should be required.

So upon GET of the confirmation link, we serve a form which will
immediately be POSTed by JS code to finalize the email change.

confirmation/models.py

@@ -264,7 +264,7 @@ _properties = {
     Confirmation.INVITATION: ConfirmationType(
         "get_prereg_key_and_redirect", validity_in_days=settings.INVITATION_LINK_VALIDITY_DAYS
     ),
-    Confirmation.EMAIL_CHANGE: ConfirmationType("confirm_email_change"),
+    Confirmation.EMAIL_CHANGE: ConfirmationType("confirm_email_change_get"),
     Confirmation.UNSUBSCRIBE: ConfirmationType(
         "unsubscribe",
         validity_in_days=1000000,  # should never expire

templates/confirmation/confirm_preregistrationuser.html

@@ -1,5 +1,5 @@
 {% extends "zerver/base.html" %}
-{% set entrypoint = "confirm-preregistrationuser" %}
+{% set entrypoint = "redirect-to-post" %}
 
 {% block title %}
 <title>{{ _("Confirming your email address") }} | Zulip</title>
@@ -13,7 +13,7 @@ requisite context to make a useful signup form. Therefore, we immediately
 post to another view which executes in our code to produce the desired form.
 #}
 
-<form id="register" action="{{ registration_url }}" method="post">
+<form id="register" class="redirect-to-post-form"  action="{{ registration_url }}" method="post">
     {{ csrf_input }}
     <input type="hidden" value="{{ key }}" name="key"/>
     <input type="hidden" value="1" name="from_confirmation"/>

templates/confirmation/redirect_to_post.html

@@ -0,0 +1,28 @@
+{% extends "zerver/base.html" %}
+{% set entrypoint = "redirect-to-post" %}
+
+{% block title %}
+<title>{{ _("Confirming your email address") }} | Zulip</title>
+{% endblock %}
+
+{% block content %}
+
+{#
+The purpose of this is to be an intermediate page, served upon GET requests
+to confirmation links. We simply serve a form which combined with some automatically
+executed JavaScript code will immediately POST the confirmation key to the intended
+endpoint.
+
+This allows us to avoid triggering the action which is being confirmed via a mere
+GET request.
+
+This largely duplicates functionality and code with confirm_preregistrationuser.html.
+We should find a way to to unify these.
+#}
+
+<form id="redirect-to-post-form" class="redirect-to-post-form"  action="{{ target_url }}" method="post">
+    {{ csrf_input }}
+    <input type="hidden" value="{{ key }}" name="key"/>
+</form>
+
+{% endblock %}

web/src/portico/confirm-preregistrationuser.ts

@@ -1,5 +0,0 @@
-import $ from "jquery";
-
-$(() => {
-    $("#register").trigger("submit");
-});

web/src/portico/redirect-to-post.ts

@@ -0,0 +1,5 @@
+import $ from "jquery";
+
+$(() => {
+    $(".redirect-to-post-form").trigger("submit");
+});

web/webpack.assets.json

@@ -98,10 +98,10 @@
     ],
     "signup": ["./src/bundles/portico.ts", "jquery-validation", "./src/portico/signup.ts"],
     "register": ["./src/bundles/portico.ts", "jquery-validation", "./src/portico/signup.ts"],
-    "confirm-preregistrationuser": [
+    "redirect-to-post": [
         "./third/bootstrap/css/bootstrap.portico.css",
         "./src/bundles/common.ts",
-        "./src/portico/confirm-preregistrationuser.ts"
+        "./src/portico/redirect-to-post.ts"
     ],
     "support": [
         "./third/bootstrap/css/bootstrap.portico.css",

zerver/tests/test_email_change.py

@@ -1,5 +1,6 @@
 from datetime import timedelta
 from email.headerregistry import Address
+from typing import Any
 
 import orjson
 import time_machine
@@ -46,11 +47,16 @@ class EmailChangeTestCase(ZulipTestCase):
         activation_url = [s for s in body.split("\n") if s][2]
         return activation_url
 
+    def use_email_change_confirmation_link(self, url: str, follow: bool = False) -> Any:
+        key = url.split("/")[-1]
+        response = self.client_post("/accounts/confirm_new_email/", {"key": key}, follow=follow)
+        return response
+
     def test_confirm_email_change_with_non_existent_key(self) -> None:
         self.login("hamlet")
         key = generate_key()
         url = confirmation_url(key, None, Confirmation.EMAIL_CHANGE)
-        response = self.client_get(url)
+        response = self.use_email_change_confirmation_link(url)
         self.assertEqual(response.status_code, 404)
         self.assert_in_response(
             "Whoops. We couldn't find your confirmation link in the system.", response
@@ -60,7 +66,7 @@ class EmailChangeTestCase(ZulipTestCase):
         self.login("hamlet")
         key = "invalid_key"
         url = confirmation_url(key, None, Confirmation.EMAIL_CHANGE)
-        response = self.client_get(url)
+        response = self.use_email_change_confirmation_link(url)
         self.assertEqual(response.status_code, 404)
         self.assert_in_response("Whoops. The confirmation link is malformed.", response)
 
@@ -79,7 +85,7 @@ class EmailChangeTestCase(ZulipTestCase):
         with time_machine.travel(date_sent, tick=False):
             url = create_confirmation_link(obj, Confirmation.EMAIL_CHANGE)
 
-        response = self.client_get(url)
+        response = self.use_email_change_confirmation_link(url)
         self.assertEqual(response.status_code, 404)
         self.assert_in_response("The confirmation link has expired or been deactivated.", response)
 
@@ -101,6 +107,10 @@ class EmailChangeTestCase(ZulipTestCase):
         response = self.client_get(url)
 
         self.assertEqual(response.status_code, 200)
+        self.assert_in_success_response(["Confirming your email address"], response)
+
+        key = url.split("/")[-1]
+        response = self.client_post("/accounts/confirm_new_email/", {"key": key})
         self.assert_in_success_response(
             [
                 "This confirms that the email address for your Zulip",
@@ -119,13 +129,13 @@ class EmailChangeTestCase(ZulipTestCase):
         self.login_user(user_profile)
 
         activation_url = self.generate_email_change_link(new_email)
-        response = self.client_get(activation_url)
+        response = self.use_email_change_confirmation_link(activation_url)
         self.assertEqual(response.status_code, 200)
 
         user_profile.refresh_from_db()
         self.assertEqual(user_profile.delivery_email, new_email)
 
-        response = self.client_get(activation_url)
+        response = self.use_email_change_confirmation_link(activation_url)
         self.assertEqual(response.status_code, 404)
 
     def test_change_email_revokes(self) -> None:
@@ -142,7 +152,7 @@ class EmailChangeTestCase(ZulipTestCase):
         user_profile.refresh_from_db()
         self.assertEqual(user_profile.delivery_email, old_email)
 
-        response = self.client_get(second_url)
+        response = self.use_email_change_confirmation_link(second_url)
         self.assertEqual(response.status_code, 200)
         user_profile.refresh_from_db()
         self.assertEqual(user_profile.delivery_email, second_email)
@@ -155,7 +165,7 @@ class EmailChangeTestCase(ZulipTestCase):
         activation_url = self.generate_email_change_link(new_email)
 
         do_deactivate_user(user_profile, acting_user=None)
-        response = self.client_get(activation_url)
+        response = self.use_email_change_confirmation_link(activation_url)
         self.assertEqual(response.status_code, 401)
         error_page_title = "<title>Account is deactivated | Zulip</title>"
         self.assert_in_response(error_page_title, response)
@@ -171,7 +181,7 @@ class EmailChangeTestCase(ZulipTestCase):
             email_owners=False,
         )
 
-        response = self.client_get(activation_url)
+        response = self.use_email_change_confirmation_link(activation_url)
         self.assertEqual(response.status_code, 302)
         self.assertTrue(response["Location"].endswith("/accounts/deactivated/"))
 
@@ -204,7 +214,7 @@ class EmailChangeTestCase(ZulipTestCase):
         self.assertEqual(email_message.extra_headers["List-Id"], "Zulip Dev <zulip.testserver>")
 
         activation_url = [s for s in body.split("\n") if s][2]
-        response = self.client_get(activation_url)
+        response = self.use_email_change_confirmation_link(activation_url)
 
         self.assert_in_success_response(["This confirms that the email address"], response)
 
@@ -256,14 +266,14 @@ class EmailChangeTestCase(ZulipTestCase):
 
         self.login_user(cordelia)
         cordelia_url = self.generate_email_change_link(conflict_email)
-        response = self.client_get(cordelia_url)
+        response = self.use_email_change_confirmation_link(cordelia_url)
         self.assertEqual(response.status_code, 200)
         cordelia.refresh_from_db()
         self.assertEqual(cordelia.delivery_email, conflict_email)
 
         self.logout()
         self.login_user(hamlet)
-        response = self.client_get(hamlet_url)
+        response = self.use_email_change_confirmation_link(hamlet_url)
         self.assertEqual(response.status_code, 400)
         self.assert_in_response("Already has an account", response)
 
@@ -284,7 +294,7 @@ class EmailChangeTestCase(ZulipTestCase):
         realm.disallow_disposable_email_addresses = True
         realm.save()
 
-        response = self.client_get(confirmation_url)
+        response = self.use_email_change_confirmation_link(confirmation_url)
         self.assertEqual(response.status_code, 400)
         self.assert_in_response("Please use your real email address.", response)
 
@@ -302,7 +312,7 @@ class EmailChangeTestCase(ZulipTestCase):
             acting_user=None,
         )
 
-        response = self.client_get(activation_url)
+        response = self.use_email_change_confirmation_link(activation_url)
 
         self.assertEqual(response.status_code, 400)
         self.assert_in_response(
@@ -346,7 +356,7 @@ class EmailChangeTestCase(ZulipTestCase):
             realm=user_profile.realm,
         )
         url = create_confirmation_link(obj, Confirmation.EMAIL_CHANGE)
-        response = self.client_get(url)
+        response = self.use_email_change_confirmation_link(url)
 
         self.assertEqual(response.status_code, 200)
         self.assert_in_success_response(
@@ -399,7 +409,7 @@ class EmailChangeTestCase(ZulipTestCase):
         self.assertEqual(email_message.extra_headers["List-Id"], "Zulip Dev <zulip.testserver>")
 
         confirmation_url = [s for s in body.split("\n") if s][2]
-        response = self.client_get(confirmation_url, follow=True)
+        response = self.use_email_change_confirmation_link(confirmation_url, follow=True)
         self.assertEqual(response.status_code, 200)
         self.assert_in_success_response(["Set a new password"], response)
 

zerver/tests/test_signup.py

@@ -1643,7 +1643,7 @@ class RealmCreationTest(ZulipTestCase):
         result = self.client_get(confirmation_url)
         self.assertEqual(result.status_code, 200)
 
-        # Simulate the initial POST that is made by confirm-preregistration.js
+        # Simulate the initial POST that is made by redirect-to-post.ts
         # by triggering submit on confirm_preregistration.html.
         payload = {
             "full_name": "",

zerver/views/development/email_log.py

@@ -8,7 +8,7 @@ from django.http import HttpRequest, HttpResponse
 from django.shortcuts import redirect, render
 from django.views.decorators.http import require_safe
 
-from confirmation.models import Confirmation, confirmation_url
+from confirmation.models import Confirmation
 from zerver.actions.realm_settings import do_send_realm_reactivation_email
 from zerver.actions.user_settings import do_change_user_delivery_email
 from zerver.actions.users import change_user_is_active
@@ -129,9 +129,8 @@ def generate_all_emails(request: HttpRequest) -> HttpResponse:
 
     # Email change successful
     key = Confirmation.objects.filter(type=Confirmation.EMAIL_CHANGE).latest("id").confirmation_key
-    url = confirmation_url(key, realm, Confirmation.EMAIL_CHANGE)
     user_profile = get_user_by_delivery_email(registered_email, realm)
-    result = client.get(url)
+    result = client.post("/accounts/confirm_new_email/", {"key": key})
     assert result.status_code == 200
 
     # Reset the email value so we can run this again

zerver/views/user_settings.py

@@ -9,6 +9,7 @@ from django.core.files.uploadedfile import UploadedFile
 from django.db import transaction
 from django.http import HttpRequest, HttpResponse, HttpResponseRedirect
 from django.shortcuts import render
+from django.urls import reverse
 from django.utils.html import escape
 from django.utils.safestring import SafeString
 from django.utils.translation import gettext as _
@@ -32,7 +33,7 @@ from zerver.actions.user_settings import (
     do_start_email_change_process,
 )
 from zerver.actions.users import generate_password_reset_url
-from zerver.decorator import human_users_only
+from zerver.decorator import human_users_only, require_post
 from zerver.lib.avatar import avatar_url
 from zerver.lib.email_notifications import enqueue_welcome_emails
 from zerver.lib.email_validation import (
@@ -87,11 +88,29 @@ def validate_email_change_request(user_profile: UserProfile, new_email: str) ->
         raise JsonableError(e.message)
 
 
-def confirm_email_change(request: HttpRequest, confirmation_key: str) -> HttpResponse:
+def confirm_email_change_get(request: HttpRequest, confirmation_key: str) -> HttpResponse:
+    try:
+        get_object_from_key(confirmation_key, [Confirmation.EMAIL_CHANGE], mark_object_used=False)
+    except ConfirmationKeyError as exception:  # nocoverage
+        return render_confirmation_key_error(request, exception)
+
+    return render(
+        request,
+        "confirmation/redirect_to_post.html",
+        context={
+            "target_url": reverse("confirm_email_change"),
+            "key": confirmation_key,
+        },
+    )
+
+
+@require_post
+@typed_endpoint
+def confirm_email_change(request: HttpRequest, *, key: str) -> HttpResponse:
     with transaction.atomic(durable=True):
         try:
             email_change_object = get_object_from_key(
-                confirmation_key, [Confirmation.EMAIL_CHANGE], mark_object_used=True
+                key, [Confirmation.EMAIL_CHANGE], mark_object_used=True
             )
         except ConfirmationKeyError as exception:
             return render_confirmation_key_error(request, exception)

zproject/urls.py

@@ -230,6 +230,7 @@ from zerver.views.user_groups import (
 )
 from zerver.views.user_settings import (
     confirm_email_change,
+    confirm_email_change_get,
     delete_avatar_backend,
     json_change_settings,
     regenerate_api_key,
@@ -668,10 +669,15 @@ i18n_urls = [
         name="get_prereg_key_and_redirect",
     ),
     path(
-        "accounts/confirm_new_email/<confirmation_key>",
+        "accounts/confirm_new_email/",
         confirm_email_change,
         name="confirm_email_change",
     ),
+    path(
+        "accounts/confirm_new_email/<confirmation_key>",
+        confirm_email_change_get,
+        name="confirm_email_change_get",
+    ),
     # Email unsubscription endpoint. Allows for unsubscribing from various types of emails,
     # including welcome emails, missed direct messages, etc.
     path(