johvargas01 / oauth-signpost

Automatically exported from code.google.com/p/oauth-signpost
0 stars 0 forks source link

incorrect POST signing when parameters are included in the query part of URL #47

Open GoogleCodeExporter opened 8 years ago

GoogleCodeExporter commented 8 years ago
When signing a POST request for e.g. 
"http://api.soundcloud.com/tracks/3130960/comments?
comment%5Bbody%5D=This+is+a+test+comment", signpost doesn't seem to correctly 
construct 
the signature base string.  This is the debug output:

[SIGNPOST] SBS: 
POST&http%3A%2F%2Fapi.soundcloud.com%2Ftracks%2F3130960%2Fcomments&comment%25255
Bbody%25255D%3D%26oauth_consumer_key%3DHXPgy7JJG5DLCMkyqBvksA%26oauth_nonce%3D
5646301471762814286%26oauth_signature_method%3DHMAC-
SHA1%26oauth_timestamp%3D1274917474%26oauth_token%3DZB4G8jA8Tsol2j7ge2yjIA%26oau
t
h_version%3D1.0
[SIGNPOST] signature: z82Wn/C7amPx8zC9jLgbyamyj7g=
[SIGNPOST] Auth header: OAuth oauth_token="ZB4G8jA8Tsol2j7ge2yjIA", 
oauth_consumer_key="HXPgy7JJG5DLCMkyqBvksA", oauth_version="1.0", 
oauth_signature_method="HMAC-SHA1", oauth_timestamp="1274917474", 
oauth_nonce="5646301471762814286", 
oauth_signature="z82Wn%2FC7amPx8zC9jLgbyamyj7g%3D"
[SIGNPOST] Request URL: http://api.soundcloud.com/tracks/3130960/comments?
comment%5Bbody%5D=This+is+a+test+comment

I'm not sure what the SBS should be, although according to 
http://googlecodesamples.com/oauth_playground/, the SBS should begin with:

POST&http%3A%2F%2Fapi.soundcloud.com%2Ftracks%2F3130960%2Fcomments&comment%25255
Bbody%25255D%3DThis%252Bis%252Ba%252Btest%252Bcomment%26oauth_consumer_key...

It's possible that this bug affects other (non-GET?) request types.

Original issue reported on code.google.com by stjepan....@gmail.com on 27 May 2010 at 12:27

GoogleCodeExporter commented 8 years ago
It looks like the problem might stem from the escapable [] characters in the 
parameter name, rather than the 
use of POST.  I tried adding the following test in SignatureBaseStringTest:

    @Test
    public void shouldWorkWithBracketsInParameterName() throws Exception {
        HttpRequest request = mock(HttpRequest.class);
        when(request.getMethod()).thenReturn("GET");
        when(request.getRequestUrl()).thenReturn("http://examplebrackets.com");

        HttpParameters params = new HttpParameters();
        params.put("a[]", "1");

        SignatureBaseString sbs = new SignatureBaseString(request, params);

        assertEquals("GET&http%3A%2F%2Fexamplebrackets.com%2F&a%255B%255D%3D1", sbs.generate());
    }

(I got the hopefully correct SBS from 
http://googlecodesamples.com/oauth_playground/)

This test failed as follows:

org.junit.ComparisonFailure: expected:<...om%2F&a%255B%255D%3D[1]> but 
was:<...om%2F&a%255B%255D%3D[]>

So the value of the parameter was missing, just like in the originally reported 
failure.  I tracked down the 
problem to the following sequence of operations in HttpParameters:

public String getAsQueryString(Object key) {
        StringBuilder sb = new StringBuilder();
        key = OAuth.percentEncode((String) key);    // the provided key is first encoded
        Set<String> values = wrappedMap.get(key);  // and then used to grab the value
        if (values == null) {   // so we get value as null
            return key + "=";
        }

Note that this is a problem because in 
SignatureBaseString.normalizeRequestParameters you have:

        Iterator<String> iter = requestParameters.keySet().iterator();

        for (int i = 0; iter.hasNext(); i++) {
            String param = iter.next();

            if (OAuth.OAUTH_SIGNATURE.equals(param) || "realm".equals(param)) {
                continue;
            }

            if (i > 0) {
                sb.append("&");
            }

            sb.append(requestParameters.getAsQueryString(param)); // <-- the key provided here is straight from 
the Map!
        }

So, normalizeRequestParameters will send to getAsQueryString exactly the value 
for the key present in the 
TreeMap, and yet getAsQueryString will encode it again before retrieving the 
corresponding value for the 
parameter - which means that unless encoding is a no-op for the particular 
parameter key, we're doing 
something wrong.

To try to resolve the bug, I tried switching the order of the operations in 
getAsQueryString like this:

Set<String> values = wrappedMap.get(key);
key = OAuth.percentEncode((String) key);

While that fixed my added test, it broke one of yours (RequestParametersTest):

        params.put("a b", "c d", true);
        assertEquals("a%20b=c%20d", params.getAsQueryString("a b"));

Taking a cue off that test, I returned the library code as it was and tried 
changing my test:

    @Test
    public void shouldWorkWithBracketsInParameterName() throws Exception {
        HttpRequest request = mock(HttpRequest.class);
        when(request.getMethod()).thenReturn("GET");
        when(request.getRequestUrl()).thenReturn("http://examplebrackets.com");

        HttpParameters params = new HttpParameters();
        params.put("a[]", "1", true);

        SignatureBaseString sbs = new SignatureBaseString(request, params);

        assertEquals("GET&http%3A%2F%2Fexamplebrackets.com%2F&a%255B%255D%3D1", sbs.generate());
    }

...but then I get the following failure:

org.junit.ComparisonFailure: expected:<...brackets.com%2F&a%25[5B%255D%3D1]> 
but 
was:<...brackets.com%2F&a%25[255B%25255D%3D]>

In this case there was additional escaping (which may or may not be correct, 
I'm not sure) but the parameter 
value was still missing.

I'm a little stuck now because:

1. I don't know what you had in mind for the percentDecode parameter.  When 
should a parameter be percent 
encoded before being inserted in the map, and when should it not?
2. I am actually using the Apache HttpClient in my application, and I don't yet 
understand how that all 
interacts with the core code.  Perhaps all of this has no bearing whatsoever on 
code that uses HttpClient, but 
the similarity in symptoms is rather suspicious.

Thoughts?

Original comment by stjepan....@gmail.com on 3 Jun 2010 at 12:06

GoogleCodeExporter commented 8 years ago
I'm also having this problem with signpost-1.2.1.1 on Android using Apache 
Commons -- square brackets "[]" fail with both GET and POST requests, requests 
without them work fine. 

Original comment by casper.g...@gmail.com on 11 Aug 2010 at 6:05

GoogleCodeExporter commented 8 years ago
This issue is KILLING me. Is there anything that can be done? Perhaps you can 
post a patch for what you describe as

"I tried switching the order of the operations in getAsQueryString like this"

so we can test it out in our apps. It may be breaking a test, but that test 
line you include looks nonsensical to me (though it lacks context, so....). 
Anyway, I'd really like to see this resolved somehow. Otherwise, I must rewrite 
my code using another library :(

Original comment by beayoha...@gmail.com on 13 Aug 2010 at 2:23

GoogleCodeExporter commented 8 years ago
OK I gave it a try.  Attached is a new .jar, see if it works for you.

My changes were committed to my fork of signpost, in a new branch 1.2.1.2.  See 
the changes here:
http://github.com/srajko/signpost/commit/1cd201f8b8e1168c48b8cd3fcc6391f706fa469
f

Please let me know if this helps out any.

Original comment by stjepan....@gmail.com on 13 Aug 2010 at 5:18

Attachments:

GoogleCodeExporter commented 8 years ago
Still fails for me.

Original comment by casper.g...@gmail.com on 13 Aug 2010 at 7:16

GoogleCodeExporter commented 8 years ago
So, I am not wise in the ways of OAuth, but I played around with it and what 
worked for me was NOT percent encoding the keys:

    public String getAsQueryString(Object key) {
        StringBuilder sb = new StringBuilder();
        //key = OAuth.percentEncode((String) key);
        //Set<String> values = wrappedMap.get(key);
        Set<String> values = wrappedMap.get(key);
        //key = OAuth.percentEncode((String) key);
        if (values == null) {
            return key + "=";
        }
        Iterator<String> iter = values.iterator();
        while (iter.hasNext()) {
            sb.append(key + "=" + iter.next());
            if (iter.hasNext()) {
                sb.append("&");
            }
        }
        return sb.toString();
    }

I don't know maven at all, but I compiled with "mvn package -DskipTests"

This makes only a tiny bit of sense to me.....

Original comment by beayoha...@gmail.com on 13 Aug 2010 at 2:50

GoogleCodeExporter commented 8 years ago
not percent encoding ever in getAsQueryString breaks other test cases, but I'm 
not sure how relevant that is for end-user use cases.  The change I made makes 
percent encoding optional (true by default), and I changed one place where 
getAsQueryString is called to opt for not percent encoding.  That makes the 
test case I added pass, but there might be other places where calls to 
getAsQueryString might need to get fixed.

Did the jar I posted work for you, bjorn?

Original comment by stjepan....@gmail.com on 13 Aug 2010 at 2:58

GoogleCodeExporter commented 8 years ago
stjepan: I made my last post without realizing you had posted in between. oops. 
I will try your jar. I wonder if the right fix is simply to avoid escaping 
brackets?

Just a thought: is anybody having trouble with a non-ruby server?

bjorn

Original comment by beayoha...@gmail.com on 13 Aug 2010 at 4:18

GoogleCodeExporter commented 8 years ago
stjepan: no, your fix did not work :(

so, I know, "works for me" does not mean that you've found the right way to do 
it, but if we pretend for a moment that it does, I think the right way to do it 
is to alter signpost-core in some way that resembles my suggestion above.

Original comment by beayoha...@gmail.com on 13 Aug 2010 at 4:30

GoogleCodeExporter commented 8 years ago
[deleted comment]
GoogleCodeExporter commented 8 years ago
One way or another, there is a serious bug in the HttpParameters.put function, 
which should read:

    public String put(String key, String value, boolean percentEncode) {
       key = percentEncode ? OAuth.percentEncode(key) : key;
        SortedSet<String> values = wrappedMap.get(key);
        if (values == null) {
            values = new TreeSet<String>();
            wrappedMap.put( key, values);
        }
        if (value != null) {
            value = percentEncode ? OAuth.percentEncode(value) : value;
            values.add(value);
        }

        return value;
    }

rather than:

    public String put(String key, String value, boolean percentEncode) {
        SortedSet<String> values = wrappedMap.get(key);
        if (values == null) {
            values = new TreeSet<String>();
            wrappedMap.put( percentEncode ? OAuth.percentEncode(key) : key, values);
        }
        if (value != null) {
            value = percentEncode ? OAuth.percentEncode(value) : value;
            values.add(value);
        }

        return value;
    }

which will cause problems when there are multiple parameters with the same name 
that need to be escaped, like

playlist[track][][id]=12
playlist[track][][id]=15
playlist[track][][id]=34
playlist[track][][id]=127

in soundcloud.

Original comment by beayoha...@gmail.com on 13 Aug 2010 at 8:34

GoogleCodeExporter commented 8 years ago
Hi Bjorn, Casper

I just got a chance to test the modified jar - and it works for me.  
Potentially good news is, I POSTED THE WRONG JAR! :-)  (sorry about that).  The 
commonshttp jar is actually no different than the 1.2.1.1. version.  It is the 
core jar that changed.

Please see if this one works for you.

(Bjorn, I'm not sure about the change you posted in your last comment.  The 
original code seems correct to me.  I think the keys and values should always 
be stored in a percent encoded state - the question is whether they are already 
supplied as percent encoded (or percent encoding is a no-op for the supplied 
strings), in which case they should not be percent encoded *again*.)

Original comment by stjepan....@gmail.com on 14 Aug 2010 at 4:13

Attachments:

GoogleCodeExporter commented 8 years ago
Stjepan,

I discovered this bug experimentally, and fixing cause my problems to go away. 
However, you can also follow the logic of the original code where percentEncode 
is true. By eliminating the percentEncode variable, it becomes:

    public String put(String key, String value ) {
        SortedSet<String> values = wrappedMap.get(key);
        if (values == null) {
            values = new TreeSet<String>();
            wrappedMap.put( OAuth.percentEncode(key), values);
        }
        if (value != null) {
            value = OAuth.percentEncode(value);
            values.add(value);
        }
        return value;
    }

In the case where key.equals( OAuth.percentEncode(key) ) is false, then:

SortedSet<String> values = wrappedMap.get(key);

will give values = null, because no key could have ever gotten into the map 
that wasn't percent encoded. Why? Because the only put call in the code is 

wrappedMap.put( OAuth.percentEncode(key), values);

With values == null, this call:

wrappedMap.put( OAuth.percentEncode(key), values);

will replace any preexisting OAuth.percentEncode(key) in the wrappedMap without 
retaining it's values. If we repeat the call with the same key, regardless of 
value, the prior value is lost.

This is definitely not the intent of the OAuth spec, and causes failures any 
time there are multiple, identical keys that require escaping.

Original comment by beayoha...@gmail.com on 14 Aug 2010 at 4:45

GoogleCodeExporter commented 8 years ago
Yes, you are right.  Let me add that change into the patched JAR so it can also 
handle multiple parameters with the same key. I'll also add an appropriate test 
to make sure this always works correctly.

Original comment by stjepan....@gmail.com on 14 Aug 2010 at 4:51

GoogleCodeExporter commented 8 years ago
OK, I've added a test for multiple identical keys that require escaping, and 
your change that makes the test pass (BTW, sorry about being initially 
skeptical about your fix - there was a crucial part that I missed when reading 
it).

I am now attaching a new JAR which now includes both my fix for keys that need 
escaping, and your fix for multiple keys that need escaping.  All old signpost 
tests still pass, so hopefully we didn't break anything.

Original comment by stjepan....@gmail.com on 14 Aug 2010 at 5:25

Attachments:

GoogleCodeExporter commented 8 years ago
Working for me now -- many thanks.  

Original comment by casper.g...@gmail.com on 14 Aug 2010 at 7:21

GoogleCodeExporter commented 8 years ago
signpost-core-1.2.1.2.jar works great... thanx a lot

Original comment by pranavch...@gmail.com on 28 Nov 2011 at 3:11