Don't bother pattern matching existing set-cookie for deletion.

[ioquatix]

Fixes #1840

[ioquatix]

I'm honestly not sure what these tests were trying to achieve. Our deletion logic doesn't matter at all if browsers don't implement it correctly. Because I imagine 99% of people trying to delete cookies would be doing so after the fact it was set in a prior request, so the tests really don't reflect reality.

[leahneukirchen]

Please don't commit .DS_Store files..

One use case could be to have middle-ware that removes cookies from some other layer, but I'm not sure how useful that is.

[ioquatix]

Please don't commit .DS_Store files..

Oh lol, I don't even know how that happened, I was committing from Linux, but I think I copied the entire repo from my old development laptop haha. Thanks for pointing it out! 😊

One use case could be to have middle-ware that removes cookies from some other layer, but I'm not sure how useful that is.

Cookie deletion still works even in this case, but we depend on the browser to do the right thing, which I tested and it appears to work as expected (and according to relevant RFCs).

I discussed this with @tenderlove and he said there is a potential security angle which would be someone adding a cookie with a sensitive value and then later deleting it. With this change we send the creation (with sensitive data) and then follow with a deletion cookie. This would all go to the browser which would in all likelihood add then remove the cookie. But I think the consensus here was that this is a poorly implemented app.

The other angle is performance. If someone added a huge cookie and then later deleted it in the same request, expecting the entire overhead to be gone... Yes there is some impact. But again, I'm not sure this is well-formed behaviour/app.

[jeremyevans]

It seems only benefit of removing the scanning is for performance in the general case. Do we have any indication that this is actually a performance issue worth fixing? It seems both the security (remove secret data) and size (remove large data) issues, even if they do not affect most applications, are reasonable reasons against this change.

Looking at the code, the only reason for delete_set_cookie_header! is when you want to scan and remove an existing cookie. Otherwise, you would just use delete_set_cookie_header.

@tenderlove merged this as I was typing this, but I think this should be reverted.

[tenderlove]

If you're sending sensitive information, and then relying on this to delete it, I think your app has bigger problems. Specifically adding sensitive info, then trying to remove it later seems like a "your app problem" not a "rack should deal with this" problem. Additionally, the regex match used for detecting if it's the right cookie could be problematic. Since the app developer has access to the array and can mutate it themselves, why is it our responsibility to try and parse their strings?

(My main gripe with this method are the regular expressions. If we kept the cookies in a more structured format like objects, I think removing existing ones would be fine)

[jeremyevans]

I agree the method implementation is ugly. However, the only reason for delete_set_cookie_header! is to do this removal. If we don't want to do this removal, it would be better to deprecate the method, and just tell people to switch to delete_set_cookie_header. Most internal callers of delete_set_cookie_header! are already deprecated, and delete_cookie_header! could switch to calling delete_set_cookie_header.

[ioquatix]

I'm happy to revert this PR and just deprecate the entire method if we all agree to that, I'll do it today.