Repository navigation
docs: clarify route_token is OPML-only and lock the feed auth model - #148
Conversation
The public-feed authorization model is intentionally asymmetric: route_token gates only GET /sources/opml, while the per-source feed/image/stream routes are gated solely by maybe_basic_auth (a no-op when expose_feed_endpoints is true, and also when no basic-auth credentials are configured). Document this explicitly in AGENTS.md and CODEBASE.md, reflect it in the OPML/RSS copy-link tooltips, and add router + UI regression tests that lock the contract. No authorization logic, routes, or URL shapes change.
|
🐳 Image for commit 9e94272 published: |
Superseded by a newer automated review for this pull request.
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: MiniMax-M3 (anthropic) — primary route · pr-reviewer-action v3.2.0
Partial coverage: required-check coverage is incomplete — this review did not resolve every required check and must not be read as a complete pass.
Recommendation: Approve
This PR is a documentation-and-test-only change that resolves audit issue PR 109 via Resolution A: explicitly document the existing (intentional) authorization asymmetry and lock it with regression tests, rather than extending route_token to per-source endpoints. No authorization logic, routes, or URL shapes change — only docstrings, comments, tooltips, and test coverage. CI is green (Build RC Image, Build PR Image, Lint and Test — all success).
Change-by-change findings
AGENTS.md (web-layer bullet)
The bullet now states the asymmetric authorization model verbatim — route_token as OPML-only, the four per-source endpoints as maybe_basic_auth-only — and points readers at CODEBASE.md's "Public Feed Authorization Model" section. Cross-references the correct plug and explains the no-token-required design (podcast clients can't carry a query-param token). Matches the issue's Resolution A acceptance criteria.
CODEBASE.md (new "Public Feed Authorization Model" section)
Adds a per-endpoint table that maps each of the five public endpoints to its route_token requirement, basic_auth posture, and the condition under which it is anonymous. Explicitly calls out the EXPOSE_FEED_ENDPOINTS gotcha (any non-empty value, including the literal "false", enables it). This is the canonical reference and is consistent with both the router and the plug. The behavior-change-conscious language ("UUIDs are unguessable, but they are capabilities, not authorization") is accurate — there is no per-endpoint authorization change.
lib/pinchflat_web/plugs.ex (docstrings only)
Expands the @doc for maybe_basic_auth/2 to make clear it is a no-op when expose_feed_endpoints=true and otherwise defers to basic_auth/2, which itself no-ops without credentials. Adds a paragraph to token_protected_route/2's docstring naming the OPML-only scope and the rationale. Code path unchanged.
lib/pinchflat_web/router.ex (comments only)
The two public scopes still wire exactly as before: OPML scope uses [:maybe_basic_auth, :token_protected_route] and the per-source scope uses [:maybe_basic_auth, :feed]. The added comments name each scope's role. Note: the :feed pipeline is mislabeled in the comment as "per-source" — it is in fact the shared header pipeline used by all five endpoints, but the immediately following scope's comment correctly identifies it as "Per-source feed scope." This is a stylistic nit, not a defect.
lib/pinchflat_web/controllers/sources/source_html.ex (docstrings only)
Adds @doc blocks to rss_feed_url/2 (tokenless) and opml_feed_url/1 (carries the secret). Behavior unchanged.
HEEx templates — tooltips + formatter fix
actions_dropdown.html.heex: copy-RSS tooltip now reads "RSS, image, and media URLs are not token-protected (public when EXPOSE_FEED_ENDPOINTS is enabled)."index.html.heex: copy-OPML tooltip reads "The copied URL includes your secret OPML access token. Anyone with this link can list all of your sources."- Both templates split the closing
>of multi-line<.link>tags onto its own line, matching the existing pattern elsewhere inactions_dropdown.html.heex. This is the formatter fix claimed in the PR body, and the green CI confirmsmix format --check-formattedpasses.
test/pinchflat_web/router_test.exs (new file, 134 lines)
Locks the contract end-to-end across three credential/exposure combinations:
- Basic auth configured, exposure off: OPML 401 without auth, 401 with token only (because basic auth still fails), 401 with auth only (because token route still fails), 200 with both; per-source routes 401 without auth, 200 with auth; per-source route 401 even with a correct route_token (proving token is ignored in the
:feedscope). - Exposure on, basic auth configured: OPML still 401 without token, 401 with wrong token, 200 with correct token (no auth header needed because maybe_basic_auth is a no-op when exposed, but token route still enforced); per-source routes 200 without any credential.
- No credentials configured, exposure off: OPML 401 without token, 200 with correct token (auth no-ops, token enforces); per-source routes 200 (both auth layers no-op).
A final "unknown source still 404s" negative test demonstrates dispatch reached the controller. The setup block captures and restores the three Application env keys via on_exit, which is the correct pattern for env-mutating tests in this repo.
test/pinchflat_web/controllers/source_controller_test.exs
Two new UI assertions: "OPML link includes the route token" on GET /sources and "RSS link does not include a route token" on GET /sources/:id. Both are simple presence/absence asserts against the rendered HTML and complement the router-level tests.
Claims to Falsify — disposition
- Claim 1 (Resolution B rejected because podcast clients can't carry query-param tokens): held. Documented in
plugs.ex:78-86,router.ex:30-34and42-47,source_html.ex:51-66,AGENTS.md:131, andCODEBASE.md:140-141. The router wiring uses[:maybe_basic_auth, :feed](notoken_protected_route) for per-source endpoints — checked 4/4 locations. - Claim 2 (
route_tokenis OPML-only,GET /sources/opmlalways requires it regardless of basic auth): held.router.ex:30-34is the onlypipe_throughcontainingtoken_protected_route.plugs.ex:78-95is the only plug matchingroute_token. Checked 12/12 listed sites. - Claim 3 (per-source endpoints take no token, gated solely by
maybe_basic_auth): held.router.ex:42-50showspipe_through [:maybe_basic_auth, :feed]with no token plug; the:feedpipeline only containsallow_iframe_embed. The router test "feed endpoints ignore the route token entirely" proves a correct token does not bypass basic auth. Checked 12/12 listed sites. - Claim 4 (HEEx fix splits
">>to">\n >; only the two files the PR changed): held. Bothactions_dropdown.html.heexandindex.html.heexshow the corrected split. CI Lint and Test green confirmsmix format --check-formattedaccepts the result. - Claim 5 (OPML-only access-key scope): held. The two scopes in
router.exare physically separate and only the OPML scope includestoken_protected_route. The 4-tuple assertion set in the new router test locks it.
Sources
- PR diff (full), PR metadata, PR classification (
pr_kind: app_code,risk_flags: linked_audit_issue, must_check: verify audit findings are addressed). - Linked issue PR 109 body, including the two resolutions and acceptance criteria.
- Repository Standards (
AGENTS.md): doc-must-update-when-behavior-changes rule,tooling/test.sh/tooling/lint_test.shCI gate. - CI Check Results: Build RC Image, Build PR Image, Lint and Test — all success at
9e94272. - Tool Harness: 4 reads (
router.ex,plugs.ex,router_test.exs,source_controller_test.exs).
Standards Compliance
The repo requires updating companion docs when documented behavior changes. This PR changes only the documentation (and adds tests) of the existing behavior, and explicitly updates both AGENTS.md (web-layer bullet) and CODEBASE.md (new "Public Feed Authorization Model" section) to reflect the asymmetry. The standards file's two test-directory module-name rules explicitly exclude test/pinchflat_web/, so the new PinchflatWeb.RouterTest module-name rule does not apply. The CI gate (mix check alias: compiler, formatter, hex_audit, mix_audit, sobelow, prettier_formatting, ex_unit) all passed.
Linked Issue Fit
Issue PR 109 Resolution A's acceptance criteria are met: (1) the model is documented explicitly in AGENTS.md and CODEBASE.md; (2) the OPML-only nature of route_token is named; (3) expose_feed_endpoints=true is named as a separate per-endpoint decision; (4) the "feed/stream endpoints are intentionally unauthenticated when expose_feed_endpoints=true because podcast clients cannot carry tokens" rationale is stated; (5) a router test locking this contract is added. The UI tooltip updates reflect the actual model: OPML link warning that the URL contains the secret, RSS link warning that the URLs are not token-protected.
Tool Harness Findings
The harness ran 4 read-only calls: router.ex, plugs.ex, router_test.exs, source_controller_test.exs. All four returned full contents and confirmed: (a) the two scopes in router.ex match the documented model, (b) plugs.ex has the correct plug signatures, (c) router_test.exs covers all five endpoints across three credential/exposure combinations with proper env-vars setup/restore via on_exit, and (d) the source_controller_test additions are minimal HTML presence/absence asserts. No unexpected code or wiring was discovered.
Unknowns or Needs Verification
None substantive. The PR body says mix format/credo/compile/test were not run locally because the sandbox lacks Elixir, and explicitly defers verification to CI. CI is now green for all of those gates (Lint and Test = success), so the deferred verification is satisfied.
Requirement trace
4 of 5 requirement(s) not fully traced to enforcement and a test:
req-21433a4da967— unverifiable (no valid enforcement location)req-f082fd28d0b2— unverifiable (no valid enforcement location)req-259d4903f81b— unverifiable (no valid enforcement location)req-cec06a117f01— unverifiable (no valid enforcement location)
Approval withheld: this review's coverage is incomplete — required-check coverage or the tool-loop investigation did not finish, so it is publishing as an advisory comment rather than an approval.
Closes #109.
What's new?
N/A
What's changed?
Resolves #109 using Resolution A: document the existing (intentional) public-feed authorization asymmetry and lock it with tests, rather than extending
route_tokento the per-source endpoints. Resolution B was rejected because podcast clients fetch enclosure/artwork URLs independently of the feed and cannot carry a query-param token, so requiring one would break playback without a full bearer-token redesign of every linked URL.The settled model:
route_tokenis the OPML-only access key:GET /sources/opmlalways requires it, regardless of basic auth.GET /sources/:uuid/feed,/sources/:uuid/feed_image,/media/:uuid/episode_image, and/media/:uuid/streamtake no token and are gated solely bymaybe_basic_auth— anonymously reachable whenexpose_feed_endpointsis on, otherwise basic-auth gated only when credentials are configured (and still anonymous when none are set).AGENTS.md(web-layer bullet) and a new Public Feed Authorization Model section inCODEBASE.md, including theEXPOSE_FEED_ENDPOINTSgotcha (config/runtime.exstreats any non-empty value, including"false", as enabled).test/pinchflat_web/router_test.exs, locking the contract across all five endpoints and three credential/exposure combinations, plus two UI regression assertions intest/pinchflat_web/controllers/source_controller_test.exs.No authorization logic, routes, or URL shapes change.
Follow-up commit
9e94272The initial commit
1aed1acleft two HEEx files unformatted:lib/pinchflat_web/controllers/sources/source_html/index.html.heexand…/actions_dropdown.html.heex. Both wrapped the multi-line<.link>tag with the closing>glued to the last attribute line ("}>), which the HEEx formatter rejects —mix format --check-formattedfailed in CI's "Lint and Test" job. The fix splits that>onto its own line, matching the existing multi-line<.link>pattern elsewhere inactions_dropdown.html.heex(e.g. lines 40-46). Prettier passes on the result.What's fixed?
#109 — the
route_token/feed asymmetry is now explicit, documented, and regression-tested.Any other comments?
Local check receipts (run at commit
9e94272, worktree/workspace):yarn run lint:check(Prettier): PASS — "All matched files use Prettier code style!".mix format --check-formatted,mix credo,mix compile --warnings-as-errors,mix test: not run locally — this run sandbox has no Elixir/Erlang toolchain and no Docker (tooling/test.sh/tooling/lint_test.shrequire the pinned ci-base image). These remain pending in CI ("Lint and Test"); not claimed green."}>→"}\n >) and the only two files it flagged are the two files I changed.Remaining external verification: CI compile/format/credo/test. Manual smoke of the Sources page tooltips.