Improve /confirm w.r.t. OWASP (#817)

- no longer send a new token upon receiving an expired token
- no longer send identity/email information as part of query params in unauthenticated requests
- add Referrer-Policy="no-referrer" as suggested by OWASP

Minor improvements to API doc.

CHANGES.rst

@@ -23,14 +23,15 @@ Fixes
   'session' as authentication-method. (N247S)
 - (:issue:`814`) /reset and /confirm and GENERIC_RESPONSES and additional form args don't mix.
 - (:issue:`281`) Reset password can be exploited and other OWASP improvements.
+- (:pr:`xxx`) Confirmation can be exploited and other OWASP improvements.
 
 Backwards Compatibility Concerns
 +++++++++++++++++++++++++++++++++
 
 - To align with the W3C WebAuthn Level2 and 3 spec - transports are now part of the registration response.
   This has been changed BOTH in the server code (using py_webauth data structures) as well as the sample
   javascript code. If an application has their own javascript front end code - it might need to be changed.
-- Reset password was changed to improve OWASP compliance and reduce possible exploitation:
+- Reset password was changed to improve adhere to OWASP recommendations and reduce possible exploitation:
 
     - A new email (with new token) is no longer sent upon expired token. Users must restart
       the reset password process.
@@ -43,6 +44,15 @@ Backwards Compatibility Concerns
     - The default for :py:data:`SECURITY_RESET_PASSWORD_WITHIN` has been changed from `5 days` to `1 days`.
     - The response to GET /reset/<token> sets the HTTP header `Referrer-Policy` to `no-referrer` as suggested
       by OWASP.
+- Confirm email was changed to adhere to OWASP recommendations and reduce possible exploitation:
+
+    - A new email (with new token) is no longer sent upon expired token. Users must restart
+      the confirmation process.
+    - Identity information (identity, email) is no longer sent as part of the URL redirect
+      query params.
+    - The SECURITY_MSG_CONFIRMATION_EXPIRED message no longer contains the user's identity/email.
+    - The response to GET /reset/<token> sets the HTTP header `Referrer-Policy` to `no-referrer` as suggested
+      by OWASP.
 
 Version 5.2.0
 -------------

docs/openapi.yaml

@@ -596,15 +596,15 @@ paths:
               description: |
                 On spa-success: SECURITY_POST_CONFIRM_VIEW?identity={identity}&email={email}&{level}={msg}
 
-                On spa-error-expired: SECURITY_CONFIRM_ERROR_VIEW?error={msg}&identity={identity}&email={email}
+                On spa-error-expired: SECURITY_CONFIRM_ERROR_VIEW?error={msg}
 
                 On spa-error-invalid-token: SECURITY_CONFIRM_ERROR_VIEW?error={msg}
 
                 On form-success: SECURITY_POST_CONFIRM_VIEW or
                                  SECURITY_POST_LOGIN_VIEW
 
                 On form-success (no auto-login): SECURITY_POST_CONFIRM_VIEW or
-                                 SECURITY_LOGIN_URL
+                                 ".login"
 
                 On form-error-expired: SECURITY_CONFIRM_ERROR_VIEW or
                                        SECURITY_CONFIRM_URL

flask_security/core.py

@@ -441,11 +441,7 @@
         "info",
     ),
     "CONFIRMATION_EXPIRED": (
-        _(
-            "You did not confirm your email within %(within)s. "
-            "New instructions to confirm your email have been sent "
-            "to %(email)s."
-        ),
+        _("You did not confirm your email within %(within)s. "),
         "error",
     ),
     "LOGIN_EXPIRED": (

flask_security/views.py

@@ -433,12 +433,18 @@ def send_confirmation():
 
 
 def confirm_email(token):
-    """View function which handles a email confirmation request."""
+    """View function which handles an email confirmation request."""
 
     expired, invalid, user = confirm_email_token_status(token)
 
-    if not user or invalid:
-        m, c = get_message("INVALID_CONFIRMATION_TOKEN")
+    if not user or invalid or expired:
+        if expired:
+            m, c = get_message(
+                "CONFIRMATION_EXPIRED",
+                within=cv("CONFIRM_EMAIL_WITHIN"),
+            )
+        else:
+            m, c = get_message("INVALID_CONFIRMATION_TOKEN")
         if _security.redirect_behavior == "spa":
             return redirect(get_url(_security.confirm_error_view, qparams={c: m}))
         do_flash(m, c)
@@ -449,61 +455,70 @@ def confirm_email(token):
 
     already_confirmed = user.confirmed_at is not None
 
-    if expired or already_confirmed:
-        if already_confirmed:
-            m, c = get_message("ALREADY_CONFIRMED")
-        else:
-            send_confirmation_instructions(user)
-            m, c = get_message(
-                "CONFIRMATION_EXPIRED",
-                email=user.email,
-                within=_security.confirm_email_within,
-            )
+    if already_confirmed:
+        m, c = get_message("ALREADY_CONFIRMED")
 
         if _security.redirect_behavior == "spa":
-            return redirect(
-                get_url(
-                    _security.confirm_error_view,
-                    qparams=user.get_redirect_qparams({c: m}),
-                )
+            # No reason to expose identity info to anyone who has the link
+            return (
+                redirect(
+                    get_url(
+                        _security.confirm_error_view,
+                        qparams={c: m},
+                    )
+                ),
+                {"Referrer-Policy": "no-referrer"},
             )
 
         do_flash(m, c)
-        return redirect(
-            get_url(_security.confirm_error_view)
-            or url_for_security("send_confirmation")
+        return (
+            redirect(
+                get_url(_security.confirm_error_view)
+                or url_for_security("send_confirmation")
+            ),
+            {"Referrer-Policy": "no-referrer"},
         )
 
     confirm_user(user)
     after_this_request(view_commit)
+    m, c = get_message("EMAIL_CONFIRMED")
 
     if user != current_user:
         logout_user()
         if cv("AUTO_LOGIN_AFTER_CONFIRM"):
             # N.B. this is a (small) security risk if email went to wrong place.
-            # and you have the LOGIN_WITH_CONFIRMATION flag since in that case
+            # and you have the LOGIN_WITHOUT_CONFIRMATION flag since in that case
             # you can be logged in and doing stuff - but another person could
             # get the email.
             response = _security.two_factor_plugins.tf_enter(
                 user, False, "confirm", next_loc=propagate_next(request.url)
             )
             if response:
-                return response
+                do_flash(m, c)
+                return response, {"Referrer-Policy": "no-referrer"}
             login_user(user, authn_via=["confirm"])
 
-    m, c = get_message("EMAIL_CONFIRMED")
     if _security.redirect_behavior == "spa":
-        return redirect(
-            get_url(
-                _security.post_confirm_view, qparams=user.get_redirect_qparams({c: m})
-            )
+        return (
+            redirect(
+                get_url(
+                    _security.post_confirm_view,
+                    qparams=user.get_redirect_qparams({c: m}),
+                )
+            ),
+            {"Referrer-Policy": "no-referrer"},
         )
     do_flash(m, c)
-    return redirect(
-        get_url(_security.post_confirm_view)
-        or get_url(
-            _security.post_login_view if cv("AUTO_LOGIN_AFTER_CONFIRM") else ".login"
-        )
+    return (
+        redirect(
+            get_url(_security.post_confirm_view)
+            or get_url(
+                _security.post_login_view
+                if cv("AUTO_LOGIN_AFTER_CONFIRM")
+                else ".login"
+            )
+        ),
+        {"Referrer-Policy": "no-referrer"},
     )
 
 

tests/test_confirmable.py

@@ -15,7 +15,6 @@
 from wtforms.validators import Length
 
 from flask_security.core import Security, UserMixin
-from flask_security.confirmable import generate_confirmation_token
 from flask_security.signals import confirm_instructions_sent, user_confirmed
 from flask_security.forms import SendConfirmationForm
 
@@ -87,25 +86,17 @@ def on_instructions_sent(app, **kwargs):
 
     # Test confirm
     token = registrations[0]["confirm_token"]
-    response = clients.get("/confirm/" + token, follow_redirects=True)
-    assert get_message("EMAIL_CONFIRMED") in response.data
+    response = clients.get("/confirm/" + token, follow_redirects=False)
     assert len(recorded_confirms) == 1
+    assert response.headers.get("Referrer-Policy", None) == "no-referrer"
+    response = clients.get(response.location)
+    assert get_message("EMAIL_CONFIRMED") in response.data
 
     # Test already confirmed
     response = clients.get("/confirm/" + token, follow_redirects=True)
     assert get_message("ALREADY_CONFIRMED") in response.data
     assert len(recorded_instructions_sent) == 2
 
-    # Test already confirmed and expired token
-    app.config["SECURITY_CONFIRM_EMAIL_WITHIN"] = "-1 days"
-    with app.test_request_context("/"):
-        email = registrations[0]["email"]
-        user = app.security.datastore.find_user(email=email)
-        expired_token = generate_confirmation_token(user)
-    response = clients.get("/confirm/" + expired_token, follow_redirects=True)
-    assert get_message("ALREADY_CONFIRMED") in response.data
-    assert len(recorded_instructions_sent) == 2
-
     # Test already confirmed when asking for confirmation instructions
     logout(clients)
 
@@ -358,17 +349,19 @@ def test_confirm_redirect_to_post_confirm(client, get_message):
 
     token = registrations[0]["confirm_token"]
 
-    response = client.get("/confirm/" + token, follow_redirects=True)
-    assert b"Post Confirm" in response.data
+    response = client.get("/confirm/" + token, follow_redirects=False)
+    assert "/post_confirm" in response.location
+    assert response.headers.get("Referrer-Policy", None) == "no-referrer"
 
 
 @pytest.mark.registerable()
 @pytest.mark.settings(
     redirect_host="localhost:8081",
     redirect_behavior="spa",
     post_confirm_view="/confirm-redirect",
+    confirm_error_view="/confirm-error",
 )
-def test_spa_get(app, client):
+def test_spa_get(app, client, get_message):
     """
     Test 'single-page-application' style redirects
     This uses json only.
@@ -390,6 +383,17 @@ def test_spa_get(app, client):
         assert "/confirm-redirect" == split.path
         qparams = dict(parse_qsl(split.query))
         assert qparams["email"] == "dude@lp.com"
+        assert response.headers.get("Referrer-Policy", None) == "no-referrer"
+
+        response = client.get("/confirm/" + token)
+        split = urlsplit(response.headers["Location"])
+        qparams = dict(parse_qsl(split.query))
+        assert response.status_code == 302
+        assert "/confirm-error" in response.location
+        assert "email" not in qparams
+        assert get_message("ALREADY_CONFIRMED") in qparams["info"].encode("utf-8")
+        assert response.headers.get("Referrer-Policy", None) == "no-referrer"
+
     # Arguably for json we shouldn't have any - this is buried in register_user
     # but really shouldn't be.
     assert len(flashes) == 1
@@ -421,12 +425,10 @@ def test_spa_get_bad_token(app, client, get_message):
         assert "localhost:8081" == split.netloc
         assert "/confirm-error" == split.path
         qparams = dict(parse_qsl(split.query))
-        assert all(k in qparams for k in ["email", "error", "identity"])
-        assert qparams["identity"] == "dude@lp.com"
+        assert "email" not in qparams
+        assert "identity" not in qparams
 
-        msg = get_message(
-            "CONFIRMATION_EXPIRED", within="1 milliseconds", email="dude@lp.com"
-        )
+        msg = get_message("CONFIRMATION_EXPIRED", within="1 milliseconds")
         assert msg == qparams["error"].encode("utf-8")
 
         # Test mangled token