From 698f1eace1dc4ec1307edcbf27ba804d7ccea9f4 Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Thu, 16 Jul 2026 10:23:10 +0600 Subject: [PATCH 1/2] fix(manager-api): require msorder_save for order write routes Split /api/mgr/orders into read (msorder_list) and write (msorder_save) groups so list-only manager roles cannot mutate orders via the API. --- .../minishop3/config/routes/manager.php | 109 +++++++----------- .../tests/OrdersRoutePermissionsTest.php | 100 ++++++++++++++++ .../minishop3/tests/stubs/ModxStub.php | 15 +++ 3 files changed, 159 insertions(+), 65 deletions(-) create mode 100644 core/components/minishop3/tests/OrdersRoutePermissionsTest.php create mode 100644 core/components/minishop3/tests/stubs/ModxStub.php diff --git a/core/components/minishop3/config/routes/manager.php b/core/components/minishop3/config/routes/manager.php index 71aa8978..1ed437d3 100644 --- a/core/components/minishop3/config/routes/manager.php +++ b/core/components/minishop3/config/routes/manager.php @@ -830,96 +830,75 @@ new PermissionMiddleware($modx, 'mssetting_save') ]); + // Orders reads: list/get and related GET under msorder_list (#377) $router->group('/orders', function($router) use ($modx) { $router->get('', function($params) use ($modx) { - $allParams = array_merge($_GET, $params); - - $controller = new \MiniShop3\Controllers\Api\Manager\OrdersController($modx); - return $controller->getList($allParams); - }); - // Create new order - must be before /{id} route - $router->post('', function($params) use ($modx) { - $input = file_get_contents('php://input'); - $data = json_decode($input, true) ?: []; - - $controller = new \MiniShop3\Controllers\Api\Manager\OrdersController($modx); - return $controller->create($data); + return (new \MiniShop3\Controllers\Api\Manager\OrdersController($modx)) + ->getList(array_merge($_GET, $params)); }); // Filters config - must be before /{id} route $router->get('/filters', function($params) use ($modx) { - $controller = new \MiniShop3\Controllers\Api\Manager\OrdersController($modx); - return $controller->getFilters($params); + return (new \MiniShop3\Controllers\Api\Manager\OrdersController($modx))->getFilters($params); + }); + $router->get('/{id}', function($params) use ($modx) { + return (new \MiniShop3\Controllers\Api\Manager\OrdersController($modx))->get($params); + }); + $router->get('/{id}/products', function($params) use ($modx) { + return (new \MiniShop3\Controllers\Api\Manager\OrdersController($modx))->getProducts($params); + }); + $router->get('/{id}/logs', function($params) use ($modx) { + return (new \MiniShop3\Controllers\Api\Manager\OrdersController($modx))->getLogs($params); + }); + }, [ + new PermissionMiddleware($modx, 'msorder_list') + ]); + + // Orders writes: mutations require msorder_save (#377) + $router->group('/orders', function($router) use ($modx) { + // Create new order - must be before /{id} route + $router->post('', function($params) use ($modx) { + $data = json_decode(file_get_contents('php://input'), true) ?: []; + return (new \MiniShop3\Controllers\Api\Manager\OrdersController($modx))->create($data); }); // Bulk delete - must be before /{id} route $router->delete('/bulk', function($params) use ($modx) { - $input = file_get_contents('php://input'); - $data = json_decode($input, true) ?: []; - - $controller = new \MiniShop3\Controllers\Api\Manager\OrdersController($modx); - return $controller->bulkDelete($data); + $data = json_decode(file_get_contents('php://input'), true) ?: []; + return (new \MiniShop3\Controllers\Api\Manager\OrdersController($modx))->bulkDelete($data); }); // Finalize order (convert draft to final) - must be before /{id} route $router->post('/{id}/finalize', function($params) use ($modx) { - $input = file_get_contents('php://input'); - $data = json_decode($input, true) ?: []; - $allParams = array_merge($params, $data); - - $controller = new \MiniShop3\Controllers\Api\Manager\OrdersController($modx); - return $controller->finalize($allParams); + $data = json_decode(file_get_contents('php://input'), true) ?: []; + return (new \MiniShop3\Controllers\Api\Manager\OrdersController($modx)) + ->finalize(array_merge($params, $data)); }); $router->post('/{id}/recalculate-cost', function($params) use ($modx) { - $input = file_get_contents('php://input'); - $data = json_decode($input, true) ?: []; - $allParams = array_merge($params, $data); - - $controller = new \MiniShop3\Controllers\Api\Manager\OrdersController($modx); - return $controller->recalculateCost($allParams); - }); - $router->get('/{id}', function($params) use ($modx) { - $controller = new \MiniShop3\Controllers\Api\Manager\OrdersController($modx); - return $controller->get($params); + $data = json_decode(file_get_contents('php://input'), true) ?: []; + return (new \MiniShop3\Controllers\Api\Manager\OrdersController($modx)) + ->recalculateCost(array_merge($params, $data)); }); $router->put('/{id}', function($params) use ($modx) { - $body = json_decode(file_get_contents('php://input'), true) ?: []; - $allParams = array_merge($params, $body, $_POST); - $controller = new \MiniShop3\Controllers\Api\Manager\OrdersController($modx); - return $controller->update($allParams); + $data = json_decode(file_get_contents('php://input'), true) ?: []; + return (new \MiniShop3\Controllers\Api\Manager\OrdersController($modx)) + ->update(array_merge($params, $data, $_POST)); }); $router->delete('/{id}', function($params) use ($modx) { - $controller = new \MiniShop3\Controllers\Api\Manager\OrdersController($modx); - return $controller->delete($params); - }); - $router->get('/{id}/products', function($params) use ($modx) { - $controller = new \MiniShop3\Controllers\Api\Manager\OrdersController($modx); - return $controller->getProducts($params); + return (new \MiniShop3\Controllers\Api\Manager\OrdersController($modx))->delete($params); }); $router->post('/{id}/products', function($params) use ($modx) { - $input = file_get_contents('php://input'); - $data = json_decode($input, true) ?: []; - $allParams = array_merge($params, $data); - - $controller = new \MiniShop3\Controllers\Api\Manager\OrdersController($modx); - return $controller->addProduct($allParams); + $data = json_decode(file_get_contents('php://input'), true) ?: []; + return (new \MiniShop3\Controllers\Api\Manager\OrdersController($modx)) + ->addProduct(array_merge($params, $data)); }); $router->put('/{id}/products/{product_id}', function($params) use ($modx) { - $input = file_get_contents('php://input'); - $data = json_decode($input, true) ?: []; - $allParams = array_merge($params, $data); - - $controller = new \MiniShop3\Controllers\Api\Manager\OrdersController($modx); - return $controller->updateProduct($allParams); + $data = json_decode(file_get_contents('php://input'), true) ?: []; + return (new \MiniShop3\Controllers\Api\Manager\OrdersController($modx)) + ->updateProduct(array_merge($params, $data)); }); $router->delete('/{id}/products/{product_id}', function($params) use ($modx) { - $controller = new \MiniShop3\Controllers\Api\Manager\OrdersController($modx); - return $controller->deleteProduct($params); + return (new \MiniShop3\Controllers\Api\Manager\OrdersController($modx))->deleteProduct($params); }); - $router->get('/{id}/logs', function($params) use ($modx) { - $controller = new \MiniShop3\Controllers\Api\Manager\OrdersController($modx); - return $controller->getLogs($params); - }); - }, [ - new PermissionMiddleware($modx, 'msorder_list') + new PermissionMiddleware($modx, 'msorder_save') ]); // Statuses dropdown (with translated names for order forms) diff --git a/core/components/minishop3/tests/OrdersRoutePermissionsTest.php b/core/components/minishop3/tests/OrdersRoutePermissionsTest.php new file mode 100644 index 00000000..9bdf7327 --- /dev/null +++ b/core/components/minishop3/tests/OrdersRoutePermissionsTest.php @@ -0,0 +1,100 @@ +getValue($middleware); + if ($found !== null && $found !== $permission) { + $fail("conflicting PermissionMiddleware values: {$found} vs {$permission}"); + } + $found = $permission; + } + + return $found; +}; + +$modx = new modX(); +$router = new Router($modx); +$router->loadRoutes(dirname(__DIR__) . '/config/routes/manager.php'); + +$routesProperty = new ReflectionProperty(Router::class, 'routes'); +$registered = $routesProperty->getValue($router); + +$expected = [ + 'GET /api/mgr/orders' => 'msorder_list', + 'GET /api/mgr/orders/filters' => 'msorder_list', + 'GET /api/mgr/orders/{id}' => 'msorder_list', + 'GET /api/mgr/orders/{id}/products' => 'msorder_list', + 'GET /api/mgr/orders/{id}/logs' => 'msorder_list', + 'POST /api/mgr/orders' => 'msorder_save', + 'DELETE /api/mgr/orders/bulk' => 'msorder_save', + 'POST /api/mgr/orders/{id}/finalize' => 'msorder_save', + 'POST /api/mgr/orders/{id}/recalculate-cost' => 'msorder_save', + 'PUT /api/mgr/orders/{id}' => 'msorder_save', + 'DELETE /api/mgr/orders/{id}' => 'msorder_save', + 'POST /api/mgr/orders/{id}/products' => 'msorder_save', + 'PUT /api/mgr/orders/{id}/products/{product_id}' => 'msorder_save', + 'DELETE /api/mgr/orders/{id}/products/{product_id}' => 'msorder_save', +]; + +$actual = []; +foreach ($registered as $route) { + $pattern = $route['pattern'] ?? ''; + if (!str_starts_with($pattern, '/api/mgr/orders')) { + continue; + } + $method = is_array($route['method'] ?? null) + ? implode('|', $route['method']) + : (string) ($route['method'] ?? ''); + $key = strtoupper($method) . ' ' . $pattern; + $actual[$key] = $permissionFromMiddlewares($route['middlewares'] ?? []); +} + +foreach ($expected as $routeKey => $permission) { + $assertSame($permission, $actual[$routeKey] ?? null, "route {$routeKey}"); +} + +$unknown = array_diff(array_keys($actual), array_keys($expected)); +if ($unknown !== []) { + $fail('unexpected orders routes: ' . implode(', ', $unknown)); +} + +fwrite(STDOUT, "OK OrdersRoutePermissionsTest\n"); +exit(0); diff --git a/core/components/minishop3/tests/stubs/ModxStub.php b/core/components/minishop3/tests/stubs/ModxStub.php new file mode 100644 index 00000000..c164a309 --- /dev/null +++ b/core/components/minishop3/tests/stubs/ModxStub.php @@ -0,0 +1,15 @@ + Date: Thu, 16 Jul 2026 10:24:10 +0600 Subject: [PATCH 2/2] test(manager-api): assert runtime 403 for order permission split Extend the #377 regression test to dispatch sample routes with a stub modX so list-only write and save-only read return 403 before handlers. --- .../tests/OrdersRoutePermissionsTest.php | 42 +++++++++++++++++- .../minishop3/tests/stubs/ModxStub.php | 44 +++++++++++++++++++ 2 files changed, 84 insertions(+), 2 deletions(-) diff --git a/core/components/minishop3/tests/OrdersRoutePermissionsTest.php b/core/components/minishop3/tests/OrdersRoutePermissionsTest.php index 9bdf7327..f30413da 100644 --- a/core/components/minishop3/tests/OrdersRoutePermissionsTest.php +++ b/core/components/minishop3/tests/OrdersRoutePermissionsTest.php @@ -4,6 +4,7 @@ * Router-level check: /api/mgr/orders reads use msorder_list, writes use msorder_save (#377). * * Uses a stub modX so manager routes can load without a full MODX install. + * Also dispatches sample routes to assert runtime 403 when the required permission is missing. * * Run: php tests/OrdersRoutePermissionsTest.php */ @@ -50,9 +51,36 @@ return $found; }; +$buildRouter = static function (modX $modx): Router { + $router = new Router($modx); + $router->loadRoutes(dirname(__DIR__) . '/config/routes/manager.php'); + $router->build(); + + return $router; +}; + +$assertDenied = static function ( + Router $router, + string $method, + string $uri, + string $requiredPermission, + string $label +) use ($assertSame, $fail): void { + $response = $router->dispatch($uri, $method); + $data = $response->getData(); + $assertSame(403, $response->getStatusCode(), "{$label}: status"); + $assertSame(false, $data['success'] ?? null, "{$label}: success"); + $assertSame(403, $data['code'] ?? null, "{$label}: body code"); + $message = (string) ($data['message'] ?? ''); + if (!str_contains($message, $requiredPermission)) { + $fail("{$label}: message should mention {$requiredPermission}, got: {$message}"); + } +}; + +$_SERVER['HTTP_MODAUTH'] = 'test-modauth-token'; + $modx = new modX(); -$router = new Router($modx); -$router->loadRoutes(dirname(__DIR__) . '/config/routes/manager.php'); +$router = $buildRouter($modx); $routesProperty = new ReflectionProperty(Router::class, 'routes'); $registered = $routesProperty->getValue($router); @@ -96,5 +124,15 @@ $fail('unexpected orders routes: ' . implode(', ', $unknown)); } +// Runtime: list-only cannot mutate; save-only cannot read (#377) +$modx->setPermissions(['msorder_list']); +$assertDenied($router, 'PUT', '/api/mgr/orders/1', 'msorder_save', 'list-only PUT order'); +$assertDenied($router, 'POST', '/api/mgr/orders/1/finalize', 'msorder_save', 'list-only finalize'); +$assertDenied($router, 'DELETE', '/api/mgr/orders/1/products/2', 'msorder_save', 'list-only delete product'); + +$modx->setPermissions(['msorder_save']); +$assertDenied($router, 'GET', '/api/mgr/orders', 'msorder_list', 'save-only GET list'); +$assertDenied($router, 'GET', '/api/mgr/orders/1', 'msorder_list', 'save-only GET order'); + fwrite(STDOUT, "OK OrdersRoutePermissionsTest\n"); exit(0); diff --git a/core/components/minishop3/tests/stubs/ModxStub.php b/core/components/minishop3/tests/stubs/ModxStub.php index c164a309..04b54029 100644 --- a/core/components/minishop3/tests/stubs/ModxStub.php +++ b/core/components/minishop3/tests/stubs/ModxStub.php @@ -9,6 +9,50 @@ */ class modX { + /** @var object|null */ + public $user; + + /** @var object|null */ + public $context; + + /** @var array */ + private array $permissions = []; + + public function __construct() + { + $this->context = new class { + public function get(string $key): string + { + return $key === 'key' ? 'mgr' : ''; + } + }; + + $this->user = new class { + public function isAuthenticated(string $context): bool + { + return $context === 'mgr'; + } + + public function getUserToken(string $contextKey): string + { + return 'test-modauth-token'; + } + }; + } + + /** + * @param list $permissions + */ + public function setPermissions(array $permissions): void + { + $this->permissions = array_fill_keys($permissions, true); + } + + public function hasPermission(string $permission): bool + { + return !empty($this->permissions[$permission]); + } + public function log($level, $message): void { }