Commit 7dcc3003 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'allow-to-use-untrusted-ruby-syntax' into 'master'

Allow to use untrusted ruby syntax

See merge request gitlab-org/gitlab-ce!26905
parents 7926384f 8a833c72
---
title: Allow to use untrusted Regexp via feature flag
merge_request: 26905
author:
type: deprecated
...@@ -414,6 +414,27 @@ job: ...@@ -414,6 +414,27 @@ job:
only: ['branches', 'tags'] 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) ### `only`/`except` (advanced)
> - `refs` and `kubernetes` policies introduced in GitLab 10.0. > - `refs` and `kubernetes` policies introduced in GitLab 10.0.
......
...@@ -35,7 +35,7 @@ module Gitlab ...@@ -35,7 +35,7 @@ module Gitlab
# patterns can be matched only when branch or tag is used # patterns can be matched only when branch or tag is used
# the pattern matching does not work for merge requests pipelines # the pattern matching does not work for merge requests pipelines
if pipeline.branch? || pipeline.tag? 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) regexp.match?(pipeline.ref)
else else
pattern == pipeline.ref pattern == pipeline.ref
......
...@@ -17,7 +17,7 @@ module Gitlab ...@@ -17,7 +17,7 @@ module Gitlab
include ::Gitlab::Config::Entry::Validatable include ::Gitlab::Config::Entry::Validatable
validations do validations do
validates :config, array_of_strings_or_regexps: true validates :config, array_of_strings_or_regexps_with_fallback: true
end end
def value def value
...@@ -38,7 +38,7 @@ module Gitlab ...@@ -38,7 +38,7 @@ module Gitlab
validate :variables_expressions_syntax validate :variables_expressions_syntax
with_options allow_nil: true do 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 :kubernetes, allowed_values: %w[active]
validates :variables, array_of_strings: true validates :variables, array_of_strings: true
validates :changes, array_of_strings: true validates :changes, array_of_strings: true
......
...@@ -129,6 +129,12 @@ module Gitlab ...@@ -129,6 +129,12 @@ module Gitlab
end end
end end
protected
def fallback
false
end
private private
def matches_syntax?(value) def matches_syntax?(value)
...@@ -137,7 +143,7 @@ module Gitlab ...@@ -137,7 +143,7 @@ module Gitlab
def validate_regexp(value) def validate_regexp(value)
matches_syntax?(value) && matches_syntax?(value) &&
Gitlab::UntrustedRegexp::RubySyntax.valid?(value) Gitlab::UntrustedRegexp::RubySyntax.valid?(value, fallback: fallback)
end end
end end
...@@ -162,6 +168,14 @@ module Gitlab ...@@ -162,6 +168,14 @@ module Gitlab
end end
end end
class ArrayOfStringsOrRegexpsWithFallbackValidator < ArrayOfStringsOrRegexpsValidator
protected
def fallback
true
end
end
class ArrayOfStringsOrStringValidator < RegexpValidator class ArrayOfStringsOrStringValidator < RegexpValidator
def validate_each(record, attribute, value) def validate_each(record, attribute, value)
unless validate_array_of_strings_or_string(value) unless validate_array_of_strings_or_string(value)
......
...@@ -6,7 +6,7 @@ module Gitlab ...@@ -6,7 +6,7 @@ module Gitlab
# and converts that to RE2 representation: # and converts that to RE2 representation:
# /<regexp>/<flags> # /<regexp>/<flags>
class RubySyntax class RubySyntax
PATTERN = %r{^/(?<regexp>.+)/(?<flags>[ismU]*)$}.freeze PATTERN = %r{^/(?<regexp>.*)/(?<flags>[ismU]*)$}.freeze
# Checks if pattern matches a regexp pattern # Checks if pattern matches a regexp pattern
# but does not enforce it's validity # but does not enforce it's validity
...@@ -16,28 +16,47 @@ module Gitlab ...@@ -16,28 +16,47 @@ module Gitlab
# The regexp can match the pattern `/.../`, but may not be fabricatable: # The regexp can match the pattern `/.../`, but may not be fabricatable:
# it can be invalid or incomplete: `/match ( string/` # it can be invalid or incomplete: `/match ( string/`
def self.valid?(pattern) def self.valid?(pattern, fallback: false)
!!self.fabricate(pattern) !!self.fabricate(pattern, fallback: fallback)
end end
def self.fabricate(pattern) def self.fabricate(pattern, fallback: false)
self.fabricate!(pattern) self.fabricate!(pattern, fallback: fallback)
rescue RegexpError rescue RegexpError
nil nil
end end
def self.fabricate!(pattern) def self.fabricate!(pattern, fallback: false)
raise RegexpError, 'Pattern is not string!' unless pattern.is_a?(String) raise RegexpError, 'Pattern is not string!' unless pattern.is_a?(String)
matches = pattern.match(PATTERN) matches = pattern.match(PATTERN)
raise RegexpError, 'Invalid regular expression!' if matches.nil? raise RegexpError, 'Invalid regular expression!' if matches.nil?
expression = matches[:regexp] begin
flags = matches[:flags] create_untrusted_regexp(matches[:regexp], matches[:flags])
expression.prepend("(?#{flags})") if flags.present? rescue RegexpError
raise unless fallback &&
Feature.enabled?(:allow_unsafe_ruby_regexp, default_enabled: 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')
UntrustedRegexp.new(expression, multiline: false) Regexp.new(pattern, options)
end end
private_class_method :create_ruby_regexp
end end
end end
end end
...@@ -101,6 +101,32 @@ describe Gitlab::Ci::Build::Policy::Refs do ...@@ -101,6 +101,32 @@ describe Gitlab::Ci::Build::Policy::Refs do
expect(described_class.new(['/fix-.*/'])) expect(described_class.new(['/fix-.*/']))
.not_to be_satisfied_by(pipeline) .not_to be_satisfied_by(pipeline)
end 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 end
context 'malicious regexp' do context 'malicious regexp' do
......
require 'fast_spec_helper' require 'fast_spec_helper'
require 'support/helpers/stub_feature_flags'
require_dependency 'active_model' require_dependency 'active_model'
describe Gitlab::Ci::Config::Entry::Policy do describe Gitlab::Ci::Config::Entry::Policy do
...@@ -33,6 +34,44 @@ describe Gitlab::Ci::Config::Entry::Policy do ...@@ -33,6 +34,44 @@ describe Gitlab::Ci::Config::Entry::Policy do
end end
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 context 'when config is a special keyword' do
let(:config) { %w[tags triggers branches] } let(:config) { %w[tags triggers branches] }
...@@ -67,6 +106,34 @@ describe Gitlab::Ci::Config::Entry::Policy do ...@@ -67,6 +106,34 @@ describe Gitlab::Ci::Config::Entry::Policy do
end end
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 context 'when specifying kubernetes policy' do
let(:config) { { kubernetes: 'active' } } let(:config) { { kubernetes: 'active' } }
......
require 'fast_spec_helper' require 'fast_spec_helper'
require 'support/shared_examples/malicious_regexp_shared_examples' require 'support/shared_examples/malicious_regexp_shared_examples'
require 'support/helpers/stub_feature_flags'
describe Gitlab::UntrustedRegexp::RubySyntax do describe Gitlab::UntrustedRegexp::RubySyntax do
describe '.matches_syntax?' do describe '.matches_syntax?' do
...@@ -33,6 +34,12 @@ describe Gitlab::UntrustedRegexp::RubySyntax do ...@@ -33,6 +34,12 @@ describe Gitlab::UntrustedRegexp::RubySyntax do
end end
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 context 'when regexp is a raw pattern' do
it 'returns error' do it 'returns error' do
expect(described_class.fabricate('some .* thing')).to be_nil expect(described_class.fabricate('some .* thing')).to be_nil
...@@ -41,6 +48,7 @@ describe Gitlab::UntrustedRegexp::RubySyntax do ...@@ -41,6 +48,7 @@ describe Gitlab::UntrustedRegexp::RubySyntax do
end end
describe '.fabricate!' do describe '.fabricate!' do
context 'safe regexp is used' do
context 'when regexp is using /regexp/ scheme with flags' do context 'when regexp is using /regexp/ scheme with flags' do
it 'fabricates regexp with a single flag' do it 'fabricates regexp with a single flag' do
regexp = described_class.fabricate!('/something/i') regexp = described_class.fabricate!('/something/i')
...@@ -61,6 +69,44 @@ describe Gitlab::UntrustedRegexp::RubySyntax do ...@@ -61,6 +69,44 @@ describe Gitlab::UntrustedRegexp::RubySyntax do
expect(regexp).to eq Gitlab::UntrustedRegexp.new('something') expect(regexp).to eq Gitlab::UntrustedRegexp.new('something')
end end
end end
end
context 'when unsafe regexp is used' do
include StubFeatureFlags
before do
stub_feature_flags(allow_unsafe_ruby_regexp: true)
allow(Gitlab::UntrustedRegexp).to receive(:new).and_raise(RegexpError)
end
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 Regexp.new('something')
end
end
end
context 'when regexp is a raw pattern' do context 'when regexp is a raw pattern' do
it 'raises an error' do it 'raises an error' 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