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

Use the secure flag in setcookie() when connecting over 443 #48

Closed
wants to merge 2 commits into from

Conversation

thewhit
Copy link

@thewhit thewhit commented Aug 1, 2016

No description provided.

@thewhit thewhit changed the title Use the secure flag when connecting over 443 Use the secure flag in setcookie() when connecting over 443 Aug 1, 2016
@codecov-io
Copy link

Current coverage is 71.42% (diff: 100%)

Merging #48 into master will increase coverage by 0.16%

@@             master        #48   diff @@
==========================================
  Files             2          2          
  Lines           174        175     +1   
  Methods          10         10          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            124        125     +1   
  Misses           50         50          
  Partials          0          0          

Powered by Codecov. Last update 283a56b...83668f9

@mebjas
Copy link
Owner

mebjas commented Aug 2, 2016

Thanks for the pull :)

@thewhit cookies in our context is used to transfer the token from server to client, so that only scripts running in that page can access it. It doesn't matter even if the cookie is sent back by the client or not, as the validation is done between the request parameter and token stored in session variable. So I don't see any benefit of setting the secure flag.

Now, just to learn more, let's say we needed the cookie back at server, aren't there cases where a page could have mix of http / https.
@abiusx your views?

@mebjas
Copy link
Owner

mebjas commented Aug 4, 2016

@thewhit should I close this PR, or you have some view on this?

@thewhit
Copy link
Author

thewhit commented Aug 4, 2016

@mebjas you can close it, as you're right it's possible the library could be used on a mix of http / https. Thanks.

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.

3 participants