-
Notifications
You must be signed in to change notification settings - Fork 23.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FW][FIX] website: prevent creation of 308 to existing controller #166070
[FW][FIX] website: prevent creation of 308 to existing controller #166070
Conversation
@rdeodoo @qsm-odoo cherrypicking of pull request #165083 failed. stdout:
stderr:
Either perform the forward-port manually (and push to this branch, proceeding as usual) or close this PR (maybe?). In the former case, you may want to edit this PR message as well. More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port |
23787c2
to
bdd35fe
Compare
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 X-original-commit: cd49b69
bdd35fe
to
6ecd260
Compare
@robodoo r+ |
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 closes #166070 X-original-commit: cd49b69 Signed-off-by: Quentin Smetz (qsm) <qsm@odoo.com> Signed-off-by: Romain Derie (rde) <rde@odoo.com>
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:
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:
Technically, here is the routing map for both cases:
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: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 theWebsiteSale.shop
.opw-3901713
Forward-Port-Of: #165083