diff --git a/core/components/minishop3/src/Controllers/Api/Web/CustomerEmailController.php b/core/components/minishop3/src/Controllers/Api/Web/CustomerEmailController.php index 89f62f19..eb1c4473 100644 --- a/core/components/minishop3/src/Controllers/Api/Web/CustomerEmailController.php +++ b/core/components/minishop3/src/Controllers/Api/Web/CustomerEmailController.php @@ -5,6 +5,7 @@ use MiniShop3\MiniShop3; use MiniShop3\Model\msCustomer; use MiniShop3\Router\Response; +use MiniShop3\Services\Customer\CustomerPublicDto; use MiniShop3\Services\Customer\EmailVerificationService; use MODX\Revolution\modX; @@ -127,7 +128,10 @@ public function verify(array $params): array|Response return $this->success( $this->modx->lexicon('ms3_customer_email_verify_success'), - ['customer_id' => $customer->id] + [ + 'customer_id' => $customer->id, + 'customer' => CustomerPublicDto::fromCustomer($customer, $this->modx, $this->ms3), + ] ); } diff --git a/core/components/minishop3/src/Controllers/Api/Web/CustomerProfileController.php b/core/components/minishop3/src/Controllers/Api/Web/CustomerProfileController.php index d8bf918c..eeca791a 100644 --- a/core/components/minishop3/src/Controllers/Api/Web/CustomerProfileController.php +++ b/core/components/minishop3/src/Controllers/Api/Web/CustomerProfileController.php @@ -20,31 +20,6 @@ */ class CustomerProfileController { - /** - * Field names never editable via POST /api/v1/customer/add (security & system counters). - * - * @var list - */ - private const PROFILE_QUICK_UPDATE_FORBIDDEN = [ - 'id', - 'token', - 'user_id', - 'password', - 'email_verified_at', - 'is_active', - 'is_blocked', - 'failed_login_attempts', - 'blocked_until', - 'created_at', - 'updated_at', - 'last_login_at', - 'orders_count', - 'total_spent', - 'last_order_at', - 'privacy_accepted_at', - 'privacy_ip', - ]; - /** @var modX */ protected modX $modx; @@ -84,9 +59,21 @@ public function update(array $data): array } $customerId = (int)$customer->get('id'); - $validator = new Validator(); - $validation = $validator->make($data, $this->getProfileFieldRules()); + $this->ms3->loadMap(); + $editableKeys = CustomerPublicDto::editableFieldKeys($this->modx, $this->ms3); + $data = array_intersect_key($data, array_flip($editableKeys)); + if ($data === []) { + return $this->error($this->modx->lexicon('ms3_customer_err_validation')); + } + + // Partial update: validate only the core profile rules for fields + // actually present in $data. Missing core fields are left untouched + // rather than rejected (#424 review). + $rules = array_intersect_key($this->getProfileFieldRules(), $data); + + $validator = new Validator(); + $validation = $validator->make($data, $rules); $validation->validate(); if ($validation->fails()) { @@ -99,21 +86,33 @@ public function update(array $data): array ); } - $newEmail = trim($data['email']); - - if (!$this->isEmailAvailable($customer, $newEmail)) { - $_SESSION['ms3']['customer_profile_errors'] = [ - 'email' => $this->modx->lexicon('ms3_customer_err_email_exists') - ]; - return $this->error($this->modx->lexicon('ms3_customer_err_email_exists')); + $fieldMeta = $this->modx->getFieldMeta(msCustomer::class); + if (!is_array($fieldMeta) || $fieldMeta === []) { + return $this->error($this->modx->lexicon('ms3_customer_err_save')); } - $this->resetEmailVerificationIfChanged($customer, $newEmail); + foreach ($data as $key => $rawValue) { + if ($key === 'email') { + $newEmail = trim((string) $rawValue); + if (!$this->isEmailAvailable($customer, $newEmail)) { + $_SESSION['ms3']['customer_profile_errors'] = [ + 'email' => $this->modx->lexicon('ms3_customer_err_email_exists'), + ]; + + return $this->error($this->modx->lexicon('ms3_customer_err_email_exists')); + } + $this->resetEmailVerificationIfChanged($customer, $newEmail); + $customer->set('email', $newEmail); + continue; + } + + if (isset($rules[$key])) { + $customer->set($key, trim((string) $rawValue)); + continue; + } - $customer->set('first_name', trim($data['first_name'])); - $customer->set('last_name', trim($data['last_name'])); - $customer->set('email', $newEmail); - $customer->set('phone', trim($data['phone'])); + $customer->set($key, $this->normalizeQuickProfileValue($rawValue, $fieldMeta[$key])); + } if (!$customer->save()) { return $this->error($this->modx->lexicon('ms3_customer_err_save')); @@ -128,7 +127,7 @@ public function update(array $data): array return $this->success( $this->modx->lexicon('ms3_customer_profile_updated'), - ['customer' => CustomerPublicDto::fromCustomer($customer)] + ['customer' => CustomerPublicDto::fromCustomer($customer, $this->modx, $this->ms3)] ); } @@ -157,16 +156,13 @@ public function updateField(array $data): array } $this->ms3->loadMap(); - $fieldMeta = $this->modx->getFieldMeta(msCustomer::class); - if (!is_array($fieldMeta) || $fieldMeta === []) { - return $this->error($this->modx->lexicon('ms3_customer_err_field_not_allowed')); - } - - if (in_array($key, self::PROFILE_QUICK_UPDATE_FORBIDDEN, true)) { + $editableKeys = CustomerPublicDto::editableFieldKeys($this->modx, $this->ms3); + if (!in_array($key, $editableKeys, true)) { return $this->error($this->modx->lexicon('ms3_customer_err_field_not_allowed')); } - if (!isset($fieldMeta[$key])) { + $fieldMeta = $this->modx->getFieldMeta(msCustomer::class); + if (!is_array($fieldMeta) || !isset($fieldMeta[$key])) { return $this->error($this->modx->lexicon('ms3_customer_err_field_not_allowed')); } @@ -206,7 +202,7 @@ public function updateField(array $data): array $this->modx->lexicon('ms3_customer_profile_updated'), [ $key => $customer->get($key), - 'customer' => CustomerPublicDto::fromCustomer($customer), + 'customer' => CustomerPublicDto::fromCustomer($customer, $this->modx, $this->ms3), ] ); } diff --git a/core/components/minishop3/src/Processors/Api/Customer/Login.php b/core/components/minishop3/src/Processors/Api/Customer/Login.php index 6e8545a5..78206541 100644 --- a/core/components/minishop3/src/Processors/Api/Customer/Login.php +++ b/core/components/minishop3/src/Processors/Api/Customer/Login.php @@ -2,7 +2,9 @@ namespace MiniShop3\Processors\Api\Customer; +use MiniShop3\MiniShop3; use MiniShop3\Services\Customer\AuthManager; +use MiniShop3\Services\Customer\CustomerPublicDto; use MiniShop3\Services\Customer\RateLimiter; use MODX\Revolution\Processors\Processor; @@ -98,15 +100,11 @@ public function process() $redirectUrl = $this->modx->makeUrl($redirectPageId, '', '', 'full'); } + /** @var MiniShop3 $ms3 */ + $ms3 = $this->modx->services->get('ms3'); + return $this->success('', [ - 'customer' => [ - 'id' => $customer->id, - 'email' => $customer->get('email'), - 'first_name' => $customer->get('first_name'), - 'last_name' => $customer->get('last_name'), - 'phone' => $customer->get('phone'), - 'email_verified' => !empty($customer->get('email_verified_at')), - ], + 'customer' => CustomerPublicDto::fromCustomer($customer, $this->modx, $ms3), 'token' => $session['token'], 'expires_at' => $session['expires_at'], 'redirect_url' => $redirectUrl, diff --git a/core/components/minishop3/src/Processors/Api/Customer/Register.php b/core/components/minishop3/src/Processors/Api/Customer/Register.php index 295b5323..87b895af 100644 --- a/core/components/minishop3/src/Processors/Api/Customer/Register.php +++ b/core/components/minishop3/src/Processors/Api/Customer/Register.php @@ -2,7 +2,9 @@ namespace MiniShop3\Processors\Api\Customer; +use MiniShop3\MiniShop3; use MiniShop3\Services\Customer\AuthManager; +use MiniShop3\Services\Customer\CustomerPublicDto; use MiniShop3\Services\Customer\EmailVerificationService; use MiniShop3\Services\Customer\RateLimiter; use MiniShop3\Services\Customer\RegisterService; @@ -110,15 +112,11 @@ public function process() } } + /** @var MiniShop3 $ms3 */ + $ms3 = $this->modx->services->get('ms3'); + return $this->success($this->modx->lexicon('ms3_customer_register_success'), [ - 'customer' => [ - 'id' => $customer->id, - 'email' => $customer->get('email'), - 'first_name' => $customer->get('first_name'), - 'last_name' => $customer->get('last_name'), - 'phone' => $customer->get('phone'), - 'email_verified' => !empty($customer->get('email_verified_at')), - ], + 'customer' => CustomerPublicDto::fromCustomer($customer, $this->modx, $ms3), 'token' => $tokenString, 'expires_at' => $expiresAt, 'email_verification_required' => $requireEmailVerification, diff --git a/core/components/minishop3/src/Processors/Api/Customer/VerifyEmail.php b/core/components/minishop3/src/Processors/Api/Customer/VerifyEmail.php index 078b7c9a..56c160dd 100644 --- a/core/components/minishop3/src/Processors/Api/Customer/VerifyEmail.php +++ b/core/components/minishop3/src/Processors/Api/Customer/VerifyEmail.php @@ -2,6 +2,8 @@ namespace MiniShop3\Processors\Api\Customer; +use MiniShop3\MiniShop3; +use MiniShop3\Services\Customer\CustomerPublicDto; use MiniShop3\Services\Customer\EmailVerificationService; use MODX\Revolution\Processors\Processor; @@ -39,12 +41,11 @@ public function process() return $this->failure($this->modx->lexicon('ms3_customer_err_email_verification_invalid')); } + /** @var MiniShop3 $ms3 */ + $ms3 = $this->modx->services->get('ms3'); + return $this->success($this->modx->lexicon('ms3_customer_email_verify_success'), [ - 'customer' => [ - 'id' => $customer->id, - 'email' => $customer->get('email'), - 'email_verified' => true, - ], + 'customer' => CustomerPublicDto::fromCustomer($customer, $this->modx, $ms3), ]); } } diff --git a/core/components/minishop3/src/Services/Customer/CustomerExtraFieldRegistry.php b/core/components/minishop3/src/Services/Customer/CustomerExtraFieldRegistry.php new file mode 100644 index 00000000..dcd98378 --- /dev/null +++ b/core/components/minishop3/src/Services/Customer/CustomerExtraFieldRegistry.php @@ -0,0 +1,38 @@ + + */ + public static function activeKeys(modX $modx): array + { + $keys = []; + $iterator = $modx->getIterator(msExtraField::class, [ + 'class' => msCustomer::class, + 'active' => 1, + ]); + + foreach ($iterator as $field) { + $keys[] = (string) $field->get('key'); + } + + return $keys; + } +} diff --git a/core/components/minishop3/src/Services/Customer/CustomerPublicDto.php b/core/components/minishop3/src/Services/Customer/CustomerPublicDto.php index 3b43041c..3712b0fc 100644 --- a/core/components/minishop3/src/Services/Customer/CustomerPublicDto.php +++ b/core/components/minishop3/src/Services/Customer/CustomerPublicDto.php @@ -4,15 +4,17 @@ namespace MiniShop3\Services\Customer; +use MiniShop3\MiniShop3; use MiniShop3\Model\msCustomer; +use MODX\Revolution\modX; /** - * Public customer payload for Web API responses (no secrets / lock internals). + * Public customer payload and profile field allowlist for Web API (#424). */ final class CustomerPublicDto { /** - * Fields allowed in public customer JSON (fail-closed for new columns). + * Core fields exposed in public customer JSON (fail-closed for new columns). * * @var list */ @@ -33,20 +35,126 @@ final class CustomerPublicDto 'privacy_accepted_at', ]; + /** + * Base profile fields editable via Web API (Rakit rules apply in controller). + * + * @var list + */ + public const CORE_PROFILE_EDITABLE_FIELDS = [ + 'first_name', + 'last_name', + 'email', + 'phone', + ]; + + /** + * Never writable from Web customer profile/add endpoints. + * + * @var list + */ + public const SYSTEM_NON_EDITABLE_FIELDS = [ + 'id', + 'token', + 'user_id', + 'password', + 'email_verified_at', + 'is_active', + 'is_blocked', + 'failed_login_attempts', + 'blocked_until', + 'created_at', + 'updated_at', + 'last_login_at', + 'orders_count', + 'total_spent', + 'last_order_at', + 'privacy_accepted_at', + 'privacy_ip', + ]; + /** * @param array $customerFields * @return array */ - public static function fromArray(array $customerFields): array + public static function fromArray(array $customerFields, array $extraPublicKeys = []): array { - return array_intersect_key($customerFields, array_flip(self::PUBLIC_FIELDS)); + $allowed = self::publicFieldKeys($extraPublicKeys); + + return array_intersect_key($customerFields, array_flip($allowed)); } /** * @return array */ - public static function fromCustomer(msCustomer $customer): array + public static function fromCustomer(msCustomer $customer, modX $modx, MiniShop3 $ms3): array + { + return self::fromArray($customer->toArray(), CustomerExtraFieldRegistry::activeKeys($modx)); + } + + /** + * @return list + */ + public static function publicFieldKeys(array $extraFieldKeys = []): array + { + $keys = self::PUBLIC_FIELDS; + + foreach ($extraFieldKeys as $key) { + $key = (string) $key; + if ($key === '' || in_array($key, self::SYSTEM_NON_EDITABLE_FIELDS, true)) { + continue; + } + if (!in_array($key, $keys, true)) { + $keys[] = $key; + } + } + + return $keys; + } + + /** + * @param list $extraFieldKeys Active msExtraField keys for msCustomer + * + * @return list + */ + public static function mergeEditableFieldKeys(array $extraFieldKeys): array + { + $allowed = self::CORE_PROFILE_EDITABLE_FIELDS; + + foreach ($extraFieldKeys as $key) { + $key = (string) $key; + if ($key === '' || in_array($key, self::SYSTEM_NON_EDITABLE_FIELDS, true)) { + continue; + } + if (!in_array($key, $allowed, true)) { + $allowed[] = $key; + } + } + + return $allowed; + } + + /** + * @return list + */ + public static function editableFieldKeys(modX $modx, MiniShop3 $ms3): array + { + $ms3->loadMap(); + $fieldMeta = $modx->getFieldMeta(msCustomer::class); + if (!is_array($fieldMeta) || $fieldMeta === []) { + return self::CORE_PROFILE_EDITABLE_FIELDS; + } + + $extraKeys = CustomerExtraFieldRegistry::activeKeys($modx); + $merged = self::mergeEditableFieldKeys($extraKeys); + + return array_values(array_filter( + $merged, + static fn(string $key): bool => isset($fieldMeta[$key]) + )); + } + + public static function isEditableField(string $key, modX $modx, MiniShop3 $ms3): bool { - return self::fromArray($customer->toArray()); + return in_array($key, self::editableFieldKeys($modx, $ms3), true); } } diff --git a/core/components/minishop3/tests/CustomerAuthProcessorsDtoTest.php b/core/components/minishop3/tests/CustomerAuthProcessorsDtoTest.php new file mode 100644 index 00000000..43e2583b --- /dev/null +++ b/core/components/minishop3/tests/CustomerAuthProcessorsDtoTest.php @@ -0,0 +1,182 @@ +modx, $ms3)')) { + $fail("{$file} must return customer via CustomerPublicDto::fromCustomer"); + } + + if (preg_match("/'email_verified'\\s*=>/", $src)) { + $fail("{$file} must not expose legacy email_verified boolean"); + } + + if (preg_match("/'customer'\\s*=>\\s*\\[/", $src)) { + $fail("{$file} must not build inline customer array"); + } +} + +// Behavior test: fromCustomer() serializes a customer with extra-field allowlist +// and never leaks secret/system columns (#424 review: replace grep-only checks). +$extraKeys = ['loyalty_tier', 'company']; + +$modx = new class ($extraKeys) extends modX { + /** @var list */ + private array $extraKeys; + + /** + * @param list $extraKeys + */ + public function __construct(array $extraKeys) + { + parent::__construct(); + $this->extraKeys = $extraKeys; + } + + public function getIterator($className, $criteria = null) + { + if ($className !== msExtraField::class) { + return new ArrayIterator([]); + } + + $fields = array_map( + static fn(string $key): object => new class ($key) { + public function __construct(private string $key) + { + } + + public function get(string $k): mixed + { + return $k === 'key' ? $this->key : null; + } + }, + $this->extraKeys + ); + + return new ArrayIterator($fields); + } +}; + +$ms3 = new class extends MiniShop3 { + public function __construct() + { + // Bypass real constructor: fromCustomer() does not use ms3. + } +}; + +$customer = new StubMsCustomer([ + 'id' => 42, + 'user_id' => 7, + 'first_name' => 'Ada', + 'last_name' => 'Lovelace', + 'email' => 'ada@example.com', + 'phone' => '+10000000000', + 'password' => '$2y$10$notarealhash', + 'token' => 'session-or-api-secret', + 'email_verified_at' => '2026-01-02 03:04:05', + 'is_active' => true, + 'is_blocked' => true, + 'privacy_ip' => '203.0.113.10', + 'failed_login_attempts' => 3, + 'blocked_until' => '2026-01-01 00:00:00', + 'created_at' => '2025-01-01 00:00:00', + 'updated_at' => '2025-06-01 00:00:00', + 'last_login_at' => '2025-12-01 00:00:00', + 'orders_count' => 2, + 'total_spent' => 99.5, + 'last_order_at' => '2025-11-01 00:00:00', + 'privacy_accepted_at' => '2025-01-01 00:00:00', + 'loyalty_tier' => 'gold', + 'company' => 'ACME', + 'future_secret_column' => 'must-not-leak', +]); + +$public = CustomerPublicDto::fromCustomer($customer, $modx, $ms3); + +$expected = [ + 'id' => 42, + 'first_name' => 'Ada', + 'last_name' => 'Lovelace', + 'email' => 'ada@example.com', + 'phone' => '+10000000000', + 'email_verified_at' => '2026-01-02 03:04:05', + 'is_active' => true, + 'created_at' => '2025-01-01 00:00:00', + 'updated_at' => '2025-06-01 00:00:00', + 'last_login_at' => '2025-12-01 00:00:00', + 'orders_count' => 2, + 'total_spent' => 99.5, + 'last_order_at' => '2025-11-01 00:00:00', + 'privacy_accepted_at' => '2025-01-01 00:00:00', + 'loyalty_tier' => 'gold', + 'company' => 'ACME', +]; + +if ($public !== $expected) { + $fail(sprintf( + "fromCustomer behavior mismatch:\nexpected: %s\nactual: %s", + json_encode($expected, JSON_UNESCAPED_SLASHES), + json_encode($public, JSON_UNESCAPED_SLASHES) + )); +} + +foreach (['password', 'token', 'privacy_ip', 'failed_login_attempts', 'blocked_until', 'is_blocked', 'user_id', 'future_secret_column'] as $hidden) { + if (array_key_exists($hidden, $public)) { + $fail("fromCustomer leaked secret/system field: {$hidden}"); + } +} + +// Empty extra-field registry → only core public fields, no extras leak. +$emptyModx = new class extends modX { + public function getIterator($className, $criteria = null) + { + return new ArrayIterator([]); + } +}; +$barePublic = CustomerPublicDto::fromCustomer($customer, $emptyModx, $ms3); +if (array_key_exists('loyalty_tier', $barePublic) || array_key_exists('company', $barePublic)) { + $fail('fromCustomer must not expose extra fields when registry has none registered'); +} + +fwrite(STDOUT, "OK CustomerAuthProcessorsDtoTest\n"); +exit(0); diff --git a/core/components/minishop3/tests/CustomerPublicDtoTest.php b/core/components/minishop3/tests/CustomerPublicDtoTest.php index c8bb1ccf..7c84ab1a 100644 --- a/core/components/minishop3/tests/CustomerPublicDtoTest.php +++ b/core/components/minishop3/tests/CustomerPublicDtoTest.php @@ -75,6 +75,21 @@ } } +$coreEditable = CustomerPublicDto::CORE_PROFILE_EDITABLE_FIELDS; +foreach (['first_name', 'last_name', 'email', 'phone'] as $key) { + if (!in_array($key, $coreEditable, true)) { + $fail("CORE_PROFILE_EDITABLE_FIELDS missing {$key}"); + } +} + +$withExtra = CustomerPublicDto::fromArray( + array_merge($input, ['loyalty_tier' => 'gold']), + ['loyalty_tier'] +); +if (($withExtra['loyalty_tier'] ?? null) !== 'gold') { + $fail('registered extra field must be included in public payload'); +} + $controllerSrc = file_get_contents(__DIR__ . '/../src/Controllers/Api/Web/CustomerProfileController.php'); if ($controllerSrc === false) { $fail('unable to read CustomerProfileController.php'); @@ -82,8 +97,8 @@ if (str_contains($controllerSrc, '$customer->toArray()')) { $fail('CustomerProfileController still serializes full customer via toArray()'); } -if (!str_contains($controllerSrc, 'CustomerPublicDto::fromCustomer')) { - $fail('CustomerProfileController missing CustomerPublicDto::fromCustomer'); +if (!str_contains($controllerSrc, 'CustomerPublicDto::fromCustomer($customer, $this->modx, $this->ms3)')) { + $fail('CustomerProfileController missing CustomerPublicDto::fromCustomer with modx/ms3'); } fwrite(STDOUT, "OK CustomerPublicDtoTest\n"); diff --git a/core/components/minishop3/tests/ProfileQuickUpdateForbiddenTest.php b/core/components/minishop3/tests/ProfileQuickUpdateForbiddenTest.php index a63b6b14..0ec29d37 100644 --- a/core/components/minishop3/tests/ProfileQuickUpdateForbiddenTest.php +++ b/core/components/minishop3/tests/ProfileQuickUpdateForbiddenTest.php @@ -1,47 +1,74 @@ getProfileFieldRules\\(\\),\\s*\\$data\\)/', $controllerSrc)) { + $fail('update must restrict validation to core rules for fields present in $data'); } -$forbidden = $keys[1]; foreach (['privacy_accepted_at', 'privacy_ip', 'password', 'token', 'email_verified_at'] as $key) { - if (!in_array($key, $forbidden, true)) { - $fail("PROFILE_QUICK_UPDATE_FORBIDDEN must include {$key}"); + if (!in_array($key, CustomerPublicDto::SYSTEM_NON_EDITABLE_FIELDS, true)) { + $fail("SYSTEM_NON_EDITABLE_FIELDS must include {$key}"); } } -if (!str_contains($src, 'in_array($key, self::PROFILE_QUICK_UPDATE_FORBIDDEN, true)')) { - $fail('updateField must enforce PROFILE_QUICK_UPDATE_FORBIDDEN'); +$coreEditable = CustomerPublicDto::CORE_PROFILE_EDITABLE_FIELDS; +foreach (['first_name', 'last_name', 'email', 'phone'] as $key) { + if (!in_array($key, $coreEditable, true)) { + $fail("CORE_PROFILE_EDITABLE_FIELDS must include {$key}"); + } +} + +$merged = CustomerPublicDto::mergeEditableFieldKeys(['company', 'password', 'privacy_accepted_at']); +if (!in_array('company', $merged, true)) { + $fail('active extra field key must merge into editable allowlist'); +} +foreach (['password', 'privacy_accepted_at'] as $blocked) { + if (in_array($blocked, $merged, true)) { + $fail("non-editable key must not merge: {$blocked}"); + } +} + +$publicWithExtra = CustomerPublicDto::fromArray( + ['id' => 1, 'company' => 'ACME', 'password' => 'secret'], + ['company'] +); +if (($publicWithExtra['company'] ?? null) !== 'ACME') { + $fail('extra public field must appear when registered in msExtraField keys'); +} +if (array_key_exists('password', $publicWithExtra)) { + $fail('password must never appear in public DTO'); } fwrite(STDOUT, "OK ProfileQuickUpdateForbiddenTest\n"); diff --git a/core/components/minishop3/tests/stubs/StubMsCustomer.php b/core/components/minishop3/tests/stubs/StubMsCustomer.php new file mode 100644 index 00000000..00013bd4 --- /dev/null +++ b/core/components/minishop3/tests/stubs/StubMsCustomer.php @@ -0,0 +1,37 @@ + */ + private array $fields; + + /** + * @param array $fields + */ + public function __construct(array $fields = []) + { + $this->fields = $fields; + } + + public function get($key) + { + return $this->fields[$key] ?? null; + } + + /** + * @return array + */ + public function toArray(): array + { + return $this->fields; + } +}