Skip to content

BUGFIX: redirect customer to cart instead of order history after Saferpay transaction abort#327

Merged
TLabutis merged 2 commits into
SL-346/accessibility-eaa-compliancefrom
BUGFIX/abort-redirect-secure-key
May 15, 2026
Merged

BUGFIX: redirect customer to cart instead of order history after Saferpay transaction abort#327
TLabutis merged 2 commits into
SL-346/accessibility-eaa-compliancefrom
BUGFIX/abort-redirect-secure-key

Conversation

@TLabutis

Copy link
Copy Markdown
Contributor

Summary

  • After aborting a Saferpay transaction (e.g. Klarna), customer was redirected to /order-history instead of back to the cart page.
  • Root cause: getRedirectionToControllerUrl() in controllers/front/return.php used $this->context->cart->secure_key, which can differ from the original cart's secure key when the context is rebuilt after the external redirect. The secure_key mismatch caused fail.php to redirect to history.
  • Fix: load the cart from the cartId query parameter and use that cart's secure_key. Falls back to context cart only if the cart cannot be loaded.

Changes

  • controllers/front/return.php: prefer cartId from URL and load original cart's secure_key.
  • changelog.md: entry under [2.0.2].

Test plan

  • Initiate a Klarna payment, abort it on the Saferpay side — customer lands on cart page with "We couldn't authorize your payment" warning (not order history).
  • Verify successful payment flow still redirects to order confirmation.
  • Verify failed payment flow still redirects to cart with warning.

@TLabutis TLabutis changed the base branch from master to SL-346/accessibility-eaa-compliance May 15, 2026 10:57

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request addresses a redirection issue where customers were incorrectly sent to their order history instead of the cart after aborting a Saferpay transaction. The changes in controllers/front/return.php modify how the cart ID and secure key are retrieved for redirection URLs. However, the current implementation introduces a security vulnerability by leaking the secure_key based on a user-provided cartId. It is recommended to use Tools::getValue('secureKey') to prevent this information disclosure and avoid an unnecessary database query.

Comment on lines +277 to +279
$cartId = (int) Tools::getValue('cartId') ?: (int) $this->context->cart->id;
$cart = new Cart($cartId);
$secureKey = Validate::isLoadedObject($cart) ? $cart->secure_key : $this->context->cart->secure_key;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

security-high high

This implementation introduces a security vulnerability (Information Disclosure). By fetching the secure_key from the database based solely on the user-provided cartId and including it in the redirection URL, the controller effectively leaks the correct secure_key to any user who knows a valid cartId. An attacker could exploit this to obtain the secure key for any cart and potentially view its contents or status.

Additionally, calling new Cart($cartId) here is inefficient as it triggers an unnecessary database query. Since the secureKey should be present in the request when returning from Saferpay, you should prefer Tools::getValue('secureKey'). This fixes the reported issue of the context being rebuilt (where context->cart->secure_key might be wrong) without compromising security.

        $cartId = (int) Tools::getValue('cartId') ?: (int) $this->context->cart->id;
        $secureKey = Tools::getValue('secureKey') ?: $this->context->cart->secure_key;

@TLabutis TLabutis merged commit 1319947 into SL-346/accessibility-eaa-compliance May 15, 2026
0 of 2 checks passed
@TLabutis TLabutis deleted the BUGFIX/abort-redirect-secure-key branch May 15, 2026 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant