itsgoingd / clockwork

Clockwork - php dev tools in your browser - server-side component
https://underground.works/clockwork
MIT License
5.66k stars 320 forks source link

Clockwork prevents any other prs7 cookies being set #467

Closed leemason closed 3 years ago

leemason commented 3 years ago

Ive just come across an issue where clockwork is destroying all other cookies sent on the request.

Specifically here: https://github.com/itsgoingd/clockwork/blob/ea3d4903c06f037af5446780be59e9c94dcbfe23/Clockwork/Support/Vanilla/Clockwork.php#L314

The problem is it's forgetting about any other cookies set on the response. It can be resolves by putting the clockwork middleware lower down your stack and having your applications cookie middleware before, but especially in my application the whole idea is to have clockwork as the first middleware to enable tracking of all the application middleware.

Cookies aren't quite as simple as added headers, so maybe bringing in something FigCookies to handle this would be a good approach?

itsgoingd commented 3 years ago

Hey, thanks for the report! I'm no expert in PSR-7, but looking at the spec, isn't the fix as easy as calling withAddedHeader instead of withHeader so the original values won't get overwritten?

leemason commented 3 years ago

Not quite unfortunately. Set-Cookie can only be 1 header line and its not delimited the same way as other headers so you have "manage it" differently.

The simplest way to manage it is with this library: https://github.com/dflydev/dflydev-fig-cookies#set-a-response-cookie but obviously its a new dependency and up to you if you want keep deps down.

itsgoingd commented 3 years ago

Are you sure? From the output of curl -D - https://github.com/itsgoingd/clockwork:

HTTP/1.1 200 OK
...
Set-Cookie: _gh_sess=65MZTqAqDoTJBvhF9SVyiJb4bArKGJEDH%2FdwrTH%2BUpHISFv532iil2UvTPkzxHSA0D6mchOYrW7jNgy%2FEdiUT0vC99eeD9E8su389xXyc9q%2FLyuq7nxwESNG1HHM%2BoVFBT5rQz%2B6AgEbZzcbUsTfYhfl4SybLJDiD2NJCu3JTQDceHRAc2DAMf41Wj5d9R%2FzefDoFfFuX8dcuQm0Q54vMVOBRBCraKkaoUC%2BztmlKETzDz7dbpzAfwR7RyZHdHFkCsa0xzsStGytJa6uWwnuXw%3D%3D--sWAFfjgkyIpkadKY--0Y9%2B7m90lzHyrgZ0Fppn%2Bg%3D%3D; Path=/; HttpOnly; Secure; SameSite=Lax
Set-Cookie: _octo=GH1.1.1266900100.1609016778; Path=/; Domain=github.com; Expires=Sun, 26 Dec 2021 21:06:18 GMT; Secure; SameSite=Lax
Set-Cookie: logged_in=no; Path=/; Domain=github.com; Expires=Sun, 26 Dec 2021 21:06:18 GMT; HttpOnly; Secure; SameSite=Lax
...

I'm pretty sure you can have multiple Set-Cookie headers, will have to try things out.

Sorry for breaking this in a minor release, must have been annoying to debug. 😕

leemason commented 3 years ago

Sorry i might of confused things with my last response, im not a cookie expert either. Its not soo much the individual cookies, it's the withHeader call. This replaces any previously set headers of the same name, ie any previously set Set-Cookie headers.

The cookie case makes this even more of an issue as with most headers that have multiple values can be comma seperated strings, but cookies are not.

The way this is solved with the package i mentioned is to have just one Set-Cookie header, and when you want to add cookies, the cookie value is appended to the single header with a colon.

No probs on the break for me, its a new framework im working on and clockwork is awesome for debugging it. Really awesome work.

Im happy to submit a pr. just might take a few days or so given the festive period.

itsgoingd commented 3 years ago

Tested my original fix idea in a sample Zend Expressive project and looks like it works fine. Also exposed a method returning the cookie payload in case you ever need to set it manually.

Tagged 5.0.6 with these changes, let me know if it works for you.

Thanks for the PR offer, I wanted to fix this one fast since it was introduced in the last minor release.

leemason commented 3 years ago

Just pulled in latest and this is looking like its solved the issue.

Top work!