fix(security): store only a hash of magic-link tokens
Magic-link tokens were persisted in plaintext, so a database read disclosed usable login/invite tokens. The service now hashes tokens (HMAC-SHA256 when a pepper is configured, else SHA-256 of the high-entropy token) and persists only the hash; the raw token is exposed solely in the registration URL and is re-attached to objects returned to callers. Refs: porchlight-42h Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
cdde3e3754
commit
91a2277664
3 changed files with 65 additions and 62 deletions
|
|
@ -1,3 +1,5 @@
|
||||||
|
import hashlib
|
||||||
|
import hmac
|
||||||
import secrets
|
import secrets
|
||||||
from datetime import UTC, datetime, timedelta
|
from datetime import UTC, datetime, timedelta
|
||||||
|
|
||||||
|
|
@ -6,11 +8,23 @@ from porchlight.store.protocols import MagicLinkRepository
|
||||||
|
|
||||||
|
|
||||||
class MagicLinkService:
|
class MagicLinkService:
|
||||||
"""Magic link token lifecycle management."""
|
"""Magic link token lifecycle management.
|
||||||
|
|
||||||
def __init__(self, repo: MagicLinkRepository, ttl: int = 86400) -> None:
|
The raw token is only ever exposed to the user (in the registration URL).
|
||||||
|
Only a hash of the token is persisted, so a database disclosure does not
|
||||||
|
yield usable tokens. A pepper, when configured, keys the hash (HMAC);
|
||||||
|
otherwise a plain SHA-256 of the high-entropy token is stored.
|
||||||
|
"""
|
||||||
|
|
||||||
|
def __init__(self, repo: MagicLinkRepository, ttl: int = 86400, pepper: str | None = None) -> None:
|
||||||
self._repo = repo
|
self._repo = repo
|
||||||
self._ttl = ttl
|
self._ttl = ttl
|
||||||
|
self._pepper = pepper
|
||||||
|
|
||||||
|
def _hash_token(self, token: str) -> str:
|
||||||
|
if self._pepper:
|
||||||
|
return hmac.new(self._pepper.encode(), token.encode(), hashlib.sha256).hexdigest()
|
||||||
|
return hashlib.sha256(token.encode()).hexdigest()
|
||||||
|
|
||||||
async def create(
|
async def create(
|
||||||
self,
|
self,
|
||||||
|
|
@ -18,34 +32,39 @@ class MagicLinkService:
|
||||||
created_by: str | None = None,
|
created_by: str | None = None,
|
||||||
note: str | None = None,
|
note: str | None = None,
|
||||||
) -> MagicLink:
|
) -> MagicLink:
|
||||||
"""Generate and store a new magic link for the given username."""
|
"""Generate and store a new magic link for the given username.
|
||||||
|
|
||||||
|
Persists only the hashed token; the returned MagicLink carries the raw
|
||||||
|
token so the caller can build the registration URL.
|
||||||
|
"""
|
||||||
token = secrets.token_urlsafe(32)
|
token = secrets.token_urlsafe(32)
|
||||||
expires_at = datetime.now(UTC) + timedelta(seconds=self._ttl)
|
expires_at = datetime.now(UTC) + timedelta(seconds=self._ttl)
|
||||||
link = MagicLink(
|
stored = MagicLink(
|
||||||
token=token,
|
token=self._hash_token(token),
|
||||||
username=username,
|
username=username,
|
||||||
expires_at=expires_at,
|
expires_at=expires_at,
|
||||||
created_by=created_by,
|
created_by=created_by,
|
||||||
note=note,
|
note=note,
|
||||||
)
|
)
|
||||||
return await self._repo.create(link)
|
await self._repo.create(stored)
|
||||||
|
return stored.model_copy(update={"token": token})
|
||||||
|
|
||||||
async def validate(self, token: str) -> MagicLink | None:
|
async def validate(self, token: str) -> MagicLink | None:
|
||||||
"""Validate a magic link token.
|
"""Validate a magic link token.
|
||||||
|
|
||||||
Returns the MagicLink if it exists, is not used, and has not expired.
|
Returns the MagicLink (with the raw token re-attached) if it exists, is
|
||||||
Returns None otherwise.
|
not used, and has not expired. Returns None otherwise.
|
||||||
"""
|
"""
|
||||||
link = await self._repo.get_by_token(token)
|
link = await self._repo.get_by_token(self._hash_token(token))
|
||||||
if link is None or link.used:
|
if link is None or link.used:
|
||||||
return None
|
return None
|
||||||
if link.expires_at < datetime.now(UTC):
|
if link.expires_at < datetime.now(UTC):
|
||||||
return None
|
return None
|
||||||
return link
|
return link.model_copy(update={"token": token})
|
||||||
|
|
||||||
async def mark_used(self, token: str) -> bool:
|
async def mark_used(self, token: str) -> bool:
|
||||||
"""Mark a magic link as used. Returns True if found and marked."""
|
"""Mark a magic link as used. Returns True if found and marked."""
|
||||||
return await self._repo.mark_used(token)
|
return await self._repo.mark_used(self._hash_token(token))
|
||||||
|
|
||||||
async def cleanup_expired(self) -> int:
|
async def cleanup_expired(self) -> int:
|
||||||
"""Delete expired unused links. Returns count deleted."""
|
"""Delete expired unused links. Returns count deleted."""
|
||||||
|
|
|
||||||
|
|
@ -1,8 +1,7 @@
|
||||||
from datetime import UTC, datetime, timedelta
|
|
||||||
|
|
||||||
from httpx import AsyncClient
|
from httpx import AsyncClient
|
||||||
|
|
||||||
from porchlight.models import MagicLink, User
|
from porchlight.invite.service import MagicLinkService
|
||||||
|
from porchlight.models import User
|
||||||
|
|
||||||
|
|
||||||
async def test_register_invalid_token_returns_error_page(client: AsyncClient) -> None:
|
async def test_register_invalid_token_returns_error_page(client: AsyncClient) -> None:
|
||||||
|
|
@ -13,42 +12,29 @@ async def test_register_invalid_token_returns_error_page(client: AsyncClient) ->
|
||||||
|
|
||||||
async def test_register_expired_token_returns_error_page(client: AsyncClient) -> None:
|
async def test_register_expired_token_returns_error_page(client: AsyncClient) -> None:
|
||||||
app = client._transport.app # type: ignore[union-attr]
|
app = client._transport.app # type: ignore[union-attr]
|
||||||
repo = app.state.magic_link_repo
|
# An already-expired link (negative TTL), stored hashed like the real service.
|
||||||
await repo.create(
|
expired_service = MagicLinkService(repo=app.state.magic_link_repo, ttl=-3600)
|
||||||
MagicLink(
|
link = await expired_service.create(username="newuser")
|
||||||
token="expired",
|
|
||||||
username="newuser",
|
|
||||||
expires_at=datetime.now(UTC) - timedelta(hours=1),
|
|
||||||
)
|
|
||||||
)
|
|
||||||
|
|
||||||
res = await client.get("/register/expired", follow_redirects=False)
|
res = await client.get(f"/register/{link.token}", follow_redirects=False)
|
||||||
assert res.status_code == 400
|
assert res.status_code == 400
|
||||||
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_valid_token_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_repo = app.state.magic_link_repo
|
magic_link_service = app.state.magic_link_service
|
||||||
user_repo = app.state.user_repo
|
user_repo = app.state.user_repo
|
||||||
|
|
||||||
await magic_link_repo.create(
|
link = await magic_link_service.create(username="newuser")
|
||||||
MagicLink(
|
|
||||||
token="t1",
|
|
||||||
username="newuser",
|
|
||||||
expires_at=datetime.now(UTC) + timedelta(hours=1),
|
|
||||||
)
|
|
||||||
)
|
|
||||||
|
|
||||||
res = await client.get("/register/t1", follow_redirects=False)
|
res = await client.get(f"/register/{link.token}", 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"]
|
||||||
|
|
||||||
# Token should be marked used
|
# Token should be consumed (no longer valid)
|
||||||
link = await magic_link_repo.get_by_token("t1")
|
assert await magic_link_service.validate(link.token) is None
|
||||||
assert link is not None
|
|
||||||
assert link.used is True
|
|
||||||
|
|
||||||
# User should exist
|
# User should exist
|
||||||
user = await user_repo.get_by_username("newuser")
|
user = await user_repo.get_by_username("newuser")
|
||||||
|
|
@ -58,48 +44,34 @@ async def test_register_valid_token_creates_user_and_redirects(client: AsyncClie
|
||||||
|
|
||||||
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]
|
||||||
repo = app.state.magic_link_repo
|
magic_link_service = app.state.magic_link_service
|
||||||
await repo.create(
|
link = await magic_link_service.create(username="newuser")
|
||||||
MagicLink(
|
await magic_link_service.mark_used(link.token)
|
||||||
token="used",
|
|
||||||
username="newuser",
|
|
||||||
expires_at=datetime.now(UTC) + timedelta(hours=1),
|
|
||||||
used=True,
|
|
||||||
)
|
|
||||||
)
|
|
||||||
|
|
||||||
res = await client.get("/register/used", follow_redirects=False)
|
res = await client.get(f"/register/{link.token}", follow_redirects=False)
|
||||||
assert res.status_code == 400
|
assert res.status_code == 400
|
||||||
|
|
||||||
|
|
||||||
async def test_register_existing_user_logs_in_and_redirects(client: AsyncClient) -> None:
|
async def test_register_existing_user_logs_in_and_redirects(client: AsyncClient) -> None:
|
||||||
"""When initial-admin creates a user, the invite link should log them in."""
|
"""When initial-admin creates a user (no credentials yet), the invite link
|
||||||
|
should let them set up their account."""
|
||||||
app = client._transport.app # type: ignore[union-attr]
|
app = client._transport.app # type: ignore[union-attr]
|
||||||
magic_link_repo = app.state.magic_link_repo
|
magic_link_service = app.state.magic_link_service
|
||||||
user_repo = app.state.user_repo
|
user_repo = app.state.user_repo
|
||||||
|
|
||||||
# Pre-create the user (as initial-admin would)
|
# Pre-create the user (as initial-admin would), with no credentials.
|
||||||
user = User(userid="lusab-bansen", username="admin", groups=["admin", "users"])
|
user = User(userid="lusab-bansen", username="admin", groups=["admin", "users"])
|
||||||
await user_repo.create(user)
|
await user_repo.create(user)
|
||||||
|
|
||||||
# Create invite for the same username
|
link = await magic_link_service.create(username="admin")
|
||||||
await magic_link_repo.create(
|
|
||||||
MagicLink(
|
|
||||||
token="admin-setup",
|
|
||||||
username="admin",
|
|
||||||
expires_at=datetime.now(UTC) + timedelta(hours=1),
|
|
||||||
)
|
|
||||||
)
|
|
||||||
|
|
||||||
res = await client.get("/register/admin-setup", follow_redirects=False)
|
res = await client.get(f"/register/{link.token}", 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"]
|
||||||
|
|
||||||
# Token should be marked used
|
# Token should be consumed
|
||||||
link = await magic_link_repo.get_by_token("admin-setup")
|
assert await magic_link_service.validate(link.token) is None
|
||||||
assert link is not None
|
|
||||||
assert link.used is True
|
|
||||||
|
|
||||||
# Original user should still exist with original groups
|
# Original user should still exist with original groups
|
||||||
existing = await user_repo.get_by_username("admin")
|
existing = await user_repo.get_by_username("admin")
|
||||||
|
|
|
||||||
|
|
@ -42,6 +42,18 @@ async def test_create_returns_magic_link(service: MagicLinkService) -> None:
|
||||||
assert link.expires_at > datetime.now(UTC)
|
assert link.expires_at > datetime.now(UTC)
|
||||||
|
|
||||||
|
|
||||||
|
async def test_token_stored_hashed_not_plaintext(service: MagicLinkService, repo: SQLiteMagicLinkRepository) -> None:
|
||||||
|
# The raw token handed to the user must never be the value persisted in
|
||||||
|
# the store; a DB read must not yield a usable token.
|
||||||
|
link = await service.create(username="alice")
|
||||||
|
raw = link.token
|
||||||
|
assert await repo.get_by_token(raw) is None
|
||||||
|
# ...but the raw token must still validate through the service.
|
||||||
|
validated = await service.validate(raw)
|
||||||
|
assert validated is not None
|
||||||
|
assert validated.username == "alice"
|
||||||
|
|
||||||
|
|
||||||
async def test_create_generates_unique_tokens(service: MagicLinkService) -> None:
|
async def test_create_generates_unique_tokens(service: MagicLinkService) -> None:
|
||||||
link1 = await service.create(username="alice")
|
link1 = await service.create(username="alice")
|
||||||
link2 = await service.create(username="bob")
|
link2 = await service.create(username="bob")
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue