fix(oauth): the consent page's form-action names one origin, this request's (#1125)

[jhgaylor]

Part 5 of 9 for #1125 (re-cut of #1489). Based on #1879. Its own PR because it is a security boundary.

The base browser CSP is form-action 'self', and a successful consent POST redirects to the app's own origin, which Chrome enforces form-action against on the redirect. #818 widened the header to every registered client's redirect origin. That was fine while the registry held two clients and both were ours.

It is not fine now. With tenant-registered apps the header would grow with the table and hand every visitor the whole registry — every origin anybody ever registered, disclosed on an unrelated account's consent page. So it names exactly one origin: the redirect_uri this request asked for, after validate_request/2 has already accepted it.

Three things to look at:

• The source is the requested URI, not the registration. A loopback client registered against :5199 may legally be asked for :5200 (RFC 8252), and a header naming :5199 has Chrome block a redirect the server has approved. That failure is invisible to an HTTP assertion — it was found by driving a browser — so the port is asserted in both directions.
• The widening moved from a plug to the three response paths, because a plug runs before there is a validated request to derive the origin from. A rejected request now widens nothing at all, and there is a test for it.
• Nothing else widens. The refute assertions check that neither another tenant's origin nor the configured https://app.test appears on a consent page for a different client.

OAuth.redirect_origins/0 (the library's) is no longer called from here. It stays on the instance for anything else that wants the config registry's origins.

Left to the next PR: the API surface.

🤖 Generated with Claude Code

https://claude.ai/code/session_01S9jevFQT5MkF3rJUieeiHW

Part of #1125

[BinaryBourbon]

This is the best PR in the stack and the one I would most want a future reader to find. Narrowing form-action from "every registered client's origins" to "this request's validated redirect origin" is required once the registry is tenant-writable — the old header would have handed the whole registry to any visitor and grown without bound — and the source of the narrowed value is the subtle half.

Deriving it from params["redirect_uri"] rather than from client.redirect_uris is right, and the reason is only visible if you have actually watched Chrome refuse it: with RFC 8252 any-port matching, a client registered on :5199 and legally asked for :5200 gets a header naming :5199, and the browser blocks a redirect the server just approved. Every HTTP-level assertion passes. The test named for it carries that story in a comment, which is what makes it survive the next refactor.

Verified the safety property the change depends on: allow_redirect_form_action/2 is only ever reached after validate_request/2 returns {:ok, _}, on all three branches (show, allow, deny), and the "an invalid request widens nothing" test pins the negative. Moving it from a plug to three explicit call sites is what made that possible — a plug cannot see a value the action has not validated yet.

Nits, both cosmetic:

• In create/2, the allow branch widens the CSP before calling authorize/3, so a :server_error renders an error page carrying a widened form-action. Harmless (no form on that page targets the origin), just slightly untidy; widening after the {:ok, code} would read tighter.
• OAuth.redirect_origins/0 — the config-wide version the macro generates — now has no production call site left; only oauth_test.exs:46 still asserts on it. Nothing to do, since it is library API, but worth knowing it is no longer load-bearing.

The IPv6 bracketing test (http://[::1]:5200) is a nice catch — URI.parse/1 strips the brackets and a CSP source expression needs them back.

Approving.

[jhgaylor]

Review feedback applied; force-pushed (amended commit).

The allow branch now widens form-action on the branch that actually redirects, after the code is minted, rather than before the call. A :server_error renders an error page, and that page has no business naming an app origin in its header. The deny branch and show were already correct.

[BinaryBourbon]

The nit is fixed and the comment explains the branch rather than the mechanic, which is what I wanted from it.

allow_redirect_form_action/2 now hangs off the {:ok, code} clause, so the :server_error render leaves the base form-action 'self' alone. I re-checked all four exits after the move:

Exit	Widened?	Right?
show {:ok, client} — renders the consent form	yes	yes; the form's own POST is same-origin but the redirect that follows it is enforced against this page's header
create allow → {:ok, code} — 302 to the app	yes	yes; this is the response Chrome checks the redirect against
create allow → {:error, _} — 500 error page	no, changed	correct; no form on that page targets the origin
create deny — 302 with error=access_denied	yes	yes
either validate_request {:error, reason}	no	unchanged, and the "an invalid request widens nothing" test still pins it

The comment earns its place by naming the branch rather than the call:

Widened on the branch that redirects, not before the mint: a :server_error renders an error page, and that page has no business naming an origin in its form-action.

A reader tidying the two redirect/2 calls back into one shared conn now has to delete that sentence to do it.

Nothing else moved. params["redirect_uri"] is still the source, which is the part that matters, and the IPv6 test is untouched.

Approving.