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

Add option for forcing https. #53

Merged
merged 3 commits into from Feb 2, 2015
Merged

Conversation

@eiriksm
Copy link
Contributor

eiriksm commented Jan 17, 2015

As mentioned in #40, this is a PR adding the option to force https in redirect_uri.

Granted, it might not be the best solution to alter the request object like this, but this certainly works. The other option would be to pass in the settings as a parameter to the location function, not sure which one you prefer? IMO the naming of the option (force) indicates some force, and thus we are forcing the request object to be https :)

PR also adds a test for this.

@peny

This comment has been minimized.

Copy link

peny commented Feb 2, 2015

+1

@geek geek added the feature label Feb 2, 2015
@geek geek added this to the 2.1.0 milestone Feb 2, 2015
@geek geek self-assigned this Feb 2, 2015
@@ -135,6 +135,10 @@ exports.v2 = function (settings) {
return reply(Boom.internal('App was rejected'));
}

// Alter the request to be able to force HTTPS.
if (settings.forceHttps) {
request.connection.info.protocol = 'https';

This comment has been minimized.

Copy link
@geek

geek Feb 2, 2015

Member

Instead, we should pass a new parameter to location(request) for protocol. Otherwise we are overwriting request connection information that may not always be safe.

@eiriksm

This comment has been minimized.

Copy link
Contributor Author

eiriksm commented Feb 2, 2015

Added a new commit that passes the protocol to the location function instead (as suggested in the diff comment. And by myself - for that matter :))

geek added a commit that referenced this pull request Feb 2, 2015
Add option for forcing https.
@geek geek merged commit 220b5d5 into hapijs:master Feb 2, 2015
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@geek

This comment has been minimized.

Copy link
Member

geek commented Feb 2, 2015

@eiriksm thanks for the help, looks good. I'll publish soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.