Non-confidential apps should always re-prompt for user consent

nbudin commented 2 years ago

(Mentioning @rgammans, since he told me about this issue and wanted to be notified about replies.)

Steps to reproduce

Log into the server application (in whatever way is necessary to pass its resource_owner_authenticator check)
Create a non-confidential application in Doorkeeper admin
Initiate authentication against it using its client ID and any OAuth2 flow
Click "allow" to let the access grant happen
Initiate authentication against it again, using a flow that only includes client_id, such as the authorization_code flow with PKCE

Expected behavior

According to RFC 8252 section 8.6, the authentication server should re-prompt for user consent, since the client's identity cannot be assured simply from the client_id parameter. (In Doorkeeper, I think this means that it should redirect to AuthorizationsController#new.)

Actual behavior

The authentication server immediately redirects with a code parameter.

System configuration

You can help us to understand your problem if you will share some very useful information about your project environment (don't forget to remove any confidential data if it exists).

Doorkeeper initializer:

# config/initializers/doorkeeper.rb
class NullResourceOwner
  def id
    nil
  end
end

Doorkeeper.configure do
  Devise::Doorkeeper.configure_doorkeeper(self)

  # Change the ORM that doorkeeper will use (needs plugins)
  orm :active_record

  # This block will be called to check whether the resource owner is authenticated or not.
  resource_owner_authenticator do
    user_signed_in? ? current_user : NullResourceOwner.new
  end

  # If you didn't skip applications controller from Doorkeeper routes in your application routes.rb
  # file then you need to declare this block in order to restrict access to the web interface for
  # adding oauth authorized applications. In other case it will return 403 Forbidden response
  # every time somebody will try to access the admin web interface.
  #
  admin_authenticator { authorize Doorkeeper::Application.new, :manage? }

  # If you are planning to use Doorkeeper in Rails 5 API-only application, then you might
  # want to use API mode that will skip all the views management and change the way how
  # Doorkeeper responds to a requests.
  #
  # api_only

  # Enforce token request content type to application/x-www-form-urlencoded.
  # It is not enabled by default to not break prior versions of the gem.
  #
  # enforce_content_type

  # Authorization Code expiration time (default 10 minutes).
  #
  # authorization_code_expires_in 10.minutes

  # Access token expiration time (default 2 hours).
  # If you want to disable expiration, set this to nil.
  #
  # access_token_expires_in 2.hours

  # Assign custom TTL for access tokens. Will be used instead of access_token_expires_in
  # option if defined. `context` has the following properties available
  #
  # `client` - the OAuth client application (see Doorkeeper::OAuth::Client)
  # `grant_type` - the grant type of the request (see Doorkeeper::OAuth)
  # `scopes` - the requested scopes (see Doorkeeper::OAuth::Scopes)
  #
  # custom_access_token_expires_in do |context|
  #   context.client.application.additional_settings.implicit_oauth_expiration
  # end

  # Use a custom class for generating the access token.
  # See https://github.com/doorkeeper-gem/doorkeeper#custom-access-token-generator
  #
  access_token_generator '::Doorkeeper::JWT'

  # The controller Doorkeeper::ApplicationController inherits from.
  # Defaults to ActionController::Base.
  # See https://github.com/doorkeeper-gem/doorkeeper#custom-base-controller
  base_controller 'ApplicationController'

  # Reuse access token for the same resource owner within an application (disabled by default)
  # Rationale: https://github.com/doorkeeper-gem/doorkeeper/issues/383
  #
  # reuse_access_token

  # Issue access tokens with refresh token (disabled by default), you may also
  # pass a block which accepts `context` to customize when to give a refresh
  # token or not. Similar to `custom_access_token_expires_in`, `context` has
  # the properties:
  #
  # `client` - the OAuth client application (see Doorkeeper::OAuth::Client)
  # `grant_type` - the grant type of the request (see Doorkeeper::OAuth)
  # `scopes` - the requested scopes (see Doorkeeper::OAuth::Scopes)
  #
  # use_refresh_token

  # Forbids creating/updating applications with arbitrary scopes that are
  # not in configuration, i.e. `default_scopes` or `optional_scopes`.
  # (disabled by default)
  enforce_configured_scopes

  # Provide support for an owner to be assigned to each registered application (disabled by default)
  # Optional parameter confirmation: true (default false) if you want to enforce ownership of
  # a registered application
  # Note: you must also run the rails g doorkeeper:application_owner generator to provide the necessary support
  #
  # enable_application_owner confirmation: false

  # Define access token scopes for your provider
  # For more information go to
  # https://github.com/doorkeeper-gem/doorkeeper/wiki/Using-Scopes
  default_scopes :public
  optional_scopes :openid,
                  :read_profile,
                  :read_signups,
                  :read_events,
                  :read_conventions,
                  :read_organizations,
                  :read_email_routing,
                  :manage_profile,
                  :manage_signups,
                  :manage_events,
                  :manage_conventions,
                  :manage_organizations,
                  :manage_email_routing

  # Change the way client credentials are retrieved from the request object.
  # By default it retrieves first from the `HTTP_AUTHORIZATION` header, then
  # falls back to the `:client_id` and `:client_secret` params from the `params` object.
  # Check out https://github.com/doorkeeper-gem/doorkeeper/wiki/Changing-how-clients-are-authenticated
  # for more information on customization
  #
  # client_credentials :from_basic, :from_params

  # Change the way access token is authenticated from the request object.
  # By default it retrieves first from the `HTTP_AUTHORIZATION` header, then
  # falls back to the `:access_token` or `:bearer_token` params from the `params` object.
  # Check out https://github.com/doorkeeper-gem/doorkeeper/wiki/Changing-how-clients-are-authenticated
  # for more information on customization
  #
  # access_token_methods :from_bearer_authorization, :from_access_token_param, :from_bearer_param

  # Change the native redirect uri for client apps
  # When clients register with the following redirect uri, they won't be redirected to any server and the authorization
  # code will be displayed within the provider
  # The value can be any string. Use nil to disable this feature. When disabled, clients must provide a valid URL
  # (Similar behaviour: https://developers.google.com/accounts/docs/OAuth2InstalledApp#choosingredirecturi)
  #
  # native_redirect_uri 'urn:ietf:wg:oauth:2.0:oob'

  # Forces the usage of the HTTPS protocol in non-native redirect uris (enabled
  # by default in non-development environments). OAuth2 delegates security in
  # communication to the HTTPS protocol so it is wise to keep this enabled.
  #
  # Callable objects such as proc, lambda, block or any object that responds to
  # #call can be used in order to allow conditional checks (to allow non-SSL
  # redirects to localhost for example).
  #
  # force_ssl_in_redirect_uri !Rails.env.development?
  #
  force_ssl_in_redirect_uri { |uri| uri.host != 'localhost' }

  # Specify what redirect URI's you want to block during Application creation.
  # Any redirect URI is whitelisted by default.
  #
  # You can use this option in order to forbid URI's with 'javascript' scheme
  # for example.
  #
  # forbid_redirect_uri { |uri| uri.scheme.to_s.downcase == 'javascript' }

  # Specify what grant flows are enabled in array of Strings. The valid
  # strings and the flows they enable are:
  #
  # "authorization_code" => Authorization Code Grant Flow
  # "implicit"           => Implicit Grant Flow
  # "password"           => Resource Owner Password Credentials Grant Flow
  # "client_credentials" => Client Credentials Grant Flow
  #
  # If not specified, Doorkeeper enables authorization_code and
  # client_credentials.
  #
  # implicit and password grant flows have risks that you should understand
  # before enabling:
  #   http://tools.ietf.org/html/rfc6819#section-4.4.2
  #   http://tools.ietf.org/html/rfc6819#section-4.4.3
  #
  grant_flows %w[authorization_code client_credentials]

  # Hook into the strategies' request & response life-cycle in case your
  # application needs advanced customization or logging:
  #
  # before_successful_strategy_response do |request|
  #   puts "BEFORE HOOK FIRED! #{request}"
  # end
  #
  # after_successful_strategy_response do |request, response|
  #   puts "AFTER HOOK FIRED! #{request}, #{response}"
  # end

  # Hook into Authorization flow in order to implement Single Sign Out
  # or add ny other functionality.
  #
  # before_successful_authorization do |controller|
  #   Rails.logger.info(params.inspect)
  # end
  #
  # after_successful_authorization do |controller|
  #   controller.session[:logout_urls] <<
  #     Doorkeeper::Application
  #       .find_by(controller.request.params.slice(:redirect_uri))
  #       .logout_uri
  # end

  # Under some circumstances you might want to have applications auto-approved,
  # so that the user skips the authorization step.
  # For example if dealing with a trusted application.
  #
  # skip_authorization do |resource_owner, client|
  #   client.superapp? or resource_owner.admin?
  # end

  # WWW-Authenticate Realm (default "Doorkeeper").
  #
  # realm "Doorkeeper"
end

Doorkeeper::JWT.configure do
  # Set the payload for the JWT token. This should contain unique information
  # about the user. Defaults to a randomly generated token in a hash:
  #     { token: "RANDOM-TOKEN" }
  token_payload do |opts|
    user = User.find(opts[:resource_owner_id])

    {
      iat: Time.current.utc.to_i,
      exp: (Time.current + opts[:expires_in].seconds).utc.to_i,
      # @see JWT reserved claims - https://tools.ietf.org/html/draft-jones-json-web-token-07#page-7
      jti: SecureRandom.uuid,
      user: {
        id: user.id,
        email: user.email,
        name: user.name,
        first_name: user.first_name,
        last_name: user.last_name
      }
    }
  end

  # Optionally set additional headers for the JWT. See
  # https://tools.ietf.org/html/rfc7515#section-4.1
  token_headers { |_opts| { kid: Doorkeeper::OpenidConnect.signing_key[:kid] } }

  # Use the application secret specified in the access grant token. Defaults to
  # `false`. If you specify `use_application_secret true`, both `secret_key` and
  # `secret_key_path` will be ignored.
  use_application_secret false

  # Set the encryption secret. This would be shared with any other applications
  # that should be able to read the payload of the token. Defaults to "secret".
  secret_key ENV.fetch('OPENID_CONNECT_SIGNING_KEY', nil)&.gsub('\n', "\n")

  # Specify encryption type (https://github.com/progrium/ruby-jwt). Defaults to
  # `nil`.
  encryption_method :RS256
end

Ruby version: 3.1.2

Gemfile.lock:

Gemfile.lock content

``` GIT remote: https://github.com/dabit/annotate_models.git revision: f38741d005052045e4d4c2c26b009d3ade0864f3 branch: rails-7 specs: annotate (3.1.1) activerecord (>= 3.2, < 7.1) rake (>= 10.4, < 14.0) GIT remote: https://github.com/nbudin/cloudwatch_scheduler.git revision: 3c4a06c3bf6596663f45d05f524ddddafc09e542 branch: ruby3_rails7_compat specs: cloudwatch_scheduler (1.1.1) aws-sdk-cloudwatchevents (~> 1.13) aws-sdk-sqs (~> 1.10) rails (>= 5.2.0) shoryuken (>= 2.0) GIT remote: https://github.com/neinteractiveliterature/civil_service.git revision: bba59c4a91c3ca61386cfca2f723031ba097da93 branch: no-double-validate specs: civil_service (2.3.0) activemodel (>= 3.0.0) GEM remote: https://rubygems.org/ specs: actioncable (7.0.4) actionpack (= 7.0.4) activesupport (= 7.0.4) nio4r (~> 2.0) websocket-driver (>= 0.6.1) actionmailbox (7.0.4) actionpack (= 7.0.4) activejob (= 7.0.4) activerecord (= 7.0.4) activestorage (= 7.0.4) activesupport (= 7.0.4) mail (>= 2.7.1) net-imap net-pop net-smtp actionmailer (7.0.4) actionpack (= 7.0.4) actionview (= 7.0.4) activejob (= 7.0.4) activesupport (= 7.0.4) mail (~> 2.5, >= 2.5.4) net-imap net-pop net-smtp rails-dom-testing (~> 2.0) actionpack (7.0.4) actionview (= 7.0.4) activesupport (= 7.0.4) rack (~> 2.0, >= 2.2.0) rack-test (>= 0.6.3) rails-dom-testing (~> 2.0) rails-html-sanitizer (~> 1.0, >= 1.2.0) actiontext (7.0.4) actionpack (= 7.0.4) activerecord (= 7.0.4) activestorage (= 7.0.4) activesupport (= 7.0.4) globalid (>= 0.6.0) nokogiri (>= 1.8.5) actionview (7.0.4) activesupport (= 7.0.4) builder (~> 3.1) erubi (~> 1.4) rails-dom-testing (~> 2.0) rails-html-sanitizer (~> 1.1, >= 1.2.0) active_storage_svg_sanitizer (0.1.0) rails (>= 5.2) activejob (7.0.4) activesupport (= 7.0.4) globalid (>= 0.3.6) activemodel (7.0.4) activesupport (= 7.0.4) activerecord (7.0.4) activemodel (= 7.0.4) activesupport (= 7.0.4) activerecord-session_store (2.0.0) actionpack (>= 5.2.4.1) activerecord (>= 5.2.4.1) multi_json (~> 1.11, >= 1.11.2) rack (>= 2.0.8, < 3) railties (>= 5.2.4.1) activestorage (7.0.4) actionpack (= 7.0.4) activejob (= 7.0.4) activerecord (= 7.0.4) activesupport (= 7.0.4) marcel (~> 1.0) mini_mime (>= 1.1.0) activesupport (7.0.4) concurrent-ruby (~> 1.0, >= 1.0.2) i18n (>= 1.6, < 2) minitest (>= 5.1) tzinfo (~> 2.0) acts_as_list (0.9.16) activerecord (>= 3.0) addressable (2.8.0) public_suffix (>= 2.0.2, < 5.0) aes_key_wrap (1.1.0) ahoy_matey (3.2.0) activesupport (>= 5) device_detector geocoder (>= 1.4.5) safely_block (>= 0.2.1) ansi (1.5.0) apollo_upload_server (2.1.4) actionpack (>= 6.1.6) graphql (>= 1.8) ast (2.4.2) aws-eventstream (1.2.0) aws-partitions (1.568.0) aws-record (2.6.0) aws-sdk-dynamodb (~> 1.18) aws-sdk-cloudwatchevents (1.57.0) aws-sdk-core (~> 3, >= 3.127.0) aws-sigv4 (~> 1.1) aws-sdk-cloudwatchlogs (1.45.0) aws-sdk-core (~> 3, >= 3.120.0) aws-sigv4 (~> 1.1) aws-sdk-core (3.130.0) aws-eventstream (~> 1, >= 1.0.2) aws-partitions (~> 1, >= 1.525.0) aws-sigv4 (~> 1.1) jmespath (~> 1.0) aws-sdk-dynamodb (1.60.0) aws-sdk-core (~> 3, >= 3.112.0) aws-sigv4 (~> 1.1) aws-sdk-kms (1.48.0) aws-sdk-core (~> 3, >= 3.120.0) aws-sigv4 (~> 1.1) aws-sdk-rails (3.6.1) aws-record (~> 2) aws-sdk-ses (~> 1) aws-sdk-sqs (~> 1) aws-sessionstore-dynamodb (~> 2) concurrent-ruby (~> 1) railties (>= 5.2.0) aws-sdk-s3 (1.102.0) aws-sdk-core (~> 3, >= 3.120.0) aws-sdk-kms (~> 1) aws-sigv4 (~> 1.4) aws-sdk-ses (1.38.0) aws-sdk-core (~> 3, >= 3.112.0) aws-sigv4 (~> 1.1) aws-sdk-sns (1.45.0) aws-sdk-core (~> 3, >= 3.120.0) aws-sigv4 (~> 1.1) aws-sdk-sqs (1.51.0) aws-sdk-core (~> 3, >= 3.127.0) aws-sigv4 (~> 1.1) aws-sessionstore-dynamodb (2.0.1) aws-sdk-dynamodb (~> 1) rack (~> 2) aws-sigv4 (1.4.0) aws-eventstream (~> 1, >= 1.0.2) backport (1.2.0) bcrypt (3.1.16) bcrypt (3.1.16-java) benchmark (0.2.0) benchmark-ips (2.10.0) bindata (2.4.11) bootsnap (1.9.3) msgpack (~> 1.0) browser (5.3.1) builder (3.2.4) cadmus (0.7.1) liquid rails (>= 5.0.0) cadmus_navbar (0.1.0) acts_as_list cadmus rails (>= 5.0.0) cloudwatchlogger (0.3.0) aws-sdk-cloudwatchlogs (~> 1) multi_json (~> 1) uuid (~> 2) coderay (1.1.3) concurrent-ruby (1.1.10) crass (1.0.6) dalli (2.7.11) dante (0.2.0) dead_end (1.1.7) debug (1.1.0) irb reline (>= 0.2.7) derailed_benchmarks (2.1.1) benchmark-ips (~> 2) dead_end get_process_mem (~> 0) heapy (~> 0) memory_profiler (>= 0, < 2) mini_histogram (>= 0.3.0) rack (>= 1) rack-test rake (> 10, < 14) ruby-statistics (>= 2.1) thor (>= 0.19, < 2) device_detector (1.0.5) devise (4.8.1) bcrypt (~> 3.0) orm_adapter (~> 0.1) railties (>= 4.1.0) responders warden (~> 1.2.3) devise-doorkeeper (1.2.0) devise doorkeeper rails devise-encryptable (0.2.0) devise (>= 2.1.0) diff-lcs (1.5.0) digest (3.1.0) digest (3.1.0-java) docile (1.3.4) domain_prefix (0.4.20210906) json simpleidn (>= 0.0.5) doorkeeper (5.6.0) railties (>= 5) doorkeeper-jwt (0.4.0) jwt (~> 2.1) doorkeeper-openid_connect (1.8.2) doorkeeper (>= 5.5, < 5.7) json-jwt (>= 1.11.0) dotenv (2.7.6) dotenv-rails (2.7.6) dotenv (= 2.7.6) railties (>= 3.2) e2mmap (0.1.0) errbase (0.2.1) erubi (1.11.0) eu_central_bank (1.6.1) money (~> 6.13, >= 6.13.6) nokogiri (~> 1.9) factory_bot (6.2.0) activesupport (>= 5.0.0) factory_bot_rails (6.2.0) factory_bot (~> 6.2.0) railties (>= 5.0.0) faker (2.19.0) i18n (>= 1.6, < 2) faraday (2.5.2) faraday-net_http (>= 2.0, < 3.1) ruby2_keywords (>= 0.0.4) faraday-net_http (3.0.0) ffi (1.15.5) ffi (1.15.5-java) flamegraph (0.9.5) geocoder (1.6.5) get_process_mem (0.2.7) ffi (~> 1.0) globalid (1.0.0) activesupport (>= 5.0) graphql (2.0.14) graphql-batch (0.5.1) graphql (>= 1.10, < 3) promise.rb (~> 0.7.2) graphql-rails_logger (1.2.3) actionpack (> 5.0) activesupport (> 5.0) railties (> 5.0) rouge (~> 3.0) haml (5.2.2) temple (>= 0.8.0) tilt heapy (0.2.0) thor httpclient (2.8.3) i18n (1.12.0) concurrent-ruby (~> 1.0) icalendar (2.7.1) ice_cube (~> 0.16) ice_cube (0.16.3) image_processing (1.12.2) mini_magick (>= 4.9.5, < 5) ruby-vips (>= 2.0.17, < 3) io-console (0.5.11) io-console (0.5.11-java) irb (1.3.7) reline (>= 0.2.7) jaro_winkler (1.5.4) jaro_winkler (1.5.4-java) jmespath (1.6.1) json (2.6.2) json (2.6.2-java) json-jwt (1.15.3) activesupport (>= 4.2) aes_key_wrap bindata httpclient jwt (2.5.0) kramdown (2.4.0) rexml kramdown-parser-gfm (1.1.0) kramdown (~> 2.0) launchy (2.4.3) addressable (~> 2.3) launchy (2.4.3-java) addressable (~> 2.3) spoon (~> 0.0.1) letter_opener (1.7.0) launchy (~> 2.2) liquid (5.1.0) listen (3.7.0) rb-fsevent (~> 0.10, >= 0.10.3) rb-inotify (~> 0.9, >= 0.9.10) lograge (0.11.2) actionpack (>= 4) activesupport (>= 4) railties (>= 4) request_store (~> 1.0) loofah (2.19.0) crass (~> 1.0.2) nokogiri (>= 1.5.9) macaddr (1.7.2) systemu (~> 2.6.5) mail (2.7.1) mini_mime (>= 0.1.1) marcel (1.0.2) memory_profiler (1.0.0) method_source (1.0.0) mini_histogram (0.3.1) mini_magick (4.11.0) mini_mime (1.1.2) mini_portile2 (2.8.0) minitest (5.16.3) minitest-focus (1.3.1) minitest (>= 4, < 6) minitest-reporters (1.5.0) ansi builder minitest (>= 5.0) ruby-progressbar minitest-spec-rails (6.0.4) minitest (>= 5.0) railties (>= 4.1) monetize (1.9.4) money (~> 6.12) money (6.13.8) i18n (>= 0.6.4, <= 2) money-rails (1.14.0) activesupport (>= 3.0) monetize (~> 1.9.0) money (~> 6.13.2) railties (>= 3.0) msgpack (1.4.2) msgpack (1.4.2-java) multi_json (1.15.0) mysql2 (0.5.3) net-imap (0.2.3) digest net-protocol strscan net-pop (0.1.1) digest net-protocol timeout net-protocol (0.1.3) timeout net-smtp (0.3.1) digest net-protocol timeout nio4r (2.5.8) nio4r (2.5.8-java) nokogiri (1.13.8) mini_portile2 (~> 2.8.0) racc (~> 1.4) nokogiri (1.13.8-java) racc (~> 1.4) oj (3.13.4) orm_adapter (0.5.0) ox (2.14.4) parallel (1.22.1) parser (3.1.2.1) ast (~> 2.4.1) pg (1.2.3) pg_search (2.3.5) activerecord (>= 5.2) activesupport (>= 5.2) phonelib (0.6.53) prettier (3.2.2) syntax_tree (>= 2.7.1) syntax_tree-haml (>= 1.1.0) syntax_tree-rbs (>= 0.2.0) prettier_print (0.1.0) promise.rb (0.7.4) pry (0.14.1) coderay (~> 1.1) method_source (~> 1.0) pry (0.14.1-java) coderay (~> 1.1) method_source (~> 1.0) spoon (~> 0.0) pry-rails (0.3.9) pry (>= 0.10.4) pry-remote (0.1.8) pry (~> 0.9) slop (~> 3.0) public_suffix (4.0.6) puma (5.6.4) nio4r (~> 2.0) puma (5.6.4-java) nio4r (~> 2.0) pundit (2.1.1) activesupport (>= 3.0.0) racc (1.6.0) racc (1.6.0-java) rack (2.2.4) rack-mini-profiler (2.3.3) rack (>= 1.2.0) rack-test (2.0.2) rack (>= 1.3) rails (7.0.4) actioncable (= 7.0.4) actionmailbox (= 7.0.4) actionmailer (= 7.0.4) actionpack (= 7.0.4) actiontext (= 7.0.4) actionview (= 7.0.4) activejob (= 7.0.4) activemodel (= 7.0.4) activerecord (= 7.0.4) activestorage (= 7.0.4) activesupport (= 7.0.4) bundler (>= 1.15.0) railties (= 7.0.4) rails-controller-testing (1.0.5) actionpack (>= 5.0.1.rc1) actionview (>= 5.0.1.rc1) activesupport (>= 5.0.1.rc1) rails-dom-testing (2.0.3) activesupport (>= 4.2.0) nokogiri (>= 1.6) rails-html-sanitizer (1.4.3) loofah (~> 2.3) railties (7.0.4) actionpack (= 7.0.4) activesupport (= 7.0.4) method_source rake (>= 12.2) thor (~> 1.0) zeitwerk (~> 2.5) rainbow (3.1.1) rake (13.0.6) rb-fsevent (0.11.0) rb-inotify (0.10.1) ffi (~> 1.0) rbs (2.0.0) rbtree (0.4.4) recaptcha (5.8.1) json redcarpet (3.5.1) regexp_parser (2.5.0) reline (0.2.7) io-console (~> 0.5) request_store (1.5.1) rack (>= 1.4) responders (3.0.1) actionpack (>= 5.0) railties (>= 5.0) reverse_markdown (2.1.1) nokogiri rexml (3.2.5) rollbar (3.2.0) rollbar-shoryuken (1.1.0) aws-sdk-sqs (~> 1.20, <= 2.0) ox (~> 2.14) rollbar (>= 2.21, <= 4.0, < 4.0) shoryuken (>= 4.0, <= 5.3) rouge (3.26.0) rubocop (1.36.0) json (~> 2.3) parallel (~> 1.10) parser (>= 3.1.2.1) rainbow (>= 2.2.2, < 4.0) regexp_parser (>= 1.8, < 3.0) rexml (>= 3.2.5, < 4.0) rubocop-ast (>= 1.20.1, < 2.0) ruby-progressbar (~> 1.7) unicode-display_width (>= 1.4.0, < 3.0) rubocop-ast (1.21.0) parser (>= 3.1.1.0) rubocop-performance (1.11.5) rubocop (>= 1.7.0, < 2.0) rubocop-ast (>= 0.4.0) rubocop-rails (2.11.3) activesupport (>= 4.2.0) rack (>= 1.1) rubocop (>= 1.7.0, < 2.0) rubocop-sequel (0.3.3) rubocop (~> 1.0) ruby-progressbar (1.11.0) ruby-statistics (3.0.0) ruby-vips (2.1.4) ffi (~> 1.12) ruby2_keywords (0.0.5) safely_block (0.3.0) errbase (>= 0.1.1) scheduled_value (1.4.0) sorted_set (~> 1.0) sequel (5.48.0) set (1.0.2) shoryuken (5.3.0) aws-sdk-core (>= 2) concurrent-ruby thor simplecov (0.21.2) docile (~> 1.1) simplecov-html (~> 0.11) simplecov_json_formatter (~> 0.1) simplecov-html (0.12.3) simplecov_json_formatter (0.1.2) simpleidn (0.2.1) unf (~> 0.1.4) skylight (5.3.2) activesupport (>= 5.2.0) slop (3.6.0) solargraph (0.46.0) backport (~> 1.2) benchmark bundler (>= 1.17.2) diff-lcs (~> 1.4) e2mmap jaro_winkler (~> 1.5) kramdown (~> 2.3) kramdown-parser-gfm (~> 1.1) parser (~> 3.0) reverse_markdown (>= 1.0.5, < 3) rubocop (>= 0.52) thor (~> 1.0) tilt (~> 2.0) yard (~> 0.9, >= 0.9.24) solargraph-rails (0.3.1) activesupport solargraph (>= 0.41.1) sorted_set (1.0.3) rbtree set (~> 1.0) sorted_set (1.0.3-java) spoon (0.0.6) ffi sprockets (4.0.2) concurrent-ruby (~> 1.0) rack (> 1, < 3) sprockets-rails (3.4.2) actionpack (>= 5.2) activesupport (>= 5.2) sprockets (>= 3.0.0) stackprof (0.2.17) stripe (5.38.0) stripe-ruby-mock (3.1.0.rc3) dante (>= 0.2.0) multi_json (~> 1.0) stripe (> 5, < 6) strscan (3.0.4) strscan (3.0.4-java) syntax_tree (3.3.0) prettier_print syntax_tree-haml (1.3.1) haml (>= 5.2) prettier_print syntax_tree (>= 2.0.1) syntax_tree-rbs (0.5.0) prettier_print rbs syntax_tree (>= 2.0.1) systemu (2.6.5) temple (0.8.2) term-ansicolor (1.7.1) tins (~> 1.0) thor (1.2.1) tilt (2.0.11) timeout (0.3.0) tins (1.20.2) twilio-ruby (5.72.0) faraday (>= 0.9, < 3.0) jwt (>= 1.5, <= 2.5) nokogiri (>= 1.6, < 2.0) typeprof (0.21.2) rbs (>= 1.8.1) tzinfo (2.0.5) concurrent-ruby (~> 1.0) tzinfo-data (1.2021.1) tzinfo (>= 1.0.0) unf (0.1.4) unf_ext unf (0.1.4-java) unf_ext (0.0.8) unicode-display_width (2.3.0) uuid (2.3.9) macaddr (~> 1.0) warden (1.2.9) rack (>= 2.0.9) webrick (1.7.0) websocket-driver (0.7.5) websocket-extensions (>= 0.1.0) websocket-driver (0.7.5-java) websocket-extensions (>= 0.1.0) websocket-extensions (0.1.5) will_paginate (3.3.1) with_advisory_lock (4.6.0) activerecord (>= 4.2) yard (0.9.28) webrick (~> 1.7.0) zeitwerk (2.6.0) PLATFORMS java ruby DEPENDENCIES active_storage_svg_sanitizer activerecord-session_store ahoy_matey annotate! apollo_upload_server (= 2.1.4) aws-sdk-rails aws-sdk-s3 aws-sdk-sns aws-sdk-sqs bootsnap (>= 1.1.0) browser cadmus (~> 0.7.1) cadmus_navbar (~> 0.1.0) civil_service! cloudwatch_scheduler! cloudwatchlogger dalli dead_end debug derailed_benchmarks devise devise-doorkeeper devise-encryptable domain_prefix doorkeeper (= 5.6.0) doorkeeper-jwt doorkeeper-openid_connect dotenv-rails eu_central_bank factory_bot factory_bot_rails faker flamegraph graphql (>= 2.0) graphql-batch graphql-rails_logger icalendar image_processing (~> 1.2) letter_opener liquid listen lograge memory_profiler minitest-focus minitest-reporters minitest-spec-rails money-rails mysql2 (~> 0.5.3) oj (~> 3.13.4) parallel pg pg_search phonelib prettier (= 3.2.2) prettier_print pry-rails pry-remote puma pundit rack-mini-profiler rails (= 7.0.4) rails-controller-testing recaptcha redcarpet reverse_markdown rollbar rollbar-shoryuken rubocop (= 1.36.0) rubocop-performance rubocop-rails rubocop-sequel ruby-vips scheduled_value (~> 1.4.0) sequel shoryuken simplecov skylight solargraph solargraph-rails (= 0.3.1) sprockets-rails stackprof stripe stripe-ruby-mock (>= 3.1.0.rc3) syntax_tree syntax_tree-haml syntax_tree-rbs term-ansicolor twilio-ruby (~> 5.72.0) typeprof tzinfo-data webrick will_paginate with_advisory_lock yard RUBY VERSION ruby 3.1.2p20 BUNDLED WITH 2.3.4 ```

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

rgammans commented 1 year ago

I'm was expected a core dev to chime about the potentially security related RFC deviation?

nbulaj commented 1 year ago

Hi @nbudin and @rgammans . I haven't checked RC yet, but from your description it sounds correct and requires a fix. Will check deeper a little bit later. Any PRs are welcome in case you wanna to fix the issue :bow:

hickford commented 1 year ago

Fixed in https://github.com/doorkeeper-gem/doorkeeper/releases/tag/v5.6.6

@nbulaj is there a CVE for this issue? It seems an example of "CWE-287: Improper Authentication" https://cwe.mitre.org/data/definitions/287.html

When an actor claims to have a given identity, the product does not prove or insufficiently proves that the claim is correct.

In this case, the actor is an OAuth client claiming the identity of some other public OAuth client

nbulaj commented 1 year ago

No we don't have it @hickford

hickford commented 1 year ago

@nbulaj what do you think about opening a CVE?

nbulaj commented 1 year ago

@hickford could you please do it? Never did it before, not sure where and what to do

hickford commented 1 year ago

@nbulaj I'm not sure either. Admins should be able to record security vulnerabilities at https://github.com/doorkeeper-gem/doorkeeper/security

Anyone with admin permissions to a repository can create a security advisory.

It looks like @stefansundin has done it before. Stefan, can you help?

Previous vulnerabilities https://osv.dev/list?ecosystem=RubyGems&q=doorkeeper

stefansundin commented 1 year ago

Uhh. I think @nbulaj did in fact create the one I helped with. This documentation page says admin permissions are required, so I couldn't possibly have done it.

Probably someone with admin permissions just have to go to https://github.com/doorkeeper-gem/doorkeeper/security and there's probably a green button to create a new one. 🤷

What I did do was create a proper CVE identifier. However it was a total pain so I don't recommend getting one. I submitted the form that they had but no one got back to me. A few days I got tired of waiting and I emailed someone and they replied back with a super short message ("Use CVE-2020-10187.").. so not the best experience lol. Just create a GitHub advisory and be satisfied with that. :)

hickford commented 1 year ago

@stefansundin thanks for your reply. According to https://github.blog/2022-04-22-removing-the-stigma-of-a-cve/ these days it's easier to assign a CVE to a GHSA.

hickford commented 1 year ago

Admin can open a GHSA at https://github.com/doorkeeper-gem/doorkeeper/security/advisories/new

hickford commented 1 year ago

@nbudin right now only admin can draft a GHSA at https://github.com/doorkeeper-gem/doorkeeper/security/advisories/new but if you enable "Private vulnerability reporting" at https://github.com/doorkeeper-gem/doorkeeper/security then non-admins should be able to draft a GHSA for admins to review

nbulaj commented 1 year ago

Enabled @hickford , thanks :bow:

hickford commented 1 year ago

@nbulaj thanks. Ready to publish at https://github.com/doorkeeper-gem/doorkeeper/security/advisories/GHSA-7w2c-w47h-789w

Maintainers and owners are able to request a CVE. Once requested, GitHub will review this advisory in order to assign a CVE

hickford commented 1 year ago

Published

doorkeeper-gem / doorkeeper