security: Validate user submitted payment methods

Previously a malicious user could craft a request to use payment methods
that were not intended to be available. This includes:
* Payment methods with available_to_users=false (called display_on
  before Solidus 2.1)
* Soft-deleted payment methods
* Payment methods intended only for certain stores (a Solidus 1.1+
  feature)
* Specifying no payment method (results in payment_method being nil)

This has been mitigated by validating the payment method specified by
the user, and raising RecordNotFound if the payment method shouldn't be
accessible.

api/app/controllers/spree/api/payments_controller.rb

@@ -16,6 +16,7 @@ def new
  end
 
  def create
+ @order.validate_payments_attributes(payment_params)
  @payment = PaymentCreate.new(@order, payment_params).build
  if @payment.save
  respond_with(@payment, status: 201, default_template: :show)

core/app/models/spree/order.rb

@@ -748,6 +748,23 @@ def record_ip_address(ip_address)
  end
  end
 
+ def payments_attributes=(attributes)
+ validate_payments_attributes(attributes)
+ super(attributes)
+ end
+
+ def validate_payments_attributes(attributes)
+ attributes = Array(attributes)
+ # Ensure the payment methods specified are allowed for this user
+ payment_methods = Spree::PaymentMethod.where(id: available_payment_methods)
+ attributes.each do |payment_attributes|
+ payment_method_id = payment_attributes[:payment_method_id]
+
+ # raise RecordNotFound unless it is an allowed payment method
+ payment_methods.find(payment_method_id) if payment_method_id
+ end
+ end
+
  private
 
  def process_payments_before_complete

core/app/models/spree/order_update_attributes.rb

@@ -14,6 +14,8 @@ def initialize(order, attributes, request_env: nil)
  # Assign the attributes to the order and save the order
  # @return true if saved, otherwise false and errors will be set on the order
  def apply
+ order.validate_payments_attributes(@payments_attributes)
+
  assign_order_attributes
  assign_payments_attributes
 

core/app/models/spree/payment.rb

@@ -40,6 +40,7 @@ class Payment < Spree::Base
 
  validates :amount, numericality: true
  validates :source, presence: true, if: :source_required?
+ validates :payment_method, presence: true
 
  default_scope -> { order(:created_at) }
 

core/spec/models/spree/order_spec.rb

@@ -1534,4 +1534,66 @@ def generate
  end
  end
  end
+
+ context 'update_params_payment_source' do
+ subject { described_class.new }
+
+ it 'is deprecated' do
+ subject.instance_variable_set('@updating_params', {})
+ expect(Spree::Deprecation).to receive(:warn)
+ subject.update_params_payment_source
+ end
+ end
+
+ describe "#validate_payments_attributes" do
+ let(:attributes) { [ActionController::Parameters.new(payment_method_id: payment_method.id)] }
+ subject do
+ order.validate_payments_attributes(attributes)
+ end
+
+ context "with empty array" do
+ let(:attributes) { [] }
+ it "doesn't error" do
+ subject
+ end
+ end
+
+ context "with no payment method specified" do
+ let(:attributes) { [ActionController::Parameters.new({})] }
+ it "doesn't error" do
+ subject
+ end
+ end
+
+ context "with valid payment method" do
+ let(:payment_method) { create(:check_payment_method) }
+ it "doesn't error" do
+ subject
+ end
+ end
+
+ context "with inactive payment method" do
+ let(:payment_method) { create(:check_payment_method, active: false) }
+
+ it "raises RecordNotFound" do
+ expect { subject }.to raise_error(ActiveRecord::RecordNotFound)
+ end
+ end
+
+ context "with unavailable payment method" do
+ let(:payment_method) { create(:check_payment_method, available_to_users: false) }
+
+ it "raises RecordNotFound" do
+ expect { subject }.to raise_error(ActiveRecord::RecordNotFound)
+ end
+ end
+
+ context "with soft-deleted payment method" do
+ let(:payment_method) { create(:check_payment_method, deleted_at: Time.current) }
+
+ it "raises RecordNotFound" do
+ expect { subject }.to raise_error(ActiveRecord::RecordNotFound)
+ end
+ end
+ end
 end

core/spec/models/spree/order_update_attributes_spec.rb

@@ -3,6 +3,7 @@
 module Spree
  RSpec.describe OrderUpdateAttributes do
  let(:order) { create(:order) }
+ let(:payment_method) { create(:payment_method) }
  let(:request_env) { nil }
  let(:update) { described_class.new(order, attributes, request_env: request_env) }
 
@@ -25,7 +26,10 @@ module Spree
  let(:attributes) do
  {
  payments_attributes: [
- { source_attributes: attributes_for(:credit_card) }
+ {
+ payment_method_id: payment_method.id,
+ source_attributes: attributes_for(:credit_card)
+ }
  ]
  }
  end

core/spec/models/spree/payment_spec.rb

@@ -653,12 +653,13 @@
  end
 
  context "completed orders" do
+ let(:payment_method) { create(:check_payment_method) }
  before { allow(order).to receive_messages completed?: true }
 
  it "updates payment_state and shipments" do
  expect(order.updater).to receive(:update_payment_state)
  expect(order.updater).to receive(:update_shipment_state)
- Spree::Payment.create(amount: 100, order: order)
+ Spree::Payment.create!(amount: 100, order: order, payment_method: payment_method)
  end
  end
 
@@ -742,12 +743,13 @@
  end
 
  describe "invalidating payments updates in memory objects" do
+ let(:payment_method) { create(:check_payment_method) }
  before do
- Spree::PaymentCreate.new(order, amount: 1).build.save!
+ Spree::PaymentCreate.new(order, amount: 1, payment_method_id: payment_method.id).build.save!
  expect(order.payments.map(&:state)).to contain_exactly(
  'checkout'
  )
- Spree::PaymentCreate.new(order, amount: 2).build.save!
+ Spree::PaymentCreate.new(order, amount: 2, payment_method_id: payment_method.id).build.save!
  end
 
  it 'should not have stale payments' do

frontend/spec/controllers/spree/checkout_controller_spec.rb

@@ -81,9 +81,9 @@ def post_address
  }
  end
 
+ let!(:payment_method) { create(:payment_method) }
  before do
  # Must have *a* shipping method and a payment method so updating from address works
- allow(order).to receive_messages available_payment_methods: [stub_model(Spree::PaymentMethod)]
  allow(order).to receive_messages ensure_available_shipping_rates: true
  order.line_items << FactoryGirl.create(:line_item)
  end

frontend/spec/features/checkout_spec.rb

@@ -211,9 +211,9 @@
 
  # Regression test for https://github.com/spree/spree/issues/2694 and https://github.com/spree/spree/issues/4117
  context "doesn't allow bad credit card numbers" do
+ let!(:payment_method) { create(:credit_card_payment_method) }
  before(:each) do
  order = OrderWalkthrough.up_to(:delivery)
- allow(order).to receive_messages(available_payment_methods: [create(:credit_card_payment_method)])
 
  user = create(:user)
  order.user = user
@@ -314,7 +314,8 @@
  end
 
  context "user has payment sources", js: true do
- let(:bogus) { create(:credit_card_payment_method) }
+ before { Spree::PaymentMethod.all.each(&:really_destroy!) }
+ let!(:bogus) { create(:credit_card_payment_method) }
  let(:user) { create(:user) }
 
  let!(:credit_card) do
@@ -324,7 +325,6 @@
  before do
  user.wallet.add(credit_card)
  order = OrderWalkthrough.up_to(:delivery)
- allow(order).to receive_messages(available_payment_methods: [bogus])
 
  allow_any_instance_of(Spree::CheckoutController).to receive_messages(current_order: order)
  allow_any_instance_of(Spree::CheckoutController).to receive_messages(try_spree_current_user: user)