From 5c7a8dd7854bd9f0e16c3881773e2a6960ea9113 Mon Sep 17 00:00:00 2001 From: Mike Wichers Date: Fri, 4 Sep 2026 16:28:58 -0400 Subject: [PATCH] login: honour next on both exits, same-origin only The console's 401 redirect carries next=/leaders/ and /login dropped it, so a leader arriving cold landed on /account. next now rides the form as a hidden field and is applied after a successful POST and when an already-signed-in person hits /login. identity.safe_next() admits only a relative path with a single leading slash - no scheme, no //, no backslash, no control characters - so the login page cannot become an open redirect. Ten checks added to tests/smoke_identity.py; the suite is 59 + 24 + 53, all green. --- app/auth.py | 18 ++++++++++++------ app/identity.py | 14 ++++++++++++++ tests/smoke_identity.py | 12 ++++++++++++ 3 files changed, 38 insertions(+), 6 deletions(-) diff --git a/app/auth.py b/app/auth.py index 9894532..87f66c8 100644 --- a/app/auth.py +++ b/app/auth.py @@ -82,10 +82,11 @@ def _shell(heading, intro, inner, error=None): # Login # --------------------------------------------------------------------------- -def _login_form(email="", error=None): +def _login_form(email="", error=None, next_url=""): return _shell( "Sign in", "For Pack 73 and Troop 73 leaders and families.", f"""
+ @@ -98,21 +99,26 @@ def _login_form(email="", error=None): @router.get("/login", response_class=HTMLResponse) def login_form(request: Request): + # `next` arrives from the leader console's 401 redirect (next=/leaders/). + # identity.safe_next() keeps it same-origin; anything odd lands on /account. + nxt = identity.safe_next(request.query_params.get("next")) if current_person(request): - return RedirectResponse(url="/account", status_code=303) - return HTMLResponse(_render("Sign in", _login_form())) + return RedirectResponse(url=nxt, status_code=303) + return HTMLResponse(_render("Sign in", _login_form(next_url=nxt))) @router.post("/login") -def login(request: Request, email: str = Form(""), password: str = Form("")): +def login(request: Request, email: str = Form(""), password: str = Form(""), + next: str = Form("")): + nxt = identity.safe_next(next) try: person, token = identity.authenticate( email, password, ip=_client_ip(request), user_agent=request.headers.get("user-agent")) except identity.IdentityError as e: - return HTMLResponse(_render("Sign in", _login_form(email, e.detail)), + return HTMLResponse(_render("Sign in", _login_form(email, e.detail, nxt)), status_code=e.status) - resp = RedirectResponse(url="/account", status_code=303) + resp = RedirectResponse(url=nxt, status_code=303) resp.set_cookie(COOKIE, token, max_age=identity.SESSION_ABSOLUTE_DAYS * 86400, httponly=True, secure=COOKIE_SECURE, samesite="lax", path="/") return resp diff --git a/app/identity.py b/app/identity.py index 1450878..7ab4c76 100644 --- a/app/identity.py +++ b/app/identity.py @@ -606,6 +606,20 @@ def consume_invite(token, full_name, password, preferred_name=None, phone=None, # Rows, not signed cookies. A leader stepping down has to be revocable now # rather than at token expiry, and "sign out everywhere" has to be possible. +def safe_next(value, default="/account"): + """Where to send someone after login. Only a same-origin relative path + survives: a single leading slash, no scheme, no protocol-relative `//`, + no backslash or control character that a browser might normalise into + one. Anything else falls back to default. This is what keeps /login from + being an open redirect the moment it starts honouring `next`.""" + v = (value or "").strip() + if not v.startswith("/") or v.startswith("//") or v.startswith("/\\"): + return default + if any(ord(c) < 32 or c in "\\" for c in v): + return default + return v + + def start_session(person_id, ip=None, user_agent=None): tok = secrets.token_urlsafe(32) con = connect() diff --git a/tests/smoke_identity.py b/tests/smoke_identity.py index ed9392e..605267e 100644 --- a/tests/smoke_identity.py +++ b/tests/smoke_identity.py @@ -168,5 +168,17 @@ con.close() for k in ("invite.created", "invite.consumed", "login.ok", "login.failed", "login.throttled"): check("auth_events records %s" % k, k in kinds) +print("\nsafe_next") +check("relative path passes", I.safe_next("/leaders/") == "/leaders/") +check("query string kept", I.safe_next("/leaders/nearby?x=1") == "/leaders/nearby?x=1") +check("empty falls back", I.safe_next("") == "/account") +check("None falls back", I.safe_next(None) == "/account") +check("absolute URL rejected", I.safe_next("https://evil.example/") == "/account") +check("protocol-relative rejected", I.safe_next("//evil.example/") == "/account") +check("backslash form rejected", I.safe_next("/\\evil.example") == "/account") +check("control char rejected", I.safe_next("/leaders\r\nX: y") == "/account") +check("no leading slash rejected", I.safe_next("leaders/") == "/account") +check("custom default honoured", I.safe_next("nope", default="/") == "/") + print("\n%d passed, %d failed" % (PASS, FAIL)) sys.exit(1 if FAIL else 0)