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

[5.2] Set HttpOnly flag #12809

Closed
wants to merge 7 commits into from
Closed

[5.2] Set HttpOnly flag #12809

wants to merge 7 commits into from

Conversation

mzaalan
Copy link

@mzaalan mzaalan commented Mar 21, 2016

Depending on pull request
laravel/laravel#3697
then :
we need also to update line 138 , so it reads the http only from session config file, then this value can not be readable bu javascript

Depending on pull request
laravel/laravel#3697
then :
we need also to update line 138 , so it reads the http only from session config file, then this value can not be readable bu javascript
@GrahamCampbell GrahamCampbell changed the title Set HttpOnly flag [5.2] Set HttpOnly flag Mar 21, 2016
@taylorotwell
Copy link
Member

Why are you not also updating the actual session cookie in StartSession?

@mzaalan
Copy link
Author

mzaalan commented Mar 22, 2016

as i mentioned, each cookie in laravel will stored using "Illuminate\Cookie\CookieServiceProvider" class that will return a single instance from "Illuminate\Cookie\CookieJar".

make function take by default true value for httpOnly flag, so we need to path it from CookieServiceProvider class, so the register function will be modified to contain http only flag from session configuration.

Then we need to add httpOnly class attribute for CookieJar class, this value will be by deafult true and then will take the value from session config while registering "CookieServiceProvider "

At class CookieJar , line 58
secure and httponly need to be null by default, so we can check if it is set or not at function "getPathAndDomain" , if these values are not set then the class values variables will be return " session config values"

  • we need to use isset as these values defined as Boolean. The previous check will return always the class variables if we pass true to the function !!

Finally: to Apply theses settings we need to store cookies using the service provider "CookieServiceProvider ", so we need to change the Class "VerifyCsrfToken" and make session using the provider.

I'm so sorry as i can not solve the styleCI.

Regards.

@mzaalan
Copy link
Author

mzaalan commented Mar 22, 2016

I'm also set the flag in the start session by using cookie service provider.

@mzaalan
Copy link
Author

mzaalan commented Mar 22, 2016

I'v updated session start, you can review it now

On Mon, Mar 21, 2016 at 4:15 PM, Taylor Otwell [email protected]
wrote:

Why are you not also updating the actual session cookie in StartSession?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#12809 (comment)

*
* @var bool
*/
protected $httpOnly = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in #12818 (comment)

Making this default to true while previously it's default to false in a patch release should be consider as breaking change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default value was true, and it can be override by passing another value to make function. Now all we make that we read this value from session config

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default value was true

How can you say that when the changelog clearly shows the opposite.

screen shot 2016-03-23 at 10 44 27 am

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and it can be override by passing another value to make function.

When anyone upgrade from say v5.2.24 to v5.2.25 (if this get accepted). People shouldn't be worry about any configuration change and everything should works as it was before.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK that,s right . I,ll update the cookie service provider file to keep it as previously settings.

*/
protected $httpOnly = true;

/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems to be a TAB issue?

@taylorotwell
Copy link
Member

What is the status of this?

@mzaalan
Copy link
Author

mzaalan commented Mar 24, 2016

i'v updated the code so we keep the previous behavior without change and test it locally, you can run test and provide me with feedback.

@crynobone
Copy link
Member

What is the status of this?

There still a mix-and-match between of http_only state all over the place. Is it safe to do so on a patch release?

@GrahamCampbell
Copy link
Member

Probs best doing this on 5.3 @taylorotwell.

@it-can
Copy link
Contributor

it-can commented Mar 25, 2016

This is the current behavior:
schermafbeelding 2016-03-25 om 12 19 33

The XSRF cookie has never http_only and the normal cookie is always http_only in the current situation. This pull request will keep this intact, but will add an option to set http_only in your config.

@crynobone
Copy link
Member

The XSRF cookie has never http_only

But with this changes, if the user set the session.http_only to true, XSRF cookie will be HTTP ONLY? Is that the intended changes now? What would be the point of using XSRF cookie if not to be read by JavaScript?

@it-can
Copy link
Contributor

it-can commented Mar 25, 2016

@crynobone the http_only setting will be applied on all cookies with the PR... Don't know if this is intended/wanted behavior though...

@mzaalan
Copy link
Author

mzaalan commented Mar 25, 2016

yes by default , xsrf will be set to false and session ID will set to true, and when define HTTP_only flag in session config will apply to all cookies.
Some times when using HTML special character then cookie can be read by Java script and it will memore secure to set HTTP only false for security reason

@mzaalan
Copy link
Author

mzaalan commented Apr 5, 2016

@taylorotwell your feedback please ?

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.

5 participants