CVE-2020-10187: Missing Authorization (CWE-862)
value: Doorkeeper version 5.0.0 and later contains an information disclosure vulnerability that allows an attacker to retrieve the client secret only intended for the OAuth application owner. After authorizing the application and allowing access, the attacker simply needs to request the list of their authorized applications in a JSON format (usually GET /oauth/authorized_applications.json). An application is vulnerable if the authorized applications controller is enabled.
CWE description: The product does not perform an authorization check when an actor attempts to access a resource or perform an action.
CVSS v3 base score: 7.5
Published: 2020-05-04T14:15Z
Repository: https://github.com/doorkeeper-gem/doorkeeper
Language: Ruby
Files changed: CHANGELOG.md, app/controllers/doorkeeper/applications_controller.rb, app/controllers/doorkeeper/authorized_applications_controller.rb, lib/doorkeeper/orm/active_record/mixins/application.rb, spec/models/doorkeeper/application_spec.rb
Commit date: 2020-05-02 15:58:58 +0300
Commit message: Merge pull request from GHSA-j7vx-8mqj-cqp9

* Attempt at fixing information disclosure vulnerability.

* Add `#as_json` method and attrs serialization restriction for Application model

* [ci skip] Add documentation for serialization

Co-authored-by: Nikita Bulai <bulaj.nikita@gmail.com>
Diff: diff --git a/CHANGELOG.md b/CHANGELOG.md
index 9528aa4c..ba8b8ed9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,10 +5,18 @@ upgrade guides.
 
 User-visible changes worth mentioning.
 
 ## master
 
+- [#1371] Add `#as_json` method and attributes serialization restriction for Application model.
+  Fixes information disclosure vulnerability (CVE-2020-10187).
+  
+  **[IMPORTANT]** you need to re-implement `#as_json` method for Doorkeeper Application model
+  if you previously used `#to_json` serialization with custom options or attributes or rely on
+  JSON response from /oauth/applications.json or /oauth/authorized_applications.json. This change
+  is a breaking change which restricts serialized attributes to a very small set of columns.
+
 - [#1395] Fix `NameError: uninitialized constant Doorkeeper::AccessToken` for Rake tasks.
 - [#1397] Add `as: :doorkeeper_application` on Doorkeeper application form in order to support
   custom configured application model.
 - [#1400] Correctly yield the application instance to `allow_grant_flow_for_client?` config
   option (fixes #1398).
diff --git a/app/controllers/doorkeeper/applications_controller.rb b/app/controllers/doorkeeper/applications_controller.rb
index 8d8043e7..993b3a79 100644
--- a/app/controllers/doorkeeper/applications_controller.rb
+++ b/app/controllers/doorkeeper/applications_controller.rb
@@ -17,11 +17,11 @@ module Doorkeeper
     end
 
     def show
       respond_to do |format|
         format.html
-        format.json { render json: @application }
+        format.json { render json: @application, as_owner: true }
       end
     end
 
     def new
       @application = Doorkeeper.config.application_model.new
@@ -34,11 +34,11 @@ module Doorkeeper
         flash[:notice] = I18n.t(:notice, scope: %i[doorkeeper flash applications create])
         flash[:application_secret] = @application.plaintext_secret
 
         respond_to do |format|
           format.html { redirect_to oauth_application_url(@application) }
-          format.json { render json: @application }
+          format.json { render json: @application, as_owner: true }
         end
       else
         respond_to do |format|
           format.html { render :new }
           format.json do
@@ -56,11 +56,11 @@ module Doorkeeper
       if @application.update(application_params)
         flash[:notice] = I18n.t(:notice, scope: i18n_scope(:update))
 
         respond_to do |format|
           format.html { redirect_to oauth_application_url(@application) }
-          format.json { render json: @application }
+          format.json { render json: @application, as_owner: true }
         end
       else
         respond_to do |format|
           format.html { render :edit }
           format.json do
diff --git a/app/controllers/doorkeeper/authorized_applications_controller.rb b/app/controllers/doorkeeper/authorized_applications_controller.rb
index fe625e66..6bc0b4c5 100644
--- a/app/controllers/doorkeeper/authorized_applications_controller.rb
+++ b/app/controllers/doorkeeper/authorized_applications_controller.rb
@@ -7,11 +7,11 @@ module Doorkeeper
     def index
       @applications = Doorkeeper.config.application_model.authorized_for(current_resource_owner)
 
       respond_to do |format|
         format.html
-        format.json { render json: @applications }
+        format.json { render json: @applications, current_resource_owner: current_resource_owner }
       end
     end
 
     def destroy
       Doorkeeper.config.application_model.revoke_tokens_and_grants_for(
diff --git a/lib/doorkeeper/orm/active_record/mixins/application.rb b/lib/doorkeeper/orm/active_record/mixins/application.rb
index b60aceef..561f2c14 100644
--- a/lib/doorkeeper/orm/active_record/mixins/application.rb
+++ b/lib/doorkeeper/orm/active_record/mixins/application.rb
@@ -59,19 +59,31 @@ module Doorkeeper::Orm::ActiveRecord::Mixins
         else
           @raw_secret
         end
       end
 
-      # This is the right way how we want to override ActiveRecord #to_json
+      # Represents client as set of it's attributes in JSON format.
+      # This is the right way how we want to override ActiveRecord #to_json.
       #
-      # @return [String] entity attributes as JSON
+      # Respects privacy settings and serializes minimum set of attributes
+      # for public/private clients and full set for authorized owners.
+      #
+      # @return [Hash] entity attributes for JSON
       #
       def as_json(options = {})
-        hash = super
-
-        hash["secret"] = plaintext_secret if hash.key?("secret")
-        hash
+        # if application belongs to some owner we need to check if it's the same as
+        # the one passed in the options or check if we render the client as an owner
+        if (respond_to?(:owner) && owner && owner == options[:current_resource_owner]) ||
+           options[:as_owner]
+          # Owners can see all the client attributes, fallback to ActiveModel serialization
+          super
+        else
+          # if application has no owner or it's owner doesn't match one from the options
+          # we render only minimum set of attributes that could be exposed to a public
+          only = extract_serializable_attributes(options)
+          super(options.merge(only: only))
+        end
       end
 
       def authorized_for_resource_owner?(resource_owner)
         Doorkeeper.configuration.authorize_resource_owner_for_client.call(self, resource_owner)
       end
@@ -98,10 +110,53 @@ module Doorkeeper::Orm::ActiveRecord::Mixins
       end
 
       def enforce_scopes?
         Doorkeeper.config.enforce_configured_scopes?
       end
+
+      # Helper method to extract collection of serializable attribute names
+      # considering serialization options (like `only`, `except` and so on).
+      #
+      # @param options [Hash] serialization options
+      #
+      # @return [Array<String>]
+      #   collection of attributes to be serialized using #as_json
+      #
+      def extract_serializable_attributes(options = {})
+        opts = options.try(:dup) || {}
+        only = Array.wrap(opts[:only]).map(&:to_s)
+
+        only = if only.blank?
+                 serializable_attributes
+               else
+                 only & serializable_attributes
+               end
+
+        only -= Array.wrap(opts[:except]).map(&:to_s) if opts.key?(:except)
+        only.uniq
+      end
+
+      # We need to hook into this method to allow serializing plan-text secrets
+      # when secrets hashing enabled.
+      #
+      # @param key [String] attribute name
+      #
+      def read_attribute_for_serialization(key)
+        return super unless key.to_s == "secret"
+
+        plaintext_secret || secret
+      end
+
+      # Collection of attributes that could be serialized for public.
+      # Override this method if you need additional attributes to be serialized.
+      #
+      # @return [Array<String>] collection of serializable attributes
+      def serializable_attributes
+        attributes = %w[id name created_at]
+        attributes << "uid" unless confidential?
+        attributes
+      end
     end
 
     module ClassMethods
       # Returns Applications associated with active (not revoked) Access Tokens
       # that are owned by the specific Resource Owner.
diff --git a/spec/models/doorkeeper/application_spec.rb b/spec/models/doorkeeper/application_spec.rb
index 24a7b173..eaa0e682 100644
--- a/spec/models/doorkeeper/application_spec.rb
+++ b/spec/models/doorkeeper/application_spec.rb
@@ -100,11 +100,11 @@ describe Doorkeeper::Application do
         expect(new_application).to be_valid
       end
     end
 
     context "application owner is required" do
-      before(:each) do
+      before do
         require_owner
         @owner = FactoryBot.build_stubbed(:doorkeeper_testing_user)
       end
 
       it "is invalid without an owner" do
@@ -419,24 +419,72 @@ describe Doorkeeper::Application do
     before do
       allow(Doorkeeper.configuration)
         .to receive(:application_secret_strategy).and_return(Doorkeeper::SecretStoring::Plain)
     end
 
-    it "includes plaintext secret" do
-      expect(app.as_json).to include("secret" => "123123123")
-    end
-
-    it "respects custom options" do
-      expect(app.as_json(except: :secret)).not_to include("secret")
-      expect(app.as_json(only: :id)).to match("id" => app.id)
-    end
-
-    # AR specific
+    # AR specific feature
     if DOORKEEPER_ORM == :active_record
       it "correctly works with #to_json" do
         ActiveRecord::Base.include_root_in_json = true
         expect(app.to_json(include_root_in_json: true)).to match(/application.+?:\{/)
         ActiveRecord::Base.include_root_in_json = false
       end
     end
+
+    context "when called without authorized resource owner" do
+      it "includes minimal set of attributes" do
+        expect(app.as_json).to match(
+          "id" => app.id,
+          "name" => app.name,
+          "created_at" => an_instance_of(String),
+        )
+      end
+
+      it "includes application UID if it's public" do
+        app = FactoryBot.create :application, secret: "123123123", confidential: false
+
+        expect(app.as_json).to match(
+          "id" => app.id,
+          "name" => app.name,
+          "created_at" => an_instance_of(String),
+          "uid" => app.uid,
+        )
+      end
+
+      it "respects custom options" do
+        expect(app.as_json(except: :id)).not_to include("id")
+        expect(app.as_json(only: %i[name created_at secret]))
+          .to match(
+            "name" => app.name,
+            "created_at" => an_instance_of(String),
+          )
+      end
+    end
+
+    context "when called with authorized resource owner" do
+      let(:owner) { FactoryBot.create(:doorkeeper_testing_user) }
+      let(:other_owner) { FactoryBot.create(:doorkeeper_testing_user) }
+      let(:app) { FactoryBot.create(:application, secret: "123123123", owner: owner) }
+
+      before do
+        Doorkeeper.configure do
+          orm DOORKEEPER_ORM
+          enable_application_owner confirmation: false
+        end
+      end
+
+      it "includes all the attributes" do
+        expect(app.as_json(current_resource_owner: owner))
+          .to include(
+            "secret" => "123123123",
+            "redirect_uri" => app.redirect_uri,
+            "uid" => app.uid,
+          )
+      end
+
+      it "doesn't include unsafe attributes if current owner isn't the same as owner" do
+        expect(app.as_json(current_resource_owner: other_owner))
+          .not_to include("redirect_uri")
+      end
+    end
   end
 end

Vulnerable code:         format.json { render json: @application }
          format.json { render json: @application }
          format.json { render json: @application }
        format.json { render json: @applications }
      # This is the right way how we want to override ActiveRecord #to_json
      # @return [String] entity attributes as JSON
        hash = super

        hash["secret"] = plaintext_secret if hash.key?("secret")
        hash
      before(:each) do
    it "includes plaintext secret" do
      expect(app.as_json).to include("secret" => "123123123")
    end

    it "respects custom options" do
      expect(app.as_json(except: :secret)).not_to include("secret")
      expect(app.as_json(only: :id)).to match("id" => app.id)
    end

    # AR specific
Fixed code: - [#1371] Add `#as_json` method and attributes serialization restriction for Application model.
  Fixes information disclosure vulnerability (CVE-2020-10187).
  
  **[IMPORTANT]** you need to re-implement `#as_json` method for Doorkeeper Application model
  if you previously used `#to_json` serialization with custom options or attributes or rely on
  JSON response from /oauth/applications.json or /oauth/authorized_applications.json. This change
  is a breaking change which restricts serialized attributes to a very small set of columns.

        format.json { render json: @application, as_owner: true }
          format.json { render json: @application, as_owner: true }
          format.json { render json: @application, as_owner: true }
        format.json { render json: @applications, current_resource_owner: current_resource_owner }
      # Represents client as set of it's attributes in JSON format.
      # This is the right way how we want to override ActiveRecord #to_json.
      # Respects privacy settings and serializes minimum set of attributes
      # for public/private clients and full set for authorized owners.
      #
      # @return [Hash] entity attributes for JSON
        # if application belongs to some owner we need to check if it's the same as
        # the one passed in the options or check if we render the client as an owner
        if (respond_to?(:owner) && owner && owner == options[:current_resource_owner]) ||
           options[:as_owner]
          # Owners can see all the client attributes, fallback to ActiveModel serialization
          super
        else
          # if application has no owner or it's owner doesn't match one from the options
          # we render only minimum set of attributes that could be exposed to a public
          only = extract_serializable_attributes(options)
          super(options.merge(only: only))
        end

      # Helper method to extract collection of serializable attribute names
      # considering serialization options (like `only`, `except` and so on).
      #
      # @param options [Hash] serialization options
      #
      # @return [Array<String>]
      #   collection of attributes to be serialized using #as_json
      #
      def extract_serializable_attributes(options = {})
        opts = options.try(:dup) || {}
        only = Array.wrap(opts[:only]).map(&:to_s)

        only = if only.blank?
                 serializable_attributes
               else
                 only & serializable_attributes
               end

        only -= Array.wrap(opts[:except]).map(&:to_s) if opts.key?(:except)
        only.uniq
      end

      # We need to hook into this method to allow serializing plan-text secrets
      # when secrets hashing enabled.
      #
      # @param key [String] attribute name
      #
      def read_attribute_for_serialization(key)
        return super unless key.to_s == "secret"

        plaintext_secret || secret
      end

      # Collection of attributes that could be serialized for public.
      # Override this method if you need additional attributes to be serialized.
      #
      # @return [Array<String>] collection of serializable attributes
      def serializable_attributes
        attributes = %w[id name created_at]
        attributes << "uid" unless confidential?
        attributes
      end
      before do
    # AR specific feature

    context "when called without authorized resource owner" do
      it "includes minimal set of attributes" do
        expect(app.as_json).to match(
          "id" => app.id,
          "name" => app.name,
          "created_at" => an_instance_of(String),
        )
      end

      it "includes application UID if it's public" do
        app = FactoryBot.create :application, secret: "123123123", confidential: false

        expect(app.as_json).to match(
          "id" => app.id,
          "name" => app.name,
          "created_at" => an_instance_of(String),
          "uid" => app.uid,
        )
      end

      it "respects custom options" do
        expect(app.as_json(except: :id)).not_to include("id")
        expect(app.as_json(only: %i[name created_at secret]))
          .to match(
            "name" => app.name,
            "created_at" => an_instance_of(String),
          )
      end
    end

    context "when called with authorized resource owner" do
      let(:owner) { FactoryBot.create(:doorkeeper_testing_user) }
      let(:other_owner) { FactoryBot.create(:doorkeeper_testing_user) }
      let(:app) { FactoryBot.create(:application, secret: "123123123", owner: owner) }

      before do
        Doorkeeper.configure do
          orm DOORKEEPER_ORM
          enable_application_owner confirmation: false
        end
      end

      it "includes all the attributes" do
        expect(app.as_json(current_resource_owner: owner))
          .to include(
            "secret" => "123123123",
            "redirect_uri" => app.redirect_uri,
            "uid" => app.uid,
          )
      end

      it "doesn't include unsafe attributes if current owner isn't the same as owner" do
        expect(app.as_json(current_resource_owner: other_owner))
          .not_to include("redirect_uri")
      end
    end