-
Notifications
You must be signed in to change notification settings - Fork 546
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
Fix #4710 - Adds language switcher component cookie set options for secure, httpOnly, sameSite + interop.cs/interop.js methods samesite and secure options #4712
Conversation
@sbwalker you can review, merge if it will be OK however should be brought to attention for anyone upgrading I am not sure if this will cause breaking changes. Maybe you can come up with a way it wont by introducing a second method? You can review this, close it and/or modify things as needed with your own updated version as you feel would work best. Thank you. |
I will merge this PR and then fix the breaking changes, etc... in a subsequent PR |
Fixes #4710
DRAFT
Issues Resolved
This PR allows setting different cookies SameSite and Secure cookie attributes via javascript interop setCookie method.
Language switcher cookie set options for "secure, httpOnly, sameSite" is also done in the code behind logic.
Also the NavigateTo function was simplified to use "true" instead of explicitly setting force reload to true.
HttpOnly can only be set server-side and so it is not set client-side via js interop as a note. This has caused issues with interactive render mode due to SignalR and JavaScript.
This is a work in progress at this point.
Additional Context
This resolves the issue but should be tested for further side effects for any third party modules that may need to be informed to make this update adding the new values to this function when used to set cookies.
Maybe a breaking change due to having to pass new settings in the method to the JavaScript interop, but it would be on the side of security.
We may need to create a second interop.js method
setCookieSecure { }
and deprecate the othersetCookie { ]
to allow time and build developer awareness to create cookies using this alternative method. Or we can just simply set these without sending the attribute settings within the current 'setCookie { } method'Please review as this will allow these cookies to not flash warnings in browser when set.
This is an example of how I believe it could be implemented allowing developers to create cookies with different SameSite and Secure Cookie settings.
Working to also resolve other issues in this area so I would like to keep this a draft and also await feedback.
Discussion #4703
I created Issue #4714 to discuss this as these are in the same logic being updated in this PR and can be addressed as well here. Or we can keep them two different PR's.
It would not be hard to create the conditional logic for the render mode interactive to set the
HttpOnly=False
to resolve this issue as well.The issue here is while in interactive modes (SignalR) Oqtane uses JavaScript to create cookies and the
HttpOnly
setting in this mode set totrue
will not access the cookie. This is true for theCulture
cookie for theLanguageSwitcher.razor
component and needs reviewed for theVisitor
cookie as well. We will need to create logic to handle which render mode is being set in the site settings to determine which cookie setting to use if we are unable to work this issue out in code. JavaScript interop won’t be able to access these cookies directly ifHttpOnly=true
.Visitor Cookie should be reviewed to see if it is functioning in both modes as intended. This is created server side and not sure it needs to be written to like the culture cookie with same issue described while in interactive mode.