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

OAuth 1.0 normalized URL must exclude default port #435

Merged
merged 1 commit into from Aug 2, 2017

Conversation

Projects
None yet
2 participants
@leeper
Contributor

leeper commented Apr 18, 2017

There is a bug in oauth_signature() related to port handling. The OAuth 1.0 spec indicates:

Unless specified, URL scheme and authority MUST be lowercase and include the port number; http default port 80 and https default port 443 MUST be excluded.

In other words, if port is specified as 80 for HTTP or 443 or HTTPS, the port number needs to be excluded from the normalized URL that is used in the OAuth 1.0 signature. oauth_signature() currently always includes the port number even if it is the default port number.

This PR fixes that.

@leeper leeper force-pushed the leeper:patch-1 branch from 59d6e78 to 5f4ed50 Apr 18, 2017

@hadley

This comment has been minimized.

Member

hadley commented Jul 24, 2017

Would you mind also adding a test? I think the easiest way would be to split oauth_signature into two pieces, one that generates oauth and the other that encodes it.

@hadley

This comment has been minimized.

Member

hadley commented Aug 1, 2017

@leeper are you interested in finishing this off?

@leeper

This comment has been minimized.

Contributor

leeper commented Aug 1, 2017

Sorry, I wanted to get to it but haven't had time. Maybe this weekend?

@hadley

This comment has been minimized.

Member

hadley commented Aug 1, 2017

I'd like to release by Friday, but I can finish it off.

@leeper

This comment has been minimized.

Contributor

leeper commented Aug 1, 2017

Ok, thanks. Sorry for not getting to it.

@hadley hadley merged commit 5f4ed50 into r-lib:master Aug 2, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hadley

This comment has been minimized.

Member

hadley commented Aug 2, 2017

No problems. Refactoring was a bit more involved than I'd expected so probably better for me to do anything. Thanks for bringing this to my attention and for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment