search

doorkeeper-gem / doorkeeper

Doorkeeper is an OAuth 2 provider for Ruby on Rails / Grape.
https://doorkeeper.gitbook.io/guides/
MIT License
5.32k stars 1.07k forks source link

Refreshing a token sending scopes separated by `+` does not work #1686

Open danilokleber opened 7 months ago

danilokleber commented 7 months ago

Steps to reproduce

Try to refresh a token sending the scope field with strings separated by +. The users of the API just tried to use the same format they used on issuing a token (authorization_code grant).

Expected behavior

It refreshes the access token like when sending scopes separated by space.

Actual behavior

It returns a 401 with:

{
  "error": "invalid_scope",
  "error_description": "O escopo requisitado é inválido, desconhecido ou malformado."
}

System configuration

Doorkeeper initializer:

# config/initializers/doorkeeper.rb
Doorkeeper.configure do
  optional_scopes SCOPES

  native_redirect_uri URIS 

  use_refresh_token

  grant_flows %w(authorization_code)

  skip_authorization do |_, client|
    client.scopes.include?("mobile")
  end
end

# just injects some stuff in the response
Doorkeeper::OAuth::TokenResponse.send :prepend, Doorkeeper::CustomTokenResponse

Ruby version: 2.6.10

Gemfile.lock:

Gemfile.lock content ``` GEM remote: http://rubygems.org/ specs: actioncable (5.2.8.1) actionpack (= 5.2.8.1) nio4r (~> 2.0) websocket-driver (>= 0.6.1) actionmailer (5.2.8.1) actionpack (= 5.2.8.1) actionview (= 5.2.8.1) activejob (= 5.2.8.1) mail (~> 2.5, >= 2.5.4) rails-dom-testing (~> 2.0) actionpack (5.2.8.1) actionview (= 5.2.8.1) activesupport (= 5.2.8.1) rack (~> 2.0, >= 2.0.8) rack-test (>= 0.6.3) rails-dom-testing (~> 2.0) rails-html-sanitizer (~> 1.0, >= 1.0.2) actionview (5.2.8.1) activesupport (= 5.2.8.1) builder (~> 3.1) erubi (~> 1.4) rails-dom-testing (~> 2.0) rails-html-sanitizer (~> 1.0, >= 1.0.3) active_model_serializers (0.9.8) activemodel (>= 3.2) concurrent-ruby (~> 1.0) activejob (5.2.8.1) activesupport (= 5.2.8.1) globalid (>= 0.3.6) activemodel (5.2.8.1) activesupport (= 5.2.8.1) activerecord (5.2.8.1) activemodel (= 5.2.8.1) activesupport (= 5.2.8.1) arel (>= 9.0) activerecord-session_store (1.1.3) actionpack (>= 4.0) activerecord (>= 4.0) multi_json (~> 1.11, >= 1.11.2) rack (>= 1.5.2, < 3) railties (>= 4.0) activestorage (5.2.8.1) actionpack (= 5.2.8.1) activerecord (= 5.2.8.1) marcel (~> 1.0.0) activesupport (5.2.8.1) concurrent-ruby (~> 1.0, >= 1.0.2) i18n (>= 0.7, < 2) minitest (~> 5.1) tzinfo (~> 1.1) addressable (2.8.1) public_suffix (>= 2.0.2, < 6.0) appsignal (3.2.1) rack arel (9.0.0) audited (5.0.2) activerecord (>= 5.0, < 7.1) autoprefixer-rails (8.6.5) execjs aws-eventstream (1.1.1) aws-partitions (1.681.0) aws-sdk-core (3.185.1) aws-eventstream (~> 1, >= 1.0.2) aws-partitions (~> 1, >= 1.651.0) aws-sigv4 (~> 1.5) jmespath (~> 1, >= 1.6.1) aws-sdk-kms (1.58.0) aws-sdk-core (~> 3, >= 3.127.0) aws-sigv4 (~> 1.1) aws-sdk-lambda (1.106.0) aws-sdk-core (~> 3, >= 3.184.0) aws-sigv4 (~> 1.1) aws-sdk-quicksight (1.94.0) aws-sdk-core (~> 3, >= 3.184.0) aws-sigv4 (~> 1.1) aws-sdk-s3 (1.136.0) aws-sdk-core (~> 3, >= 3.181.0) aws-sdk-kms (~> 1) aws-sigv4 (~> 1.6) aws-sdk-sqs (1.64.0) aws-sdk-core (~> 3, >= 3.184.0) aws-sigv4 (~> 1.1) aws-sigv4 (1.6.1) aws-eventstream (~> 1, >= 1.0.2) bcrypt (3.1.18) better_errors (2.9.1) coderay (>= 1.0.0) erubi (>= 1.0.0) rack (>= 0.9.0) bourbon (3.2.4) sass (~> 3.2) thor breadcrumbs (0.2.0) actionview activesupport i18n browser (5.3.1) builder (3.2.4) byebug (11.1.3) cancancan (2.3.0) carrierwave (2.2.3) activemodel (>= 5.0.0) activesupport (>= 5.0.0) addressable (~> 2.6) image_processing (~> 1.1) marcel (~> 1.0.0) mini_mime (>= 0.1.3) ssrf_filter (~> 1.0) cells (4.1.7) declarative-builder (< 0.2.0) declarative-option (< 0.2.0) tilt (>= 1.4, < 3) uber (< 0.2.0) cells-erb (0.1.0) cells (~> 4.0) erbse (>= 0.1.1) cells-rails (0.1.5) actionpack (>= 5.0) cells (>= 4.1.6, < 5.0.0) clockwork (3.0.1) activesupport tzinfo clockwork-test (0.4.0) clockwork timecop coderay (1.1.3) coffee-rails (5.0.0) coffee-script (>= 2.2.0) railties (>= 5.2.0) coffee-script (2.4.1) coffee-script-source execjs coffee-script-source (1.12.2) concurrent-ruby (1.1.10) cpf_cnpj (0.5.0) crack (0.4.5) rexml crass (1.0.6) css_parser (1.12.0) addressable date (3.3.3) declarative-builder (0.1.0) declarative-option (< 0.2.0) declarative-option (0.1.0) devise (4.6.2) bcrypt (~> 3.0) orm_adapter (~> 0.1) railties (>= 4.1.0, < 6.0) responders warden (~> 1.2.3) devise_invitable (1.7.5) actionmailer (>= 4.1.0) devise (>= 4.0.0) diff-lcs (1.5.0) docile (1.4.0) domain_name (0.5.20190701) unf (>= 0.0.5, < 1.0.0) doorkeeper (4.2.6) railties (>= 4.2) dotenv (2.8.1) dotenv-rails (2.8.1) dotenv (= 2.8.1) railties (>= 3.2) erbse (0.1.4) temple erubi (1.11.0) ethon (0.15.0) ffi (>= 1.15.0) excon (0.95.0) execjs (2.8.1) factory_bot (4.11.1) activesupport (>= 3.0.0) faker (2.22.0) i18n (>= 1.8.11, < 2) ffi (1.15.5) flipper (0.26.0) concurrent-ruby (< 2) flipper-active_record (0.26.0) activerecord (>= 4.2, < 8) flipper (~> 0.26.0) flipper-api (0.26.0) flipper (~> 0.26.0) rack (>= 1.4, < 3) flipper-ui (0.26.0) erubi (>= 1.0.0, < 2.0.0) flipper (~> 0.26.0) rack (>= 1.4, < 3) rack-protection (>= 1.5.3, <= 4.0.0) sanitize (< 7) fog-aws (3.15.0) fog-core (~> 2.1) fog-json (~> 1.1) fog-xml (~> 0.1) fog-core (2.3.0) builder excon (~> 0.71) formatador (>= 0.2, < 2.0) mime-types fog-json (1.2.0) fog-core multi_json (~> 1.10) fog-xml (0.1.4) fog-core nokogiri (>= 1.5.11, < 2.0.0) formatador (1.1.0) globalid (1.0.0) activesupport (>= 5.0) handlebars_assets (0.23.2) execjs (~> 2.0) sprockets (>= 2.0.0) tilt (>= 1.2) hashdiff (1.0.1) htmlentities (4.3.4) http-cookie (1.0.4) domain_name (~> 0.5) httpclient (2.8.3) i18n (1.12.0) concurrent-ruby (~> 1.0) image_processing (1.12.2) mini_magick (>= 4.9.5, < 5) ruby-vips (>= 2.0.17, < 3) jbuilder (2.11.5) actionview (>= 5.0.0) activesupport (>= 5.0.0) jmespath (1.6.2) jquery-rails (4.5.1) rails-dom-testing (>= 1, < 3) railties (>= 4.2.0) thor (>= 0.14, < 2.0) jwt (2.6.0) keen (1.1.1) addressable (~> 2.5) multi_json (~> 1.12) loofah (2.19.1) crass (~> 1.0.2) nokogiri (>= 1.5.9) mail (2.8.0) mini_mime (>= 0.1.1) net-imap net-pop net-smtp marcel (1.0.2) method_source (1.0.0) mime-types (3.3.1) mime-types-data (~> 3.2015) mime-types-data (3.2021.0901) mini_magick (4.12.0) mini_mime (1.1.2) mini_portile2 (2.8.1) minitest (5.16.3) monetize (1.12.0) money (~> 6.12) money (6.16.0) i18n (>= 0.6.4, <= 2) multi_json (1.15.0) net-imap (0.3.3) date net-protocol net-pop (0.1.2) net-protocol net-protocol (0.2.1) timeout net-smtp (0.3.3) net-protocol netrc (0.11.0) nio4r (2.5.8) nokogiri (1.13.10) mini_portile2 (~> 2.8.0) racc (~> 1.4) nori (2.6.0) orm_adapter (0.5.0) pagy (6.0.4) paper_trail (13.0.0) activerecord (>= 5.2) request_store (~> 1.1) paper_trail-association_tracking (2.2.1) paper_trail (>= 12.0) pg (1.4.5) pg_search (2.1.7) activerecord (>= 4.2) activesupport (>= 4.2) premailer (1.15.0) addressable css_parser (>= 1.6.0) htmlentities (>= 4.0.0) premailer-rails (1.12.0) actionmailer (>= 3) net-smtp premailer (~> 1.7, >= 1.7.9) pry (0.13.1) coderay (~> 1.1) method_source (~> 1.0) pry-byebug (3.9.0) byebug (~> 11.0) pry (~> 0.13.0) pry-rails (0.3.9) pry (>= 0.10.4) public_suffix (5.0.1) puma (4.3.12) nio4r (~> 2.0) pusher (2.0.3) httpclient (~> 2.8) multi_json (~> 1.15) pusher-signature (~> 0.1.8) pusher-signature (0.1.8) racc (1.6.2) rack (2.2.4) rack-cors (1.1.1) rack (>= 2.0.0) rack-protection (2.2.2) rack rack-test (2.0.2) rack (>= 1.3) rails (5.2.8.1) actioncable (= 5.2.8.1) actionmailer (= 5.2.8.1) actionpack (= 5.2.8.1) actionview (= 5.2.8.1) activejob (= 5.2.8.1) activemodel (= 5.2.8.1) activerecord (= 5.2.8.1) activestorage (= 5.2.8.1) activesupport (= 5.2.8.1) bundler (>= 1.3.0) railties (= 5.2.8.1) sprockets-rails (>= 2.0.0) 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.4) loofah (~> 2.19, >= 2.19.1) railties (5.2.8.1) actionpack (= 5.2.8.1) activesupport (= 5.2.8.1) method_source rake (>= 0.8.7) thor (>= 0.19.0, < 2.0) rake (13.0.6) ransack (2.1.1) actionpack (>= 5.0) activerecord (>= 5.0) activesupport (>= 5.0) i18n request_store (1.5.1) rack (>= 1.4) responders (3.0.1) actionpack (>= 5.0) railties (>= 5.0) rest-client (2.0.2) http-cookie (>= 1.0.2, < 2.0) mime-types (>= 1.16, < 4.0) netrc (~> 0.8) rexml (3.2.5) rmagick (3.2.0) rspec-core (3.12.0) rspec-support (~> 3.12.0) rspec-expectations (3.12.1) diff-lcs (>= 1.2.0, < 2.0) rspec-support (~> 3.12.0) rspec-mocks (3.12.1) diff-lcs (>= 1.2.0, < 2.0) rspec-support (~> 3.12.0) rspec-rails (5.1.2) actionpack (>= 5.2) activesupport (>= 5.2) railties (>= 5.2) rspec-core (~> 3.10) rspec-expectations (~> 3.10) rspec-mocks (~> 3.10) rspec-support (~> 3.10) rspec-support (3.12.0) ruby-vips (2.1.4) ffi (~> 1.12) sanitize (6.0.0) crass (~> 1.0.2) nokogiri (>= 1.12.0) sass (3.2.19) sass-rails (6.0.0) sassc-rails (~> 2.1, >= 2.1.1) sassc (2.4.0) ffi (~> 1.9) sassc-rails (2.1.2) railties (>= 4.0.0) sassc (>= 2.0) sprockets (> 3.0) sprockets-rails tilt shoryuken (5.3.2) aws-sdk-core (>= 2) concurrent-ruby thor shoulda-matchers (4.5.1) activesupport (>= 4.2.0) simple_form (5.1.0) actionpack (>= 5.2) activemodel (>= 5.2) 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.4) smart_assets (0.5.0) rails (>= 4.0, < 7.1) sprockets-rails (>= 2, < 4) sprockets (3.7.2) concurrent-ruby (~> 1.0) rack (> 1, < 3) sprockets-rails (3.4.2) actionpack (>= 5.2) activesupport (>= 5.2) sprockets (>= 3.0.0) ssrf_filter (1.1.1) state_machines (0.5.0) state_machines-activemodel (0.8.0) activemodel (>= 5.1) state_machines (>= 0.5.0) state_machines-activerecord (0.8.0) activerecord (>= 5.1) state_machines-activemodel (>= 0.8.0) temple (0.9.1) thor (1.2.1) thread_safe (0.3.6) tilt (2.0.11) timecop (0.9.6) timeliness (0.4.5) timeout (0.3.1) turbolinks (2.5.4) coffee-rails typhoeus (1.4.0) ethon (>= 0.9.0) tzinfo (1.2.10) thread_safe (~> 0.1) uber (0.1.0) uglifier (4.2.0) execjs (>= 0.3.0, < 3) unf (0.1.4) unf_ext unf_ext (0.0.7.7) validates_timeliness (5.0.1) timeliness (>= 0.3.10, < 1) warden (1.2.9) rack (>= 2.0.9) webmock (3.18.1) addressable (>= 2.8.0) crack (>= 0.3.2) hashdiff (>= 0.4.0, < 2.0.0) websocket-driver (0.7.5) websocket-extensions (>= 0.1.0) websocket-extensions (0.1.5) wicked_pdf (2.6.3) activesupport wkhtmltopdf-binary (0.12.3.1) xml-simple (1.1.9) rexml PLATFORMS ruby DEPENDENCIES active_model_serializers (~> 0.9.0) activerecord-session_store (~> 1.1.3) appsignal audited (~> 5) autoprefixer-rails (~> 8.6) aws-sdk-lambda (~> 1) aws-sdk-quicksight (= 1.94.0) aws-sdk-s3 (~> 1) aws-sdk-sqs (~> 1) better_errors bourbon breadcrumbs browser cancancan (~> 2) carrierwave (~> 2.2.2) cells-erb cells-rails clockwork clockwork-test cpf_cnpj devise (= 4.6.2) devise_invitable (= 1.7.5) doorkeeper (~> 4.2.0) dotenv-rails factory_bot (~> 4) faker flipper flipper-active_record flipper-api flipper-ui fog-aws (~> 3.15.0) handlebars_assets jbuilder jquery-rails (= 4.5.1) jwt (~> 2.6) keen monetize money nori pagy paper_trail (~> 13) paper_trail-association_tracking pg (~> 1) pg_search (~> 2.1.2) premailer-rails pry-byebug pry-rails puma (~> 4) pusher rack-cors rails (= 5.2.8.1) rails-controller-testing ransack (~> 2.1.1) rest-client (= 2.0.2) rmagick (~> 3.2.0) rspec-rails (~> 5) sass-rails (= 6.0.0) shoryuken (~> 5) shoulda-matchers (~> 4) simple_form (~> 5.1) simplecov smart_assets state_machines-activerecord (~> 0.8.0) timecop turbolinks (~> 2.5, >= 2.5.4) typhoeus (= 1.4.0) uglifier validates_timeliness (~> 5.0.1) webmock wicked_pdf wkhtmltopdf-binary xml-simple RUBY VERSION ruby 2.6.10p210 BUNDLED WITH 2.3.26 ```
ransombriggs commented 1 month ago

I am not a maintainer, but I do not think this would be possible to support since + is an allowed character in a scopes according to the rfc. So if a server had scopes foo, bar and foo+bar and someone requested foo+bar in the way you are proposing, should that be interpreted as requesting [foo, bar] or [foo+bar] ?

danilokleber commented 1 month ago

Fair enough. I think the issue here is inconsistency? I don’t have a + interpolated scope available.

luke-zhou commented 3 weeks ago

Fair enough. I think the issue here is inconsistency? I don’t have a + interpolated scope available.

Does that only happen in the authorization_code grant? I have done the following test in password grant:

danilokleber commented 3 weeks ago

Does that only happen in the authorization_code grant?

Interesting question. I can’t test other grants right now but yours is helpful enough.

ransombriggs commented 3 weeks ago

I do not think this is a matter of consistency, when the client does the redirect they are url-encoding spaces to +, if there were a plus in the scopes, they would url-encode them to %2B. When the web server reads the query parameters it converts + to spaces, which is the correct behavior.