Skip to content

Fix oauth_callback_confirmed comparison when server returns extra whitespace - #56

Merged
dghubble merged 1 commit into
dghubble:mainfrom
pscohn:trim-values
Dec 21, 2023
Merged

dghubble merged 1 commit into
dghubble:mainfrom
pscohn:trim-values

Conversation

@pscohn

@pscohn pscohn commented Jan 29, 2022

Copy link
Copy Markdown
Contributor

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.

@dghubble

dghubble commented Feb 1, 2022

Copy link
Copy Markdown
Owner

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.

@pscohn

pscohn commented Feb 2, 2022

Copy link
Copy Markdown
Contributor Author

@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?

@mdestagnol

Copy link
Copy Markdown

@dghubble Would be awesome to get this one merged! Thx a lot

@dghubble
dghubble merged commit ef86807 into dghubble:main Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants