[eliasjpr]

Summary

Fixes the two P0 findings of the security review (and the device-approval P1 that shares the root cause of P0-1).

P0-1: no authentication at the authorization endpoint

Before. AuthorizationHandler read user_id and consent=approved from the query string:

GET /oauth/authorize?response_type=code&client_id=APP&redirect_uri=<registered>&scope=read&state=s1                       -> consent page (stores s1)
GET /oauth/authorize?...&state=s1&consent=approved&user_id=admin                                                           -> 302 <registered>?code=<code for admin>&state=s1
GET /oauth/authorize?response_type=token&client_id=IMPLICIT_APP&redirect_uri=...&scope=anything&consent=approved          -> 302 <registered>#access_token=<token with any scope>

The state check only validated a value the same caller had stored; PAR also let the query string override scope, redirect_uri, user_id.

After. The subject comes only from an authenticated session supplied by the host, through one hook:

Authly.configure do |config|
  # String? = subject id of the signed-in resource owner, nil = nobody
  config.authenticate_user = ->(context : HTTP::Server::Context) { MySession.user_id(context) }
  # optional: where to send a visitor who is not signed in (otherwise 401 {"error":"login_required"})
  config.login_url = ->(context : HTTP::Server::Context) { "/login?return_to=#{URI.encode_www_form(context.request.resource)}" }
  config.consent_ttl = 10.minutes
end

• Fail closed: while authenticate_user is nil, /oauth/authorize and /device answer 500 ("set Authly.config.authenticate_user") and issue nothing; query/body parameters are never trusted.
• user_id and consent (query or body) are ignored. PAR: only the pushed parameters are used (query cannot add/override), and user_id, consent, client_secret are no longer stored with a pushed request.
• Validation before anything is rendered or redirected: exact redirect_uri match against the client registration, response_type, grant type allowed, PKCE rules, and requested scope against the client's scopes (code and implicit paths; invalid_scope redirect to the validated URI).
• Consent is a POST (Authly::Handler now routes POST /oauth/authorize). The consent page's form carries one consent_token: a signed (HMAC-SHA256 keyed from secret_key, purpose-bound), expiring token that binds client, redirect URI, scope, state, PKCE data, response type and the authenticated subject, plus a single-use nonce (consumed through state_store). The POST must come from the same signed-in subject, so it is also the anti-CSRF token: a token obtained by an attacker for their own session is rejected (403) when replayed against a victim, a tampered/foreign/expired/replayed token is rejected (400), and the POST cannot change scope or redirect URI.
• Implicit tokens now carry the authenticated subject (before: random sub).
• /device: the approving user is the signed-in owner (hook), not a user_id form field; the form has a signed single-use token bound to that owner. Device user codes use Random::Secure (was Random).

P0-2: reflected XSS on the consent and device pages

Before. GET /oauth/authorize?client_id="><script>... and GET /device?user_code="><script>... executed script on the authorization server's origin (client_id, scope, state, redirect_uri, user_code, and error messages were interpolated raw, in text and value="..." attributes).

After. Every interpolated value goes through HTML.escape (text and attribute contexts); unregistered redirect_uri/unknown client_id are rejected with a plain-text error that does not echo input, before any rendering; the consent, device and device error/success pages send Content-Security-Policy: default-src 'none'; style-src 'unsafe-inline'; base-uri 'none'; frame-ancestors 'none', X-Frame-Options: DENY, X-Content-Type-Options: nosniff, Cache-Control: no-store, Referrer-Policy: no-referrer. (No form-action directive on purpose: Chrome applies it to the redirect that follows the consent POST.)

Breaking change and migration

The hook is mandatory for AuthorizationHandler / Authly::Handler (/oauth/authorize) and DeviceVerificationHandler (/device). Without it they answer 500. Set config.authenticate_user (see above). Clients/tests that sent consent=approved&user_id=... must go through the consent page (GET, then POST consent_token + decision=approve|deny). state is passed through to the client but no longer checked against a store the caller filled. Hosts that call Authly.code(...) themselves are unaffected. Details are in CHANGELOG.md ("Security", "Upgrading") and README.md ("Authorization Endpoint").

What authority must change

azutoolkit/authority does not mount Authly::Handler's authorization/device handlers: it has its own Authorize::NewEndpoint/CreateEndpoint (POST /authorize, user from current_session.user_id, consent stored via Consent.grant!) and calls Authly.code(...) directly. So: nothing is required to keep working. If authority wants Authly's built-in endpoints or its device page (DeviceAuthorizationHandler is used for device codes in ClientProvider), it sets Authly.config.authenticate_user = ->(ctx) { session user id from its session middleware } and login_url to its login page, and mounts Authly::Handler. Separately, authority's own authorize pages and /authorize POST should get the same review (client/redirect/scope validation before rendering, escaping, CSRF token on the consent POST); not covered here.

Tests

Failing specs first (see spec/handlers/authorization_security_spec.cr, 33 examples, red against the old handlers: query bypass issued a code for admin, implicit token without a user, unescaped <script> in the page), covering: fail-closed without the hook (code, implicit, POST, device), 401/login redirect, user_id/consent ignored, consent POST without/with tampered/foreign/other-subject/replayed token, deny, scope escalation (code and implicit), unregistered redirect_uri / unknown client not reflected, XSS payloads in client_id/scope/state/user_code/error message, security headers, PAR override attempts, PAR not storing user_id/consent, device approval with the authenticated subject only, FormToken round trip/expiry/tamper. Existing handler and PAR specs were moved to the new flow (spec/support/authorization_flow.cr); the PAR "expired request_uri" spec had been passing for the wrong reason (invalid state) and now really expires the request.

Results: crystal spec 455 examples, 0 failures; crystal tool format --check src spec clean; Ameba (crystal run lib/ameba/bin/ameba.cr) 92 files, 0 failures; all on the CI image (crystallang/crystal, latest). On Crystal 1.19.1 the suite does not compile on master either (jarm_builder.cr:247: OpenSSL::Cipher#gcm_tag missing; shard.yml requires >= 1.22.0), unrelated to this change.

Review follow-ups (second commit)

PAR request_uri consumed only after sign-in (PAR + login_url works); device page shows client and scopes before approval; unknown client / unregistered redirect_uri / missing grant is 400; StateStore#consume returns Bool and is the only check (custom stores: breaking, documented), InMemoryStateStore mutexed and sweeping; multi-node (secret_key, shared state_store) and PKCE notes in the README; subject stripped; forms post to their own path; blank scope is invalid_scope, allowed_scopes? ignores doubled spaces; form_post.jwt page under a script-hash CSP with no inline handler.

Not in this PR (remaining authly findings from the review)

• device_code subject (DeviceCode has no subject), approval repeatable, attempt limit; JARM key separation

• P1: refresh tokens: no revocation / token_type / client binding check, rotation is a no-op, refreshed tokens lose scope, no reuse detection

• P1: device flow: per-code / per-IP attempt limit (user-code RNG is fixed here), and showing the client and scope on the device page before approval

• P1: anonymous dynamic client registration accepts any grant types/scopes/redirect URIs (javascript:// passes) and has no body size limit

• P1: /introspect and /revoke client authentication

• The implicit grant also issues a refresh token (AccessToken always does)

---

Generated by Claude Code