From 4dd0a73d3c339173a4edf61676409c1afb472ced Mon Sep 17 00:00:00 2001 From: Cam Reeves Date: Mon, 18 May 2026 19:49:22 +1000 Subject: [PATCH 1/3] fix(omniauth): validate state parameter by default on oauth2 callback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OmniAuth validates the OAuth2 `state` parameter on the callback as a defence against CSRF (RFC 6749 §10.12). The custom strategy was setting `provider_ignores_state = true` unconditionally — added originally to work around legacy providers that mishandle state in multi-factor or cross-domain flows — which disabled the protection for every authority. Switch the default to enabled and expose the legacy behaviour behind the `OAUTH_PROVIDER_IGNORES_STATE` env var so deployments that need it can opt back in explicitly. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/omniauth/strategies/generic_oauth.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/omniauth/strategies/generic_oauth.rb b/lib/omniauth/strategies/generic_oauth.rb index aa63365..00150d2 100644 --- a/lib/omniauth/strategies/generic_oauth.rb +++ b/lib/omniauth/strategies/generic_oauth.rb @@ -77,8 +77,11 @@ def set_options(id) options.client_id = strat.client_id options.client_secret = strat.client_secret - # prevent csrf errors - options.provider_ignores_state = true + # Validate the OAuth2 `state` parameter on callback (OmniAuth default). + # Some legacy providers mishandle `state` during multi-factor or + # cross-domain flows; operators can opt out by setting the env var + # `OAUTH_PROVIDER_IGNORES_STATE=true`. + options.provider_ignores_state = ENV["OAUTH_PROVIDER_IGNORES_STATE"] == "true" end def access_token_options From aab871d1867a70a0262ba3a66502922a44ebeaf6 Mon Sep 17 00:00:00 2001 From: Cam Reeves Date: Mon, 18 May 2026 19:50:16 +1000 Subject: [PATCH 2/3] fix(doorkeeper): require PKCE and drop the implicit grant flow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two OAuth 2.1 alignment changes for the authorization server: * `force_pkce` enabled. Public clients are now required to send a `code_challenge` on /oauth/authorize and a matching `code_verifier` on /oauth/token (RFC 7636). Confidential clients are unaffected. * The OAuth 2.0 implicit grant is removed so `response_type=token` now returns `unsupported_response_type` (OAuth 2.1 §2.1.2). Note: we deliberately do NOT use the `implicit_oidc` grant-flow alias. doorkeeper-openid_connect registers it as an alias for ["implicit", "id_token", "id_token token"], and Doorkeeper's calculate_grant_flows expands aliases into the effective flow set — so listing `implicit_oidc` would silently re-enable the bare `implicit` flow and keep `response_type=token` working. The OpenID Connect identity-token flows are therefore listed explicitly (`id_token`, `id_token token`), which drops `implicit` while retaining OIDC support. Co-Authored-By: Claude Fable 5 --- config/initializers/doorkeeper.rb | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb index 81f79e3..bbb8f36 100644 --- a/config/initializers/doorkeeper.rb +++ b/config/initializers/doorkeeper.rb @@ -119,7 +119,23 @@ access_token_generator "::Doorkeeper::JWT" force_ssl_in_redirect_uri false - grant_flows %w[authorization_code client_credentials implicit password implicit_oidc] + + # Require non-confidential clients to use PKCE (RFC 7636) on + # authorization_code grants. Public clients without a code_verifier are + # rejected by Doorkeeper at the /oauth/token exchange. + force_pkce + + # Drop the OAuth 2.0 implicit grant (`response_type=token`). OAuth 2.1 + # §2.1.2 removes it; clients should use authorization_code with PKCE. + # + # NOTE: we must NOT use the `implicit_oidc` alias here. doorkeeper-openid_connect + # registers it as an alias for `["implicit", "id_token", "id_token token"]` + # (openid_connect.rb:83), and Doorkeeper's calculate_grant_flows expands + # aliases into the effective flow set — so listing `implicit_oidc` would + # silently re-enable the bare `implicit` flow and keep `response_type=token` + # working. Instead we list the OpenID Connect identity-token flows + # explicitly, which drops `implicit` while retaining OIDC id_token support. + grant_flows ["authorization_code", "client_credentials", "password", "id_token", "id_token token"] end Doorkeeper::JWT.configure do From 6c18f8dacd24323ff4ad15edd8e3354f41da3e61 Mon Sep 17 00:00:00 2001 From: Cam Reeves Date: Mon, 18 May 2026 19:50:53 +1000 Subject: [PATCH 3/3] fix(doorkeeper): revoke refresh tokens on use to prevent reuse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the `oauth_access_tokens.previous_refresh_token` column is present, Doorkeeper switches the refresh flow from immediate to deferred revocation (lib/doorkeeper/oauth/refresh_token_request.rb:37): the caller-supplied refresh token is left valid, the new pair stores its value in `previous_refresh_token`, and the old token is only revoked later when the application invokes `revoke_previous_refresh_token!` on the new access token. Until that follow-up call lands, the same refresh token can mint additional token pairs — i.e. refresh tokens are not single-use, which contradicts RFC 6819 §5.2.2.3. Drop the column. `refresh_token_revoked_on_use?` now returns false, so `before_successful_response` revokes the old refresh token in the same transaction that mints the new pair. The explicit `revoke_previous_refresh_token!` call in AuthoritiesController#current is no longer needed and is removed. Trade-off: the deferred-revoke pattern existed so a client that lost the response to a refresh request could safely retry with the old token. Single-use refresh removes that grace window; clients that hit a network failure mid-refresh re-authenticate, which is the recommended posture. Co-Authored-By: Claude Opus 4.7 (1M context) --- app/controllers/auth/authorities_controller.rb | 1 - ...us_refresh_token_from_oauth_access_tokens.rb | 17 +++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20260518095026_remove_previous_refresh_token_from_oauth_access_tokens.rb diff --git a/app/controllers/auth/authorities_controller.rb b/app/controllers/auth/authorities_controller.rb index d5dc01b..2510b89 100644 --- a/app/controllers/auth/authorities_controller.rb +++ b/app/controllers/auth/authorities_controller.rb @@ -15,7 +15,6 @@ def current begin access_token = doorkeeper_token if access_token - access_token.revoke_previous_refresh_token! auth[:token_valid] = true configure_asset_access else diff --git a/db/migrate/20260518095026_remove_previous_refresh_token_from_oauth_access_tokens.rb b/db/migrate/20260518095026_remove_previous_refresh_token_from_oauth_access_tokens.rb new file mode 100644 index 0000000..034a6f6 --- /dev/null +++ b/db/migrate/20260518095026_remove_previous_refresh_token_from_oauth_access_tokens.rb @@ -0,0 +1,17 @@ +class RemovePreviousRefreshTokenFromOauthAccessTokens < ActiveRecord::Migration[8.1] + # Dropping `previous_refresh_token` switches Doorkeeper's refresh flow + # from deferred revocation to immediate revocation: once a refresh token + # is used, it's marked revoked before the new pair is returned, so the + # same refresh token can't mint another set of tokens. + # + # Trade-off: there is no longer a grace window for the client to retry + # if it lost the response to the new pair. The accepted behaviour is to + # re-authenticate, in line with RFC 6819 §5.2.2.3. + def up + remove_column :oauth_access_tokens, :previous_refresh_token + end + + def down + add_column :oauth_access_tokens, :previous_refresh_token, :string, null: false, default: "" + end +end