oauth1
oauth1 copied to clipboard
dghubble
Fix oauth_callback_confirmed comparison when server returns extra whitespace
I'm working against the Netsuite API and found that there is an added newline in the response body during the RequestToken flow, and since their response sets oauth_callback_confirmed as the last body parameter, this causes the comparison values.Get(oauthCallbackConfirmedParam) != "true" to fail while comparing against the parsed value true\n.
Since the tests don't use the same order of parameters in the mocked response I've added a newline to the response and trimmed that during body parsing to ensure trailing whitespace always gets removed. In this case the mock change will cause a failure without the TrimSpace while checking the expectedSecret.
As far as I can tell, Netsuite isn't really allowed to have a trailing newline in their application/x-www-form-urlencoded body. Characters there seem to be legitimately part of the last parameter's value. I could choose to strip out "whitespace" characters, but its not on solid authority from a spec.
- https://url.spec.whatwg.org/#urlencoded-parsing
- https://github.com/thomasp85/reqres/issues/4#issuecomment-319899245
@dghubble For other parts of the flow (e.g. getting the access secret) I'm able to work around the same issue by stripping the newlines on our side since we get the values back from the library, but since the case for oauth_callback_confirmed returns an error without returning the request token/secret, I'd have to use a fork (assuming Netsuite won't make any changes).
Would it be more reasonable to strip whitespace just when checking values.Get(oauthCallbackConfirmedParam)? Even if the response is not exactly to spec, if the value is true with any whitespace I can't imagine the intention is for anything other than to signify the callback was confirmed.
The only non-breaking alternative I can think of would be to allow adding a custom oauthCallbackConfirmed comparison function to the config, which seems like overkill to me and for most users an unneeded bloat of the config.
Anything sound good here?
