diff --git a/Gemfile b/Gemfile index 07235d3d500..0147b491349 100644 --- a/Gemfile +++ b/Gemfile @@ -201,6 +201,7 @@ gem "nokogiri", "~> 1.19.1" gem "carrierwave", "~> 2.2.6" gem "carrierwave_direct", "~> 3.0.0" +gem "ssrf_filter", "~> 1.3" gem "fog-aws" gem "aws-sdk-core", "~> 3.241" diff --git a/Gemfile.lock b/Gemfile.lock index 78c9dd2e86b..9ab34165d48 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1753,6 +1753,7 @@ DEPENDENCIES spring-commands-rubocop sprockets (~> 3.7.2) sprockets-rails (~> 3.5.1) + ssrf_filter (~> 1.3) stackprof statesman (~> 13.1.0) store_attribute (~> 2.0) diff --git a/modules/webhooks/spec/services/webhooks/outgoing/request_webhook_service_spec.rb b/modules/webhooks/spec/services/webhooks/outgoing/request_webhook_service_spec.rb index 3ad13e89f60..1d826303b63 100644 --- a/modules/webhooks/spec/services/webhooks/outgoing/request_webhook_service_spec.rb +++ b/modules/webhooks/spec/services/webhooks/outgoing/request_webhook_service_spec.rb @@ -43,7 +43,7 @@ RSpec.describe Webhooks::Outgoing::RequestWebhookService, :webmock, type: :model describe "#call!" do context "when the request is successful" do before do - stub_request(:post, ssrf_resolved_url(webhook.url)) + stub_request(:post, webhook.url) .with(body: "body", headers: { "X-Custom" => "header" }) .to_return(status: 200, body: "OK", headers: { "Content-Type" => "application/json" }) end @@ -52,7 +52,7 @@ RSpec.describe Webhooks::Outgoing::RequestWebhookService, :webmock, type: :model it "makes a POST request to the webhook URL with the given body and headers" do subject - expect(WebMock).to have_requested(:post, ssrf_resolved_url(webhook.url)) + expect(WebMock).to have_requested(:post, webhook.url) .with(body: "body", headers: { "X-Custom" => "header", "Host" => "example.net" }).once end @@ -67,11 +67,24 @@ RSpec.describe Webhooks::Outgoing::RequestWebhookService, :webmock, type: :model expect(log.response_body).to eq("OK") expect(log.url).to eq(webhook.url) end + + it "connects to the original hostname while routing through the resolved safe IP address" do + http_start_args = nil + allow(Net::HTTP).to receive(:start).and_wrap_original do |original, *args, **kwargs, &block| + http_start_args = { host: args[0], options: kwargs } + original.call(*args, **kwargs, &block) + end + + subject + + expect(http_start_args[:host]).to eq("example.net") + expect(http_start_args[:options]).to include(ipaddr: WithSsrfWebhookStubsMixin::SSRF_TEST_IP) + end end context "when the request times out" do before do - stub_request(:post, ssrf_resolved_url(webhook.url)).to_timeout + stub_request(:post, webhook.url).to_timeout end it "re-raises the timeout error while still creating a log entry" do @@ -83,7 +96,7 @@ RSpec.describe Webhooks::Outgoing::RequestWebhookService, :webmock, type: :model context "when request_url fails with SSL errors" do before do - stub_request(:post, ssrf_resolved_url(webhook.url)).to_raise(OpenSSL::SSL::SSLError) + stub_request(:post, webhook.url).to_raise(OpenSSL::SSL::SSLError) end it "still logs the exception" do @@ -135,7 +148,7 @@ RSpec.describe Webhooks::Outgoing::RequestWebhookService, :webmock, type: :model context "when an unexpected error occurs" do before do - stub_request(:post, ssrf_resolved_url(webhook.url)).to_raise(StandardError.new("something went wrong")) + stub_request(:post, webhook.url).to_raise(StandardError.new("something went wrong")) end it "creates a log entry" do diff --git a/modules/webhooks/spec/workers/attachment_webhook_job_spec.rb b/modules/webhooks/spec/workers/attachment_webhook_job_spec.rb index 63f9885e99f..04b0d8366b6 100644 --- a/modules/webhooks/spec/workers/attachment_webhook_job_spec.rb +++ b/modules/webhooks/spec/workers/attachment_webhook_job_spec.rb @@ -52,7 +52,7 @@ RSpec.describe AttachmentWebhookJob, :webmock, type: :job do end let(:stub) do - stub_request(:post, ssrf_resolved_url(stubbed_url)) + stub_request(:post, stubbed_url) .with( body: hash_including( "action" => event, diff --git a/modules/webhooks/spec/workers/project_webhook_job_spec.rb b/modules/webhooks/spec/workers/project_webhook_job_spec.rb index 63813ee0136..5bfc2864c65 100644 --- a/modules/webhooks/spec/workers/project_webhook_job_spec.rb +++ b/modules/webhooks/spec/workers/project_webhook_job_spec.rb @@ -56,7 +56,7 @@ RSpec.describe ProjectWebhookJob, :webmock, type: :job do end let(:stub) do - stub_request(:post, ssrf_resolved_url(stubbed_url)) + stub_request(:post, stubbed_url) .with( body: hash_including( "action" => event, diff --git a/modules/webhooks/spec/workers/time_entry_webhook_job_spec.rb b/modules/webhooks/spec/workers/time_entry_webhook_job_spec.rb index a99a56b546e..affb68d46a8 100644 --- a/modules/webhooks/spec/workers/time_entry_webhook_job_spec.rb +++ b/modules/webhooks/spec/workers/time_entry_webhook_job_spec.rb @@ -53,7 +53,7 @@ RSpec.describe TimeEntryWebhookJob, :webmock, type: :job do end let(:stub) do - stub_request(:post, ssrf_resolved_url(stubbed_url)) + stub_request(:post, stubbed_url) .with( body: hash_including( "action" => event, diff --git a/modules/webhooks/spec/workers/work_package_comment_webhook_job_spec.rb b/modules/webhooks/spec/workers/work_package_comment_webhook_job_spec.rb index 43908098b1b..843c9cd4bcd 100644 --- a/modules/webhooks/spec/workers/work_package_comment_webhook_job_spec.rb +++ b/modules/webhooks/spec/workers/work_package_comment_webhook_job_spec.rb @@ -53,7 +53,7 @@ RSpec.describe WorkPackageCommentWebhookJob, :webmock, type: :model do end let(:stub) do - stub_request(:post, ssrf_resolved_url(stubbed_url)).with( + stub_request(:post, stubbed_url).with( body: hash_including( "action" => event_name, "activity" => hash_including( diff --git a/modules/webhooks/spec/workers/work_package_webhook_job_spec.rb b/modules/webhooks/spec/workers/work_package_webhook_job_spec.rb index d38f97b707b..40c6eff15af 100644 --- a/modules/webhooks/spec/workers/work_package_webhook_job_spec.rb +++ b/modules/webhooks/spec/workers/work_package_webhook_job_spec.rb @@ -56,7 +56,7 @@ RSpec.describe WorkPackageWebhookJob, :webmock, type: :model do end let(:stub) do - stub_request(:post, ssrf_resolved_url(stubbed_url)) + stub_request(:post, stubbed_url) .with( body: hash_including( "action" => event, diff --git a/spec/support/shared/with_ssrf_webhook_stubs.rb b/spec/support/shared/with_ssrf_webhook_stubs.rb index 2114f66bddf..23bb7590060 100644 --- a/spec/support/shared/with_ssrf_webhook_stubs.rb +++ b/spec/support/shared/with_ssrf_webhook_stubs.rb @@ -29,23 +29,11 @@ module WithSsrfWebhookStubsMixin ## # A safe public IP returned by the stubbed resolver for any hostname. - # It is not in SsrfFilter's private-address blocklist, so SSRF validation passes, - # and WebMock stubs using this IP will match the actual Net::HTTP request. + # It is not in SsrfFilter's private-address blocklist, so SSRF validation passes. + # ssrf_filter 1.3+ makes requests to the original hostname URL (not the resolved IP), + # passing the resolved IP via the `ipaddr:` option to Net::HTTP.start instead. SSRF_TEST_IP = "93.184.216.34" - ## - # Translates a webhook URL containing a hostname to the IP-based URL that - # SsrfFilter will use when making the actual HTTP request. Use this when - # setting up WebMock stubs so that they match the resolved request. - # - # URLs that already contain an IP address are returned unchanged. - def ssrf_resolved_url(url) - uri = URI.parse(url) - return url if ip_address?(uri.host) - - url.sub(uri.host, SSRF_TEST_IP) - end - def ip_address?(host) [Resolv::IPv4::Regex, Resolv::IPv6::Regex].any? { host.match? it } end