fix: return a 422 if the URL path contains a new line or tab

Author: bethesque

lib/rack/pact_broker/invalid_uri_protection.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'uri'
 
 # This class is for https://github.com/pact-foundation/pact_broker/issues/101
@@ -6,31 +8,46 @@
 module Rack
   module PactBroker
     class InvalidUriProtection
-
       def initialize app
         @app = app
       end
 
       def call env
-        if valid_uri? env
-          @app.call(env)
+        if (uri = valid_uri?(env))
+          if (error_message = validate(uri))
+            [422, {'Content-Type' => 'text/plain'}, [error_message]]
+          else
+            app.call(env)
+          end
         else
           [404, {}, []]
         end
       end
 
+      private
+
+      attr_reader :app
+
       def valid_uri? env
         begin
           parse(::Rack::Request.new(env).url)
-          true
         rescue URI::InvalidURIError, ArgumentError
-          false
+          nil
         end
       end
 
       def parse uri
         URI.parse(uri)
       end
+
+      def validate(uri)
+        decoded_path = URI.decode(uri.path)
+        if decoded_path.include?("\n")
+          'URL path cannot contain a new line character.'
+        elsif decoded_path.include?("\t")
+          'URL path cannot contain a tab character.'
+        end
+      end
     end
   end
 end

spec/lib/rack/pact_broker/invalid_uri_protection_spec.rb

@@ -3,12 +3,14 @@
 module Rack
   module PactBroker
     describe InvalidUriProtection do
+      let(:target_app) { ->(env){ [200, {}, []] } }
+      let(:app) { InvalidUriProtection.new(target_app) }
+      let(:path) { URI.encode("/foo") }
 
-      let(:app) { InvalidUriProtection.new(->(env){ [200,{},[]] }) }
-
-      subject { get "/badpath"; last_response }
+      subject { get(path) }
 
       context "with a URI that the Ruby default URI library cannot parse" do
+        let(:path) { "/badpath" }
 
         before do
           # Can't use or stub URI.parse because rack test uses it to execute the actual test
@@ -24,6 +26,22 @@ module PactBroker
         it "passes the request to the underlying app" do
           expect(subject.status).to eq 200
         end
+
+        context "when the URI contains a new line because someone forgot to strip the result of `git rev-parse HEAD`, and I have totally never done this before myself" do
+          let(:path) { URI.encode("/foo\n/bar") }
+
+          it "returns a 422" do
+            expect(subject.status).to eq 422
+          end
+        end
+
+        context "when the URI contains a tab because sooner or later someone is eventually going to do this" do
+          let(:path) { URI.encode("/foo\t/bar") }
+
+          it "returns a 422" do
+            expect(subject.status).to eq 422
+          end
+        end
       end
     end
   end