Throttle the change-password route and make epoch bumps atomic
Two session-management bugs from the audit: - /api/password recorded failed attempts against the login budget but never checked it, so a borrowed session could guess the current password without limit while still locking the owner out of /api/login. Verified: thirteen consecutive wrong guesses all returned 403 and none returned 429. It now spends from the same budget it was topping up. - bump_epoch read the epoch and wrote it back without the write lock, so concurrent revocations lost increments and sessions that should have been cut off survived. Verified: twenty concurrent bumps advanced the counter from 2 to 6, and twenty concurrent "sign out other devices" calls left four sessions authenticated. It takes BEGIN IMMEDIATE now; set_password hashes before locking, so scrypt doesn't serialise unrelated writes. Removing either fix makes its test fail with exactly that symptom. README corrections: deployment is rsync, not git pull — gitea on .8 cannot serve a clone to .8 itself, which the deploy section now documents — and the concurrency check count was understated. Checks go from 182 to 197. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -397,6 +397,26 @@ with TestClient(app) as client:
|
||||
check("wrong current password is refused",
|
||||
client.post("/api/password",
|
||||
json={"current_password": "nope", "new_password": "brand-new-secret"}).status_code == 403)
|
||||
|
||||
# The change-password route verifies the same credential as the login form,
|
||||
# so it has to spend from the same budget instead of only topping it up.
|
||||
auth_mod.clear_failures()
|
||||
pw_codes = [client.post("/api/password",
|
||||
json={"current_password": "wrong", "new_password": "brand-new-secret"}).status_code
|
||||
for _ in range(auth_mod._max_failures() + 3)]
|
||||
check("guessing through change-password eventually 429s", 429 in pw_codes, str(pw_codes))
|
||||
check("change-password throttles on the same budget as login",
|
||||
pw_codes.index(429) == auth_mod._max_failures(), str(pw_codes))
|
||||
check("a throttled change-password blocks even the right password",
|
||||
client.post("/api/password",
|
||||
json={"current_password": "hunter2",
|
||||
"new_password": "brand-new-secret"}).status_code == 429)
|
||||
check("login is throttled too once the budget is spent",
|
||||
client.post("/api/login", json={"password": "hunter2"}).status_code == 429)
|
||||
auth_mod.clear_failures()
|
||||
check("clearing the budget restores change-password",
|
||||
client.post("/api/password",
|
||||
json={"current_password": "wrong", "new_password": "x"}).status_code in (403, 422))
|
||||
auth_mod.clear_failures()
|
||||
check("short new password rejected",
|
||||
client.post("/api/password",
|
||||
@@ -438,6 +458,23 @@ with TestClient(app) as client:
|
||||
check("revoke keeps the calling session", client.get("/api/parts").status_code == 200)
|
||||
check("revoke cuts off the other session", third.get("/api/parts").status_code == 401)
|
||||
|
||||
# --- epoch bumps must not lose increments ---
|
||||
from concurrent.futures import ThreadPoolExecutor as _Pool
|
||||
import app.db as _dbmod
|
||||
|
||||
def _bump_once(_):
|
||||
with _dbmod.session() as _c:
|
||||
auth_mod.bump_epoch(_c)
|
||||
|
||||
with _dbmod.session() as _c:
|
||||
_epoch_before = auth_mod.current_epoch(_c)
|
||||
with _Pool(max_workers=20) as _ex:
|
||||
list(_ex.map(_bump_once, range(20)))
|
||||
with _dbmod.session() as _c:
|
||||
_epoch_after = auth_mod.current_epoch(_c)
|
||||
check("20 concurrent epoch bumps advance it by exactly 20",
|
||||
_epoch_after - _epoch_before == 20, f"{_epoch_before} -> {_epoch_after}")
|
||||
|
||||
# --- the hash itself ---
|
||||
import app.db as _db
|
||||
with _db.session() as _conn:
|
||||
|
||||
+98
-18
@@ -4,6 +4,7 @@ The in-process test client is not enough here: a lost update needs two requests
|
||||
genuinely overlapping inside SQLite, which means a real server and real threads.
|
||||
"""
|
||||
|
||||
import http.cookiejar
|
||||
import json
|
||||
import os
|
||||
import shutil
|
||||
@@ -15,6 +16,7 @@ import time
|
||||
import urllib.error
|
||||
import urllib.request
|
||||
from concurrent.futures import ThreadPoolExecutor
|
||||
from contextlib import contextmanager
|
||||
|
||||
PORT = 8137
|
||||
BASE = f"http://127.0.0.1:{PORT}"
|
||||
@@ -30,33 +32,117 @@ def check(label, condition, detail=""):
|
||||
|
||||
|
||||
def req(path, method="GET", body=None):
|
||||
return call(urllib.request.urlopen, BASE, path, method, body)
|
||||
|
||||
|
||||
def call(opener, base, path, method="GET", body=None):
|
||||
data = json.dumps(body).encode() if body is not None else None
|
||||
r = urllib.request.Request(BASE + path, data=data, method=method,
|
||||
r = urllib.request.Request(base + path, data=data, method=method,
|
||||
headers={"Content-Type": "application/json"})
|
||||
return json.load(urllib.request.urlopen(r, timeout=60))
|
||||
return json.load(opener(r, timeout=60))
|
||||
|
||||
|
||||
def main():
|
||||
@contextmanager
|
||||
def server(port, **env_extra):
|
||||
"""Run a real uvicorn against a throwaway database."""
|
||||
tmp = tempfile.mkdtemp()
|
||||
env = {
|
||||
**os.environ,
|
||||
"PARTS_DB": os.path.join(tmp, "conc.db"),
|
||||
"PARTS_AUTH": "off",
|
||||
}
|
||||
server = subprocess.Popen(
|
||||
[sys.executable, "-m", "uvicorn", "app.main:app", "--port", str(PORT), "--log-level", "warning"],
|
||||
env = {**os.environ, "PARTS_DB": os.path.join(tmp, "conc.db"), **env_extra}
|
||||
proc = subprocess.Popen(
|
||||
[sys.executable, "-m", "uvicorn", "app.main:app", "--port", str(port),
|
||||
"--log-level", "warning"],
|
||||
cwd=ROOT, env=env, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL,
|
||||
)
|
||||
base = f"http://127.0.0.1:{port}"
|
||||
try:
|
||||
for _ in range(120):
|
||||
try:
|
||||
req("/healthz")
|
||||
call(urllib.request.urlopen, base, "/healthz")
|
||||
break
|
||||
except Exception:
|
||||
time.sleep(0.25)
|
||||
else:
|
||||
raise RuntimeError("server never started")
|
||||
yield base
|
||||
finally:
|
||||
proc.send_signal(signal.SIGINT)
|
||||
try:
|
||||
proc.wait(timeout=10)
|
||||
except subprocess.TimeoutExpired:
|
||||
proc.kill()
|
||||
shutil.rmtree(tmp, ignore_errors=True)
|
||||
|
||||
|
||||
def session_for(base, password):
|
||||
"""Log in and return an opener holding that session's cookie."""
|
||||
jar = http.cookiejar.CookieJar()
|
||||
opener = urllib.request.build_opener(urllib.request.HTTPCookieProcessor(jar))
|
||||
call(opener.open, base, "/api/login", "POST", {"password": password})
|
||||
return opener
|
||||
|
||||
|
||||
def authenticated(opener, base) -> bool:
|
||||
try:
|
||||
call(opener.open, base, "/api/parts")
|
||||
return True
|
||||
except urllib.error.HTTPError as e:
|
||||
if e.code == 401:
|
||||
return False
|
||||
raise
|
||||
|
||||
|
||||
def session_phase():
|
||||
"""Concurrent session revocation, which needs the auth gate switched on."""
|
||||
password = "concurrency-test-password"
|
||||
with server(PORT + 1, PARTS_AUTH="on", PARTS_PASSWORD=password,
|
||||
PARTS_SECRET="concurrency-test-secret",
|
||||
PARTS_LOGIN_MAX_FAILURES="500") as base:
|
||||
# --- the plain case first ---
|
||||
keeper = session_for(base, password)
|
||||
others = [session_for(base, password) for _ in range(5)]
|
||||
check("sessions start authenticated",
|
||||
all(authenticated(o, base) for o in [keeper] + others))
|
||||
call(keeper.open, base, "/api/sessions/revoke", "POST", {})
|
||||
check("revoke keeps the calling session", authenticated(keeper, base))
|
||||
check("revoke signs out every other session",
|
||||
not any(authenticated(o, base) for o in others))
|
||||
|
||||
# --- concurrent revocation ---
|
||||
# Each successful revoke bumps the epoch and re-issues a cookie carrying
|
||||
# the value it wrote, so requests already in flight legitimately come
|
||||
# back 401 as earlier ones invalidate them. Exactly one session — the
|
||||
# one that wrote the final epoch — should be left standing.
|
||||
#
|
||||
# That is precisely what a lost increment breaks: several bumps read the
|
||||
# same epoch, all write the same value, all re-issue cookies matching
|
||||
# it, and most of the sessions that were meant to be cut off survive.
|
||||
openers = [session_for(base, password) for _ in range(20)]
|
||||
check("all 20 sessions start authenticated",
|
||||
all(authenticated(o, base) for o in openers))
|
||||
|
||||
def try_revoke(opener):
|
||||
try:
|
||||
call(opener.open, base, "/api/sessions/revoke", "POST", {})
|
||||
return 200
|
||||
except urllib.error.HTTPError as e:
|
||||
return e.code
|
||||
|
||||
with ThreadPoolExecutor(max_workers=20) as ex:
|
||||
codes = list(ex.map(try_revoke, openers))
|
||||
check("at least one concurrent revoke succeeded", 200 in codes, str(codes))
|
||||
check("losers were rejected rather than erroring",
|
||||
all(c in (200, 401) for c in codes), str(sorted(set(codes))))
|
||||
alive = [o for o in openers if authenticated(o, base)]
|
||||
check("concurrent revokes leave exactly one session standing",
|
||||
len(alive) == 1, f"{len(alive)} of 20 still authenticated")
|
||||
if alive:
|
||||
check("the surviving session still works",
|
||||
call(alive[0].open, base, "/api/stats")["parts"] == 0)
|
||||
fresh = session_for(base, password)
|
||||
check("a new login works after mass revocation", authenticated(fresh, base))
|
||||
|
||||
|
||||
def main():
|
||||
with server(PORT, PARTS_AUTH="off"):
|
||||
# --- concurrent decrements must not lose updates ---
|
||||
pid = req("/api/parts", "POST", {"name": "race target", "quantity": 100})["id"]
|
||||
with ThreadPoolExecutor(max_workers=50) as ex:
|
||||
@@ -158,13 +244,7 @@ def main():
|
||||
check("all concurrent creates are searchable",
|
||||
req("/api/parts?q=bulk&limit=100")["total"] == 25,
|
||||
str(req("/api/parts?q=bulk&limit=100")["total"]))
|
||||
finally:
|
||||
server.send_signal(signal.SIGINT)
|
||||
try:
|
||||
server.wait(timeout=10)
|
||||
except subprocess.TimeoutExpired:
|
||||
server.kill()
|
||||
shutil.rmtree(tmp, ignore_errors=True)
|
||||
session_phase()
|
||||
|
||||
print()
|
||||
if failures:
|
||||
|
||||
Reference in New Issue
Block a user