Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Form#redirect_signature: don't fail on empty input #46

Merged
merged 3 commits into from

2 participants

@rngtng
Collaborator

This PR fixes failure of Form#redirect_signature on empty input by forcing the secret_key to be a string.

rngtng@ce96438

In addition in an extra commit: cleaned up whitespace chars of related files

@wvanbergen
Owner

In what case should we calculate a signature without a secret key?

@rngtng
Collaborator

we had the case where the page was called without any params. It returned 500 error because of this problem. I'd like to be very defensive here and expect even a nil secretkey (which is default value anyway).

(btw. saw jruby tests failed, I'm on it..)

@wvanbergen
Owner

I prefer to raise an ArgumentError in this case, given that signing a form without a secret key is never what you want. Swallowing the error and send erroneous requests to Adyen might be even more confusing to users, causing debugging headaches.

In the end, your app should always make sure to set the secret key, and if for some reason this can be nil in your app, rescue the ArgumentError to deal with it nicely.

@rngtng
Collaborator

jruby fixed.

@rngtng
Collaborator

true ArgumentError is better solution here, I'll update it..

@wvanbergen
Owner

Cool, merging.

@wvanbergen wvanbergen merged commit 2aaffa8 into wvanbergen:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 66 additions and 43 deletions.
  1. +36 −32 lib/adyen/form.rb
  2. +30 −11 spec/form_spec.rb
View
68 lib/adyen/form.rb
@@ -2,8 +2,8 @@
module Adyen
- # The Adyen::Form module contains all functionality that is used to send payment requests
- # to the Adyen payment system, using either a HTML form (see {Adyen::Form.hidden_fields})
+ # The Adyen::Form module contains all functionality that is used to send payment requests
+ # to the Adyen payment system, using either a HTML form (see {Adyen::Form.hidden_fields})
# or a HTTP redirect (see {Adyen::Form.redirect_url}).
#
# Moreover, this module contains the method {Adyen::Form.redirect_signature_check} to
@@ -12,7 +12,7 @@ module Adyen
#
# You can use different skins in Adyen to define different payment environments. You can
# register these skins under a custom name in the module. The other methods will automatically
- # use this information (i.e. the skin code and the shared secret) if it is available.
+ # use this information (i.e. the skin code and the shared secret) if it is available.
# Otherwise, you have to provide it yourself for every method call you make. See
# {Adyen::Configuration#register_form_skin} for more information.
#
@@ -33,7 +33,7 @@ module Form
# Returns the URL of the Adyen payment system, adjusted for an Adyen environment.
#
- # @param [String] environment The Adyen environment to use. This parameter can be
+ # @param [String] environment The Adyen environment to use. This parameter can be
# left out, in which case the 'current' environment will be used.
# @return [String] The absolute URL of the Adyen payment system that can be used
# for payment forms or redirects.
@@ -62,7 +62,7 @@ def do_parameter_transformations!(parameters = {})
parameters[:order_data] = Adyen::Encoding.gzip_base64(parameters.delete(:order_data_raw)) if parameters[:order_data_raw]
parameters[:ship_before_date] = Adyen::Formatter::DateTime.fmt_date(parameters[:ship_before_date])
parameters[:session_validity] = Adyen::Formatter::DateTime.fmt_time(parameters[:session_validity])
-
+
if parameters[:skin]
skin = Adyen.configuration.form_skin_by_name(parameters.delete(:skin))
parameters[:skin_code] ||= skin[:skin_code]
@@ -73,7 +73,7 @@ def do_parameter_transformations!(parameters = {})
# Transforms the payment parameters to be in the correct format and calculates the merchant
# signature parameter. It also does some basic health checks on the parameters hash.
#
- # @param [Hash] parameters The payment parameters. The parameters set in the
+ # @param [Hash] parameters The payment parameters. The parameters set in the
# {Adyen::Configuration#default_form_params} hash will be included automatically.
# @param [String] shared_secret The shared secret that should be used to calculate
# the payment request signature. This parameter can be left if the skin that is
@@ -83,7 +83,7 @@ def do_parameter_transformations!(parameters = {})
# @raise [ArgumentError] Thrown if some parameter health check fails.
def payment_parameters(parameters = {}, shared_secret = nil)
do_parameter_transformations!(parameters)
-
+
raise ArgumentError, "Cannot generate form: :currency code attribute not found!" unless parameters[:currency_code]
raise ArgumentError, "Cannot generate form: :payment_amount code attribute not found!" unless parameters[:payment_amount]
raise ArgumentError, "Cannot generate form: :merchant_account attribute not found!" unless parameters[:merchant_account]
@@ -93,14 +93,14 @@ def payment_parameters(parameters = {}, shared_secret = nil)
shared_secret ||= parameters.delete(:shared_secret)
raise ArgumentError, "Cannot calculate payment request signature without shared secret!" unless shared_secret
parameters[:merchant_sig] = calculate_signature(parameters, shared_secret)
-
+
return parameters
end
-
+
# Returns an absolute URL to the Adyen payment system, with the payment parameters included
# as GET parameters in the URL. The URL also depends on the current Adyen enviroment.
#
- # The payment parameters that are provided to this method will be merged with the
+ # The payment parameters that are provided to this method will be merged with the
# {Adyen::Configuration#default_form_params} hash. The default parameter values will be
# overrided if another value is provided to this method.
#
@@ -108,7 +108,7 @@ def payment_parameters(parameters = {}, shared_secret = nil)
# if you provide either a registered skin name as the +:skin+ parameter or provide both the
# +:skin_code+ and +:shared_secret+ parameters.
#
- # Note that Internet Explorer has a maximum length for URLs it can handle (2083 characters).
+ # Note that Internet Explorer has a maximum length for URLs it can handle (2083 characters).
# Make sure that the URL is not longer than this limit if you want your site to work in IE.
#
# @example
@@ -117,7 +117,7 @@ def payment_parameters(parameters = {}, shared_secret = nil)
# # Genarate a URL to redirect to Adyen's payment system.
# adyen_url = Adyen::Form.redirect_url(:skin => :my_skin, :currency_code => 'USD',
# :payment_amount => 1000, merchant_account => 'MyMerchant', ... )
- #
+ #
# respond_to do |format|
# format.html { redirect_to(adyen_url) }
# end
@@ -126,14 +126,14 @@ def payment_parameters(parameters = {}, shared_secret = nil)
# @param [Hash] parameters The payment parameters to include in the payment request.
# @return [String] An absolute URL to redirect to the Adyen payment system.
def redirect_url(parameters = {})
- url + '?' + payment_parameters(parameters).map{|k,v| [k.to_s,v] }.sort.map { |(k, v)|
+ url + '?' + payment_parameters(parameters).map{|k,v| [k.to_s,v] }.sort.map { |(k, v)|
"#{camelize(k)}=#{CGI.escape(v.to_s)}" }.join('&')
end
-
- # Returns a HTML snippet of hidden INPUT tags with the provided payment parameters.
+
+ # Returns a HTML snippet of hidden INPUT tags with the provided payment parameters.
# The snippet can be included in a payment form that POSTs to the Adyen payment system.
#
- # The payment parameters that are provided to this method will be merged with the
+ # The payment parameters that are provided to this method will be merged with the
# {Adyen::Configuration#default_form_params} hash. The default parameter values will be
# overrided if another value is provided to this method.
#
@@ -152,15 +152,15 @@ def redirect_url(parameters = {})
# @return [String] An HTML snippet that can be included in a form that POSTs to the
# Adyen payment system.
def hidden_fields(parameters = {})
-
+
# Generate a hidden input tag per parameter, join them by newlines.
form_str = payment_parameters(parameters).map { |key, value|
"<input type=\"hidden\" name=\"#{CGI.escapeHTML(camelize(key))}\" value=\"#{CGI.escapeHTML(value.to_s)}\" />"
}.join("\n")
-
+
form_str.respond_to?(:html_safe) ? form_str.html_safe : form_str
end
-
+
######################################################
# MERCHANT SIGNATURE CALCULATION
######################################################
@@ -181,7 +181,7 @@ def calculate_signature_string(parameters)
parameters[:billing_address_type].to_s << parameters[:offset].to_s
end
- # Calculates the payment request signature for the given payment parameters.
+ # Calculates the payment request signature for the given payment parameters.
#
# This signature is used by Adyen to check whether the request is
# genuinely originating from you. The resulting signature should be
@@ -190,12 +190,14 @@ def calculate_signature_string(parameters)
#
# @param [Hash] parameters The payment parameters for which to calculate
# the payment request signature.
- # @param [String] shared_secret The shared secret to use for this signature.
- # It should correspond with the skin_code parameter. This parameter can be
+ # @param [String] shared_secret The shared secret to use for this signature.
+ # It should correspond with the skin_code parameter. This parameter can be
# left out if the shared_secret is included as key in the parameters.
# @return [String] The signature of the payment request
+ # @raise [ArgumentError] Thrown if shared_secret is empty
def calculate_signature(parameters, shared_secret = nil)
shared_secret ||= parameters.delete(:shared_secret)
+ raise ArgumentError, "Cannot calculate payment request signature with empty shared_secret" if shared_secret.to_s.empty?
Adyen::Encoding.hmac_base64(shared_secret, calculate_signature_string(parameters))
end
@@ -207,20 +209,22 @@ def calculate_signature(parameters, shared_secret = nil)
# @param [Hash] params A hash of HTTP GET parameters for the redirect request.
# @return [String] The signature string.
def redirect_signature_string(params)
- params[:authResult].to_s + params[:pspReference].to_s + params[:merchantReference].to_s +
+ params[:authResult].to_s + params[:pspReference].to_s + params[:merchantReference].to_s +
params[:skinCode].to_s + params[:merchantReturnData].to_s
end
-
- # Computes the redirect signature using the request parameters, so that the
+
+ # Computes the redirect signature using the request parameters, so that the
# redirect can be checked for forgery.
#
# @param [Hash] params A hash of HTTP GET parameters for the redirect request.
# @param [String] shared_secret The shared secret for the Adyen skin that was used for
- # the original payment form. You can leave this out of the skin is registered
+ # the original payment form. You can leave this out of the skin is registered
# using the {Adyen::Form.register_skin} method.
# @return [String] The redirect signature
+ # @raise [ArgumentError] Thrown if shared_secret is empty
def redirect_signature(params, shared_secret = nil)
shared_secret ||= Adyen.configuration.form_skin_shared_secret_by_code(params[:skinCode])
+ raise ArgumentError, "Cannot compute redirect signature with empty shared_secret" if shared_secret.to_s.empty?
Adyen::Encoding.hmac_base64(shared_secret, redirect_signature_string(params))
end
@@ -235,14 +239,14 @@ def redirect_signature(params, shared_secret = nil)
# @example
# class PaymentsController < ApplicationController
# before_filter :check_signature, :only => [:return_from_adyen]
- #
+ #
# def return_from_adyen
# @invoice = Invoice.find(params[:merchantReference])
# @invoice.set_paid! if params[:authResult] == 'AUTHORISED'
# end
- #
+ #
# private
- #
+ #
# def check_signature
# raise "Forgery!" unless Adyen::Form.redirect_signature_check(params)
# end
@@ -251,19 +255,19 @@ def redirect_signature(params, shared_secret = nil)
# @param [Hash] params params A hash of HTTP GET parameters for the redirect request. This
# should include the +:merchantSig+ parameter, which contains the signature.
# @param [String] shared_secret The shared secret for the Adyen skin that was used for
- # the original payment form. You can leave this out of the skin is registered
+ # the original payment form. You can leave this out of the skin is registered
# using the {Adyen::Configuration#register_form_skin} method.
# @return [true, false] Returns true only if the signature in the parameters is correct.
def redirect_signature_check(params, shared_secret = nil)
params[:merchantSig] == redirect_signature(params, shared_secret)
end
-
+
# Returns the camelized version of a string.
# @param [:to_s] identifier The identifier to turn to camelcase
# @return [String] The camelcase version of the identifier provided.
def camelize(identifier)
identifier.to_s.gsub(/_(.)/) { $1.upcase }
end
-
+
end
end
View
41 spec/form_spec.rb
@@ -35,17 +35,17 @@
it "should generate correct live url if explicitely asked for" do
Adyen::Form.url(:live).should == 'https://live.adyen.com/hpp/select.shtml'
end
-
+
it "should generate correct testing url if the payment flow selection is set to select" do
Adyen.configuration.payment_flow = :select
Adyen::Form.url.should == 'https://test.adyen.com/hpp/select.shtml'
end
-
+
it "should generate correct testing url if the payment flow selection is set to pay" do
Adyen.configuration.payment_flow = :pay
Adyen::Form.url.should == 'https://test.adyen.com/hpp/pay.shtml'
end
-
+
it "should generate correct testing url if the payment flow selection is set to details" do
Adyen.configuration.payment_flow = :details
Adyen::Form.url.should == 'https://test.adyen.com/hpp/details.shtml'
@@ -80,6 +80,18 @@
Adyen::Form.redirect_signature_check(@params).should be_true
end
+ it "should raise ArgumentError on missing skinCode" do
+ expect do
+ @params.delete(:skinCode)
+ Adyen::Form.redirect_signature_check(@params).should be_false
+ end.to raise_error ArgumentError
+ end
+
+ it "should raise ArgumentError on empty input" do
+ expect do
+ Adyen::Form.redirect_signature_check({}).should be_false
+ end.to raise_error ArgumentError
+ end
it "should detect a tampered field" do
Adyen::Form.redirect_signature_check(@params.merge(:pspReference => 'tampered')).should be_false
@@ -99,16 +111,16 @@
@redirect_url = Adyen::Form.redirect_url(@attributes)
end
-
+
it "should return an URL pointing to the adyen server" do
@redirect_url.should =~ %r[^#{Adyen::Form.url}]
end
-
+
it "should include all provided attributes" do
params = @redirect_url.split('?', 2).last.split('&').map { |param| param.split('=', 2).first }
params.should include(*(@attributes.keys.map { |k| Adyen::Form.camelize(k) }))
end
-
+
it "should include the merchant signature" do
params = @redirect_url.split('?', 2).last.split('&').map { |param| param.split('=', 2).first }
params.should include('merchantSig')
@@ -127,7 +139,7 @@
html_snippet = <<-HTML
<form action="#{CGI.escapeHTML(Adyen::Form.url)}" method="post">#{Adyen::Form.hidden_fields(@attributes)}</form>
HTML
-
+
html_snippet.should have_adyen_payment_form
end
end
@@ -148,17 +160,24 @@
it "should construct the signature base string correctly" do
signature_string = Adyen::Form.calculate_signature_string(@parameters)
signature_string.should == "10000GBP2007-10-20Internet Order 123454aD37dJATestMerchant2007-10-11T11:00:00Z"
-
+
signature_string = Adyen::Form.calculate_signature_string(@parameters.merge(:merchant_return_data => 'testing123'))
signature_string.should == "10000GBP2007-10-20Internet Order 123454aD37dJATestMerchant2007-10-11T11:00:00Ztesting123"
-
+
end
-
+
it "should calculate the signature correctly" do
signature = Adyen::Form.calculate_signature(@parameters)
signature.should == 'x58ZcRVL1H6y+XSeBGrySJ9ACVo='
end
+ it "should raise ArgumentError on empty shared_secret" do
+ expect do
+ @parameters.delete(:shared_secret)
+ signature = Adyen::Form.calculate_signature(@parameters)
+ end.to raise_error ArgumentError
+ end
+
it "should calculate the signature base string correctly for a recurring payment" do
# Add the required recurrent payment attributes
@parameters.merge!(:recurring_contract => 'DEFAULT', :shopper_reference => 'grasshopper52', :shopper_email => 'gras.shopper@somewhere.org')
@@ -175,4 +194,4 @@
signature.should == 'F2BQEYbE+EUhiRGuPtcD16Gm7JY='
end
end
-end
+end
Something went wrong with that request. Please try again.