From cbced144d56ae04c62366b02217747518302f123 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 13 Aug 2025 22:21:35 +1000 Subject: [PATCH 1/3] Clean up styling --- .../stripe/authorize_response_patcher_spec.rb | 40 +++++++++---------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/spec/lib/stripe/authorize_response_patcher_spec.rb b/spec/lib/stripe/authorize_response_patcher_spec.rb index c99b2da340..9e0c0be3ec 100644 --- a/spec/lib/stripe/authorize_response_patcher_spec.rb +++ b/spec/lib/stripe/authorize_response_patcher_spec.rb @@ -2,31 +2,29 @@ require 'spec_helper' -module Stripe - RSpec.describe AuthorizeResponsePatcher do - describe "#call!" do - let(:patcher) { Stripe::AuthorizeResponsePatcher.new(response) } - let(:params) { {} } - let(:response) { ActiveMerchant::Billing::Response.new(true, "Transaction approved", params) } +RSpec.describe Stripe::AuthorizeResponsePatcher do + describe "#call!" do + subject(:patcher) { Stripe::AuthorizeResponsePatcher.new(response) } + let(:params) { {} } + let(:response) { ActiveMerchant::Billing::Response.new(true, "Transaction approved", params) } - context "when url not found in response" do - it "does nothing" do - new_response = patcher.call! - expect(new_response).to eq response - end + context "when url not found in response" do + it "does nothing" do + new_response = patcher.call! + expect(new_response).to eq response end + end - context "when url is found in response" do - let(:params) { - { "status" => "requires_source_action", - "next_source_action" => { "type" => "authorize_with_url", - "authorize_with_url" => { "url" => "https://www.stripe.com/authorize" } } } - } + context "when url is found in response" do + let(:params) { + { "status" => "requires_source_action", + "next_source_action" => { "type" => "authorize_with_url", + "authorize_with_url" => { "url" => "https://test.stripe.com/authorize" } } } + } - it "patches response.cvv_result.message with the url in the response" do - new_response = patcher.call! - expect(new_response.cvv_result['message']).to eq "https://www.stripe.com/authorize" - end + it "patches response.cvv_result.message with the url in the response" do + new_response = patcher.call! + expect(new_response.cvv_result['message']).to eq "https://www.stripe.com/authorize" end end end From 118e18a78ee5c5a5ea4d657bee1a850c1d5e5add Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 13 Aug 2025 22:27:42 +1000 Subject: [PATCH 2/3] Tighten url validation Per recommendation from https://github.com/openfoodfoundation/openfoodnetwork/security/code-scanning/241 --- lib/stripe/authorize_response_patcher.rb | 3 ++- .../stripe/authorize_response_patcher_spec.rb | 27 ++++++++++++++++--- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/lib/stripe/authorize_response_patcher.rb b/lib/stripe/authorize_response_patcher.rb index 4128bf6577..06e8fde247 100644 --- a/lib/stripe/authorize_response_patcher.rb +++ b/lib/stripe/authorize_response_patcher.rb @@ -28,7 +28,8 @@ module Stripe return unless %w(authorize_with_url redirect_to_url).include?(next_action_type) url = next_action[next_action_type]["url"] - url if url.match(%r{https?://\S+}) && url.include?("stripe.com") + host = URI(url).host + url if url.match(%r{https?://\S+}) && host.match?(/\S?stripe.com\Z/) end # This field is used because the Spree code recognizes and stores it diff --git a/spec/lib/stripe/authorize_response_patcher_spec.rb b/spec/lib/stripe/authorize_response_patcher_spec.rb index 9e0c0be3ec..395dac7d60 100644 --- a/spec/lib/stripe/authorize_response_patcher_spec.rb +++ b/spec/lib/stripe/authorize_response_patcher_spec.rb @@ -17,15 +17,36 @@ RSpec.describe Stripe::AuthorizeResponsePatcher do context "when url is found in response" do let(:params) { - { "status" => "requires_source_action", - "next_source_action" => { "type" => "authorize_with_url", - "authorize_with_url" => { "url" => "https://test.stripe.com/authorize" } } } + { + "status" => "requires_source_action", + "next_source_action" => { + "type" => "authorize_with_url", + "authorize_with_url" => { "url" => "https://www.stripe.com/authorize" } + } + } } it "patches response.cvv_result.message with the url in the response" do new_response = patcher.call! expect(new_response.cvv_result['message']).to eq "https://www.stripe.com/authorize" end + + context "with invalid url containing 'stripe.com'" do + let(:params) { + { + "status" => "requires_source_action", + "next_source_action" => { + "type" => "authorize_with_url", + "authorize_with_url" => { "url" => "https://www.stripe.com.malicious.org/authorize" } + } + } + } + + it "patches response.cvv_result.message with nil" do + new_response = patcher.call! + expect(new_response.cvv_result['message']).to be_nil + end + end end end end From 1f15f094ce90c19280c62c23cfc9021ddf16fa80 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 8 Sep 2025 11:00:11 +1000 Subject: [PATCH 3/3] Per review, check the URL is from a stripe subdomain --- lib/stripe/authorize_response_patcher.rb | 6 +++--- spec/lib/stripe/authorize_response_patcher_spec.rb | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/stripe/authorize_response_patcher.rb b/lib/stripe/authorize_response_patcher.rb index 06e8fde247..5586718949 100644 --- a/lib/stripe/authorize_response_patcher.rb +++ b/lib/stripe/authorize_response_patcher.rb @@ -27,9 +27,9 @@ module Stripe next_action_type = next_action["type"] return unless %w(authorize_with_url redirect_to_url).include?(next_action_type) - url = next_action[next_action_type]["url"] - host = URI(url).host - url if url.match(%r{https?://\S+}) && host.match?(/\S?stripe.com\Z/) + url = URI(next_action[next_action_type]["url"]) + # Check the URL is from a stripe subdomain + url.to_s if url.is_a?(URI::HTTPS) && url.host.match?(/\.stripe.com\Z/) end # This field is used because the Spree code recognizes and stores it diff --git a/spec/lib/stripe/authorize_response_patcher_spec.rb b/spec/lib/stripe/authorize_response_patcher_spec.rb index 395dac7d60..3eb2f2e626 100644 --- a/spec/lib/stripe/authorize_response_patcher_spec.rb +++ b/spec/lib/stripe/authorize_response_patcher_spec.rb @@ -37,7 +37,7 @@ RSpec.describe Stripe::AuthorizeResponsePatcher do "status" => "requires_source_action", "next_source_action" => { "type" => "authorize_with_url", - "authorize_with_url" => { "url" => "https://www.stripe.com.malicious.org/authorize" } + "authorize_with_url" => { "url" => "https://www.evil-stripe.com.malicious.org/authorize" } } } }