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 error if using non-https server when isSecure is set to true #162

Merged
merged 3 commits into from Nov 3, 2015

Conversation

@tanepiper
Copy link
Contributor

tanepiper commented Nov 3, 2015

Today I've spent most of my morning digging around the Hapi internals to find why the state cookie was being destroyed, only to find that it was because I'm developing locally using http, but had not set isSecure: false in the options.

The issue is that the error about the cookie not being set at line 77 isn't too helpful. Took me into Hapi's own request internals because I was seeing on the outgoing request when I did console.log

{ 'bell-twitter':
   { name: 'bell-twitter',
     value:
      { token: '<token>',
        secret: '<secret>',
        query: {} } } }
Debug: internal, implementation, error
    TypeError: Cannot read property 'cookies' of undefined
    at [object Object].internals.Request.internals.Request._execute.internals.Request._lifecycle.internals.Request._invoke.internals.Request._reply.internals.Request._finalize (D:\Documents\GitHub\myapp\node_modules\hapi\lib\request.js:473:28)

Then on the request back from twitter:

{ undefined: '' }
{}

It was quite unhelpful. Hopefully making this message explicit will make it easier for other people to avoid the same mistake

@tanepiper

This comment has been minimized.

Copy link
Contributor Author

tanepiper commented Nov 3, 2015

I was able to get the first test passing, but not the one for V2. This will check for forceHttps being set.

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Nov 3, 2015

I like better error messages for the developer 👍 . You will also have to add !settings.location since you can redefine the protocol using that feature too. It's a bit of a mess of features.

Thank you for adding tests too. Do you mind if this eventually gets merged for node 4+, Hapi 11?

@tanepiper

This comment has been minimized.

Copy link
Contributor Author

tanepiper commented Nov 3, 2015

@ldesplat OK - re-written to check for the correct protocol, and tests written to confirm it works in call cases (forceHttps, location and not set). Happy for this to be merged in ASAP.

@tanepiper tanepiper force-pushed the tanepiper:master branch from be009ab to f896ff6 Nov 3, 2015
@tanepiper tanepiper force-pushed the tanepiper:master branch from f896ff6 to def9881 Nov 3, 2015
@tanepiper

This comment has been minimized.

Copy link
Contributor Author

tanepiper commented Nov 3, 2015

@ldesplat OK I've rebased the code to be ES6 and up to date with the master branch here - but it doesn't seem to be happy with the tests on the CI, but they seem to all run locally fine (never mind seemed to be the linting, fixed in the commit below)

@ldesplat ldesplat added the feature label Nov 3, 2015
@ldesplat ldesplat added this to the 6.0.0 milestone Nov 3, 2015
@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Nov 3, 2015

Top notch contribution. Thank you so much.

ldesplat added a commit that referenced this pull request Nov 3, 2015
Add error if using non-https server when isSecure is set to true
@ldesplat ldesplat merged commit 91f7274 into hapijs:master Nov 3, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ldesplat ldesplat mentioned this pull request Nov 3, 2015
@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Nov 3, 2015

It is now included in the 6.0.0 release that's released on NPM.

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