Fix audit findings: lost updates, token signing, dependency advisories
Blocking: - Stock adjustments read-modified-wrote outside a transaction, so concurrent changes silently overwrote each other. Verified: 50 concurrent -1 requests moved a quantity of 100 to 99 rather than 50, while all 51 history rows were written, leaving the ledger disagreeing with the stock. Both adjust and patch now take SQLite's write lock up front. - Session tokens joined payload and HMAC with "." and split on the last occurrence. A raw digest can contain that byte, so ~12% of issued tokens failed their own validator (measured 121/1000). The digest is fixed width; slice by length instead. - starlette 0.41.3 and python-multipart 0.0.20 carried 15 advisories between them, including a FileResponse Range-header DoS reachable through the public static assets. Pinned starlette explicitly; python-multipart was unused. Also: - PATCH quantity now writes history, and a floored adjustment logs the delta it applied rather than the one requested, so the log sums to the stock. - Renaming or deleting a category or location rebuilds the search index for every part beneath it; full paths are indexed, so "Workshop" finds Bin A3. - Blank names, negative quantities and explicit nulls on NOT NULL columns are 422s instead of silent writes or 500s; taxonomy routes 404 on missing ids, 409 on duplicates, and reject indirect parent cycles. - Security headers, HSTS behind X-Forwarded-Proto, Secure cookie via the forwarded scheme, content-hashed asset URLs so Cloudflare cannot serve stale frontend code, and a global failed-login throttle. - README documents a WAL-safe backup; cp of parts.db alone could lose commits. Checks go from 68 to 134, including a suite that runs against a real server because lost updates only appear when requests genuinely overlap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
+90
-7
@@ -9,17 +9,26 @@ import base64
|
||||
import hashlib
|
||||
import hmac
|
||||
import os
|
||||
import threading
|
||||
import time
|
||||
|
||||
from fastapi import Cookie, HTTPException
|
||||
from fastapi import Cookie, HTTPException, Request
|
||||
|
||||
COOKIE_NAME = "parts_session"
|
||||
DIGEST_SIZE = hashlib.sha256().digest_size
|
||||
|
||||
|
||||
def _env(name: str, default: str = "") -> str:
|
||||
return os.environ.get(name, default).strip()
|
||||
|
||||
|
||||
def _env_int(name: str, default: int, minimum: int = 1) -> int:
|
||||
try:
|
||||
return max(minimum, int(_env(name, str(default))))
|
||||
except ValueError:
|
||||
return default
|
||||
|
||||
|
||||
def auth_enabled() -> bool:
|
||||
return _env("PARTS_AUTH", "on").lower() not in ("off", "0", "false", "no")
|
||||
|
||||
@@ -34,10 +43,7 @@ def _secret() -> bytes:
|
||||
|
||||
|
||||
def session_days() -> int:
|
||||
try:
|
||||
return max(1, int(_env("PARTS_SESSION_DAYS", "30")))
|
||||
except ValueError:
|
||||
return 30
|
||||
return _env_int("PARTS_SESSION_DAYS", 30)
|
||||
|
||||
|
||||
def check_password(candidate: str) -> bool:
|
||||
@@ -47,17 +53,27 @@ def check_password(candidate: str) -> bool:
|
||||
return hmac.compare_digest(candidate.encode(), expected.encode())
|
||||
|
||||
|
||||
# --- session tokens ---------------------------------------------------------
|
||||
#
|
||||
# The signature is raw HMAC bytes, which can contain any byte value including
|
||||
# the one for ".". An earlier version joined payload and signature with "." and
|
||||
# split on the last occurrence, so roughly 12% of issued tokens (1 - (255/256)^32)
|
||||
# split in the wrong place and failed their own validator. The digest is a fixed
|
||||
# 32 bytes, so slice by length instead of looking for a delimiter.
|
||||
|
||||
def issue_token() -> str:
|
||||
expires = int(time.time()) + session_days() * 86400
|
||||
payload = str(expires).encode()
|
||||
sig = hmac.new(_secret(), payload, hashlib.sha256).digest()
|
||||
return base64.urlsafe_b64encode(payload + b"." + sig).decode()
|
||||
return base64.urlsafe_b64encode(payload + sig).decode()
|
||||
|
||||
|
||||
def token_valid(token: str) -> bool:
|
||||
try:
|
||||
raw = base64.urlsafe_b64decode(token.encode())
|
||||
payload, sig = raw.rsplit(b".", 1)
|
||||
if len(raw) <= DIGEST_SIZE:
|
||||
return False
|
||||
payload, sig = raw[:-DIGEST_SIZE], raw[-DIGEST_SIZE:]
|
||||
expected = hmac.new(_secret(), payload, hashlib.sha256).digest()
|
||||
if not hmac.compare_digest(sig, expected):
|
||||
return False
|
||||
@@ -66,6 +82,73 @@ def token_valid(token: str) -> bool:
|
||||
return False
|
||||
|
||||
|
||||
# --- failed-login throttling ------------------------------------------------
|
||||
#
|
||||
# Deliberately a GLOBAL window rather than per-IP. Caddy appends to
|
||||
# X-Forwarded-For rather than replacing it, so the client-supplied end of that
|
||||
# header is attacker-controlled and a per-IP bucket would be trivially evaded by
|
||||
# forging it. There is exactly one legitimate user, so a global cap costs
|
||||
# nothing in practice and cannot be side-stepped by rotating source addresses.
|
||||
#
|
||||
# The trade-off: someone spraying guesses can lock the login form for the
|
||||
# cooldown. Existing sessions keep working (they carry a valid cookie), so that
|
||||
# is an annoyance rather than a lockout.
|
||||
|
||||
_failures: list[float] = []
|
||||
_failures_lock = threading.Lock()
|
||||
|
||||
|
||||
def _window() -> int:
|
||||
return _env_int("PARTS_LOGIN_WINDOW", 300)
|
||||
|
||||
|
||||
def _max_failures() -> int:
|
||||
return _env_int("PARTS_LOGIN_MAX_FAILURES", 10)
|
||||
|
||||
|
||||
def login_retry_after() -> int:
|
||||
"""Seconds until the next login attempt is allowed, or 0 if allowed now."""
|
||||
now = time.time()
|
||||
window = _window()
|
||||
with _failures_lock:
|
||||
recent = [t for t in _failures if now - t < window]
|
||||
_failures[:] = recent
|
||||
if len(recent) < _max_failures():
|
||||
return 0
|
||||
return max(1, int(window - (now - recent[0])))
|
||||
|
||||
|
||||
def record_failure():
|
||||
with _failures_lock:
|
||||
_failures.append(time.time())
|
||||
|
||||
|
||||
def clear_failures():
|
||||
with _failures_lock:
|
||||
_failures.clear()
|
||||
|
||||
|
||||
# --- request helpers --------------------------------------------------------
|
||||
|
||||
def is_https(request: Request) -> bool:
|
||||
"""Whether the *original* client request used TLS.
|
||||
|
||||
Caddy terminates TLS and proxies to this app over plain HTTP, so
|
||||
`request.url.scheme` is always "http" in production and can't decide whether
|
||||
the session cookie may carry the Secure flag. X-Forwarded-Proto is what the
|
||||
proxy records the real scheme in.
|
||||
"""
|
||||
override = _env("PARTS_SECURE_COOKIE", "auto").lower()
|
||||
if override in ("on", "1", "true", "yes"):
|
||||
return True
|
||||
if override in ("off", "0", "false", "no"):
|
||||
return False
|
||||
forwarded = request.headers.get("x-forwarded-proto", "")
|
||||
if forwarded:
|
||||
return forwarded.split(",")[0].strip().lower() == "https"
|
||||
return request.url.scheme == "https"
|
||||
|
||||
|
||||
def require_auth(parts_session: str | None = Cookie(default=None)):
|
||||
"""FastAPI dependency guarding every data route."""
|
||||
if not auth_enabled():
|
||||
|
||||
Reference in New Issue
Block a user