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

Forces "secure" to be re-added to parsed cookies #1964

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

maxrolon
Copy link

At Half Helix, we've recently had an issue with submitting a critical HTTP-based password form on Shopify sites when the site is proxied with BrowserSync. This is relevant to sites that use: https://kit.halfhelix.com for development, which in turn uses BrowserSync behind the scenes.

The issue stems from cookies that have SameSite=Note (see link below). Since SameSite=None requires "secure" to be set but secure does not have a key value pair, the BrowserSync code is removing "secure" from the cookie definition and thus invalidating this type of cookie.

This is likely to have downstream effects so it might be too primitive to merge in immediately but it is an issue that needs to be addressed.

See: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite#samesitenone_requires_secure.

@shakyShane
Copy link
Contributor

@maxrolon Thanks for this.

The only problem I can imagine would be if rawCookie.match(/secure/i) ended up adding 'secure' to cookies where it shouldn't - eg: because 'secure' is a common english word.

Could you match more accurately? for example by checking if it's the last item in the string or something?

@shakyShane
Copy link
Contributor

given the size of the Browsersync user-base and hence the possibility of breakage, another option would be for me to introduce a callback handler as an option that gives direct access to the cookie after processing.

@maxrolon
Copy link
Author

Yea for sure, that's a good call out. My primarily concern there is that the current solution will fundamentally not work with SameSite=None cookies as browsers gradually update their rejection of insecure SiteSite=None cookies. Perhaps we can update the code to be acutely specific to this use case and add some tests to support it?

Related to this feature I believe: https://chromestatus.com/feature/5633521622188032

@shakyShane
Copy link
Contributor

@maxrolon yes sounds good to me, would you like to attempt to do so? Parts of the codebase are many, many years old, but there's a good testing setup that you'd be able to work in a TDD way

@shakyShane
Copy link
Contributor

@maxrolon I would also like to offer my time/help if you get stuck

@maxrolon
Copy link
Author

@shakyShane Thanks! I don't think it is gonna be too crazy to implement, just need to carve out a couple hours. Happy to take this on

@maxrolon
Copy link
Author

@shakyShane Add a more specific statement and added a test for this use case, let me know if you have any other ideas to improve this update.

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.

2 participants