[pve-devel] [PATCH widget-toolkit] fix external linking to products by setting cookie SameSite attribute to lax

[Dominik Csapak]

On 10/14/24 13:57, Dominik Csapak wrote:
> On 10/8/24 14:10, Max Carrara wrote:
>> On Mon Oct 7, 2024 at 5:02 PM CEST, Dominik Csapak wrote:
>>> We introduced the 'strict' setting when browsers warned about our
>>> cookies not having any SameSite setting [0]
>>>
>>> While this works in general, it had an unforeseen side effect:
>>>
>>> When linking to a URL of our products, the cookie does not get sent on
>>> the initial page load, leading to the username/CSRFPreventionToken not
>>> being set in the index response.
>>>
>>> Our UI code interprets this as being logged out (e.g. because the ticket
>>> is not valid) and clears the cookie, displaying the login window.
>>>
>>> The MDN reference[1] says that setting it to 'lax' is similar to
>>> 'strict', but sends the cookie when navigating *to* our origin even from
>>> other sites, which is what we want when linking from elsewhere. (This
>>> would have also been the default if we wouldn't have set any attribute).
>>>
>>> 0: https://lore.proxmox.com/pve-devel/20230315162630.289768-1-m.carrara@proxmox.com/
>>> 1: https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#SameSite_attribute
>>>
>>> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
>>
>> As mentioned off list, this seems fine to me, as our API doesn't have
>> any side effects during GET requests (otherwise, we'd have different
>> problems anyway ...) and we only modify, delete etc. via other methods.
>>
>> Though, as you probably saw in the series I had sent in back then, I
>> also set SameSite=Strict in a couple other places:
>>
>> - pve-apiclient/trees/master/src/PVE/APIClient/LWP.pm
> 
> is not really relevant, as it's not used for any browser
> 
>> - pve-http-server/trees/master/src/PVE/APIServer/Formatter.pm
>> - pve-http-server/trees/master/src/PVE/APIServer/Formatter/Bootstrap.pm
> 
> yes those two we want to change too, i can send a followup for pve-http-server

actually only the bootstrap one should be relevant, since the other
is also not used outside of the proxy connection AFAICS

> 
>>
>> I guess it would make sense to set them to `lax` there too?
>>
>>> ---
>>> Alternatively, we could try to modify the client code to retry with the
>>> content of the cookie (which the client still has), to generate a
>>> ticket. Though I'm not sure that will work correctly, since I observed
>>> some even stricter behaviour in Firefox ESR, where not only the
>>> navigation part did not send the cookie, but also subsequent requests
>>> did not do that (I did not test for long, so this may have been an
>>> artifact of a different issue).
>>>
>>> I'd generally prefer this solution, since it's less intrusive and
>>> restores the behaviour we had before the change, altough I'm not against
>>> implementing a retry in the client code if that's preferred.
>>>
>>>   src/Utils.js | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/Utils.js b/src/Utils.js
>>> index 7dd034a..b68c0f4 100644
>>> --- a/src/Utils.js
>>> +++ b/src/Utils.js
>>> @@ -317,7 +317,7 @@ utilities: {
>>>       // that way the cookie gets deleted after the browser window is closed
>>>       if (data.ticket) {
>>>           Proxmox.CSRFPreventionToken = data.CSRFPreventionToken;
>>> -        Ext.util.Cookies.set(Proxmox.Setup.auth_cookie_name, data.ticket, null, '/', null, true, 
>>> "strict");
>>> +        Ext.util.Cookies.set(Proxmox.Setup.auth_cookie_name, data.ticket, null, '/', null, true, 
>>> "lax");
>>>       }
>>>       if (data.token) {
>>> @@ -343,7 +343,7 @@ utilities: {
>>>           return;
>>>       }
>>>       // ExtJS clear is basically the same, but browser may complain if any cookie isn't "secure"
>>> -    Ext.util.Cookies.set(Proxmox.Setup.auth_cookie_name, "", new Date(0), null, null, true, 
>>> "strict");
>>> +    Ext.util.Cookies.set(Proxmox.Setup.auth_cookie_name, "", new Date(0), null, null, true, "lax");
>>>       window.localStorage.removeItem("ProxmoxUser");
>>>       },
>>
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel at lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>