Merge remote-tracking branch 'dev/master'

CHANGELOG-EE.md

 Please view this file on the master branch, on stable branches it's out of date.
 
+## 13.7.4 (2021-01-13)
+
+- No changes.
+
 ## 13.7.3 (2021-01-08)
 
 - No changes.
@@ -170,6 +174,10 @@ Please view this file on the master branch, on stable branches it's out of date.
 - Rename code coverage analytics sections. !49931
 
 
+## 13.6.5 (2021-01-13)
+
+- No changes.
+
 ## 13.6.4 (2021-01-07)
 
 - No changes.
@@ -369,6 +377,10 @@ Please view this file on the master branch, on stable branches it's out of date.
 - Remove duplicated BS display properties from member overriding UI. !47126 (Takuya Noguchi)
 
 
+## 13.5.7 (2021-01-13)
+
+- No changes.
+
 ## 13.5.6 (2021-01-07)
 
 - No changes.

CHANGELOG.md

@@ -2,6 +2,13 @@
 documentation](doc/development/changelog.md) for instructions on adding your own
 entry.
 
+## 13.7.4 (2021-01-13)
+
+### Security (1 change)
+
+- Deny implicit flow for confidential apps.
+
+
 ## 13.7.3 (2021-01-08)
 
 ### Fixed (7 changes)
@@ -497,6 +504,13 @@ entry.
 - Update GitLab Workhorse to v8.57.0.
 
 
+## 13.6.5 (2021-01-13)
+
+### Security (1 change)
+
+- Deny implicit flow for confidential apps.
+
+
 ## 13.6.4 (2021-01-07)
 
 ### Security (7 changes)
@@ -1068,6 +1082,13 @@ entry.
 - Change wording on the project remove fork page. !47878
 
 
+## 13.5.7 (2021-01-13)
+
+### Security (1 change)
+
+- Deny implicit flow for confidential apps.
+
+
 ## 13.5.6 (2021-01-07)
 
 ### Security (7 changes)

app/controllers/oauth/authorizations_controller.rb

@@ -4,7 +4,7 @@ class Oauth::AuthorizationsController < Doorkeeper::AuthorizationsController
   include Gitlab::Experimentation::ControllerConcern
   include InitializesCurrentUserMode
 
-  before_action :verify_confirmed_email!
+  before_action :verify_confirmed_email!, :verify_confidential_application!
 
   layout 'profile'
 
@@ -24,18 +24,19 @@ class Oauth::AuthorizationsController < Doorkeeper::AuthorizationsController
     end
   end
 
-  def create
-    # Confidential apps require the client_secret to be sent with the request.
-    # Doorkeeper allows implicit grant flow requests (response_type=token) to
-    # work without client_secret regardless of the confidential setting.
-    if pre_auth.authorizable? && pre_auth.response_type == 'token' && pre_auth.client.application.confidential
-      render "doorkeeper/authorizations/error"
-    else
-      super
-    end
+  private
+
+  # Confidential apps require the client_secret to be sent with the request.
+  # Doorkeeper allows implicit grant flow requests (response_type=token) to
+  # work without client_secret regardless of the confidential setting.
+  # This leads to security vulnerabilities and we want to block it.
+  def verify_confidential_application!
+    render 'doorkeeper/authorizations/error' if authorizable_confidential?
   end
 
-  private
+  def authorizable_confidential?
+    pre_auth.authorizable? && pre_auth.response_type == 'token' && pre_auth.client.application.confidential
+  end
 
   def verify_confirmed_email!
     return if current_user&.confirmed?

spec/controllers/oauth/authorizations_controller_spec.rb

@@ -51,10 +51,27 @@ RSpec.describe Oauth::AuthorizationsController do
     end
   end
 
+  shared_examples "Implicit grant can't be used in confidential application" do
+    context 'when application is confidential' do
+      before do
+        application.update(confidential: true)
+        params[:response_type] = 'token'
+      end
+
+      it 'does not allow the implicit flow' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to render_template('doorkeeper/authorizations/error')
+      end
+    end
+  end
+
   describe 'GET #new' do
     subject { get :new, params: params }
 
     include_examples 'OAuth Authorizations require confirmed user'
+    include_examples "Implicit grant can't be used in confidential application"
 
     context 'when the user is confirmed' do
       let(:confirmed_at) { 1.hour.ago }
@@ -95,26 +112,14 @@ RSpec.describe Oauth::AuthorizationsController do
     subject { post :create, params: params }
 
     include_examples 'OAuth Authorizations require confirmed user'
-
-    context 'when application is confidential' do
-      before do
-        application.update(confidential: true)
-        params[:response_type] = 'token'
-      end
-
-      it 'does not allow the implicit flow' do
-        subject
-
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(response).to render_template('doorkeeper/authorizations/error')
-      end
-    end
+    include_examples "Implicit grant can't be used in confidential application"
   end
 
   describe 'DELETE #destroy' do
     subject { delete :destroy, params: params }
 
     include_examples 'OAuth Authorizations require confirmed user'
+    include_examples "Implicit grant can't be used in confidential application"
   end
 
   it 'includes Two-factor enforcement concern' do