Fix zero amount sent to SaferPay API for products with dynamic pricing#312
Fix zero amount sent to SaferPay API for products with dynamic pricing#312TLabutis wants to merge 1 commit into
Conversation
Pre-compute cart total before order creation to prevent third-party modules from affecting the amount sent to SaferPay Initialize API. When order creation rule is "Before authorization", validateOrder triggers hooks that can clean up dynamic pricing data, causing subsequent cart total reads to return 0.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical issue where products with dynamic pricing, such as gift cards, would incorrectly send a zero amount to the SaferPay API. This occurred when the "Order creation rule" was set to "Before authorization," as third-party modules could inadvertently clear dynamic pricing data during order validation. The fix ensures that the correct cart total is calculated and preserved early in the checkout process, preventing subsequent modifications from affecting the payment initialization. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a critical bug where a zero amount could be sent to the SaferPay API for products with dynamic pricing, by ensuring the cart total is correctly pre-computed early in the checkout process. However, a security audit identified two critical vulnerabilities: Broken Access Control (IDOR) and logic flaws. These issues stem from inconsistent cart data sources, allowing price manipulation by using a user-supplied cart ID for amount calculation while the session context cart is used for order details. Additionally, the use of saved card aliases lacks proper ownership verification, potentially enabling unauthorized use of other users' payment methods. These security concerns must be addressed by implementing strict ownership checks and ensuring data consistency across the checkout flow.
| @@ -109,7 +111,8 @@ public function run(CheckoutData $data) | |||
| $data->getSelectedCard(), | |||
There was a problem hiding this comment.
The selectedCard parameter, which originates from user-supplied data in the CheckoutData object, is passed to the payment initialization service on line 111. This ID is used to retrieve a saved card alias from the database without any ownership verification in the downstream SaferPayCardAliasRepository::getSavedCardAliasFromId method. An attacker could potentially use another user's saved card by providing the corresponding id_saferpay_card_alias in their request.
To remediate this, verify that the selectedCard ID belongs to the currently authenticated customer before using it. You can use the SaferPayCardAliasRepository::getCustomerIdByReferenceId method for this purpose.
| $totalPrice = $cartTotal; | ||
| if ($cartTotal === null) { | ||
| $cartDetails = $cart->getSummaryDetails(); | ||
| $totalPrice = (int) round($cartDetails['total_price'] * SaferPayConfig::AMOUNT_MULTIPLIER_FOR_API); | ||
| } |
There was a problem hiding this comment.
For improved readability and to avoid assigning a value to $totalPrice that might be immediately overwritten, you can restructure this conditional logic.
if ($cartTotal !== null) {
$totalPrice = $cartTotal;
} else {
$cartDetails = $cart->getSummaryDetails();
$totalPrice = (int) round($cartDetails['total_price'] * SaferPayConfig::AMOUNT_MULTIPLIER_FOR_API);
}1 participant
Summary
CheckoutProcessor::run()beforeprocessCreateOrder()to prevent third-party modules from affecting the amount sent to SaferPay Initialize APIvalidateOrder()firesactionValidateOrderhook which allows third-party modules (e.g. Webkul Gift Card) to clean up dynamic pricing data, causing subsequent cart total reads to return 0processInitializePayment()→buildRequest()→InitializeRequestObjectCreator::create()with backward-compatible optional parameterTest plan