Simplify untrusted regexp factory method

4 files changed, 16 insertions, 21 deletions

diff --git a/lib/gitlab/untrusted_regexp.rb b/lib/gitlab/untrusted_regexp.rb
index 70d1a7c6535..dc2d91dfa23 100644
--- a/lib/gitlab/untrusted_regexp.rb
+++ b/lib/gitlab/untrusted_regexp.rb
@@ -55,7 +55,7 @@ module Gitlab
     end
 
     def self.valid?(pattern)
-      self.fabricate(pattern)
+      !!self.fabricate(pattern)
     rescue RegexpError
       false
     end
@@ -63,16 +63,13 @@ module Gitlab
     def self.fabricate(pattern)
       matches = pattern.match(%r{^/(?<regexp>.+)/(?<flags>[ismU]*)$})
 
-      if matches
-        expression = matches[:regexp]
-        flags = matches[:flags]
+      raise RegexpError, 'Invalid regular expression!' if matches.nil?
 
-        expression.prepend("(?#{flags})") if flags.present?
+      expression = matches[:regexp]
+      flags = matches[:flags]
+      expression.prepend("(?#{flags})") if flags.present?
 
-        self.new(expression, multiline: false)
-      else
-        self.new(pattern, multiline: false)
-      end
+      self.new(expression, multiline: false)
     end
 
     private
diff --git a/spec/lib/gitlab/ci/pipeline/expression/lexeme/pattern_spec.rb b/spec/lib/gitlab/ci/pipeline/expression/lexeme/pattern_spec.rb
index c63c38b1dbc..3ebc2e94727 100644
--- a/spec/lib/gitlab/ci/pipeline/expression/lexeme/pattern_spec.rb
+++ b/spec/lib/gitlab/ci/pipeline/expression/lexeme/pattern_spec.rb
@@ -79,7 +79,7 @@ describe Gitlab::Ci::Pipeline::Expression::Lexeme::Pattern do
 
   describe '#evaluate' do
     it 'returns a regular expression' do
-      regexp = described_class.new('abc')
+      regexp = described_class.new('/abc/')
 
       expect(regexp.evaluate).to eq Gitlab::UntrustedRegexp.new('abc')
     end
@@ -87,7 +87,7 @@ describe Gitlab::Ci::Pipeline::Expression::Lexeme::Pattern do
     it 'raises error if evaluated regexp is not valid' do
       allow(Gitlab::UntrustedRegexp).to receive(:valid?).and_return(true)
 
-      regexp = described_class.new('invalid ( .*')
+      regexp = described_class.new('/invalid ( .*/')
 
       expect { regexp.evaluate }
         .to raise_error(Gitlab::Ci::Pipeline::Expression::RuntimeError)
diff --git a/spec/lib/gitlab/untrusted_regexp_spec.rb b/spec/lib/gitlab/untrusted_regexp_spec.rb
index 4bca320ac2c..0a6ac0aa294 100644
--- a/spec/lib/gitlab/untrusted_regexp_spec.rb
+++ b/spec/lib/gitlab/untrusted_regexp_spec.rb
@@ -4,9 +4,13 @@ require 'support/shared_examples/malicious_regexp_shared_examples'
 describe Gitlab::UntrustedRegexp do
   describe '.valid?' do
     it 'returns true if regexp is valid' do
+      expect(described_class.valid?('/some ( thing/'))
+        .to be false
     end
 
     it 'returns true if regexp is invalid' do
+      expect(described_class.valid?('/some .* thing/'))
+        .to be true
     end
   end
 
@@ -32,17 +36,9 @@ describe Gitlab::UntrustedRegexp do
       end
     end
 
-    context 'when regexp is not plain pattern' do
-      it 'fabricates regexp without flags' do
-        regexp = described_class.fabricate('something')
-
-        expect(regexp).to eq described_class.new('something')
-      end
-    end
-
-    context 'when regexp is invalid' do
+    context 'when regexp is a raw pattern' do
       it 'raises an error' do
-        expect { described_class.fabricate('/some ( thing/') }
+        expect { described_class.fabricate('some .* thing') }
           .to raise_error(RegexpError)
       end
     end
diff --git a/spec/support/shared_examples/malicious_regexp_shared_examples.rb b/spec/support/shared_examples/malicious_regexp_shared_examples.rb
index ac5d22298bb..65026f1d7c0 100644
--- a/spec/support/shared_examples/malicious_regexp_shared_examples.rb
+++ b/spec/support/shared_examples/malicious_regexp_shared_examples.rb
@@ -1,3 +1,5 @@
+require 'timeout'
+
 shared_examples 'malicious regexp' do
   let(:malicious_text)  { 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa!' }
   let(:malicious_regexp) { '(?i)^(([a-z])+.)+[A-Z]([a-z])+$' }