-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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] update httponly flags #12818
[5.2] update httponly flags #12818
Conversation
@@ -135,7 +135,7 @@ protected function addCookieToResponse($request, $response) | |||
$response->headers->setCookie( | |||
new Cookie( | |||
'XSRF-TOKEN', $request->session()->token(), time() + 60 * 120, | |||
$config['path'], $config['domain'], $config['secure'], false | |||
$config['path'], $config['domain'], $config['secure'], $config['http_only'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break on systems were that key does not exist.
Okay @GrahamCampbell, it should be fixed now... |
Test seems to be failing because of this commit: b2f29b7 |
@@ -135,7 +135,7 @@ protected function addCookieToResponse($request, $response) | |||
$response->headers->setCookie( | |||
new Cookie( | |||
'XSRF-TOKEN', $request->session()->token(), time() + 60 * 120, | |||
$config['path'], $config['domain'], $config['secure'], false | |||
$config['path'], $config['domain'], Arr::get($config, 'secure', false), Arr::get($config, 'http_only', true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this introduce a breaking change/behaviour change?
Currently, http_only
is set to false
, but with this change (and without updating laravel/laravel
configuration), http_only
would default to true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this would be breaking.
@it-can stop changing unrelated code and just make your change. |
this is a updated version of #12809
in StartSession I also changed this:
to
Is this correct?