Merge branch 'security-fix-pat-web-access-11-3' into 'security-11-3'

[11.3] Resolve "Personal access token with only `read_user` scope can be used to authenticate any web request"

See merge request gitlab/gitlabhq!2657

app/controllers/application_controller.rb

@@ -10,8 +10,8 @@ class ApplicationController < ActionController::Base
   include WorkhorseHelper
   include EnforcesTwoFactorAuthentication
   include WithPerformanceBar
-  before_action :authenticate_sessionless_user!
+  include SessionlessAuthentication
   before_action :authenticate_user!
   before_action :enforce_terms!, if: :should_enforce_terms?
   before_action :validate_user_service_ticket!
@@ -140,13 +140,6 @@ class ApplicationController < ActionController::Base
     end
   end
-  # This filter handles personal access tokens, and atom requests with rss tokens
-  def authenticate_sessionless_user!
-    user = Gitlab::Auth::RequestAuthenticator.new(request).find_sessionless_user
-
-    sessionless_sign_in(user) if user
-  end
-
   def log_exception(exception)
     Raven.capture_exception(exception) if sentry_enabled?
@@ -412,25 +405,11 @@ class ApplicationController < ActionController::Base
     Gitlab::I18n.with_user_locale(current_user, &block)
   end
-  def sessionless_sign_in(user)
-    if user && can?(user, :log_in)
-      # Notice we are passing store false, so the user is not
-      # actually stored in the session and a token is needed
-      # for every request. If you want the token to work as a
-      # sign in token, you can simply remove store: false.
-      sign_in(user, store: false, message: :sessionless_sign_in)
-    end
-  end
-
   def set_page_title_header
     # Per https://tools.ietf.org/html/rfc5987, headers need to be ISO-8859-1, not UTF-8
     response.headers['Page-Title'] = URI.escape(page_title('GitLab'))
   end
-  def sessionless_user?
-    current_user && !session.keys.include?('warden.user.user.key')
-  end
-
   def peek_request?
     request.path.start_with?('/-/peek')
   end

app/controllers/concerns/sessionless_authentication.rb

+# frozen_string_literal: true
+
+# == SessionlessAuthentication
+#
+# Controller concern to handle PAT and RSS token authentication methods
+#
+module SessionlessAuthentication
+  # This filter handles personal access tokens, and atom requests with rss tokens
+  def authenticate_sessionless_user!(request_format)
+    user = Gitlab::Auth::RequestAuthenticator.new(request).find_sessionless_user(request_format)
+
+    sessionless_sign_in(user) if user
+  end
+
+  def sessionless_user?
+    current_user && !session.keys.include?('warden.user.user.key')
+  end
+
+  def sessionless_sign_in(user)
+    if user && can?(user, :log_in)
+      # Notice we are passing store false, so the user is not
+      # actually stored in the session and a token is needed
+      # for every request. If you want the token to work as a
+      # sign in token, you can simply remove store: false.
+      sign_in(user, store: false, message: :sessionless_sign_in)
+    end
+  end
+end

app/controllers/dashboard/projects_controller.rb

@@ -2,6 +2,7 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController
   include ParamsBackwardCompatibility
   include RendersMemberAccess
+  prepend_before_action(only: [:index]) { authenticate_sessionless_user!(:rss) }
   before_action :set_non_archived_param
   before_action :default_sorting
   skip_cross_project_access_check :index, :starred

app/controllers/dashboard_controller.rb

@@ -9,6 +9,9 @@ class DashboardController < Dashboard::ApplicationController
     :label_name
   ].freeze
+  prepend_before_action(only: [:issues]) { authenticate_sessionless_user!(:rss) }
+  prepend_before_action(only: [:issues_calendar]) { authenticate_sessionless_user!(:ics) }
+
   before_action :event_filter, only: :activity
   before_action :projects, only: [:issues, :merge_requests]
   before_action :set_show_full_reference, only: [:issues, :merge_requests]

app/controllers/graphql_controller.rb

 class GraphqlController < ApplicationController
   # Unauthenticated users have access to the API for public data
   skip_before_action :authenticate_user!
+  prepend_before_action(only: [:execute]) { authenticate_sessionless_user!(:api) }
   before_action :check_graphql_feature_flag!

app/controllers/groups_controller.rb

@@ -7,6 +7,9 @@ class GroupsController < Groups::ApplicationController
   respond_to :html
+  prepend_before_action(only: [:show, :issues]) { authenticate_sessionless_user!(:rss) }
+  prepend_before_action(only: [:issues_calendar]) { authenticate_sessionless_user!(:ics) }
+
   before_action :authenticate_user!, only: [:new, :create]
   before_action :group, except: [:index, :new, :create]

app/controllers/projects/commits_controller.rb

@@ -4,6 +4,7 @@ class Projects::CommitsController < Projects::ApplicationController
   include ExtractsPath
   include RendersCommits
+  prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:rss) }
   before_action :whitelist_query_limiting, except: :commits_root
   before_action :require_non_empty_project
   before_action :assign_ref_vars, except: :commits_root

app/controllers/projects/issues_controller.rb

@@ -7,7 +7,10 @@ class Projects::IssuesController < Projects::ApplicationController
   include IssuesCalendar
   include SpammableActions
-  prepend_before_action :authenticate_user!, only: [:new]
+  prepend_before_action(only: [:index]) { authenticate_sessionless_user!(:rss) }
+  prepend_before_action(only: [:calendar]) { authenticate_sessionless_user!(:ics) }
+  prepend_before_action :authenticate_new_issue!, only: [:new]
+  prepend_before_action :store_uri, only: [:new, :show]
   before_action :whitelist_query_limiting, only: [:create, :create_merge_request, :move, :bulk_update]
   before_action :check_issues_available!
@@ -213,16 +216,18 @@ class Projects::IssuesController < Projects::ApplicationController
     ] + [{ label_ids: [], assignee_ids: [] }]
   end
-  def authenticate_user!
+  def authenticate_new_issue!
     return if current_user
     notice = "Please sign in to create the new issue."
+    redirect_to new_user_session_path, notice: notice
+  end
+
+  def store_uri
     if request.get? && !request.xhr?
       store_location_for :user, request.fullpath
     end
-
-    redirect_to new_user_session_path, notice: notice
   end
   def serializer

app/controllers/projects/tags_controller.rb

 class Projects::TagsController < Projects::ApplicationController
   include SortingHelper
+  prepend_before_action(only: [:index]) { authenticate_sessionless_user!(:rss) }
+
   # Authorize
   before_action :require_non_empty_project
   before_action :authorize_download_code!

app/controllers/projects_controller.rb

@@ -5,6 +5,8 @@ class ProjectsController < Projects::ApplicationController
   include PreviewMarkdown
   include SendFileUpload
+  prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:rss) }
+
   before_action :whitelist_query_limiting, only: [:create]
   before_action :authenticate_user!, except: [:index, :show, :activity, :refs]
   before_action :redirect_git_extension, only: [:show]

app/controllers/users_controller.rb

@@ -12,6 +12,7 @@ class UsersController < ApplicationController
                                 calendar_activities: true
   skip_before_action :authenticate_user!
+  prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:rss) }
   before_action :user, except: [:exists]
   before_action :authorize_read_user_profile!,
                 only: [:calendar, :calendar_activities, :groups, :projects, :contributed_projects, :snippets]

changelogs/unreleased/security-fix-pat-web-access.yml

+---
+title: Restrict Personal Access Tokens to API scope on web requests
+merge_request:
+author:
+type: security

config/initializers/rack_attack_global.rb

@@ -33,22 +33,22 @@ class Rack::Attack
   throttle('throttle_authenticated_api', Gitlab::Throttle.authenticated_api_options) do |req|
     Gitlab::Throttle.settings.throttle_authenticated_api_enabled &&
       req.api_request? &&
-      req.authenticated_user_id
+      req.authenticated_user_id([:api])
   end
   throttle('throttle_authenticated_web', Gitlab::Throttle.authenticated_web_options) do |req|
     Gitlab::Throttle.settings.throttle_authenticated_web_enabled &&
       req.web_request? &&
-      req.authenticated_user_id
+      req.authenticated_user_id([:api, :rss, :ics])
   end
   class Request
     def unauthenticated?
-      !authenticated_user_id
+      !authenticated_user_id([:api, :rss, :ics])
     end
-    def authenticated_user_id
-      Gitlab::Auth::RequestAuthenticator.new(self).user&.id
+    def authenticated_user_id(request_formats)
+      Gitlab::Auth::RequestAuthenticator.new(self).user(request_formats)&.id
     end
     def api_request?

lib/gitlab/auth/request_authenticator.rb

@@ -11,12 +11,18 @@ module Gitlab
         @request = request
       end
-      def user
-        find_sessionless_user || find_user_from_warden
+      def user(request_formats)
+        request_formats.each do |format|
+          user = find_sessionless_user(format)
+
+          return user if user
+        end
+
+        find_user_from_warden
       end
-      def find_sessionless_user
-        find_user_from_access_token || find_user_from_feed_token
+      def find_sessionless_user(request_format)
+        find_user_from_web_access_token(request_format) || find_user_from_feed_token(request_format)
       rescue Gitlab::Auth::AuthenticationError
         nil
       end

lib/gitlab/auth/user_auth_finders.rb

@@ -25,8 +25,8 @@ module Gitlab
         current_request.env['warden']&.authenticate if verified_request?
       end
-      def find_user_from_feed_token
-        return unless rss_request? || ics_request?
+      def find_user_from_feed_token(request_format)
+        return unless valid_rss_format?(request_format)
         # NOTE: feed_token was renamed from rss_token but both needs to be supported because
         #       users might have already added the feed to their RSS reader before the rename
@@ -36,6 +36,17 @@ module Gitlab
         User.find_by_feed_token(token) || raise(UnauthorizedError)
       end
+      # We only allow Private Access Tokens with `api` scope to be used by web
+      # requests on RSS feeds or ICS files for backwards compatibility.
+      # It is also used by GraphQL/API requests.
+      def find_user_from_web_access_token(request_format)
+        return unless access_token && valid_web_access_format?(request_format)
+
+        validate_access_token!(scopes: [:api])
+
+        access_token.user || raise(UnauthorizedError)
+      end
+
       def find_user_from_access_token
         return unless access_token
@@ -107,6 +118,26 @@ module Gitlab
         @current_request ||= ensure_action_dispatch_request(request)
       end
+      def valid_web_access_format?(request_format)
+        case request_format
+        when :rss
+          rss_request?
+        when :ics
+          ics_request?
+        when :api
+          api_request?
+        end
+      end
+
+      def valid_rss_format?(request_format)
+        case request_format
+        when :rss
+          rss_request?
+        when :ics
+          ics_request?
+        end
+      end
+
       def rss_request?
         current_request.path.ends_with?('.atom') || current_request.format.atom?
       end
@@ -114,6 +145,10 @@ module Gitlab
       def ics_request?
         current_request.path.ends_with?('.ics') || current_request.format.ics?
       end
+
+      def api_request?
+        current_request.path.starts_with?("/api/")
+      end
     end
   end
 end

spec/controllers/application_controller_spec.rb

@@ -107,59 +107,6 @@ describe ApplicationController do
     end
   end
-  describe "#authenticate_user_from_personal_access_token!" do
-    before do
-      stub_authentication_activity_metrics(debug: false)
-    end
-
-    controller(described_class) do
-      def index
-        render text: 'authenticated'
-      end
-    end
-
-    let(:personal_access_token) { create(:personal_access_token, user: user) }
-
-    context "when the 'personal_access_token' param is populated with the personal access token" do
-      it "logs the user in" do
-        expect(authentication_metrics)
-          .to increment(:user_authenticated_counter)
-          .and increment(:user_session_override_counter)
-          .and increment(:user_sessionless_authentication_counter)
-
-        get :index, private_token: personal_access_token.token
-
-        expect(response).to have_gitlab_http_status(200)
-        expect(response.body).to eq('authenticated')
-      end
-    end
-
-    context "when the 'PERSONAL_ACCESS_TOKEN' header is populated with the personal access token" do
-      it "logs the user in" do
-        expect(authentication_metrics)
-          .to increment(:user_authenticated_counter)
-          .and increment(:user_session_override_counter)
-          .and increment(:user_sessionless_authentication_counter)
-
-        @request.headers["PRIVATE-TOKEN"] = personal_access_token.token
-        get :index
-
-        expect(response).to have_gitlab_http_status(200)
-        expect(response.body).to eq('authenticated')
-      end
-    end
-
-    it "doesn't log the user in otherwise" do
-      expect(authentication_metrics)
-        .to increment(:user_unauthenticated_counter)
-
-      get :index, private_token: "token"
-
-      expect(response.status).not_to eq(200)
-      expect(response.body).not_to eq('authenticated')
-    end
-  end
-
   describe 'session expiration' do
     controller(described_class) do
       # The anonymous controller will report 401 and fail to run any actions.
@@ -248,74 +195,6 @@ describe ApplicationController do
     end
   end
-  describe '#authenticate_sessionless_user!' do
-    before do
-      stub_authentication_activity_metrics(debug: false)
-    end
-
-    describe 'authenticating a user from a feed token' do
-      controller(described_class) do
-        def index
-          render text: 'authenticated'
-        end
-      end
-
-      context "when the 'feed_token' param is populated with the feed token" do
-        context 'when the request format is atom' do
-          it "logs the user in" do
-            expect(authentication_metrics)
-              .to increment(:user_authenticated_counter)
-              .and increment(:user_session_override_counter)
-              .and increment(:user_sessionless_authentication_counter)
-
-            get :index, feed_token: user.feed_token, format: :atom
-
-            expect(response).to have_gitlab_http_status 200
-            expect(response.body).to eq 'authenticated'
-          end
-        end
-
-        context 'when the request format is ics' do
-          it "logs the user in" do
-            expect(authentication_metrics)
-              .to increment(:user_authenticated_counter)
-              .and increment(:user_session_override_counter)
-              .and increment(:user_sessionless_authentication_counter)
-
-            get :index, feed_token: user.feed_token, format: :ics
-
-            expect(response).to have_gitlab_http_status 200
-            expect(response.body).to eq 'authenticated'
-          end
-        end
-
-        context 'when the request format is neither atom nor ics' do
-          it "doesn't log the user in" do
-            expect(authentication_metrics)
-              .to increment(:user_unauthenticated_counter)
-
-            get :index, feed_token: user.feed_token
-
-            expect(response.status).not_to have_gitlab_http_status 200
-            expect(response.body).not_to eq 'authenticated'
-          end
-        end
-      end
-
-      context "when the 'feed_token' param is populated with an invalid feed token" do
-        it "doesn't log the user" do
-          expect(authentication_metrics)
-            .to increment(:user_unauthenticated_counter)
-
-          get :index, feed_token: 'token', format: :atom
-
-          expect(response.status).not_to eq 200
-          expect(response.body).not_to eq 'authenticated'
-        end
-      end
-    end
-  end
-
   describe '#route_not_found' do
     it 'renders 404 if authenticated' do
       allow(controller).to receive(:current_user).and_return(user)
@@ -581,36 +460,6 @@ describe ApplicationController do
         expect(response).to have_gitlab_http_status(200)
       end
-
-      context 'for sessionless users' do
-        render_views
-
-        before do
-          sign_out user
-        end
-
-        it 'renders a 403 when the sessionless user did not accept the terms' do
-          get :index, feed_token: user.feed_token, format: :atom
-
-          expect(response).to have_gitlab_http_status(403)
-        end
-
-        it 'renders the error message when the format was html' do
-          get :index,
-              private_token: create(:personal_access_token, user: user).token,
-              format: :html
-
-          expect(response.body).to have_content /accept the terms of service/i
-        end
-
-        it 'renders a 200 when the sessionless user accepted the terms' do
-          accept_terms(user)
-
-          get :index, feed_token: user.feed_token, format: :atom
-
-          expect(response).to have_gitlab_http_status(200)
-        end
-      end
     end
   end

spec/controllers/dashboard/projects_controller_spec.rb

+require 'spec_helper'
+
+describe Dashboard::ProjectsController do
+  it_behaves_like 'authenticates sessionless user', :index, :atom
+end

spec/controllers/dashboard_controller_spec.rb

 require 'spec_helper'
 describe DashboardController do
-  let(:user) { create(:user) }
-  let(:project) { create(:project) }
-  before do
-    project.add_maintainer(user)
-    sign_in(user)
-  end
-  describe 'GET issues' do
-    it_behaves_like 'issuables list meta-data', :issue, :issues
-    it_behaves_like 'issuables requiring filter', :issues
-  end
-  describe 'GET merge requests' do
-    it_behaves_like 'issuables list meta-data', :merge_request, :merge_requests
-    it_behaves_like 'issuables requiring filter', :merge_requests
+  context 'signed in' do
+    let(:user) { create(:user) }
+    let(:project) { create(:project) }
+    before do
+      project.add_maintainer(user)
+      sign_in(user)
+    end
+    describe 'GET issues' do
+      it_behaves_like 'issuables list meta-data', :issue, :issues
+      it_behaves_like 'issuables requiring filter', :issues
+    end
+    describe 'GET merge requests' do
+      it_behaves_like 'issuables list meta-data', :merge_request, :merge_requests
+      it_behaves_like 'issuables requiring filter', :merge_requests
+    end
   end
+
+  it_behaves_like 'authenticates sessionless user', :issues, :atom, author_id: User.first
+  it_behaves_like 'authenticates sessionless user', :issues_calendar, :ics
 end

spec/controllers/graphql_controller_spec.rb

@@ -52,15 +52,58 @@ describe GraphqlController do
     end
   end
+  context 'token authentication' do
+    before do
+      stub_authentication_activity_metrics(debug: false)
+    end
+
+    let(:user) { create(:user, username: 'Simon') }
+    let(:personal_access_token) { create(:personal_access_token, user: user) }
+
+    context "when the 'personal_access_token' param is populated with the personal access token" do
+      it 'logs the user in' do
+        expect(authentication_metrics)
+          .to increment(:user_authenticated_counter)
+                .and increment(:user_session_override_counter)
+                       .and increment(:user_sessionless_authentication_counter)
+
+        run_test_query!(private_token: personal_access_token.token)
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(query_response).to eq('echo' => '"Simon" says: test success')
+      end
+    end
+
+    context 'when the personal access token has no api scope' do
+      it 'does not log the user in' do
+        personal_access_token.update(scopes: [:read_user])
+
+        run_test_query!(private_token: personal_access_token.token)
+
+        expect(response).to have_gitlab_http_status(200)
+
+        expect(query_response).to eq('echo' => 'nil says: test success')
+      end
+    end
+
+    context 'without token' do
+      it 'shows public data' do
+        run_test_query!
+
+        expect(query_response).to eq('echo' => 'nil says: test success')
+      end
+    end
+  end
+
   # Chosen to exercise all the moving parts in GraphqlController#execute
-  def run_test_query!(variables: { 'text' => 'test success' })
+  def run_test_query!(variables: { 'text' => 'test success' }, private_token: nil)
     query = <<~QUERY
       query Echo($text: String) {
         echo(text: $text)
       }
     QUERY
-    post :execute, query: query, operationName: 'Echo', variables: variables
+    post :execute, query: query, operationName: 'Echo', variables: variables, private_token: private_token
   end
   def query_response

spec/controllers/groups_controller_spec.rb

@@ -581,4 +581,24 @@ describe GroupsController do
       end
     end
   end
+
+  context 'token authentication' do
+    it_behaves_like 'authenticates sessionless user', :show, :atom, public: true do
+      before do
+        default_params.merge!(id: group)
+      end
+    end
+
+    it_behaves_like 'authenticates sessionless user', :issues, :atom, public: true do
+      before do
+        default_params.merge!(id: group, author_id: user.id)
+      end
+    end
+
+    it_behaves_like 'authenticates sessionless user', :issues_calendar, :ics, public: true do
+      before do
+        default_params.merge!(id: group)
+      end
+    end
+  end
 end