Skip to content
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

[FIX] website: prevent creation of 308 to existing controller #165083

Closed

Conversation

rdeodoo
Copy link
Contributor

@rdeodoo rdeodoo commented May 10, 2024

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>>)>,
  1. 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

@robodoo
Copy link
Contributor

robodoo commented May 10, 2024

@rdeodoo rdeodoo force-pushed the 17.0-fix-308-existing-controller-rde branch from 0b6532e to 5379828 Compare May 10, 2024 11:42
@rdeodoo rdeodoo marked this pull request as ready for review May 10, 2024 11:42
@C3POdoo C3POdoo requested a review from a team May 10, 2024 11:44
@C3POdoo C3POdoo added the OE the report is linked to a support ticket (opw-...) label May 10, 2024
@rdeodoo rdeodoo force-pushed the 17.0-fix-308-existing-controller-rde branch 2 times, most recently from 6ba7d8a to ad4ea9a Compare May 10, 2024 12:08
@rdeodoo
Copy link
Contributor Author

rdeodoo commented May 10, 2024

@qsm-odoo Do you want to review this one? Or dispatch it (to the team or JUC/JKE?)

@rdeodoo rdeodoo force-pushed the 17.0-fix-308-existing-controller-rde branch from ad4ea9a to 290ddfa Compare May 10, 2024 12:23
@qsm-odoo
Copy link
Contributor

I'll review it.

Copy link
Contributor

@qsm-odoo qsm-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rdeodoo I have a weird bug + a question

  1. Install website_sale
  2. Create a 308 rewrite from /shop to /, save
  3. You get the homepage error, nice.
  4. Change / to /contactus -> you can. Question: should not we prevent that also? 🤔
  5. Change /contactus to /@ -> you can ! Somehow this does not work if you create the rewrite rule with /@ but it works if you modify the existing one with it 🤷

@rdeodoo rdeodoo force-pushed the 17.0-fix-308-existing-controller-rde branch from 290ddfa to a035c45 Compare May 16, 2024 13:25
@rdeodoo
Copy link
Contributor Author

rdeodoo commented May 16, 2024

Good catch @qsm-odoo

You literally found the only controller ending with a trailing slash (as we made an effort to remove those from route definitions over the years), which is the /@/ because it's not possible to define it as /@ technically.

See here, you can grep /':

List of routes in local with a few modules

['/mail/rtc/channel/cancel_call_invitation', '/mail/rtc/session/update_and_broadcast', '/mail/rtc/session/notify_call_members', '/mail/rtc/channel/leave_call', '/mail/rtc/channel/join_call', '/web_unsplash/attachment/add', '/web_editor/attachment/add_data', '/web_editor/attachment/add_url', '/web_editor/attachment/remove', '/web_editor/video_url/data', '/calendar/recurrence/decline', '/calendar/recurrence/accept', '/calendar/meeting/decline', '/calendar/meeting/accept', '/calendar/meeting/join', '/calendar/meeting/view', '/profile/validate_email/close', '/mailing/blocklist/remove', '/mailing/blocklist/add', '/product/document/upload', '/discuss/channel/update_custom_notifications', '/product/catalog/update_order_line_info', '/discuss/channel/set_last_seen_message', '/product/catalog/order_lines_info', '/website/snippet/filter_templates', '/discuss/channel/pinned_messages', '/website/snippet/options_filters', '/discuss/channel/update_avatar', '/discuss/channel/notify_typing', '/website/snippet/autocomplete', '/discuss/channel/attachments', '/discuss/channel/messages', '/discuss/channel/members', '/website/snippet/filters', '/discuss/channel/info', '/discuss/channel/mute', '/discuss/channel/ping', '/mailing/report/unsubscribe', '/mailing/mobile/preview', '/website/rating/comment', '/payment/status/poll', '/discuss/voice/worklet_processor', '/website/form/shop.sale.order', '/mailing/list/update', '/profile/user/save', '/discuss/gif/remove_favorite', '/discuss/gif/add_favorite', '/discuss/gif/categories', '/discuss/gif/favorites', '/discuss/gif/search', '/portal/attachment/remove', '/portal/attachment/add', '/shop/access_point/close_locations', '/mail/link_preview/delete', '/shop/access_point/get', '/shop/access_point/set', '/mail/attachment/delete', '/mail/attachment/upload', '/mail/attachment/zip', '/shop/products/recently_viewed_delete', '/shop/products/recently_viewed_update', '/shop/express/shipping_address_change', '/shop/product/is_add_to_cart_allowed', '/shop/product/resequence-image', '/mail/message/update_content', '/shop/product/extra-images', '/shop/product/clear-images', '/mail/partner/from_email', '/mail/message/translate', '/mail/history/messages', '/mail/starred/messages', '/mail/message/reaction', '/shop/payment/validate', '/mail/message/post', '/shop/config/attribute', '/mail/thread/messages', '/shop/config/website', '/shop/config/product', '/mail/thread/data', '/mail/guest/update_name', '/mail/inbox/messages', '/shop/cart/update_address', '/shop/cart/update_option', '/shop/cart/update_json', '/shop/cart/quantity', '/shop/cart/update', '/shop/cart/clear', '/mail/rtc/audio_worklet_processor', '/web/webclient/bootstrap_translations', '/web/webclient/version_info', '/web/database/change_password', '/web/database/duplicate', '/web/database/selector', '/web/database/manager', '/web/database/restore', '/web/database/backup', '/web/database/create', '/web/database/drop', '/web/database/list', '/web/session/get_session_info', '/web/session/get_lang_list', '/web/session/authenticate', '/web/dataset/call_button', '/web/dataset/resequence', '/web/dataset/call_kw', '/web/session/account', '/web/session/destroy', '/web/session/modules', '/web/session/logout', '/web/session/check', '/web/partner/vcard', '/web/binary/upload_attachment', '/web/binary/company_logo', '/web/export/get_fields', '/web/domain/validate', '/web/export/namelist', '/web/export/formats', '/web/action/load', '/web/export/xlsx', '/web/action/run', '/web/export/csv', '/web/pivot/check_xlsxwriter', '/web/pivot/export_xlsx', '/web/tests/mobile', '/web/login/totp', '/web/view/edit_custom', '/web/sign/get_fonts', '/my/orders/reorder_modal_content', '/sale_product_configurator/show_advanced_configurator', '/sale_product_configurator/optional_product_items', '/sale_product_configurator/get_optional_products', '/sale_product_configurator/update_combination', '/sale_product_configurator/create_product', '/sale_product_configurator/get_values', '/website_mass_mailing/is_subscriber', '/website_mass_mailing/subscribe', '/base_import_module/login_upload', '/website_links/recent_links', '/website_links/add_code', '/website_links/new', '/test_website/test_redirect_view_qs', '/website_sale/get_combination_info', '/web_unsplash/save_unsplash', '/web_unsplash/fetch_images', '/website_mail/is_follower', '/web_unsplash/get_app_id', '/google_gmail/confirm', '/website_mail/follow', '/base_import/set_file', '/ignore_args/none', '/ignore_args/kw', '/ignore_args/a', '/web_editor/get_assets_editor_resources', '/web_editor/media_library_search', '/web_editor/save_library_media', '/web_editor/get_ice_servers', '/web_editor/get_image_info', '/web_editor/bus_broadcast', '/web_editor/generate_text', '/base_setup/demo_active', '/web_editor/checklist', '/web_editor/tests', '/web_editor/stars', '/base_setup/data', '/websocket/update_bus_presence', '/websocket/peek_notifications', '/websocket/health', '/calendar/check_credentials', '/calendar/notify_ack', '/calendar/notify', '/donation/pay', '/website/check_new_content_access_rights', '/website/theme_customize_bundle_reload', '/website/get_switchable_related_views', '/website/save_session_layout_mode', '/website/theme_customize_data_get', '/website/track_installing_modules', '/website/get_new_page_templates', '/profile/send_validation_email', '/account/export_zip_documents', '/website/get_current_currency', '/website/theme_customize_data', '/website/fetch_dashboard_data', '/website/get_suggested_links', '/website/google_maps_api_key', '/website/reset_template', '/website/iframefallback', '/profile/validate_email', '/payment/archive_token', '/website/get_languages', '/payment/confirmation', '/website/get_seo_data', '/website/configurator', '/profile/ranks_badges', '/payment/transaction', '/website/seo_suggest', '/mailing/feedback', '/website/publish', '/payment/status', '/website/search', '/website/image', '/profile/users', '/website/info', '/website/form', '/profile/edit', '/payment/pay', '/website/add', '/mailing/my', '/report/check_wkhtmltopdf', '/report/download', '/report/barcode', '/event/init_barcode_interface', '/forum/get_url_title', '/forum/get_tags', '/forum/all', '/mail/read_subscription_data', '/sale/create_product_variant', '/mail/load_message_failures', '/shop/carrier_rate_shipment', '/shop/save_shop_layout_mode', '/mail/update_is_internal', '/shop/express_checkout', '/lead/case_mark_lost', '/mail/init_messaging', '/shop/update_carrier', '/lead/case_mark_won', '/shop/confirm_order', '/mail/chatter_fetch', '/mail/link_preview', '/shop/confirmation', '/mail/chatter_init', '/mail/chatter_post', '/shop/extra_info', '/shop/pricelist', '/mail/unfollow', '/shop/checkout', '/lead/convert', '/shop/address', '/shop/payment', '/shop/print', '/mail/view', '/shop/cart', '/bus/websocket_worker_bundle', '/bus/get_model_definitions', '/web/manifest.webmanifest', '/web/service-worker.js', '/web/login_successful', '/web/reset_password', '/web/set_profiling', '/web/speedscope', '/web/content', '/web/offline', '/sms/status', '/web/health', '/web/become', '/web/signup', '/web/image', '/web/login', '/web/tests', '/my/deactivate_account', '/my/payment_method', '/my/opportunities', '/my/counters', '/my/invoices', '/my/projects', '/my/security', '/my/account', '/my/orders', '/my/quotes', '/my/leads', '/my/tasks', '/my/home', '/my/task', '/test_validation_error_http', '/test_validation_error_json', '/test_internal_error_http', '/test_internal_error_json', '/test_access_denied_http', '/test_access_denied_json', '/test_missing_error_http', '/test_missing_error_json', '/test_access_error_http', '/test_access_error_json', '/unsubscribe_from_list', '/empty_controller_test', '/multi_company_website', '/get_post_nomultilang', '/test_website_sitemap', '/test_user_error_http', '/test_user_error_json', '/test_countries_308', '/test_error_view', '/test_get_dbname', '/favicon.ico', '/sitemap.xml', '/robots.txt', '/google_map', '/websocket', '/test_view', '/contactu', '/logo.png', '/get_post', '/partners', '/jsonrpc', '/events', '/terms', '/pages', '/event', '/forum', '/shop', '/logo', '/post', '/blog', '/get', '/web', '/my', '/@/', '/r', '/', '/mail/avatar/mail.message/int:res_id/author_avatar/int:widthxint:height', '/partners/grade/<model("res.partner.grade"):grade>/country/<model("res.country"):country>/page/int:page', '/discuss/channel/int:channel_id/partner/int:partner_id/avatar_128', '/discuss/channel/int:channel_id/guest/int:guest_id/avatar_128', '/discuss/channel/int:channel_id/image/int:attachment_id/int:widthxint:height', '/forum/<model("forum.forum"):forum>/post/<model("forum.post"):post>/comment/<model("mail.message"):comment>/convert_to_answer', '/forum/<model("forum.forum"):forum>/post/<model("forum.post"):post>/comment/<model("mail.message"):comment>/delete', '/forum/<model("forum.forum"):forum>/tag/<model("forum.tag"):tag>/questions/page/int:page', '/web/image/int:id-string:unique/int:widthxint:height/string:filename', '/web/image/int:id-string:unique/int:widthxint:height', '/my/projects/int:project_id/task/int:task_id/subtasks', '/web_enterprise/partner/<model("res.partner"):partner>/vcard', '/ignore_args/converter/string:a/nokw', '/web_editor/font_to_img///int:widthxint:height/int:alpha', '/web_editor/font_to_img///int:widthxint:height', '/web_editor/font_to_img////int:widthxint:height/int:alpha', '/web_editor/font_to_img////int:widthxint:height', '/partners/country/<model("res.country"):country>/page/int:page', '/partners/grade/<model("res.partner.grade"):grade>/country/<model("res.country"):country>', '/partners/grade/<model("res.partner.grade"):grade>/page/int:page', '/discuss/channel/int:channel_id/attachment/int:attachment_id', '/discuss/channel/int:channel_id/avatar_128', '/discuss/channel/int:channel_id/image/int:attachment_id', '/website/search/page/int:page', '/website/search/string:search_type/page/int:page', '/profile/users/page/int:page', '/website/image//int:widthxint:height', '/website/image///int:widthxint:height', '/website/image////int:widthxint:height', '/forum/all/page/int:page', '/event/<model("event.event"):event>/registration/success', '/event/<model("event.event"):event>/registration/confirm', '/event/<model("event.event"):event>/registration/new', '/forum/<model("forum.forum"):forum>/question/<model("forum.post"):question>/toggle_favourite', '/forum/<model("forum.forum"):forum>/question/<model("forum.post"):question>/ask_for_close', '/forum/<model("forum.forum"):forum>/question/<model("forum.post"):question>/edit_answer', '/forum/<model("forum.forum"):forum>/question/<model("forum.post"):question>/undelete', '/forum/<model("forum.forum"):forum>/question/<model("forum.post"):question>/delete', '/forum/<model("forum.forum"):forum>/question/<model("forum.post"):question>/reopen', '/forum/<model("forum.forum"):forum>/question/<model("forum.post"):question>/close', '/forum/<model("forum.forum"):forum>/post/<model("forum.post"):post>/ask_for_mark_as_offensive', '/forum/<model("forum.forum"):forum>/post/<model("forum.post"):post>/convert_to_comment', '/forum/<model("forum.forum"):forum>/post/<model("forum.post"):post>/mark_as_offensive', '/forum/<model("forum.forum"):forum>/post/<model("forum.post"):post>/toggle_correct', '/forum/<model("forum.forum"):forum>/post/<model("forum.post"):post>/validate', '/forum/<model("forum.forum"):forum>/post/<model("forum.post"):post>/downvote', '/forum/<model("forum.forum"):forum>/post/<model("forum.post"):post>/comment', '/forum/<model("forum.forum"):forum>/post/<model("forum.post"):post>/delete', '/forum/<model("forum.forum"):forum>/post/<model("forum.post"):post>/refuse', '/forum/<model("forum.forum"):forum>/post/<model("forum.post"):post>/upvote', '/forum/<model("forum.forum"):forum>/post/<model("forum.post"):post>/edit', '/forum/<model("forum.forum"):forum>/post/<model("forum.post"):post>/flag', '/forum/<model("forum.forum"):forum>/post/<model("forum.post"):post>/save', '/forum/<model("forum.forum"):forum>/faq/karma', '/forum/<model("forum.forum"):forum>/tag/<model("forum.tag"):tag>/questions', '/shop/category/<model("product.public.category"):category>/page/int:page', '/shop/payment/transaction/int:order_id', '/mail/mailing/int:mailing_id/unsubscribe', '/mail/track/int:mail_id/string:token/blank.gif', '/blog/tag/string:tag/page/int:page', '/blog/<model("blog.blog"):blog>/tag/string:tag/page/int:page', '/web/webclient/translations/string:unique', '/web/webclient/load_menus/string:unique', '/web/webclient/locale/string:lang', '/web/dataset/call_kw/path:path', '/web/image/int:id-string:unique/string:filename', '/web/image/int:id-string:unique', '/web/image/int:id/int:widthxint:height/string:filename', '/web/image/string:xmlid/int:widthxint:height/string:filename', '/web/image/int:id/int:widthxint:height', '/web/image/string:xmlid/int:widthxint:height', '/web/image/string:model/int:id/string:field/int:widthxint:height/string:filename', '/web/image/string:model/int:id/string:field/int:widthxint:height', '/web/sign/get_fonts/string:fontname', '/my/opportunities/page/int:page', '/my/invoices/page/int:page', '/my/projects/page/int:page', '/my/projects/int:project_id/project_sharing', '/my/projects/int:project_id/page/int:page', '/my/projects/int:project_id/task/int:task_id', '/my/project/int:project_id/project_sharing', '/my/project/int:project_id/page/int:page', '/my/project/int:project_id/task/int:task_id', '/my/orders/page/int:page', '/my/quotes/page/int:page', '/my/orders/int:order_id/transaction', '/my/orders/int:order_id/document/int:document_id', '/my/orders/int:order_id/decline', '/my/orders/int:order_id/accept', '/my/leads/page/int:page', '/my/tasks/page/int:page', '/my/task/page/int:page', '/test_website_sitemap/something/<model("test.model"):rec>', '/test_website/model_item/int:record_id', '/test_website/country/<model("res.country"):country>', '/test_website/200/<model("test.model"):rec>', '/ignore_args/converteronly/string:a', '/ignore_args/converter/string:a', '/web_editor/modify_image/<model("ir.attachment"):attachment>', '/web_editor/font_to_img///int:size/int:alpha', '/web_editor/font_to_img////int:size', '/web_editor/font_to_img///int:size', '/web_editor/font_to_img///', '/web_editor/image_shape/string:img_key//path:filename', '/web_editor/font_to_img//', '/web_editor/font_to_img/', '/web_editor/shape//path:filename', '/dashboard/download/int:share_id/', '/dashboard/share/int:share_id/', '/dashboard/data/int:share_id/', '/calendar/join_videocall/string:access_token', '/donation/transaction/<minimum_amount>', '/partners/country/<model("res.country"):country>', '/partners/grade/<model("res.partner.grade"):grade>', '/partners/page/int:page', '/website/country_infos/<model("res.country"):country>', '/website/configurator/int:step', '/website/translations/string:unique', '/invoice/transaction/int:invoice_id', '/discuss/channel/int:channel_id', '/website/action/<path_or_xml_id_or_id>/path:path', '/profile/avatar/int:user_id', '/website/action/<path_or_xml_id_or_id>', '/website/search/string:search_type', '/website/social/string:social', '/website/image///', '/website/image//', '/website/force/int:website_id', '/website/image/', '/profile/user/int:user_id', '/website/lang/', '/website/form/string:model_name', '/website/add/path:path', '/mailing/int:mailing_id/unsubscribe', '/mailing/int:mailing_id/view', '/report/barcode/<barcode_type>/path:value', '/events/page/int:page', '/xmlrpc/2/', '/digest/int:digest_id/set_periodicity', '/digest/int:digest_id/unsubscribe', '/google<string(length=16):key>.html', '/pages/page/int:page', '/event/page/int:page', '/forum/user/int:user_id', '/forum/<model("forum.post"):post>/ask_for_mark_as_offensive', '/forum/<model("forum.forum"):forum>/validation_queue', '/forum/<model("forum.forum"):forum>/offensive_posts', '/forum/<model("forum.forum"):forum>/flagged_queue', '/forum/<model("forum.forum"):forum>/closed_posts', '/event/int:event_id/my_tickets', '/event/<model("event.event"):event>/community', '/forum/<model("forum.forum"):forum>/question/<model("forum.post", "[('forum_id','=',forum.id),('parent_id','=',False),('can_view', '=', True)]"):question>', '/event/<model("event.event"):event>/register', '/forum/<model("forum.forum"):forum>/partner/int:partner_id', '/model/string:page_name_slugified/page/int:page_number', '/forum/<model("forum.forum"):forum>/page/int:page', '/event/<model("event.event"):event>/page/path:page', '/forum/<model("forum.forum"):forum>/tag/string:tag_char', '/event/<model("event.event"):event>/ics', '/forum/<model("forum.forum"):forum>/faq', '/forum/<model("forum.forum"):forum>/ask', '/forum/<model("forum.forum"):forum>/new', '/forum/<model("forum.forum"):forum>/tag', '/forum/<model("forum.forum"):forum>/<model("forum.post"):post_parent>/reply', '/shop/change_pricelist/<model("product.pricelist"):pricelist>', '/shop/country_infos/<model("res.country"):country>', '/shop/category/<model("product.public.category"):category>', '/shop/product/<model("product.template"):product>', '/shop/page/int:page', '/blog/page/int:page', '/blog/tag/string:tag', '/rate/string:token/submit_feedback', '/shop/<model("product.template"):product_template>/document/int:document_id', '/blog/<model("blog.blog"):blog>/page/int:page', '/blog/<model("blog.blog"):blog>/post/<model("blog.post"):blog_post>', '/blog/<model("blog.blog"):blog>/feed', '/blog/<model("blog.blog"):blog>/tag/string:tag', '/web/speedscope/<model("ir.profile"):profile>', '/web/filestore/path:_path', '/web/content/string:model/int:id/string:field/string:filename', '/web/content/string:model/int:id/string:field', '/web/content/int:id/string:filename', '/web/content/string:xmlid/string:filename', '/web/content/int:id', '/web/content/string:xmlid', '/web/assets/int:website_id//string:filename', '/web/assets/string:unique/string:filename', '/web/bundle/string:bundle_name', '/web/image/string:model/int:id/string:field/string:filename', '/web/image/string:model/int:id/string:field', '/web/image/int:id/string:filename', '/web/image/string:xmlid/string:filename', '/web/image/int:id', '/web/image/string:xmlid', '/web/hook/string:rule_uuid', '/my/opportunity/<model('crm.lead', "[('type','=', 'opportunity')]"):opp>', '/my/invoices/int:invoice_id', '/my/projects/int:project_id', '/my/project/int:project_id', '/my/orders/int:order_id', '/my/tasks/int:task_id', '/my/task/int:task_id', '/my/lead/<model('crm.lead', "[('type','=', 'lead')]"):lead>', '/r/string:code/m/int:mailing_trace_id', '/r/string:code+', '/test_countries_308/<model("test.model"):rec>', '/test_lang_url/<model("res.country"):country>', '/onboarding/string:route_name', '/partners/<partner_id>', '/report///', '/report//', '/xmlrpc/', '/model/string:page_name_slugified/string:record_slug', '/forum/<model("forum.forum"):forum>/<model("forum.post"):question>', '/model/string:page_name_slugified', '/event/<model("event.event"):event>', '/forum/<model("forum.forum"):forum>', '/chat/int:channel_id/string:invitation_token', '/rate/string:token/int:rate', '/chat/string:create_token/string:channel_name', '/meet/string:create_token/string:channel_name', '/blog/<model("blog.blog"):blog>/<model("blog.post", "[('blog_id','=',blog.id)]"):blog_post>', '/chat/string:create_token', '/meet/string:create_token', '/shop/<model("product.template"):product>', '/blog/<model("blog.blog"):blog>', '/r/string:code', '/@/path:path']

And since I knew controllers would not end with a slash, I just trimmed the trailing slash from the "url_to" URL, not the routing rule URL which I was comparing to... I just trimmed that one too for the comparison and it's fixing the /@/ case.


About /contactus, it seems fine as the bug described here (having 2 rule for the same url) is only about controllers, it doesn't seem to lead to any bug if you select a website.page url. It will "just" make it so the page will be shadowed by the controller, exactly like if you created a /shop website.page, you actually will never be able to see it as we serve controllers before pages.

@rdeodoo rdeodoo force-pushed the 17.0-fix-308-existing-controller-rde branch from a035c45 to 41708d3 Compare May 17, 2024 14:00
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
@qsm-odoo qsm-odoo force-pushed the 17.0-fix-308-existing-controller-rde branch from 41708d3 to adaa80a Compare May 17, 2024 16:44
Copy link
Contributor

@qsm-odoo qsm-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I push-forced to adapt the comment that was now wrong

@robodoo r+

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OE the report is linked to a support ticket (opw-...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants