From 9095f264ddc610a00235b1f96ced92599dbc1feb Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Mon, 21 Sep 2026 23:32:07 +0600 Subject: [PATCH 1/5] fix(catalog): align Fenom ACL and harden customer tokens Member-aware resource-group visibility for storefront snippets matches Web API, revoked session tokens are not reused, query-string ms3_token no longer elevates ACL, reset denies inactive accounts, and unsafe snippet sortby parts are dropped. --- .../minishop3/config/routes/web.php | 2 +- .../elements/snippets/ms3_gallery.php | 2 +- .../elements/snippets/ms3_options.php | 2 +- .../elements/snippets/ms3_product_options.php | 2 +- .../elements/snippets/ms3_products.php | 51 ++++- .../minishop3/lexicon/en/setting.inc.php | 2 +- .../minishop3/lexicon/ru/setting.inc.php | 2 +- .../Api/Manager/CustomerGroupsController.php | 13 ++ .../Api/Manager/CustomersController.php | 9 + .../Processors/Api/Customer/ResetPassword.php | 2 +- .../Catalog/CatalogAclCacheInvalidator.php | 12 ++ .../CatalogResourceGroupVisibility.php | 70 ++++++- .../Catalog/CatalogSortbyQualifier.php | 196 ++++++++++++++--- .../minishop3/src/Services/TokenService.php | 42 ++-- .../CatalogResourceGroupVisibilityTest.php | 19 +- .../tests/CatalogSortbyQualifierTest.php | 78 ++++++- .../tests/CustomerAuthRoutesTest.php | 8 +- .../tests/CustomerSessionContractTest.php | 5 + .../tests/ResetPasswordAccessTest.php | 49 +++++ .../tests/TokenMiddlewareQueryTokenTest.php | 4 +- .../CustomerGroupsControllerAclCacheTest.php | 198 ++++++++++++++++++ .../Manager/CustomersControllerUpdateTest.php | 150 ++++++++++++- .../TokenServiceGenerateCustomerTokenTest.php | 92 ++++++++ 23 files changed, 926 insertions(+), 84 deletions(-) create mode 100644 core/components/minishop3/tests/ResetPasswordAccessTest.php create mode 100644 core/components/minishop3/tests/Unit/Controllers/Api/Manager/CustomerGroupsControllerAclCacheTest.php create mode 100644 core/components/minishop3/tests/Unit/Services/TokenServiceGenerateCustomerTokenTest.php diff --git a/core/components/minishop3/config/routes/web.php b/core/components/minishop3/config/routes/web.php index f26f88c69..73dc10d4f 100644 --- a/core/components/minishop3/config/routes/web.php +++ b/core/components/minishop3/config/routes/web.php @@ -184,7 +184,7 @@ } else { return Response::error($response['message'] ?? 'Token generation failed', $response['code'] ?? 500); } - }); + }, [$tokenMiddleware]); $router->post('/token/refresh', function () use ($customerAuth) { return $customerAuth()->refreshToken(); diff --git a/core/components/minishop3/elements/snippets/ms3_gallery.php b/core/components/minishop3/elements/snippets/ms3_gallery.php index f3c4e5fb0..d3ba3bf03 100644 --- a/core/components/minishop3/elements/snippets/ms3_gallery.php +++ b/core/components/minishop3/elements/snippets/ms3_gallery.php @@ -31,7 +31,7 @@ $modx->log(modX::LOG_LEVEL_ERROR, "[msGallery] Resource {$product->id} is not msProduct"); return ''; } - if ($_ms3LoadedById && !(new CatalogResourceGroupVisibility($modx))->isVisible((int) $product->id)) { + if ($_ms3LoadedById && !(new CatalogResourceGroupVisibility($modx))->isVisibleForRequest((int) $product->id)) { return ''; } diff --git a/core/components/minishop3/elements/snippets/ms3_options.php b/core/components/minishop3/elements/snippets/ms3_options.php index 85e821125..5d5fd2ce0 100644 --- a/core/components/minishop3/elements/snippets/ms3_options.php +++ b/core/components/minishop3/elements/snippets/ms3_options.php @@ -34,7 +34,7 @@ 'id' => $product->id ]); } -if ($_ms3LoadedById && !(new CatalogResourceGroupVisibility($modx))->isVisible((int) $product->id)) { +if ($_ms3LoadedById && !(new CatalogResourceGroupVisibility($modx))->isVisibleForRequest((int) $product->id)) { return ''; } diff --git a/core/components/minishop3/elements/snippets/ms3_product_options.php b/core/components/minishop3/elements/snippets/ms3_product_options.php index 13f57499a..793728b74 100644 --- a/core/components/minishop3/elements/snippets/ms3_product_options.php +++ b/core/components/minishop3/elements/snippets/ms3_product_options.php @@ -31,7 +31,7 @@ 'id' => $product->id ]); } -if ($_ms3LoadedById && !(new CatalogResourceGroupVisibility($modx))->isVisible((int) $product->id)) { +if ($_ms3LoadedById && !(new CatalogResourceGroupVisibility($modx))->isVisibleForRequest((int) $product->id)) { return ''; } diff --git a/core/components/minishop3/elements/snippets/ms3_products.php b/core/components/minishop3/elements/snippets/ms3_products.php index 618f1fcc0..5fd2d0ba3 100644 --- a/core/components/minishop3/elements/snippets/ms3_products.php +++ b/core/components/minishop3/elements/snippets/ms3_products.php @@ -174,10 +174,49 @@ $_ms3SortBy = $scriptProperties['sortby'] ?? ''; if (!is_array($_ms3SortBy)) { $_ms3SortBy = (string) $_ms3SortBy; - // Qualify bare resource columns before menuindex CASE injects commas (#741 / #742 review). + // Whitelist + qualify bare columns before menuindex / sortbyOptions inject expressions (#755). + $_ms3Passthrough = []; + if (!empty($scriptProperties['includeTVs'])) { + $tvList = is_array($scriptProperties['includeTVs']) + ? $scriptProperties['includeTVs'] + : array_map('trim', explode(',', (string) $scriptProperties['includeTVs'])); + foreach ($tvList as $tvName) { + if (is_string($tvName) && $tvName !== '') { + $_ms3Passthrough[] = $tvName; + } + } + } + if (!empty($scriptProperties['includeVendor']) || !empty($scriptProperties['includeVendorFields'])) { + $vendorFields = !empty($scriptProperties['includeVendorFields']) + ? (is_array($scriptProperties['includeVendorFields']) + ? $scriptProperties['includeVendorFields'] + : array_map('trim', explode(',', (string) $scriptProperties['includeVendorFields']))) + : ['name']; + foreach ($vendorFields as $vendorField) { + if (!is_string($vendorField) || $vendorField === '') { + continue; + } + $_ms3Passthrough[] = str_starts_with($vendorField, 'vendor_') + ? $vendorField + : 'vendor_' . $vendorField; + } + } + if (!empty($scriptProperties['sortbyOptions'])) { + foreach (array_map('trim', explode(',', (string) $scriptProperties['sortbyOptions'])) as $sortOpt) { + $optKey = explode(':', $sortOpt)[0] ?? ''; + if ($optKey !== '') { + $_ms3Passthrough[] = $optKey; + } + } + } $_ms3SortBy = CatalogSortbyQualifier::qualifyUnaliasedResourceFields( $_ms3SortBy, - array_keys($modx->getFields(msProduct::class) ?: []) + array_keys($modx->getFields(msProduct::class) ?: []), + 'msProduct', + true, + array_keys($modx->getFields(msProductData::class) ?: []), + 'Data', + $_ms3Passthrough, ); $scriptProperties['sortby'] = $_ms3SortBy; } @@ -270,18 +309,14 @@ } } -// Anonymous RG ACL for storefront listing (#670); same SQL as Web API. +// Member-aware RG ACL for storefront listing (#670 / #755); same resolver path as Web API. // Put the NOT EXISTS in an INNER JOIN ON — not in $where[] — so pdoTools // additionalConditions() does not false-positive-suppress &resources / &context // (raw numeric where strings that mention msProduct + \bid\b / context_key). // Self-join on site_content duplicates resource columns: bare multi-column sortby // is qualified via CatalogSortbyQualifier before pdoTools (#741 / #742 review). $_ms3RgVisibility = new CatalogResourceGroupVisibility($modx); -$_ms3RgContext = trim((string) ($modx->context->key ?? '')); -if ($_ms3RgContext === '') { - $_ms3RgContext = 'web'; -} -$_ms3RgWhere = $_ms3RgVisibility->buildWhereFragment('msProduct', $_ms3RgContext); +$_ms3RgWhere = $_ms3RgVisibility->buildWhereFragmentForRequest('msProduct'); if ($_ms3RgWhere !== null) { $innerJoin['ms3RgVisibility'] = [ 'class' => msProduct::class, diff --git a/core/components/minishop3/lexicon/en/setting.inc.php b/core/components/minishop3/lexicon/en/setting.inc.php index e36d0db9e..b9af390fa 100644 --- a/core/components/minishop3/lexicon/en/setting.inc.php +++ b/core/components/minishop3/lexicon/en/setting.inc.php @@ -268,7 +268,7 @@ $_lang['setting_ms3_api_debug'] = 'API debug mode'; $_lang['setting_ms3_api_debug_desc'] = 'Enables extended logging of API requests and responses for debugging. Not recommended in production.'; $_lang['setting_ms3_web_catalog_respect_resource_groups'] = 'Respect resource group ACL in public catalog'; -$_lang['setting_ms3_web_catalog_respect_resource_groups_desc'] = 'When enabled, the public Web API catalog and Fenom storefront snippets (e.g. ms3_products) hide products and categories that belong to a MODX resource group with Resource Group Access ACL for the request context (anonymous MVP). MiniShop3 invalidates MODX resource (page) cache and facet cache when resource-group ACL or membership changes via the manager plugin. Residual gaps: ACL rows written outside MODX processors (SQL, custom scripts) and HTML cached by an external CDN may stay stale until you clear cache manually. Disable to restore pre-#659 catalog behavior.'; +$_lang['setting_ms3_web_catalog_respect_resource_groups_desc'] = 'When enabled, the public Web API catalog and Fenom storefront snippets (e.g. ms3_products) hide products and categories that belong to a MODX resource group with Resource Group Access ACL for the request context. Logged-in customers with a customer group linked to a MODX user group see the same member catalog as Web API. For those member requests MiniShop3 sets the current resource cacheable=0 so member HTML is not stored under the shared page-cache key; prefer uncached calls [[!ms3_products]] / [[!ms3_gallery]] when the page itself must stay cacheable for anonymous visitors. MiniShop3 also invalidates MODX resource (page) cache and facet cache when resource-group ACL, membership, or customer group mapping changes. Residual gaps: ACL rows written outside MODX processors (SQL, custom scripts) and HTML cached by an external CDN may stay stale until you clear cache manually. Disable to restore pre-#659 catalog behavior.'; $_lang['setting_ms3_cors_allowed_origins'] = 'Allowed CORS origins'; $_lang['setting_ms3_cors_allowed_origins_desc'] = 'Comma-separated origins allowed to call the Web API (e.g. https://shop.example.com). Empty = no cross-origin CORS (same-origin only). Use "*" for any origin without credentials; for headless with cookies list explicit domains.'; $_lang['setting_ms3_rate_limit_max_attempts'] = 'API rate limit'; diff --git a/core/components/minishop3/lexicon/ru/setting.inc.php b/core/components/minishop3/lexicon/ru/setting.inc.php index dd8ef0c2d..d075bc3a9 100644 --- a/core/components/minishop3/lexicon/ru/setting.inc.php +++ b/core/components/minishop3/lexicon/ru/setting.inc.php @@ -268,7 +268,7 @@ $_lang['setting_ms3_api_debug'] = 'Режим отладки API'; $_lang['setting_ms3_api_debug_desc'] = 'Включает расширенное логирование API запросов и ответов для отладки. Не рекомендуется на продакшене.'; $_lang['setting_ms3_web_catalog_respect_resource_groups'] = 'Учитывать ACL групп ресурсов в публичном каталоге'; -$_lang['setting_ms3_web_catalog_respect_resource_groups_desc'] = 'Если включено, публичный Web API каталог и Fenom-сниппеты витрины (например ms3_products) скрывают товары и категории, входящие в группу ресурсов MODX с ACL «Доступ к группе ресурсов» для контекста запроса (анонимный MVP). MiniShop3 сбрасывает кэш страниц MODX (resource) и кэш фасетов при изменении ACL или членства в группах ресурсов через плагин менеджера. Остаточные пробелы: ACL, записанный в обход процессоров MODX (SQL, свои скрипты), и HTML за внешним CDN могут оставаться устаревшими до ручной очистки кэша. Отключите, чтобы вернуть поведение каталога до #659.'; +$_lang['setting_ms3_web_catalog_respect_resource_groups_desc'] = 'Если включено, публичный Web API каталог и Fenom-сниппеты витрины (например ms3_products) скрывают товары и категории, входящие в группу ресурсов MODX с ACL «Доступ к группе ресурсов» для контекста запроса. Залогиненный покупатель с группой клиентов, привязанной к группе пользователей MODX, видит тот же member-каталог, что и Web API. Для таких запросов MiniShop3 ставит текущему ресурсу cacheable=0, чтобы HTML с расширенным каталогом не попал в общий ключ кэша страницы; для кэшируемых страниц с анонимной витриной предпочтительны вызовы [[!ms3_products]] / [[!ms3_gallery]]. Также сбрасывается кэш страниц MODX (resource) и кэш фасетов при изменении ACL, членства в группах ресурсов или привязки группы клиентов. Остаточные пробелы: ACL, записанный в обход процессоров MODX (SQL, свои скрипты), и HTML за внешним CDN могут оставаться устаревшими до ручной очистки кэша. Отключите, чтобы вернуть поведение каталога до #659.'; $_lang['setting_ms3_cors_allowed_origins'] = 'Разрешённые CORS origins'; $_lang['setting_ms3_cors_allowed_origins_desc'] = 'Origins через запятую для Web API (например https://shop.example.com). Пусто = CORS только same-origin. «*» — любой origin без credentials; для headless с cookies укажите домены явно.'; $_lang['setting_ms3_rate_limit_max_attempts'] = 'Лимит запросов API'; diff --git a/core/components/minishop3/src/Controllers/Api/Manager/CustomerGroupsController.php b/core/components/minishop3/src/Controllers/Api/Manager/CustomerGroupsController.php index 90a50e670..c94234012 100644 --- a/core/components/minishop3/src/Controllers/Api/Manager/CustomerGroupsController.php +++ b/core/components/minishop3/src/Controllers/Api/Manager/CustomerGroupsController.php @@ -6,6 +6,7 @@ use MiniShop3\Model\msCustomer; use MiniShop3\Model\msCustomerGroup; +use MiniShop3\Services\Catalog\CatalogAclCacheInvalidator; use MiniShop3\Router\HttpStatus; use MiniShop3\Router\Response; use MODX\Revolution\modUserGroup; @@ -170,6 +171,9 @@ public function update(array $data = []): array )->getData(); } + $previousUserGroupId = (int) $group->get('user_group_id'); + $previousActive = (bool) $group->get('active'); + if (array_key_exists('name', $data)) { $name = trim((string) $data['name']); if ($name === '') { @@ -203,6 +207,13 @@ public function update(array $data = []): array )->getData(); } + $aclFieldsChanged = (array_key_exists('user_group_id', $data) + && (int) $group->get('user_group_id') !== $previousUserGroupId) + || (array_key_exists('active', $data) && (bool) $group->get('active') !== $previousActive); + if ($aclFieldsChanged) { + CatalogAclCacheInvalidator::scheduleForModx($this->modx); + } + return Response::success( $this->formatGroup($group), $this->lexicon('ms3_customer_group_updated'), @@ -247,6 +258,8 @@ public function delete(array $params = []): array )->getData(); } + CatalogAclCacheInvalidator::scheduleForModx($this->modx); + return Response::success([], $this->lexicon('ms3_customer_group_deleted'))->getData(); } diff --git a/core/components/minishop3/src/Controllers/Api/Manager/CustomersController.php b/core/components/minishop3/src/Controllers/Api/Manager/CustomersController.php index 835a8be8f..aa4b2c13d 100644 --- a/core/components/minishop3/src/Controllers/Api/Manager/CustomersController.php +++ b/core/components/minishop3/src/Controllers/Api/Manager/CustomersController.php @@ -7,6 +7,7 @@ use MiniShop3\Model\msCustomerGroup; use MiniShop3\Router\HttpStatus; use MiniShop3\Router\Response; +use MiniShop3\Services\Catalog\CatalogAclCacheInvalidator; use MiniShop3\Services\Customer\AuthManager; use MiniShop3\Services\Customer\CustomerAccess; use MiniShop3\Services\Grid\ManagerListFilterPolicy; @@ -175,6 +176,7 @@ public function update(array $data = []): array } $wasBlocked = (bool) $customer->get('is_blocked'); + $previousCustomerGroupId = $customer->get('customer_group_id'); $allowedFields = ['first_name', 'last_name', 'email', 'phone', 'is_active', 'is_blocked', 'customer_group_id']; @@ -223,6 +225,13 @@ public function update(array $data = []): array $authManager->revokeTokens($customer); } + if ( + array_key_exists('customer_group_id', $data) + && (int) ($customer->get('customer_group_id') ?? 0) !== (int) ($previousCustomerGroupId ?? 0) + ) { + CatalogAclCacheInvalidator::scheduleForModx($this->modx); + } + return Response::success($this->formatCustomer($customer), 'Customer updated successfully')->getData(); } diff --git a/core/components/minishop3/src/Processors/Api/Customer/ResetPassword.php b/core/components/minishop3/src/Processors/Api/Customer/ResetPassword.php index c00eeef33..24da3c3ff 100644 --- a/core/components/minishop3/src/Processors/Api/Customer/ResetPassword.php +++ b/core/components/minishop3/src/Processors/Api/Customer/ResetPassword.php @@ -68,7 +68,7 @@ public function process() /** @var msCustomer $customer */ $customer = $authManager->validateToken($token, msCustomerToken::TYPE_PASSWORD_RESET); - if (!$customer) { + if (!$customer || CustomerAccess::isPasswordResetDenied($customer)) { return $this->failure($this->modx->lexicon('ms3_customer_err_token_invalid')); } diff --git a/core/components/minishop3/src/Services/Catalog/CatalogAclCacheInvalidator.php b/core/components/minishop3/src/Services/Catalog/CatalogAclCacheInvalidator.php index ed6e35326..0e73b53c1 100644 --- a/core/components/minishop3/src/Services/Catalog/CatalogAclCacheInvalidator.php +++ b/core/components/minishop3/src/Services/Catalog/CatalogAclCacheInvalidator.php @@ -29,6 +29,18 @@ public function __construct( ) { } + public static function scheduleForModx(modX $modx): void + { + if (!$modx->services->has('ms3_catalog_acl_cache')) { + return; + } + + $invalidator = $modx->services->get('ms3_catalog_acl_cache'); + if ($invalidator instanceof self) { + $invalidator->schedule(); + } + } + /** * Defer {@see invalidate()} until after the current processor finishes. */ diff --git a/core/components/minishop3/src/Services/Catalog/CatalogResourceGroupVisibility.php b/core/components/minishop3/src/Services/Catalog/CatalogResourceGroupVisibility.php index a942113e0..3981a70dd 100644 --- a/core/components/minishop3/src/Services/Catalog/CatalogResourceGroupVisibility.php +++ b/core/components/minishop3/src/Services/Catalog/CatalogResourceGroupVisibility.php @@ -104,11 +104,30 @@ public function apply( } } + /** + * Visibility SQL for the current HTTP visitor (Fenom snippets, same path as Web API #755). + * Member-aware results also disable MODX page cache for this request so member HTML + * is not stored under the anonymous resource cache key. + * + * @return non-empty-string|null + */ + public function buildWhereFragmentForRequest( + string $resourceAlias, + ?string $contextKey = null, + ): ?string { + $contextKey = $this->requestContextKey($contextKey); + $allowedIds = $this->customerResourceGroupResolver()->resolveAllowedIdsForRequest($contextKey); + $this->disablePageCacheForMemberCatalog($allowedIds); + + return $this->buildWhereFragment($resourceAlias, $contextKey, $allowedIds); + } + /** * Visibility SQL for pdoTools INNER JOIN ON / xPDO where. * - * Fenom listings pass no allowed ids (anonymous-safe). Member-aware callers - * pass ids from {@see CustomerResourceGroupResolver} or use {@see applyForRequest()}. + * Empty $allowedResourceGroupIds → anonymous gate. Member-aware callers pass ids from + * {@see CustomerResourceGroupResolver} or use {@see buildWhereFragmentForRequest()} / + * {@see applyForRequest()}. * * @param list $allowedResourceGroupIds * @@ -142,6 +161,39 @@ public function buildWhereFragment( ); } + /** + * Single-resource visibility for the current HTTP visitor (#755 Fenom &product=). + * Member-aware lookups disable MODX page cache for this request (#755 cache key). + */ + public function isVisibleForRequest( + int $resourceId, + string $resourceAlias = 'msProduct', + ?string $contextKey = null, + ): bool { + $contextKey = $this->requestContextKey($contextKey); + $allowedIds = $this->customerResourceGroupResolver()->resolveAllowedIdsForRequest($contextKey); + $this->disablePageCacheForMemberCatalog($allowedIds); + + return $this->isVisible($resourceId, $resourceAlias, $contextKey, $allowedIds); + } + + /** + * MODX page cache keys are per-resource, not per visitor. Member-expanded HTML must + * not be written into that shared key (#755). + * + * @param list $allowedResourceGroupIds + */ + private function disablePageCacheForMemberCatalog(array $allowedResourceGroupIds): void + { + if (self::positiveIntIds($allowedResourceGroupIds) === []) { + return; + } + $resource = $this->modx->resource ?? null; + if (is_object($resource) && method_exists($resource, 'set')) { + $resource->set('cacheable', 0); + } + } + /** * Single-resource visibility (Fenom &product= and similar). * @@ -157,10 +209,7 @@ public function isVisible( return false; } - $contextKey ??= (string) ($this->modx->context->key ?? ''); - if ($contextKey === '') { - $contextKey = 'web'; - } + $contextKey = $this->requestContextKey($contextKey); $fragment = $this->buildWhereFragment($resourceAlias, $contextKey, $allowedResourceGroupIds); if ($fragment === null) { @@ -195,6 +244,15 @@ private function customerResourceGroupResolver(): CustomerResourceGroupResolver return new CustomerResourceGroupResolver($this->modx); } + private function requestContextKey(?string $contextKey = null): string + { + if ($contextKey === null) { + $contextKey = trim((string) ($this->modx->context->key ?? '')); + } + + return $contextKey === '' ? 'web' : $contextKey; + } + /** * @param list $allowedResourceGroupIds * @param string $quotedPrincipalClass Already connection-quoted principal_class diff --git a/core/components/minishop3/src/Services/Catalog/CatalogSortbyQualifier.php b/core/components/minishop3/src/Services/Catalog/CatalogSortbyQualifier.php index df36a105f..9714db6c5 100644 --- a/core/components/minishop3/src/Services/Catalog/CatalogSortbyQualifier.php +++ b/core/components/minishop3/src/Services/Catalog/CatalogSortbyQualifier.php @@ -5,73 +5,221 @@ namespace MiniShop3\Services\Catalog; /** - * Qualify bare modResource columns in multi-field pdoTools sortby strings (#741 / #742). + * Qualify bare columns in multi-field pdoTools sortby strings (#741 / #742 / #755). * * pdoTools qualifies a single field and JSON keys itself. A comma-separated string like * `pagetitle DESC, publishedon` stays bare. With the RG self-join on site_content that - * makes ORDER BY ambiguous. Simple `field [ASC|DESC]` pieces get an `msProduct.` prefix - * when the field exists on the resource; expressions with `(` (CASE, functions) are left alone. + * makes ORDER BY ambiguous. + * + * Snippet mode ($dropUnmatched): drop unknown *simple* parts before menuindex / sortbyOptions + * inject expressions. Known resource fields → msProduct.*; known Data fields → Data.*; + * optional passthrough names (TVs, vendor_*) stay bare. Parenthetical / CASE input from the + * caller is rejected in drop mode (expressions are injected by the snippet afterwards). + * Prefixed parts are kept only for allowed table aliases (not arbitrary table.field). */ final class CatalogSortbyQualifier { + private const SIMPLE_SORT_PART = '/^(?:`?(?P[A-Za-z_][\w]*)`?\.)?`?(?P[A-Za-z_][\w]*)`?(?P\s+(?:ASC|DESC))?$/i'; + /** * @param list $resourceFieldNames Field names from modResource / msProduct + * @param list $dataFieldNames Field names from msProductData (qualified as Data.*) + * @param list $passthroughNames Bare names left as-is (TVs, vendor_*, option keys) + * @param list $allowedTableAliases When dropUnmatched, only these table prefixes pass */ public static function qualifyUnaliasedResourceFields( string $sortby, array $resourceFieldNames, string $alias = 'msProduct', + bool $dropUnmatched = false, + array $dataFieldNames = [], + string $dataAlias = 'Data', + array $passthroughNames = [], + array $allowedTableAliases = ['msProduct', 'Data', 'Vendor'], ): string { $trimmed = ltrim($sortby); - if ($trimmed === '' || str_starts_with($trimmed, '{') || str_contains($sortby, '(')) { + if ($trimmed === '' || str_starts_with($trimmed, '{')) { return $sortby; } - $fields = []; - foreach ($resourceFieldNames as $name) { - $name = strtolower(trim((string) $name)); - if ($name !== '') { - $fields[$name] = true; - } + if (!$dropUnmatched && str_contains($sortby, '(')) { + return $sortby; } - if ($fields === []) { + + $resourceFields = self::indexNames($resourceFieldNames); + $dataFields = self::indexNames($dataFieldNames); + $passthrough = self::indexNames($passthroughNames); + $allowedTables = self::indexNames($allowedTableAliases); + if ($resourceFields === [] && $dataFields === [] && $passthrough === []) { return $sortby; } - $parts = array_map(static fn (string $part): string => trim($part), explode(',', $sortby)); + $parts = $dropUnmatched + ? self::splitSortParts($sortby) + : array_map(static fn (string $part): string => trim($part), explode(',', $sortby)); $qualified = []; foreach ($parts as $part) { if ($part === '') { continue; } - $qualified[] = self::qualifySimplePart($part, $fields, $alias); + if ($dropUnmatched && !self::isAllowedSortPart( + $part, + $resourceFields, + $dataFields, + $passthrough, + $allowedTables, + )) { + continue; + } + $qualified[] = self::qualifySimplePart( + $part, + $resourceFields, + $dataFields, + $passthrough, + $alias, + $dataAlias, + ); + } + + if ($qualified === []) { + return $dropUnmatched ? $alias . '.id' : ''; } return implode(', ', $qualified); } /** - * @param array $fields Lowercase field set + * @param list $names + * @return array */ - private static function qualifySimplePart(string $part, array $fields, string $alias): string + private static function indexNames(array $names): array { - if (!preg_match( - '/^(?:`?(?P
[A-Za-z_][\w]*)`?\.)?`?(?P[A-Za-z_][\w]*)`?(?P\s+(?:ASC|DESC))?$/i', - $part, - $match - )) { - return $part; + $indexed = []; + foreach ($names as $name) { + $name = strtolower(trim((string) $name)); + if ($name !== '') { + $indexed[$name] = true; + } + } + + return $indexed; + } + + /** + * @return list + */ + private static function splitSortParts(string $sortby): array + { + $parts = []; + $current = ''; + $depth = 0; + $length = strlen($sortby); + + for ($i = 0; $i < $length; $i++) { + $char = $sortby[$i]; + if ($char === '(') { + ++$depth; + $current .= $char; + } elseif ($char === ')') { + --$depth; + $current .= $char; + } elseif ($char === ',' && $depth === 0) { + $trimmed = trim($current); + if ($trimmed !== '') { + $parts[] = $trimmed; + } + $current = ''; + } else { + $current .= $char; + } + } + + $trimmed = trim($current); + if ($trimmed !== '') { + $parts[] = $trimmed; + } + + return $parts; + } + + /** + * @param array $resourceFields + * @param array $dataFields + * @param array $passthrough + * @param array $allowedTables + */ + private static function isAllowedSortPart( + string $part, + array $resourceFields, + array $dataFields, + array $passthrough, + array $allowedTables, + ): bool { + // Caller-supplied expressions are rejected in drop mode; snippet injects CASE/CAST later. + if (str_contains($part, '(') || preg_match('/^CASE\s+/i', trim($part))) { + return false; + } + + $match = self::matchSimpleSortPart($part); + if ($match === null) { + return false; } if ($match['table'] !== '') { + return isset($allowedTables[strtolower($match['table'])]); + } + + $field = strtolower($match['field']); + + return isset($resourceFields[$field]) + || isset($dataFields[$field]) + || isset($passthrough[$field]); + } + + /** + * @param array $resourceFields + * @param array $dataFields + * @param array $passthrough + */ + private static function qualifySimplePart( + string $part, + array $resourceFields, + array $dataFields, + array $passthrough, + string $alias, + string $dataAlias, + ): string { + $match = self::matchSimpleSortPart($part); + if ($match === null || $match['table'] !== '') { return $part; } $field = $match['field']; - if (!isset($fields[strtolower($field)])) { - return $part; + $lower = strtolower($field); + $dir = $match['dir'] ?? ''; + + if (isset($resourceFields[$lower])) { + return $alias . '.' . $field . $dir; + } + if (isset($dataFields[$lower])) { + return $dataAlias . '.' . $field . $dir; + } + if (isset($passthrough[$lower])) { + return $field . $dir; + } + + return $part; + } + + /** + * @return array{table: string, field: string, dir?: string}|null + */ + private static function matchSimpleSortPart(string $part): ?array + { + if (!preg_match(self::SIMPLE_SORT_PART, $part, $match)) { + return null; } - return $alias . '.' . $field . ($match['dir'] ?? ''); + return $match; } } diff --git a/core/components/minishop3/src/Services/TokenService.php b/core/components/minishop3/src/Services/TokenService.php index 273b74e91..3043096fc 100644 --- a/core/components/minishop3/src/Services/TokenService.php +++ b/core/components/minishop3/src/Services/TokenService.php @@ -50,16 +50,22 @@ public function generateCustomerToken(?int $ttl = null): array { $existingToken = $this->ensureCustomerTokenLoaded(); if ($existingToken !== null) { - $expires = (int)($_SESSION['ms3']['customer_token_expires'] ?? (time() + 86400)); - $lifetime = max(0, $expires - time()); + $resolved = $this->resolveApiToken($existingToken); + if ($resolved['reason'] === 'ok') { + $tokenObj = $resolved['token']; + $expires = (int) strtotime((string) $tokenObj->get('expires_at')); + $lifetime = max(0, $expires - time()); - CookieHelper::setTokenCookie($this->modx, $existingToken); + CookieHelper::setTokenCookie($this->modx, $existingToken); - return [ - 'token' => $existingToken, - 'expires' => $expires, - 'lifetime' => $lifetime * 1000, - ]; + return [ + 'token' => $existingToken, + 'expires' => $expires, + 'lifetime' => $lifetime * 1000, + ]; + } + + $this->discardStaleCustomerSession(); } $customerId = (int)($_SESSION['ms3']['customer_id'] ?? 0); @@ -200,9 +206,7 @@ public function resolveOrCreateToken(): string return $sessionToken; } - $this->clearCustomerToken(); - CookieHelper::clearTokenCookie($this->modx); - unset($_SESSION['ms3']['customer_id']); + $this->discardStaleCustomerSession(); } // 2. Check cookie @@ -452,6 +456,16 @@ public function getSnippetData(string $token): ?array return $data ?: null; } + /** + * Drop revoked session token, cookie, and bound customer id before minting anew. + */ + private function discardStaleCustomerSession(): void + { + $this->clearCustomerToken(); + CookieHelper::clearTokenCookie($this->modx); + unset($_SESSION['ms3']['customer_id']); + } + /** * Clear customer token from session * @@ -518,10 +532,10 @@ public function ensureSessionActive(): void * 1. Authorization: Bearer * 2. HTTP_MS3TOKEN (legacy) * 3. httpOnly cookie `ms3_token` - * 4. $_REQUEST['ms3_token'] after middleware cookie inject (#576: query stripped) + * 4. $_REQUEST['ms3_token'] when not from query string (cookie inject / POST body) * 5. PHP session cache * - * Query-string `token` / `ms3_token` are not accepted here. + * Query-string `ms3_token` is never accepted (#576, #755 public catalog ACL). */ public static function resolveTokenFromRequest(): string { @@ -536,7 +550,7 @@ public static function resolveTokenFromRequest(): string } $fromRequest = $_REQUEST['ms3_token'] ?? ''; - if ($fromRequest !== '') { + if ($fromRequest !== '' && !array_key_exists('ms3_token', $_GET)) { return (string) $fromRequest; } diff --git a/core/components/minishop3/tests/CatalogResourceGroupVisibilityTest.php b/core/components/minishop3/tests/CatalogResourceGroupVisibilityTest.php index f4c420813..eb91654ef 100644 --- a/core/components/minishop3/tests/CatalogResourceGroupVisibilityTest.php +++ b/core/components/minishop3/tests/CatalogResourceGroupVisibilityTest.php @@ -238,10 +238,18 @@ public function quote($string) $ms3ProductsSrc = (string) file_get_contents(__DIR__ . '/../elements/snippets/ms3_products.php'); $assertTrue( str_contains($ms3ProductsSrc, "\$innerJoin['ms3RgVisibility']") + && str_contains($ms3ProductsSrc, 'buildWhereFragmentForRequest') && str_contains($ms3ProductsSrc, 'CatalogSortbyQualifier::qualifyUnaliasedResourceFields') && !str_contains($ms3ProductsSrc, "unset(\$leftJoin['Data'])"), - 'ms3_products wires RG via ms3RgVisibility + sortby qualify (not Data INNER)' + 'ms3_products wires RG via ms3RgVisibility + request-aware ACL + sortby qualify' ); +foreach (['ms3_gallery.php', 'ms3_options.php', 'ms3_product_options.php'] as $snippetFile) { + $snippetSrc = (string) file_get_contents(__DIR__ . '/../elements/snippets/' . $snippetFile); + $assertTrue( + str_contains($snippetSrc, 'isVisibleForRequest'), + $snippetFile . ' uses isVisibleForRequest for member ACL' + ); +} $assertTrue( !preg_match('/\$where\[\]\s*=\s*\$_ms3RgWhere/', $ms3ProductsSrc), 'ms3_products does not append RG fragment to numeric where' @@ -253,9 +261,14 @@ public function quote($string) $assertTrue($visibleWhenDisabled->isVisible(1), 'isVisible true when setting disabled'); $assertTrue(!$visibleWhenDisabled->isVisible(0), 'isVisible false for non-positive id'); -$isVisibleSrc = (string) file_get_contents(__DIR__ . '/../src/Services/Catalog/CatalogResourceGroupVisibility.php'); +$visibilitySrc = (string) file_get_contents(__DIR__ . '/../src/Services/Catalog/CatalogResourceGroupVisibility.php'); +$assertTrue( + str_contains($visibilitySrc, 'buildWhereFragmentForRequest') + && str_contains($visibilitySrc, 'isVisibleForRequest'), + 'CatalogResourceGroupVisibility exposes request-aware helpers' +); $assertTrue( - str_contains($isVisibleSrc, "'class_key' => \$class"), + str_contains($visibilitySrc, "'class_key' => \$class"), 'isVisible must pin class_key because getCount skips derivative criteria' ); diff --git a/core/components/minishop3/tests/CatalogSortbyQualifierTest.php b/core/components/minishop3/tests/CatalogSortbyQualifierTest.php index 5299e7be6..62a7ce071 100644 --- a/core/components/minishop3/tests/CatalogSortbyQualifierTest.php +++ b/core/components/minishop3/tests/CatalogSortbyQualifierTest.php @@ -1,7 +1,7 @@ true, 'handlerMethod' => 'refreshToken', ], + 'GET /api/v1/customer/token/get' => [ + 'tokenMiddleware' => true, + 'handlerMethod' => null, + ], ]; $actual = []; @@ -176,7 +180,9 @@ $fail("route {$routeKey} must have callable handler"); } - $handlerUsesAuthController($handler, $expectation['handlerMethod']); + if ($expectation['handlerMethod'] !== null) { + $handlerUsesAuthController($handler, $expectation['handlerMethod']); + } } echo "OK CustomerAuthRoutesTest\n"; diff --git a/core/components/minishop3/tests/CustomerSessionContractTest.php b/core/components/minishop3/tests/CustomerSessionContractTest.php index 30c047bfe..9cbbb59bf 100644 --- a/core/components/minishop3/tests/CustomerSessionContractTest.php +++ b/core/components/minishop3/tests/CustomerSessionContractTest.php @@ -59,6 +59,11 @@ unset($_SESSION['ms3']['customer_token']); $assertSame('', TokenService::resolveTokenFromRequest(), 'empty when nothing set'); +$_REQUEST['ms3_token'] = 'query-elevated-token'; +$_GET['ms3_token'] = 'query-elevated-token'; +$assertSame('', TokenService::resolveTokenFromRequest(), 'query-string ms3_token must not elevate ACL'); +unset($_GET['ms3_token'], $_REQUEST['ms3_token']); + // --- CustomerPublicDto must not expose secrets (me allowlist) --- $leaky = [ 'id' => 1, diff --git a/core/components/minishop3/tests/ResetPasswordAccessTest.php b/core/components/minishop3/tests/ResetPasswordAccessTest.php new file mode 100644 index 000000000..5293f7721 --- /dev/null +++ b/core/components/minishop3/tests/ResetPasswordAccessTest.php @@ -0,0 +1,49 @@ + 2, + 'name' => 'VIP', + 'user_group_id' => 5, + 'active' => true, + ]); + [$modx, $invalidator] = $this->modx($group, userGroupExists: true); + + $data = (new CustomerGroupsController($modx))->update([ + 'id' => 2, + 'user_group_id' => 8, + ]); + + self::assertTrue($data['success'] ?? false); + self::assertSame(8, $group->get('user_group_id')); + self::assertTrue($invalidator->wasScheduledForTests()); + } + + public function testUpdateDoesNotScheduleAclCacheWhenOnlyNameChanges(): void + { + $group = new FakeUpdateCustomerGroup([ + 'id' => 2, + 'name' => 'VIP', + 'user_group_id' => 5, + 'active' => true, + ]); + [$modx, $invalidator] = $this->modx($group, userGroupExists: true); + + $data = (new CustomerGroupsController($modx))->update([ + 'id' => 2, + 'name' => 'VIP Plus', + ]); + + self::assertTrue($data['success'] ?? false); + self::assertSame('VIP Plus', $group->get('name')); + self::assertFalse($invalidator->wasScheduledForTests()); + } + + public function testDeleteSchedulesAclCache(): void + { + $group = new FakeUpdateCustomerGroup([ + 'id' => 2, + 'name' => 'VIP', + 'user_group_id' => 5, + 'active' => true, + ]); + [$modx, $invalidator] = $this->modx($group, userGroupExists: true); + + $data = (new CustomerGroupsController($modx))->delete(['id' => 2]); + + self::assertTrue($data['success'] ?? false); + self::assertTrue($group->wasRemoved); + self::assertTrue($invalidator->wasScheduledForTests()); + } + + /** @return array{0: modX, 1: CatalogAclCacheInvalidator} */ + private function modx(FakeUpdateCustomerGroup $group, bool $userGroupExists): array + { + $invalidator = new CatalogAclCacheInvalidator(new CatalogAclInvalidatorModxStub( + new class { + public function refresh(array $providers = [], array &$results = []): bool + { + return true; + } + }, + new class { + public function has(string $key): bool + { + return false; + } + }, + [ + 'access_resource_group_enabled' => true, + CatalogResourceGroupVisibility::SETTING_KEY => true, + ], + ['web'], + )); + + $modx = new class ($group, $invalidator, $userGroupExists) extends modX { + public function __construct( + private FakeUpdateCustomerGroup $group, + private CatalogAclCacheInvalidator $invalidator, + private bool $userGroupExists, + ) { + parent::__construct(); + $this->lexicon = new class { + public function load(string $topic): void + { + } + + public function __invoke(string $key): string + { + return $key; + } + }; + $this->services = new class ($this->invalidator) { + public function __construct(private CatalogAclCacheInvalidator $invalidator) + { + } + + public function has(string $key): bool + { + return $key === 'ms3_catalog_acl_cache'; + } + + public function get(string $key): mixed + { + return $key === 'ms3_catalog_acl_cache' ? $this->invalidator : null; + } + }; + } + + public function lexicon(string $key, array $params = []): string + { + return $key; + } + + public function getObject($className, $criteria = null, $cacheFlag = true) + { + if ($className === msCustomerGroup::class) { + $id = is_array($criteria) ? (int) ($criteria['id'] ?? 0) : (int) $criteria; + + return $id === (int) $this->group->get('id') ? $this->group : null; + } + + return null; + } + + public function getCount($className, $criteria = null) + { + return $className === modUserGroup::class && $this->userGroupExists ? 1 : 0; + } + + public function updateCollection($className, array $set, $criteria = null) + { + return 0; + } + }; + + return [$modx, $invalidator]; + } +} + +final class FakeUpdateCustomerGroup extends msCustomerGroup +{ + public bool $wasRemoved = false; + + /** @param array $fields */ + public function __construct(private array $fields) + { + } + + public function get($key) + { + return $this->fields[$key] ?? null; + } + + public function set($key, $value, $vType = '') + { + $this->fields[$key] = $value; + + return $this; + } + + public function save($cacheFlag = null) + { + return true; + } + + public function remove(array $ancestors = []) + { + $this->wasRemoved = true; + + return true; + } +} diff --git a/core/components/minishop3/tests/Unit/Controllers/Api/Manager/CustomersControllerUpdateTest.php b/core/components/minishop3/tests/Unit/Controllers/Api/Manager/CustomersControllerUpdateTest.php index 3e0bf4c1e..ae433f059 100644 --- a/core/components/minishop3/tests/Unit/Controllers/Api/Manager/CustomersControllerUpdateTest.php +++ b/core/components/minishop3/tests/Unit/Controllers/Api/Manager/CustomersControllerUpdateTest.php @@ -6,10 +6,16 @@ use MiniShop3\Controllers\Api\Manager\CustomersController; use MiniShop3\Model\msCustomer; +use MiniShop3\Model\msCustomerGroup; +use MiniShop3\Services\Catalog\CatalogAclCacheInvalidator; +use MiniShop3\Services\Catalog\CatalogResourceGroupVisibility; use MiniShop3\Services\Customer\AuthManager; +use MiniShop3\Tests\Stubs\CatalogAclInvalidatorModxStub; use MODX\Revolution\modX; use PHPUnit\Framework\TestCase; +require_once dirname(__DIR__, 4) . '/stubs/CatalogAclModxStub.php'; + final class CustomersControllerUpdateTest extends TestCase { protected function setUp(): void @@ -143,6 +149,51 @@ public function testUpdateUnblockClearsUntilWithoutRevoke(): void self::assertSame(0, $customer->get('failed_login_attempts')); } + public function testUpdateSchedulesAclCacheWhenCustomerGroupChanges(): void + { + $customer = new FakeUpdateCustomer([ + 'id' => 11, + 'first_name' => 'Gus', + 'is_active' => 1, + 'is_blocked' => 0, + 'customer_group_id' => 1, + ]); + $authManager = $this->createMock(AuthManager::class); + $authManager->expects(self::never())->method('revokeTokens'); + [$modx, $invalidator] = $this->modxWithAclInvalidator($customer, $authManager, aclEnabled: true); + + $data = (new CustomersController($modx))->update([ + 'id' => 11, + 'customer_group_id' => 2, + ]); + + self::assertTrue($data['success'] ?? false); + self::assertSame(2, $customer->get('customer_group_id')); + self::assertTrue($invalidator->wasScheduledForTests()); + } + + public function testUpdateDoesNotScheduleAclCacheWhenGroupUnchanged(): void + { + $customer = new FakeUpdateCustomer([ + 'id' => 12, + 'first_name' => 'Hal', + 'is_active' => 1, + 'is_blocked' => 0, + 'customer_group_id' => 3, + ]); + $authManager = $this->createMock(AuthManager::class); + $authManager->expects(self::never())->method('revokeTokens'); + [$modx, $invalidator] = $this->modxWithAclInvalidator($customer, $authManager, aclEnabled: true); + + $data = (new CustomersController($modx))->update([ + 'id' => 12, + 'first_name' => 'Harry', + ]); + + self::assertTrue($data['success'] ?? false); + self::assertFalse($invalidator->wasScheduledForTests()); + } + public function testUpdateDoesNotRevokeWhenOnlyProfileFieldsChange(): void { $customer = new FakeUpdateCustomer([ @@ -163,41 +214,118 @@ public function testUpdateDoesNotRevokeWhenOnlyProfileFieldsChange(): void self::assertSame('Daniel', $customer->get('first_name')); } - private function modx(FakeUpdateCustomer $customer, AuthManager $authManager): modX - { - return new class ($customer, $authManager) extends modX { + /** @return array{0: modX, 1: CatalogAclCacheInvalidator} */ + private function modxWithAclInvalidator( + FakeUpdateCustomer $customer, + AuthManager $authManager, + bool $aclEnabled, + ): array { + $invalidator = new CatalogAclCacheInvalidator(new CatalogAclInvalidatorModxStub( + new class { + public function refresh(array $providers = [], array &$results = []): bool + { + return true; + } + }, + new class { + public function has(string $key): bool + { + return false; + } + }, + $aclEnabled + ? [ + 'access_resource_group_enabled' => true, + CatalogResourceGroupVisibility::SETTING_KEY => true, + ] + : [CatalogResourceGroupVisibility::SETTING_KEY => false], + ['web'], + )); + + $modx = new class ($customer, $authManager, $invalidator) extends modX { public function __construct( private FakeUpdateCustomer $customer, private AuthManager $authManager, + private CatalogAclCacheInvalidator $invalidator, ) { parent::__construct(); - $this->services = new class ($this->authManager) { - public function __construct(private AuthManager $authManager) + $this->lexicon = new class { + public function load(string $topic): void + { + } + + public function __invoke(string $key): string { + return $key; + } + }; + $this->services = new class ($this->authManager, $this->invalidator) { + public function __construct( + private AuthManager $authManager, + private CatalogAclCacheInvalidator $invalidator, + ) { } public function has(string $key): bool { - return $key === 'ms3_auth_manager'; + return in_array($key, ['ms3_auth_manager', 'ms3_catalog_acl_cache'], true); } public function get(string $key): mixed { - return $key === 'ms3_auth_manager' ? $this->authManager : null; + return match ($key) { + 'ms3_auth_manager' => $this->authManager, + 'ms3_catalog_acl_cache' => $this->invalidator, + default => null, + }; } }; } + public function lexicon(string $key, array $params = []): string + { + return $key; + } + public function getObject($className, $criteria = null, $cacheFlag = true) { - if ($className !== msCustomer::class) { - return null; + if ($className === msCustomer::class) { + $id = is_array($criteria) ? (int) ($criteria['id'] ?? 0) : (int) $criteria; + + return $id === (int) $this->customer->get('id') ? $this->customer : null; + } + if ($className === msCustomerGroup::class) { + $id = is_array($criteria) ? (int) ($criteria['id'] ?? 0) : (int) $criteria; + if ($id <= 0) { + return null; + } + + return new class ($id) extends msCustomerGroup { + public function __construct(private int $id) + { + } + + public function get($key) + { + return match ($key) { + 'id' => $this->id, + 'active' => 1, + default => null, + }; + } + }; } - $id = is_array($criteria) ? (int) ($criteria['id'] ?? 0) : (int) $criteria; - return $id === (int) $this->customer->get('id') ? $this->customer : null; + return null; } }; + + return [$modx, $invalidator]; + } + + private function modx(FakeUpdateCustomer $customer, AuthManager $authManager): modX + { + return $this->modxWithAclInvalidator($customer, $authManager, aclEnabled: false)[0]; } } diff --git a/core/components/minishop3/tests/Unit/Services/TokenServiceGenerateCustomerTokenTest.php b/core/components/minishop3/tests/Unit/Services/TokenServiceGenerateCustomerTokenTest.php new file mode 100644 index 000000000..8c295c8c2 --- /dev/null +++ b/core/components/minishop3/tests/Unit/Services/TokenServiceGenerateCustomerTokenTest.php @@ -0,0 +1,92 @@ + $staleToken, + 'customer_token_expires' => time() + 3600, + 'customer_id' => 7, + ]; + + $modx = new class extends modX { + public int $persistCalls = 0; + + public function getObject($className, $criteria = null, $cacheFlag = true) + { + return null; + } + + public function newObject($className = '', $attributes = []) + { + return new class extends msCustomerToken { + /** @var array */ + private array $data; + + public function __construct() + { + $this->data = [ + 'token' => bin2hex(random_bytes(32)), + 'expires_at' => date('Y-m-d H:i:s', time() + 604800), + 'customer_id' => 0, + 'type' => msCustomerToken::TYPE_API, + ]; + } + + public function set($key, $value, $vType = '') + { + $this->data[$key] = $value; + + return $this; + } + + public function get($key) + { + return $this->data[$key] ?? null; + } + + public function save($cacheFlag = null) + { + return true; + } + }; + } + + public function getOption(string $key, $options = null, $default = null) + { + return $key === 'ms3_customer_token_ttl' ? 604800 : $default; + } + + public function log($level, $msg, $target = '', $def = '', $file = '', $line = '', $fields = []): void + { + } + }; + + $service = new TokenService($modx); + $result = $service->generateCustomerToken(); + + self::assertNotSame('', $result['token']); + self::assertNotSame($staleToken, $result['token']); + self::assertSame($result['token'], $_SESSION['ms3']['customer_token'] ?? null); + self::assertSame(0, $_SESSION['ms3']['customer_id'] ?? -1); + } +} From 3348fe72cdbb73e6a22d17733a30a67d42360f30 Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Mon, 21 Sep 2026 23:40:25 +0600 Subject: [PATCH 2/5] fix(catalog): satisfy PHPStan on member page-cache bypass Replace ?? on $modx->resource with isset so nullCoalesce.initializedProperty does not fire on the typed modx property. --- .../Services/Catalog/CatalogResourceGroupVisibility.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/core/components/minishop3/src/Services/Catalog/CatalogResourceGroupVisibility.php b/core/components/minishop3/src/Services/Catalog/CatalogResourceGroupVisibility.php index 3981a70dd..ccbf02bef 100644 --- a/core/components/minishop3/src/Services/Catalog/CatalogResourceGroupVisibility.php +++ b/core/components/minishop3/src/Services/Catalog/CatalogResourceGroupVisibility.php @@ -188,10 +188,11 @@ private function disablePageCacheForMemberCatalog(array $allowedResourceGroupIds if (self::positiveIntIds($allowedResourceGroupIds) === []) { return; } - $resource = $this->modx->resource ?? null; - if (is_object($resource) && method_exists($resource, 'set')) { - $resource->set('cacheable', 0); + // API / CLI may have no current resource; isset avoids nullCoalesce on the typed $modx property. + if (!isset($this->modx->resource) || !is_object($this->modx->resource)) { + return; } + $this->modx->resource->set('cacheable', 0); } /** From 6f55c46b7d9cb4ece3f2da2691b2f2881f9b58de Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Mon, 21 Sep 2026 23:47:04 +0600 Subject: [PATCH 3/5] fix(catalog): avoid isset on typed modx for page-cache bypass PHPStan flags isset()/?? on CatalogResourceGroupVisibility::$modx. Read resource into a local and guard with is_object instead. --- .../Services/Catalog/CatalogResourceGroupVisibility.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/core/components/minishop3/src/Services/Catalog/CatalogResourceGroupVisibility.php b/core/components/minishop3/src/Services/Catalog/CatalogResourceGroupVisibility.php index ccbf02bef..80cb8f320 100644 --- a/core/components/minishop3/src/Services/Catalog/CatalogResourceGroupVisibility.php +++ b/core/components/minishop3/src/Services/Catalog/CatalogResourceGroupVisibility.php @@ -188,11 +188,12 @@ private function disablePageCacheForMemberCatalog(array $allowedResourceGroupIds if (self::positiveIntIds($allowedResourceGroupIds) === []) { return; } - // API / CLI may have no current resource; isset avoids nullCoalesce on the typed $modx property. - if (!isset($this->modx->resource) || !is_object($this->modx->resource)) { + // Fenom has a current resource; Web API / CLI often do not ($resource is null at runtime). + $resource = $this->modx->resource; + if (!is_object($resource)) { return; } - $this->modx->resource->set('cacheable', 0); + $resource->set('cacheable', 0); } /** From 7eb748c752ee0f105047ec8c273a9397a3a0405b Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Tue, 22 Sep 2026 15:00:09 +0600 Subject: [PATCH 4/5] fix(catalog): keep safe sortby functions and clarify member cache Allow RAND/FIELD/IFNULL/COALESCE/CAST in snippet drop mode, warn on dropped parts, accept leftJoin aliases, and state uncached snippets as required for member ACL on cacheable pages. --- .../elements/snippets/ms3_products.php | 13 + .../minishop3/lexicon/en/setting.inc.php | 2 +- .../minishop3/lexicon/ru/setting.inc.php | 2 +- .../Catalog/CatalogSortbyQualifier.php | 267 +++++++++++++++++- .../CatalogResourceGroupVisibilityTest.php | 37 ++- .../tests/CatalogSortbyQualifierTest.php | 89 ++++++ 6 files changed, 395 insertions(+), 15 deletions(-) diff --git a/core/components/minishop3/elements/snippets/ms3_products.php b/core/components/minishop3/elements/snippets/ms3_products.php index 5fd2d0ba3..9a5913e73 100644 --- a/core/components/minishop3/elements/snippets/ms3_products.php +++ b/core/components/minishop3/elements/snippets/ms3_products.php @@ -209,6 +209,7 @@ } } } + $_ms3DroppedSortParts = []; $_ms3SortBy = CatalogSortbyQualifier::qualifyUnaliasedResourceFields( $_ms3SortBy, array_keys($modx->getFields(msProduct::class) ?: []), @@ -217,7 +218,19 @@ array_keys($modx->getFields(msProductData::class) ?: []), 'Data', $_ms3Passthrough, + array_values(array_unique(array_merge( + ['msProduct', 'Data', 'Vendor'], + CatalogSortbyQualifier::tableAliasesFromJoins($leftJoin, $innerJoin), + ))), + $_ms3DroppedSortParts, ); + if ($_ms3DroppedSortParts !== []) { + $modx->log( + \MODX\Revolution\modX::LOG_LEVEL_WARN, + '[MiniShop3] ms3_products dropped unsafe/unknown sortby part(s): ' + . implode(' | ', $_ms3DroppedSortParts) + ); + } $scriptProperties['sortby'] = $_ms3SortBy; } if ($_ms3MenuindexCategoryIds !== [] && CategoryProductMenuindexService::sortbyRefersToMenuindex($_ms3SortBy)) { diff --git a/core/components/minishop3/lexicon/en/setting.inc.php b/core/components/minishop3/lexicon/en/setting.inc.php index b9af390fa..1c75d57b2 100644 --- a/core/components/minishop3/lexicon/en/setting.inc.php +++ b/core/components/minishop3/lexicon/en/setting.inc.php @@ -268,7 +268,7 @@ $_lang['setting_ms3_api_debug'] = 'API debug mode'; $_lang['setting_ms3_api_debug_desc'] = 'Enables extended logging of API requests and responses for debugging. Not recommended in production.'; $_lang['setting_ms3_web_catalog_respect_resource_groups'] = 'Respect resource group ACL in public catalog'; -$_lang['setting_ms3_web_catalog_respect_resource_groups_desc'] = 'When enabled, the public Web API catalog and Fenom storefront snippets (e.g. ms3_products) hide products and categories that belong to a MODX resource group with Resource Group Access ACL for the request context. Logged-in customers with a customer group linked to a MODX user group see the same member catalog as Web API. For those member requests MiniShop3 sets the current resource cacheable=0 so member HTML is not stored under the shared page-cache key; prefer uncached calls [[!ms3_products]] / [[!ms3_gallery]] when the page itself must stay cacheable for anonymous visitors. MiniShop3 also invalidates MODX resource (page) cache and facet cache when resource-group ACL, membership, or customer group mapping changes. Residual gaps: ACL rows written outside MODX processors (SQL, custom scripts) and HTML cached by an external CDN may stay stale until you clear cache manually. Disable to restore pre-#659 catalog behavior.'; +$_lang['setting_ms3_web_catalog_respect_resource_groups_desc'] = 'When enabled, the public Web API catalog and Fenom storefront snippets (e.g. ms3_products) hide products and categories that belong to a MODX resource group with Resource Group Access ACL for the request context. Logged-in customers with a customer group linked to a MODX user group see the same member catalog as Web API. For those member requests MiniShop3 sets the current resource cacheable=0 so member HTML is not stored under the shared page-cache key. On a cacheable MODX page the cached HTML is served before snippets run: after the first anonymous hit a logged-in customer still gets the anonymous list. Therefore member catalog on cacheable pages requires uncached calls [[!ms3_products]] / [[!ms3_gallery]] (without “!” the storefront member ACL never applies). MiniShop3 also invalidates MODX resource (page) cache and facet cache when resource-group ACL, membership, or customer group mapping changes. Residual gaps: ACL rows written outside MODX processors (SQL, custom scripts) and HTML cached by an external CDN may stay stale until you clear cache manually. Disable to restore pre-#659 catalog behavior.'; $_lang['setting_ms3_cors_allowed_origins'] = 'Allowed CORS origins'; $_lang['setting_ms3_cors_allowed_origins_desc'] = 'Comma-separated origins allowed to call the Web API (e.g. https://shop.example.com). Empty = no cross-origin CORS (same-origin only). Use "*" for any origin without credentials; for headless with cookies list explicit domains.'; $_lang['setting_ms3_rate_limit_max_attempts'] = 'API rate limit'; diff --git a/core/components/minishop3/lexicon/ru/setting.inc.php b/core/components/minishop3/lexicon/ru/setting.inc.php index d075bc3a9..10280e62b 100644 --- a/core/components/minishop3/lexicon/ru/setting.inc.php +++ b/core/components/minishop3/lexicon/ru/setting.inc.php @@ -268,7 +268,7 @@ $_lang['setting_ms3_api_debug'] = 'Режим отладки API'; $_lang['setting_ms3_api_debug_desc'] = 'Включает расширенное логирование API запросов и ответов для отладки. Не рекомендуется на продакшене.'; $_lang['setting_ms3_web_catalog_respect_resource_groups'] = 'Учитывать ACL групп ресурсов в публичном каталоге'; -$_lang['setting_ms3_web_catalog_respect_resource_groups_desc'] = 'Если включено, публичный Web API каталог и Fenom-сниппеты витрины (например ms3_products) скрывают товары и категории, входящие в группу ресурсов MODX с ACL «Доступ к группе ресурсов» для контекста запроса. Залогиненный покупатель с группой клиентов, привязанной к группе пользователей MODX, видит тот же member-каталог, что и Web API. Для таких запросов MiniShop3 ставит текущему ресурсу cacheable=0, чтобы HTML с расширенным каталогом не попал в общий ключ кэша страницы; для кэшируемых страниц с анонимной витриной предпочтительны вызовы [[!ms3_products]] / [[!ms3_gallery]]. Также сбрасывается кэш страниц MODX (resource) и кэш фасетов при изменении ACL, членства в группах ресурсов или привязки группы клиентов. Остаточные пробелы: ACL, записанный в обход процессоров MODX (SQL, свои скрипты), и HTML за внешним CDN могут оставаться устаревшими до ручной очистки кэша. Отключите, чтобы вернуть поведение каталога до #659.'; +$_lang['setting_ms3_web_catalog_respect_resource_groups_desc'] = 'Если включено, публичный Web API каталог и Fenom-сниппеты витрины (например ms3_products) скрывают товары и категории, входящие в группу ресурсов MODX с ACL «Доступ к группе ресурсов» для контекста запроса. Залогиненный покупатель с группой клиентов, привязанной к группе пользователей MODX, видит тот же member-каталог, что и Web API. Для таких запросов MiniShop3 ставит текущему ресурсу cacheable=0, чтобы HTML с расширенным каталогом не попал в общий ключ кэша страницы. На кэшируемой странице MODX отдаёт HTML до сниппетов: после первого анонимного визита покупатель снова получит анонимный список. Поэтому для member-каталога на кэшируемых страницах обязательны некэшируемые вызовы [[!ms3_products]] / [[!ms3_gallery]] (без «!» member ACL на витрине не работает). Также сбрасывается кэш страниц MODX (resource) и кэш фасетов при изменении ACL, членства в группах ресурсов или привязки группы клиентов. Остаточные пробелы: ACL, записанный в обход процессоров MODX (SQL, свои скрипты), и HTML за внешним CDN могут оставаться устаревшими до ручной очистки кэша. Отключите, чтобы вернуть поведение каталога до #659.'; $_lang['setting_ms3_cors_allowed_origins'] = 'Разрешённые CORS origins'; $_lang['setting_ms3_cors_allowed_origins_desc'] = 'Origins через запятую для Web API (например https://shop.example.com). Пусто = CORS только same-origin. «*» — любой origin без credentials; для headless с cookies укажите домены явно.'; $_lang['setting_ms3_rate_limit_max_attempts'] = 'Лимит запросов API'; diff --git a/core/components/minishop3/src/Services/Catalog/CatalogSortbyQualifier.php b/core/components/minishop3/src/Services/Catalog/CatalogSortbyQualifier.php index 9714db6c5..731525b51 100644 --- a/core/components/minishop3/src/Services/Catalog/CatalogSortbyQualifier.php +++ b/core/components/minishop3/src/Services/Catalog/CatalogSortbyQualifier.php @@ -13,19 +13,22 @@ * * Snippet mode ($dropUnmatched): drop unknown *simple* parts before menuindex / sortbyOptions * inject expressions. Known resource fields → msProduct.*; known Data fields → Data.*; - * optional passthrough names (TVs, vendor_*) stay bare. Parenthetical / CASE input from the - * caller is rejected in drop mode (expressions are injected by the snippet afterwards). - * Prefixed parts are kept only for allowed table aliases (not arbitrary table.field). + * optional passthrough names (TVs, vendor_*) stay bare. Safe SQL functions (RAND, FIELD, + * IFNULL, COALESCE, CAST) with validated arguments are kept (#757 review). Prefixed parts + * are kept only for allowed table aliases (including keys from snippet leftJoin/innerJoin). */ final class CatalogSortbyQualifier { private const SIMPLE_SORT_PART = '/^(?:`?(?P
[A-Za-z_][\w]*)`?\.)?`?(?P[A-Za-z_][\w]*)`?(?P\s+(?:ASC|DESC))?$/i'; + private const SAFE_FUNCTION = '/^(?PRAND|FIELD|IFNULL|COALESCE|CAST)\s*\((?P.*)\)(?P\s+(?:ASC|DESC))?$/is'; + /** * @param list $resourceFieldNames Field names from modResource / msProduct * @param list $dataFieldNames Field names from msProductData (qualified as Data.*) * @param list $passthroughNames Bare names left as-is (TVs, vendor_*, option keys) * @param list $allowedTableAliases When dropUnmatched, only these table prefixes pass + * @param list|null $droppedParts Filled with rejected sort parts when dropUnmatched */ public static function qualifyUnaliasedResourceFields( string $sortby, @@ -36,7 +39,9 @@ public static function qualifyUnaliasedResourceFields( string $dataAlias = 'Data', array $passthroughNames = [], array $allowedTableAliases = ['msProduct', 'Data', 'Vendor'], + ?array &$droppedParts = null, ): string { + $droppedParts = []; $trimmed = ltrim($sortby); if ($trimmed === '' || str_starts_with($trimmed, '{')) { return $sortby; @@ -62,14 +67,30 @@ public static function qualifyUnaliasedResourceFields( if ($part === '') { continue; } - if ($dropUnmatched && !self::isAllowedSortPart( - $part, - $resourceFields, - $dataFields, - $passthrough, - $allowedTables, - )) { - continue; + if ($dropUnmatched) { + $safeFn = self::qualifySafeFunctionPart( + $part, + $resourceFields, + $dataFields, + $passthrough, + $allowedTables, + $alias, + $dataAlias, + ); + if ($safeFn !== null) { + $qualified[] = $safeFn; + continue; + } + if (!self::isAllowedSortPart( + $part, + $resourceFields, + $dataFields, + $passthrough, + $allowedTables, + )) { + $droppedParts[] = $part; + continue; + } } $qualified[] = self::qualifySimplePart( $part, @@ -88,6 +109,27 @@ public static function qualifyUnaliasedResourceFields( return implode(', ', $qualified); } + /** + * Collect pdoTools join map keys as allowed ORDER BY table aliases (#757 review). + * + * @param array ...$joinMaps + * @return list + */ + public static function tableAliasesFromJoins(array ...$joinMaps): array + { + $aliases = []; + foreach ($joinMaps as $map) { + foreach (array_keys($map) as $key) { + $key = trim((string) $key); + if ($key !== '' && preg_match('/^[A-Za-z_][\w]*$/', $key)) { + $aliases[] = $key; + } + } + } + + return array_values(array_unique($aliases)); + } + /** * @param list $names * @return array @@ -155,7 +197,7 @@ private static function isAllowedSortPart( array $passthrough, array $allowedTables, ): bool { - // Caller-supplied expressions are rejected in drop mode; snippet injects CASE/CAST later. + // Caller-supplied CASE / arbitrary expressions stay rejected; snippet injects those later. if (str_contains($part, '(') || preg_match('/^CASE\s+/i', trim($part))) { return false; } @@ -176,6 +218,207 @@ private static function isAllowedSortPart( || isset($passthrough[$field]); } + /** + * Allowlisted SQL functions with validated / qualified arguments (#757 review). + * + * @param array $resourceFields + * @param array $dataFields + * @param array $passthrough + * @param array $allowedTables + */ + private static function qualifySafeFunctionPart( + string $part, + array $resourceFields, + array $dataFields, + array $passthrough, + array $allowedTables, + string $alias, + string $dataAlias, + ): ?string { + if (!preg_match(self::SAFE_FUNCTION, trim($part), $match)) { + return null; + } + + $fn = strtoupper($match['fn']); + $args = trim($match['args']); + $dir = $match['dir'] ?? ''; + + if ($fn === 'RAND') { + return $args === '' ? 'RAND()' . $dir : null; + } + + if ($args === '' || !self::isSafeFunctionArgs($args, $fn === 'CAST')) { + return null; + } + + if (!self::functionArgsUseOnlyAllowedIdentifiers( + $args, + $resourceFields, + $dataFields, + $passthrough, + $allowedTables, + $fn === 'CAST', + )) { + return null; + } + + $qualifiedArgs = self::qualifyIdentifiersInExpression( + $args, + $resourceFields, + $dataFields, + $passthrough, + $alias, + $dataAlias, + $fn === 'CAST', + ); + + return $fn . '(' . $qualifiedArgs . ')' . $dir; + } + + private static function isSafeFunctionArgs(string $args, bool $allowCastAs): bool + { + // No nested calls, comments, or statement separators. + if (str_contains($args, '(') || str_contains($args, ')') + || str_contains($args, ';') || str_contains($args, '--') + || str_contains($args, '/*') || str_contains($args, '*/')) { + return false; + } + + if (preg_match('/\b(SELECT|UNION|INSERT|UPDATE|DELETE|DROP|ALTER|INTO|SLEEP|BENCHMARK)\b/i', $args)) { + return false; + } + + // Strip string literals, then only identifiers / numbers / punctuation may remain. + $stripped = preg_replace( + '/\'(?:\\\\.|[^\'\\\\])*\'|"(?:\\\\.|[^"\\\\])*"/', + ' ', + $args, + ) ?? $args; + + if ($allowCastAs) { + $stripped = (string) preg_replace('/\bAS\b/i', ' ', $stripped); + } + + return (bool) preg_match( + '/^[\s,.`0-9A-Za-z_]+$/', + $stripped, + ); + } + + /** + * @param array $resourceFields + * @param array $dataFields + * @param array $passthrough + * @param array $allowedTables + */ + private static function functionArgsUseOnlyAllowedIdentifiers( + string $args, + array $resourceFields, + array $dataFields, + array $passthrough, + array $allowedTables, + bool $allowCastAs, + ): bool { + $withoutStrings = preg_replace( + '/\'(?:\\\\.|[^\'\\\\])*\'|"(?:\\\\.|[^"\\\\])*"/', + ' ', + $args, + ) ?? $args; + + if ($allowCastAs) { + $withoutStrings = (string) preg_replace('/\bAS\b/i', ' ', $withoutStrings); + } + + if (!preg_match_all('/`?([A-Za-z_][\w]*)`?(?:\.`?([A-Za-z_][\w]*)`?)?/', $withoutStrings, $matches, PREG_SET_ORDER)) { + return true; + } + + foreach ($matches as $match) { + $first = $match[1]; + $second = $match[2] ?? ''; + if ($second !== '') { + if (!isset($allowedTables[strtolower($first)])) { + return false; + } + continue; + } + $lower = strtolower($first); + // CAST type names (CHAR, SIGNED, …) and ASC/DESC never appear as lone first tokens + // after AS strip for CAST; still allow common SQL type tokens. + if ($allowCastAs && self::isSqlTypeToken($lower)) { + continue; + } + if (!isset($resourceFields[$lower]) && !isset($dataFields[$lower]) && !isset($passthrough[$lower])) { + return false; + } + } + + return true; + } + + private static function isSqlTypeToken(string $lower): bool + { + return in_array($lower, [ + 'char', 'varchar', 'binary', 'date', 'datetime', 'time', 'signed', 'unsigned', + 'decimal', 'integer', 'int', 'bigint', 'float', 'double', 'real', 'json', + ], true); + } + + /** + * @param array $resourceFields + * @param array $dataFields + * @param array $passthrough + */ + private static function qualifyIdentifiersInExpression( + string $args, + array $resourceFields, + array $dataFields, + array $passthrough, + string $alias, + string $dataAlias, + bool $allowCastAs, + ): string { + return (string) preg_replace_callback( + '/(\'(?:\\\\.|[^\'\\\\])*\'|"(?:\\\\.|[^"\\\\])*")|(`?[A-Za-z_][\w]*`?(?:\.`?[A-Za-z_][\w]*`?)?)/', + static function (array $m) use ( + $resourceFields, + $dataFields, + $passthrough, + $alias, + $dataAlias, + $allowCastAs, + ): string { + if (($m[1] ?? '') !== '') { + return $m[1]; + } + $token = $m[2]; + if (str_contains($token, '.')) { + return $token; + } + if ($allowCastAs && preg_match('/^AS$/i', $token)) { + return $token; + } + $bare = trim($token, '`'); + $lower = strtolower($bare); + if ($allowCastAs && self::isSqlTypeToken($lower)) { + return $token; + } + if (isset($resourceFields[$lower])) { + return $alias . '.' . $bare; + } + if (isset($dataFields[$lower])) { + return $dataAlias . '.' . $bare; + } + if (isset($passthrough[$lower])) { + return $bare; + } + + return $token; + }, + $args, + ); + } + /** * @param array $resourceFields * @param array $dataFields diff --git a/core/components/minishop3/tests/CatalogResourceGroupVisibilityTest.php b/core/components/minishop3/tests/CatalogResourceGroupVisibilityTest.php index eb91654ef..befd395ce 100644 --- a/core/components/minishop3/tests/CatalogResourceGroupVisibilityTest.php +++ b/core/components/minishop3/tests/CatalogResourceGroupVisibilityTest.php @@ -240,8 +240,10 @@ public function quote($string) str_contains($ms3ProductsSrc, "\$innerJoin['ms3RgVisibility']") && str_contains($ms3ProductsSrc, 'buildWhereFragmentForRequest') && str_contains($ms3ProductsSrc, 'CatalogSortbyQualifier::qualifyUnaliasedResourceFields') + && str_contains($ms3ProductsSrc, 'tableAliasesFromJoins') + && str_contains($ms3ProductsSrc, 'dropped unsafe/unknown sortby') && !str_contains($ms3ProductsSrc, "unset(\$leftJoin['Data'])"), - 'ms3_products wires RG via ms3RgVisibility + request-aware ACL + sortby qualify' + 'ms3_products wires RG via ms3RgVisibility + request-aware ACL + sortby qualify/log' ); foreach (['ms3_gallery.php', 'ms3_options.php', 'ms3_product_options.php'] as $snippetFile) { $snippetSrc = (string) file_get_contents(__DIR__ . '/../elements/snippets/' . $snippetFile); @@ -272,5 +274,38 @@ public function quote($string) 'isVisible must pin class_key because getCount skips derivative criteria' ); +// #757 review: disablePageCacheForMemberCatalog must set cacheable=0 for member RG sets. +$cacheResource = new class { + private int $cacheable = 1; + + public function set(string $key, mixed $value): void + { + if ($key === 'cacheable') { + $this->cacheable = (int) $value; + } + } + + public function get(string $key): mixed + { + return $key === 'cacheable' ? $this->cacheable : null; + } +}; +$cacheModx = new class ($cacheResource) extends modX { + public function __construct(public object $resource) + { + } + + public function getOption(string $key, $options = null, $default = null) + { + return $default; + } +}; +$cacheService = new CatalogResourceGroupVisibility($cacheModx); +$disablePageCache = new ReflectionMethod(CatalogResourceGroupVisibility::class, 'disablePageCacheForMemberCatalog'); +$disablePageCache->invoke($cacheService, []); +$assertSame(1, $cacheResource->get('cacheable'), 'empty member RG set leaves cacheable alone'); +$disablePageCache->invoke($cacheService, [12]); +$assertSame(0, $cacheResource->get('cacheable'), 'member RG set forces cacheable=0'); + fwrite(STDOUT, "OK CatalogResourceGroupVisibilityTest\n"); exit(0); diff --git a/core/components/minishop3/tests/CatalogSortbyQualifierTest.php b/core/components/minishop3/tests/CatalogSortbyQualifierTest.php index 62a7ce071..b0cb1a394 100644 --- a/core/components/minishop3/tests/CatalogSortbyQualifierTest.php +++ b/core/components/minishop3/tests/CatalogSortbyQualifierTest.php @@ -124,4 +124,93 @@ 'snippet drop mode falls back to msProduct.id when all parts dropped' ); +$assertSame( + 'RAND()', + CatalogSortbyQualifier::qualifyUnaliasedResourceFields('RAND()', $fields, 'msProduct', true, $dataFields), + 'snippet drop mode keeps RAND() without arguments' +); +$assertSame( + 'RAND() DESC', + CatalogSortbyQualifier::qualifyUnaliasedResourceFields('RAND() DESC', $fields, 'msProduct', true, $dataFields), + 'snippet drop mode keeps RAND() with direction' +); +$assertSame( + 'FIELD(msProduct.id, 5, 3, 1)', + CatalogSortbyQualifier::qualifyUnaliasedResourceFields( + 'FIELD(id, 5, 3, 1)', + $fields, + 'msProduct', + true, + $dataFields, + ), + 'snippet drop mode keeps FIELD() and qualifies bare id' +); +$assertSame( + 'IFNULL(msProduct.pagetitle, \'\') DESC', + CatalogSortbyQualifier::qualifyUnaliasedResourceFields( + 'IFNULL(pagetitle, \'\') DESC', + $fields, + 'msProduct', + true, + $dataFields, + ), + 'snippet drop mode keeps IFNULL() and qualifies bare fields' +); +$assertSame( + 'msProduct.id', + CatalogSortbyQualifier::qualifyUnaliasedResourceFields( + 'RAND(1), SLEEP(1)', + $fields, + 'msProduct', + true, + $dataFields, + ), + 'snippet drop mode rejects RAND with args and unsafe functions' +); + +$dropped = null; +$assertSame( + 'msProduct.pagetitle', + CatalogSortbyQualifier::qualifyUnaliasedResourceFields( + 'pagetitle, evil_injection, (SELECT 1)', + $fields, + 'msProduct', + true, + $dataFields, + 'Data', + [], + ['msProduct', 'Data', 'Vendor'], + $dropped, + ), + 'snippet drop mode keeps valid parts while collecting drops' +); +$assertSame( + ['evil_injection', '(SELECT 1)'], + $dropped, + 'snippet drop mode reports dropped parts' +); + +$assertSame( + ['Custom', 'Vendor'], + CatalogSortbyQualifier::tableAliasesFromJoins( + ['Custom' => ['class' => 'X'], 'Vendor' => ['class' => 'Y']], + ['bad key' => []], + ), + 'join alias helper collects safe keys only' +); +$assertSame( + 'Custom.score DESC, msProduct.pagetitle', + CatalogSortbyQualifier::qualifyUnaliasedResourceFields( + 'Custom.score DESC, pagetitle, Other.x', + $fields, + 'msProduct', + true, + $dataFields, + 'Data', + [], + ['msProduct', 'Data', 'Vendor', 'Custom'], + ), + 'snippet drop mode allows leftJoin aliases and rejects unknown prefixes' +); + fwrite(STDOUT, "OK: CatalogSortbyQualifierTest\n"); From 2f8a6e05f4476b62b32a359f0e063063847b0cfe Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Tue, 22 Sep 2026 15:06:50 +0600 Subject: [PATCH 5/5] fix(catalog): satisfy PHPStan on droppedParts by-ref type Stop assigning a fresh array into the optional by-ref out-param up front; only write list when the caller actually passed the argument. --- .../Catalog/CatalogSortbyQualifier.php | 23 ++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/core/components/minishop3/src/Services/Catalog/CatalogSortbyQualifier.php b/core/components/minishop3/src/Services/Catalog/CatalogSortbyQualifier.php index 731525b51..331f26111 100644 --- a/core/components/minishop3/src/Services/Catalog/CatalogSortbyQualifier.php +++ b/core/components/minishop3/src/Services/Catalog/CatalogSortbyQualifier.php @@ -28,7 +28,7 @@ final class CatalogSortbyQualifier * @param list $dataFieldNames Field names from msProductData (qualified as Data.*) * @param list $passthroughNames Bare names left as-is (TVs, vendor_*, option keys) * @param list $allowedTableAliases When dropUnmatched, only these table prefixes pass - * @param list|null $droppedParts Filled with rejected sort parts when dropUnmatched + * @param list|null $droppedParts Optional out-list of rejected parts when the argument is passed */ public static function qualifyUnaliasedResourceFields( string $sortby, @@ -41,13 +41,22 @@ public static function qualifyUnaliasedResourceFields( array $allowedTableAliases = ['msProduct', 'Data', 'Vendor'], ?array &$droppedParts = null, ): string { - $droppedParts = []; + $collectDropped = \func_num_args() >= 9; + $dropped = []; $trimmed = ltrim($sortby); if ($trimmed === '' || str_starts_with($trimmed, '{')) { + if ($collectDropped) { + $droppedParts = $dropped; + } + return $sortby; } if (!$dropUnmatched && str_contains($sortby, '(')) { + if ($collectDropped) { + $droppedParts = $dropped; + } + return $sortby; } @@ -56,6 +65,10 @@ public static function qualifyUnaliasedResourceFields( $passthrough = self::indexNames($passthroughNames); $allowedTables = self::indexNames($allowedTableAliases); if ($resourceFields === [] && $dataFields === [] && $passthrough === []) { + if ($collectDropped) { + $droppedParts = $dropped; + } + return $sortby; } @@ -88,7 +101,7 @@ public static function qualifyUnaliasedResourceFields( $passthrough, $allowedTables, )) { - $droppedParts[] = $part; + $dropped[] = $part; continue; } } @@ -102,6 +115,10 @@ public static function qualifyUnaliasedResourceFields( ); } + if ($collectDropped) { + $droppedParts = $dropped; + } + if ($qualified === []) { return $dropUnmatched ? $alias . '.id' : ''; }