[FIX] payment_sips: prevent clearing the session cookie

[adwid]

When buying a product on website shop, after the payment with SIPS, the
page is redirected to an Error message: "We are not able to find your
payment, but don't worry. You should receive an email confirming your
payment in a few minutes. If the payment hasn't been confirmed you can
contact us."

To reproduce the error:

1. In Payment Acquirers, enable Sips
2. Go on website shop
3. Add a product to the cart, Checkout
4. Pay with Sips
   • Visa card number: 4100000000000000
5. Back to Web-shop, if the payment has been successfully processed,
   repeat steps 2 -> 4

Error: The message "Your payment has been successfully processed. Thank
you!" is not displayed. Instead, the message "We are not able [...] you
can contact us." is displayed.

This message is displayed when:

odoo/addons/payment/controllers/portal.py

Lines 65 to 69 in 5945806

	if not payment_transaction_ids:
	return {
	'success': False,
	'error': 'no_tx_found',
	}

i.e., when the transactions list is empty. Here is how to get the list:

odoo/addons/payment/controllers/portal.py

Lines 38 to 42 in 5945806

	def get_payment_transaction_ids():
	# return the ids and not the recordset, since we might need to
	# sudo the browse to access all the record
	# I prefer to let the controller chose when to access to payment.transaction using sudo
	return request.session.get("__payment_tx_ids__", [])

It uses the session of the request. The cookie session_id is used to
identify the current session. However, after the payment on SIPS, the
page is redirected to /payment/sips/dpn with a POST request. Since the
session cookie has the attribute SameSite=Lax and the HTTP request is
a POST, the cookie will be filtered out:
https://drive.google.com/file/d/1xfx3YWkfonO3nK-8Rew45uSoR4lkpjpY/view?usp=sharing
(Browser information: This cookie didn't specify a "SameSite" attribute
when it was stored and was defaulted to "SameSite=Lax," and was blocked
because the request was made from a different site and was not initiated
by a top-level navigation. The cookie had to have been set with
"SameSite=None" to enable cross-site usage)
As a result, the server creates a new one. This is the reason why the
transactions list is empty: the list is based on a new session.

Adding the attribute save_session = False to the route will prevent
the server from creating a new session cookie and adding it in the POST
response.

OPW-2518377

[robodoo]

Pull request status dashboard.

[adwid]

@AntoineVDV Here it is :)

[AntoineVDV]

Nice! This is a welcome fix, especially since it addresses the issue at the cookies level :)

Two remarks:

• The commit title is not super clear about what it does IMHO. Something like "[FIX] payment_sips: prevent clearing the session cookie" would maybe make more sense when browsing the commit history.
• I'd add a comment that explains very clearly why there is a save_session=False in the route, or it will be lost someday soon.

[AntoineVDV]

@robodoo r+

[odony]

Some more context information about this patch.

References:

• For a generic introduction about SameSite cookies, please see https://web.dev/samesite-cookies-explained/
• Browser support for the new SameSite spec changes rather quickly. Cfr the launch timelines of the various browsers (Chrome, Firefox, Edge) and the compatibility matrix. ⚠ Many browsers launched the new spec partially to a fraction of their users. So having a browser version that supports the new spec does not mean it is currently active for you!
• A website to test the current behavior of your browser: https://samesite-sandbox.glitch.me/

What about Odoo?

Odoo does not specify the new SameSite attribute for any cookie, and relies on the default behavior, which is supposed to be SameSite=Lax. In many aspects, using the default behavior is safer than explicitly defining SameSite=Lax, because some older browsers implement different versions of the SameSite spec and may actually break when SameSite=Lax is set.

The 2 other modes for SameSite are not suited for Odoo:

• SameSite=None means "insecure by default", and would be a bad choice from a security standpoint. It defeats the whole purpose of this new feature for improving cookie security and fighting against CSRF attacks.
• SameSite=Strict would break many parts of Odoo because it blocks cookies in top-level navigations, so the "main page request" would never have any cookies.

So, SameSite=Lax. The behavior for this mode is to allow cookies when the request is:

• a first-party request, directly initiated by a page that is from the same origin as the cookie (= SameSite=Strict behavior)
• or a top-level navigation going to the origin of the cookie

Note: the only cookie where this generally matters is session_id. Other cookies can often be missing for a single request without causing any major problems, but the session_id is the only way for the server to recognize the user session.

When using an Odoo-based website, we're normally in one of the 2 situations where the Lax policy allows cookies, so they will always be sent. There is one exception: when the user's browser is coming back from a third-party website (e.g. a payment provider). If the third-party website triggers a POST request from the user's browser back to the Odoo domain, there won't be any cookies submitted. This is expected because it's not a top-level navigation, nor a first-party request.

However at that point, the Odoo server will see no user session, and will assign a new session, returning a new session_id cookie in the response. And the browser will have to accept it, discarding the original user session 💥!

Solutions?

There are two simple solutions to avoid this issue:

1. Avoid having third-party website POST requests back to the Odoo website. Instead, have the third-party website redirect the user, in order to trigger a proper top-level navigation. This is the best option, but not always an easy thing to change depending on integration constraints.
2. Alternatively, mark the route where the POST is being sent with save_session=False, to prevent Odoo from assigning a new session if that "third-party request" comes without a session cookie. This way the user will keep their session for the next page view. Of course this only works if the route is a "stateless" route that does not require the user to be logged in, otherwise the user won't be recognized anyway. Those routes are usually marked as auth=none or auth=public.

This PR implements solution 2), to work around the behavior of the SIPS integration for the DPN POST request, with minimal changes.

Q: The devtools console tells me that Odoo cookies are going to be rejected soon!

Over time browsers have updated their developer warnings, and just like the implementations, the warnings are often incorrect, partial or misleading. The current specifications as described in RFC 6265bis clearly states that the behavior when no SameSite attribute is present should be Lax, or possibly temporary virtual mode nicknamed "Lax-Allowing-Unsafe", slightly more permissive than Lax. And this is definitely the behavior we want to keep for Odoo cookies.

When integrations fail due to cookies being restricted, the solution cannot be to change the session_id cookie to SameSite=None, as that would require an insecure default for Odoo globally.

[AntoineVDV]

@odony Wow, that was incredibly clear and informative. This should definitely be integrated into the doc!

[wahibimoh]

Some more context information about this patch.

References:

• For a generic introduction about SameSite cookies, please see https://web.dev/samesite-cookies-explained/
• Browser support for the new SameSite spec changes rather quickly. Cfr the launch timelines of the various browsers (Chrome, Firefox, Edge) and the compatibility matrix. ⚠ Many browsers launched the new spec partially to a fraction of their users. So having a browser version that supports the new spec does not mean it is currently active for you!
• A website to test the current behavior of your browser: https://samesite-sandbox.glitch.me/

What about Odoo?

Odoo does not specify the new SameSite attribute for any cookie, and relies on the default behavior, which is supposed to be SameSite=Lax. In many aspects, using the default behavior is safer than explicitly defining SameSite=Lax, because some older browsers implement different versions of the SameSite spec and may actually break when SameSite=Lax is set.

The 2 other modes for SameSite are not suited for Odoo:

• SameSite=None means "insecure by default", and would be a bad choice from a security standpoint. It defeats the whole purpose of this new feature for improving cookie security and fighting against CSRF attacks.
• SameSite=Strict would break many parts of Odoo because it blocks cookies in top-level navigations, so the "main page request" would never have any cookies.

So, SameSite=Lax. The behavior for this mode is to allow cookies when the request is:

• a first-party request, directly initiated by a page that is from the same origin as the cookie (= SameSite=Strict behavior)
• or a top-level navigation going to the origin of the cookie

Note: the only cookie where this generally matters is session_id. Other cookies can often be missing for a single request without causing any major problems, but the session_id is the only way for the server to recognize the user session.

When using an Odoo-based website, we're normally in one of the 2 situations where the Lax policy allows cookies, so they will always be sent. There is one exception: when the user's browser is coming back from a third-party website (e.g. a payment provider). If the third-party website triggers a POST request from the user's browser back to the Odoo domain, there won't be any cookies submitted. This is expected because it's not a top-level navigation, nor a first-party request.

However at that point, the Odoo server will see no user session, and will assign a new session, returning a new session_id cookie in the response. And the browser will have to accept it, discarding the original user session 💥!

Solutions?

There are two simple solutions to avoid this issue:

1. Avoid having third-party website POST requests back to the Odoo website. Instead, have the third-party website redirect the user, in order to trigger a proper top-level navigation. This is the best option, but not always an easy thing to change depending on integration constraints.
2. Alternatively, mark the route where the POST is being sent with save_session=False, to prevent Odoo from assigning a new session if that "third-party request" comes without a session cookie. This way the user will keep their session for the next page view. Of course this only works if the route is a "stateless" route that does not require the user to be logged in, otherwise the user won't be recognized anyway. Those routes are usually marked as auth=none or auth=public.

This PR implements solution 2), to work around the behavior of the SIPS integration for the DPN POST request, with minimal changes.

Q: The devtools console tells me that Odoo cookies are going to be rejected soon!

Over time browsers have updated their developer warnings, and just like the implementations, the warnings are often incorrect, partial or misleading. The current specifications as described in RFC 6265bis clearly states that the behavior when no SameSite attribute is present should be Lax, or possibly temporary virtual mode nicknamed "Lax-Allowing-Unsafe", slightly more permissive than Lax. And this is definitely the behavior we want to keep for Odoo cookies.

When integrations fail due to cookies being restricted, the solution cannot be to change the session_id cookie to SameSite=None, as that would require an insecure default for Odoo globally.

Sounds good, but also iframe are impacted as well. I propose that we do the following in odoo/http.py:

Change 'set_cookie' From:

    @functools.wraps(werkzeug.Response.set_cookie)
    def set_cookie(self, key, value='', max_age=None, expires=None, path='/', domain=None, secure=False, httponly=False, samesite=None, cookie_type='required'):
        if request.db and not request.env['ir.http']._is_allowed_cookie(cookie_type):
            expires = 0
            max_age = 0
        werkzeug.Response.set_cookie(self, key, value=value, max_age=max_age, expires=expires, path=path, domain=domain, secure=secure, httponly=httponly, samesite=samesite)

To:

    @functools.wraps(werkzeug.Response.set_cookie)
    def set_cookie(self, key, value='', max_age=None, expires=None, path='/', domain=None, secure=False, httponly=False, samesite=None, cookie_type='required'):
        if request.db and not request.env['ir.http']._is_allowed_cookie(cookie_type):
            expires = 0
            max_age = 0
	samesite_override = self.env['ir.config_parameter'].sudo().get_param('website.cookie_samesite_override')
        if samesite_override:	
		samesite = samesite_override
	secure_override = self.env['ir.config_parameter'].sudo().get_param('website.cookie_secure_override')
        if secure_override:	
		secure = secure_override	
        werkzeug.Response.set_cookie(self, key, value=value, max_age=max_age, expires=expires, path=path, domain=domain, secure=secure, httponly=httponly, samesite=samesite)

This way, the default behavior will be as is today, with giving an option to Saas user an option to change it in system parameters if they need it.