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

[8.x] Remove decrypting array cookies #35130

Merged
merged 4 commits into from
Nov 9, 2020

Conversation

marcvdm
Copy link
Contributor

@marcvdm marcvdm commented Nov 6, 2020

This is a follow up on my previous merge request: #35105

The problem is that when someone manually adjusts cookie names in the browser or through any other method it wil generate unnecessary errors.

ErrorException(code: 0): strpos() expects parameter 1 to be string, array given at /vendor/laravel/framework/src/Illuminate/Cookie/Middleware/EncryptCookies.php:86)

Because Laravel does not support setting cookie array's, the middleware will never encrypt array cookies. This means we can remove the support for decrypting array cookies.

When there are array cookies they will not be decrypted and just passthrough as is

@driesvints driesvints changed the title Remove decrypting array cookies [8.x] Remove decrypting array cookies Nov 6, 2020
@driesvints
Copy link
Member

I think this is best done in 9.x at the earliest as removing methods like this is still a breaking change.

@marcvdm
Copy link
Contributor Author

marcvdm commented Nov 6, 2020

Wel we could leave the methods there but this is more a bug fix i think.

Right now when the cookie value is an array it will always fail because you cannot do a strpos on an array.

The other solution is to check for the prefix in the array values itself but that is weird because we never prefix array cookies in the first place.

@driesvints
Copy link
Member

Removing the decryptArray method is definitely a breaking change because if anyone's extending the middleware and calling it they'll see their apps crash. I'm not disputing the changes themselves.

@paras-malhotra
Copy link
Contributor

paras-malhotra commented Nov 6, 2020

What happens when 2 cookies have the same key? I think they will be considered as an array and then the decryption will fail with this PR. RFC 6265 allows multiple cookies with the same key. This PR would break that functionality. The failing test PR that you sent only considers associative array cookies and doesnt consider the scenario of 2 cookies set with the same key.

@marcvdm
Copy link
Contributor Author

marcvdm commented Nov 6, 2020

The $_COOKIE global never contains duplicate cookies. The browser, or other client, can send 1 or more cookies with the same name but if there are multiple cookies with the same name the global only contains the first one.

So this wil not change with this merge request

@paras-malhotra
Copy link
Contributor

The $_COOKIE global never contains duplicate cookies.

Ahh okay 👍

@marcvdm
Copy link
Contributor Author

marcvdm commented Nov 6, 2020

Actually i can replicate this error very easy.

Route::get('/', function () {
    return response('test')->withCookie('array_cookie[encrypted]','value');
});

If you hit the route twice it wil generate the error.

@taylorotwell taylorotwell reopened this Nov 6, 2020
@driesvints
Copy link
Member

One thing that has me thinking is maybe it's better to throw an exception so it's explicit that you're passing an array based cookie which isn't supported?

@taylorotwell
Copy link
Member

@driesvints isn't that just going to be the problem we have now? It seems like the goal of this PR is to have no error at all and silently discard the cookie?

@driesvints
Copy link
Member

@taylorotwell right.. that's true. Bit torn on this one. Maybe we should just keep the exception throwing but give a more clear exception.

@marcvdm
Copy link
Contributor Author

marcvdm commented Nov 6, 2020

Well, the cookie is not going to be discarded just not decrypted. You can still access it.

Another issue is that when a cookie is set as cookie('array_cookie[encrypted]','value'); the key used for the prefix is going to be array_cookie[encrypted]. but that is never reconstructed so an array cookie with encrypted data is neven decrypted with the right key.

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.

4 participants