From 3b8d085436fa38aed4d5fa3650e433fc7215b104 Mon Sep 17 00:00:00 2001
From: Eugen Rochko <eugen@zeonfederated.com>
Date: Thu, 15 Apr 2021 16:28:43 +0200
Subject: [PATCH] Fix app name, website and redirect URIs not having a maximum
 length (#16042)

Fix app scopes not being validated
---
 app/lib/application_extension.rb              |  4 +-
 config/initializers/doorkeeper.rb             |  5 ++
 .../api/v1/apps_controller_spec.rb            | 78 ++++++++++++++++---
 3 files changed, 77 insertions(+), 10 deletions(-)

diff --git a/app/lib/application_extension.rb b/app/lib/application_extension.rb
index 1d80b8c6d..e61cd0721 100644
--- a/app/lib/application_extension.rb
+++ b/app/lib/application_extension.rb
@@ -4,6 +4,8 @@ module ApplicationExtension
   extend ActiveSupport::Concern
 
   included do
-    validates :website, url: true, if: :website?
+    validates :name, length: { maximum: 60 }
+    validates :website, url: true, length: { maximum: 2_000 }, if: :website?
+    validates :redirect_uri, length: { maximum: 2_000 }
   end
 end
diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb
index 63cff7c59..f78db8653 100644
--- a/config/initializers/doorkeeper.rb
+++ b/config/initializers/doorkeeper.rb
@@ -52,6 +52,11 @@ Doorkeeper.configure do
   # Issue access tokens with refresh token (disabled by default)
   # 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
diff --git a/spec/controllers/api/v1/apps_controller_spec.rb b/spec/controllers/api/v1/apps_controller_spec.rb
index 60a4c3b41..70cd62d48 100644
--- a/spec/controllers/api/v1/apps_controller_spec.rb
+++ b/spec/controllers/api/v1/apps_controller_spec.rb
@@ -4,23 +4,83 @@ RSpec.describe Api::V1::AppsController, type: :controller do
   render_views
 
   describe 'POST #create' do
+    let(:client_name) { 'Test app' }
+    let(:scopes) { nil }
+    let(:redirect_uris) { 'urn:ietf:wg:oauth:2.0:oob' }
+    let(:website) { nil }
+
+    let(:app_params) do
+      {
+        client_name: client_name,
+        redirect_uris: redirect_uris,
+        scopes: scopes,
+        website: website,
+      }
+    end
+
     before do
-      post :create, params: { client_name: 'Test app', redirect_uris: 'urn:ietf:wg:oauth:2.0:oob' }
+      post :create, params: app_params
     end
 
-    it 'returns http success' do
-      expect(response).to have_http_status(200)
+    context 'with valid params' do
+      it 'returns http success' do
+        expect(response).to have_http_status(200)
+      end
+
+      it 'creates an OAuth app' do
+        expect(Doorkeeper::Application.find_by(name: client_name)).to_not be nil
+      end
+
+      it 'returns client ID and client secret' do
+        json = body_as_json
+
+        expect(json[:client_id]).to_not be_blank
+        expect(json[:client_secret]).to_not be_blank
+      end
     end
 
-    it 'creates an OAuth app' do
-      expect(Doorkeeper::Application.find_by(name: 'Test app')).to_not be nil
+    context 'with an unsupported scope' do
+      let(:scopes) { 'hoge' }
+
+      it 'returns http unprocessable entity' do
+        expect(response).to have_http_status(422)
+      end
     end
 
-    it 'returns client ID and client secret' do
-      json = body_as_json
+    context 'with many duplicate scopes' do
+      let(:scopes) { (%w(read) * 40).join(' ') }
 
-      expect(json[:client_id]).to_not be_blank
-      expect(json[:client_secret]).to_not be_blank
+      it 'returns http success' do
+        expect(response).to have_http_status(200)
+      end
+
+      it 'only saves the scope once' do
+        expect(Doorkeeper::Application.find_by(name: client_name).scopes.to_s).to eq 'read'
+      end
+    end
+
+    context 'with a too-long name' do
+      let(:client_name) { 'hoge' * 20 }
+
+      it 'returns http unprocessable entity' do
+        expect(response).to have_http_status(422)
+      end
+    end
+
+    context 'with a too-long website' do
+      let(:website) { 'https://foo.bar/' + ('hoge' * 2_000) }
+
+      it 'returns http unprocessable entity' do
+        expect(response).to have_http_status(422)
+      end
+    end
+
+    context 'with a too-long redirect_uris' do
+      let(:redirect_uris) { 'https://foo.bar/' + ('hoge' * 2_000) }
+
+      it 'returns http unprocessable entity' do
+        expect(response).to have_http_status(422)
+      end
     end
   end
 end