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>

CHANGELOG.md

@@ -7,6 +7,14 @@ 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.

app/controllers/doorkeeper/applications_controller.rb

@@ -19,7 +19,7 @@ def index
     def show
       respond_to do |format|
         format.html
-        format.json { render json: @application }
+        format.json { render json: @application, as_owner: true }
       end
     end
 
@@ -36,7 +36,7 @@ def create
 
         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|
@@ -58,7 +58,7 @@ def 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|

app/controllers/doorkeeper/authorized_applications_controller.rb

@@ -9,7 +9,7 @@ def index
 
       respond_to do |format|
         format.html
-        format.json { render json: @applications }
+        format.json { render json: @applications, current_resource_owner: current_resource_owner }
       end
     end
 

lib/doorkeeper/orm/active_record/mixins/application.rb

@@ -61,15 +61,27 @@ def plaintext_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)
@@ -100,6 +112,49 @@ def scopes_match_configured
       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

spec/models/doorkeeper/application_spec.rb

@@ -102,7 +102,7 @@
     end
 
     context "application owner is required" do
-      before(:each) do
+      before do
         require_owner
         @owner = FactoryBot.build_stubbed(:doorkeeper_testing_user)
       end
@@ -421,22 +421,70 @@
         .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