Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -64,15 +64,18 @@ public function __construct(
*/
public function isAvailable(ExpressCheckoutAvailabilityRequest $request): ExpressCheckoutAvailabilityResponse
{
$available = $this->expressCheckoutService->isExpressCheckoutAvailable(
$request->getPage(),
$request->getCountry(),
$request->getCurrency(),
$request->getIpAddress(),
$request->getProductIds(),
$request->getCategoryIds()
);

return new ExpressCheckoutAvailabilityResponse(
$this->expressCheckoutService->isExpressCheckoutAvailable(
$request->getPage(),
$request->getCountry(),
$request->getCurrency(),
$request->getIpAddress(),
$request->getProductIds(),
$request->getCategoryIds()
)
$available,
$available ? $this->resolveButtonStyle() : null

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏗️ altitude

The controller now owns a business rule ("suppress the style when Express Checkout is unavailable") plus a private domain-lookup helper, contrary to the repo's thin-controller rule.

.claude/docs/codingStandard.md §7: "Controllers are thin: translate Request → domain call → Response. No business logic in controllers."

$available ? $this->resolveButtonStyle() : null (line 78), the mirrored $hasCountries ? $this->resolveButtonStyle() : null (line 113), and the resolveButtonStyle() null-settings fallback (lines 150-155) are all policy decided in the adapter layer.

The rule is now duplicated across two methods and is untestable at the domain level; the next endpoint that needs the style has to re-derive it. It belongs in ExpressCheckoutService — e.g. a method returning availability and the applicable style together, which also fixes the double repository read.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declined for now. It is a ternary duplicated across two methods, and the cure — having the service return availability and the applicable style together — is the same refactor as the double-read finding above. Both stay on the table together, neither justifies the churn on its own.

);
}

Expand Down Expand Up @@ -102,8 +105,13 @@ public function isAvailableForGuest(
);

$countries = $available ? $this->countryConfigurationService->getCountryCodes() : [];
$hasCountries = !empty($countries);

return new GuestExpressCheckoutAvailabilityResponse(!empty($countries), $countries);
return new GuestExpressCheckoutAvailabilityResponse(
$hasCountries,
$countries,
$hasCountries ? $this->resolveButtonStyle() : null
);
}

/**
Expand Down Expand Up @@ -135,4 +143,14 @@ public function solicit(ExpressCheckoutSolicitRequest $request): IdentificationF

return new IdentificationFormResponse($form);
}

/**
* @return string|null
*/
private function resolveButtonStyle(): ?string

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚡ efficiency

resolveButtonStyle() re-reads Express Checkout settings from the repository even though the availability call it follows has already loaded the exact same row, doubling DB queries on the storefront hot path.

isAvailable()ExpressCheckoutService::isExpressCheckoutAvailable()isAvailableForGuest()getExpressCheckoutSettings()ExpressCheckoutSettingsRepository::getEntity()selectOne($queryFilter) — one query.

When $available is true, resolveButtonStyle() calls getExpressCheckoutSettings() again → a second identical selectOne, with no cache anywhere in the chain. Same in isAvailableForGuest() (line 113).

Every product / cart / mini-cart page render on every store now issues two settings queries instead of one. Fix: have the service expose the loaded settings (e.g. return them alongside availability) rather than re-fetching in the controller.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declined. What this costs is a second selectOne on a single indexed row; restructuring the service signature to return availability and style together is a bigger change than it buys. If profiling on a real storefront says otherwise, this is the first thing to revisit — and it comes bundled with the altitude finding below.

{
$settings = $this->expressCheckoutService->getExpressCheckoutSettings();

return $settings ? $settings->getButtonStyle() : null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,29 @@ class ExpressCheckoutAvailabilityResponse extends Response
*/
protected $available;

/**
* @var string|null
*/
protected $buttonStyle;

/**
* @param bool $available
* @param string|null $buttonStyle
*/
public function __construct(bool $available)
public function __construct(bool $available, ?string $buttonStyle = null)
{
$this->available = $available;
$this->buttonStyle = $buttonStyle;
}

/**
* @inheritDoc
*/
public function toArray(): array
{
return ['available' => $this->available];
return [
'available' => $this->available,
'buttonStyle' => $this->buttonStyle,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 security

A merchant-controlled string is echoed verbatim into the public storefront availability response with no escaping, no content validation and no length bound, making every deployed plugin solely responsible for preventing stored XSS.

ConfigurationWebhookAPITest.php:2348 explicitly asserts that '{"attributeThisReleaseNeverHeardOf":"<script>x</script>", ...}' survives the round trip untouched. That string is persisted and later returned by isAvailable()/isAvailableForGuest().

Any integration that interpolates buttonStyle into an inline <style>/<script>/attribute rather than passing it through JSON.parse + the CDN allow-list gets stored XSS on every product and cart page.

The core has the choke point here (one toArray()) and does nothing with it. At minimum re-encode via json_encode(json_decode(...)) to strip anything that isn't valid JSON structure, and cap the length.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partly taken, partly declined — leaving open for your call.

Taken: the length bound is in (62d572a), and the style must now decode to a JSON object, so the blob is no longer unbounded or structurally arbitrary.

Declined: the json_encode(json_decode(...)) re-encode. It does not sanitise values — PHP does not escape < by default, and with JSON_HEX_TAG it becomes \u003C, which JSON.parse turns straight back into <. So it only helps a plugin that interpolates the blob into an inline <script>, and that plugin has larger problems than this field.

On the escaping premise: the validation exists, in integration-assets#669, and it is a whitelist at the point of use — five known keys, a hex regex for colours, a token map for the radius, a clamped font size, all applied through style.setProperty and never through the markup template. The attributeThisReleaseNeverHeardOf value in that test is not one of the five keys, so it is dropped before its value is ever read; put <script> in backgroundColor and it fails the regex. The blob reaching the plugin as a data- attribute still needs ordinary attribute escaping, which esc_attr / escapeHtmlAttr already do for every attribute.

The core deliberately does not know the key names: they are owned by the portal and the button page so that adding one does not need a release of this library and an update of every installed plugin. Checking the shape (object, bounded) is schema-agnostic and compatible with that; checking the contents would not be.

];
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,24 @@ class GuestExpressCheckoutAvailabilityResponse extends Response
*/
protected $availableCountries;

/**
* @var string|null
*/
protected $buttonStyle;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ simplification

GuestExpressCheckoutAvailabilityResponse now duplicates a third field ($buttonStyle) already declared identically in ExpressCheckoutAvailabilityResponse, along with its docblock, constructor assignment and toArray() entry.

The two classes now hold identical protected $available + protected $buttonStyle declarations, identical @var string|null docblocks, identical assignments and identical 'buttonStyle' => $this->buttonStyle lines — the guest response is ExpressCheckoutAvailabilityResponse plus one array field.

A fourth style-related field means editing both files again, and a fix applied to only one (e.g. re-encoding the blob, per the escaping finding) silently leaves the guest endpoint unprotected.

Have the guest response extend ExpressCheckoutAvailabilityResponse and add only availableCountries.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declined. The argument that carried this one was that a fix applied to one response would leave the other unprotected — and that dissolves now that the validation lives in the domain model rather than in either DTO. What is left is cosmetic duplication of two fields.


/**
* @param bool $available
* @param string[] $availableCountries ISO country codes for which Express Checkout is available.
* @param string|null $buttonStyle
*/
public function __construct(bool $available, array $availableCountries)
{
public function __construct(
bool $available,
array $availableCountries,
?string $buttonStyle = null
) {
$this->available = $available;
$this->availableCountries = $availableCountries;
$this->buttonStyle = $buttonStyle;
}

/**
Expand All @@ -39,6 +49,7 @@ public function toArray(): array
return [
'available' => $this->available,
'availableCountries' => $this->availableCountries,
'buttonStyle' => $this->buttonStyle,
];
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use SeQura\Core\BusinessLogic\ConfigurationWebhookAPI\Requests\ExpressCheckout\SaveExpressCheckoutSettingsRequest;
use SeQura\Core\BusinessLogic\ConfigurationWebhookAPI\Responses\ExpressCheckout\SaveExpressCheckoutSettingsResponse;
use SeQura\Core\BusinessLogic\Domain\ExpressCheckout\Exceptions\DuplicatedExpressCheckoutPageException;
use SeQura\Core\BusinessLogic\Domain\ExpressCheckout\Exceptions\InvalidExpressCheckoutButtonStyleException;
use SeQura\Core\BusinessLogic\Domain\ExpressCheckout\Exceptions\InvalidExpressCheckoutPageConfigException;
use SeQura\Core\BusinessLogic\Domain\ExpressCheckout\Exceptions\InvalidExpressCheckoutPageException;
use SeQura\Core\BusinessLogic\Domain\ExpressCheckout\Services\ExpressCheckoutService;
Expand Down Expand Up @@ -39,6 +40,7 @@ public function __construct(ExpressCheckoutService $expressCheckoutService)
* @throws InvalidExpressCheckoutPageException
* @throws DuplicatedExpressCheckoutPageException
* @throws InvalidExpressCheckoutPageConfigException
* @throws InvalidExpressCheckoutButtonStyleException
*/
public function handle(array $payload): Response
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use SeQura\Core\BusinessLogic\ConfigurationWebhookAPI\Requests\ConfigurationWebhookRequest;
use SeQura\Core\BusinessLogic\Domain\ExpressCheckout\Exceptions\DuplicatedExpressCheckoutPageException;
use SeQura\Core\BusinessLogic\Domain\ExpressCheckout\Exceptions\InvalidExpressCheckoutButtonStyleException;
use SeQura\Core\BusinessLogic\Domain\ExpressCheckout\Exceptions\InvalidExpressCheckoutPageConfigException;
use SeQura\Core\BusinessLogic\Domain\ExpressCheckout\Exceptions\InvalidExpressCheckoutPageException;
use SeQura\Core\BusinessLogic\Domain\ExpressCheckout\Models\ExpressCheckoutPageConfig;
Expand All @@ -21,12 +22,19 @@ class SaveExpressCheckoutSettingsRequest extends ConfigurationWebhookRequest
*/
protected $expressCheckoutConfigs;

/**
* @var string|null
*/
protected $buttonStyle;

/**
* @param ExpressCheckoutPageConfig[] $expressCheckoutConfigs
* @param string|null $buttonStyle
*/
public function __construct(array $expressCheckoutConfigs)
public function __construct(array $expressCheckoutConfigs, ?string $buttonStyle = null)
{
$this->expressCheckoutConfigs = $expressCheckoutConfigs;
$this->buttonStyle = $buttonStyle;
}

/**
Expand All @@ -35,6 +43,7 @@ public function __construct(array $expressCheckoutConfigs)
* @return self
*
* @throws InvalidExpressCheckoutPageException
* @throws InvalidExpressCheckoutButtonStyleException
*/
public static function fromPayload(array $payload): object
{
Expand All @@ -47,17 +56,24 @@ public static function fromPayload(array $payload): object
}
}

return new self($configs);
$buttonStyle = $payload['buttonStyle'] ?? null;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 correctness

A save-express-checkout-settings webhook payload that omits buttonStyle silently wipes the merchant's stored style instead of leaving it untouched.

$payload['buttonStyle'] ?? null cannot distinguish "key absent" from "explicitly cleared", so transformToDomainModel() builds ExpressCheckoutSettings($configs, null) and setExpressCheckoutSettings() upserts it.

A merchant configures a style, then any client that posts only {topic, expressCheckoutConfigs} — an older portal build, a targeted page-enable/disable call, a retried partial payload — resets buttonStyle to null and the storefront button reverts to defaults with no error.

The PR's own test at ConfigurationWebhookAPITest.php:2303 (assertNull($persisted->getButtonStyle()) after a payload with no buttonStyle) locks this behaviour in.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not changing this, but flagging it as a product decision rather than a bug.

save-express-checkout-settings is a full-replace save: expressCheckoutConfigs behaves exactly the same way, so a payload that omits it clears the configs too. Under those semantics buttonStyle going back to null when it is absent is correct, and the test at ConfigurationWebhookAPITest.php:2303 documents that intent rather than locking a defect.

If the topic is meant to be a partial patch instead, then this is a real bug and the fix is distinguishing "key absent" from "explicitly cleared" — but that is a contract question for the portal (sequra/merchant-portal-frontend#1731), not something to guess at here. Leaving the thread open for that.


if ($buttonStyle !== null && !\is_string($buttonStyle)) {
throw new InvalidExpressCheckoutButtonStyleException();
}

return new self($configs, $buttonStyle);
}

/**
* @return ExpressCheckoutSettings
*
* @throws DuplicatedExpressCheckoutPageException
* @throws InvalidExpressCheckoutPageConfigException
* @throws InvalidExpressCheckoutButtonStyleException
*/
public function transformToDomainModel(): ExpressCheckoutSettings
{
return new ExpressCheckoutSettings($this->expressCheckoutConfigs);
return new ExpressCheckoutSettings($this->expressCheckoutConfigs, $this->buttonStyle);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ public function toArray(): array
$configs = $this->expressCheckoutSettings
? $this->expressCheckoutSettings->getExpressCheckoutConfigs()
: [];
$buttonStyle = $this->expressCheckoutSettings

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ simplification

GetExpressCheckoutSettingsResponse::toArray() hand-rebuilds the settings shape field by field even though the same PR just refactored the ORM entity to delegate to ExpressCheckoutSettings::toArray().

Adding buttonStyle required editing this file (two new lines plus a second null-guard ternary) and the domain toArray() and the entity — three places for one field.

This could be $this->expressCheckoutSettings ? $this->expressCheckoutSettings->toArray() : ['expressCheckoutConfigs' => [], 'buttonStyle' => null] merged with availablePages.

As written, the persisted shape and the webhook GET shape are two independent definitions kept in sync by hand; the next field will drift between them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declined, same reasoning as the guest-response thread: cosmetic duplication with no correctness consequence now that the shape is validated once in the domain. Worth doing the next time this response changes for another reason.

? $this->expressCheckoutSettings->getButtonStyle()
: null;

return [
'availablePages' => array_map(static function (ExpressCheckoutPage $page) {
Expand All @@ -50,6 +53,7 @@ public function toArray(): array
'expressCheckoutConfigs' => array_map(static function (ExpressCheckoutPageConfig $config) {
return $config->toArray();
}, $configs),
'buttonStyle' => $buttonStyle,
];
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace SeQura\Core\BusinessLogic\DataAccess\ExpressCheckout\Entities;

use SeQura\Core\BusinessLogic\Domain\ExpressCheckout\Exceptions\DuplicatedExpressCheckoutPageException;
use SeQura\Core\BusinessLogic\Domain\ExpressCheckout\Exceptions\InvalidExpressCheckoutButtonStyleException;
use SeQura\Core\BusinessLogic\Domain\ExpressCheckout\Exceptions\InvalidExpressCheckoutPageConfigException;
use SeQura\Core\BusinessLogic\Domain\ExpressCheckout\Exceptions\InvalidExpressCheckoutPageException;
use SeQura\Core\BusinessLogic\Domain\ExpressCheckout\Models\ExpressCheckoutPageConfig;
Expand Down Expand Up @@ -53,7 +54,20 @@ public function inflate(array $data): void
}
}

$this->expressCheckoutSettings = new DomainExpressCheckoutSettings($configs);
$buttonStyle = static::getDataValue($expressCheckoutSettings, 'buttonStyle', null);
if (!\is_string($buttonStyle)) {
$buttonStyle = null;
}

// A style is rejected when it is written, so a stored one that no longer
// validates means the row was corrupted or hand-edited. Dropping just the
// style keeps the page configs readable: the button falls back to its
// default look instead of every read of this row failing.
try {
$this->expressCheckoutSettings = new DomainExpressCheckoutSettings($configs, $buttonStyle);
} catch (InvalidExpressCheckoutButtonStyleException $exception) {
$this->expressCheckoutSettings = new DomainExpressCheckoutSettings($configs);
}
}

/**
Expand All @@ -63,11 +77,7 @@ public function toArray(): array
{
$data = parent::toArray();
$data['storeId'] = $this->storeId;
$data['expressCheckoutSettings'] = [
'expressCheckoutConfigs' => array_map(static function (ExpressCheckoutPageConfig $config) {
return $config->toArray();
}, $this->expressCheckoutSettings->getExpressCheckoutConfigs()),
];
$data['expressCheckoutSettings'] = $this->expressCheckoutSettings->toArray();
Comment thread
mescalantea marked this conversation as resolved.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 correctness

Replacing the entity's explicit persistence mapping with $this->expressCheckoutSettings->toArray() couples the on-disk storage format to the domain model's serialization, so any future field added to the domain silently changes what is written to storage.

The deleted code spelled out exactly which keys are persisted.

Now, if someone adds a derived / presentation field to ExpressCheckoutSettings::toArray() (a resolved default style, a computed enabledPages list), it is written into every stored row without anyone touching the DataAccess layer — and inflate(), which reads only expressCheckoutConfigs and buttonStyle, silently drops it on the next load, producing an asymmetric round trip.

Keep the entity's mapping explicit, or add a matching ExpressCheckoutSettings::fromArray() so the two halves stay symmetric by construction.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declined. The asymmetry is real, but there is no defect today, and the explicit mapping was removed on purpose in 2fbc9c1 to stop the same field being spelled out in three places. Reinstating it, or adding a fromArray() to pair with it, is more code for a hypothetical field. Noted as a footgun rather than fixed.


return $data;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<?php

namespace SeQura\Core\BusinessLogic\Domain\ExpressCheckout\Exceptions;

use SeQura\Core\BusinessLogic\Domain\Translations\Model\BaseTranslatableException;
use SeQura\Core\BusinessLogic\Domain\Translations\Model\TranslatableLabel;
use Throwable;

/**
* Class InvalidExpressCheckoutButtonStyleException.
*
* @package SeQura\Core\BusinessLogic\Domain\ExpressCheckout\Exceptions
*/
class InvalidExpressCheckoutButtonStyleException extends BaseTranslatableException
{
/**
* @var int
*/
protected $code = 400;

/**
* @param ?Throwable $previous
*/
public function __construct(?Throwable $previous = null)
{
parent::__construct(new TranslatableLabel(
'Invalid express checkout button style.',
'general.errors.expressCheckout.invalidButtonStyle'
), $previous);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace SeQura\Core\BusinessLogic\Domain\ExpressCheckout\Models;

use SeQura\Core\BusinessLogic\Domain\ExpressCheckout\Exceptions\DuplicatedExpressCheckoutPageException;
use SeQura\Core\BusinessLogic\Domain\ExpressCheckout\Exceptions\InvalidExpressCheckoutButtonStyleException;
use SeQura\Core\BusinessLogic\Domain\ExpressCheckout\Exceptions\InvalidExpressCheckoutPageConfigException;

/**
Expand All @@ -14,21 +15,40 @@
*/
class ExpressCheckoutSettings
{
/**
* The style is an opaque blob: the keys it may carry are owned by the
* merchant portal and the express checkout button page, so that neither
* needs a release of this library to grow. Only its shape is checked here.
* The size ceiling is what the button page's URL query parameter can carry
* (integration-assets drops anything larger), and it also keeps an oversized
* style from truncating the serialized column it shares with the page
* configs.
*/
private const BUTTON_STYLE_MAX_BYTES = 1536;

/**
* @var ExpressCheckoutPageConfig[]
*/
protected $expressCheckoutConfigs;

/**
* @var string|null
*/
protected $buttonStyle;

/**
* @param ExpressCheckoutPageConfig[] $expressCheckoutConfigs
* @param string|null $buttonStyle
*
* @throws InvalidExpressCheckoutPageConfigException When an entry is not an ExpressCheckoutPageConfig.
* @throws DuplicatedExpressCheckoutPageException When two entries reference the same page.
* @throws InvalidExpressCheckoutButtonStyleException When the style is not a JSON object within the size limit.
*/
public function __construct(array $expressCheckoutConfigs = [])
public function __construct(array $expressCheckoutConfigs = [], ?string $buttonStyle = null)
Comment thread
mescalantea marked this conversation as resolved.
{
$this->validateConfigs($expressCheckoutConfigs);
$this->expressCheckoutConfigs = array_values($expressCheckoutConfigs);
$this->buttonStyle = $this->normalizeButtonStyle($buttonStyle);
}

/**
Expand All @@ -39,6 +59,14 @@ public function getExpressCheckoutConfigs(): array
return $this->expressCheckoutConfigs;
}

/**
* @return string|null
*/
public function getButtonStyle(): ?string
{
return $this->buttonStyle;
}

/**
* @param string $page Page identifier string (see ExpressCheckoutPage factories).
*
Expand All @@ -64,6 +92,7 @@ public function toArray(): array
'expressCheckoutConfigs' => array_map(static function (ExpressCheckoutPageConfig $config) {
return $config->toArray();
}, $this->expressCheckoutConfigs),
'buttonStyle' => $this->buttonStyle,
];
}

Expand Down Expand Up @@ -95,4 +124,28 @@ private function validateConfigs(array $expressCheckoutConfigs): void
$seenPages[] = $page;
}
}

/**
* Treats an empty style as unset and asserts that anything else is a JSON
* object within the size limit. A decoded scalar or list is rejected: the
* button page reads named properties off an object.
*
* @param string|null $buttonStyle
*
* @return string|null
*
* @throws InvalidExpressCheckoutButtonStyleException
*/
private function normalizeButtonStyle(?string $buttonStyle): ?string
{
if ($buttonStyle === null || $buttonStyle === '') {
return null;
}

if (\strlen($buttonStyle) > self::BUTTON_STYLE_MAX_BYTES || !\is_object(json_decode($buttonStyle))) {
throw new InvalidExpressCheckoutButtonStyleException();
}

return $buttonStyle;
}
}
Loading