Skip to content

Apply codeowner validations to web requests

Michael Kozono requested to merge mk/apply-codeowner-validations-to-web-requests into master

Background

Originally, the security vulnerability was closed in master with https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/346.

However, at least one large customer on GitLab.com uses CODEOWNERS that references ancestor groups of the project. When the above MR was deployed, the customer's workflow became blocked. We did not realize that this was "expected" due to the unresolved #32432 (closed). So, we reverted the security fix to unblock the customer.

We attempted to fix the CODEOWNERS logic https://gitlab.com/gitlab-org/gitlab/-/issues/216345#note_336154279, however we mistakenly only fixed it for the case where the project's parent group was referenced in CODEOWNERS. So after rereverting the security fix, the customer was blocked again.

The security fix and the CODEOWNERS change were reverted.

There were still unanswered questions regarding how the CODEOWNERS logic should work in these cases. It's being worked on, but it cannot be rushed.

Update: !31152 (merged) was merged. We expect it to fix the majority of customer cases where CODEOWNERS was unexpectedly blocking Merge via the UI.

What does this MR do?

This MR reapplies the original security fix but adds a feature flag to let you skip this fix for specific projects, or for all. This helps us rollout the fix carefully for known previously affected customers, and allows self-hosted customers to skip validations on the Merge button on the web UI if they really want to.

The additional feature flag logic sounds trivial but it is easy to make a mistake. Reviewers, please scrutinize it. Unit tests of the affected method are now tested with the feature flag on and off.

  • We can skip (or not skip) these validations for everyone, at a moment's notice
  • We can skip these validations for specific projects, if customers find a new edge case of CODEOWNERS blocking them, and workarounds are not feasible

Rollout plan

https://gitlab.com/gitlab-org/gitlab/-/issues/216345#note_340599018:

The feature flag is now skip_web_ui_code_owner_validations to make it easy to skip it for specific customers if needed.

Rollout plan:

  • Enable skip_web_ui_code_owner_validations feature flag on GitLab.com for all => https://gitlab.slack.com/archives/CK75EF2A2/p1589258447358900
  • Merge/deploy this MR (to no effect)
  • Disable skip_web_ui_code_owner_validations feature flag on GitLab.com for all
  • Enable skip_web_ui_code_owner_validations feature flag on GitLab.com for known previously affected customers (because we want extreme care for these cases at first) (would it work to do this before the previous step?)
  • Validate CODEOWNERS logic + enforcement in web UI on GitLab.com
  • Wait ~2 days
  • Search for issues
  • Disable skip_web_ui_code_owner_validations feature flag on GitLab.com for all
  • Remove feature flag #217427 (closed)

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by 🤖 GitLab Bot 🤖

Merge request reports