Use the admin path name variable to allow the prefix value to be changed #196

delyriand · 2023-05-16T07:41:15Z

and see the comment in #118 (comment)

delyriand · 2024-09-03T12:33:22Z

Can I have a review? @GSadee or @diimpp?

diimpp

Hi @delyriand, thank you for your contribution. 🙏

Since optimal solution is unavailable due BC promise and flex recipe, both Resources/config/app/routing.yml and Resources/config/app/ajax.yml files should be left where they are, but content can be changed.

So what do you say changing it like so,

# config/app/routing.yml
sylius_admin_order_creation:
    resource: "@SyliusAdminOrderCreationPlugin/Resources/config/app/routing/admin.yml"
    prefix: "/%sylius_admin.path_name%"

sylius_admin_order_creation_ajax:
    resource: "@SyliusAdminOrderCreationPlugin/Resources/config/app/ajax.yml"

and

# config/app/ajax.yml
sylius_admin_order_creation_admin_ajax:
    resource: "@SyliusAdminOrderCreationPlugin/Resources/config/app/routing/admin/ajax.yml"
    prefix: "/%sylius_admin.path_name%/ajax"

Yeah, second sylius_admin_order_creation_admin_ajax declaration forces to add admin_ajax to make it unique, which is correct overall name, but doesn't match existing ajax route declaration, but shouldn't introduce any issues and acceptable compromise for me.

Keeping routes at config/app/routing instead of config/routing is acceptable since main file config/app/routing.yml already there and cannot be moved.

diimpp · 2024-09-03T17:20:31Z

src/Resources/config/app/routing/admin_ajax.yml


 sylius_admin_order_creation_ajax_provide_available_shipping_methods:
-    path: /admin/orders/available-shipping-methods/{customerId}/{channelCode}/{shipmentNumber}
+    path: /orders/available-shipping-methods/{customerId}/{channelCode}/{shipmentNumber}


While this is correct change since route before this was /admin/ajax/admin/..., it's a sort of BC and should be avoided if possible.

diimpp · 2024-09-03T17:23:04Z

src/Resources/config/app/routing/admin_ajax.yml

Renaming this file, which could have been imported directly before is a sort of BC.

diimpp · 2024-09-03T17:25:58Z

src/Resources/config/app/routing.yml

+    prefix: "/%sylius_admin.path_name%"
+    resource: "@SyliusAdminOrderCreationPlugin/Resources/config/app/routing/admin.yml"


Suggested change

prefix: "/%sylius_admin.path_name%"

resource: "@SyliusAdminOrderCreationPlugin/Resources/config/app/routing/admin.yml"

resource: "@SyliusAdminOrderCreationPlugin/Resources/config/app/routing/admin.yml"

prefix: "/%sylius_admin.path_name%"

diimpp · 2024-09-03T17:27:40Z

src/Resources/config/app/routing.yml

-    methods: [GET]
-    defaults:
-        _controller: Sylius\AdminOrderCreationPlugin\Controller\SelectNewOrderCustomerAction



Suggested change

diimpp · 2024-09-03T17:29:00Z

src/Resources/config/app/routing/admin.yml

+    prefix: "/ajax"
+    resource: "@SyliusAdminOrderCreationPlugin/Resources/config/app/routing/admin_ajax.yml"


Suggested change

prefix: "/ajax"

resource: "@SyliusAdminOrderCreationPlugin/Resources/config/app/routing/admin_ajax.yml"

resource: "@SyliusAdminOrderCreationPlugin/Resources/config/app/routing/admin_ajax.yml"

prefix: /ajax

esserj · 2025-06-13T11:59:02Z

⚠️ SECURITY NOTICE
⚠️ This is technically a BC break and I think this would expose the admin functionality without firewall:

@SyliusAdminOrderCreationPlugin/Resources/config/app/routing/admin.yml
`@SyliusAdminOrderCreationPlugin/Resources/config/app/ajax.yml

if i included these directly my routes would not end up being without the prefix and there are no further permission check build into the action handlers, resulting in exposed order creation endpoints

vvasiloi · 2025-06-13T13:26:50Z

⚠️ SECURITY NOTICE ⚠️ This is technically a BC break and I think this would expose the admin functionality without firewall:

@SyliusAdminOrderCreationPlugin/Resources/config/app/routing/admin.yml `@SyliusAdminOrderCreationPlugin/Resources/config/app/ajax.yml

if i included these directly my routes would not end up being without the prefix and there are no further permission check build into the action handlers, resulting in exposed order creation endpoints

@esserj You're not supposed to include those files directly. The Symfony Flex recipe will include the correct files automatically.

esserj · 2025-06-16T07:43:18Z

@vvasiloi I disagree
If one needs to customize certain aspects of the routing, it is perfectly valid to choose and pick routing the bundle offers..
Nowhere in the file does it say, internal, do not include it.
Symfony routing allows one to configure it as one chooses. I don't think a recipe definition is meant to be set in stone configuration?

Isn't it a bit like saying your not support to use that class?
If we offer it and it is accessible we must assume it sill be used somewhere by someone and I believe it is therefor subject to BC?

vvasiloi · 2025-06-16T08:36:45Z

@vvasiloi I disagree
If one needs to customize certain aspects of the routing, it is perfectly valid to choose and pick routing the bundle offers..
Nowhere in the file does it say, internal, do not include it.
Symfony routing allows one to configure it as one chooses. I don't think a recipe definition is meant to be set in stone configuration?

Isn't it a bit like saying your not support to use that class?
If we offer it and it is accessible we must assume it sill be used somewhere by someone and I believe it is therefor subject to BC?

@esserj Of course, you may use and customize the bundle anyway you want. In fact, one of the main ideas behind the Sylius project is to make it flexible and extensibile and to facilitate custom development. Despite that, it's inevitable that there are some common practices and expectations that the developers set. When you choose to do something that doesn't align with those, then whatever the outcome is, it's your responsibility.
I don't think it's reasonable to be overly verbose and overprotective just to potentially avoid someone making mistakes by using your code in a way that is not common, or you just didn't expect it.
In this particular example, if one needs to customize certain aspects of the routing, then in case of one or few routes, one can override the config by declaring the same routes again, and in case of many or all routes, one can follow the recommendations from the Symfony docs: https://symfony.com/doc/current/bundles/override.html#routing.
There's a file that serves as a routing configuration entry point and which must be part of the BC promise because importing it is part of the standard installation process. Other files exist mainly for having a better structure and organisation and should not be part of the BC promise as it makes maintenance and development more difficult and slows the progress.

esserj · 2025-06-16T09:44:34Z

@vvasiloi fair enough, but the impact of the change is potentially so high that it would at least warrant an upgrade warning. It's also fairly trivial to maintain BC and deprecate the legacy routing files in favour of new prefixed ones to avoid exposing anyones admin only code.
I know the impact will be limited to a less than 1%, but given this is security related I believe it warrants the extra effort.

vvasiloi · 2025-06-16T10:03:09Z

@vvasiloi fair enough, but the impact of the change is potentially so high that it would at least warrant an upgrade warning. It's also fairly trivial to maintain BC and deprecate the legacy routing files in favour of new prefixed ones to avoid exposing anyones admin only code.
I know the impact will be limited to a less than 1%, but given this is security related I believe it warrants the extra effort.

@esserj, sure, I agree. Given how old this PR is, it's likely our conversation will remain hypothetical anyway.

esserj · 2025-06-16T10:57:13Z

@vvasiloi true. I did consider that, but rather have the information out and not need it then have unpleasant surprises

feat: use the admin path name variable to allow change value

025e80d

delyriand requested a review from a team as a code owner May 16, 2023 07:41

diimpp reviewed Sep 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Use the admin path name variable to allow the prefix value to be changed #196

Use the admin path name variable to allow the prefix value to be changed #196

Uh oh!

delyriand commented May 16, 2023

Uh oh!

delyriand commented Sep 3, 2024

Uh oh!

diimpp left a comment

Uh oh!

diimpp Sep 3, 2024

Uh oh!

diimpp Sep 3, 2024

Uh oh!

diimpp Sep 3, 2024

Uh oh!

diimpp Sep 3, 2024

Uh oh!

diimpp Sep 3, 2024

Uh oh!

esserj commented Jun 13, 2025

Uh oh!

vvasiloi commented Jun 13, 2025

Uh oh!

esserj commented Jun 16, 2025

Uh oh!

vvasiloi commented Jun 16, 2025

Uh oh!

esserj commented Jun 16, 2025

Uh oh!

vvasiloi commented Jun 16, 2025

Uh oh!

esserj commented Jun 16, 2025

Uh oh!

Uh oh!

		prefix: "/%sylius_admin.path_name%"
		resource: "@SyliusAdminOrderCreationPlugin/Resources/config/app/routing/admin.yml"

		prefix: "/ajax"
		resource: "@SyliusAdminOrderCreationPlugin/Resources/config/app/routing/admin_ajax.yml"

Uh oh!

Use the admin path name variable to allow the prefix value to be changed #196

Are you sure you want to change the base?

Use the admin path name variable to allow the prefix value to be changed #196

Uh oh!

Conversation

delyriand commented May 16, 2023

Uh oh!

delyriand commented Sep 3, 2024

Uh oh!

diimpp left a comment

Choose a reason for hiding this comment

Uh oh!

diimpp Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

diimpp Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

diimpp Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

diimpp Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

diimpp Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

esserj commented Jun 13, 2025

Uh oh!

vvasiloi commented Jun 13, 2025

Uh oh!

esserj commented Jun 16, 2025

Uh oh!

vvasiloi commented Jun 16, 2025

Uh oh!

esserj commented Jun 16, 2025

Uh oh!

vvasiloi commented Jun 16, 2025

Uh oh!

esserj commented Jun 16, 2025

Uh oh!

Uh oh!