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

Updated example #19

Merged
merged 1 commit into from
Apr 27, 2014
Merged

Updated example #19

merged 1 commit into from
Apr 27, 2014

Conversation

felixsanz
Copy link
Contributor

Ok, the example made me waste 10+ hours trying to figure why my login didn't work.

Not all people understand 100% the library and the example is just plain wrong. The example used secure: true and copy-pasting it on a non-https site would fail to work always. Also, the Options doesn't include anything about cookie secure, so i updated the example to make it working and simple, and also updated Options to include secure defaults.

Ok, the example made me waste 10+ hours trying to figure why my login didn't work.

Not all people understand 100% the library and the example is just plain wrong. The example used `secure: true` and copy-pasting it on a non-https site would fail to work always. Also, the **Options** doesn't include anything about **cookie secure**, so i updated the example to make it working and simple, and also updated **Options** to include `secure` defaults.
@dougwilson
Copy link
Contributor

Ideally we want the example to show people how to do it correctly, since people unfortunately copy-paste into production. This is why the example has secure cookies and http-only cookies enabled.

@felixsanz
Copy link
Contributor Author

Ok but "secure" doesn't appear in Options. It's not documented!

@dougwilson
Copy link
Contributor

Sure, sure. The options given to cookie are passed into the cookie module, so maybe a PR to add to the docs like "for the options to cookie, see https://github.com/defunctzombie/node-cookie#more" or something

@btraut
Copy link

btraut commented Apr 8, 2014

Thanks for posting this comment, @felixsanz! I've now spent 5+ hours trying to figure out why secure: true didn't work only to finally realize that HTTPS is necessary for secure cookies.

@guax
Copy link

guax commented Apr 16, 2014

+1 for this to be merged. I spent 2 hours trying to figure out what was wrong and decided to check the PR. :(

As of using the secure as default. I think a lot of people will use proxies for that (proxypass, nginx or even heroku). I think secure should be false in the example. To mitigate the problem of people not being aware of secure:true for ssl connections we could add a comment stating about option and its requirement if using SSL directly.

@azat-co azat-co merged commit d694cef into expressjs:master Apr 27, 2014
azat-co pushed a commit that referenced this pull request Apr 27, 2014
…nks to @felixsanz

gist: secure cookies need https, docs didn't mention it
@azat-co
Copy link

azat-co commented Apr 27, 2014

@felixsanz thank you. I had to manually merge this due to some conflicts.

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.

None yet

5 participants