diff --git a/config.ru b/config.ru index 51bb876d3..dfb8142e5 100644 --- a/config.ru +++ b/config.ru @@ -27,7 +27,7 @@ app = PactBroker::App.new do | config | config.webhook_scheme_whitelist = ['http', 'https'] config.webhook_http_method_whitelist = ['GET', 'POST'] config.webhook_http_code_success = [200, 201, 202, 203, 204, 205, 206] - #config.base_url = ENV['PACT_BROKER_BASE_URL'] + config.base_url = ENV['PACT_BROKER_BASE_URL'] database_logger = PactBroker::DB::LogQuietener.new(config.logger) config.database_connection = Sequel.connect(DATABASE_URL, DB_OPTIONS.merge(logger: database_logger)) diff --git a/docker-compose-dev-postgres.yml b/docker-compose-dev-postgres.yml index dbd48c1de..b992d2d30 100644 --- a/docker-compose-dev-postgres.yml +++ b/docker-compose-dev-postgres.yml @@ -23,7 +23,9 @@ services: environment: DATABASE_URL: postgres://postgres:postgres@postgres/postgres PACT_BROKER_HIDE_PACTFLOW_MESSAGES: 'true' - command: dockerize -wait tcp://postgres:5432 /home/start.sh + PACT_BROKER_BASE_URL: 'http://localhost:9292 http://pact-broker:9292' + command: -wait tcp://postgres:5432 /usr/local/bin/start + entrypoint: dockerize volumes: - ./lib:/home/lib - ./db:/home/db @@ -31,5 +33,11 @@ services: - ./tasks:/home/tasks - ./Rakefile:/home/Rakefile + shell: + image: ruby:2.5.3-alpine + depends_on: + - pact-broker + entrypoint: /bin/sh + volumes: postgres-volume: diff --git a/lib/pact_broker/api/resources/default_base_resource.rb b/lib/pact_broker/api/resources/default_base_resource.rb index c9d6eab78..e901a637b 100644 --- a/lib/pact_broker/api/resources/default_base_resource.rb +++ b/lib/pact_broker/api/resources/default_base_resource.rb @@ -68,6 +68,11 @@ def identifier_from_path alias_method :path_info, :identifier_from_path def base_url + # Have to use something for the base URL here - we can't use an empty string as we can in the UI. + # Can't work out if cache poisoning is a vulnerability for APIs or not. + # Using the request base URI as a fallback if the base_url is not configured may be a vulnerability, + # but the documentation recommends that the + # base_url should be set in the configuration to mitigate this. request.env["pactbroker.base_url"] || request.base_uri.to_s.chomp('/') end diff --git a/lib/pact_broker/app.rb b/lib/pact_broker/app.rb index 0affef8b7..ec50d31e5 100644 --- a/lib/pact_broker/app.rb +++ b/lib/pact_broker/app.rb @@ -191,7 +191,7 @@ def configure_middleware @app_builder.use Rack::Static, :urls => ["/favicon.ico"], :root => PactBroker.project_root.join("public/images"), header_rules: [[:all, {'Content-Type' => 'image/x-icon'}]] @app_builder.use Rack::PactBroker::ConvertFileExtensionToAcceptHeader # Rack::PactBroker::SetBaseUrl needs to be before the Rack::PactBroker::HalBrowserRedirect - @app_builder.use Rack::PactBroker::SetBaseUrl, configuration.base_url + @app_builder.use Rack::PactBroker::SetBaseUrl, configuration.base_urls if configuration.use_hal_browser logger.info "Mounting HAL browser" diff --git a/lib/pact_broker/configuration.rb b/lib/pact_broker/configuration.rb index 28c3a5a81..d8b2e42b8 100644 --- a/lib/pact_broker/configuration.rb +++ b/lib/pact_broker/configuration.rb @@ -255,6 +255,10 @@ def webhook_host_whitelist= webhook_host_whitelist @webhook_host_whitelist = parse_space_delimited_string_list_property('webhook_host_whitelist', webhook_host_whitelist) end + def base_urls + base_url ? base_url.split(" ") : [] + end + def warning_error_classes warning_error_class_names.collect do | class_name | begin diff --git a/lib/rack/pact_broker/set_base_url.rb b/lib/rack/pact_broker/set_base_url.rb index 92ba2c8ee..e528cf1ec 100644 --- a/lib/rack/pact_broker/set_base_url.rb +++ b/lib/rack/pact_broker/set_base_url.rb @@ -1,22 +1,52 @@ module Rack module PactBroker class SetBaseUrl - def initialize app, base_url + X_FORWARDED_PATTERN = /_X_FORWARDED_/.freeze + + def initialize app, base_urls @app = app - @base_url = base_url + @base_urls = base_urls end - def call env + def call(env) if env["pactbroker.base_url"] app.call(env) else - app.call(env.merge("pactbroker.base_url" => base_url)) + app.call(env.merge("pactbroker.base_url" => select_matching_base_url(env))) end end private - attr_reader :app, :base_url + attr_reader :app, :base_urls + + def select_matching_base_url(env) + if base_urls.size > 1 + return matching_base_url_considering_x_forwarded_headers(env) || + matching_base_url_not_considering_x_forwarded_headers(env) || + default_base_url + end + default_base_url + end + + def default_base_url + base_urls.first + end + + def matching_base_url_considering_x_forwarded_headers(env) + matching_base_url(env) + end + + def matching_base_url_not_considering_x_forwarded_headers(env) + matching_base_url(env.reject{ |k, _| k =~ X_FORWARDED_PATTERN} ) + end + + def matching_base_url(env) + request_base_url = Rack::Request.new(env).base_url + if base_urls.include?(request_base_url) + request_base_url + end + end end end end diff --git a/spec/lib/rack/pact_broker/set_base_url_spec.rb b/spec/lib/rack/pact_broker/set_base_url_spec.rb new file mode 100644 index 000000000..902aa5aa2 --- /dev/null +++ b/spec/lib/rack/pact_broker/set_base_url_spec.rb @@ -0,0 +1,86 @@ +require 'rack/pact_broker/set_base_url' + +module Rack + module PactBroker + describe SetBaseUrl do + let(:base_urls) { ["http://pact-broker"] } + let(:rack_env) { {} } + let(:target_app) { double('app', call: [200, {}, []]) } + let(:app) { SetBaseUrl.new(target_app, base_urls) } + + subject { get("/", {}, rack_env) } + + describe "#call" do + context "when the base_url is already set" do + let(:rack_env) { { "pactbroker.base_url" => "http://foo"} } + + it "does not overwrite it" do + expect(target_app).to receive(:call).with(hash_including("pactbroker.base_url" => "http://foo")) + subject + end + end + + context "when there is one base URL" do + it "sets that base_url" do + expect(target_app).to receive(:call).with(hash_including("pactbroker.base_url" => "http://pact-broker")) + subject + end + end + + context "when there are no base URLs" do + let(:base_urls) { [] } + + it "sets the base URL to nil" do + expect(target_app).to receive(:call).with(hash_including("pactbroker.base_url" => nil)) + subject + end + end + + context "when there are multiple base URLs" do + let(:base_urls) { ["https://foo", "https://pact-broker-external", "http://pact-broker-internal"] } + + let(:host) { "pact-broker-internal" } + let(:scheme) { "http" } + let(:forwarded_host) { "pact-broker-external" } + let(:forwarded_scheme) { "https" } + let(:rack_env) do + { + Rack::HTTP_HOST => host, + Rack::RACK_URL_SCHEME => scheme, + "HTTP_X_FORWARDED_HOST" => forwarded_host, + "HTTP_X_FORWARDED_SCHEME" => forwarded_scheme + } + end + + context "when the base URL created taking any X-Forwarded headers into account matches one of the base URLs" do + it "uses that base URL" do + expect(target_app).to receive(:call).with(hash_including("pactbroker.base_url" => "https://pact-broker-external")) + subject + end + end + + context "when the base URL created NOT taking the X-Forwarded headers into account matches one of the base URLs (potential cache poisoning)" do + let(:forwarded_host) { "pact-broker-external-wrong" } + + it "uses that base URL" do + expect(target_app).to receive(:call).with(hash_including("pactbroker.base_url" => "http://pact-broker-internal")) + subject + end + end + + context "when neither base URL matches the base URLs (potential cache poisoning)" do + before do + rack_env["HTTP_HOST"] = "silly-buggers-1" + rack_env["HTTP_X_FORWARDED_HOST"] = "silly-buggers-1" + end + + it "uses the first base URL" do + expect(target_app).to receive(:call).with(hash_including("pactbroker.base_url" => "https://foo")) + subject + end + end + end + end + end + end +end