fix(security): harden OAuth flow defaults — PKCE, single-use refresh, drop implicit, validate state

[camreeves]

Summary

Four small changes to align the OAuth flow with OAuth 2.1 / RFC 6819 / RFC 7636 and close a CSRF gap in the OmniAuth strategy. Each change is in its own commit so reviewers can take them independently.

M1 — validate the state parameter by default

lib/omniauth/strategies/generic_oauth.rb was setting provider_ignores_state = true unconditionally. The original commit (3ae08a5) added it as a workaround for legacy providers that mishandle state in multi-factor flows, but it disables OmniAuth's default CSRF protection (RFC 6749 §10.12) for every authority.

Switch the default to enabled and expose the legacy behaviour as opt-in via the env var OAUTH_PROVIDER_IGNORES_STATE=true for deployments that still need it.

M2 + M4 — require PKCE and drop the implicit grant flow

Two changes in config/initializers/doorkeeper.rb:

• Add force_pkce. Public clients using the authorization_code grant must now send a code_challenge on /oauth/authorize and a matching code_verifier on /oauth/token (RFC 7636). Confidential clients are unaffected.
• Remove implicit from grant_flows. OAuth 2.1 §2.1.2 drops the flow; response_type=token now returns unsupported_response_type. implicit_oidc is retained — it's defined by a different spec and still used by some identity-token consumers.

M3 — revoke refresh tokens on use

With oauth_access_tokens.previous_refresh_token present, Doorkeeper takes the deferred-revoke path at lib/doorkeeper/oauth/refresh_token_request.rb:37: the caller-supplied refresh token stays valid, the new pair stores its value in previous_refresh_token, and the old token is only revoked later when revoke_previous_refresh_token! is invoked on the new access token. Until that follow-up call lands, the same refresh token can mint additional token pairs, which contradicts RFC 6819 §5.2.2.3 (single-use refresh).

The migration drops the column. refresh_token_revoked_on_use? then returns false, so before_successful_response revokes the old refresh token in the same transaction that mints the new pair. The now-no-op revoke_previous_refresh_token! call in AuthoritiesController#current is removed.

Trade-off: there's no longer a grace window for a client to retry with the same refresh token if it lost the response. The accepted behaviour is re-authentication, consistent with the RFC.

Backwards compatibility notes

• force_pkce — rejects authorization_code requests from public clients that don't send PKCE. Confidential clients are unaffected. Deployments with non-PKCE public clients need to update the clients or mark them confidential.
• Removing implicit from grant_flows — response_type=token requests start returning unsupported_response_type. implicit_oidc is retained.
• Refresh token migration — existing rows lose their previous_refresh_token value, so any in-flight refresh relying on the deferred-revoke grace window will need a re-auth instead. The migration is reversible.

Test plan

• CI to exercise the existing auth specs (no new tests added — repo doesn't currently have OAuth-flow coverage to extend)
• Manual verification on a deployment: response_type=token returns unsupported_response_type; /oauth/token without code_verifier from a public client returns invalid_request; second /oauth/token call with the same refresh token returns invalid_grant; OAuth callback without state returns CSRF error

🤖 Generated with Claude Code

[camreeves]

Here's the pentest report this PR references: DataArt_McKinsey.Converge_PenetrationTesting_Report_v1.0_05152026.pdf

[camreeves]

OAuth client audit (McKinsey deployment) for force_pkce + dropping the implicit grant

I went through every OAuth client on the McKinsey stack to confirm what breaks under M2 (force_pkce) and M4 (removing implicit from grant_flows). Result: no actively-deployed McKinsey client is broken by these changes.

Client	Auth flow	Verdict
Staff (SPA)	was implicit (response_type=token); now response_type=code + PKCE	Safe. Fixed in the frontend (auth_type: 'auth_code'), deployed to UAT and verified (authorize uses code + code_challenge=S256, token exchanged with code_verifier, no token in the URL fragment).
Concierge (SPA)	same as Staff	Safe. Same fix, deployed to UAT.
Backoffice (SPA)	response_type=code + PKCE already (this is the client the PKCE-downgrade finding was tested against)	Safe.
Booking panel / mobile (Flutter)	X-API-KEY on PlaceOS (no OAuth token flow at all); password + refresh_token grants only in legacy ACA-Engine mode	Not affected. password/refresh_token are retained, and its refresh logic stores the rotated token, so the M3 single-use change is also handled.
Kiosk	a module inside the Staff app, sharing the same ts-client setup	Safe (covered by the Staff fix).
Drivers (mck-drivers)	server-side; the o365 driver uses client_credentials	Not affected (client_credentials is retained).
analytics (Chronograf)	a registered OAuth app, but Chronograf is not deployed on this stack (/analytics/ returns 404 and it is absent from the EKSv2 charts)	Moot. If it is ever deployed it would need PKCE, or to be registered as confidential: true, since Chronograf does not send a code_challenge. Worth removing the stale registration as cleanup.

Note: every app is registered confidential: false (the PlaceOS default), so force_pkce applies to all of them. Staff/Concierge/Backoffice pass because they send PKCE; the panel and drivers don't use the authorization-code grant at all.

Critical deployment sequencing

The Staff/Concierge frontend change that moves them onto response_type=code + PKCE is the prerequisite for this PR. It is live on UAT, but the prod frontend is currently pinned (blocked by the levels regression). This backend change must not reach prod until the Staff/Concierge auth_type: 'auth_code' frontend build is also in prod, otherwise prod Staff/Concierge logins will start returning unsupported_response_type (and, once they fall back, PKCE-required).

Frontend fix for reference: mckinsey-converge 50a900628 (auth_type: 'auth_code') plus e267e5450 (clears the consumed OAuth callback params), both on uat-placeos.

M3 (single-use refresh) compatibility

The clients that use refresh tokens are the ts-client SPAs (Staff/Concierge/Backoffice) and the panel in legacy ACA mode. ts-client stores the rotated refresh_token from each token response, and the panel's refresh path does the same, so single-use refresh does not break them. Existing outstanding refresh tokens will still need the one-off data cleanup at prod-migration time (already on the list).

CI note

The red checks on this PR (security, style, tests) are pre-existing repo-infrastructure failures, the jobs use stale third-party actions (Ruby 2.6 images, missing docker-compose/make) and fail on every master commit too. The meaningful Build job passes.

[camreeves]

Heads up on a force-push I just made to this branch while re-reviewing before requesting review — I amended the PKCE/implicit commit (a9bce2f → 6c18f8d) to fix a real defect in the implicit-grant removal.

What was wrong: the commit dropped bare implicit from grant_flows but kept implicit_oidc. implicit_oidc isn't a flow — it's a doorkeeper-openid_connect alias registered as: ['implicit', 'id_token', 'id_token token'] (openid_connect.rb:83). Doorkeeper's calculate_grant_flows expands aliases into the effective flow set, so bare implicit (response_type_matches: 'token') got silently re-added. Net effect: response_type=token was still accepted and the implicit-flow removal was a no-op, despite the commit message.

The fix: list the OpenID Connect identity-token flows explicitly instead of the alias:

grant_flows ["authorization_code", "client_credentials", "password", "id_token", "id_token token"]

This drops implicit for real (response_type=token now returns unsupported_response_type) while retaining OIDC id_token support. Verified by replicating calculate_grant_flows: with the old implicit_oidc config the effective set contained "implicit"; with the explicit flows it does not.

One open question for you: I kept id_token token, but note it also returns an access token in the URL fragment (same shape as plain implicit). Our client audit found no deployed client actually uses OIDC implicit, so we could drop id_token/id_token token entirely for a stricter posture. I left them in to match the original intent — happy to tighten if you'd prefer.