Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix oauth_callback_confirmed comparison when server returns extra whitespace #56

Merged
merged 1 commit into from
Dec 21, 2023

Conversation

pscohn
Copy link
Contributor

@pscohn pscohn commented Jan 29, 2022

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
Copy link
Owner

dghubble commented Feb 1, 2022

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
Copy link
Contributor Author

pscohn commented Feb 2, 2022

@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

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

@dghubble dghubble merged commit ef86807 into dghubble:main Dec 21, 2023
2 checks passed
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