Merge branch 'security-fix-uri-xss-applications' into 'master'

[master] Resolve "Reflected XSS in OAuth Authorize window due to redirect_uri allowing arbitrary protocols"

See merge request gitlab/gitlabhq!2572

3 files changed, 76 insertions, 1 deletions

diff --git a/spec/controllers/oauth/applications_controller_spec.rb b/spec/controllers/oauth/applications_controller_spec.rb
index ace8a954e92..b4219856fc0 100644
--- a/spec/controllers/oauth/applications_controller_spec.rb
+++ b/spec/controllers/oauth/applications_controller_spec.rb
@@ -40,6 +40,23 @@ describe Oauth::ApplicationsController do
         expect(response).to have_gitlab_http_status(302)
         expect(response).to redirect_to(profile_path)
       end
+
+      context 'redirect_uri' do
+        render_views
+
+        it 'shows an error for a forbidden URI' do
+          invalid_uri_params = {
+            doorkeeper_application: {
+              name: 'foo',
+              redirect_uri: 'javascript://alert()'
+            }
+          }
+
+          post :create, invalid_uri_params
+
+          expect(response.body).to include 'Redirect URI is forbidden by the server'
+        end
+      end
     end
   end
 
diff --git a/spec/migrations/migrate_forbidden_redirect_uris_spec.rb b/spec/migrations/migrate_forbidden_redirect_uris_spec.rb
new file mode 100644
index 00000000000..0bc13a3974a
--- /dev/null
+++ b/spec/migrations/migrate_forbidden_redirect_uris_spec.rb
@@ -0,0 +1,48 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20181026091631_migrate_forbidden_redirect_uris.rb')
+
+describe MigrateForbiddenRedirectUris, :migration do
+  let(:oauth_application) { table(:oauth_applications) }
+  let(:oauth_access_grant) { table(:oauth_access_grants) }
+
+  let!(:control_app) { oauth_application.create(random_params) }
+  let!(:control_access_grant) { oauth_application.create(random_params) }
+  let!(:forbidden_js_app) { oauth_application.create(random_params.merge(redirect_uri: 'javascript://alert()')) }
+  let!(:forbidden_vb_app) { oauth_application.create(random_params.merge(redirect_uri: 'VBSCRIPT://alert()')) }
+  let!(:forbidden_access_grant) { oauth_application.create(random_params.merge(redirect_uri: 'vbscript://alert()')) }
+
+  context 'oauth application' do
+    it 'migrates forbidden javascript URI' do
+      expect { migrate! }.to change { forbidden_js_app.reload.redirect_uri }.to('http://forbidden-scheme-has-been-overwritten')
+    end
+
+    it 'migrates forbidden VBScript URI' do
+      expect { migrate! }.to change { forbidden_vb_app.reload.redirect_uri }.to('http://forbidden-scheme-has-been-overwritten')
+    end
+
+    it 'does not migrate a valid URI' do
+      expect { migrate! }.not_to change { control_app.reload.redirect_uri }
+    end
+  end
+
+  context 'access grant' do
+    it 'migrates forbidden VBScript URI' do
+      expect { migrate! }.to change { forbidden_access_grant.reload.redirect_uri }.to('http://forbidden-scheme-has-been-overwritten')
+    end
+
+    it 'does not migrate a valid URI' do
+      expect { migrate! }.not_to change { control_access_grant.reload.redirect_uri }
+    end
+  end
+
+  def random_params
+    {
+      name: 'test',
+      secret: 'test',
+      uid: Doorkeeper::OAuth::Helpers::UniqueToken.generate,
+      redirect_uri: 'http://valid.com'
+    }
+  end
+end
diff --git a/spec/requests/api/applications_spec.rb b/spec/requests/api/applications_spec.rb
index 270e12bf201..6154be5c425 100644
--- a/spec/requests/api/applications_spec.rb
+++ b/spec/requests/api/applications_spec.rb
@@ -25,7 +25,7 @@ describe API::Applications, :api do
 
       it 'does not allow creating an application with the wrong redirect_uri format' do
         expect do
-          post api('/applications', admin_user), name: 'application_name', redirect_uri: 'wrong_url_format', scopes: ''
+          post api('/applications', admin_user), name: 'application_name', redirect_uri: 'http://', scopes: ''
         end.not_to change { Doorkeeper::Application.count }
 
         expect(response).to have_gitlab_http_status(400)
@@ -33,6 +33,16 @@ describe API::Applications, :api do
         expect(json_response['message']['redirect_uri'][0]).to eq('must be an absolute URI.')
       end
 
+      it 'does not allow creating an application with a forbidden URI format' do
+        expect do
+          post api('/applications', admin_user), name: 'application_name', redirect_uri: 'javascript://alert()', scopes: ''
+        end.not_to change { Doorkeeper::Application.count }
+
+        expect(response).to have_gitlab_http_status(400)
+        expect(json_response).to be_a Hash
+        expect(json_response['message']['redirect_uri'][0]).to eq('is forbidden by the server.')
+      end
+
       it 'does not allow creating an application without a name' do
         expect do
           post api('/applications', admin_user), redirect_uri: 'http://application.url', scopes: ''