From d76642f85d18df16277f49aa737e1a4daa6f4574 Mon Sep 17 00:00:00 2001 From: Jay Date: Mon, 24 Aug 2026 11:04:52 -0400 Subject: [PATCH] Throttle the change-password route and make epoch bumps atomic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- README.md | 40 +++++++++---- app/auth.py | 10 +++- app/main.py | 10 ++++ tests/test_api.py | 37 ++++++++++++ tests/test_concurrency.py | 116 ++++++++++++++++++++++++++++++++------ 5 files changed, 183 insertions(+), 30 deletions(-) diff --git a/README.md b/README.md index 05aeffb..619f37e 100644 --- a/README.md +++ b/README.md @@ -45,22 +45,24 @@ Then open http://127.0.0.1:8123. ## Tests ```sh -.venv/bin/python -m tests.test_api # 166 checks, in-process -.venv/bin/python -m tests.test_concurrency # 15 checks, against a real uvicorn +.venv/bin/python -m tests.test_api # 172 checks, in-process +.venv/bin/python -m tests.test_concurrency # 25 checks, against a real uvicorn ``` `test_api` exercises the API end to end against a throwaway database — the auth gate and session-token signing, nested categories and locations, search across every indexed field, filter rollups, stock adjustment and history, patch semantics including explicit nulls, taxonomy cycle rejection, security headers -and asset versioning, login throttling, and the full password-management flow -including scrypt hashing, session invalidation and the recovery CLI. +and asset versioning, login throttling on both the login and change-password +routes, and the full password-management flow including scrypt hashing, session +invalidation and the recovery CLI. `test_concurrency` needs a real server process, because a lost update only shows up when two requests genuinely overlap inside SQLite. It fires overlapping adjustments, patches and creates at one part and asserts the stock log always -sums to the stored quantity, and races taxonomy renames against reads to check -the search index never describes a name the tree no longer has. +sums to the stored quantity; races taxonomy renames against reads to check the +search index never describes a name the tree no longer has; and races session +revocations to check no epoch increment is lost. ## Configuration @@ -100,15 +102,31 @@ Lives at `/home/jay/srv/parts/` on `.8` and follows the same conventions as the other services there — `build: .`, external `caddy_web` network, named volume for `/data`. -`~/srv/parts` is a checkout of this repository, so deploying is a pull: +`~/srv/parts` is a checkout of this repository — `git log` there tells you +exactly what is deployed — but **deploys go over rsync, not `git pull`**: ```sh -ssh jay@192.168.50.8 -cd ~/srv/parts && git pull && docker compose up -d --build +rsync -az --delete \ + --exclude '.venv' --exclude 'data' --exclude '__pycache__' --exclude '*.pyc' \ + --exclude '.env' --exclude '.env.bak-*' \ + ./ jay@192.168.50.8:/home/jay/srv/parts/ +ssh jay@192.168.50.8 'cd ~/srv/parts && docker compose up -d --build' ``` -`.env` is untracked and gitignored, so it survives the pull. `~/srv` (the infra -repo) ignores `parts/`, since this directory is its own repository. +`.env` is excluded from the transfer, and rsync's `--delete` leaves excluded +files alone, so the deployed credentials survive. + +The reason it isn't `git pull`: **gitea on `.8` cannot serve a clone or fetch to +`.8` itself.** Auth succeeds and gitea logs `git-upload-pack ... 200 OK`, then +the transfer dies with `fetch-pack: unexpected disconnect while reading sideband +packet`. Reproduced against both `git.tjm77.com:2222` and `127.0.0.1:2222`, with +`--depth 1`, with protocol v0, and with fsck disabled; `git ls-remote` succeeds +every time, and cloning the same repo from a laptop works. Undiagnosed. Until +it's fixed, the checkout in `~/srv/parts` is placed there by rsyncing the +working tree *including* `.git`, which is why `git status` there is clean. + +`~/srv` (the infra repo) ignores `parts/`, since this directory is its own +repository. The database is in the `parts_parts_data` docker volume, which survives rebuilds. diff --git a/app/auth.py b/app/auth.py index ea9c9a4..d328588 100644 --- a/app/auth.py +++ b/app/auth.py @@ -115,7 +115,11 @@ def check_password(conn, candidate: str) -> bool: def set_password(conn, new_password: str): """Store a new password and invalidate every outstanding session.""" - db.set_setting(conn, PASSWORD_KEY, hash_password(new_password)) + # Hash before taking the lock: scrypt is deliberately slow, and holding + # SQLite's write lock across it would serialise unrelated writes. + hashed = hash_password(new_password) + db.begin_immediate(conn) + db.set_setting(conn, PASSWORD_KEY, hashed) bump_epoch(conn) @@ -141,6 +145,10 @@ def current_epoch(conn) -> int: def bump_epoch(conn) -> int: + # Read-modify-write, so it needs the write lock across both halves. Without + # it, concurrent "sign out other devices" calls read the same epoch and + # overwrite each other, and sessions that should have been cut off survive. + db.begin_immediate(conn) epoch = current_epoch(conn) + 1 db.set_setting(conn, EPOCH_KEY, epoch) return epoch diff --git a/app/main.py b/app/main.py index c4f72d3..5ead433 100644 --- a/app/main.py +++ b/app/main.py @@ -367,6 +367,16 @@ async def change_password( if not auth.auth_enabled(): raise HTTPException(400, "Authentication is disabled, so there is no password to change") + # This route verifies the same credential the login form does, so it has to + # honour the same budget. It was recording failures without checking them, + # which meant a borrowed session could guess the password without limit — + # while still locking the owner out of /api/login. + retry_after = auth.login_retry_after() + if retry_after: + raise HTTPException( + 429, "Too many failed attempts", headers={"Retry-After": str(retry_after)} + ) + if not auth.check_password(conn, body.current_password): auth.record_failure() await asyncio.sleep(0.5) diff --git a/tests/test_api.py b/tests/test_api.py index ae0c5b1..042f72e 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -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: diff --git a/tests/test_concurrency.py b/tests/test_concurrency.py index 07cd934..e262153 100644 --- a/tests/test_concurrency.py +++ b/tests/test_concurrency.py @@ -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: