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";