fix(auth): address R4 S1 code-review findings
- link/upgrade OAuth cancel now redirects to /?link=cancelled (a channel App.tsx surfaces as a toast) instead of the pre-auth /?login=oauth_cancelled banner a signed-in user never sees; a plain sign-in cancel is unchanged - the login banner (suspended / oauth_cancelled) is now state, cleared on the first deliberate action like the verify banner — no more stale lingering - add a "Didn't get the verification email?" resend entry on the sign-in screen so a never-arrived verification mail isn't a dead end (was only reachable from the expired-link banner) - unit-test _oauth_redirect_uri (dev-host derive, unknown-host fallback, prod ignores the Host header — the injection guard) - collapse the 5-deep title/button ternaries into Record<Mode,string> maps
This commit is contained in:
+6
-2
@@ -308,9 +308,13 @@ async def callback(
|
||||
try:
|
||||
token = await client.authorize_access_token(request)
|
||||
except OAuthError as exc:
|
||||
# The user cancelled or denied consent at Google — land back on the login page with a
|
||||
# friendly banner instead of a raw 400 JSON error page (applies to sign-in and link flows).
|
||||
# The user cancelled or denied consent at Google — land back with a friendly notice instead
|
||||
# of a raw 400 JSON error page. A link/upgrade cancel is an ALREADY-signed-in user who never
|
||||
# sees the pre-auth login banner, so route it through the ?link= channel App.tsx surfaces as
|
||||
# a toast; a plain sign-in cancel lands on the login page with the oauth_cancelled banner.
|
||||
log.info("Google OAuth cancelled/failed: %s", exc.error)
|
||||
if link_uid is not None:
|
||||
return RedirectResponse(url="/?link=cancelled")
|
||||
return RedirectResponse(url="/?login=oauth_cancelled")
|
||||
|
||||
userinfo = token.get("userinfo")
|
||||
|
||||
@@ -0,0 +1,58 @@
|
||||
"""_oauth_redirect_uri decides where Google sends the OAuth code back. In production it must ALWAYS
|
||||
use the fixed configured URL and never trust the request Host (a forged Host must not be able to
|
||||
redirect the code); in local dev it honors a small allowlist of dev origins so a login started on the
|
||||
Vite server (:5173) returns to :5173."""
|
||||
from types import SimpleNamespace
|
||||
|
||||
from app import auth
|
||||
|
||||
|
||||
def _req(host):
|
||||
"""Minimal stand-in for a Starlette Request: only .headers.get('host') is read."""
|
||||
return SimpleNamespace(headers={"host": host} if host is not None else {})
|
||||
|
||||
|
||||
def _dev_settings():
|
||||
return SimpleNamespace(
|
||||
session_https_only=False,
|
||||
oauth_redirect_url="http://localhost:8080/auth/callback",
|
||||
)
|
||||
|
||||
|
||||
def _prod_settings():
|
||||
return SimpleNamespace(
|
||||
session_https_only=True,
|
||||
oauth_redirect_url="https://siftlode.example.eu/auth/callback",
|
||||
)
|
||||
|
||||
|
||||
def test_dev_known_host_returns_to_that_origin(monkeypatch):
|
||||
monkeypatch.setattr(auth, "settings", _dev_settings())
|
||||
assert auth._oauth_redirect_uri(_req("localhost:5173")) == "http://localhost:5173/auth/callback"
|
||||
assert auth._oauth_redirect_uri(_req("localhost:8080")) == "http://localhost:8080/auth/callback"
|
||||
assert auth._oauth_redirect_uri(_req("127.0.0.1:5173")) == "http://127.0.0.1:5173/auth/callback"
|
||||
|
||||
|
||||
def test_dev_host_is_case_insensitive(monkeypatch):
|
||||
monkeypatch.setattr(auth, "settings", _dev_settings())
|
||||
assert auth._oauth_redirect_uri(_req("LOCALHOST:5173")) == "http://localhost:5173/auth/callback"
|
||||
|
||||
|
||||
def test_dev_unknown_or_missing_host_falls_back_to_configured(monkeypatch):
|
||||
monkeypatch.setattr(auth, "settings", _dev_settings())
|
||||
# An origin not on the allowlist (or a request with no Host) must not drive the redirect.
|
||||
assert auth._oauth_redirect_uri(_req("evil.example.com")) == "http://localhost:8080/auth/callback"
|
||||
assert auth._oauth_redirect_uri(_req(None)) == "http://localhost:8080/auth/callback"
|
||||
|
||||
|
||||
def test_prod_ignores_host_even_when_it_matches_a_dev_origin(monkeypatch):
|
||||
monkeypatch.setattr(auth, "settings", _prod_settings())
|
||||
# The host-injection guard: in production the fixed URL wins regardless of the Host header.
|
||||
assert (
|
||||
auth._oauth_redirect_uri(_req("localhost:5173"))
|
||||
== "https://siftlode.example.eu/auth/callback"
|
||||
)
|
||||
assert (
|
||||
auth._oauth_redirect_uri(_req("attacker.test"))
|
||||
== "https://siftlode.example.eu/auth/callback"
|
||||
)
|
||||
Reference in New Issue
Block a user