Skip to content

Commit

Permalink
[FIX] website: prevent creation of 308 to existing controller
Browse files Browse the repository at this point in the history
Creating a 308 which redirects to an existing controller will have
unpredictable (and unwanted) behaviors.

Indeed, 308 are there to redirect an existing URL (like `/shop`) to a
non-existing URL (like `/my-super-shop`) and to make it so that non
existing URL will respond with the content of the existing URL.

The way it's done is that it simply replace the routing map rule for the
given URL by two new rules:
- One for the non-existing URL (chosen url_to) which will serve the
  existing url endpoint
- One for the existing URL, which will be turned into a redirect
  endpoint

This works fine except if you actually select an existing controller as
url_to in the 308 rewrite.

In this case, there will be 2 werkzeug Rules for the same URL, which is
bad. Worst than that, depending of the selected controller the order of
those 2 Rules will change, leading to different behavior.

Step to reproduce:
- Create a 308 from /blog to / (note that "/" is a controller)
- Go to /blog, it will redirect and show the homepage
- Go to /, it will show the homepage
- Now edit the 308 and redirect /shop to /
- Go to /shop, it redirects to / but won't show the homepage, it will
  show the shop page
- Go to /, it will show the shop page

Technically, here is the routing map for both cases:
1. 308 shop case
   ```
   <FasterRule '/' -> functools.partial(<bound method WebsiteSale.shop of <odoo.http.CustomerPortal (extended by PortalAccount, PaymentPortal, CustomerPortalExternalTax, SaleStockPortal, CustomerPortal, PaymentPortal, PaymentPortal, WebsiteSaleDelivery, WebsiteSaleExternalTaxCalculation, WebsiteSale, WebsiteSaleStockRenting, WebsiteSaleStockRenting, WebsiteSale, WebsiteSaleRenting, PaymentPortal, CustomerPortalExternalTax, CustomerPortal, WebsiteAccount) object at 0x7f4d9468a6b0>>)>,
   <FasterRule '/' -> functools.partial(<bound method Website.index of <odoo.http.Home (extended by Home, Home, Routing, AuthSignupHome, Website) object at 0x7f4d94583460>>)>,
   ```
2. 308 blog case
   ```
    <FasterRule '/' -> functools.partial(<bound method Website.index of <odoo.http.Home (extended by Home, Home, Routing, AuthSignupHome, Website) object at 0x7f7fc9ebd7e0>>)>,
    <FasterRule '/' -> functools.partial(<bound method WebsiteBlog.blog of <odoo.http.WebsiteBlog object at 0x7f7fc9d69090>>)>,
   ```

You see that the Rule order is inverted from one case to another.

We could have decided to do another fix and adapt the
`_generate_routing_rules()` method to keep only one Route but that seems
worst as:
1. 308 are not designed for that in the first place, not even sure what
   we would want
2. it will technically be far from ideal, having the check routing map
   to check if exists already and ensure the same behavior all the time

Note that testing a few main controllers, only the /shop seems to lead
to this different behavior.

Note that it's a bit of a non-stable change, so 17.0 seems like a good
compromise. Especially since the /shop example is not buggy before 17.0
as somehow the `Website.index` Rule is before the `WebsiteSale.shop`.
```
<FasterRule '/' -> functools.partial(<bound method Website.index of <odoo.http.Home (extended by Home, Home, Routing, AuthSignupHome, Website, WebsiteTest) object at 0x7fad28571660>>)>,
<FasterRule '/' -> functools.partial(<bound method WebsiteSale.shop of <odoo.http.WebsiteSale (extended by WebsiteSaleDelivery, WebsiteSale) object at 0x7fad28435e40>>)>,
```

opw-3901713
  • Loading branch information
rdeodoo authored and qsm-odoo committed May 17, 2024
1 parent 6b03497 commit adaa80a
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 0 deletions.
17 changes: 17 additions & 0 deletions addons/website/i18n/website.pot
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,23 @@ msgstr ""
msgid "\"URL from\" can not be empty."
msgstr ""

#. module: website
#. odoo-python
#: code:addons/website/models/website_rewrite.py:0
#, python-format
msgid ""
"\"URL to\" cannot be set to \"/\". To change the homepage content, use the "
"\"Homepage URL\" field in the website settings or the page properties on any"
" custom page."
msgstr ""

#. module: website
#. odoo-python
#: code:addons/website/models/website_rewrite.py:0
#, python-format
msgid "\"URL to\" cannot be set to an existing page."
msgstr ""

#. module: website
#. odoo-python
#: code:addons/website/models/website_rewrite.py:0
Expand Down
12 changes: 12 additions & 0 deletions addons/website/models/website_rewrite.py
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,18 @@ def _check_url_to(self):
for param in re.findall('/<.*?>', rewrite.url_to):
if param not in rewrite.url_from:
raise ValidationError(_('"URL to" cannot contain parameter %s which is not used in "URL from".', param))

if rewrite.url_to == '/':
raise ValidationError(_('"URL to" cannot be set to "/". To change the homepage content, use the "Homepage URL" field in the website settings or the page properties on any custom page.'))

if any(
rule for rule in self.env['ir.http'].routing_map().iter_rules()
# Odoo routes are normally always defined without trailing
# slashes + strict_slashes=False, but there are exceptions.
if rule.rule.rstrip('/') == rewrite.url_to.rstrip('/')
):
raise ValidationError(_('"URL to" cannot be set to an existing page.'))

try:
converters = self.env['ir.http']._get_converters()
routing_map = werkzeug.routing.Map(strict_slashes=False, converters=converters)
Expand Down
1 change: 1 addition & 0 deletions addons/website/tests/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
from . import test_page_manager
from . import test_performance
from . import test_qweb
from . import test_redirect
from . import test_res_users
from . import test_snippets
from . import test_theme
Expand Down
35 changes: 35 additions & 0 deletions addons/website/tests/test_redirect.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# Part of Odoo. See LICENSE file for full copyright and licensing details.

from odoo.models import ValidationError
from odoo.tests import TransactionCase, tagged


@tagged('-at_install', 'post_install')
class TestWebsiteRedirect(TransactionCase):
def test_01_website_redirect_validation(self):
with self.assertRaises(ValidationError) as error:
self.env['website.rewrite'].create({
'name': 'Test Website Redirect',
'redirect_type': '308',
'url_from': '/website/info',
'url_to': '/',
})
self.assertIn('homepage', str(error.exception))

with self.assertRaises(ValidationError) as error:
self.env['website.rewrite'].create({
'name': 'Test Website Redirect',
'redirect_type': '308',
'url_from': '/website/info',
'url_to': '/favicon.ico',
})
self.assertIn('existing page', str(error.exception))

with self.assertRaises(ValidationError) as error:
self.env['website.rewrite'].create({
'name': 'Test Website Redirect',
'redirect_type': '308',
'url_from': '/website/info',
'url_to': '/favicon.ico/', # trailing slash on purpose
})
self.assertIn('existing page', str(error.exception))

0 comments on commit adaa80a

Please sign in to comment.