Code review: Issues with duplicate request parameters and square brackets

[Chirag Shah]

I've stumbled on a couple issues with the latest version of OAuth.php
and posted a code review here: http://codereview.appspot.com/41076

1) If you have a request with "a=1&a=2" as the request parameters, the
contents of $_GET and $_POST will only see a=2

Section 9.1.1. of the OAuth spec states "If two or more parameters
share
the same name, they are sorted by their value"

To fix this, I have introduced a function
OAuthUtil::oauth_parse_string to parse the raw request parameters.
This
function ensures that duplicate parameters are not stripped.
Instead of $_POST, you can use
OAuthUtil::oauth_parse_string(file_get_contents('php://
input'));

Instead of $_GET, you can use
OAuthUtil::oauth_parse_string($_SERVER["QUERY_STRING"]);


2) With the latest version of OAuth.php, if you have "a[]=1&a[]=2" in
the
query string, the function get_signable_parameters() will return
"a=1&a=2"
which is incorrect because "[" and "]" should be escaped.

Thanks,
Chirag Shah

[Morten Fangel]

Hi Shah

This is pretty much the direction I discussed off-list with R. Gareus,
and it does indeed fix a few issues we currently have..

The changes me and Gareus ended up agreeing on, and I sent a
mail to Andy Smith (termie) to clear, was to have the parameters
-array into an array of key-value arrays so that
?a[]=foo&a[]=bar
would be represented by
array(
array( 'key' => 'a[]', 'value'=>'foo' ),
array( 'key' => 'a[]', 'value'=>'bar')
)
(Smith never replied though, which is why I never started writing the
patch needed myself)

But I might lean towards your way of doing it, after having read
though the patch.

So all in all I agree with the functionality of the patch, but I haven't
looked though the coding style yet..

The remaining issues is to actually read the parameters from get/
post in OAuthRequest::from_request.. It needs to actually override
get-parameters with any incoming from post, so some sort of
elaborate merging is needed..

Also, get_parameter's key => value functionality is now somewhat
broken, and get_parameters doesn't return a _GET/_POST-like
array, but I guess this is expendable because I don't see a way to
keep these while maintaining all variables of the request..

- Morten

[Morten Fangel]

Hi OAuth-PHP

I have created a branch with Shah's patch included, as well as a few
other things..

Could you please review it, so we can move it into trunk..
http://code.google.com/p/oauth/source/branch?spec=issue104

Thanks
-Morten

[Morten Fangel]

Andy Smith (termie) reviewed the changes today, and the changes has
been imported into the main library.

The changes are quite extensive, but I have done all the testing I
could. There are Unit Tests that cover OAuthRequest and OAuthUtil and
I have tested it on my own Service Provider project.

There are a few BC-breaks, but nothing major so most won't have to
change anything to use the latest version.

But are there _anyone_ who experience any problems because of the
changes, please let me know.
I will then update the tests so the same won't happen again..

-Morten