Merge remote-tracking branch 'dev/13-11-stable' into 13-11-stable

CHANGELOG.md

@@ -2,6 +2,17 @@
 documentation](doc/development/changelog.md) for instructions on adding your own
 entry.
 
+## 13.11.2 (2021-04-27)
+
+### Security (5 changes)
+
+- Prevent tokens with only read_api scope from executing mutations.
+- Do not allow deploy tokens in the dependency proxy authentication service.
+- Disable keyset pagination for branches by default.
+- Bump Carrierwave gem to v1.3.2.
+- Restrict setting system_note_timestamp to owners.
+
+
 ## 13.11.1 (2021-04-22)
 
 ### Changed (1 change)

GITALY_SERVER_VERSION

-13.11.1
+13.11.2
\ No newline at end of file

Gemfile.lock

@@ -171,10 +171,11 @@ GEM
     capybara-screenshot (1.0.22)
       capybara (>= 1.0, < 4)
       launchy
-    carrierwave (1.3.1)
+    carrierwave (1.3.2)
       activemodel (>= 4.0.0)
       activesupport (>= 4.0.0)
       mime-types (>= 1.16)
+      ssrf_filter (~> 1.0)
     cbor (0.5.9.6)
     character_set (1.4.0)
     charlock_holmes (0.7.7)
@@ -1205,6 +1206,7 @@ GEM
       sprockets (>= 3.0.0)
     sqlite3 (1.3.13)
     sshkey (2.0.0)
+    ssrf_filter (1.0.7)
     stackprof (0.2.15)
     state_machines (0.5.0)
     state_machines-activemodel (0.8.0)

VERSION

-13.11.1
+13.11.2
\ No newline at end of file

app/controllers/concerns/sessionless_authentication.rb

@@ -7,11 +7,15 @@
 module SessionlessAuthentication
   # This filter handles personal access tokens, atom requests with rss tokens, and static object tokens
   def authenticate_sessionless_user!(request_format)
-    user = Gitlab::Auth::RequestAuthenticator.new(request).find_sessionless_user(request_format)
+    user = request_authenticator.find_sessionless_user(request_format)
 
     sessionless_sign_in(user) if user
   end
 
+  def request_authenticator
+    @request_authenticator ||= Gitlab::Auth::RequestAuthenticator.new(request)
+  end
+
   def sessionless_user?
     current_user && !session.key?('warden.user.user.key')
   end

app/controllers/graphql_controller.rb

@@ -110,7 +110,13 @@ class GraphqlController < ApplicationController
   end
 
   def context
-    @context ||= { current_user: current_user, is_sessionless_user: !!sessionless_user?, request: request }
+    api_user = !!sessionless_user?
+    @context ||= {
+      current_user: current_user,
+      is_sessionless_user: api_user,
+      request: request,
+      scope_validator: ::Gitlab::Auth::ScopeValidator.new(api_user, request_authenticator)
+    }
   end
 
   def build_variables(variable_info)

app/controllers/projects/branches_controller.rb

@@ -185,7 +185,7 @@ class Projects::BranchesController < Projects::ApplicationController
     # Here we get one more branch to indicate if there are more data we're not showing
     limit = @overview_max_branches + 1
 
-    if Feature.enabled?(:branch_list_keyset_pagination, project, default_enabled: true)
+    if Feature.enabled?(:branch_list_keyset_pagination, project, default_enabled: :yaml)
       @active_branches =
         BranchesFinder.new(@repository, { per_page: limit, sort: sort_value_recently_updated })
           .execute(gitaly_pagination: true).select(&:active?)

app/graphql/mutations/base_mutation.rb

@@ -44,9 +44,18 @@ module Mutations
       end
     end
 
+    def self.authorizes_object?
+      true
+    end
+
     def self.authorized?(object, context)
-      # we never provide an object to mutations, but we do need to have a user.
-      context[:current_user].present? && !context[:current_user].blocked?
+      auth = ::Gitlab::Graphql::Authorize::ObjectAuthorization.new(:execute_graphql_mutation, :api)
+
+      return true if auth.ok?(:global, context[:current_user],
+                              scope_validator: context[:scope_validator])
+
+      # in our mutations we raise, rather than returning a null value.
+      raise_resource_not_available_error!
     end
 
     # See: AuthorizeResource#authorized_resource?

app/policies/global_policy.rb

@@ -23,6 +23,7 @@ class GlobalPolicy < BasePolicy
     prevent :receive_notifications
     prevent :use_quick_actions
     prevent :create_group
+    prevent :execute_graphql_mutation
   end
 
   rule { default }.policy do
@@ -32,6 +33,7 @@ class GlobalPolicy < BasePolicy
     enable :receive_notifications
     enable :use_quick_actions
     enable :use_slash_commands
+    enable :execute_graphql_mutation
   end
 
   rule { inactive }.policy do
@@ -48,6 +50,8 @@ class GlobalPolicy < BasePolicy
     prevent :use_slash_commands
   end
 
+  rule { ~can?(:access_api) }.prevent :execute_graphql_mutation
+
   rule { blocked | (internal & ~migration_bot & ~security_bot) }.policy do
     prevent :access_git
   end

app/services/auth/dependency_proxy_authentication_service.rb

@@ -8,7 +8,10 @@ module Auth
 
     def execute(authentication_abilities:)
       return error('dependency proxy not enabled', 404) unless ::Gitlab.config.dependency_proxy.enabled
-      return error('access forbidden', 403) unless current_user
+
+      # Because app/controllers/concerns/dependency_proxy/auth.rb consumes this
+      # JWT only as `User.find`, we currently only allow User (not DeployToken, etc)
+      return error('access forbidden', 403) unless current_user.is_a?(User)
 
       { token: authorized_token.encoded }
     end

app/services/issues/base_service.rb

@@ -34,7 +34,7 @@ module Issues
 
     private
 
-    def filter_params(merge_request)
+    def filter_params(issue)
       super
 
       moved_issue = params.delete(:moved_issue)
@@ -44,6 +44,8 @@ module Issues
       params.delete(:iid) unless current_user.can?(:set_issue_iid, project)
       params.delete(:created_at) unless moved_issue || current_user.can?(:set_issue_created_at, project)
       params.delete(:updated_at) unless moved_issue || current_user.can?(:set_issue_updated_at, project)
+
+      issue.system_note_timestamp = params[:created_at] || params[:updated_at]
     end
 
     def create_assignee_note(issue, old_assignees)

app/services/projects/branches_by_mode_service.rb

@@ -37,7 +37,7 @@ class Projects::BranchesByModeService
   def use_gitaly_pagination?
     return false if params[:page].present? || params[:search].present?
 
-    Feature.enabled?(:branch_list_keyset_pagination, project, default_enabled: true)
+    Feature.enabled?(:branch_list_keyset_pagination, project, default_enabled: :yaml)
   end
 
   def fetch_branches_via_offset_pagination

app/views/projects/mirrors/_authentication_method.html.haml

@@ -13,4 +13,4 @@
 .form-group
   .well-password-auth.collapse.js-well-password-auth
     = f.label :password, _("Password"), class: "label-bold"
-    = f.password_field :password, value: mirror.password, class: 'form-control gl-form-input qa-password', autocomplete: 'new-password'
+    = f.password_field :password, class: 'form-control gl-form-input qa-password', autocomplete: 'new-password'

config/feature_flags/development/branch_list_keyset_pagination.yml

@@ -5,4 +5,4 @@ rollout_issue_url:
 milestone: '13.2'
 type: development
 group: group::source code
-default_enabled: true
+default_enabled: false

lib/api/issues.rb

@@ -249,7 +249,6 @@ module API
         authorize! :create_issue, user_project
 
         issue_params = declared_params(include_missing: false)
-        issue_params[:system_note_timestamp] = params[:created_at]
 
         issue_params = convert_parameters_from_legacy_format(issue_params)
 
@@ -293,8 +292,6 @@ module API
         issue = user_project.issues.find_by!(iid: params.delete(:issue_iid))
         authorize! :update_issue, issue
 
-        issue.system_note_timestamp = params[:updated_at]
-
         update_params = declared_params(include_missing: false).merge(request: request, api: true)
 
         update_params = convert_parameters_from_legacy_format(update_params)

lib/gitlab/auth/scope_validator.rb

+# frozen_string_literal: true
+
+# Wrapper around a RequestAuthenticator to
+# perform authorization of scopes. Access is limited to
+# only those methods needed to validate that an API user
+# has at least one permitted scope.
+module Gitlab
+  module Auth
+    class ScopeValidator
+      def initialize(api_user, request_authenticator)
+        @api_user = api_user
+        @request_authenticator = request_authenticator
+      end
+
+      def valid_for?(permitted)
+        return true unless @api_user
+        return true if permitted.none?
+
+        scopes = permitted.map { |s| API::Scope.new(s) }
+        @request_authenticator.valid_access_token?(scopes: scopes)
+      end
+    end
+  end
+end

lib/gitlab/graphql/authorize/object_authorization.rb

@@ -4,10 +4,11 @@ module Gitlab
   module Graphql
     module Authorize
       class ObjectAuthorization
-        attr_reader :abilities
+        attr_reader :abilities, :permitted_scopes
 
-        def initialize(abilities)
+        def initialize(abilities, scopes = %i[api read_api])
           @abilities = Array.wrap(abilities).flatten
+          @permitted_scopes = Array.wrap(scopes)
         end
 
         def none?
@@ -18,7 +19,13 @@ module Gitlab
           abilities.present?
         end
 
-        def ok?(object, current_user)
+        def ok?(object, current_user, scope_validator: nil)
+          scopes_ok?(scope_validator) && abilities_ok?(object, current_user)
+        end
+
+        private
+
+        def abilities_ok?(object, current_user)
           return true if none?
 
           subject = object.try(:declarative_policy_subject) || object
@@ -26,6 +33,12 @@ module Gitlab
             Ability.allowed?(current_user, ability, subject)
           end
         end
+
+        def scopes_ok?(validator)
+          return true unless validator.present?
+
+          validator.valid_for?(permitted_scopes)
+        end
       end
     end
   end

lib/gitlab/pagination/gitaly_keyset_pager.rb

@@ -26,11 +26,11 @@ module Gitlab
       private
 
       def keyset_pagination_enabled?
-        Feature.enabled?(:branch_list_keyset_pagination, project, default_enabled: true) && params[:pagination] == 'keyset'
+        Feature.enabled?(:branch_list_keyset_pagination, project, default_enabled: :yaml) && params[:pagination] == 'keyset'
       end
 
       def paginate_first_page?
-        Feature.enabled?(:branch_list_keyset_pagination, project, default_enabled: true) && (params[:page].blank? || params[:page].to_i == 1)
+        Feature.enabled?(:branch_list_keyset_pagination, project, default_enabled: :yaml) && (params[:page].blank? || params[:page].to_i == 1)
       end
 
       def paginate_via_gitaly(finder)

spec/requests/api/graphql/mutations/notes/create/note_spec.rb

@@ -31,6 +31,8 @@ RSpec.describe 'Adding a Note' do
       project.add_developer(current_user)
     end
 
+    it_behaves_like 'a working GraphQL mutation'
+
     it_behaves_like 'a Note mutation that creates a Note'
 
     it_behaves_like 'a Note mutation when there are active record validation errors'

spec/requests/api/issues/issues_spec.rb

@@ -943,6 +943,34 @@ RSpec.describe API::Issues do
     it_behaves_like 'issuable update endpoint' do
       let(:entity) { issue }
     end
+
+    describe 'updated_at param' do
+      let(:fixed_time) { Time.new(2001, 1, 1) }
+      let(:updated_at) { Time.new(2000, 1, 1) }
+
+      before do
+        travel_to fixed_time
+      end
+
+      it 'allows admins to set the timestamp' do
+        put api("/projects/#{project.id}/issues/#{issue.iid}", admin), params: { labels: 'label1', updated_at: updated_at }
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(Time.parse(json_response['updated_at'])).to be_like_time(updated_at)
+        expect(ResourceLabelEvent.last.created_at).to be_like_time(updated_at)
+      end
+
+      it 'does not allow other users to set the timestamp' do
+        reporter = create(:user)
+        project.add_developer(reporter)
+
+        put api("/projects/#{project.id}/issues/#{issue.iid}", reporter), params: { labels: 'label1', updated_at: updated_at }
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(Time.parse(json_response['updated_at'])).to be_like_time(fixed_time)
+        expect(ResourceLabelEvent.last.created_at).to be_like_time(fixed_time)
+      end
+    end
   end
 
   describe 'DELETE /projects/:id/issues/:issue_iid' do

spec/requests/api/issues/post_projects_issues_spec.rb

@@ -330,15 +330,21 @@ RSpec.describe API::Issues do
     end
 
     context 'setting created_at' do
+      let(:fixed_time) { Time.new(2001, 1, 1) }
       let(:creation_time) { 2.weeks.ago }
       let(:params) { { title: 'new issue', labels: 'label, label2', created_at: creation_time } }
 
+      before do
+        travel_to fixed_time
+      end
+
       context 'by an admin' do
         it 'sets the creation time on the new issue' do
           post api("/projects/#{project.id}/issues", admin), params: params
 
           expect(response).to have_gitlab_http_status(:created)
           expect(Time.parse(json_response['created_at'])).to be_like_time(creation_time)
+          expect(ResourceLabelEvent.last.created_at).to be_like_time(creation_time)
         end
       end
 
@@ -348,6 +354,7 @@ RSpec.describe API::Issues do
 
           expect(response).to have_gitlab_http_status(:created)
           expect(Time.parse(json_response['created_at'])).to be_like_time(creation_time)
+          expect(ResourceLabelEvent.last.created_at).to be_like_time(creation_time)
         end
       end
 
@@ -356,19 +363,24 @@ RSpec.describe API::Issues do
           group = create(:group)
           group_project = create(:project, :public, namespace: group)
           group.add_owner(user2)
+
           post api("/projects/#{group_project.id}/issues", user2), params: params
 
           expect(response).to have_gitlab_http_status(:created)
           expect(Time.parse(json_response['created_at'])).to be_like_time(creation_time)
+          expect(ResourceLabelEvent.last.created_at).to be_like_time(creation_time)
         end
       end
 
       context 'by another user' do
         it 'ignores the given creation time' do
+          project.add_developer(user2)
+
           post api("/projects/#{project.id}/issues", user2), params: params
 
           expect(response).to have_gitlab_http_status(:created)
-          expect(Time.parse(json_response['created_at'])).not_to be_like_time(creation_time)
+          expect(Time.parse(json_response['created_at'])).to be_like_time(fixed_time)
+          expect(ResourceLabelEvent.last.created_at).to be_like_time(fixed_time)
         end
       end
     end

spec/requests/jwt_controller_spec.rb

@@ -263,25 +263,21 @@ RSpec.describe JwtController do
       let(:credential_user) { group_deploy_token.username }
       let(:credential_password) { group_deploy_token.token }
 
-      it_behaves_like 'with valid credentials'
+      it_behaves_like 'returning response status', :forbidden
     end
 
     context 'with project deploy token' do
       let(:credential_user) { project_deploy_token.username }
       let(:credential_password) { project_deploy_token.token }
 
-      it_behaves_like 'with valid credentials'
+      it_behaves_like 'returning response status', :forbidden
     end
 
     context 'with invalid credentials' do
       let(:credential_user) { 'foo' }
       let(:credential_password) { 'bar' }
 
-      it 'returns unauthorized' do
-        subject
-
-        expect(response).to have_gitlab_http_status(:unauthorized)
-      end
+      it_behaves_like 'returning response status', :unauthorized
     end
   end
 

spec/services/auth/dependency_proxy_authentication_service_spec.rb

@@ -13,28 +13,31 @@ RSpec.describe Auth::DependencyProxyAuthenticationService do
   describe '#execute' do
     subject { service.execute(authentication_abilities: nil) }
 
+    shared_examples 'returning' do |status:, message:|
+      it "returns #{message}", :aggregate_failures do
+        expect(subject[:http_status]).to eq(status)
+        expect(subject[:message]).to eq(message)
+      end
+    end
+
     context 'dependency proxy is not enabled' do
       before do
         stub_config(dependency_proxy: { enabled: false })
       end
 
-      it 'returns not found' do
-        result = subject
-
-        expect(result[:http_status]).to eq(404)
-        expect(result[:message]).to eq('dependency proxy not enabled')
-      end
+      it_behaves_like 'returning', status: 404, message: 'dependency proxy not enabled'
     end
 
     context 'without a user' do
       let(:user) { nil }
 
-      it 'returns forbidden' do
-        result = subject
+      it_behaves_like 'returning', status: 403, message: 'access forbidden'
+    end
+
+    context 'with a deploy token as user' do
+      let_it_be(:user) { create(:deploy_token) }
 
-        expect(result[:http_status]).to eq(403)
-        expect(result[:message]).to eq('access forbidden')
-      end
+      it_behaves_like 'returning', status: 403, message: 'access forbidden'
     end
 
     context 'with a user' do

spec/services/projects/download_service_spec.rb

@@ -20,8 +20,9 @@ RSpec.describe Projects::DownloadService do
 
     context 'for URLs that are on the whitelist' do
       before do
-        stub_request(:get, 'http://mycompany.fogbugz.com/rails_sample.jpg').to_return(body: File.read(Rails.root + 'spec/fixtures/rails_sample.jpg'))
-        stub_request(:get, 'http://mycompany.fogbugz.com/doc_sample.txt').to_return(body: File.read(Rails.root + 'spec/fixtures/doc_sample.txt'))
+        # `ssrf_filter` resolves the hostname. See https://github.com/carrierwaveuploader/carrierwave/commit/91714adda998bc9e8decf5b1f5d260d808761304
+        stub_request(:get, %r{http://[\d\.]+/rails_sample.jpg}).to_return(body: File.read(Rails.root + 'spec/fixtures/rails_sample.jpg'))
+        stub_request(:get, %r{http://[\d\.]+/doc_sample.txt}).to_return(body: File.read(Rails.root + 'spec/fixtures/doc_sample.txt'))
       end
 
       context 'an image file' do

spec/support/helpers/graphql_helpers.rb

@@ -396,17 +396,21 @@ module GraphqlHelpers
     post api('/', current_user, version: 'graphql'), params: { _json: queries }, headers: headers
   end
 
-  def post_graphql(query, current_user: nil, variables: nil, headers: {})
+  def post_graphql(query, current_user: nil, variables: nil, headers: {}, token: {})
     params = { query: query, variables: serialize_variables(variables) }
-    post api('/', current_user, version: 'graphql'), params: params, headers: headers
+    post api('/', current_user, version: 'graphql', **token), params: params, headers: headers
 
-    if graphql_errors # Errors are acceptable, but not this one:
-      expect(graphql_errors).not_to include(a_hash_including('message' => 'Internal server error'))
-    end
+    return unless graphql_errors
+
+    # Errors are acceptable, but not this one:
+    expect(graphql_errors).not_to include(a_hash_including('message' => 'Internal server error'))
   end
 
-  def post_graphql_mutation(mutation, current_user: nil)
-    post_graphql(mutation.query, current_user: current_user, variables: mutation.variables)
+  def post_graphql_mutation(mutation, current_user: nil, token: {})
+    post_graphql(mutation.query,
+                 current_user: current_user,
+                 variables: mutation.variables,
+                 token: token)
   end
 
   def post_graphql_mutation_with_uploads(mutation, current_user: nil)

spec/support/shared_examples/requests/graphql_shared_examples.rb

@@ -10,6 +10,52 @@ RSpec.shared_examples 'a working graphql query' do
   end
 end
 
+RSpec.shared_examples 'a working GraphQL mutation' do
+  include GraphqlHelpers
+
+  before do
+    post_graphql_mutation(mutation, current_user: current_user, token: token)
+  end
+
+  shared_examples 'allows access to the mutation' do
+    let(:scopes) { ['api'] }
+
+    it_behaves_like 'a working graphql query' do
+      it 'returns data' do
+        expect(graphql_data.compact).not_to be_empty
+      end
+    end
+  end
+
+  shared_examples 'prevents access to the mutation' do
+    let(:scopes) { ['read_api'] }
+
+    it 'does not resolve the mutation' do
+      expect(graphql_data.compact).to be_empty
+      expect(graphql_errors).to be_present
+    end
+  end
+
+  context 'with a personal access token' do
+    let(:token) do
+      pat = create(:personal_access_token, user: current_user, scopes: scopes)
+      { personal_access_token: pat }
+    end
+
+    it_behaves_like 'prevents access to the mutation'
+    it_behaves_like 'allows access to the mutation'
+  end
+
+  context 'with an OAuth token' do
+    let(:token) do
+      { oauth_access_token: create(:oauth_access_token, resource_owner: current_user, scopes: scopes.join(' ')) }
+    end
+
+    it_behaves_like 'prevents access to the mutation'
+    it_behaves_like 'allows access to the mutation'
+  end
+end
+
 RSpec.shared_examples 'a mutation on an unauthorized resource' do
   it_behaves_like 'a mutation that returns top-level errors',
                       errors: [::Gitlab::Graphql::Authorize::AuthorizeResource::RESOURCE_ACCESS_ERROR]