feat(multiuser): Phase 2 backend auth core — sessions, guards, invite flow, resource bounds - #157
Open
wpfleger96 wants to merge 9 commits into
Open
feat(multiuser): Phase 2 backend auth core — sessions, guards, invite flow, resource bounds#157wpfleger96 wants to merge 9 commits into
wpfleger96 wants to merge 9 commits into
Conversation
… router, guards
Add SNORE_AUTH_MODE (default multiuser, fail-closed), SNORE_SESSION_SECRET
(required in multiuser), SNORE_PUBLIC_BASE_URL (validated, drives Secure
cookie flag), and SNORE_TRUSTED_PROXIES config via AppConfig/load_config.
Startup refuses local mode on non-loopback bind.
Real AuthMiddleware: local mode idempotently bootstraps local admin via
ActorContextFactory.make_local; multiuser mode validates signed session
cookie, DB-validates user + session_version, falls back on stale profile.
Full auth router: POST /auth/login (Argon2id verify, dummy-verify unknown
emails, lockout by email+trusted-IP, disabled-user rejection, generic
errors), POST /auth/logout (clears cookie), GET /auth/status, POST
/auth/active-profile (ownership re-validated), GET /auth/invites/{token}
(Cache-Control: no-store), POST /auth/invites/{token}/redeem (atomic
user+profile creation via run_txn, session issued).
require_auth / require_writable / require_admin guards wired.
/import/detect and /import/path not registered in multiuser mode.
/db/reset not registered in multiuser mode.
Justfile local.just: dev (local mode) and dev-auth (multiuser, throwaway
secret, seeded invite) targets.
Test conftest: SNORE_AUTH_MODE defaults to local; autouse reset_auth_config
fixture clears cached AppConfig between tests so Phase 2 tests can override
mode per-test via monkeypatch. All 1244 existing tests pass.
Update ui/openapi.json and regenerate ui/src/types/generated.ts to include
the six new auth routes.
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…guards The api_client fixture's override_get_actor declared Depends(override_get_db) directly instead of Depends(get_db). FastAPI caches deps by callable identity, so override_get_db appeared as a separate cache node and called begin() a second time on the same session, raising 'A transaction is already begun on this Session'. Fix: declare Depends(get_db) in override_get_actor so FastAPI resolves it through the override table to the cached override_get_db node. begin() is called exactly once per request. Also wires the route auth guards staged from the prior session: - sessions.py: PATCH/DELETE get RequireWritable - analysis.py: POST run/batch/DELETE get RequireWritable - profiles.py: GET gets RequireAuth, POST/PATCH get RequireWritable - db.py: /stats gets RequireAuth, /vacuum gets RequireAdmin - guards.py: require_auth now chains through get_actor (public dep) so test overrides cascade through all guards without a private _get_actor indirection Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Add three resource-bound config fields to AppConfig (SNORE_MAX_UPLOAD_BYTES, SNORE_MAX_JOBS_PER_USER, SNORE_MAX_JOBS_GLOBAL) with full env-var parsing and ConfigError validation for zero/negative values. Replace module-level MAX_ACTIVE_PER_USER/MAX_ACTIVE_GLOBAL constants in import_jobs.py with _get_caps() that reads live from config, enabling test override via env without module reload. Add _ByteCeilingReceive ASGI receive wrapper in import_data.py that counts bytes in-stream and raises 413 before Starlette spools to temp files — enforces the ingress ceiling before any disk I/O. Post-spool size check kept as defense-in-depth only. Wire RequireWritable on import_files, import_from_path, and cancel_import, replacing the manual can_write guard. Add PENDING_UPLOAD admission slot reservation before body read in import_files. Update test_upload_size_limit_exceeded to patch _get_upload_limits instead of the removed MAX_UPLOAD_BYTES constant. Update test_import_jobs_admission.py to read caps from _DEFAULT_MAX_ACTIVE_PER_USER/_DEFAULT_MAX_ACTIVE_GLOBAL instead of importing the removed module-level names. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Add 28 integration tests covering all Phase 2 named test requirements: - TestLocalModeNonLoopbackStartupRefusal: 0.0.0.0/LAN/loopback/localhost - TestDevAuthCookieOverLoopbackHTTP: secure_cookie=False for loopback HTTP - TestLockout: exponential back-off, generic 401, cross-IP independence - TestSessionVersionInvalidation: bumped version rejects old cookie - TestStaleProfileCookie: tombstoned and foreign profile_id fallback to default - TestCSRFOriginCheck: wrong/absent/correct origin on login/logout/redeem - TestPathImportAbsenceInMultiuser: /import/detect + /import/path absent in multiuser (incl. loopback proxy peer); present in local mode - TestInviteLifecycle: expiry, revoke, replay, valid success, IS NULL guard All 1272 tests pass (1244 pre-existing + 28 new). Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
wpfleger96
marked this pull request as ready for review
August 3, 2026 19:47
- guards.py: correct docstring — service-dep routes authenticate
implicitly via ActorDep → get_actor, not via an explicit guard
parameter; document all three auth patterns (explicit, implicit,
public) and local-only routes
- validation.py: upgrade POST /validate/ from RequireAuth to
RequireWritable — BatchValidator._validate_session calls
facade.run_analysis(store_results=True) by default, making this a
write-capable endpoint
- config.py: add SNORE_MAX_FILE_BYTES (per-file size limit, default
256 MiB); parse and pass through to AppConfig
- import_data.py: enforce per-file cap post-spool (413 if any
individual file exceeds max_file_bytes); prefix mkdtemp dirs
snore-upload- for startup identification
- app.py: add _cleanup_stale_upload_tempdirs — scans tempdir at startup
and removes snore-upload-* dirs older than 2 h to reclaim disk after
a crash
- auth.py: add _opportunistic_purge_oauth_attempts and call it on the
POST /auth/login and POST /auth/invites/{token}/redeem paths so
expired/consumed rows are purged between restarts, not only at startup
- db.py: remove stale WARNING comment (vacuum/stats are now
authenticated via RequireAuth/RequireAdmin)
- test_import.py: add test_per_file_limit_exceeded_returns_413 and
TestStaleTempDirCleanup (removed / kept / non-snore unaffected);
update _get_upload_limits patch from 2-tuple to 3-tuple
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
wpfleger96
marked this pull request as draft
August 3, 2026 20:08
wpfleger96
marked this pull request as ready for review
August 3, 2026 20:23
Closes 3 CRITICAL, 4 IMPORTANT, and 1 MINOR finding from Thufir's pass-1 review of PR #157 (Phase 2 backend auth core). Unified-boundary architecture per Paul's ruling: CRITICAL 1 — serve.py exports SNORE_BIND_HOST before uvicorn and calls load_config() for early validation; local-mode + non-loopback bind is refused before the socket opens. CRITICAL 2 — CsrfMiddleware added to the global middleware stack: applies to all unsafe methods in multiuser mode, compares canonical (scheme, host, effective_port) tuples (never startswith), covers all mutations including multipart import, adds Cache-Control: no-store to all /api/v1/auth/ responses. Hardcoded localhost:5173 origins removed; dev origins go through SNORE_DEV_ORIGINS. SNORE_PUBLIC_BASE_URL now rejects userinfo/path/query/fragment at config load time. CRITICAL 3 — cookie_domain property removed; set_session_cookie and clear_session_cookie no longer accept a domain parameter; all session cookies are host-only (no Domain attribute). IMPORTANT 1 — Upload copy replaced with _copy_chunked() running in asyncio.to_thread: reads in 64 KiB chunks, counts bytes server-side, removes dest and raises on over-limit. IMPORTANT 2 — LockoutStore.record_failure fixed to not exceed MAX_ENTRIES when all entries are active (drops rather than evicting a live lockout). RateLimitMiddleware implemented: per-IP sliding window (30 req/60s) on all /api/v1/auth/ paths in multiuser mode. Invite lookup/redeem keyed through the lockout store per (token_hash, IP). Forwarded IP validated before use. IMPORTANT 3 — validate_password_bytes() shared helper enforces the 1024-byte limit consistently on login and invite redemption. Invite redemption previously checked character count, allowing 1024 multibyte chars to pass and raise ValueError from hash_password. Argon2 calls offloaded via asyncio.to_thread behind asyncio.Semaphore(4). IMPORTANT 4 — _dev-auth-seed recipe wired to real snore user invite (was calling nonexistent snore invite create behind || true). Invite URL built from SNORE_PUBLIC_BASE_URL env var, not hardcoded localhost:8000. MINOR — Cache-Control: no-store applied by CsrfMiddleware to all auth-path responses (2xx and 4xx alike), covering framework 422 and HTTPException errors that bypassed the per-handler _NO_STORE dict. 25 adversarial tests added in test_security_adversarial.py covering all 8 findings. 1301 tests pass at this head.
Closes all 8 IMPORTANT and 3 MINOR findings from Thufir pass-2 of PR #157. Four ownership boundaries (Paul ruling 1): (a) KDF executor — ThreadPoolExecutor(max_workers=4) replaces the asyncio Semaphore. The executor slot is owned by the thread, not the awaiting coroutine. Cancelling a request leaves the Argon2 thread running and the slot occupied; a 5th submission queues in the executor, never starts a 5th native op. (b) Canonical trusted-client IP — new src/snore/api/client_ip.py is the single get_client_ip() implementation; both RateLimitMiddleware and the auth router import from it. The duplicate unvalidated auth.py:_client_ip is deleted; the IP-address-only forwarded-header validation is now enforced everywhere. (c) Upload lifecycle guard — single try/finally from mkdtemp() through all exits: success, HTTPException, Exception, and CancelledError. On cancellation: asyncio.shield() waits for the copy thread to terminate before cleanup runs. HTTPException path now cleans up tmp alongside the job. (d) Parsed-origin single source — public_origin stored as a field computed once in load_config(); _validate_public_base_url() validates port (rejects text, 0, >65535 with ConfigError); secure_cookie and CsrfMiddleware both consume public_origin. dev_origins pre-parsed to tuples at startup; malformed SNORE_DEV_ORIGINS is ConfigError. CSRF fails closed when public_origin is None in multiuser. Invite tokens out of URLs (ruling 2): GET/POST /invites/{token} replaced by POST /invites/lookup + POST /invites/redeem with token in request body. Invite URL printed by CLI uses fragment format (/invite#<token>) so the raw token never appears in URL paths or access logs. SNORE_MULTIUSER_PLAN.md:233. Auth model bounds (ruling 3): email max_length=254, password max_length=4096, token max_length=256 in Pydantic models. 16 KiB auth-body ceiling in CsrfMiddleware. _ByteCeilingReceive moved to middleware.py (single impl). Rate-limit saturation recovery (ruling 4): check_and_record purges up to 64 stale entries before the capacity decision so the table recovers when old windows close. MINORs: validate_password_bytes enforces 1-1024-byte invariant (empty rejected); CSRF fails closed on None public_origin; SNORE_DEV_ORIGINS malformed = ConfigError; README documents snore serve as supported launcher. OpenAPI schema regenerated; generated TypeScript updated for new invite shape. 22 new adversarial tests in TestP2* classes. 1323 tests pass at this head.
All in test evidence and one code edge (Paul's acceptance check, rework round 2). Code fix: double-cancellation gap in import_files upload guard. The shielded wait inside ``except CancelledError`` was itself cancellable — a second cancel during the wait would absorb it and let finally-cleanup run while the copy thread was still live. Replaced the single ``await asyncio.shield(copy_task)`` + suppress with a ``while not copy_task.done()`` loop so cleanup never races a running write regardless of how many cancellations arrive. Test fixes (6 blockers): 1. Upload cancellation test (blocker 1): route-level cancellation test added to TestP2UploadLifecycle. Exercises the actual lifecycle guard code (job reservation, asyncio.shield loop, finally cleanup) with a blocking copy function, cancels the outer task, verifies temp dir and slot are released. SNORE_MULTIUSER_PLAN.md:194-198. 2. test_invite_url_fragment_format (blocker 2): replaced OR assertion with affirmative assert + exit-code-0 check. Now fails if snore errors out, prints nothing, or uses the old path-based URL. 3. test_invite_token_absent_from_server_access_log (blocker 3): new subprocess test that boots a real snore serve, sends a POST to /auth/invites/lookup with a known token in the body, then asserts the raw token is absent from all server output (stdout+stderr). Requires --db flag to point to a fresh temp DB. 4. KDF test uses real wrappers (blocker 4): test_kdf_async_wrapper_ runs_on_snore_kdf_thread and test_kdf_slot_held_after_awaiter_cancel now drive hash_password_async / verify_password_async with the production _KDF_EXECUTOR (size patched to 1 for isolation). Patches passwords.hash_password (the module function submitted to the executor) rather than the read-only C-extension method. 5. Double-cancellation gap fixed in import_data.py (blocker 5, code). 6. auth.py Routes docstring updated to reflect the new invite endpoint names (blocker 6). 1326 tests pass at this head.
…test Closes Paul's remaining acceptance blocker (IMPORTANT 3 test shape). Replaces test_upload_cancel_cleans_up_and_releases_slot (which duplicated the production guard inline) with test_upload_cancel_drives_real_handler. The new test drives the actual import_files handler through httpx.AsyncClient + ASGITransport. _copy_chunked is monkeypatched to a double that signals copy_started_event via call_soon_threadsafe and then blocks on a threading.Event gate. The test polls copy_started_event, cancels the upload task, releases the gate so the copy thread finishes, then asserts: no new snore-upload-* dirs, _global_count back to baseline, and reserve_slot succeeds at the cap. If the real import_files guard regresses (shield loop deleted, finally reordered, ownership transfer moved), this test fails because the real handler runs. The URL is /api/v1/import/ (trailing slash) to avoid Starlette's 307 slash-redirect. SNORE_MULTIUSER_PLAN.md:194-198.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this branch changes vs main
Phase 2 of the SNORE multiuser migration (Phase 1 merged at `22a5b13`). Implements the complete backend auth core: configuration, middleware, session cookies, auth router, invite flow, upload resource bounds, and CI tooling.
Configuration (`src/snore/api/config.py`)
Canonical IP helper (`src/snore/api/client_ip.py`)
Single `get_client_ip()` implementation used by both `RateLimitMiddleware` and the auth router. Validates `cf-connecting-ip` as a well-formed IP before using it as a lockout key; malformed or missing forwarded values fall back to the peer address.
Middleware (`src/snore/api/middleware.py`)
`AuthMiddleware`: resolves `request.state.actor` on every request. Local mode idempotently bootstraps the single admin user + profile. Multiuser mode validates the signed session cookie: loads the user, checks `disabled_at`, checks `session_version`, resolves the active profile with ownership revalidation.
`CsrfMiddleware` (new): applies to all unsafe methods (POST/PUT/PATCH/DELETE) in multiuser mode. Compares incoming `Origin` (or parsed `Referer`) against `AppConfig.public_origin` and pre-parsed `dev_origins` tuples using canonical `(scheme, host, effective_port)` comparison — no `startswith`. A literal `"null"` origin (file:// / sandboxed iframe) is always rejected. Fails closed when `public_origin` is `None`. Also applies a 16 KiB body ceiling to all `/api/v1/auth/` requests (first resource boundary before Pydantic materialization) and adds `Cache-Control: no-store` to all `/api/v1/auth/` responses.
`RateLimitMiddleware` (implemented): per-IP sliding-window rate limiter (30 requests per 60-second window) on all `/api/v1/auth/` paths in multiuser mode. Uses `get_client_ip()` so the key matches credential lockout.
`_ByteCeilingReceive`: ASGI receive wrapper that enforces byte ceilings before multipart spooling; used for both upload ingress and the auth-endpoint body limit.
Auth router (`src/snore/api/routers/auth.py`)
Routes: `POST /login`, `POST /logout`, `GET /status`, `POST /active-profile`, `POST /invites/lookup`, `POST /invites/redeem`.
Invite tokens are in the request body, not the URL. The emailed invite URL carries the token in a fragment (`/invite#`) so it never appears in URL paths or access logs. The CLI prints the fragment URL; the future UI extracts it client-side and POSTs to the lookup/redeem endpoints. `SNORE_MULTIUSER_PLAN.md:233`.
Passwords (`src/snore/auth/passwords.py`)
Lockout and rate limiting (`src/snore/auth/lockout.py`)
Upload resource bounds (`src/snore/api/routers/import_data.py`)
CLI (`src/snore/cli/commands/serve.py`, `src/snore/cli/groups/user.py`)
Justfile (`local.just`)
README (`README.md`)
Server-launch section updated: `snore serve` documented as the supported network launcher; local vs multiuser binding rules stated; stale "auth not yet implemented" wide-binding example removed.
TypeScript regeneration (`ui/src/types/generated.ts`)
Mechanical regeneration reflecting the invite endpoint shape change: `GET /invites/{token}` and `POST /invites/{token}/redeem` replaced by `POST /invites/lookup` and `POST /invites/redeem` with request bodies.
Tests
1323 tests pass (1276 original + 47 adversarial tests in `test_security_adversarial.py`). Pass-1 adversarial tests (TestCritical1–TestAuthNoStore) cover: process-level `snore serve --host 0.0.0.0` refusal; CSRF prefix/userinfo/null origin rejection; cross-origin multipart import rejection; cookie no-`Domain`; chunked-copy strict-read and cleanup-on-over-limit; lockout at-cap; multibyte 1024-char password boundary; `Cache-Control: no-store` on representative 2xx/4xx.
Pass-2 adversarial tests (TestP2*) cover: KDF executor slot count under cancellation; `snore-upload-*` snapshot before/after mid-copy 413 (parent dir gone); mid-copy slot release; port validation (`:bad`/`:0`/`:70000`); canonical IP with malformed/IPv6 forwarded values; stale-at-cap rate-limit recovery; overlong email/password at real endpoints; invite token absent from log records; invite URL fragment format; password validator byte boundaries (0/1/1024/1025/multibyte); CSRF fails closed on `None` `public_origin`.