fix(security): require CSRF-protected POST to consume a registration link
GET /register/{token} consumed the magic-link token and created a session, so
a side-effecting state change happened on a safe method — link prefetchers,
email scanners, or a cross-site GET could trigger account setup/login.
Split the flow: GET validates the token (without consuming) and renders a
confirmation form; POST /register/{token} consumes the token, runs the
existing checks, and establishes the session. The POST carries a CSRF token
and the session is reset on login as for other auth paths.
Refs: porchlight-9k0
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
efb265a68b
commit
baef5e0e2e
6 changed files with 96 additions and 10 deletions
|
|
@ -86,7 +86,28 @@ async def logout(request: Request) -> Response:
|
||||||
return response
|
return response
|
||||||
|
|
||||||
|
|
||||||
@router.get("/register/{token}")
|
@router.get("/register/{token}", response_class=HTMLResponse)
|
||||||
|
async def register_magic_link_page(request: Request, token: str) -> Response:
|
||||||
|
"""Show the registration confirmation page.
|
||||||
|
|
||||||
|
GET is side-effect free: it only validates the token (without consuming it)
|
||||||
|
and renders a form. The token is consumed by the POST below, so that simply
|
||||||
|
visiting the link (email scanners, prefetchers) cannot create a session.
|
||||||
|
"""
|
||||||
|
magic_link_service = request.app.state.magic_link_service
|
||||||
|
link = await magic_link_service.validate(token)
|
||||||
|
if link is None:
|
||||||
|
return HTMLResponse("<p>Invalid or expired registration link.</p>", status_code=400)
|
||||||
|
|
||||||
|
templates = request.app.state.templates
|
||||||
|
return templates.TemplateResponse(
|
||||||
|
request,
|
||||||
|
"register.html",
|
||||||
|
{"token": token, "username": link.username},
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
@router.post("/register/{token}")
|
||||||
async def register_magic_link(request: Request, token: str) -> Response:
|
async def register_magic_link(request: Request, token: str) -> Response:
|
||||||
magic_link_service = request.app.state.magic_link_service
|
magic_link_service = request.app.state.magic_link_service
|
||||||
user_repo = request.app.state.user_repo
|
user_repo = request.app.state.user_repo
|
||||||
|
|
@ -118,8 +139,7 @@ async def register_magic_link(request: Request, token: str) -> Response:
|
||||||
user = User(userid=userid, username=link.username, groups=["users"])
|
user = User(userid=userid, username=link.username, groups=["users"])
|
||||||
await user_repo.create(user)
|
await user_repo.create(user)
|
||||||
|
|
||||||
request.session["userid"] = user.userid
|
_establish_authenticated_session(request, user)
|
||||||
request.session["username"] = user.username
|
|
||||||
|
|
||||||
return RedirectResponse("/manage/credentials?setup=1", status_code=303)
|
return RedirectResponse("/manage/credentials?setup=1", status_code=303)
|
||||||
|
|
||||||
|
|
|
||||||
14
src/porchlight/templates/register.html
Normal file
14
src/porchlight/templates/register.html
Normal file
|
|
@ -0,0 +1,14 @@
|
||||||
|
{% extends "base.html" %}
|
||||||
|
|
||||||
|
{% block title %}Set up your account — Porchlight{% endblock %}
|
||||||
|
|
||||||
|
{% block content %}
|
||||||
|
<h1>Set up your account</h1>
|
||||||
|
|
||||||
|
<p>You're about to set up the account for <strong>{{ username }}</strong>.</p>
|
||||||
|
|
||||||
|
<form method="post" action="/register/{{ token }}">
|
||||||
|
<input type="hidden" name="csrf_token" value="{{ csrf_token_processor(request) }}">
|
||||||
|
<button type="submit">Continue</button>
|
||||||
|
</form>
|
||||||
|
{% endblock %}
|
||||||
|
|
@ -9,7 +9,9 @@ test.describe('Full user journey', () => {
|
||||||
expect(fixtures.register_token).toBeTruthy();
|
expect(fixtures.register_token).toBeTruthy();
|
||||||
|
|
||||||
// ---- Step 1: Register via magic link ----
|
// ---- Step 1: Register via magic link ----
|
||||||
|
// GET shows a confirmation page; submitting it (POST) consumes the token.
|
||||||
await page.goto(`/register/${fixtures.register_token}`);
|
await page.goto(`/register/${fixtures.register_token}`);
|
||||||
|
await page.click('button[type="submit"]');
|
||||||
|
|
||||||
// Should redirect to /manage/credentials?setup=1
|
// Should redirect to /manage/credentials?setup=1
|
||||||
await page.waitForURL('**/manage/credentials?setup=1', { timeout: 5000 });
|
await page.waitForURL('**/manage/credentials?setup=1', { timeout: 5000 });
|
||||||
|
|
|
||||||
|
|
@ -4,6 +4,7 @@ from httpx import AsyncClient
|
||||||
from porchlight.authn.password import PasswordService
|
from porchlight.authn.password import PasswordService
|
||||||
from porchlight.invite.service import MagicLinkService
|
from porchlight.invite.service import MagicLinkService
|
||||||
from porchlight.models import PasswordCredential, User
|
from porchlight.models import PasswordCredential, User
|
||||||
|
from tests.conftest import get_csrf_token
|
||||||
|
|
||||||
|
|
||||||
async def test_register_invalid_token_returns_error_page(client: AsyncClient) -> None:
|
async def test_register_invalid_token_returns_error_page(client: AsyncClient) -> None:
|
||||||
|
|
@ -23,14 +24,33 @@ async def test_register_expired_token_returns_error_page(client: AsyncClient) ->
|
||||||
assert "Invalid or expired" in res.text
|
assert "Invalid or expired" in res.text
|
||||||
|
|
||||||
|
|
||||||
async def test_register_valid_token_creates_user_and_redirects(client: AsyncClient) -> None:
|
async def test_register_get_does_not_consume_token(client: AsyncClient) -> None:
|
||||||
|
"""GET is side-effect free: it shows a confirmation form and must not
|
||||||
|
consume the token or create a session."""
|
||||||
|
app = client._transport.app # type: ignore[union-attr]
|
||||||
|
magic_link_service = app.state.magic_link_service
|
||||||
|
|
||||||
|
link = await magic_link_service.create(username="newuser")
|
||||||
|
res = await client.get(f"/register/{link.token}", follow_redirects=False)
|
||||||
|
assert res.status_code == 200
|
||||||
|
assert f'action="/register/{link.token}"' in res.text
|
||||||
|
# Token still valid (not consumed by the GET).
|
||||||
|
assert await magic_link_service.validate(link.token) is not None
|
||||||
|
|
||||||
|
|
||||||
|
async def test_register_post_creates_user_and_redirects(client: AsyncClient) -> None:
|
||||||
app = client._transport.app # type: ignore[union-attr]
|
app = client._transport.app # type: ignore[union-attr]
|
||||||
magic_link_service = app.state.magic_link_service
|
magic_link_service = app.state.magic_link_service
|
||||||
user_repo = app.state.user_repo
|
user_repo = app.state.user_repo
|
||||||
|
|
||||||
link = await magic_link_service.create(username="newuser")
|
link = await magic_link_service.create(username="newuser")
|
||||||
|
|
||||||
res = await client.get(f"/register/{link.token}", follow_redirects=False)
|
csrf = await get_csrf_token(client)
|
||||||
|
res = await client.post(
|
||||||
|
f"/register/{link.token}",
|
||||||
|
headers={"X-CSRF-Token": csrf},
|
||||||
|
follow_redirects=False,
|
||||||
|
)
|
||||||
assert res.status_code in (302, 303)
|
assert res.status_code in (302, 303)
|
||||||
assert "/manage/credentials" in res.headers["location"]
|
assert "/manage/credentials" in res.headers["location"]
|
||||||
assert "setup=1" in res.headers["location"]
|
assert "setup=1" in res.headers["location"]
|
||||||
|
|
@ -44,6 +64,17 @@ async def test_register_valid_token_creates_user_and_redirects(client: AsyncClie
|
||||||
assert "users" in user.groups
|
assert "users" in user.groups
|
||||||
|
|
||||||
|
|
||||||
|
async def test_register_post_requires_csrf(client: AsyncClient) -> None:
|
||||||
|
app = client._transport.app # type: ignore[union-attr]
|
||||||
|
magic_link_service = app.state.magic_link_service
|
||||||
|
link = await magic_link_service.create(username="newuser")
|
||||||
|
|
||||||
|
res = await client.post(f"/register/{link.token}", follow_redirects=False)
|
||||||
|
assert res.status_code == 403
|
||||||
|
# Token must not have been consumed by the rejected request.
|
||||||
|
assert await magic_link_service.validate(link.token) is not None
|
||||||
|
|
||||||
|
|
||||||
async def test_register_used_token_returns_error(client: AsyncClient) -> None:
|
async def test_register_used_token_returns_error(client: AsyncClient) -> None:
|
||||||
app = client._transport.app # type: ignore[union-attr]
|
app = client._transport.app # type: ignore[union-attr]
|
||||||
magic_link_service = app.state.magic_link_service
|
magic_link_service = app.state.magic_link_service
|
||||||
|
|
@ -67,7 +98,12 @@ async def test_register_existing_user_logs_in_and_redirects(client: AsyncClient)
|
||||||
|
|
||||||
link = await magic_link_service.create(username="admin")
|
link = await magic_link_service.create(username="admin")
|
||||||
|
|
||||||
res = await client.get(f"/register/{link.token}", follow_redirects=False)
|
csrf = await get_csrf_token(client)
|
||||||
|
res = await client.post(
|
||||||
|
f"/register/{link.token}",
|
||||||
|
headers={"X-CSRF-Token": csrf},
|
||||||
|
follow_redirects=False,
|
||||||
|
)
|
||||||
assert res.status_code in (302, 303)
|
assert res.status_code in (302, 303)
|
||||||
assert "/manage/credentials" in res.headers["location"]
|
assert "/manage/credentials" in res.headers["location"]
|
||||||
assert "setup=1" in res.headers["location"]
|
assert "setup=1" in res.headers["location"]
|
||||||
|
|
@ -96,7 +132,12 @@ async def test_register_rejects_user_that_already_has_credentials(client: AsyncC
|
||||||
await cred_repo.create_password(PasswordCredential(user_id=user.userid, password_hash=svc.hash("existing-pass")))
|
await cred_repo.create_password(PasswordCredential(user_id=user.userid, password_hash=svc.hash("existing-pass")))
|
||||||
|
|
||||||
link = await magic_link_service.create(username="hascreds")
|
link = await magic_link_service.create(username="hascreds")
|
||||||
res = await client.get(f"/register/{link.token}", follow_redirects=False)
|
csrf = await get_csrf_token(client)
|
||||||
|
res = await client.post(
|
||||||
|
f"/register/{link.token}",
|
||||||
|
headers={"X-CSRF-Token": csrf},
|
||||||
|
follow_redirects=False,
|
||||||
|
)
|
||||||
|
|
||||||
# No passwordless login: rejected, not redirected to setup.
|
# No passwordless login: rejected, not redirected to setup.
|
||||||
assert res.status_code == 400
|
assert res.status_code == 400
|
||||||
|
|
|
||||||
|
|
@ -61,6 +61,11 @@ async def test_inactive_user_cannot_register_magic_link(client: AsyncClient) ->
|
||||||
|
|
||||||
link = await magic_link_service.create(username="deactivated", created_by="admin", note="test")
|
link = await magic_link_service.create(username="deactivated", created_by="admin", note="test")
|
||||||
|
|
||||||
response = await client.get(f"/register/{link.token}", follow_redirects=False)
|
csrf = await get_csrf_token(client)
|
||||||
|
response = await client.post(
|
||||||
|
f"/register/{link.token}",
|
||||||
|
headers={"X-CSRF-Token": csrf},
|
||||||
|
follow_redirects=False,
|
||||||
|
)
|
||||||
|
|
||||||
assert response.status_code == 400 or "deactivated" in response.text.lower() or "Invalid" in response.text
|
assert response.status_code == 400 or "deactivated" in response.text.lower() or "Invalid" in response.text
|
||||||
|
|
|
||||||
|
|
@ -48,11 +48,15 @@ async def _login_user_without_password(client: AsyncClient) -> str:
|
||||||
)
|
)
|
||||||
await user_repo.create(user)
|
await user_repo.create(user)
|
||||||
|
|
||||||
# Simulate session login via magic link
|
# Simulate session login via magic link (GET shows the form, POST consumes)
|
||||||
token = await get_csrf_token(client)
|
token = await get_csrf_token(client)
|
||||||
magic_link_service = app.state.magic_link_service
|
magic_link_service = app.state.magic_link_service
|
||||||
link = await magic_link_service.create(username="newuser", created_by="admin", note="test")
|
link = await magic_link_service.create(username="newuser", created_by="admin", note="test")
|
||||||
await client.get(f"/register/{link.token}", follow_redirects=False)
|
await client.post(
|
||||||
|
f"/register/{link.token}",
|
||||||
|
headers={"X-CSRF-Token": token},
|
||||||
|
follow_redirects=False,
|
||||||
|
)
|
||||||
# Re-fetch CSRF token after session change
|
# Re-fetch CSRF token after session change
|
||||||
token = await get_csrf_token(client)
|
token = await get_csrf_token(client)
|
||||||
return token
|
return token
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue