Open GoogleCodeExporter opened 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
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
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
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:
Still fails for me.
Original comment by casper.g...@gmail.com
on 13 Aug 2010 at 7:16
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
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
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
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
[deleted comment]
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
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:
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
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
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:
Working for me now -- many thanks.
Original comment by casper.g...@gmail.com
on 14 Aug 2010 at 7:21
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
Original issue reported on code.google.com by
stjepan....@gmail.com
on 27 May 2010 at 12:27