Merge branch 'master' of dev.gitlab.org:gitlab/gitlabhq

5 files changed, 251 insertions, 54 deletions

diff --git a/spec/lib/banzai/filter/wiki_link_filter_spec.rb b/spec/lib/banzai/filter/wiki_link_filter_spec.rb
index 877d99b68f3..d2d539a62fc 100644
--- a/spec/lib/banzai/filter/wiki_link_filter_spec.rb
+++ b/spec/lib/banzai/filter/wiki_link_filter_spec.rb
@@ -72,47 +72,5 @@ describe Banzai::Filter::WikiLinkFilter do
         expect(filtered_link.attribute('href').value).to eq(invalid_link)
       end
     end
-
-    context "when the slug is deemed unsafe or invalid" do
-      let(:link) { "alert(1);" }
-
-      invalid_slugs = [
-        "javascript:",
-        "JaVaScRiPt:",
-        "\u0001java\u0003script:",
-        "javascript    :",
-        "javascript:    ",
-        "javascript    :   ",
-        ":javascript:",
-        "javascript:",
-        "javascript:",
-        "javascript:",
-        "javascript:",
-        "java\0script:",
-        "   javascript:"
-      ]
-
-      invalid_slugs.each do |slug|
-        context "with the slug #{slug}" do
-          it "doesn't rewrite a (.) relative link" do
-            filtered_link = filter(
-              "<a href='.#{link}'>Link</a>",
-              project_wiki: wiki,
-              page_slug: slug).children[0]
-
-            expect(filtered_link.attribute('href').value).not_to include(slug)
-          end
-
-          it "doesn't rewrite a (..) relative link" do
-            filtered_link = filter(
-              "<a href='..#{link}'>Link</a>",
-              project_wiki: wiki,
-              page_slug: slug).children[0]
-
-            expect(filtered_link.attribute('href').value).not_to include(slug)
-          end
-        end
-      end
-    end
   end
 end
diff --git a/spec/lib/banzai/pipeline/wiki_pipeline_spec.rb b/spec/lib/banzai/pipeline/wiki_pipeline_spec.rb
index cb94944fdfb..015af20f220 100644
--- a/spec/lib/banzai/pipeline/wiki_pipeline_spec.rb
+++ b/spec/lib/banzai/pipeline/wiki_pipeline_spec.rb
@@ -179,6 +179,85 @@ describe Banzai::Pipeline::WikiPipeline do
         end
       end
     end
+
+    describe "checking slug validity when assembling links" do
+      context "with a valid slug" do
+        let(:valid_slug) { "http://example.com" }
+
+        it "includes the slug in a (.) relative link" do
+          output = described_class.to_html(
+            "[Link](./alert(1);)",
+            project: project,
+            project_wiki: project_wiki,
+            page_slug: valid_slug
+          )
+
+          expect(output).to include(valid_slug)
+        end
+
+        it "includeds the slug in a (..) relative link" do
+          output = described_class.to_html(
+            "[Link](../alert(1);)",
+            project: project,
+            project_wiki: project_wiki,
+            page_slug: valid_slug
+          )
+
+          expect(output).to include(valid_slug)
+        end
+      end
+
+      context "when the slug is deemed unsafe or invalid" do
+        invalid_slugs = [
+          "javascript:",
+          "JaVaScRiPt:",
+          "\u0001java\u0003script:",
+          "javascript    :",
+          "javascript:    ",
+          "javascript    :   ",
+          ":javascript:",
+          "javascript&#58;",
+          "javascript&#0058;",
+          "javascript&#x3A;",
+          "javascript&#x003A;",
+          "java\0script:",
+          " &#14;  javascript:"
+        ]
+
+        invalid_js_links = [
+          "alert(1);",
+          "alert(document.location);"
+        ]
+
+        invalid_slugs.each do |slug|
+          context "with the invalid slug #{slug}" do
+            invalid_js_links.each do |link|
+              it "doesn't include a prohibited slug in a (.) relative link '#{link}'" do
+                output = described_class.to_html(
+                  "[Link](./#{link})",
+                  project: project,
+                  project_wiki: project_wiki,
+                  page_slug: slug
+                )
+
+                expect(output).not_to include(slug)
+              end
+
+              it "doesn't include a prohibited slug in a (..) relative link '#{link}'" do
+                output = described_class.to_html(
+                  "[Link](../#{link})",
+                  project: project,
+                  project_wiki: project_wiki,
+                  page_slug: slug
+                )
+
+                expect(output).not_to include(slug)
+              end
+            end
+          end
+        end
+      end
+    end
   end
 
   describe 'videos' do
diff --git a/spec/lib/gitlab/octokit/middleware_spec.rb b/spec/lib/gitlab/octokit/middleware_spec.rb
new file mode 100644
index 00000000000..7f2b523f5b7
--- /dev/null
+++ b/spec/lib/gitlab/octokit/middleware_spec.rb
@@ -0,0 +1,68 @@
+require 'spec_helper'
+
+describe Gitlab::Octokit::Middleware do
+  let(:app) { double(:app) }
+  let(:middleware) { described_class.new(app) }
+
+  shared_examples 'Public URL' do
+    it 'does not raise an error' do
+      expect(app).to receive(:call).with(env)
+
+      expect { middleware.call(env) }.not_to raise_error
+    end
+  end
+
+  shared_examples 'Local URL' do
+    it 'raises an error' do
+      expect { middleware.call(env) }.to raise_error(Gitlab::UrlBlocker::BlockedUrlError)
+    end
+  end
+
+  describe '#call' do
+    context 'when the URL is a public URL' do
+      let(:env) { { url: 'https://public-url.com' } }
+
+      it_behaves_like 'Public URL'
+    end
+
+    context 'when the URL is a localhost adresss' do
+      let(:env) { { url: 'http://127.0.0.1' } }
+
+      context 'when localhost requests are not allowed' do
+        before do
+          stub_application_setting(allow_local_requests_from_hooks_and_services: false)
+        end
+
+        it_behaves_like 'Local URL'
+      end
+
+      context 'when localhost requests are allowed' do
+        before do
+          stub_application_setting(allow_local_requests_from_hooks_and_services: true)
+        end
+
+        it_behaves_like 'Public URL'
+      end
+    end
+
+    context 'when the URL is a local network address' do
+      let(:env) { { url: 'http://172.16.0.0' } }
+
+      context 'when local network requests are not allowed' do
+        before do
+          stub_application_setting(allow_local_requests_from_hooks_and_services: false)
+        end
+
+        it_behaves_like 'Local URL'
+      end
+
+      context 'when local network requests are allowed' do
+        before do
+          stub_application_setting(allow_local_requests_from_hooks_and_services: true)
+        end
+
+        it_behaves_like 'Public URL'
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb
index 93194de4a1b..45d9022abeb 100644
--- a/spec/lib/gitlab/url_blocker_spec.rb
+++ b/spec/lib/gitlab/url_blocker_spec.rb
@@ -68,6 +68,16 @@ describe Gitlab::UrlBlocker do
           expect(uri).to eq(Addressable::URI.parse('https://example.org'))
           expect(hostname).to eq(nil)
         end
+
+        context 'when it cannot be resolved' do
+          let(:import_url) { 'http://foobar.x' }
+
+          it 'raises error' do
+            stub_env('RSPEC_ALLOW_INVALID_URLS', 'false')
+
+            expect { described_class.validate!(import_url) }.to raise_error(described_class::BlockedUrlError)
+          end
+        end
       end
 
       context 'when the URL hostname is an IP address' do
@@ -79,6 +89,16 @@ describe Gitlab::UrlBlocker do
           expect(uri).to eq(Addressable::URI.parse('https://93.184.216.34'))
           expect(hostname).to be(nil)
         end
+
+        context 'when it is invalid' do
+          let(:import_url) { 'http://1.1.1.1.1' }
+
+          it 'raises an error' do
+            stub_env('RSPEC_ALLOW_INVALID_URLS', 'false')
+
+            expect { described_class.validate!(import_url) }.to raise_error(described_class::BlockedUrlError)
+          end
+        end
       end
     end
   end
@@ -180,8 +200,6 @@ describe Gitlab::UrlBlocker do
     end
 
     it 'returns true for a non-alphanumeric hostname' do
-      stub_resolv
-
       aggregate_failures do
         expect(described_class).to be_blocked_url('ssh://-oProxyCommand=whoami/a')
 
@@ -454,10 +472,6 @@ describe Gitlab::UrlBlocker do
     end
 
     context 'when enforce_user is' do
-      before do
-        stub_resolv
-      end
-
       context 'false (default)' do
         it 'does not block urls with a non-alphanumeric username' do
           expect(described_class).not_to be_blocked_url('ssh://-oProxyCommand=whoami@example.com/a')
@@ -505,6 +519,18 @@ describe Gitlab::UrlBlocker do
         expect(described_class.blocked_url?('https://git‌lab.com/foo/foo.bar', ascii_only: true)).to be true
       end
     end
+
+    it 'blocks urls with invalid ip address' do
+      stub_env('RSPEC_ALLOW_INVALID_URLS', 'false')
+
+      expect(described_class).to be_blocked_url('http://8.8.8.8.8')
+    end
+
+    it 'blocks urls whose hostname cannot be resolved' do
+      stub_env('RSPEC_ALLOW_INVALID_URLS', 'false')
+
+      expect(described_class).to be_blocked_url('http://foobar.x')
+    end
   end
 
   describe '#validate_hostname' do
@@ -536,10 +562,4 @@ describe Gitlab::UrlBlocker do
       end
     end
   end
-
-  # Resolv does not support resolving UTF-8 domain names
-  # See https://bugs.ruby-lang.org/issues/4270
-  def stub_resolv
-    allow(Resolv).to receive(:getaddresses).and_return([])
-  end
 end
diff --git a/spec/lib/gitlab/utils/sanitize_node_link_spec.rb b/spec/lib/gitlab/utils/sanitize_node_link_spec.rb
new file mode 100644
index 00000000000..064c2707d06
--- /dev/null
+++ b/spec/lib/gitlab/utils/sanitize_node_link_spec.rb
@@ -0,0 +1,72 @@
+require 'spec_helper'
+
+describe Gitlab::Utils::SanitizeNodeLink do
+  let(:klass) do
+    struct = Struct.new(:value)
+    struct.include(described_class)
+
+    struct
+  end
+
+  subject(:object) { klass.new(:value) }
+
+  invalid_schemes = [
+    "javascript:",
+    "JaVaScRiPt:",
+    "\u0001java\u0003script:",
+    "javascript    :",
+    "javascript:    ",
+    "javascript    :   ",
+    ":javascript:",
+    "javascript&#58;",
+    "javascript&#0058;",
+    " &#14;  javascript:"
+  ]
+
+  invalid_schemes.each do |scheme|
+    context "with the scheme: #{scheme}" do
+      describe "#remove_unsafe_links" do
+        tags = {
+          a: {
+            doc: HTML::Pipeline.parse("<a href='#{scheme}alert(1);'>foo</a>"),
+            attr: "href",
+            node_to_check: -> (doc) { doc.children.first }
+          },
+          img: {
+            doc: HTML::Pipeline.parse("<img src='#{scheme}alert(1);'>"),
+            attr: "src",
+            node_to_check: -> (doc) { doc.children.first }
+          },
+          video: {
+            doc: HTML::Pipeline.parse("<video><source src='#{scheme}alert(1);'></video>"),
+            attr: "src",
+            node_to_check: -> (doc) { doc.children.first.children.filter("source").first }
+          }
+        }
+
+        tags.each do |tag, opts|
+          context "<#{tag}> tags" do
+            it "removes the unsafe link" do
+              node = opts[:node_to_check].call(opts[:doc])
+
+              expect { object.remove_unsafe_links({ node: node }, remove_invalid_links: true) }
+                .to change { node[opts[:attr]] }
+
+              expect(node[opts[:attr]]).to be_blank
+            end
+          end
+        end
+      end
+
+      describe "#safe_protocol?" do
+        let(:doc) { HTML::Pipeline.parse("<a href='#{scheme}alert(1);'>foo</a>") }
+        let(:node) { doc.children.first }
+        let(:uri) { Addressable::URI.parse(node['href'])}
+
+        it "returns false" do
+          expect(object.safe_protocol?(scheme)).to be_falsy
+        end
+      end
+    end
+  end
+end