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

jwt ssl warning #214

Merged
merged 2 commits into from Jun 9, 2016

Conversation

Projects
None yet
3 participants
@aboutlo
Copy link
Contributor

aboutlo commented May 30, 2016

  • Fire a warning if the request is insecure rather than refusing to send the cookie.
  • Allow override a single cookie property

Please have a look https://github.com/feathersjs/feathers-authentication/compare/master...aboutlo:ls-JWT-ssl-warning?diff=unified&expand=1&name=ls-JWT-ssl-warning#diff-1fdf421c05c1140f6d71444ea2b27638L60

In my opinion app.get('auth') is always undefined at this stage.

@@ -32,7 +32,7 @@ const defaults = {
cookie: {
name: 'feathers-jwt',
httpOnly: false,
secure: process.env.NODE_ENV === 'production' ? true : false
secure: process.env.NODE_ENV === 'production'

This comment has been minimized.

@ekryski

ekryski May 30, 2016

Member

👍 Thank you!

// Merge and flatten options
const authOptions = Object.assign({}, defaults, app.get('auth'), config);

This comment has been minimized.

@ekryski

ekryski May 30, 2016

Member

I think we do need this back. The reason being is you can just do app.configure(authentication()) if you want to use all the default options plus your auth config defined in default.json or production.json. For example, your token secret, etc.

console.error(`You should be using HTTPS in production! Refusing to send JWT in a cookie`);
// Check HTTPS and cookie status in production
if (!req.secure && process.env.NODE_ENV === 'production' && options.cookie.secure) {
console.warn('WARN: Request isn\'t served through HTTPS: JWT in the cookie is exposed.');

This comment has been minimized.

@ekryski

ekryski May 30, 2016

Member

Suweeet!

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented May 30, 2016

@aboutlo this is awesome! Nice work! I think we do need app.get('auth') from what I explained above. I probably need to add a test for that. If you can add that back in this is good to go! 🍻

@aboutlo aboutlo force-pushed the aboutlo:ls-JWT-ssl-warning branch from fc3c054 to 7ed2fda May 30, 2016

@aboutlo

This comment has been minimized.

Copy link
Contributor Author

aboutlo commented May 30, 2016

Thank you @ekryski for the kind words. I added back app.get('auth'). Tests are still green :)

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Jun 9, 2016

Oh sorry, this was totally good to go to. I'll merge it in, it'll make it into the next release.

@daffl daffl merged commit 107957d into feathersjs:master Jun 9, 2016

1 check passed

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

This comment has been minimized.

Copy link
Member

ekryski commented Jun 9, 2016

Yup :shipit:

@aboutlo

This comment has been minimized.

Copy link
Contributor Author

aboutlo commented Jun 9, 2016

cool

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Jun 20, 2016

Released v0.7.9 with this fix in! 🍻 @aboutlo!

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