broken request if parameter contains ";"

[mnovozhylov]

hey @mrjones ,

first of all, thanks for the nice library! i'm trying to re-use it in upwork/golang-upwork project, and i've found an interesting issue:

params := make(map[string]string)
params["q"] = "golang"
params["paging"] = "1;10"

line 675: t := userRequest.URL.Query()
line 676: log.Fatal(userRequest)
2015/08/10 07:59:26 &{GET https://www.upwork.com/api/profiles/v2/search/jobs.json?paging=1;10&q=golang HTTP/1.1 1 1 map[] <nil> 0 [] false www.upwork.com map[] map[] <nil> map[]   <nil>}

seems to be OK, but:
line 677: log.Fatal(t)
2015/08/10 08:02:40 map[10:[] q:[golang] paging:[1]]
HOW?!

let's change params[q] to params["q"] = "golang;test"
userRequest looks correct:
2015/08/10 08:11:25 &{GET https://www.upwork.com/api/profiles/v2/search/jobs.json?q=golang;test&paging=1;10 HTTP/1.1 1 1 map[] <nil> 0 [] false www.upwork.com map[] map[] <nil> map[]   <nil>}
but suddenly userRequest.URL.Query() returns:
2015/08/10 08:12:51 map[paging:[1] 10:[] q:[golang] test:[]]

jfyi, URL.Query() recognizes ";" and "&" as delimeters (http://stackoverflow.com/questions/3481664/semicolon-as-url-query-separator)

quick fix can look like:

diff --git a/oauth.go b/oauth.go
index 3d195ce..d1b9b61 100644
--- a/oauth.go
+++ b/oauth.go
@@ -669,6 +669,7 @@ func (rt *RoundTripper) RoundTrip(userRequest *http.Request) (*http.Response, er
        if userRequest.Header.Get("Content-Type") !=
                "application/x-www-form-urlencoded" {
                // Most of the time we get parameters from the query string:
+               userRequest.URL.RawQuery = strings.Replace(userRequest.URL.RawQuery, ";", "%3B", -1)
                for k, vs := range(userRequest.URL.Query()) {
                        if len(vs) != 1 {
                                return nil, fmt.Errorf("Must have exactly one value per param")

in that case library works as expected

or the library could encode ";" before creating a request (and might decode it before signing a request to have a valid signature created), or smth similar

[mrjones]

Hi! Thanks so much for the extremely well researched bug report, and I'm really sorry that it's taken me so long to reply.

This is a tricky one!! Since: it is not clear to me whether its "Right" to consider ";" a separator or not.

Personally, I had never considered semicolon a separator. On the other hand, clearly the Go authors do, and (from your link) the W3C says: "We recommend that HTTP server implementors, and in particular, CGI implementors support the use of ";" in place of "&" to save authors the trouble of escaping "&" characters in this manner." http://www.w3.org/TR/html401/appendix/notes.html#h-B.2.2

Neither choice seems obviously right (or wrong), so we've just got to choose one.

I'm inclined to consider semicolon a separator, primarily because the behavior of url.Query seems like a reasonable precedent to follow for Go programs.

Once we assume that decision, that implies two things:

1. This library should continue to treat semicolon as a separator and not make any changes.
2. For users, like you, who want to embed a semicolon without it being treated as a separator, they should escape it in application code, before sending it in.

So, my suggestion is that you change the URL you pass to GET to: https://www.upwork.com/api/profiles/v2/search/jobs.json?paging=1%3B10&q=golang

I hope that this is a reasonable compromise that should allow applications to continue to use semicolon as a separator (by passing ";" raw) or not (by passing %3B).

Please do let me know if this works for you!

[mnovozhylov]

@mrjones , unfortunately your suggestion won't work, because by specifying "%3B" in the parameter, means that "%" will be encoded as "%25" and the final url will contain "%253B" - as a result, server side never decodes "%253B" to ";"

i understand that old rfc describes ";" and "&" as delimiters, but it's far from reality nowadays :)

[mrjones]

Ah, I think I understand better now. However, I could still be mis-understanding something, if so please let me know.

I just committed change c4fac5ef43dacbdd6dfe0b159fb24b0e33ea5bbb which I hope will help you.

To communicate fully what's happening here, I must point out that as of recently, there are two different ways to make HTTP requests. This all started with 5faa557585ac2fde321eada0a28eb6f1b4176319, when I introduced a second way.

• The old way works by calling Get()/Post()/etc directly on the Consumer object.
• The new way works by using an http.Client from the MakeHttpClient method on the Consumer.

I think the second way is more flexible, and generally "better", but I'd like to keep both ways working. So, there is a function (makeAuthorizedRequestReader) which translates from the old interface to the new one internally.

Correct me if I'm wrong, but I assume that you are using the "old" way (calling Get directly)? People who use the new interface should be able to access to RawQuery directly on the http.Request object, and therefore shouldn't need any special support inside the library.

Assuming I understand correctly: We only need to fix the old interface. Therefore, I applied the exact fix that you recommended (replacing ";" with "%3B" in the RawQuery) in the old-to-new translation method (makeAuthorizedRequestReader).

I hope this will fix your code as it stands. I also believe you could switch to the new http.Client interface, which would enable you to control the escaping yourself by editing RawQuery. You might like other aspects of the new interface as well.

I committed two tests as part of c4fac5ef43dacbdd6dfe0b159fb24b0e33ea5bbb, to demonstrate what I think should happen.

Please do let me know if this fixes your issue!

[mnovozhylov]

@mrjones , actually, i use a new method, i.e. MakeHttpClient, and i can move the fix under my hood :) i believed:

• other could experience the same issue; moreover, i wasn't sure about the root and history of the issue - thanks for the detailed explanation
• old method still required the fix - good to know you have updated it :+1:

p.s. feel free to close the issue in case you don't plan any further updates - thanks!