Close #1036: forbid redirect URI's

Allow to forbid Application redirect URI's with specific rules
during creation. This allows to add custom checks for redirect URI
validator.

NEWS.md

@@ -4,6 +4,7 @@ User-visible changes worth mentioning.
 
 ## master
 
+- [#1036] Allow to forbid Application redirect URI's with specific rules. 
 - [#1029] Deprecate `order_method` and introduce `ordered_by`. Sort applications
   by `created_at` in index action.
 - [#1033] Allow Doorkeeper configuration option #force_ssl_in_redirect_uri to be a callable object.

app/validators/redirect_uri_validator.rb

@@ -11,7 +11,8 @@ def validate_each(record, attribute, value)
     else
       value.split.each do |val|
         uri = ::URI.parse(val)
-        return false if native_redirect_uri?(uri)
+        break if native_redirect_uri?(uri)
+        record.errors.add(attribute, :forbidden_uri) if forbidden_uri?(uri)
         record.errors.add(attribute, :fragment_present) unless uri.fragment.nil?
         record.errors.add(attribute, :relative_uri) if uri.scheme.nil? || uri.host.nil?
         record.errors.add(attribute, :secured_uri) if invalid_ssl_uri?(uri)
@@ -27,6 +28,10 @@ def native_redirect_uri?(uri)
     self.class.native_redirect_uri.present? && uri.to_s == self.class.native_redirect_uri.to_s
   end
 
+  def forbidden_uri?(uri)
+    Doorkeeper.configuration.forbid_redirect_uri.call(uri)
+  end
+
   def invalid_ssl_uri?(uri)
     forces_ssl = Doorkeeper.configuration.force_ssl_in_redirect_uri
 

config/locales/en.yml

@@ -13,6 +13,7 @@ en:
               invalid_uri: 'must be a valid URI.'
               relative_uri: 'must be an absolute URI.'
               secured_uri: 'must be an HTTPS/SSL URI.'
+              forbidden_uri: 'is forbidden by the server.'
 
   doorkeeper:
     applications:

lib/doorkeeper/config.rb

@@ -59,28 +59,28 @@ def build
       # @option opts[Boolean] :confirmation (false)
       #   Set confirm_application_owner variable
       def enable_application_owner(opts = {})
-        @config.instance_variable_set('@enable_application_owner', true)
+        @config.instance_variable_set(:@enable_application_owner, true)
         confirm_application_owner if opts[:confirmation].present? && opts[:confirmation]
       end
 
       def confirm_application_owner
-        @config.instance_variable_set('@confirm_application_owner', true)
+        @config.instance_variable_set(:@confirm_application_owner, true)
       end
 
       # Define default access token scopes for your provider
       #
       # @param scopes [Array] Default set of access (OAuth::Scopes.new)
       # token scopes
       def default_scopes(*scopes)
-        @config.instance_variable_set('@default_scopes', OAuth::Scopes.from_array(scopes))
+        @config.instance_variable_set(:@default_scopes, OAuth::Scopes.from_array(scopes))
       end
 
       # Define default access token scopes for your provider
       #
       # @param scopes [Array] Optional set of access (OAuth::Scopes.new)
       # token scopes
       def optional_scopes(*scopes)
-        @config.instance_variable_set('@optional_scopes', OAuth::Scopes.from_array(scopes))
+        @config.instance_variable_set(:@optional_scopes, OAuth::Scopes.from_array(scopes))
       end
 
       # Change the way client credentials are retrieved from the request object.
@@ -90,7 +90,7 @@ def optional_scopes(*scopes)
       #
       # @param methods [Array] Define client credentials
       def client_credentials(*methods)
-        @config.instance_variable_set('@client_credentials', methods)
+        @config.instance_variable_set(:@client_credentials, methods)
       end
 
       # Change the way access token is authenticated from the request object.
@@ -100,19 +100,19 @@ def client_credentials(*methods)
       #
       # @param methods [Array] Define access token methods
       def access_token_methods(*methods)
-        @config.instance_variable_set('@access_token_methods', methods)
+        @config.instance_variable_set(:@access_token_methods, methods)
       end
 
       # Issue access tokens with refresh token (disabled by default)
       def use_refresh_token
-        @config.instance_variable_set('@refresh_token_enabled', true)
+        @config.instance_variable_set(:@refresh_token_enabled, true)
       end
 
       # Reuse access token for the same resource owner within an application
       # (disabled by default)
       # Rationale: https://github.com/doorkeeper-gem/doorkeeper/issues/383
       def reuse_access_token
-        @config.instance_variable_set("@reuse_access_token", true)
+        @config.instance_variable_set(:@reuse_access_token, true)
       end
     end
 
@@ -202,6 +202,13 @@ def option(name, options = {})
     option :active_record_options,          default: {}
     option :grant_flows,                    default: %w[authorization_code client_credentials]
 
+    # Allows to forbid specific Application redirect URI's by custom rules.
+    # Doesn't forbid any URI by default.
+    #
+    # @param forbid_redirect_uri [Proc] Block or any object respond to #call
+    #
+    option :forbid_redirect_uri,            default: ->(_uri) { false }
+
     # WWW-Authenticate Realm (default "Doorkeeper").
     #
     # @param realm [String] ("Doorkeeper") Authentication realm

lib/generators/doorkeeper/templates/initializer.rb

@@ -90,6 +90,14 @@
   #
   # force_ssl_in_redirect_uri { |uri| uri.host != 'localhost' }
 
+  # Specify what redirect URI's you want to block during 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:
   #

spec/lib/config_spec.rb

@@ -218,6 +218,23 @@
     end
   end
 
+  describe 'forbid_redirect_uri' do
+    it 'is false by default' do
+      expect(subject.forbid_redirect_uri.call(URI.parse('https://localhost'))).to be_falsey
+    end
+
+    it 'can be a callable object' do
+      block = proc { true }
+      Doorkeeper.configure do
+        orm DOORKEEPER_ORM
+        forbid_redirect_uri(&block)
+      end
+
+      expect(subject.forbid_redirect_uri).to eq(block)
+      expect(subject.forbid_redirect_uri.call).to be_truthy
+    end
+  end
+
   describe 'enable_application_owner' do
     it 'is disabled by default' do
       expect(Doorkeeper.configuration.enable_application_owner?).not_to be_truthy

spec/validators/redirect_uri_validator_spec.rb

@@ -91,6 +91,21 @@
       expect(application).not_to be_valid
     end
 
+    it 'forbids redirect uri if required' do
+      subject.redirect_uri = 'javascript://document.cookie'
+
+      Doorkeeper.configure do
+        orm DOORKEEPER_ORM
+        forbid_redirect_uri { |uri| uri.scheme == 'javascript' }
+      end
+
+      expect(subject).to be_invalid
+      expect(subject.errors[:redirect_uri].first).to eq('is forbidden by the server.')
+
+      subject.redirect_uri = 'https://localhost/callback'
+      expect(subject).to be_valid
+    end
+
     it 'invalidates the uri when the uri does not use a secure protocol' do
       subject.redirect_uri = 'http://example.com/callback'
       expect(subject).not_to be_valid