From 3d9ff1c7a70f14b3cd6505a14cb6bd2eb80bca19 Mon Sep 17 00:00:00 2001 From: Jayakrishna Konda Date: Tue, 20 Oct 2026 03:38:44 -0400 Subject: [PATCH] Remove the AI build specs and session report from the repo MISSION.md, MISSION_settings_ui.md and REPORT.md were the build orders and progress log of the autonomous Claude Code sessions that built Phase 1. They are working files, not project docs, and should not ship. They stay on disk and are now ignored. The README pointed at REPORT.md twice; those links now go to SECURITY.md and CHANGELOG.md, and a code comment in api.ts no longer cites MISSION.md. --- .gitignore | 4 + MISSION.md | 234 ------------- MISSION_settings_ui.md | 81 ----- README.md | 7 +- REPORT.md | 747 ----------------------------------------- web/src/types/api.ts | 2 +- 6 files changed, 8 insertions(+), 1067 deletions(-) delete mode 100644 MISSION.md delete mode 100644 MISSION_settings_ui.md delete mode 100644 REPORT.md diff --git a/.gitignore b/.gitignore index 617dafc..0130d9f 100644 --- a/.gitignore +++ b/.gitignore @@ -48,3 +48,7 @@ Thumbs.db # Editors .vscode/ .idea/ + +# AI build specs and session reports: kept locally, never committed +MISSION*.md +REPORT.md diff --git a/MISSION.md b/MISSION.md deleted file mode 100644 index 749cde1..0000000 --- a/MISSION.md +++ /dev/null @@ -1,234 +0,0 @@ -# Mission: Cadence - -This file is the build command for this project. It is written so it can be handed verbatim -to an autonomous Claude Code session (local or cloud) and produce the same product. It is also -the acceptance contract each phase's output is judged against. Phases are sequential and each -must reach the "Definition of done" bar below before the next one starts. - -## Mission statement - -Build, polish, verify, and ship Cadence: a self-hostable, single-track music request service. -It is the Overseerr/Ombi pattern (user searches, requests, admin approves, backend fulfills) -applied to individual songs instead of movies, shows, or whole albums/artists. The entire -reason this exists: every existing tool in this space (Lidarr, Ombi, Headphones) is built -around artist/album as the unit of work, and pulls whole discographies once an artist is -monitored. Cadence's non-negotiable premise is that requesting one song never touches, grabs, -or monitors anything beyond that one recording. - -Work autonomously, make decisions, record them, and finish each phase. Finish every phase with -an update to REPORT.md at the repo root (append a dated section per phase, do not overwrite -prior phases' entries). - -## Operator context (public-safe, never embed any of it) - -The operator self-hosts about 40 Docker services behind Nginx Proxy Manager under his own -domain, on one amd64 host (Ubuntu) and one arm64 Oracle A1 VPS, so published Docker images -must be multi-arch (linux/amd64 and linux/arm64). He already runs Prowlarr (indexer -aggregator, proxied) and qBittorrent (behind a VPN container, proxied), both reachable over -his internal Docker network. He tried Ombi as a Lidarr-backed music request tool and it did -not solve this problem, because Ombi's music requests still resolve through Lidarr, which -still pulls full artist/album data once anything is monitored. He has not yet deployed slskd -(Soulseek daemon); when he does, it will need its own dedicated Soulseek account (the network -allows one session per account) and a read-only share of his existing library (some indexers -silently throttle or ban accounts that only leech). Never hardcode his URLs, tokens, indexer -names, or account credentials. All of it is runtime configuration supplied by whoever deploys -the app, documented as environment variables with a .env.example. - -His music library spans many languages, most heavily English, Hindi, and Telugu, with -Malayalam, Punjabi, Korean, and others also present. MusicBrainz and Spotify catalog coverage -is materially worse for regional film/devotional music than for English/K-pop; iTunes/Apple's -keyless search API has historically had decent Indian film catalog coverage. Search must -degrade gracefully per source rather than treating any one source as authoritative. - -## Product spec - -### Phase 1 (build this first, ship it, prove it end to end): Backend + Web client - -1. **Multi-source search, presented as tabs**, queried in parallel per request: - - Apple/iTunes Search API (keyless, no auth, primary source for regional coverage) - - MusicBrainz (recording search, keyless but rate-limited to 1 req/sec per their policy, - used for canonical metadata/tagging once a track is picked) - - Spotify (Client Credentials OAuth flow, app-only, no per-user login required for this - tab; register one app under the operator's own Spotify developer account) - - Prowlarr (search across the operator's configured indexers directly via Prowlarr's own - `/api/v1/search` endpoint; raw release results, filename-derived, not canonical metadata) - - Every tab must independently succeed, fail, or return empty without blocking the others. - A single slow or dead source degrades that one tab, never the whole search. - - A later, explicitly optional Spotify _personalization_ tab: if a user has connected their - own Spotify account (separate user-level OAuth, Authorization Code flow, opt-in), surface - their saved tracks/playlists as a quick-pick source. If not connected, that tab shows a - "Connect Spotify" prompt instead of results. This is additive to, never a replacement for, - the app-only Spotify search tab above. -2. **Request flow**: user searches, picks the correct result (title, artist, album art, - source badge shown so duplicates across tabs are distinguishable), submits a request. - Requesting is idempotent: a second request for a track already pending/fulfilled surfaces - its existing status instead of creating a duplicate row. -3. **Admin approval queue**: pending requests list, approve or reject with an optional note, - visible to the requesting user. Approval enqueues fulfillment; rejection is terminal unless - the user re-requests. -4. **Fulfillment worker**, one track at a time, no artist/album monitoring created anywhere: - - Try Prowlarr first: search its indexers for the specific track (using the metadata - captured at request time: title, artist, album, duration if available), evaluate - candidate releases, grab the best single-track match via Prowlarr's grab API, hand off to - the operator's existing qBittorrent instance, poll for completion. - - If Prowlarr yields nothing usable within a bounded attempt budget, fall back to Soulseek - via slskd's API: search, evaluate candidates (bitrate, format, exact filename plausibility - against the requested title/artist), download the single file, poll for completion. - - On completion: move/rename the file into the operator's library layout (configurable - target path pattern, e.g. `{Artist}/{Title}.{ext}`), tag it using the metadata captured - at request time (prefer MusicBrainz canonical tags when available), mark the request - fulfilled with the final file path recorded. - - On exhaustion of both paths: mark the request failed with a reason, visible to the admin - and to the requesting user, no silent disappearance. - - Every fulfillment attempt (source tried, query used, candidates considered, one chosen and - why, or why none qualified) is logged in a structured, retrievable-per-request audit trail, - not just application logs. -5. **User accounts**: registration/login for requesting users, a separate admin role for the - approval queue and system configuration (indexer/slskd connection settings, source API - keys, fulfillment attempt budgets). Single admin is sufficient; do not build multi-tenant - organizations or SSO. -6. **Request history and statuses** visible to each user for their own requests: pending, - approved (queued), downloading, fulfilled (with a way to know where the file landed or get - it, appropriate to a self-hosted single-user-household context, not a distribution feature), - rejected, failed. -7. **Demo mode**: a flag that seeds realistic fake search results and a fake request queue with - every status represented, so the UI, screenshots, and first-run evaluation need zero real - external API keys or a real Prowlarr/slskd instance. - -Explicitly out of scope for Phase 1: any of the native clients (Phase 2+), bulk/album-level -requests of any kind (this is the one thing Cadence must never grow, even as a convenience -feature), multi-admin roles, public unauthenticated access. - -### Phase 2+ (separate, later efforts, each its own milestone against the Phase 1 API) - -Once Phase 1's API is stable and versioned, native clients are built against it as independent -projects, each using its platform's first-party toolkit so each feels genuinely native rather -than a shared cross-platform shell: - -- Windows: WinUI 3 / .NET -- macOS: SwiftUI -- iOS: SwiftUI -- Android: Kotlin + Jetpack Compose - -Each native client phase gets its own MISSION.md derived from this one plus the finished -Phase 1 OpenAPI contract, written once Phase 1 ships. Do not start any of these until Phase 1 -has passed its full verification pass below and produced a real, stable OpenAPI spec. - -## Architecture and security (non-negotiable, Phase 1) - -- **Backend**: Python, FastAPI, Pydantic v2 models for every request/response so the OpenAPI - spec generated at `/openapi.json` is accurate and complete enough for later client codegen. - SQLite for Phase 1 (matches the operator's other self-hosted single-node services; document - the upgrade path to Postgres in REPORT.md if load ever warranted it, do not build it now). -- **Background jobs**: the fulfillment worker runs as an async task queue in-process (e.g. - arq or a simple asyncio-based worker) backed by the same SQLite database for job state, so - the whole stack is one deployable unit with no separate Redis/broker dependency for Phase 1. -- **Web client**: a single-page app (React or SvelteKit, pick one and justify it in REPORT.md) - served by the same container, calling the FastAPI backend over its own origin behind the - operator's existing reverse proxy. -- **Configuration via environment only**: DATABASE_URL/DATA_DIR, SESSION_SECRET (generated on - first boot if unset, persisted with owner-only permissions), ADMIN_BOOTSTRAP_EMAIL, - PROWLARR_URL, PROWLARR_API_KEY, QBITTORRENT_URL, QBITTORRENT_USERNAME, - QBITTORRENT_PASSWORD, SLSKD_URL, SLSKD_API_KEY (all optional at boot, feature-detected; - Cadence must start and serve search/browsing even if fulfillment backends are unconfigured, - clearly surfacing "not configured" rather than failing to boot), SPOTIFY_CLIENT_ID, - SPOTIFY_CLIENT_SECRET, MUSIC_LIBRARY_PATH, DEMO_MODE. Provide .env.example. A committed - secret of any kind is a failed run. -- User passwords hashed with a modern algorithm (argon2 or bcrypt), never logged. Session - cookies HMAC-signed or JWT with a real expiry and rotation on privilege change. Login is - rate-limited. Login/session comparison paths are constant-time where relevant. -- All third-party API keys (Spotify, Prowlarr, slskd) live only in backend config, never sent - to or callable directly from the browser; the web client only ever talks to Cadence's own - API, which proxies and rate-limits outbound calls to each source. -- Strict CSP, X-Content-Type-Options, Referrer-Policy, frame-ancestors none on every response. -- Docker: multi-stage build, non-root user, pinned base images, HEALTHCHECK against - `/api/health`, SQLite and downloaded-file staging area on named volumes. docker-compose.yml - provided, including commented-out example wiring for Prowlarr/qBittorrent/slskd service - names as they would appear on the operator's own Docker network. -- `/api/health` returns version, mode, database-ok, and per-integration configured/reachable - booleans (Prowlarr, qBittorrent, slskd, Spotify) without leaking URLs, keys, or paths. - -## Error handling bar - -No unhandled path anywhere. Every route validates input, handles auth failures and database -errors with typed client responses and detailed server-side logs. Every external API call -(Apple, MusicBrainz, Spotify, Prowlarr, slskd, qBittorrent) has an explicit timeout, a bounded -retry policy, and a failure mode that degrades that one feature rather than the request or the -process. The fulfillment worker must survive a crash mid-download: on restart, in-flight jobs -are resumed or re-evaluated from persisted state, never silently lost or double-grabbed. -Unhandled exceptions in the worker terminate that job loudly (marked failed, logged) and never -take down the worker loop itself. - -## Testing - -- Unit: search-result normalization per source (mapping Apple/MusicBrainz/Spotify/Prowlarr's - very different response shapes into one internal model), fulfillment candidate scoring, - idempotent-request logic, password hashing/session logic, tag-writing. -- Integration: every route against a real SQLite test database; search endpoints against - recorded/mocked upstream responses (happy path and every error class: timeout, malformed - JSON, empty results, auth failure); fulfillment worker against a mocked Prowlarr/slskd/ - qBittorrent so the Prowlarr-then-Soulseek-fallback logic is provably correct without needing - live services in CI. -- End-to-end: Playwright (or equivalent) against demo mode covering search across tabs, - submitting a request, admin approval, watching a demo fulfillment complete, request history. -- Everything runs in CI; the run is red if any of it fails. - -## CI/CD and releases (semver-driven) - -- ci.yml on every push and PR to main: install from lockfile, lint, typecheck, unit and - integration tests, end-to-end tests, production build, dependency audit failing on high or - critical advisories. -- release.yml on tags matching v*.*.\*: full validation, then a multi-arch image - (linux/amd64, linux/arm64) pushed to GHCR tagged :vX.Y.Z and :latest, then a GitHub Release - with generated notes. Conventional commits throughout, CHANGELOG.md maintained, first - release v0.1.0 tagged when the pipeline is proven end to end. - -## Docs - -README.md: pitch (what problem this solves and why existing \*arr tools do not), screenshots -from demo mode, quickstart compose, full configuration reference including how to obtain a -Spotify Client Credentials app and a Prowlarr API key, reverse proxy notes, security posture -section, development guide. CONTRIBUTING.md and a runbook covering common operational issues -(a fulfillment source misconfigured, a stuck job, how to re-run a failed request). MIT license -in the name of Jayakrishna Konda. All prose in every file avoids em dashes and double hyphens; -use commas, conjunctions, or separate sentences. - -Apply the operator's standard repo lifecycle baseline once the app is functional: GitHub issue -and PR templates, Discussions enabled with a basic category setup, docs/PACKAGES.md cataloguing -every dependency (generated, gitignored, never pushed), a security baseline pass appropriate to -a self-hosted service handling third-party API keys and a download pipeline. - -## Git conduct (strict) - -Author every commit as Jayakrishna Konda . Never add AI attribution of any -kind anywhere: no Co-Authored-By trailers, no generated-with lines, nothing in commits, docs, -or code comments. Small, focused, conventionally named commits. Do not push; the operator -pushes. - -## Final verification and adversarial pass (end of Phase 1) - -1. Full test suite green, production build clean, audits clean. -2. Run the production build in demo mode and exercise the whole flow end to end: search across - every tab, submit a request, approve it as admin, observe demo fulfillment, check history. -3. A dedicated adversarial review of the whole codebase hunting specifically: auth bypass on - admin routes (especially the approval queue and integration config), SSRF via any - user-influenced URL passed to an outbound fetch (search queries reaching Apple/MusicBrainz/ - Spotify/Prowlarr must be strictly parameterized, never raw-interpolated), path traversal in - the file-move/tag-writing step (a maliciously crafted title/artist must never escape - MUSIC_LIBRARY_PATH), secrets or API keys anywhere in the repo or logs, timing leaks in - password/session comparison, rate-limit gaps on search and auth endpoints, CSP holes, - dependency confusion in the multi-source client code. Fix findings, re-run the suite. -4. A documented failure in REPORT.md is acceptable; a hidden one is not. - -## REPORT.md - -Append one dated section per phase (never overwrite a previous phase's section). Each section -covers: what was built and why this shape; stack justification (backend framework, web -framework, job queue approach); every route with its validation, auth, and rate limit; the -full search-source integration matrix (source, auth method, rate limit respected, failure -mode); the fulfillment decision logic (how Prowlarr candidates are scored, when the Soulseek -fallback triggers, how ties/ambiguous matches are handled); security checklist with honest -covered and not-covered status; test coverage summary; dependency list with justifications; -deployment steps for the operator's stack (compose snippet, reverse proxy, environment -reference); known gaps ranked by risk; the next three things worth doing; anything attempted -and abandoned, with reasons. diff --git a/MISSION_settings_ui.md b/MISSION_settings_ui.md deleted file mode 100644 index 3badeea..0000000 --- a/MISSION_settings_ui.md +++ /dev/null @@ -1,81 +0,0 @@ -# Mission addendum: Admin-editable integration settings - -This is an addition to the already-shipped Phase 1 (see MISSION.md and REPORT.md's "Phase 1" -section, which is complete and must not be reverted or re-litigated). Read both before starting. -This addendum has its own REPORT.md section requirement at the end; append it, do not touch the -existing Phase 1 section. - -## Why this exists - -Phase 1 only reads Prowlarr/qBittorrent/slskd/Spotify/library connection details from -environment variables at boot, via `backend/app/core/config.py`'s `Settings` object (loaded -once, cached by `get_settings()`). That means every operator who wants to actually run Cadence -has to SSH into their host and hand-edit a `.env` file to wire up their own instances, then -restart the container. That is a real usability gap for a tool meant to be self-hosted by -people other than its author: it should be configurable from the admin UI, the same way an -admin approves requests from the UI rather than editing the database directly. - -## What to build - -1. **A database-backed settings store.** Add an `IntegrationSettings`-style table (or one row - per integration, your call, justify the shape in the addendum REPORT.md section) holding the - same fields currently in `Settings` for Prowlarr, qBittorrent, slskd, Spotify, and the - library path/pattern. Secrets (API keys, qBittorrent password, Spotify client secret) must be - encrypted at rest, not stored in plaintext in the SQLite file. Use a symmetric encryption key - derived from or stored alongside `SESSION_SECRET`'s existing generate-and-persist pattern in - `config.py` (do not invent a second secret-management mechanism; extend the existing one). -2. **Precedence: environment variables remain the bootstrap/override path, DB-stored values are - the normal runtime path.** On boot, if an env var for a given integration field is set, it - seeds (or overrides, your call, document which) the DB-stored value so existing - docker-compose-based deployments (env-var only, no UI interaction) keep working unchanged. - Once a value exists in the DB and no env var overrides it, admin UI edits take effect - immediately without a restart. `get_settings()`'s cached, process-wide object is the wrong - shape for values that can change at runtime from the UI; refactor the read path for these - specific fields so a save from the settings UI is reflected on the next request, not only - after a restart. Fields that are not integration credentials (e.g. `SESSION_TTL_MINUTES`, - `OUTBOUND_TIMEOUT_SECONDS`) can stay env-only; scope this addition to exactly the fields - listed in item 1. -3. **Backend API**: admin-only routes to read the current settings (with secrets masked in the - response, e.g. show `PROWLARR_API_KEY` as `"••••••••ab12"` or similar, never the real value - once saved) and to update them (accepting a full or partial payload per integration). A - "test connection" action per integration (Prowlarr, qBittorrent, slskd, Spotify) that makes - one real outbound call using the SafeHttpClient and reports success/failure without - persisting anything, so an admin can validate credentials before saving. -4. **Admin settings page in the web client**: one page, grouped by integration (Prowlarr, - qBittorrent, slskd, Spotify, Library), each with its fields, a masked display of any - already-saved secret, a "Test connection" button wired to the backend action above, and a - save action. Reachable only to the admin role, same guard pattern as the existing admin - approval queue page. -5. **`/api/health`'s existing configured/reachable booleans must reflect DB-stored settings too**, - not just env vars, since that endpoint is how the rest of the app (and any future native - client) knows what is actually usable. - -## Constraints carried over from MISSION.md (still apply, do not relax them) - -- No secret ever committed to the repo, logged, or returned unmasked once saved. -- All outbound calls, including the new "test connection" actions, go through the existing - SSRF-hardened `SafeHttpClient`. Do not add a second, unguarded HTTP path for this feature. -- Path safety, rate limiting, CSP, and the existing auth/session model all still apply - unchanged to any new route. -- Small, focused, conventionally named commits. Author every commit as - "Jayakrishna Konda ". No AI attribution anywhere. Do not push. -- Full tests for everything new: unit tests for the encryption/decryption round trip and the - env-var-vs-DB precedence logic, integration tests for the new routes (including a masked-value - response and a rejected non-admin request), an end-to-end test that saves a setting through - the UI and confirms it takes effect (e.g. `/api/health` flips a `configured` boolean) without - a restart. Update the existing Playwright demo-mode suite if the new page needs a demo-mode - fixture. -- Re-run the full existing test suite (backend and web) and confirm it is still green; this - addition must not regress anything from the Phase 1 REPORT.md. - -## Definition of done - -- An admin can open the settings page, enter real Prowlarr/qBittorrent/slskd/Spotify details, - test each connection, save, and have search/fulfillment immediately use those values, with - zero SSH access or file editing required. -- `docker-compose.yml` and `.env.example` still work exactly as before for an operator who - prefers env-var-only configuration (this is additive, not a breaking change to the existing - deployment path). -- Append a new dated section to REPORT.md (do not touch the existing Phase 1 section) covering: - what was built, the encryption approach and why, the precedence rule chosen and why, the new - routes with their validation/auth, test coverage added, and any gaps. diff --git a/README.md b/README.md index 8897952..b7bb23a 100644 --- a/README.md +++ b/README.md @@ -114,8 +114,7 @@ target path). owner-only permissions, the same pattern `SESSION_SECRET` uses. The settings API masks every secret in its responses (e.g. `••••••••ab12`), and admin-only routes guard both reading and updating them. -- Full detail, including an honest covered/not-covered checklist, is in - [REPORT.md](REPORT.md). +- To report a vulnerability, see [SECURITY.md](SECURITY.md). ## Development guide @@ -124,8 +123,8 @@ end-to-end tests) and coding conventions. ## Status -Phase 1 (backend + web client) is built; see [REPORT.md](REPORT.md) for what shipped, what was -decided along the way, and known gaps. Native apps (Windows, macOS, iOS, Android) are later, +Phase 1 (backend + web client) is built; see [CHANGELOG.md](CHANGELOG.md) for what shipped in +each release. Native apps (Windows, macOS, iOS, Android) are later, separate phases, each built against Phase 1's stable, OpenAPI-documented API. ## License diff --git a/REPORT.md b/REPORT.md deleted file mode 100644 index 932ebf7..0000000 --- a/REPORT.md +++ /dev/null @@ -1,747 +0,0 @@ -# REPORT - -This file is appended to, never overwritten, once per phase. This is Phase 1's entry. - -## Phase 1: Backend + Web Client (2026-07-17) - -### What was built and why this shape - -A FastAPI backend and a React web client, deployed as one container, implementing the full -Phase 1 product spec from MISSION.md: multi-source search across four sources shown as tabs, -an idempotent request flow, an admin approval queue, a Prowlarr-first/Soulseek-fallback -fulfillment worker with a structured per-request audit trail, demo mode, and the full security -bar (SSRF protection, path-traversal protection, argon2 hashing, JWT sessions, rate limiting, -CSP). Everything lives under `backend/` and `web/`, sharing the repo root's Docker/CI tooling. - -The shape follows the Overseerr/Ombi request-approval-fulfillment pattern the mission asks for, -but with one hard constraint threaded through every layer: the `Request` model has no -album/artist equivalent anywhere in the schema, the fulfillment worker only ever searches for -and grabs the one track named on the request, and the path-safety module only ever files a -single resolved file path. There is no code path in this codebase capable of monitoring or -bulk-grabbing anything. - -### Stack justification - -- **Backend framework: FastAPI.** Mission-specified. Async-native, and Pydantic v2 models used - for every request/response give an accurate, complete `/openapi.json` for free, which the - mission calls out as needed for later native-client codegen. -- **Database: SQLite via SQLAlchemy 2.0 async ORM + aiosqlite.** Mission-specified for Phase 1. - Chose SQLAlchemy's async ORM over raw `aiosqlite`/`databases` for the mapped models, migration - path (Alembic is installed but not yet wired to a migration, since `create_all()` is - sufficient pre-v1; noted as a gap below), and because the async session pattern composes - cleanly with FastAPI's dependency injection. -- **Job queue: in-process asyncio worker, no Redis/broker.** Mission-specified ("no separate - Redis/broker dependency for Phase 1"). `FulfillmentWorker` polls the `requests` table for rows - in `APPROVED` status, claims one at a time with an optimistic lock token, and drives it - through `app.services.fulfillment`. Job state lives entirely in the database, so the whole - stack really is one deployable unit. Considered `arq` (mentioned as an option in the mission) - but it wants its own Redis-backed queue; a bare asyncio loop over the existing SQLite table - meets every requirement (crash recovery, single active job, wake-on-approval via an - `asyncio.Event`) with zero new infrastructure. -- **Web framework: React + TypeScript + Vite, not SvelteKit.** Both were explicitly left open - by the mission. Chose React for: (1) the broadest pool of contributors likely to touch a - self-hosted OSS project already knowing it; (2) TanStack Query's request/response caching and - polling primitives being an exact fit for the "poll a downloading request until it resolves" - UI need, with a more mature ecosystem than SvelteKit's equivalents; (3) React Router's - well-trodden nested-route + loader patterns mapping cleanly onto the five-page app - (search/requests/detail/admin/auth) with clean auth-guarding via a wrapper component. -- **Rate limiting: slowapi.** A thin wrapper over the `limits` library giving a decorator-based - API (`@limiter.limit(...)`) that composes with FastAPI's dependency/exception-handler model - without a separate proxy-layer rate limiter, appropriate for a single-container self-hosted - app with no separate edge/gateway layer. -- **Password hashing: argon2 (via passlib).** Mission-specified as one of the two acceptable - algorithms; argon2 chosen over bcrypt for its memory-hardness against GPU-accelerated - cracking, at the cost of a slightly heavier dependency (`argon2-cffi`). -- **HTTP client: httpx**, wrapped in a custom `SafeHttpClient` (see security section) rather - than used directly anywhere in the codebase, so every outbound call is provably covered by - the same SSRF policy without relying on each adapter to remember to apply it. - -### Every route, its validation, auth, and rate limit - -| Method & path | Auth | Rate limit | Validation | -| ----------------------------------------- | ---------------------------------------- | ------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `GET /api/health` | none (public) | none | n/a; never leaks URLs/keys/paths, only configured/reachable booleans | -| `POST /api/auth/register` | none | 10/min per IP | Pydantic `RegisterIn`: `EmailStr`, password min 10 chars | -| `POST /api/auth/login` | none | 10/min per IP | Pydantic `LoginIn`; constant-time-safe (dummy-hash verify on unknown email so timing doesn't reveal account existence) | -| `POST /api/auth/logout` | none required (clears cookie either way) | none | n/a | -| `GET /api/auth/me` | Bearer token or session cookie | none | n/a | -| `GET /api/search` | required (any authenticated user) | 30/min per IP | `q`: 1-300 chars; `limit`: 1-50 | -| `POST /api/requests` | required | 20/min per IP | `CreateRequestIn`: source is an enum of the four allowed values, title/artist required non-empty (whitespace-stripped), `musicbrainz_recording_id` validated as a strict MBID/UUID (see security findings), all string fields length-capped | -| `GET /api/requests/mine` | required | none | n/a | -| `GET /api/requests/{request_id}` | required, owner-or-admin | none | 404 (not 403) for a non-owner, so existence isn't leaked to other users | -| `GET /api/requests/admin/pending` | admin only | none | n/a | -| `GET /api/requests/admin/all` | admin only | none | n/a | -| `POST /api/requests/{request_id}/approve` | admin only | none | `ApproveRequestIn`: optional note, max 2000 chars | -| `POST /api/requests/{request_id}/reject` | admin only | none | `RejectRequestIn`: same | - -Every route funnels database errors and unhandled exceptions through the app-level exception -handlers (`SQLAlchemyError` -> 500 with a generic message, anything else -> 500 with a generic -message, both logged server-side with full detail); a `RequestValidationError` handler passes -Pydantic's error list through `jsonable_encoder` (a real bug this caught and fixed: the raw -error objects aren't always JSON-serializable, see findings below). - -### Search-source integration matrix - -| Source | Auth method | Rate limit respected | Failure mode | -| ------------ | ----------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------- | -| Apple/iTunes | none (keyless) | none published; not throttled client-side beyond the shared 30/min per-user search limit | Timeout/5xx/malformed JSON caught in `SearchSource.run()`, reported as `ok: false` with an error string; other tabs unaffected | -| MusicBrainz | none (keyless) | 1 req/sec, enforced by a process-wide `asyncio.Lock`-based limiter shared across all concurrent searches | Same isolation as above; the limiter adds latency, never a failure, under load | -| Spotify | Client Credentials OAuth (app-only); token cached in-process, refreshed 30s before expiry | Spotify's own per-app limits, not separately throttled client-side | Token-fetch failure or search failure both isolated the same way; reports "Not configured" distinctly from a live failure when no client ID/secret is set | -| Prowlarr | operator API key via `X-Api-Key` header | none (operator's own instance) | Same isolation; "Not configured" when no URL/key set | - -All four run via `asyncio.gather(..., return_exceptions=True)` in -`app.services.search.multi_source_search`, with each adapter's own `SearchSource.run()` already -catching its own errors as a first line of defense and the outer `gather` as a second line in -case an adapter has a bug that raises during normalization. - -### Fulfillment decision logic - -1. **Prowlarr attempt**: search `/api/v1/search` with `"{artist} {title}"` as the query, audio - category IDs. Every raw release is scored via `app.services.scoring`: a `difflib`-based fuzzy - similarity between the release's filename and both `"{artist} {title}"` and - `"{title} {artist}"` orderings (whichever scores higher), plus a bonus if the exact title - substring appears, plus a format-quality signal (FLAC/ALAC > WAV > MP3/AAC > OGG/M4A) and - file size as tertiary tie-breakers. A candidate must score >= 0.45 similarity to qualify at - all (chosen empirically: high enough to reject clearly unrelated releases, low enough to - tolerate messy real-world filenames like `01 - Artist_Title (Official).mp3`). The - highest-scoring qualifying candidate is grabbed via Prowlarr's own `/api/v1/search` POST - (grab) endpoint, using its `indexerId`. Every step (search, scoring, grab attempt) is logged - as a `FulfillmentAttempt` row regardless of outcome. -2. **qBittorrent poll**: once grabbed, Cadence polls qBittorrent (find the torrent by name hint, - check progress, list files, pick the largest file in the torrent as the audio file) rather - than submitting the download itself, since Prowlarr hands the release directly to - qBittorrent (the operator's existing download client) on grab. -3. **Soulseek fallback**: triggers whenever Prowlarr's path returns `False` for any reason - (not configured, no candidates, no qualifying candidate, grab failure, or qBittorrent - never reporting completion within that fulfillment pass). Searches slskd, applies the same - scoring function against filenames (so a single scoring implementation, unit-tested in - isolation, governs both backends' candidate selection), and downloads the winning file. -4. **Ties/ambiguous matches**: resolved deterministically by the sort key - `(-similarity, -format_score, -size_bytes)`, so the same input always produces the same - winner; there is no randomness or first-match behavior anywhere in the scoring path. -5. **Exhaustion**: if both backends fail to produce a fulfilled file, the request is marked - `FAILED` with a human-readable reason, visible to both the admin and the requesting user. - Nothing disappears silently. -6. **Crash recovery**: `FulfillmentWorker.recover_crashed_jobs()` runs once at worker startup, - resetting any request stuck in `DOWNLOADING` (meaning a previous process died mid-job) back - to `APPROVED`. Recovery re-evaluates from scratch (a fresh search) rather than trying to - resume an in-flight download, a deliberate simplification: Soulseek/Prowlarr downloads are - cheap to re-initiate, and re-scoring guards against double-grabbing something already - fulfilled by checking `fulfill_request`'s early-return for already-`FULFILLED` requests. - -### Security checklist (honest) - -**Covered:** - -- SSRF protection: every outbound call (Apple, MusicBrainz, Spotify, Prowlarr, qBittorrent, - slskd) goes through `SafeHttpClient`, which resolves DNS itself and blocks link-local, - loopback, multicast, and cloud-metadata addresses before ever opening a connection, re-checked - on every retry (not just once) to close a DNS-rebinding race. Public sources use a strict - policy that also blocks RFC1918 private ranges; operator-configured integrations - (Prowlarr/qBittorrent/slskd) use a narrower policy that permits RFC1918 (since they - legitimately live on the operator's own Docker network) but still blocks the metadata address - unconditionally. Verified by adversarial testing directly against the module. -- Path traversal: `app.services.pathsafety.render_library_path` sanitizes every path component - independently (strips separators, NUL/control characters, leading dots, wildcard characters, - truncates length) and then verifies the final _resolved_ path is still inside the resolved - library root before any filesystem write, as defense in depth on top of the component-level - sanitization. Adversarially tested against `../../../etc/passwd`, absolute paths, NUL-byte - injection, Windows-style paths, and 10,000-character inputs; every case stayed inside the - library root. -- Auth bypass: every admin-prefixed route requires `require_admin`; request-detail enforces - owner-or-admin and returns 404 (not 403) to a non-owner so existence isn't leaked. Verified - the `/{request_id}` catch-all route pattern cannot shadow the two-segment `/admin/*` routes - (Starlette's path converter only captures a single segment for `{request_id}`). -- Secrets: no secret or API key exists in the repository, logs, or error messages. Verified - every API key is transmitted via HTTP headers, never URL query strings, so even a logged - `SafeHttpError` (which includes the target URL) cannot leak one. Verified httpx's own - exception messages never echo header values. -- Password hashing: argon2, never logged; login timing does not distinguish "unknown email" - from "wrong password" (a dummy-hash verify runs on the unknown-email path too). -- Session tokens: JWT (HS256) with real expiry and a `token_version` claim; rotating a user's - `token_version` invalidates every previously issued token. -- Rate limiting: auth (register/login) and search are both rate-limited; request creation is - rate-limited too (not explicitly required by the mission but a reasonable extension, since - fulfillment work is triggered downstream of it). -- CSP: `default-src 'self'`, `script-src 'self'` (no inline scripts, no `unsafe-inline` for JS), - `frame-ancestors 'none'`, plus `X-Content-Type-Options: nosniff`, `Referrer-Policy: -no-referrer`, `X-Frame-Options: DENY`, `Permissions-Policy` denying geolocation/mic/camera, on - every response via middleware. -- Dependency confusion: every keyless/OAuth third-party base URL (Apple, MusicBrainz, Spotify) - is a hardcoded module-level constant, never derived from configuration or user input, so - there is no path by which a search query or environment variable could redirect those calls - elsewhere. -- Dependency audit: `pip-audit` and `npm audit` both clean as of this report (a pytest CVE, - PYSEC-2026-1845, was found and fixed by bumping to the patched 9.0.3+ line). - -**Found and fixed during this pass (see git history for full detail):** - -- `musicbrainz_recording_id` was accepted from the client with only a length cap, then - raw-interpolated into the MusicBrainz lookup URL path in - `app.search_sources.musicbrainz.lookup_recording`. A crafted value such as - `"../../v2/artist/x"` would have redirected that outbound request to an arbitrary - MusicBrainz API path (never an internal address, since the hostname itself is hardcoded, but - still a real input-validation gap). Fixed at both the Pydantic input boundary (a strict - MBID/UUID regex validator) and independently at `lookup_recording`'s point of use, so a - future call site that bypasses the schema layer is still protected. Covered by - `tests/unit/test_musicbrainz_recording_id_validation.py`. -- The backend's static-file mount only served `index.html` for the literal root path, so any - direct navigation or refresh on a React Router path (`/login`, `/search`, - `/requests/`, `/admin`) 404'd instead of loading the SPA. Not a security bug, but found - via the same adversarial-minded verification pass (the Playwright e2e suite caught it - immediately, failing at the very first navigation in every test). Fixed with an explicit - `/assets` mount plus a catch-all route serving the matching static file when one exists and - falling back to `index.html` otherwise, with an explicit guard so an unknown `/api/*` path - still 404s as JSON instead of silently serving HTML, plus a resolved-path containment check - on the requested path as defense in depth. -- The Soulseek completion check compared `state.lower().endswith("completed")`, which never - matches real slskd's composite state strings like `"Completed, Succeeded"` (vs. - `"Completed, Errored"`) -- found via integration testing with realistic mocked slskd - responses, not by inspection. Fixed to check both `"completed"` and `"succeeded"` appear in - the state string, so a genuinely completed-but-errored transfer is correctly still not - treated as fulfilled. -- The multi-stage Docker build failed entirely: `backend/pyproject.toml` declared - `readme = "../README.md"`, which resolves fine when installing from the monorepo directly - (the file exists one directory up from `backend/`) but not inside the Dockerfile's - `backend-builder` stage, which only copies `backend/pyproject.toml` and `backend/app` into - the build context, so hatchling's metadata validation failed on a missing file mid-build. - Found by actually building the image (`docker build .`) rather than trusting that source - running locally implied the packaged build would work too. Fixed by dropping the `readme` - field; verified afterward with a full build, a container run against `/api/health`, a - successful `docker` `HEALTHCHECK` transition to `healthy`, and confirmation the process runs - as the intended non-root `cadence` (uid 10001) user. - -**Not covered / accepted low-risk gaps:** - -- MusicBrainz Lucene query syntax is built from the user's raw search term - (`f'recording:"{query}"'`) and passed as a `params=` value, so it cannot escape the URL or - cause SSRF/injection against the HTTP request itself, but a user could craft Lucene operators - (e.g. `" OR recording:*`) to broaden their own search results. This only affects what that - same user sees in their own search tab; no other user's data or any write path is affected. - Not fixed, since escaping quotes would break legitimate searches for titles containing them, - and the blast radius is limited to the requester's own read-only search view. -- `constant_time_compare()` in `app.core.security` is implemented and unit-tested but not - currently called by any application code path (password verification goes through passlib's - own constant-time compare; session tokens are JWT-signature-verified, which is already - timing-safe). Left in place as available utility for any future raw-secret comparison need, - documented honestly rather than presented as protecting something it doesn't currently touch. -- The session cookie (`cadence_session`, set alongside the Bearer token on login) uses - `SameSite=Lax`, which blocks the common cross-site POST/fetch CSRF vectors but not a - cross-site top-level GET navigation that itself triggers a state change. No route in this - API performs a state change on GET, so this does not currently matter, but if a future route - violated that convention the cookie path alone would not protect it. The web client - authenticates exclusively via the `Authorization: Bearer` header (stored client-side, sent - explicitly), so the cookie is a secondary/legacy-compatible path, not the client's actual auth - mechanism. -- No HSTS header is set. Deliberate: Cadence does not terminate TLS itself (documented in the - README as a reverse-proxy responsibility), and setting HSTS from behind a proxy that may not - always be HTTPS-only in a given operator's setup risks a self-inflicted lockout; left to the - reverse-proxy layer to add if desired. -- Alembic is installed as a dependency but no migration has been authored; schema changes today - happen via SQLAlchemy's `create_all()`, which only creates missing tables and never alters - existing ones. Fine for a fresh v0.1.0 install; a real migration is needed before any - backward-incompatible schema change ships in a later version (documented as the first "next - thing worth doing" below). - -### Test coverage summary - -- **Backend**: 202 tests (unit + integration), 87% line coverage, CI gate set at 80%. Unit tests - cover search-result normalization per source (including malformed JSON, timeouts, HTTP errors, - empty results for all four sources), candidate scoring and tie-breaking, path-traversal - defenses against a battery of real attack strings, password hashing and session-token - lifecycle (including tampered and expired tokens), request idempotency (including - rejected/failed re-submission and cross-user isolation), the SSRF policy engine directly - (loopback/private/metadata/multicast addresses under both policies), and the new MBID - validation. Integration tests exercise every route against a real (temp-file) SQLite - database, and the fulfillment worker's Prowlarr-first/Soulseek-fallback logic against mocked - Prowlarr/qBittorrent/slskd HTTP responses, including the full success path through both - backends and worker crash-recovery. -- **Web client**: 19 Vitest/Testing-Library component and unit tests (status badge rendering - per status, track card rendering and request-button state machine, API client error handling - and auth-header attachment). -- **End-to-end**: 8 Playwright tests running against the real backend in demo mode serving the - real built web client (the actual production topology, not a dev-server proxy or a mocked - API): search returns results across every one of the four tabs, a user can submit a request - and see it pending, an admin can see and approve a pending request, request history displays - every seeded status (pending/approved/downloading/fulfilled/rejected/failed), a fulfilled - request's detail view shows its filed path, a failed request's detail view shows its failure - reason, and unauthenticated/non-admin access is correctly redirected. All 8 pass. - -### Dependency list with justifications - -Backend (`backend/pyproject.toml`): - -| Package | Why | -| -------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------- | -| fastapi, uvicorn[standard] | Mission-specified web framework and ASGI server | -| pydantic, pydantic-settings, email-validator | Request/response models and typed env-var config | -| sqlalchemy, aiosqlite, alembic | Async ORM, SQLite driver, migration tooling (installed, not yet used - see gaps) | -| httpx | Async HTTP client, wrapped by the custom SSRF-safe client | -| passlib[argon2], argon2-cffi | Password hashing | -| pyjwt | Session token signing/verification | -| python-multipart | Required by FastAPI for form data (future-proofing; not currently exercised by any route) | -| mutagen | Audio file tag reading/writing (MP3/FLAC) | -| slowapi | Rate limiting | -| tenacity | Bounded retry policy for outbound HTTP calls | -| structlog | Structured JSON logging | -| pytest, pytest-asyncio, pytest-cov, pytest-httpx, ruff, mypy, pip-audit, types-passlib | Dev/test/lint tooling only, not shipped in the runtime image | - -Web (`web/package.json`): - -| Package | Why | -| --------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------ | -| react, react-dom | UI framework | -| react-router-dom | Client-side routing with auth-guarded routes | -| @tanstack/react-query | Server-state caching and the polling behavior request history needs | -| vite, @vitejs/plugin-react, typescript | Build tooling | -| eslint + plugins, prettier | Lint/format, matching the repo's stated Python-equivalent tooling (ruff/black) | -| vitest, @testing-library/react, @testing-library/jest-dom, @testing-library/user-event, jsdom | Component/unit testing | -| @playwright/test | End-to-end testing | - -A full generated `docs/PACKAGES.md` (gitignored, regenerate locally via the `packages` skill) -was not generated as part of this session; noted as a next-step below. - -### Deployment steps - -1. `cp .env.example .env`, fill in at least `ADMIN_BOOTSTRAP_EMAIL`/`ADMIN_BOOTSTRAP_PASSWORD` - (or leave `DEMO_MODE=true` to explore without any configuration). -2. `docker compose up -d`. This builds the multi-stage image (Node stage builds the web client, - Python stage installs the backend into a venv, final stage is a non-root user running - uvicorn on `:8000`), and starts the container with a named volume for `/app/data` (SQLite + - staging area) and `/app/library` (fulfilled tracks). -3. Point the operator's existing reverse proxy (Nginx Proxy Manager, in his case) at `:8000` - for TLS termination and a real hostname. No CORS configuration is needed since one container - serves both the API and the web client from the same origin. -4. To wire Prowlarr/qBittorrent/slskd: uncomment and fill in the relevant block in - `docker-compose.yml`'s `environment:` section, using the operator's own Docker network - service names (a commented `networks:` block is provided for that case too). -5. `docker compose up -d` again to pick up the new environment; `/api/health` will report - `configured: true` (and `reachable` once the integration responds) for whichever - integrations were just added, without needing zero-downtime handling since Cadence - feature-detects at request time, not at boot. - -### Known gaps, ranked by risk - -1. **No Alembic migration authored yet** (medium risk once a schema-changing v0.2.0 ships): - `create_all()` only adds missing tables, never alters existing ones. Any future column - change needs a real migration or existing self-hosted installs will break silently on - upgrade. -2. **Spotify personalization tab (opt-in, per-user Authorization Code OAuth) not built** (low - risk, explicitly additive per the mission): the mission describes this as "a later, explicitly - optional" tab. The app-only Client Credentials search tab it augments is fully built; the - personalization tab was deprioritized to keep this phase's scope to what the mission marks - as required now versus later. -3. **No load/stress testing of the fulfillment worker under many concurrent approved requests** - (low risk for the stated single-household deployment target): the worker processes one job - at a time by design, so this is a throughput characteristic, not a correctness risk, but has - not been measured. -4. **docs/PACKAGES.md not generated** (low risk, purely a documentation completeness gap): - the repo lifecycle standard calls for this; not done in this session, easy to add via the - `packages` skill in a follow-up pass. - -### Next three things worth doing - -1. Author a real Alembic migration (baseline + a no-op first revision) so the upgrade path from - this v0.1.0 schema is safe once any future field is added or changed. -2. Build the opt-in Spotify personalization tab (per-user Authorization Code OAuth, saved - tracks/playlists as a quick-pick source), the one explicitly-deferred piece of the Phase 1 - spec. -3. Generate `docs/PACKAGES.md` and run the `security-baseline` skill's fuller checklist pass - (this report's security section was done by hand against the mission's specific callouts, - not the skill's full generic checklist) before the first tagged release. - -### Decisions made without asking (per the mission's "make the most sensible call" instruction) - -- React over SvelteKit, in-process asyncio worker over `arq`, argon2 over bcrypt, slowapi for - rate limiting: all justified above in the stack-justification section. -- Demo admin/user account emails use a `.internal` TLD rather than `.local`, because - `email-validator` (which Pydantic's `EmailStr` uses) unconditionally rejects `.local` as an - mDNS-reserved special-use domain regardless of deliverability-check settings; discovered - while first wiring up demo-mode auth tests. -- The library file-move step re-searches and re-scores from scratch on worker restart rather - than attempting to resume an in-flight download exactly where it left off; simpler, and safe - given how cheap both Prowlarr grabs and Soulseek downloads are to re-initiate, at the cost of - redoing already-completed download progress on the rare crash-mid-download case. -- Rate limiting the request-creation route (`POST /api/requests`) at 20/min per IP, which the - mission does not explicitly call out (only "search and auth" are named) but is a reasonable - extension since request creation is the trigger for real downstream work once approved. - -### Anything attempted and abandoned - -Nothing was fully abandoned mid-implementation in this phase. The one meaningful scope -narrowing was the Spotify personalization tab, addressed above as a known gap and a next step -rather than an attempted-and-abandoned effort, since it was never started. - -## Addendum: Admin-editable integration settings (2026-07-17) - -This section covers the addition specified in `MISSION_settings_ui.md`: a database-backed, -admin-editable settings store for the Prowlarr/qBittorrent/slskd/Spotify/library fields that -previously only loaded from environment variables at boot. Phase 1's section above is -unchanged and still describes the shipped baseline this addendum builds on. - -### What was built and why this shape - -**Database model**: a single `IntegrationSetting` table -(`backend/app/models/integration_setting.py`), one row per `(integration, field_name)` pair, -rather than one wide table with a fixed column per field. Chosen because: adding a field to an -integration later (or a new integration entirely) becomes a data change, not a schema -migration; masking/encryption is naturally a per-field concern here (`is_secret` is a column, -not a naming convention spread across many columns, since some fields on the same integration -are secrets and others aren't, e.g. `qbittorrent_url` vs `qbittorrent_password`); and a partial -update from the admin UI (edit just one field) maps directly onto upserting a single row -instead of a read-modify-write of a wide row. The trade-off, accepted deliberately: every -stored value is a string (no per-column typing), which costs nothing here since every field in -scope is already a string in `Settings`. - -**Encryption**: `backend/app/core/crypto.py` wraps Fernet (AES-128-CBC + HMAC-SHA256, -authenticated, from the `cryptography` package, added as a new backend dependency) for -encrypt/decrypt, plus a `mask_secret()` helper for API responses. The key is managed by a new -`Settings.resolved_settings_encryption_key()` in `config.py`, extending the exact -generate-and-persist pattern `resolved_session_secret()` already used (lazy-generate on first -boot, written to a file under `DATA_DIR` with owner-only `0600` permissions), per the mission's -explicit instruction not to invent a second secret-management mechanism. It is a **separate -file** (`settings_encryption.key`, sibling to `session_secret.key`), not a key derived from -`SESSION_SECRET`, because the two keys protect different things with different rotation -implications: `SESSION_SECRET` signs JWTs, and rotating it just invalidates active sessions -(cheap, users log in again); this key decrypts persisted integration credentials, and rotating -it without a re-encryption pass would permanently strand every already-saved secret in the -database. Keeping them independent means a future session-secret rotation can never -accidentally brick saved Prowlarr/qBittorrent/slskd/Spotify credentials, and vice versa. - -**Precedence rule chosen: an environment variable, when set, always wins over a DB-stored -value for the same field.** DB-stored values are the fallback/normal runtime path used -whenever the env var is unset. Implemented in `backend/app/services/settings_store.py` -(`get_effective_settings()`, `apply_effective_settings()`), with a `FIELD_REGISTRY` naming -exactly the eleven fields in scope (`prowlarr_url`, `prowlarr_api_key`, `qbittorrent_url`, -`qbittorrent_username`, `qbittorrent_password`, `slskd_url`, `slskd_api_key`, -`spotify_client_id`, `spotify_client_secret`, `music_library_path`, `library_path_pattern`). -Chosen over "DB overrides env once present" because: it matches the mission's framing of env -vars as the bootstrap/override path (an operator who sets one is making a deliberate -infrastructure decision that should not be silently shadowed by whatever was last clicked in a -web form); it keeps every existing docker-compose-only deployment byte-for-byte unaffected, -since nothing in the UI could ever race ahead of an env var that's already set, making this -addition strictly additive; and it gives an operator a clean escape hatch if a UI-saved value -is ever wrong or the encryption key is lost (unset the DB row, or just ignore it, and set the -env var, which takes over immediately on the next request with no manual DB surgery). On boot, -`seed_db_from_env_on_boot()` copies any currently-set env var into its DB row (only when the -env var is actually set; it never clears a DB row just because an env var happens to be absent -that boot), so the settings page can show "sourced from the environment" with the actual value -rather than a blank field, and so a value survives if an operator later removes the env var to -switch to UI-managed configuration. - -**Live refresh, replacing `get_settings()`'s cache for these fields only**: -`apply_effective_settings()` takes the process's cached `Settings` object and returns a copy -with only the registry fields overlaid from the fresh env-or-DB resolution, done via -`model_copy(update=...)` and _only_ for fields where the resolver actually found a value. -This is purely additive: a field with neither an env var nor a DB row keeps whatever the -passed-in `Settings` object already carried (important for tests and any future non-HTTP -caller that constructs `Settings` directly). Wired in at the three places that read these -fields today, at their existing `session`-bearing call sites, with no signature break for -existing callers: `app.services.search.multi_source_search` (session is now an optional -parameter, defaulting to `None`, so the handful of unit tests that call it with a bare -`Settings` and no DB still pass unchanged), `app.services.fulfillment.fulfill_request` (already -received `session`, so the overlay is applied at the top of the function body, meaning even a -long-running `FulfillmentWorker` picks up a mid-session settings save on its very next -fulfillment pass), and `GET /api/health` (applied before computing `configured`/`reachable` -booleans, skipped only in demo mode). No new process-wide cache was introduced; every call -re-queries the (small, indexed) `integration_settings` table, which is the deliberate trade a -mission explicitly asked for over `get_settings()`'s `lru_cache` for exactly these fields. - -### New routes, with validation and auth - -All under `backend/app/api/routes_settings.py`, prefix `/api/admin/settings`, guarded by the -same `require_admin` dependency the existing approval-queue admin routes use (403 for a -non-admin, 401 unauthenticated). - -| Method & path | Auth | Validation | -| ---------------------------------------------------------------------- | ---------- | ------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `GET /api/admin/settings` | admin only | n/a; every secret field masked (`mask_secret`, e.g. `••••••••ab12`), plus a `source` tag (`env`/`db`/`unset`) per field | -| `PUT /api/admin/settings/{prowlarr,qbittorrent,slskd,spotify,library}` | admin only | one Pydantic model per integration (`ProwlarrSettingsIn`, etc.), every field optional (omit = leave unchanged), length-capped | -| `POST /api/admin/settings/{integration}/test` | admin only | 404 for an unknown integration name; makes exactly one outbound call via the existing `SafeHttpClient`, never persists anything, reports `{ok, detail}` | - -No new rate limit was added to these routes: they are admin-only (already the narrowest -practical audience) and, unlike public auth/search endpoints, are not exposed to -unauthenticated or general-user traffic, so a credential-stuffing/abuse rate limit does not -apply the same way; this mirrors the existing approval-queue admin routes, which are likewise -unrated-limited beyond their auth gate. - -### Test coverage added - -- **Unit** (`backend/tests/unit/`): `test_crypto.py` (encrypt/decrypt round trip, ciphertext - never contains the plaintext, wrong-key and tampered-ciphertext both raise a typed - `DecryptionError`, repeated encryption of the same value is non-deterministic, `mask_secret` - never leaks the real value); `test_settings_precedence.py` (field registry completeness and - scope, DB-only resolution, env-overrides-DB for both plain and secret fields, - `apply_effective_settings` is additive and never wipes a constructor-supplied value, the - core "no restart" guarantee proven directly by calling `apply_effective_settings` twice with - an `upsert_setting` in between, boot-time env-to-DB seeding); `test_settings_test_connection.py` - (all four per-integration checks, success/auth-failure/server-error/missing-fields, and an - unreachable-host case proving `SafeHttpError` from the DNS-check layer is turned into a - reported failure, never a raw exception). -- **Integration** (`backend/tests/integration/`): `test_settings_routes.py` (non-admin - rejection on every route including test-connection, a saved secret's GET response is masked - and never contains the plaintext anywhere in the response body, partial updates leave - untouched fields alone, an env var visibly overrides a saved DB value in the GET response, - test-connection never persists regardless of outcome, unknown-integration 404s, oversized - input 422s); a new class in `test_fulfillment.py` proving the worker's actual read path - (`fulfill_request`, not just `/api/health`/`/api/search`) uses a Prowlarr connection that - exists _only_ as a DB row (no env var, no constructor override); `test_settings_takes_effect_live.py`, - the mission's required end-to-end proof, hits the real app instance through its HTTP routes - exactly as an admin's browser would: read `/api/health` (not configured), `PUT` a Prowlarr/ - slskd connection through the settings API, read `/api/health` again in the same test (same - running app, same DB engine, no fixture re-entry, no cache clear) and confirm `configured` - flips to `true`; a further case in the same file proves the live `reachable` probe and the - `/api/search` fan-out both honor the DB-saved value too, not just the feature-detection - boolean. -- **Web** (`web/src/`): `AdminSettingsPage.test.tsx` (all five integrations render grouped, a - masked secret is never pre-filled into its input's value, an env-sourced field renders - disabled with an explanatory note, the Save button stays disabled until a field is actually - edited and submits only the changed fields, Test connection surfaces its result message, - Library correctly has no Test-connection button); three new assertions added to - `client.test.ts` for the new `api.adminSettings.get/update/testConnection` methods. -- **End-to-end** (`web/e2e/demo-flow.spec.ts`, Playwright against the real backend in demo - mode serving the real built client): a non-admin is redirected away from - `/admin/settings`; an admin opens the Settings page, sees all five integration sections, - edits and saves a Prowlarr field, and sees it reflected back tagged as saved (`db` source), - proving the real round trip through the browser rather than an optimistic local echo; a - Test-connection click against an unreachable demo host renders a result message rather than - hanging, timing out, or crashing the page. -- **Full existing suite re-run**: backend 264 tests passing (was 202 before this addendum), - 88% coverage (CI gate is 80%), ruff and mypy both clean; web 28 Vitest tests passing (was - 19), lint and typecheck both clean; all 11 Playwright e2e tests passing (was 8); production - web build (`npm run build`) still succeeds. Nothing from Phase 1's suite was touched or - weakened to make this pass; the two places an existing test's assumption needed adjusting - were both cases where the test's `Settings(...)` fixture value or the test harness's own - `MUSIC_LIBRARY_PATH` env var was, correctly, no longer being silently bypassed by the new - precedence logic (documented inline in those tests). - -### Security checklist for this addition - -- Secrets are never returned unmasked once saved, in any route, including the update routes - (which echo back the fresh masked view, not the submitted plaintext). -- Test-connection uses the existing `SafeHttpClient` exclusively (`POLICY_ALLOW_PRIVATE` for - Prowlarr/qBittorrent/slskd, matching how the live search/fulfillment code treats them; - `POLICY_PUBLIC_ONLY` for Spotify's token endpoint), so it inherits the same SSRF protections - (DNS resolved and re-checked before every attempt, link-local/metadata/loopback blocked) as - every other outbound call in the codebase. No second HTTP path was added. -- Masking is verified at the full HTTP response level, not just the parsed field: integration - tests assert the raw response text (`resp.text`) never contains a saved secret's plaintext - anywhere, not only that the parsed `value` field is masked. -- Every settings route requires `require_admin`; verified by tests for both an unauthenticated - caller (401) and an authenticated non-admin (403), on every route including test-connection. -- Encryption key file (`settings_encryption.key`) follows the same 0600, `DATA_DIR`-relative, - gitignored-by-directory pattern as `session_secret.key`; never logged, never part of any API - response. - -### Known gaps - -1. **No rotation tooling for `settings_encryption.key`** (low-to-medium risk, same class of gap - Phase 1 already accepted for `SESSION_SECRET`/Alembic): if the key file is lost or - deliberately rotated, every previously saved secret becomes an unrecoverable - `DecryptionError` on read; `_load_db_values` degrades this to "value unavailable" (returns - `None` for that field) rather than crashing the settings page, but there is no admin-facing - "your saved Prowlarr key could not be decrypted, please re-enter it" messaging yet, just a - silently-empty field. Acceptable for a first pass; worth a follow-up if key rotation ever - becomes a real operational need. -2. **Test-connection routes are not rate-limited.** They are admin-only, which is the main - mitigation, but an admin session with a compromised token could still be used to hammer an - internal Prowlarr/qBittorrent/slskd instance via repeated test-connection calls. Low risk in - a single-admin, self-hosted context, but noted honestly rather than silently. -3. **`docs/PACKAGES.md`** still not generated (a pre-existing Phase 1 gap, not introduced by - this addendum, but the new `cryptography` dependency should be included whenever it is). -4. **Web `types/api.ts` has one pre-existing line (`RequestStatus`, unrelated to this - addendum) that a newer Prettier version now reports as unformatted** (`format:check` warns - on it). This was already present, byte-for-byte, in the Phase 1 commit, so it was left - untouched rather than folded into this addition's diff; worth a dedicated formatting-only - commit later rather than fixing incidentally here. - -### Decisions made without asking (per the mission's "make the most sensible call" instruction) - -- One `IntegrationSetting` row per `(integration, field_name)` rather than one row/table per - integration: justified in "What was built" above. -- A dedicated encryption key file, sibling to (not derived from) `SESSION_SECRET`: justified - above; the mission left this "your call" but was explicit that it must extend, not replace or - duplicate, the existing generate-and-persist mechanism, which this does structurally while - keeping the two keys' blast radii independent. -- Env-always-wins precedence (vs. "DB overrides env once present"): justified above. -- `music_library_path`/`library_path_pattern` included in the DB-backed registry alongside the - four integrations, per the mission's explicit item 1 listing; no additional non-credential - fields (e.g. fulfillment attempt budgets) were added to the registry, per the mission's - explicit scoping instruction. -- No additional rate limit on the new admin routes, matching the existing approval-queue admin - routes' precedent (admin-only auth is the gate, not a request-rate budget). - -## Addendum: post-ship hardening (2026-07-19) - -This session picked up the three items listed under "Next three things worth doing" at the end -of the Phase 1 section above: a real Alembic migration baseline, the deferred Spotify -personalization tab, and a `docs/PACKAGES.md`/security-baseline pass. All three are now done. - -### 1. Real Alembic migration baseline - -Replaces `Base.metadata.create_all()` at boot with a proper Alembic setup: `backend/alembic/` -(async-aware `env.py` wired to the app's own `Settings`/`Base.metadata`, so migrations always -target the same database the app would boot against) and a single baseline revision -(`184e48b9d542`) whose `upgrade()` produces byte-for-byte the same schema `create_all()` used -to. `backend/app/db/migrations.py`'s `run_migrations()` is now called from `main.py`'s lifespan -in place of `create_all()`: a brand-new database upgrades from nothing to head; a database -predating this change (has the app's tables already, but no `alembic_version` row, because it -was created by the old `create_all()` path) is stamped at the baseline revision first, so its -existing tables are never replayed against. Alembic's `command.upgrade`/`command.stamp` wrap -their own `asyncio.run()` internally, which cannot be called from FastAPI's own running -lifespan loop; `run_migrations()` runs that step via `asyncio.to_thread` to avoid the -`RuntimeError` this would otherwise raise. - -Shipped in the Docker image too: the Dockerfile now copies `backend/alembic/` and -`backend/alembic.ini` into both the builder and runtime stages, so migrations run in -production, not only in local dev. - -Verified beyond the test suite: built and ran the real Docker image against a fresh named -Docker volume (upgrade-from-nothing path, confirmed via the container's own `/api/health` and a -direct `sqlite3` table listing) and against a restart of that same populated volume (confirmed -idempotent, no "Running upgrade" log line the second time). `ruff format`/`ruff check` exclude -`alembic/versions/*` (Alembic-autogenerated, regenerated per revision, not hand-maintained to -house style); `alembic/env.py` itself is hand-written and stays linted. - -### 2. Opt-in Spotify personalization tab - -The one explicitly-deferred piece of the Phase 1 search spec: a user can connect their own -Spotify account (Authorization Code OAuth, `user-library-read playlist-read-private` scopes -only, nothing that can modify their account) and quick-pick from their saved tracks and -playlists, additive to the existing app-only Client Credentials search tab and sharing the -same app registration (`SPOTIFY_CLIENT_ID`/`SECRET`), per the mission's explicit framing. - -**Backend** (`backend/app/services/spotify_oauth.py`, `backend/app/api/routes_spotify_personalization.py`, -`backend/app/schemas/spotify_personalization.py`): token exchange/refresh against Spotify's own -token endpoint; tokens encrypted at rest with the exact same Fernet key already used for -DB-stored integration secrets (`resolved_settings_encryption_key()`), never returned to the -client in any response; saved-tracks/playlists/playlist-tracks fetch normalized to the -existing `TrackResult` shape (`source="spotify"`), so a quick-picked track flows through the -unchanged `POST /api/requests` path with no new request-creation logic. A new -`create_oauth_state_token`/`decode_oauth_state_token` pair in `app/core/security.py` (a -short-lived JWT reusing `SESSION_SECRET`) binds the OAuth callback to the user who started it: -the redirect-based flow (browser navigates away to Spotify and back) cannot carry a Bearer -token across that round trip, so this signed `state` parameter is the CSRF protection instead, -the standard mitigation for this exact OAuth shape. New routes: `GET /api/spotify/status`, -`GET /api/spotify/connect` (redirects to Spotify), `GET /api/spotify/callback` (the only route -that cannot require a logged-in session, since Spotify's redirect carries no auth header; -validates the signed state instead), `POST /api/spotify/disconnect`, -`GET /api/spotify/library/{saved-tracks,playlists,playlists/{id}/tracks}`. A new -`APP_BASE_URL` config field builds the OAuth `redirect_uri`. - -**Frontend** (`web/src/components/SpotifyPersonalizationPanel.tsx`): a fifth "Your Spotify" tab -in `SearchPage.tsx`, architecturally separate from the four parallel search-source tabs (a -standing library browse, reachable without ever submitting a search, not part of the per-query -fan-out). Shows a "Connect Spotify" prompt when not connected, otherwise the connected display -name, a disconnect action, and sub-tabs for saved tracks plus each of the user's playlists. - -**A real, pre-existing-class-of-bug found and fixed along the way**: SQLite's -`DateTime(timezone=True)` columns come back **naive** (no tzinfo) once reloaded from a real -query, even though they are written as UTC-aware. SQLAlchemy has no native tz-aware type for -sqlite; it stores an ISO string and reads it back as-is. This is true of every timestamp column -in the app in principle, but only `get_valid_access_token`'s expiry check does aware-vs-naive -comparison arithmetic on a reloaded value, so it was the only place that ever raised -(`TypeError: can't compare offset-naive and offset-aware datetimes`), caught by an integration -test hitting the real route through a real reloaded `User` row. Fixed by re-attaching `UTC` -tzinfo before comparing, scoped to that one function; other timestamp columns were left -untouched since nothing else does arithmetic on them. - -**Tests**: 24 new backend unit tests (`tests/unit/test_spotify_oauth.py`, HTTP mocked via -pytest-httpx: authorize URL construction, token exchange/refresh including Spotify's -"no new refresh_token in the response" behavior, encrypted storage round-trip, saved-tracks/ -playlist/playlist-tracks normalization, the not-yet-connected and refresh-failure paths); 24 -new backend integration tests (`tests/integration/test_spotify_personalization_routes.py`, -against the real app through its HTTP routes: unauthenticated rejection on every route, the -not-configured guard, the full connect-callback-status round trip, disconnect, the state-token -CSRF validation including a state signed for a nonexistent user, the library routes' 400/404 -distinction between "not configured" and "configured but not connected," and the two new -rate-limit tests below); 5 new Vitest component tests -(`SpotifyPersonalizationPanel.test.tsx`); one assertion added to an _existing_ Playwright test -rather than a new standalone test (see the e2e rate-limit note below). - -**Verified beyond the test suite**: a full Playwright run against the real backend (demo mode) -serving the real built web client, 11/11 passing twice in a row for stability, including the -new tab's "Connect Spotify" prompt rendering correctly with no crash when Spotify is -unconfigured, which is the default/common case for anyone who hasn't set up a Spotify app yet. - -### 3. Security-baseline full checklist pass - -The Phase 1 and settings-addendum security reviews were done by hand against the mission's own -specific callouts (SSRF, path traversal, secrets masking, auth checks); this pass ran the -project's standing `security-baseline` skill's full generic 6-layer checklist for the first -time. Findings: - -- **Fixed (real gap)**: none of the new `/api/spotify/*` routes had rate limiting, unlike - `/api/auth/*`, `/api/search`, and request creation, despite `disconnect` being state-changing - and the three library-browse routes each triggering a real outbound Spotify call (worst case, - a token refresh too). Added `SPOTIFY_CONNECTION_RATE_LIMIT` ("10/minute", matching auth) for - `connect`/`disconnect`, and `SPOTIFY_LIBRARY_RATE_LIMIT` ("30/minute", matching search) for - `status` and the three library routes. Two new integration tests prove a 429 actually fires - after the threshold on both budgets, following this repo's existing rate-limit test pattern. -- **Fixed (missing standard hygiene)**: added `.github/dependabot.yml` (pip for `backend/`, npm - for `web/`, github-actions), which this repo was missing despite the standing repo-lifecycle - convention already applied to the operator's other repos. CI's `pip-audit`/`npm audit --audit-level=high` - steps (already present) catch known vulnerabilities on every push; Dependabot adds the - ongoing scheduled-PR remediation on top, which CI alone does not provide. -- **Checked, no gap found**: session cookies (`httponly`, `secure`, `samesite=lax`); CSP and - the other four security headers already applied to every response via - `SecurityHeadersMiddleware`; no second, unguarded outbound HTTP path in the new Spotify code - (every call goes through the existing `SafeHttpClient`); no open-redirect surface in the OAuth - callback (redirect targets are either the fixed Spotify authorize URL or hardcoded literal - paths, never built from `code`/`state`/`error` query params); no reflected-XSS surface in - those same params (compared against literal strings only, never rendered as HTML); no secrets - committed to git history or logged; `.gitignore` already covered `.env`. -- **Not actionable from a local session**: GitHub-account-level operational items (2FA on the - account, branch protection specifics, Dependabot auto-merge policy) are outside what a local - checkout can configure or verify; flagged rather than silently skipped. -- **Docs fixed as part of this pass**: `README.md`'s Spotify env var row was stale (only - described the Client Credentials search tab); updated to also document `APP_BASE_URL` and the - personalization tab. `CHANGELOG.md` updated with today's three additions. - -### Full suite re-run - -Backend: 315 tests passing (was 264 at the start of this session), 87% coverage (CI gate is -80%), ruff and mypy both clean. Web: 33 Vitest tests passing (was 28), lint and typecheck both -clean, production build succeeds. Playwright e2e: 11/11 passing (was 8 at Phase 1, 11 after the -settings addendum, confirmed still 11/11 with the new tab folded into the existing search test -rather than added as a 12th standalone test, for the rate-limit reason noted above). - -### Known gaps (carried forward or newly found, ranked by risk) - -1. **`PUT /api/admin/settings/{integration}` cannot actually clear a previously-saved DB - setting.** A `null` value in the payload is treated as "field absent from this partial - update" (skipped), not "clear this field" (this predates this session; discovered while - writing a test that assumed otherwise). Low risk (an operator can still override with an env - var, or the value can be overwritten with a new one), but worth a deliberate design decision - if "clear a saved secret without setting a replacement" ever becomes a real need. -2. **No rotation tooling for `settings_encryption.key`**, same class of gap Phase 1's addendum - already accepted (now also covers the Spotify OAuth tokens, which share this key). -3. **Test-connection routes and the new Spotify routes are rate-limited but not IP-allowlisted**; - acceptable for a single-admin, self-hosted context, noted for completeness. -4. **`docs/PACKAGES.md` was stale** (missing `cryptography`, an outdated Alembic note); this is - generated and gitignored, so it was simply regenerated during this session rather than - representing a real product gap, but is worth re-running after future dependency changes. -5. **A `coverage.py` measurement artifact** on `app/api/routes_spotify_personalization.py`: - specific redirect/exception-raising lines inside the async route handlers are reported as - uncovered by `coverage.py` even though a manual sentinel-file write proved they execute every - time the corresponding test runs. Tried the pure-Python tracer, cache clears, isolating to a - single test, and bypassing pytest-cov entirely via `coverage run -m pytest`; all reproduced - the same wrong result. Not architecture-specific (this box is aarch64, ruled out as the - cause) and not a general `RedirectResponse` issue (a minimal repro outside this app measured - 100% correctly). Root cause not found. Not a real testing gap, since the tests genuinely - exercise and assert on that exact behavior; noted so a future session doesn't re-run the same - investigation from scratch. - -### Decisions made without asking (per the mission's "make the most sensible call" instruction) - -- Migrations run via `asyncio.to_thread` rather than restructuring Alembic's `env.py` to avoid - its own internal `asyncio.run()` call, since that internal call is Alembic's own async - template code, not something this project should fork/maintain a patched copy of. -- The Spotify personalization tab is a standing, always-visible tab reachable without - submitting a search first (a library browse, not a search result), rather than only appearing - after a query is submitted like the other four tabs; this matches the mission's framing of it - as a "quick-pick source" a user browses, not something they search for. -- Rate limits for the new Spotify routes reuse the existing budget constants' values - (`10/minute` for state-changing/infrequent-use routes, `30/minute` for routes that make a real - outbound call) rather than inventing new numbers, for consistency with the rest of the app's - rate-limiting philosophy. diff --git a/web/src/types/api.ts b/web/src/types/api.ts index b651266..6d14807 100644 --- a/web/src/types/api.ts +++ b/web/src/types/api.ts @@ -1,6 +1,6 @@ // Mirrors backend/app/schemas/*.py. Kept as plain hand-written types rather than generated // from the OpenAPI spec for Phase 1 to avoid adding a codegen step; revisit once the API is -// stable and native clients need the same contract (see MISSION.md Phase 2+). +// stable and native clients need the same contract (see Status in the README for the planned native apps). export type UserRole = "user" | "admin";