public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Dominik Csapak <d.csapak@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
	Max Carrara <m.carrara@proxmox.com>
Subject: Re: [pve-devel] [PATCH widget-toolkit] fix external linking to products by setting cookie SameSite attribute to lax
Date: Mon, 14 Oct 2024 14:03:27 +0200	[thread overview]
Message-ID: <bf980759-b0bb-4372-98b2-ef717b8ad05a@proxmox.com> (raw)
In-Reply-To: <a4f014c9-46df-46f6-a7ba-1987db2266ad@proxmox.com>

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@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@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

  reply	other threads:[~2024-10-14 12:03 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-10-07 15:02 Dominik Csapak
2024-10-08 12:10 ` Max Carrara
2024-10-14 11:57   ` Dominik Csapak
2024-10-14 12:03     ` Dominik Csapak [this message]
2024-10-15 13:30 ` [pve-devel] applied: " Thomas Lamprecht

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=bf980759-b0bb-4372-98b2-ef717b8ad05a@proxmox.com \
    --to=d.csapak@proxmox.com \
    --cc=m.carrara@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal