ESCALATED: Forks with open merge requests to each other can throw GitLab into an endless permission check loop

Summary

Forks of projects with internal visibility with open merge requests

  1. allowing commits from members who can merge to the target branch
  2. targeting each other with the same branch name

result in a code loop when we check permissions for users who are not members of either projects.

This also results in the project's home page being unavailable.

This was reported (Zendesk, internal use only) by a 1,500-seat premium customer.

Steps to reproduce

First, create a project original/project-a with visibility level internal and initialize repo with README.

In original/project-a:

  1. Edit README.md to add a line and commit the change to a new branch test-branch.
  2. Create a fork the project (We'll refer to this as fork/project-a going forward).

In fork/project-a:

  1. Edit README.md in test-branch to add a line and commit to the same branch.
  2. Create an MR with source test-branch and target original/project-a:test-branch. Check option Allow commits from members who can merge to the target branch.

In original/project-a:

  1. Create an MR with source test-branch and target fork/project-a:test-branch. Check option Allow commits from members who can merge to the target branch.

Now, log in as a user that is not a member of either project and browse to those project pages. You get a 500 error.

Example Project

N/A

What is the current bug behavior?

(What actually happens)

What is the expected correct behavior?

(What you should see instead)

Relevant logs and/or screenshots

Here's an annotated and abridged stack trace from when I attempt to open https://gitlab.example.com/original/project-a. My apologies in advance for the dank memes, I've spent several hours figuring this one out and needed some fun.

app/views/projects/_files.html.haml:10:in `_app_views_projects__files_html_haml___1234566409494462343_69929741437180'
app/views/projects/show.html.haml:33:in `_app_views_projects_show_html_haml___3330509772191764956_69929735763260'
app/views/projects/tree/_tree_header.html.haml:1:in `_app_views_projects_tree__tree_header_html_haml__3572462774810798508_69929742106660'
app/controllers/concerns/checks_collaboration.rb:12:in `can_collaborate_with_project?'

All good so far. This is the entry point of the loop.

lib/gitlab/user_access.rb:70:in `can_push_to_branch?'
app/models/project.rb:2179:in `branch_allows_collaboration?'
app/models/project.rb:2412:in `block (3 levels) in fetch_branch_allows_collaboration'

Here, we do:

        merge_requests_allowing_collaboration(branch_name).any? do |merge_request|
          merge_request.can_be_merged_by?(user)
        end

Note: At this point, branch_name is nil. And for reference, from app/models/project.rb:

  has_many :source_of_merge_requests, foreign_key: 'source_project_id', class_name: 'MergeRequest'
  def merge_requests_allowing_collaboration(source_branch = nil)
    relation = source_of_merge_requests.opened.where(allow_collaboration: true)
    relation = relation.where(source_branch: source_branch) if source_branch
    relation
  end

Because we have an open merge request from fork/project-a, from here on, we're operating in the context of a merge_request object from fork/project-a.

app/models/merge_request.rb:1078:in `can_be_merged_by?'
  def can_be_merged_by?(user)
    access = ::Gitlab::UserAccess.new(user, project: project)
    access.can_update_branch?(target_branch)
  end

Uh oh. We just passed the merge_request's project to Gitlab::UserAccess, meaning we just switched contexts from the original project to the forked project and target_branch as passed is test-branch.

lib/gitlab/user_access.rb:63:in `can_update_branch?'
lib/gitlab/user_access.rb:70:in `can_push_to_branch?'

Here's what we are doing in lib/gitlab/user_access.rb:

    request_cache def can_push_to_branch?(ref)
      return false unless can_access_git?
      return false unless project

      return false if !user.can?(:push_code, project) && !project.branch_allows_collaboration?(user, ref) ### This

      if protected?(ProtectedBranch, project, ref)
        protected_branch_accessible_to?(ref, action: :push)
      else
        true
      end
    end

Now we're checking test-branch on fork/project-a.

app/models/project.rb:2179:in `branch_allows_collaboration?'
app/models/project.rb:2406:in `fetch_branch_allows_collaboration'
app/models/project.rb:2412:in `block (3 levels) in fetch_branch_allows_collaboration'

This should look familiar! We're back here again:

        merge_requests_allowing_collaboration(branch_name).any? do |merge_request|
          merge_request.can_be_merged_by?(user)
        end

This time, we're operating in the context of fork/project-a and branch_name is now test-branch. And since we have an open merge request from original/project-a also targeting test-branch, now we're going to run #can_be_merged_by? on that.

app/models/merge_request.rb:1078:in `can_be_merged_by?'
  def can_be_merged_by?(user)
    access = ::Gitlab::UserAccess.new(user, project: project)
    access.can_update_branch?(target_branch)
  end

And now we're back to another iteration, this time on original/project-a.

We get bounced between fork/project-a and original/project-a endlessly, until Ruby decides it has had enough.

Output of checks

This happens in v12.4.2-ee. GitLab.com's codebase should be affected as well, but we don't have Internal visibility enabled here.

Possible fixes

Relevant lines of code:

https://gitlab.com/gitlab-org/gitlab/blob/53b17f030161ba2afade8fe3d41b849a7fa41a89/lib/gitlab/user_access.rb#L70 https://gitlab.com/gitlab-org/gitlab/blob/f037740742556bc816d0d33e9019e7f9031972f8/app/models/project.rb#L2408 https://gitlab.com/gitlab-org/gitlab/blob/e404c31152cb6ce7acc82969ba103505912946ce/app/models/merge_request.rb#L1070

Edited by GitLab SecurityBot