summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSean McGivern <sean@gitlab.com>2019-04-04 15:00:56 +0000
committerSean McGivern <sean@gitlab.com>2019-04-04 15:00:56 +0000
commit7dcc3003119666c75a35c27d73ffb297c696fcc8 (patch)
tree54b714d8a8f18f6e6f0f8f5da56fae5203f002e8
parent7926384ff32b9ad8833dcfffc9bb87d036c4bd21 (diff)
parent8a833c720e91c7b4d764e85c30e3be18ee5221fd (diff)
downloadgitlab-ce-7dcc3003119666c75a35c27d73ffb297c696fcc8.tar.gz
Merge branch 'allow-to-use-untrusted-ruby-syntax' into 'master'
Allow to use untrusted ruby syntax See merge request gitlab-org/gitlab-ce!26905
-rw-r--r--changelogs/unreleased/allow-to-use-untrusted-ruby-syntax.yml5
-rw-r--r--doc/ci/yaml/README.md21
-rw-r--r--lib/gitlab/ci/build/policy/refs.rb2
-rw-r--r--lib/gitlab/ci/config/entry/policy.rb4
-rw-r--r--lib/gitlab/config/entry/validators.rb16
-rw-r--r--lib/gitlab/untrusted_regexp/ruby_syntax.rb39
-rw-r--r--spec/lib/gitlab/ci/build/policy/refs_spec.rb26
-rw-r--r--spec/lib/gitlab/ci/config/entry/policy_spec.rb67
-rw-r--r--spec/lib/gitlab/untrusted_regexp/ruby_syntax_spec.rb68
9 files changed, 223 insertions, 25 deletions
diff --git a/changelogs/unreleased/allow-to-use-untrusted-ruby-syntax.yml b/changelogs/unreleased/allow-to-use-untrusted-ruby-syntax.yml
new file mode 100644
index 00000000000..731c9c10b00
--- /dev/null
+++ b/changelogs/unreleased/allow-to-use-untrusted-ruby-syntax.yml
@@ -0,0 +1,5 @@
+---
+title: Allow to use untrusted Regexp via feature flag
+merge_request: 26905
+author:
+type: deprecated
diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md
index e75f7050a09..686d36c50ee 100644
--- a/doc/ci/yaml/README.md
+++ b/doc/ci/yaml/README.md
@@ -414,6 +414,27 @@ job:
only: ['branches', 'tags']
```
+### Supported `only`/`except` regexp syntax
+
+CAUTION: **Warning:**
+This is a breaking change that was introduced with GitLab 11.9.4.
+
+In GitLab 11.9.4, GitLab begun internally converting regexp used
+in `only` and `except` parameters to [RE2](https://github.com/google/re2/wiki/Syntax).
+
+This means that only subset of features provided by [Ruby Regexp](https://ruby-doc.org/core/Regexp.html)
+is supported. [RE2](https://github.com/google/re2/wiki/Syntax) limits the set of features
+provided due to computational complexity, which means some features became unavailable in GitLab 11.9.4.
+For example, negative lookaheads.
+
+For GitLab versions from 11.9.7 and up to GitLab 12.0, GitLab provides a feature flag that can be
+enabled by administrators that allows users to use unsafe regexp syntax. This brings compatibility
+with previously allowed syntax version and allows users to gracefully migrate to the new syntax.
+
+```ruby
+Feature.enable(:allow_unsafe_ruby_regexp)
+```
+
### `only`/`except` (advanced)
> - `refs` and `kubernetes` policies introduced in GitLab 10.0.
diff --git a/lib/gitlab/ci/build/policy/refs.rb b/lib/gitlab/ci/build/policy/refs.rb
index 360424bec11..c3005303fd8 100644
--- a/lib/gitlab/ci/build/policy/refs.rb
+++ b/lib/gitlab/ci/build/policy/refs.rb
@@ -35,7 +35,7 @@ module Gitlab
# patterns can be matched only when branch or tag is used
# the pattern matching does not work for merge requests pipelines
if pipeline.branch? || pipeline.tag?
- if regexp = Gitlab::UntrustedRegexp::RubySyntax.fabricate(pattern)
+ if regexp = Gitlab::UntrustedRegexp::RubySyntax.fabricate(pattern, fallback: true)
regexp.match?(pipeline.ref)
else
pattern == pipeline.ref
diff --git a/lib/gitlab/ci/config/entry/policy.rb b/lib/gitlab/ci/config/entry/policy.rb
index adc3660d950..7b14218d3ea 100644
--- a/lib/gitlab/ci/config/entry/policy.rb
+++ b/lib/gitlab/ci/config/entry/policy.rb
@@ -17,7 +17,7 @@ module Gitlab
include ::Gitlab::Config::Entry::Validatable
validations do
- validates :config, array_of_strings_or_regexps: true
+ validates :config, array_of_strings_or_regexps_with_fallback: true
end
def value
@@ -38,7 +38,7 @@ module Gitlab
validate :variables_expressions_syntax
with_options allow_nil: true do
- validates :refs, array_of_strings_or_regexps: true
+ validates :refs, array_of_strings_or_regexps_with_fallback: true
validates :kubernetes, allowed_values: %w[active]
validates :variables, array_of_strings: true
validates :changes, array_of_strings: true
diff --git a/lib/gitlab/config/entry/validators.rb b/lib/gitlab/config/entry/validators.rb
index d0ee94370ba..746fe83f90f 100644
--- a/lib/gitlab/config/entry/validators.rb
+++ b/lib/gitlab/config/entry/validators.rb
@@ -129,6 +129,12 @@ module Gitlab
end
end
+ protected
+
+ def fallback
+ false
+ end
+
private
def matches_syntax?(value)
@@ -137,7 +143,7 @@ module Gitlab
def validate_regexp(value)
matches_syntax?(value) &&
- Gitlab::UntrustedRegexp::RubySyntax.valid?(value)
+ Gitlab::UntrustedRegexp::RubySyntax.valid?(value, fallback: fallback)
end
end
@@ -162,6 +168,14 @@ module Gitlab
end
end
+ class ArrayOfStringsOrRegexpsWithFallbackValidator < ArrayOfStringsOrRegexpsValidator
+ protected
+
+ def fallback
+ true
+ end
+ end
+
class ArrayOfStringsOrStringValidator < RegexpValidator
def validate_each(record, attribute, value)
unless validate_array_of_strings_or_string(value)
diff --git a/lib/gitlab/untrusted_regexp/ruby_syntax.rb b/lib/gitlab/untrusted_regexp/ruby_syntax.rb
index 91f300f97d0..6adf119aa75 100644
--- a/lib/gitlab/untrusted_regexp/ruby_syntax.rb
+++ b/lib/gitlab/untrusted_regexp/ruby_syntax.rb
@@ -6,7 +6,7 @@ module Gitlab
# and converts that to RE2 representation:
# /<regexp>/<flags>
class RubySyntax
- PATTERN = %r{^/(?<regexp>.+)/(?<flags>[ismU]*)$}.freeze
+ PATTERN = %r{^/(?<regexp>.*)/(?<flags>[ismU]*)$}.freeze
# Checks if pattern matches a regexp pattern
# but does not enforce it's validity
@@ -16,28 +16,47 @@ module Gitlab
# The regexp can match the pattern `/.../`, but may not be fabricatable:
# it can be invalid or incomplete: `/match ( string/`
- def self.valid?(pattern)
- !!self.fabricate(pattern)
+ def self.valid?(pattern, fallback: false)
+ !!self.fabricate(pattern, fallback: fallback)
end
- def self.fabricate(pattern)
- self.fabricate!(pattern)
+ def self.fabricate(pattern, fallback: false)
+ self.fabricate!(pattern, fallback: fallback)
rescue RegexpError
nil
end
- def self.fabricate!(pattern)
+ def self.fabricate!(pattern, fallback: false)
raise RegexpError, 'Pattern is not string!' unless pattern.is_a?(String)
matches = pattern.match(PATTERN)
raise RegexpError, 'Invalid regular expression!' if matches.nil?
- expression = matches[:regexp]
- flags = matches[:flags]
- expression.prepend("(?#{flags})") if flags.present?
+ begin
+ create_untrusted_regexp(matches[:regexp], matches[:flags])
+ rescue RegexpError
+ raise unless fallback &&
+ Feature.enabled?(:allow_unsafe_ruby_regexp, default_enabled: false)
- UntrustedRegexp.new(expression, multiline: false)
+ create_ruby_regexp(matches[:regexp], matches[:flags])
+ end
end
+
+ def self.create_untrusted_regexp(pattern, flags)
+ pattern.prepend("(?#{flags})") if flags.present?
+
+ UntrustedRegexp.new(pattern, multiline: false)
+ end
+ private_class_method :create_untrusted_regexp
+
+ def self.create_ruby_regexp(pattern, flags)
+ options = 0
+ options += Regexp::IGNORECASE if flags&.include?('i')
+ options += Regexp::MULTILINE if flags&.include?('m')
+
+ Regexp.new(pattern, options)
+ end
+ private_class_method :create_ruby_regexp
end
end
end
diff --git a/spec/lib/gitlab/ci/build/policy/refs_spec.rb b/spec/lib/gitlab/ci/build/policy/refs_spec.rb
index ec0450643c3..22ca681cfd3 100644
--- a/spec/lib/gitlab/ci/build/policy/refs_spec.rb
+++ b/spec/lib/gitlab/ci/build/policy/refs_spec.rb
@@ -101,6 +101,32 @@ describe Gitlab::Ci::Build::Policy::Refs do
expect(described_class.new(['/fix-.*/']))
.not_to be_satisfied_by(pipeline)
end
+
+ context 'when unsafe regexp is used' do
+ let(:subject) { described_class.new(['/^(?!master).+/']) }
+
+ context 'when allow_unsafe_ruby_regexp is disabled' do
+ before do
+ stub_feature_flags(allow_unsafe_ruby_regexp: false)
+ end
+
+ it 'ignores invalid regexp' do
+ expect(subject)
+ .not_to be_satisfied_by(pipeline)
+ end
+ end
+
+ context 'when allow_unsafe_ruby_regexp is enabled' do
+ before do
+ stub_feature_flags(allow_unsafe_ruby_regexp: true)
+ end
+
+ it 'is satisfied by regexp' do
+ expect(subject)
+ .to be_satisfied_by(pipeline)
+ end
+ end
+ end
end
context 'malicious regexp' do
diff --git a/spec/lib/gitlab/ci/config/entry/policy_spec.rb b/spec/lib/gitlab/ci/config/entry/policy_spec.rb
index 1c987e13a9a..fba5671594d 100644
--- a/spec/lib/gitlab/ci/config/entry/policy_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/policy_spec.rb
@@ -1,4 +1,5 @@
require 'fast_spec_helper'
+require 'support/helpers/stub_feature_flags'
require_dependency 'active_model'
describe Gitlab::Ci::Config::Entry::Policy do
@@ -33,6 +34,44 @@ describe Gitlab::Ci::Config::Entry::Policy do
end
end
+ context 'when config is an empty regexp' do
+ let(:config) { ['//'] }
+
+ describe '#valid?' do
+ it 'is valid' do
+ expect(entry).to be_valid
+ end
+ end
+ end
+
+ context 'when using unsafe regexp' do
+ include StubFeatureFlags
+
+ let(:config) { ['/^(?!master).+/'] }
+
+ subject { described_class.new([regexp]) }
+
+ context 'when allow_unsafe_ruby_regexp is disabled' do
+ before do
+ stub_feature_flags(allow_unsafe_ruby_regexp: false)
+ end
+
+ it 'is not valid' do
+ expect(entry).not_to be_valid
+ end
+ end
+
+ context 'when allow_unsafe_ruby_regexp is enabled' do
+ before do
+ stub_feature_flags(allow_unsafe_ruby_regexp: true)
+ end
+
+ it 'is valid' do
+ expect(entry).to be_valid
+ end
+ end
+ end
+
context 'when config is a special keyword' do
let(:config) { %w[tags triggers branches] }
@@ -67,6 +106,34 @@ describe Gitlab::Ci::Config::Entry::Policy do
end
end
+ context 'when using unsafe regexp' do
+ include StubFeatureFlags
+
+ let(:config) { { refs: ['/^(?!master).+/'] } }
+
+ subject { described_class.new([regexp]) }
+
+ context 'when allow_unsafe_ruby_regexp is disabled' do
+ before do
+ stub_feature_flags(allow_unsafe_ruby_regexp: false)
+ end
+
+ it 'is not valid' do
+ expect(entry).not_to be_valid
+ end
+ end
+
+ context 'when allow_unsafe_ruby_regexp is enabled' do
+ before do
+ stub_feature_flags(allow_unsafe_ruby_regexp: true)
+ end
+
+ it 'is valid' do
+ expect(entry).to be_valid
+ end
+ end
+ end
+
context 'when specifying kubernetes policy' do
let(:config) { { kubernetes: 'active' } }
diff --git a/spec/lib/gitlab/untrusted_regexp/ruby_syntax_spec.rb b/spec/lib/gitlab/untrusted_regexp/ruby_syntax_spec.rb
index 005d41580de..f1882e03581 100644
--- a/spec/lib/gitlab/untrusted_regexp/ruby_syntax_spec.rb
+++ b/spec/lib/gitlab/untrusted_regexp/ruby_syntax_spec.rb
@@ -1,5 +1,6 @@
require 'fast_spec_helper'
require 'support/shared_examples/malicious_regexp_shared_examples'
+require 'support/helpers/stub_feature_flags'
describe Gitlab::UntrustedRegexp::RubySyntax do
describe '.matches_syntax?' do
@@ -33,6 +34,12 @@ describe Gitlab::UntrustedRegexp::RubySyntax do
end
end
+ context 'when regexp is empty' do
+ it 'fabricates regexp correctly' do
+ expect(described_class.fabricate('//')).not_to be_nil
+ end
+ end
+
context 'when regexp is a raw pattern' do
it 'returns error' do
expect(described_class.fabricate('some .* thing')).to be_nil
@@ -41,24 +48,63 @@ describe Gitlab::UntrustedRegexp::RubySyntax do
end
describe '.fabricate!' do
- context 'when regexp is using /regexp/ scheme with flags' do
- it 'fabricates regexp with a single flag' do
- regexp = described_class.fabricate!('/something/i')
+ context 'safe regexp is used' do
+ context 'when regexp is using /regexp/ scheme with flags' do
+ it 'fabricates regexp with a single flag' do
+ regexp = described_class.fabricate!('/something/i')
+
+ expect(regexp).to eq Gitlab::UntrustedRegexp.new('(?i)something')
+ expect(regexp.scan('SOMETHING')).to be_one
+ end
- expect(regexp).to eq Gitlab::UntrustedRegexp.new('(?i)something')
- expect(regexp.scan('SOMETHING')).to be_one
+ it 'fabricates regexp with multiple flags' do
+ regexp = described_class.fabricate!('/something/im')
+
+ expect(regexp).to eq Gitlab::UntrustedRegexp.new('(?im)something')
+ end
+
+ it 'fabricates regexp without flags' do
+ regexp = described_class.fabricate!('/something/')
+
+ expect(regexp).to eq Gitlab::UntrustedRegexp.new('something')
+ end
end
+ end
- it 'fabricates regexp with multiple flags' do
- regexp = described_class.fabricate!('/something/im')
+ context 'when unsafe regexp is used' do
+ include StubFeatureFlags
- expect(regexp).to eq Gitlab::UntrustedRegexp.new('(?im)something')
+ before do
+ stub_feature_flags(allow_unsafe_ruby_regexp: true)
+
+ allow(Gitlab::UntrustedRegexp).to receive(:new).and_raise(RegexpError)
end
- it 'fabricates regexp without flags' do
- regexp = described_class.fabricate!('/something/')
+ context 'when no fallback is enabled' do
+ it 'raises an exception' do
+ expect { described_class.fabricate!('/something/') }
+ .to raise_error(RegexpError)
+ end
+ end
+
+ context 'when fallback is used' do
+ it 'fabricates regexp with a single flag' do
+ regexp = described_class.fabricate!('/something/i', fallback: true)
+
+ expect(regexp).to eq Regexp.new('something', Regexp::IGNORECASE)
+ end
+
+ it 'fabricates regexp with multiple flags' do
+ regexp = described_class.fabricate!('/something/im', fallback: true)
+
+ expect(regexp).to eq Regexp.new('something', Regexp::IGNORECASE | Regexp::MULTILINE)
+ end
+
+ it 'fabricates regexp without flags' do
+ regexp = described_class.fabricate!('/something/', fallback: true)
- expect(regexp).to eq Gitlab::UntrustedRegexp.new('something')
+ expect(regexp).to eq Regexp.new('something')
+ end
end
end