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

[Dominik Csapak]

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

> 
> 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
> 
>