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.
This commit is contained in:
+12
-6
@@ -82,10 +82,11 @@ def _shell(heading, intro, inner, error=None):
|
|||||||
# Login
|
# Login
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
def _login_form(email="", error=None):
|
def _login_form(email="", error=None, next_url=""):
|
||||||
return _shell(
|
return _shell(
|
||||||
"Sign in", "For Pack 73 and Troop 73 leaders and families.",
|
"Sign in", "For Pack 73 and Troop 73 leaders and families.",
|
||||||
f"""<form method="post" action="/login">
|
f"""<form method="post" action="/login">
|
||||||
|
<input type="hidden" name="next" value="{_esc(next_url)}">
|
||||||
<label for="email">Email</label>
|
<label for="email">Email</label>
|
||||||
<input id="email" name="email" type="email" autocomplete="username" required value="{_esc(email)}">
|
<input id="email" name="email" type="email" autocomplete="username" required value="{_esc(email)}">
|
||||||
<label for="password">Password</label>
|
<label for="password">Password</label>
|
||||||
@@ -98,21 +99,26 @@ def _login_form(email="", error=None):
|
|||||||
|
|
||||||
@router.get("/login", response_class=HTMLResponse)
|
@router.get("/login", response_class=HTMLResponse)
|
||||||
def login_form(request: Request):
|
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):
|
if current_person(request):
|
||||||
return RedirectResponse(url="/account", status_code=303)
|
return RedirectResponse(url=nxt, status_code=303)
|
||||||
return HTMLResponse(_render("Sign in", _login_form()))
|
return HTMLResponse(_render("Sign in", _login_form(next_url=nxt)))
|
||||||
|
|
||||||
|
|
||||||
@router.post("/login")
|
@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:
|
try:
|
||||||
person, token = identity.authenticate(
|
person, token = identity.authenticate(
|
||||||
email, password, ip=_client_ip(request),
|
email, password, ip=_client_ip(request),
|
||||||
user_agent=request.headers.get("user-agent"))
|
user_agent=request.headers.get("user-agent"))
|
||||||
except identity.IdentityError as e:
|
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)
|
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,
|
resp.set_cookie(COOKIE, token, max_age=identity.SESSION_ABSOLUTE_DAYS * 86400,
|
||||||
httponly=True, secure=COOKIE_SECURE, samesite="lax", path="/")
|
httponly=True, secure=COOKIE_SECURE, samesite="lax", path="/")
|
||||||
return resp
|
return resp
|
||||||
|
|||||||
@@ -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
|
# 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.
|
# 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):
|
def start_session(person_id, ip=None, user_agent=None):
|
||||||
tok = secrets.token_urlsafe(32)
|
tok = secrets.token_urlsafe(32)
|
||||||
con = connect()
|
con = connect()
|
||||||
|
|||||||
@@ -168,5 +168,17 @@ con.close()
|
|||||||
for k in ("invite.created", "invite.consumed", "login.ok", "login.failed", "login.throttled"):
|
for k in ("invite.created", "invite.consumed", "login.ok", "login.failed", "login.throttled"):
|
||||||
check("auth_events records %s" % k, k in kinds)
|
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))
|
print("\n%d passed, %d failed" % (PASS, FAIL))
|
||||||
sys.exit(1 if FAIL else 0)
|
sys.exit(1 if FAIL else 0)
|
||||||
|
|||||||
Reference in New Issue
Block a user