ML-5: document Last.fm OAuth callback trust boundary

Closes GitHub issue #178.

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
+status: Done
-assignee: []
+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`.
+<!-- SECTION:DESCRIPTION:END -->
 
 ## Acceptance Criteria
 <!-- AC:BEGIN -->
-- `last_fm_controller.ex` has a comment documenting the trust boundary
+- [x] #1 last_fm_controller.ex has a comment documenting the trust boundary
-- `router.ex:55` notes the deliberate pipeline exemption
+- [x] #2 router.ex:55 notes the deliberate pipeline exemption
-<!-- SECTION:DESCRIPTION:END -->
-
-- [ ] #1 `last_fm_controller.ex` has a comment documenting the trust boundary
-- [ ] #2 `router.ex:55` notes the deliberate pipeline exemption
 <!-- AC:END -->
+
+## Implementation Plan
+
+<!-- SECTION:PLAN:BEGIN -->
+## 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).
+<!-- SECTION:PLAN:END -->
+
+## Final Summary
+
+<!-- SECTION:FINAL_SUMMARY:BEGIN -->
+## 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.
+<!-- SECTION:FINAL_SUMMARY:END -->

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

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