From e92ffcb6006396cf5293b3b143adb401041b753e Mon Sep 17 00:00:00 2001 From: Oleg Valter Date: Sun, 6 Sep 2026 01:24:22 +0300 Subject: [PATCH 1/7] Clean up donations controller --- app/controllers/donations_controller.rb | 57 ++++++++++++++++++------- 1 file changed, 41 insertions(+), 16 deletions(-) diff --git a/app/controllers/donations_controller.rb b/app/controllers/donations_controller.rb index ec63fd3e9..202106b2d 100644 --- a/app/controllers/donations_controller.rb +++ b/app/controllers/donations_controller.rb @@ -30,8 +30,10 @@ def intent # amount * 100 because Stripe takes amounts in pence @amount = amount - @intent = Stripe::PaymentIntent.create({ amount: (amount * 100).to_i, currency: @currency, - metadata: { user_id: current_user&.id }, description: params[:desc] }, + @intent = Stripe::PaymentIntent.create({ amount: (amount * 100).to_i, + currency: @currency, + metadata: { user_id: current_user&.id }, + description: params[:desc] }, { idempotency_key: params[:authenticity_token] }) end @@ -39,11 +41,16 @@ def success @amount = params[:amount] @symbol = params[:currency] @referrer = params[:return_to] + Stripe::PaymentIntent.update(params[:intent], { metadata: { public_name: params[:public_name], public_comment: params[:public_comments] } }) - DonationMailer.with(amount: @amount, currency: @symbol, email: params[:billing_email], + + DonationMailer.with(amount: @amount, + currency: @symbol, + email: params[:billing_email], name: current_user&.username || params[:billing_name]) - .donation_successful.deliver_now + .donation_successful + .deliver_now end def callback @@ -56,20 +63,24 @@ def callback rescue JSON::ParserError respond_to do |format| format.json do - render status: 400, json: { error: 'Check yo JSON syntax. Fam.' } + render status: :bad_request, + json: { error: 'Check yo JSON syntax. Fam.' } end format.any do - render status: 400, plain: 'Check yo JSON syntax. Fam.' + render status: :bad_request, + plain: 'Check yo JSON syntax. Fam.' end end return rescue Stripe::SignatureVerificationError respond_to do |format| format.json do - render status: 400, json: { error: "You're not Stripe. Go away." } + render status: :bad_request, + json: { error: "You're not Stripe. Go away." } end format.any do - render status: 400, plain: "You're not Stripe. Go away." + render status: :bad_request, + plain: "You're not Stripe. Go away." end end return @@ -78,10 +89,12 @@ def callback if event.nil? respond_to do |format| format.json do - render status: 500, json: { error: 'Webhook event not created. ???' } + render status: :internal_server_error, + json: { error: 'Webhook event not created. ???' } end format.any do - render status: 500, plain: 'Webhook event not created. ???' + render status: :internal_server_error, + plain: 'Webhook event not created. ???' end end return @@ -92,16 +105,28 @@ def callback if StripeEventProcessor.respond_to?(method) begin result = StripeEventProcessor.send(method, object, event) - render status: 200, json: { status: 'Accepted for processing.', result: result } + + render status: :success, + json: { status: 'Accepted for processing.', result: result } rescue Stripe::StripeError => e error_id = SecureRandom.uuid - ErrorLog.create(community: RequestContext.community, user: current_user, klass: e&.class, - message: e&.message, backtrace: e&.backtrace&.join("\n"), request_uri: request.original_url, - host: request.raw_host_with_port, uuid: error_id, user_agent: request.user_agent) - render status: 500, json: { error: "#{e&.class}: #{error_id} created." } + + ErrorLog.create(community: RequestContext.community, + user: current_user, + klass: e&.class, + message: e&.message, + backtrace: e&.backtrace&.join("\n"), + request_uri: request.original_url, + host: request.raw_host_with_port, + uuid: error_id, + user_agent: request.user_agent) + + render status: :internal_server_error, + json: { error: "#{e&.class}: #{error_id} created." } end else - render status: 202, json: { status: 'Accepted, not processed.' } + render status: :accepted, + json: { status: 'Accepted, not processed.' } end end end From 22adc4ff8a0027adc1e96a382bbe1d28403cf689 Mon Sep 17 00:00:00 2001 From: Oleg Valter Date: Sun, 6 Sep 2026 03:33:15 +0300 Subject: [PATCH 2/7] Fix runtime error in :intent due to typo in local var name Only triggers for donations less than 0.5 --- app/controllers/donations_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/donations_controller.rb b/app/controllers/donations_controller.rb index 202106b2d..9868a856a 100644 --- a/app/controllers/donations_controller.rb +++ b/app/controllers/donations_controller.rb @@ -22,7 +22,7 @@ def intent end if amount < 0.50 - flash[:danger] = "Sorry, we can't accept amounts below #{symbol}0.50. We appreciate your generosity, but the " \ + flash[:danger] = "Sorry, we can't accept amounts below #{@symbol}0.50. We appreciate your generosity, but the " \ 'processing fees make it prohibitive.' redirect_to donate_path return From ecbe3171da1e5b9d687fffbfb5c7c0ca97f6a8ce Mon Sep 17 00:00:00 2001 From: Oleg Valter Date: Sun, 6 Sep 2026 03:33:41 +0300 Subject: [PATCH 3/7] Add UriHelper for URI-specific controller utilities --- app/helpers/uri_helper.rb | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 app/helpers/uri_helper.rb diff --git a/app/helpers/uri_helper.rb b/app/helpers/uri_helper.rb new file mode 100644 index 000000000..9792da174 --- /dev/null +++ b/app/helpers/uri_helper.rb @@ -0,0 +1,24 @@ +module UriHelper + [:http, :https].each do |method| + # Does a given URI have the relevant scheme? + # @param {String} uri URI to check + # @return {Boolean} check result + define_method "#{method}_uri?" do |uri| + URI(uri).scheme.casecmp(method.to_s).zero? + end + end + + # Is a given URI a relative one? + # @param {String} uri URI to check + # @return {Boolean} check result + def relative_uri?(uri) + URI(uri).relative? + end + + # Is a given URI a safe one (absolute http://, https://, or relative)? + # @param {String} uri URI to check + # @return {Boolean} check result + def safe_uri?(uri) + relative_uri?(uri) || http_uri?(uri) || https_uri?(uri) + end +end From e9820fa04721bb8e6c12140ebd5043df2753b1b3 Mon Sep 17 00:00:00 2001 From: Oleg Valter Date: Sun, 6 Sep 2026 03:34:59 +0300 Subject: [PATCH 4/7] Make return_to donations controller param safe Only HTTP, HTTP(S), or relative URIs are allowed --- app/controllers/donations_controller.rb | 19 +++++++--- test/controllers/donations_controller_test.rb | 36 ++++++++++++++++++- 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/app/controllers/donations_controller.rb b/app/controllers/donations_controller.rb index 9868a856a..13c3a7c6f 100644 --- a/app/controllers/donations_controller.rb +++ b/app/controllers/donations_controller.rb @@ -1,15 +1,17 @@ class DonationsController < ApplicationController layout 'stripe' + skip_forgery_protection only: [:callback] skip_before_action :set_globals, only: [:callback] skip_before_action :check_if_warning_or_suspension_pending, only: [:callback] skip_before_action :stop_the_awful_troll, only: [:callback] skip_before_action :distinguish_fake_community, only: [:callback] + before_action :set_referrer, only: [:intent, :success] + def index; end def intent - @referrer = params[:return_to] currencies = ['GBP', 'USD', 'EUR'] @currency = currencies.include?(params[:currency]) ? params[:currency] : 'GBP' @symbol = { 'GBP' => '£', 'USD' => '$', 'EUR' => '€' }[@currency] @@ -40,7 +42,6 @@ def intent def success @amount = params[:amount] @symbol = params[:currency] - @referrer = params[:return_to] Stripe::PaymentIntent.update(params[:intent], { metadata: { public_name: params[:public_name], public_comment: params[:public_comments] } }) @@ -64,11 +65,11 @@ def callback respond_to do |format| format.json do render status: :bad_request, - json: { error: 'Check yo JSON syntax. Fam.' } + json: { error: 'Check your JSON syntax.' } end format.any do render status: :bad_request, - plain: 'Check yo JSON syntax. Fam.' + plain: 'Check your JSON syntax.' end end return @@ -129,4 +130,14 @@ def callback json: { status: 'Accepted, not processed.' } end end + + private + + def set_referrer + return_to = params[:return_to] + + return unless return_to.present? && helpers.safe_uri?(return_to) + + @referrer = return_to + end end diff --git a/test/controllers/donations_controller_test.rb b/test/controllers/donations_controller_test.rb index e675e2f26..5666dd864 100644 --- a/test/controllers/donations_controller_test.rb +++ b/test/controllers/donations_controller_test.rb @@ -16,11 +16,45 @@ class DonationsControllerTest < ActionController::TestCase assert_response(:success) end - test 'should create PaymentIntent' do + test ':intent should safely handle referrer URIs' do + Stripe::PaymentIntent.stub(:create, nil) do + referrer_test_cases.each do |test_case| + get :intent, params: { return_to: test_case.first } + assert_equal test_case.second, assigns(:referrer).present? + end + end + end + + test ':success should safely handle referrer URIs' do + Stripe::PaymentIntent.stub(:update, true) do + referrer_test_cases.each do |test_case| + get :success, params: { + billing_email: 'donator@example.com', + billing_name: 'donations_tester', + return_to: test_case.first + } + assert_equal test_case.second, assigns(:referrer).present? + end + end + end + + test ':intent should correctly create PaymentIntent' do skip unless Stripe.api_key post :intent, params: { currency: 'EUR', amount: '24.99', desc: 'Created from Rails test' } assert_response(:success) assert_not_nil assigns(:intent)&.id end + + private + + def referrer_test_cases + [ + ["http://example.com/qa", true], + ["https://example.com/qa", true], + ["/relative_path", true], + ["javascript:alert('oops!')", false], + ["ftp://example.com/donwload", false], + ] + end end From f54c3260778e1b456b054395601488c9bd743bfe Mon Sep 17 00:00:00 2001 From: Oleg Valter Date: Sun, 6 Sep 2026 03:42:59 +0300 Subject: [PATCH 5/7] Rubocop fixes --- app/controllers/donations_controller.rb | 72 +++++++++++-------- test/controllers/donations_controller_test.rb | 8 +-- 2 files changed, 46 insertions(+), 34 deletions(-) diff --git a/app/controllers/donations_controller.rb b/app/controllers/donations_controller.rb index 13c3a7c6f..c616f55f0 100644 --- a/app/controllers/donations_controller.rb +++ b/app/controllers/donations_controller.rb @@ -62,42 +62,15 @@ def callback begin event = Stripe::Webhook.construct_event(payload, signature, secret) rescue JSON::ParserError - respond_to do |format| - format.json do - render status: :bad_request, - json: { error: 'Check your JSON syntax.' } - end - format.any do - render status: :bad_request, - plain: 'Check your JSON syntax.' - end - end + respond_to_invalid_json return rescue Stripe::SignatureVerificationError - respond_to do |format| - format.json do - render status: :bad_request, - json: { error: "You're not Stripe. Go away." } - end - format.any do - render status: :bad_request, - plain: "You're not Stripe. Go away." - end - end + respond_to_signature_error return end if event.nil? - respond_to do |format| - format.json do - render status: :internal_server_error, - json: { error: 'Webhook event not created. ???' } - end - format.any do - render status: :internal_server_error, - plain: 'Webhook event not created. ???' - end - end + respond_to_missing_event return end @@ -133,6 +106,45 @@ def callback private + def respond_to_invalid_json + respond_to do |format| + format.json do + render status: :bad_request, + json: { error: 'Check your JSON syntax.' } + end + format.any do + render status: :bad_request, + plain: 'Check your JSON syntax.' + end + end + end + + def respond_to_missing_event + respond_to do |format| + format.json do + render status: :internal_server_error, + json: { error: 'Webhook event not created. ???' } + end + format.any do + render status: :internal_server_error, + plain: 'Webhook event not created. ???' + end + end + end + + def respond_to_signature_error + respond_to do |format| + format.json do + render status: :bad_request, + json: { error: "You're not Stripe. Go away." } + end + format.any do + render status: :bad_request, + plain: "You're not Stripe. Go away." + end + end + end + def set_referrer return_to = params[:return_to] diff --git a/test/controllers/donations_controller_test.rb b/test/controllers/donations_controller_test.rb index 5666dd864..665b87527 100644 --- a/test/controllers/donations_controller_test.rb +++ b/test/controllers/donations_controller_test.rb @@ -50,11 +50,11 @@ class DonationsControllerTest < ActionController::TestCase def referrer_test_cases [ - ["http://example.com/qa", true], - ["https://example.com/qa", true], - ["/relative_path", true], + ['http://example.com/qa', true], + ['https://example.com/qa', true], + ['/relative_path', true], ["javascript:alert('oops!')", false], - ["ftp://example.com/donwload", false], + ['ftp://example.com/donwload', false] ] end end From 6a61f01199c0b465f16f4afa4ba4610bfce2c255 Mon Sep 17 00:00:00 2001 From: Oleg Valter Date: Sun, 6 Sep 2026 04:49:11 +0300 Subject: [PATCH 6/7] Add test for donations callback missing event handling JSON formatting only for now --- app/controllers/donations_controller.rb | 4 ++-- test/controllers/donations_controller_test.rb | 13 +++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/app/controllers/donations_controller.rb b/app/controllers/donations_controller.rb index c616f55f0..3f05c1d98 100644 --- a/app/controllers/donations_controller.rb +++ b/app/controllers/donations_controller.rb @@ -123,11 +123,11 @@ def respond_to_missing_event respond_to do |format| format.json do render status: :internal_server_error, - json: { error: 'Webhook event not created. ???' } + json: { error: 'Webhook event not created.' } end format.any do render status: :internal_server_error, - plain: 'Webhook event not created. ???' + plain: 'Webhook event not created.' end end end diff --git a/test/controllers/donations_controller_test.rb b/test/controllers/donations_controller_test.rb index 665b87527..00fabdf05 100644 --- a/test/controllers/donations_controller_test.rb +++ b/test/controllers/donations_controller_test.rb @@ -46,6 +46,19 @@ class DonationsControllerTest < ActionController::TestCase assert_not_nil assigns(:intent)&.id end + test ':callback should correctly handle missing events' do + Stripe::Webhook.stub(:construct_event, nil) do + post :callback, params: { format: :json } + + assert_response(:internal_server_error) + assert_nothing_raised do + parsed = JSON.parse(response.body) + assert_not_nil(parsed) + assert_equal 'Webhook event not created.', parsed['error'] + end + end + end + private def referrer_test_cases From 2295bc2eaaffa21d72b66c8633da185f6f1395ac Mon Sep 17 00:00:00 2001 From: Oleg Valter Date: Sun, 6 Sep 2026 05:01:40 +0300 Subject: [PATCH 7/7] Add test for donations callback signature error handling --- test/controllers/donations_controller_test.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/controllers/donations_controller_test.rb b/test/controllers/donations_controller_test.rb index 00fabdf05..93fc88f19 100644 --- a/test/controllers/donations_controller_test.rb +++ b/test/controllers/donations_controller_test.rb @@ -46,6 +46,19 @@ class DonationsControllerTest < ActionController::TestCase assert_not_nil assigns(:intent)&.id end + test ':callback should correctly handle signature errors' do + @request.set_header('Stripe-Signature', 'invalid') + + post :callback, params: { format: :json } + + assert_response(:bad_request) + assert_nothing_raised do + parsed = JSON.parse(response.body) + assert_not_nil(parsed) + assert_equal "You're not Stripe. Go away.", parsed['error'] + end + end + test ':callback should correctly handle missing events' do Stripe::Webhook.stub(:construct_event, nil) do post :callback, params: { format: :json }