diff --git a/backlog/tasks/ml-5 - Document-Last.fm-OAuth-callback-trust-boundary.md b/backlog/tasks/ml-5 - Document-Last.fm-OAuth-callback-trust-boundary.md index a539d52f..6a7fc9f8 100644 --- a/backlog/tasks/ml-5 - Document-Last.fm-OAuth-callback-trust-boundary.md +++ b/backlog/tasks/ml-5 - Document-Last.fm-OAuth-callback-trust-boundary.md @@ -1,9 +1,11 @@ --- id: ML-5 title: Document Last.fm OAuth callback trust boundary -status: To Do -assignee: [] +status: Done +assignee: + - Claudio Ortolina created_date: '2026-04-20 08:48' +updated_date: '2026-04-24 07:04' labels: [] dependencies: [] references: @@ -36,13 +38,49 @@ Add a short `@moduledoc` or inline comment in `last_fm_controller.ex` explaining 3. What the failure modes look like (invalid token → store nothing, error logged) Similarly in `router.ex:55`, a brief comment noting the deliberate exception from `:logged_in`. + ## Acceptance Criteria -- `last_fm_controller.ex` has a comment documenting the trust boundary -- `router.ex:55` notes the deliberate pipeline exemption - - -- [ ] #1 `last_fm_controller.ex` has a comment documenting the trust boundary -- [ ] #2 `router.ex:55` notes the deliberate pipeline exemption +- [x] #1 last_fm_controller.ex has a comment documenting the trust boundary +- [x] #2 router.ex:55 notes the deliberate pipeline exemption + +## Implementation Plan + + +## Implementation Plan + +1. **`lib/music_library_web/controllers/last_fm_controller.ex`** — add `@moduledoc` covering: + - Why unauthenticated: OAuth redirect from Last.fm cannot carry session cookies across third-party redirect. + - What protects it: Last.fm validates the `token` via `LastFm.get_session/1`; the call is rate-limited at the `Req` layer (`Req.RateLimiter`, 500ms/req); this is a single-user deployment. + - Failure mode: invalid/forged tokens cause `get_session/1` to return `{:error, _}`; nothing is written to `secrets`, an error toast is shown, and the Req-layer error is logged. + +2. **`lib/music_library_web/router.ex:55`** — add a brief comment above the route noting deliberate exemption from the `:logged_in` pipeline (with pointer to the controller moduledoc for details). + +3. Run `mise run dev:precommit` to verify formatting, credo (including `ModuleDoc` rule), sobelow, and tests still pass. + +## Notes + +- Controllers are excluded from the `Credo.Check.Readability.ModuleDoc` regex in `.credo.exs`, so `@moduledoc` here is optional for lint purposes — using it is a deliberate choice for this security-sensitive module. +- No test changes required (documentation-only). + + +## Final Summary + + +## Summary + +Documented the Last.fm OAuth callback trust boundary in two places: + +- **`lib/music_library_web/controllers/last_fm_controller.ex`** — added `@moduledoc` explaining why the endpoint is unauthenticated (OAuth redirect from Last.fm cannot carry session cookies), what protects it (Last.fm-validated token, `Req.RateLimiter` bounding quota burn, single-user deployment), and the failure mode (invalid token → nothing stored, error toast shown). +- **`lib/music_library_web/router.ex`** — added a comment above the `/auth/last_fm/callback` route noting the deliberate exemption from the `:logged_in` pipeline, with a pointer to the controller moduledoc for the full trust boundary. + +## Verification + +`mise run dev:precommit` passes: credo, sobelow, format, translations, unused deps, and tests (827 passed, 43 doctests). + +## Notes + +Controllers are excluded from the `Credo.Check.Readability.ModuleDoc` regex in `.credo.exs`, so the `@moduledoc` here is a deliberate choice for a security-sensitive module, not a lint requirement. + diff --git a/lib/music_library_web/controllers/last_fm_controller.ex b/lib/music_library_web/controllers/last_fm_controller.ex index f8b752de..772e1ae0 100644 --- a/lib/music_library_web/controllers/last_fm_controller.ex +++ b/lib/music_library_web/controllers/last_fm_controller.ex @@ -1,4 +1,30 @@ defmodule MusicLibraryWeb.LastFmController do + @moduledoc """ + Handles the Last.fm OAuth callback. + + ## Trust boundary + + `GET /auth/last_fm/callback` is deliberately outside the `:logged_in` + pipeline: Last.fm redirects the browser back to us with a `token` query + parameter, and that third-party redirect cannot carry our session cookie, + so requiring a logged-in session here would break the OAuth flow. + + What protects the endpoint: + + * The `token` is validated by Last.fm via `LastFm.get_session/1` + against the `LAST_FM_API_KEY` / `LAST_FM_SHARED_SECRET` pair — it + cannot be forged locally. + * The Last.fm HTTP client is rate-limited at the `Req` layer + (`Req.RateLimiter`, 500ms between requests), bounding the quota a + malicious caller could burn. + * This is a single-user deployment; the stored session key grants + scrobble access for one account, not multi-tenant credentials. + + Failure mode: an invalid or forged `token` makes `get_session/1` return + `{:error, reason}`; nothing is written to the `secrets` table, the user + sees an error toast, and the upstream error is logged by the Req layer. + """ + use MusicLibraryWeb, :controller alias MusicLibrary.Secrets diff --git a/lib/music_library_web/router.ex b/lib/music_library_web/router.ex index 16df0195..21550deb 100644 --- a/lib/music_library_web/router.ex +++ b/lib/music_library_web/router.ex @@ -52,6 +52,10 @@ defmodule MusicLibraryWeb.Router do get "/login", SessionController, :new post "/sessions/create", SessionController, :create + # Deliberately outside the `:logged_in` pipeline: this is the OAuth + # callback from Last.fm and the third-party redirect can't carry our + # session cookie. See `MusicLibraryWeb.LastFmController` moduledoc for + # the full trust boundary. get "/auth/last_fm/callback", LastFmController, :callback get "/public/assets/:transform_payload", AssetController, :show