From 4ae613a681bdd2eba69070d153417e62b037ce7d Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Fri, 17 Jul 2026 15:21:20 +0600 Subject: [PATCH] fix(manager): whitelist deliveries-active dropdown fields GET /api/mgr/deliveries-active previously returned full msDelivery::toArray(), leaking integration properties/class/validation_rules to any authenticated manager. Serve only id/name/price/active/position via DeliveriesController. --- .../minishop3/config/routes/manager.php | 18 +--- .../Api/Manager/DeliveriesController.php | 69 ++++++++++++++ .../DeliveriesActiveDropdownFieldsTest.php | 89 +++++++++++++++++++ 3 files changed, 161 insertions(+), 15 deletions(-) create mode 100644 core/components/minishop3/tests/DeliveriesActiveDropdownFieldsTest.php diff --git a/core/components/minishop3/config/routes/manager.php b/core/components/minishop3/config/routes/manager.php index 71aa8978..1b4444ad 100644 --- a/core/components/minishop3/config/routes/manager.php +++ b/core/components/minishop3/config/routes/manager.php @@ -940,22 +940,10 @@ return \MiniShop3\Router\Response::success(['results' => $results])->getData(); }); - // Dropdown list of active deliveries (for order forms) + // Dropdown list of active deliveries (for order forms; no properties/class secrets) $router->get('/deliveries-active', function($params) use ($modx) { - $modx->lexicon->load('minishop3:default'); - $results = []; - $collection = $modx->getIterator(\MiniShop3\Model\msDelivery::class, ['active' => 1]); - foreach ($collection as $item) { - $data = $item->toArray(); - if (!empty($data['name']) && str_starts_with($data['name'], 'ms3_')) { - $translated = $modx->lexicon($data['name']); - if ($translated !== $data['name']) { - $data['name'] = $translated; - } - } - $results[] = $data; - } - return \MiniShop3\Router\Response::success(['results' => $results])->getData(); + $controller = new \MiniShop3\Controllers\Api\Manager\DeliveriesController($modx); + return $controller->getActiveDropdown($params); }); $router->group('/grid-config', function($router) use ($modx) { diff --git a/core/components/minishop3/src/Controllers/Api/Manager/DeliveriesController.php b/core/components/minishop3/src/Controllers/Api/Manager/DeliveriesController.php index 583ae814..0f836601 100644 --- a/core/components/minishop3/src/Controllers/Api/Manager/DeliveriesController.php +++ b/core/components/minishop3/src/Controllers/Api/Manager/DeliveriesController.php @@ -18,6 +18,19 @@ */ class DeliveriesController { + /** + * Public fields for GET /api/mgr/deliveries-active (order-form dropdown). + * + * Must never include integration secrets: properties, class, validation_rules. + */ + public const ACTIVE_DROPDOWN_FIELDS = [ + 'id', + 'name', + 'price', + 'active', + 'position', + ]; + protected modX $modx; public function __construct(modX $modx) @@ -25,6 +38,36 @@ public function __construct(modX $modx) $this->modx = $modx; } + /** + * Active deliveries for order-form dropdowns (no integration secrets). + * GET /api/mgr/deliveries-active + */ + public function getActiveDropdown(array $params = []): array + { + $this->modx->lexicon->load('minishop3:default'); + + $q = $this->modx->newQuery(msDelivery::class, ['active' => 1]); + $q->sortby('position', 'ASC'); + + $results = []; + foreach ($this->modx->getIterator(msDelivery::class, $q) as $delivery) { + $results[] = $this->formatActiveDropdownItem($delivery); + } + + return Response::success(['results' => $results])->getData(); + } + + /** + * Project a delivery row to the dropdown whitelist (no MODX required). + * + * @param array $data + * @return array + */ + public static function projectActiveDropdownFields(array $data): array + { + return array_intersect_key($data, array_flip(self::ACTIVE_DROPDOWN_FIELDS)); + } + /** * Get list of deliveries with pagination and search * GET /api/mgr/deliveries @@ -376,6 +419,32 @@ protected function formatDelivery(msDelivery $delivery): array ]; } + /** + * Dropdown row: whitelist only + translated name. + */ + protected function formatActiveDropdownItem(msDelivery $delivery): array + { + $data = []; + foreach (self::ACTIVE_DROPDOWN_FIELDS as $field) { + $raw = $delivery->get($field); + $data[$field] = match ($field) { + 'id', 'position' => (int) $raw, + 'active' => (int) (bool) $raw, + default => $raw, + }; + } + + $name = (string) ($data['name'] ?? ''); + if ($name !== '' && str_starts_with($name, 'ms3_')) { + $translated = $this->modx->lexicon($name); + if ($translated !== $name) { + $data['name'] = $translated; + } + } + + return $data; + } + /** * Get payments for specific delivery * GET /api/mgr/deliveries/{id}/payments diff --git a/core/components/minishop3/tests/DeliveriesActiveDropdownFieldsTest.php b/core/components/minishop3/tests/DeliveriesActiveDropdownFieldsTest.php new file mode 100644 index 00000000..08ed6599 --- /dev/null +++ b/core/components/minishop3/tests/DeliveriesActiveDropdownFieldsTest.php @@ -0,0 +1,89 @@ + 7, + 'name' => 'Courier', + 'price' => '300', + 'active' => 1, + 'position' => 2, + 'properties' => ['api_key' => 'secret'], + 'class' => 'MiniShop3\\Controllers\\Delivery\\Delivery', + 'validation_rules' => '{"phone":"required"}', + 'description' => 'should not leak', +]; + +$projected = DeliveriesController::projectActiveDropdownFields($raw); + +foreach ($forbidden as $field) { + if (array_key_exists($field, $projected)) { + $fail("projected payload must not contain \"{$field}\""); + } +} + +if (array_key_exists('description', $projected)) { + $fail('projected payload must not contain non-whitelist field "description"'); +} + +foreach ($required as $field) { + if (!array_key_exists($field, $projected)) { + $fail("projected payload must keep \"{$field}\""); + } +} + +if ($projected['id'] !== 7 || $projected['name'] !== 'Courier' || $projected['position'] !== 2) { + $fail('projected field values must not be altered'); +} + +// Guard: production formatter must loop the same whitelist (source scan). +$controllerSource = file_get_contents(__DIR__ . '/../src/Controllers/Api/Manager/DeliveriesController.php'); +if ($controllerSource === false) { + $fail('cannot read DeliveriesController.php'); +} +if (!str_contains($controllerSource, 'foreach (self::ACTIVE_DROPDOWN_FIELDS as $field)')) { + $fail('formatActiveDropdownItem must iterate ACTIVE_DROPDOWN_FIELDS (not ad-hoc toArray)'); +} +if (preg_match('/function formatActiveDropdownItem.*?toArray\(/s', $controllerSource)) { + $fail('formatActiveDropdownItem must not call toArray()'); +} + +fwrite(STDOUT, "OK DeliveriesActiveDropdownFieldsTest\n"); +exit(0);