search

mrjones / oauth

OAuth 1.0 implementation in go (golang).
http://www.mrjon.es
MIT License
269 stars 119 forks source link

Allows providers to include query string params in the base string for signing #63

Closed ericksonjoseph closed 7 years ago

ericksonjoseph commented 7 years ago

Hey MrJones!

So this PR is a little different. The issue we were having is that one of our devices (Roku) does not follow the oAuth specification as strictly as it should. The device is sending a POST request with query-string-parameters in the URL. Normally these values should not be included in the base string that get signed, however our company decided to tolerate this as this situation may exist in other devices.

I would like to continue to use your library instead of forking and adding our own logic so I submit this PR in case you find that it would be useful in your library as an option that is off by default.

mrjones commented 7 years ago

Thanks for the patch, and sorry for the delay.

It's not an ideal situation, but seems reasonable given that it defaults to off. Let's merge it.

komod commented 5 years ago

Hi @mrjones

I'm trying to implement a verifying function and found this nice project. And I encounter similar situation as @ericksonjoseph mentioned above. It brings my interest so I tried to check the spec. However, RFC 5849 section 3.4.1.3.1 doesn't address that the query parameters should only be fetched with GET request. Do you know any further detailed explanation about this?

Thanks!

mrjones commented 5 years ago

Hi @komod ! Thanks a lot for your comment and for your careful reading of the spec.

RFC 5849 section 3.4.1.3.1 says "The parameters from the following sources are collected into a single list of name/value pairs: [...] The query component of the HTTP request URI".

(Notably there are no conditions about when to include the query params; it sounds like they should always be include).

If SignQueryParams is false (which is the default), we will not include the query params in the signature. And this seems incorrect based on the above statement from the RFC. We probably should remove the SignQueryParams option, and always include the query params.

I'm a little nervous about changing the default, but it sounds like the current default behavior is just incorrect.

Let me know if you have any further thoughts, otherwise we can push this forward.

Thanks again!

komod commented 5 years ago

Hi @mrjones , glad we have same understanding to the words in RFC. I do modified the behavior on my forked repo for my own use as you said: remove the SignQueryParams and also update the getBody function. However, I haven't read the whole project so I'm not sure it's a proper fix and might not be able to fire a PR very soon. Anyway, I agree we should do this.