Bug #60189 for Net-OAuth: Bug 60154: Repeated parameters incorrectly handled - patch and test cases

Please find below a patch against Net-OAuth-0.27 in unified diff format, including test cases. --- lib/Net/OAuth.old.pm 2010-08-09 12:43:51.000000000 +0100 +++ lib/Net/OAuth.pm 2010-08-09 12:43:17.000000000 +0100 @@ -243,7 +243,9 @@ =head2 API PARAMETERS vs MESSAGE PARAMETERS -Net::OAuth defines 'message parameters' as parameters that are part of the transmitted OAuth message. These include any protocol parameter (prefixed with 'oauth_' in the message), and any additional message parameters (the extra_params hash). +Net::OAuth defines 'message parameters' as parameters that are part of the transmitted OAuth message. These include any protocol parameter (prefixed with 'oauth_' in the message), and any additional message parameters (the extra_params hash). Multiple values can be specified for the same message parameter by supplying an arrayref of values in the extra_params hash. E.g. foo=1&bar=2&bar=3 would be specified as: + + extra_params => { foo => 1, bar => [ 2, 3 ] } 'API parameters' are parameters required to build a message object that are not transmitted with the message, e.g. consumer_secret, token_secret, request_url, request_method. @@ -280,7 +282,7 @@ =head2 THE REQUEST_URL PARAMETER -Any query parameters in the request_url are removed and added to the extra_params hash when generating the signature. +Any query parameters in the request_url are removed and added to the extra_params hash when generating the signature. (Note: if the same parameter name appears multiple times, all the values will be accumulated into an arrayref, instead of a simple scalar). E.g. the following requests are pretty much equivalent: --- lib/Net/OAuth/Message.old.pm 2010-06-16 21:46:47.000000000 +0100 +++ lib/Net/OAuth/Message.pm 2010-08-09 14:43:15.000000000 +0100 @@ -123,30 +123,61 @@ my %opts = @_; $opts{quote} = "" unless defined $opts{quote}; $opts{params} ||= []; - my %params; - foreach my $k (@{$self->required_message_params}, @{$self->optional_message_params}, @{$opts{add}}) { + +# CHANGED - collect params into an array, as extra_params may now specify multiple values +# - using an arrayref, e.g. extra_params => { x => 1, y => 2, z => [ 3, 4, 5 ] } +# my %params; + my @params; + +# CHANGED - deduplicate parameter names (in particular this ensures that 'signature' is not processed twice) +# foreach my $k (@{$self->required_message_params}, @{$self->optional_message_params}, @{$opts{add}}) { + my %want = map {$_=>1} @{$self->required_message_params}, @{$self->optional_message_params}, @{$opts{add}}; + foreach my $k (keys %want) { + next if $k eq 'signature' and (!$self->sign_message or !grep ($_ eq 'signature', @{$opts{add}})); my $message_key = $self->is_extension_param($k) ? $k : OAUTH_PREFIX . $k; my $v = $self->$k; - $params{$message_key} = $v if defined $v; + +# CHANGED - we're now collecting into an array +# $params{$message_key} = $v if defined $v; + push @params, $message_key => $v if defined $v; + } if ($self->{extra_params} and !$opts{no_extra} and $self->allow_extra_params) { - foreach my $k (keys %{$self->{extra_params}}) { - $params{$k} = $self->{extra_params}{$k}; + +# CHANGED - collecting into an array +# foreach my $k (keys %{$self->{extra_params}}) { +# $params{$k} = $self->{extra_params}{$k}; +# } + while(my ($k, $v) = each %{$self->{extra_params}}) { + push @params, $k => $_ foreach ref $v ? @$v : ($v); } + if ($self->can('request_url')) { my $url = $self->request_url; _ensure_uri_object($url); foreach my $k ($url->query_param) { - $params{$k} = $url->query_param($k); + +# CHANGED - collecting into an array +# $params{$k} = $url->query_param($k); + push @params, $k => $_ foreach $url->query_param($k); + } } } - if ($opts{hash}) { - return \%params; + if ($opts{array}) { + +# CHANGED - we're now returning an arrayref +# return \%params; + return \@params; + } my @pairs; - while (my ($k,$v) = each %params) { + +# CHANGED - we now need to iterate key/values from an array instead +# while (my ($k,$v) = each %params) { + while (my ($k,$v) = splice(@params, 0, 2)) { + push @pairs, join('=', encode($k), $opts{quote} . encode($v) . $opts{quote}); } return sort(@pairs); @@ -196,6 +227,21 @@ join($sep, $self->gather_message_parameters(quote => '"', add => [qw/signature/], no_extra => 1)); } +# ADDED - when repeated values for the same param appear, accumulate them +# - into an arrayref, instead of overwriting the same scalar +sub _accumulate { + my ($hash, $k, $v) = @_; + foreach(ref $v eq 'ARRAY' ? @$v : ($v)) { + if (exists $hash->{$k}) { + $hash->{$k} = [ $hash->{$k} ] unless ref $hash->{$k} eq 'ARRAY'; + push @{ $hash->{$k} }, $_; + } + else { + $hash->{$k} = $_; + } + } +} + sub from_authorization_header { my $proto = shift; my $header = shift; @@ -218,7 +264,11 @@ if (defined $k and defined $v) { $v =~ s/(^"|"$)//g; ($k,$v) = map decode($_), $k, $v; - $params{$k} = $v; + +# CHANGED - allow for repeated parameters +# $params{$k} = $v; + _accumulate(\%params, $k, $v); + } } return $class->from_hash(\%params, @_); @@ -255,7 +305,11 @@ } } else { - $msg_params{extra_params}->{$k} = $hash->{$k}; + +# CHANGED - allow for repeated parameters +# $msg_params{extra_params}->{$k} = $hash->{$k}; + _accumulate(\%{ $msg_params{extra_params} }, $k, $hash->{$k}); + } } $api_params{from_hash} = 1; @@ -288,7 +342,14 @@ sub to_hash { my $self = shift; - return $self->gather_message_parameters(hash => 1, add => [qw/signature/]); +# CHANGED - all for repeated parameters +# return $self->gather_message_parameters(hash => 1, add => [qw/signature/]); + my @params = @{ $self->gather_message_parameters(array => 1, add => [qw/signature/]) }; + my %params; + while(my ($k, $v) = splice(@params, 0, 2)) { + _accumulate(\%params, $k, $v); + } + return \%params; } sub to_url { @@ -301,13 +362,18 @@ _ensure_uri_object($url); $url = $url->clone; # don't modify the URL that was passed in $url->query(undef); # remove any existing query params, as these may cause the signature to break - my $params = $self->to_hash; - my $sep = '?'; - foreach my $k (sort keys %$params) { - $url .= $sep . encode($k) . '=' . encode( $params->{$k} ); - $sep = '&' if $sep eq '?'; - } - return $url; + +# CHANGED - gather_message_parameters does what we need, and copes with repeated params, so let's reuse it... +# my $params = $self->to_hash; +# my $sep = '?'; +# foreach my $k (sort keys %$params) { +# $url .= $sep . encode($k) . '=' . encode( $params->{$k} ); +# $sep = '&' if $sep eq '?'; +# } + my $query_string = join '&', $self->gather_message_parameters(add => [qw/signature/]); + $url .= '?' . $query_string if $query_string ne ''; + + return $url; } else { return $self->to_post_body; --- t/10-misc.old.t 2010-08-09 12:53:15.000000000 +0100 +++ t/10-misc.t 2010-08-09 14:51:05.000000000 +0100 @@ -1,6 +1,6 @@ use strict; use warnings; -use Test::More tests => 5; +use Test::More tests => 9; BEGIN { use Net::OAuth; @@ -58,3 +58,56 @@ ok($request->verify); +# https://rt.cpan.org/Ticket/Display.html?id=60154 +# Make sure repeated parameters are handled correctly +$request = Net::OAuth->request("consumer")->new( + consumer_key => 'dpf43f3p2l4k3l03', + consumer_secret => 'kd94hf93k423kf44', + request_url => 'https://photos.example.net/example?xyz=1&xyz=2', + request_method => 'GET', + signature_method => 'HMAC-SHA1', + timestamp => '1191242090', + nonce => 'hsu94j3884jdopsl', +); + +$request->sign; + +is($request->to_url(), 'https://photos.example.net/example?oauth_consumer_key=dpf43f3p2l4k3l03& oauth_nonce=hsu94j3884jdopsl&oauth_signature=6Z2WZLQCgFW2jFgl3ROTR9LFFxc %3D&oauth_signature_method=HMAC-SHA1&oauth_timestamp=1191242090&oauth_ve rsion=1.0&xyz=1&xyz=2'); + + +$request = Net::OAuth->request("consumer")->new( + consumer_key => 'dpf43f3p2l4k3l03', + consumer_secret => 'kd94hf93k423kf44', + request_url => 'https://photos.example.net/example', + request_method => 'GET', + signature_method => 'HMAC-SHA1', + timestamp => '1191242090', + nonce => 'hsu94j3884jdopsl', + extra_params => { xyz => [1, 2] }, +); + +$request->sign; + +is($request->to_url(), 'https://photos.example.net/example?oauth_consumer_key=dpf43f3p2l4k3l03& oauth_nonce=hsu94j3884jdopsl&oauth_signature=6Z2WZLQCgFW2jFgl3ROTR9LFFxc %3D&oauth_signature_method=HMAC-SHA1&oauth_timestamp=1191242090&oauth_ve rsion=1.0&xyz=1&xyz=2'); + + +$request = Net::OAuth->request("consumer")->from_url( + 'https://photos.example.net/example?oauth_consumer_key=dpf43f3p2l4k3l03& oauth_nonce=hsu94j3884jdopsl&oauth_signature=6Z2WZLQCgFW2jFgl3ROTR9LFFxc %3D&oauth_signature_method=HMAC-SHA1&oauth_timestamp=1191242090&oauth_ve rsion=1.0&xyz=1&xyz=2', + consumer_secret => 'kd94hf93k423kf44', + request_url => 'https://photos.example.net/example', + request_method => 'GET', +); + +ok($request->verify); + + +$request = Net::OAuth->request("consumer")->from_post_body( + 'oauth_consumer_key=dpf43f3p2l4k3l03&oauth_nonce=hsu94j3884jdopsl&oauth_ signature=HlCKYbZJCoXRL4G2Qm4sIfI6wK8%3D&oauth_signature_method=HMAC-SHA 1&oauth_timestamp=1191242090&oauth_version=1.0&xyz=1&xyz=2', + consumer_secret => 'kd94hf93k423kf44', + request_url => 'https://photos.example.net/example', + request_method => 'POST', +); + +ok($request->verify); + + ----------------------------------------------------------------------------------------------------------------------------------------- LOVEFiLM UK Limited is a company registered in England and Wales. Registered Number: 06528297. Registered Office: No.9, 6 Portal Way, London W3 6RU, United Kingdom. This e-mail is confidential to the ordinary user of the e-mail address to which it was addressed. If you have received it in error, please delete it from your system and notify the sender immediately. This email message has been delivered safely and archived online by Mimecast. For more information please visit http://www.mimecast.co.uk -----------------------------------------------------------------------------------------------------------------------------------------