fix: use relative URLs when base_url not explictly set to ensure app is not vulnerable to host header attacks

bethesque · bethesque · commit 92c45a0add2a · 2020-06-01T16:56:29.000+10:00
diff --git a/lib/pact_broker/doc/controllers/app.rb b/lib/pact_broker/doc/controllers/app.rb
@@ -46,7 +46,11 @@ def resource_exists? rel_name, context = nil
         private
 
         def base_url
-          PactBroker.configuration.base_url || request.base_url
+          # Using the X-Forwarded headers in the UI can leave the app vulnerable
+          # https://www.acunetix.com/blog/articles/automated-detection-of-host-header-attacks/
+          # Either use the explicitly configured base url or an empty string,
+          # rather than request.base_url, which uses the X-Forwarded headers.
+          PactBroker.configuration.base_url || ''
         end
       end
     end
diff --git a/lib/pact_broker/ui/controllers/base_controller.rb b/lib/pact_broker/ui/controllers/base_controller.rb
@@ -12,7 +12,7 @@ class Base < Padrino::Application
         set :dump_errors, false # The padrino logger logs these for us. If this is enabled we get duplicate logging.
 
         def base_url
-          PactBroker.configuration.base_url || request.base_url
+          PactBroker.configuration.base_url || ''
         end
       end
     end
diff --git a/lib/pact_broker/ui/views/index/_navbar.haml b/lib/pact_broker/ui/views/index/_navbar.haml
@@ -7,7 +7,7 @@
           %a{href: "#{base_url}?tags=true"}
             Show latest tags
         - else
-          %a{href: "#{base_url}"}
+          %a{href: "#{base_url}/"}
             Hide latest tags
 
         %a{href: "#{base_url}/hal-browser/browser.html"}
diff --git a/spec/integration/ui/index_spec.rb b/spec/integration/ui/index_spec.rb
@@ -32,5 +32,21 @@
         expect(subject.body.scan('<tr').to_a.count).to eq 3
       end
     end
+
+    context "with the base_url not set" do
+      it "returns relative links" do
+        expect(subject.body).to include "href='/stylesheets"
+      end
+    end
+
+    context "with the base_url set" do
+      before do
+        allow(PactBroker.configuration).to receive(:base_url).and_return('http://example.org/pact-broker')
+      end
+
+      it "returns absolute links" do
+        expect(subject.body).to include "href='http://example.org/pact-broker/stylesheets"
+      end
+    end
   end
 end
diff --git a/spec/lib/pact_broker/doc/controllers/app_spec.rb b/spec/lib/pact_broker/doc/controllers/app_spec.rb
@@ -26,6 +26,22 @@ module Controllers
               subject
               expect(last_response.body).to include "<html>"
             end
+
+            context "with the base_url not set" do
+              it "returns relative links" do
+                expect(subject.body).to include "href='/css"
+              end
+            end
+
+            context "with the base_url set" do
+              before do
+                allow(PactBroker.configuration).to receive(:base_url).and_return('http://example.org/pact-broker')
+              end
+
+              it "returns absolute links" do
+                expect(subject.body).to include "href='http://example.org/pact-broker/css"
+              end
+            end
           end
 
           context "when the resource does not exist" do
