feat(endpoint-auth): validate cross-host redirect_uri against client_id - #872
Conversation
|
Addressing the TODO seems like a good improvement. The wildcard addition definitely seems like something to review. I'd be nervous extending auth code. @rmdes What the Omnibear browser extension does is it uses a page script to pull the auth from a redirect to omnibear.com rather than redirecting to the extension itself. Worth considering for Plume. |
|
Sounds like the Oauth working group strongly discourages wildcards indieweb/indieauth#22 (comment) |
031ce14 to
18daa92
Compare
|
You're right, and it turns out the wildcard isn't needed at all. I've removed it. Thanks for the link — that's about as authoritative as it gets, and it prompted me to actually check the assumption underneath my patch. I'd believed a browser extension can't predeclare its callback because the browser issues the host. That's wrong in both browsers:
So both callbacks are fixed and declarable. I've replaced the wildcards on Plume's That leaves the PR doing only what the Worth flagging one caveat I can't rule out: Firefox's SHA-1 derivation is observed behaviour, not a documented guarantee — MDN says only "derived from your extension's ID". Stable in practice, but a literal declaration would break if Mozilla ever changed it, where a wildcard wouldn't. I think that's the right trade given the security argument. On Omnibear — thanks, I didn't know it worked that way. Redirecting to a page on the |
ade3f7b to
2b7c903
Compare
redirect_uri against client_id
validateRedirect only compared hosts, leaving a @todo for the rest of the specification: when a redirect URI is on a different host to client_id, it must be checked against the URIs declared at the client_id URL. Because that was never implemented, every client whose callback lives on another host was rejected with 'Invalid value provided for redirect_uri'. Browser extensions are the common case. Their redirect host is issued by the browser (<extension-id>.chromiumapp.org in Chrome, sha1(<extension-id>).extensions.allizom.org in Firefox) and never equals the client_id host, so no browser extension could complete an IndieAuth flow against Indiekit. Fetch client_id and collect redirect URIs declared in <link rel="redirect_uri"> tags and Link HTTP headers. HTML is parsed with microformats-parser, already a dependency here and already used by client.js, which resolves relative hrefs against the base URL for free. Declared URIs are matched exactly, ignoring only a trailing slash on the path. Patterns are deliberately unsupported: the OAuth working group's position is that wildcards in redirect URLs open up attack vectors, and extensions do not need them — both browsers derive the redirect host from the extension ID, so it is fixed for a given extension and can be declared literally. Validation fails closed: a network error, non-2xx response or unparseable markup yields no declared URIs and therefore no match. Fetches time out after 5 seconds. validateRedirect becomes async, so its two callers now await it; codeValidator becomes async to do so. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
18daa92 to
e5a9e4f
Compare
|
This looks great, thanks @rmdes. And yes, best that this PR implemented only the |
Aligns this fork with the implementation proposed upstream in getindiekit/indiekit#872. Wildcard support is removed. The OAuth working group's position is that any wildcard support in redirect URLs opens up attack vectors, and the browser extension case that motivated it does not actually need one: Chrome derives its callback host from the extension id, and Firefox uses sha1(browser_specific_settings.gecko.id), so both are fixed for a given extension and can be declared literally. Plume's client_id page now declares both literally. See indieweb/indieauth#22 (comment) Other changes carried over from the upstream version: - HTML is parsed with microformats-parser rather than regexes. It is already a dependency, and it resolves relative hrefs against the base URL, so a declared href="/callback" now works. - The per-client_id cache is gone. client_id is supplied by whoever begins the authorization request, so caching by it is an unbounded map keyed on untrusted input. Client information discovery already fetches client_id uncached on every request, so this matches existing behaviour. - The _clearRedirectCache test-only export goes with it. Tests are rewritten without undici and without @indiekit-test helpers, neither of which resolve in this standalone package; the suite's two redirect failures are fixed as a result. Adds a regression test pinning Plume's two literal callbacks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Carries two fixes since beta.31: - Redirect URIs declared at a client_id are matched exactly, with wildcard support removed. The OAuth working group's position is that wildcards in redirect URLs open up attack vectors, and the browser extension case that motivated them does not need one: both Chrome and Firefox derive the callback host from the extension id, so it is fixed and declarable. Matches the implementation merged upstream in getindiekit/indiekit#872. - Client information discovery is restricted to public HTTP(S) origins, and no longer throws on responses it cannot use. client_id is supplied by whoever begins an authorization request, so it decided where this server sent a request; loopback, link-local, private and unspecified addresses are now refused, along with the numeric host encodings that resolve to them. Separately, fetch had no error handling and mf2() throws on an empty or non-HTML body, so a client that was briefly unreachable or served JSON failed the whole authorization request instead of falling back. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem
validateRedirectonly compares hosts, with the rest of the specificationleft as a
@todo:Because the cross-host branch was never implemented, any client whose
callback lives on a different host to its
client_idis rejected with"Invalid value provided for
redirect_uri", even when that clientcorrectly declares the URI as the spec requires.
Browser extensions are the case people actually hit. The browser issues the
redirect host, and it can never equal the client's own host:
<extension-id>.chromiumapp.org<uuid>.extensions.allizom.orgSo no browser-extension IndieAuth client can complete a login against
Indiekit today. This was reported by a user signing in to their own server
(beta 28) from Firefox; it reproduces identically in Chrome.
What this does
Implements the
@todo. When hosts differ, fetchclient_idand collect theredirect URIs it declares, via
<link rel="redirect_uri">tags andLinkHTTP headers, then match the request against them.
microformats-parser, already a dependency of thispackage and already used by
client.js. It exposesrels.redirect_uridirectly and resolves relative
hrefs against the base URL, sohref="/callback"works. No new dependencies.Linkheaders are parsed for values whoserelincludesredirect_uri, handling quoted and unquoted forms and multi-valuerel.yields no declared URIs, so nothing matches. Fetches time out after 5s.
Subdomain wildcards — the part worth discussing
A declared URI may replace its left-most host label with
*.:This isn't in the IndieAuth specification, and I'd rather flag it than bury
it. The motivation is that an extension's redirect host is generated per
installation, so a literal declaration is impossible — the client genuinely
cannot know its own callback host ahead of time.
It's deliberately narrow:
So
https://*.example.com/acceptshttps://abc.example.com/and rejectshttps://example.com/,https://a.b.example.com/,http://abc.example.com/andhttps://abc.example.com/other.If you'd prefer wildcards gated behind an option, or dropped entirely, the
rest of the change stands on its own — declared-URI matching is useful
regardless.
Behavioural changes
validateRedirectis now async. Both callers await it, andcodeValidatorbecomes async to do so.client_id. Same-host requests are unaffected.I deliberately did not add caching.
getClientInformationalreadyfetches
client_iduncached on every authorization request, so this matchesexisting behaviour; and since
client_idis attacker-supplied, a naiveper-
client_idcache is an unbounded map keyed by untrusted input. Happy toadd a bounded cache if you'd like one.
Testing
test/unit/redirect.js; the original assertions are unchanged.25/25 unit tests pass in
endpoint-auth(up from 17).helpers/mock-agent/endpoint-auth.js, following theexisting
mockAgent("endpoint-auth")convention.<link>tag and viaLinkheader; undeclared andunrelated hosts; path and scheme mismatch; wildcard match; wildcard
rejection at the apex, at a deeper subdomain, and on a different domain;
unfetchable
client_id; malformed URLs.eslintandprettierclean.Verified end to end against a real client: with this change, a
client_idpage declaring
https://*.chromiumapp.org/andhttps://*.extensions.allizom.org/accepts the Chrome and Firefox callbacksand rejects an undeclared host.