fix(endpoint-auth): accept code exchange without grant_type at the authorization endpoint

[rmdes]

Fixes #890.

codeValidator requires grant_type on both routes it guards — the
authorization endpoint, where a code is exchanged for a profile URL, and the
token endpoint, where it is exchanged for an access token.

Clients predating the specification that introduced the parameter perform the
profile exchange without it. indieauth.com is one. I originally cited its
developer documentation for that; its source is more direct. The verification
request is built at
controllers/auth-web.rb:562:

data = RestClient.post session[:attempted_profile], {
  :code => params[:code],
  :client_id => "https://#{request.host}/",
  :redirect_uri => "https://#{request.host}/auth/indieauth/redirect"
}, :accept => 'application/json'

Three parameters; grant_type appears nowhere in that repository. So signing in
to an Indiekit site through it fails after the user has authenticated and been
redirected back:

{"error":"bad_request","error_description":"Missing parameter: `grant_type`"}

The change

const isProfileExchange = request.path === "/";

const requiredParameters = ["client_id", "code", "redirect_uri"];
if (!isProfileExchange) {
  requiredParameters.push("grant_type");
}

and the value check becomes (grant_type ?? "authorization_code") !== "authorization_code", so a parameter that is present but wrong is still
rejected on both routes.

Why scope it rather than drop the requirement

Only the authorization endpoint needs the allowance. For indieauth.com this is
checkable rather than assumed: token_endpoint appears nowhere in its source,
and its only outbound request to a user's server is the profile exchange quoted
above, so it never reaches /token at all.

Generalising from that one client is a judgement, not a measurement — I have not
surveyed every client that omits grant_type. But the asymmetry favours
scoping: the authorization endpoint returns a profile URL, /token issues
credentials, and widening validation there buys compatibility only for a client
that would have to be simultaneously old enough to omit the parameter and new
enough to want a token. /token therefore keeps requiring it.

Tests

• 200-authorization-profile-no-grant-type.js — the profile exchange without
  grant_type. Fails on main with 400, passes with this change.
• 400-token-grant-no-grant-type.js — the token endpoint still rejects its
  absence. This passes on main too; there was no coverage for that case, so
  nothing would have caught the allowance being widened to both routes.

node --test in packages/endpoint-auth: 68 tests, 68 passing with the
change. Reverting only code.js and keeping both tests leaves exactly one
failure, the new profile-exchange test. main is 66/66 before this change.

Relationship to #884

That PR fixes the same class of problem one step earlier, where response_type
is required of clients that predate it. They are independent — this branch is
cut from main, not from that one — but a client using indieauth.com needs
both to sign in: without #884 the authorization request is rejected, and
without this the code exchange is. Both are now confirmed together against a
live deployment; see the comment below.

Relationship to #893

#893 also changes packages/endpoint-auth/lib/middleware/code.js, and I want to
flag an interaction I only found after opening both.

The two merge cleanly — this PR adds a grant_type allowance to the
required-parameter list, #893 replaces the checks that follow it, and git
resolves that without conflict. But the merged result fails a test, and it is
this PR's: 200-authorization-profile-no-grant-type.js signs a code carrying
neither client_id nor redirect_uri, and #893 begins rejecting codes missing
those claims. It passes on either branch alone and fails on both together.

Adding the two claims to that fixture resolves it, after which the combined
branches give 70 tests, 70 passing.

So if this merges second, CI goes red until that one-line fixture change is
made, and the cause is in this branch rather than in #893. Nothing structural,
but it will not go green on the merge alone.

Update: the client this affects is deprecated

indieauth.com now carries a deprecation notice — the service is being retired in
favour of indielogin.com.

The replacement does not need this fix. When it delegates to a user's own
IndieAuth server it sends the parameters: response_type=code is set by
indieauth-client-php
(src/IndieAuth/Client.php:435),
and grant_type=authorization_code together with code_verifier at
app/Provider/IndieAuth.php:73.

So this is compatibility with a service on its way out, and worth weighing on
that basis. Against that: indieauth.com is still serving, and people still sign
in through it — confirmed against a live Indiekit deployment today — so the
breakage is real until the shutdown happens.

I also wrote above that indieauth.com is what the IndieWeb wiki authenticates
with. I went to verify that and could not: I found no reference to either
service on indieweb.org's login or front page. I should not have asserted it, so
treat that claim as withdrawn — it does not affect the rest, which is about what
indieauth.com sends.

[rmdes]

The issue this fixes now carries direct source evidence rather than the documentation inference it originally cited: indieauth.com's verification request is built at controllers/auth-web.rb:562 and sends code, client_id and redirect_uri only — grant_type appears nowhere in that repository. See #890 (comment).

[rmdes]

Confirmed end to end against a live Indiekit deployment: signing in through
indieauth.com now completes, returning "You've successfully authenticated as
https://rmendes.net/".

To be precise about what that does and does not evidence. The deployment runs a
fork of this package, but lib/middleware/code.js there is byte-identical to
this branch, so the code exercised is the change proposed here.

Both fixes were needed together. With response_type handled (#884) but not
this one, the user authenticates, is redirected back, and the exchange then
fails with Missing parameter: grant_type — the failure is invisible until
after consent, which makes it look like the sign-in worked.

One thing this surfaced that is out of scope here, but is a real bug in main:
a POST to /auth without a preceding authorization request crashes with an
unhandled TypeError: Cannot read properties of undefined (reading 'id') and a
500, at the client.id comparison, because client is read from
request.app.locals and is undefined when no authorization request has
populated it. Reachable unauthenticated. Happy to open a separate issue if
useful.

[paulrobertlloyd]

Same thoughts as on #884, ultimately this feels like better UX and a more robust approach broadening compatibility with services using older IndieAuth services. I just want to check, is this behaviour compatible with the IndieAuth spec do you think?

[rmdes]

Same trade-off as #884, and the spec answer is firmer: no. grant_type=authorization_code is required in both revisions — I checked the 2020 one specifically, expecting it to be the lenient one, and it isn't. There's no "the older spec allowed this" defence.

Same asymmetry too. indielogin.com sends it (IndieAuth.php:73), along with code_verifier. indieauth.com doesn't — its verification request is at auth-web.rb:562 and sends code, client_id, redirect_uri only. The string grant_type appears nowhere in that repository.

What makes me more comfortable with this one than #884: it's scoped to the authorization endpoint, and /token still requires the parameter. Nothing old enough to omit grant_type is requesting an access token, so the leniency sits only on the profile exchange, and the route that issues credentials keeps enforcing the spec. A grant_type that's present but wrong is still rejected on both.

Still your call whether that's worth carrying for a deprecated client.