Merge branch 'jej/group-saml-sso-button-link-description' into 'master'

Group SAML SSO page warns when linking account

Closes #8260

See merge request gitlab-org/gitlab-ee!8295

app/validators/url_validator.rb

@@ -69,6 +69,7 @@ class UrlValidator < ActiveModel::EachValidator
       ports: [],
       allow_localhost: true,
       allow_local_network: true,
+      ascii_only: false,
       enforce_user: false
     }
   end

ee/app/controllers/groups/sso_controller.rb

@@ -15,6 +15,8 @@ class Groups::SsoController < Groups::ApplicationController
   def saml
     @group_path = params[:group_id]
     @group_name = @unauthenticated_group.full_name
+    @group_saml_identity = linked_identity
+    @idp_url = @unauthenticated_group.saml_provider.sso_url
   end
 
   def unlink

ee/app/models/saml_provider.rb

@@ -5,7 +5,7 @@ class SamlProvider < ActiveRecord::Base
   has_many :identities
 
   validates :group, presence: true, top_level_group: true
-  validates :sso_url, presence: true, url: { protocols: %w(https) }
+  validates :sso_url, presence: true, url: { protocols: %w(https), ascii_only: true }
   validates :certificate_fingerprint, presence: true, certificate_fingerprint: true
 
   after_initialize :set_defaults, if: :new_record?

ee/app/views/groups/sso/saml.html.haml

@@ -3,8 +3,19 @@
 = render 'devise/shared/tab_single', tab_title: _('SAML SSO')
 .login-box
   .login-body
-    %h4= _("Sign in to %{group_name}") % { group_name: @group_name }
+    - if @group_saml_identity
+      %h4= _('Sign in to "%{group_name}"') % { group_name: @group_name }
+    - else
+      %h4= _('Allow "%{group_name}" to sign you in') % { group_name: @group_name }
 
-    %p= _("This group allows you to sign in with your %{group_name} Single Sign-On account. This will redirect you to an external sign in page.") % { group_name: @group_name }
+    %p= _('The "%{group_path}" group allows you to sign in with your Single Sign-On Account') % { group_path: @group_path }
 
-    = saml_link _('Sign in with Single Sign-On'), @group_path, html_class: 'btn btn-success btn-block qa-saml-sso-signin-button'
+    - if @group_saml_identity
+      %p= _("This will redirect you to an external sign in page.")
+
+      = saml_link _('Sign in with Single Sign-On'), @group_path, html_class: 'btn btn-success btn-block qa-saml-sso-signin-button'
+    - else
+      .card.card-body.bs-callout-warning
+        = _("Only proceed if you trust %{idp_url} to control your GitLab account sign in.") % { idp_url: @idp_url }
+
+      = saml_link _('Authorize'), @group_path, html_class: 'btn btn-success btn-block qa-saml-sso-signin-button'

ee/changelogs/unreleased/jej-group-saml-sso-button-link-description.yml

+---
+title: Group SAML SSO page warns when linking account
+merge_request: 8295
+author:
+type: changed

ee/spec/features/groups/saml_providers_spec.rb

@@ -154,15 +154,37 @@ describe 'SAML provider settings' do
       context 'when signed in' do
         before do
           sign_in(user)
+        end
 
+        it 'shows warning that linking accounts authorizes control over sign in' do
           visit sso_group_saml_providers_path(group)
+
+          expect(page).to have_content(/Allow .* to sign you in/)
+          expect(page).to have_content(saml_provider.sso_url)
+          expect(page).to have_content('Authorize')
         end
 
-        it 'Sign in button redirects to auth flow and back to group' do
-          click_link 'Sign in with Single Sign-On'
+        it 'Authorize/link button redirects to auth flow' do
+          visit sso_group_saml_providers_path(group)
+
+          click_link 'Authorize'
 
           expect(current_path).to eq callback_path
         end
+
+        context 'with linked account' do
+          before do
+            create(:group_saml_identity, saml_provider: saml_provider, user: user)
+          end
+
+          it 'Sign in button redirects to auth flow' do
+            visit sso_group_saml_providers_path(group)
+
+            click_link 'Sign in with Single Sign-On'
+
+            expect(current_path).to eq callback_path
+          end
+        end
       end
 
       context 'for a private group' do
@@ -187,7 +209,7 @@ describe 'SAML provider settings' do
             expect(current_path).to eq sso_group_saml_providers_path(group)
 
             within '.login-box' do
-              expect(page).to have_link 'Sign in with Single Sign-On'
+              expect(page).to have_link 'Authorize'
             end
           end
 

ee/spec/models/saml_provider_spec.rb

@@ -16,6 +16,15 @@ describe SamlProvider do
       expect(subject).not_to allow_value('http://example.com').for(:sso_url)
     end
 
+    it 'prevents homoglyph phishing attacks by only allowing ascii URLs' do
+      expect(subject).to allow_value('https://gitlab.com/adfs/ls').for(:sso_url)
+      expect(subject).not_to allow_value('https://𝕘itⅼaƄ.ᴄοｍ/adfs/ls').for(:sso_url)
+    end
+
+    it 'allows unicode domain names when encoded as ascii punycode' do
+      expect(subject).to allow_value('https://xn--gitl-ocb944a.xn--m-rmb025q/adfs/ls').for(:sso_url)
+    end
+
     it 'expects certificate_fingerprint to be in an accepted format' do
       expect(subject).to allow_value('000030EDC285E01D6B5EA33010A79ADD142F5004').for(:certificate_fingerprint)
       expect(subject).to allow_value('00:00:30:ED:C2:85:E0:1D:6B:5E:A3:30:10:A7:9A:DD:14:2F:50:04').for(:certificate_fingerprint)

lib/gitlab/url_blocker.rb

@@ -8,7 +8,7 @@ module Gitlab
     BlockedUrlError = Class.new(StandardError)
 
     class << self
-      def validate!(url, allow_localhost: false, allow_local_network: true, enforce_user: false, ports: [], protocols: [])
+      def validate!(url, ports: [], protocols: [], allow_localhost: false, allow_local_network: true, ascii_only: false, enforce_user: false)
         return true if url.nil?
 
         # Param url can be a string, URI or Addressable::URI
@@ -22,6 +22,7 @@ module Gitlab
         validate_port!(port, ports) if ports.any?
         validate_user!(uri.user) if enforce_user
         validate_hostname!(uri.hostname)
+        validate_unicode_restriction!(uri) if ascii_only
 
         begin
           addrs_info = Addrinfo.getaddrinfo(uri.hostname, port, nil, :STREAM).map do |addr|
@@ -91,6 +92,12 @@ module Gitlab
         raise BlockedUrlError, "Hostname or IP address invalid"
       end
 
+      def validate_unicode_restriction!(uri)
+        return if uri.to_s.ascii_only?
+
+        raise BlockedUrlError, "URI must be ascii only #{uri.to_s.dump}"
+      end
+
       def validate_localhost!(addrs_info)
         local_ips = ["::", "0.0.0.0"]
         local_ips.concat(Socket.ip_address_list.map(&:ip_address))

locale/gitlab.pot

@@ -578,6 +578,9 @@ msgstr ""
 msgid "All users"
 msgstr ""
 
+msgid "Allow \"%{group_name}\" to sign you in"
+msgstr ""
+
 msgid "Allow commits from members who can merge to the target branch."
 msgstr ""
 
@@ -6003,6 +6006,9 @@ msgstr ""
 msgid "Only mirror protected branches"
 msgstr ""
 
+msgid "Only proceed if you trust %{idp_url} to control your GitLab account sign in."
+msgstr ""
+
 msgid "Only project members can comment."
 msgstr ""
 
@@ -7875,7 +7881,7 @@ msgstr ""
 msgid "Sign in / Register"
 msgstr ""
 
-msgid "Sign in to %{group_name}"
+msgid "Sign in to \"%{group_name}\""
 msgstr ""
 
 msgid "Sign in via 2FA code"
@@ -8454,6 +8460,9 @@ msgstr ""
 msgid "Thanks! Don't show me this again"
 msgstr ""
 
+msgid "The \"%{group_path}\" group allows you to sign in with your Single Sign-On Account"
+msgstr ""
+
 msgid "The Advanced Global Search in GitLab is a powerful search service that saves you time. Instead of creating duplicate code and wasting time, you can now search for code within other teams that can help your own project."
 msgstr ""
 
@@ -8673,9 +8682,6 @@ msgstr ""
 msgid "This group"
 msgstr ""
 
-msgid "This group allows you to sign in with your %{group_name} Single Sign-On account. This will redirect you to an external sign in page."
-msgstr ""
-
 msgid "This group does not provide any group Runners yet."
 msgstr ""
 
@@ -8811,6 +8817,9 @@ msgstr ""
 msgid "This user will be the author of all events in the activity feed that are the result of an update, like new branches being created or new commits being pushed to existing branches. Upon creation or when reassigning you can only assign yourself to be the mirror user."
 msgstr ""
 
+msgid "This will redirect you to an external sign in page."
+msgstr ""
+
 msgid "Those emails automatically become issues (with the comments becoming the email conversation) listed here."
 msgstr ""
 

spec/lib/gitlab/url_blocker_spec.rb

@@ -249,6 +249,27 @@ describe Gitlab::UrlBlocker do
         end
       end
     end
+
+    context 'when ascii_only is true' do
+      it 'returns true for unicode domain' do
+        expect(described_class.blocked_url?('https://𝕘itⅼαƄ.com/foo/foo.bar', ascii_only: true)).to be true
+      end
+
+      it 'returns true for unicode tld' do
+        expect(described_class.blocked_url?('https://gitlab.ᴄοｍ/foo/foo.bar', ascii_only: true)).to be true
+      end
+
+      it 'returns true for unicode path' do
+        expect(described_class.blocked_url?('https://gitlab.com/𝒇οο/𝒇οο.Ƅαꮁ', ascii_only: true)).to be true
+      end
+
+      it 'returns true for IDNA deviations' do
+        expect(described_class.blocked_url?('https://mißile.com/foo/foo.bar', ascii_only: true)).to be true
+        expect(described_class.blocked_url?('https://miςςile.com/foo/foo.bar', ascii_only: true)).to be true
+        expect(described_class.blocked_url?('https://git‍lab.com/foo/foo.bar', ascii_only: true)).to be true
+        expect(described_class.blocked_url?('https://git‌lab.com/foo/foo.bar', ascii_only: true)).to be true
+      end
+    end
   end
 
   describe '#validate_hostname!' do

spec/validators/url_validator_spec.rb

@@ -143,4 +143,33 @@ describe UrlValidator do
       end
     end
   end
+
+  context 'when ascii_only is' do
+    let(:url) { 'https://𝕘itⅼαƄ.com/foo/foo.bar'}
+    let(:validator) { described_class.new(attributes: [:link_url], ascii_only: ascii_only) }
+
+    context 'true' do
+      let(:ascii_only) { true }
+
+      it 'prevents unicode characters' do
+        badge.link_url = url
+
+        subject
+
+        expect(badge.errors.empty?).to be false
+      end
+    end
+
+    context 'false (default)' do
+      let(:ascii_only) { false }
+
+      it 'does not prevent unicode characters' do
+        badge.link_url = url
+
+        subject
+
+        expect(badge.errors.empty?).to be true
+      end
+    end
+  end
 end