Commit 4d73ef61 authored by Kerri Miller's avatar Kerri Miller

Apply CODEOWNER validations to web requests

We had previously not applied these to web requests, thinking them to be
redundant. However, this poses a potential security issue, so we need to
enable them.

Format error msg for web requests

We'll eventually clean this messaging up for the web, but the simplest
thing we can do is remove the \n characters so that the API can send
(and the front end display) the error message legibly; the \n were
causing us to drop everything after the first line. It's ugly, but it is
approved language and is FAR more helpful for a user to be able to
resolve the issue.
parent 0a49a02f
---
title: Apply CODEOWNERS validations to web requests
merge_request:
author:
type: security
...@@ -12,7 +12,7 @@ module EE ...@@ -12,7 +12,7 @@ module EE
def path_validations def path_validations
validations = [super].flatten validations = [super].flatten
if !updated_from_web? && project.branch_requires_code_owner_approval?(branch_name) if project.branch_requires_code_owner_approval?(branch_name)
validations << validate_code_owners validations << validate_code_owners
end end
...@@ -34,11 +34,13 @@ module EE ...@@ -34,11 +34,13 @@ module EE
matched_rules = loader.entries.collect { |e| "- #{e.pattern}" } matched_rules = loader.entries.collect { |e| "- #{e.pattern}" }
code_owner_path = project.repository.code_owners_blob(ref: branch_name).path || "CODEOWNERS" code_owner_path = project.repository.code_owners_blob(ref: branch_name).path || "CODEOWNERS"
"Pushes to protected branches that contain changes to files that\n" \ msg = "Pushes to protected branches that contain changes to files that\n" \
"match patterns defined in `#{code_owner_path}` are disabled for\n" \ "match patterns defined in `#{code_owner_path}` are disabled for\n" \
"this project. Please submit these changes via a merge request.\n\n" \ "this project. Please submit these changes via a merge request.\n\n" \
"The following pattern(s) from `#{code_owner_path}` were matched:\n" \ "The following pattern(s) from `#{code_owner_path}` were matched:\n" \
"#{matched_rules.join('\n')}\n" "#{matched_rules.join('\n')}\n"
updated_from_web? ? msg.tr("\n", " ") : msg
end end
def validate_path_locks? def validate_path_locks?
......
...@@ -89,8 +89,22 @@ describe Gitlab::Checks::DiffCheck do ...@@ -89,8 +89,22 @@ describe Gitlab::Checks::DiffCheck do
stub_feature_flags(sectional_codeowners: false) stub_feature_flags(sectional_codeowners: false)
end end
context "for a non-web-based request" do
it "returns an error message" do it "returns an error message" do
expect(validation_result).to include("Pushes to protected branches") expect(validation_result).to include("Pushes to protected branches")
expect(validation_result).to include("\n")
end
end
context "for a web-based request" do
before do
expect(subject).to receive(:updated_from_web?).and_return(true)
end
it "returns an error message with newline chars removed" do
expect(validation_result).to include("Pushes to protected branches")
expect(validation_result).not_to include("\n")
end
end end
end end
...@@ -128,9 +142,7 @@ describe Gitlab::Checks::DiffCheck do ...@@ -128,9 +142,7 @@ describe Gitlab::Checks::DiffCheck do
end end
context "when the feature is enabled on the project" do context "when the feature is enabled on the project" do
context "updated_from_web? == false" do
before do before do
expect(subject).to receive(:updated_from_web?).and_return(false)
expect(project).to receive(:branch_requires_code_owner_approval?) expect(project).to receive(:branch_requires_code_owner_approval?)
.once.and_return(true) .once.and_return(true)
end end
...@@ -142,18 +154,6 @@ describe Gitlab::Checks::DiffCheck do ...@@ -142,18 +154,6 @@ describe Gitlab::Checks::DiffCheck do
expect(validations.any? { |v| !v.is_a? Proc }).to be_falsy expect(validations.any? { |v| !v.is_a? Proc }).to be_falsy
end end
end end
context "updated_from_web? == true" do
before do
expect(subject).to receive(:updated_from_web?).and_return(true)
expect(project).not_to receive(:branch_requires_code_owner_approval?)
end
it "returns an empty array" do
expect(subject.send(:path_validations)).to eq([])
end
end
end
end end
context 'file name rules' do context 'file name rules' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment