Conversation
|
This PR exceeds the recommended size of 400 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
|
This pull request does not contain a valid label. Please add one of the following labels: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| validTransfer.ifPresent( | ||
| elem -> { | ||
| elem.setAmount(elem.getAmount() - oldNotificationFee); | ||
| elem.setAmount(elem.getAmount() + notificationFeeAmount); |
Check failure
Code scanning / CodeQL
User-controlled data in arithmetic expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix this issue, we need to ensure that any arithmetic operation involving user-controlled data (here, notificationFeeAmount) does not result in an overflow or underflow. The best way is to validate the input before performing the arithmetic, ensuring that the result of the addition or subtraction will not exceed the bounds of the long type. We should also ensure that the notification fee is not negative, as that would likely be invalid in the business context.
Specifically, in updateAmountsWithNotificationFee, before performing:
paymentOption.setAmount(paymentOption.getAmount() + notificationFeeAmount)elem.setAmount(elem.getAmount() + notificationFeeAmount)
We should check that the sum will not exceed Long.MAX_VALUE and will not be less than Long.MIN_VALUE. Similarly, for subtraction, we should check for underflow. If the check fails, we should throw an exception (e.g., AppException) indicating invalid input.
We will add a helper method to perform safe addition and subtraction with overflow checks, and use it in place of the direct arithmetic. We will also validate that notificationFeeAmount is non-negative.
All changes are within src/main/java/it/gov/pagopa/debtposition/service/payments/PaymentsService.java.
| @@ -35,6 +35,32 @@ | ||
|
|
||
| @Service | ||
| @Slf4j | ||
|
|
||
| /** | ||
| * Safely adds two long values, throwing an AppException if overflow would occur. | ||
| */ | ||
| private static long safeAdd(long a, long b, String organizationFiscalCode) { | ||
| if (b > 0L && a > Long.MAX_VALUE - b) { | ||
| throw new AppException(AppError.PAYMENT_OPTION_NOTIFICATION_FEE_UPDATE_FAILED, organizationFiscalCode, "Addition would cause overflow"); | ||
| } | ||
| if (b < 0L && a < Long.MIN_VALUE - b) { | ||
| throw new AppException(AppError.PAYMENT_OPTION_NOTIFICATION_FEE_UPDATE_FAILED, organizationFiscalCode, "Addition would cause underflow"); | ||
| } | ||
| return a + b; | ||
| } | ||
|
|
||
| /** | ||
| * Safely subtracts two long values, throwing an AppException if overflow would occur. | ||
| */ | ||
| private static long safeSubtract(long a, long b, String organizationFiscalCode) { | ||
| if (b > 0L && a < Long.MIN_VALUE + b) { | ||
| throw new AppException(AppError.PAYMENT_OPTION_NOTIFICATION_FEE_UPDATE_FAILED, organizationFiscalCode, "Subtraction would cause underflow"); | ||
| } | ||
| if (b < 0L && a > Long.MAX_VALUE + b) { | ||
| throw new AppException(AppError.PAYMENT_OPTION_NOTIFICATION_FEE_UPDATE_FAILED, organizationFiscalCode, "Subtraction would cause overflow"); | ||
| } | ||
| return a - b; | ||
| } | ||
| public class PaymentsService { | ||
|
|
||
| private final PaymentOptionRepository paymentOptionRepository; | ||
| @@ -255,6 +281,10 @@ | ||
|
|
||
| public static void updateAmountsWithNotificationFee( | ||
| PaymentOption paymentOption, String organizationFiscalCode, long notificationFeeAmount) { | ||
| // Validate notificationFeeAmount is non-negative | ||
| if (notificationFeeAmount < 0) { | ||
| throw new AppException(AppError.PAYMENT_OPTION_NOTIFICATION_FEE_UPDATE_FAILED, organizationFiscalCode, "Notification fee cannot be negative"); | ||
| } | ||
| // Get the first valid transfer to add the fee | ||
| Optional<Transfer> validTransfer = findPrimaryTransfer(paymentOption, organizationFiscalCode); | ||
|
|
||
| @@ -267,14 +297,14 @@ | ||
| // Setting the new value of the notification fee, updating the amount of the payment option and | ||
| // the last updated date fee | ||
| paymentOption.setNotificationFee(notificationFeeAmount); | ||
| paymentOption.setAmount(paymentOption.getAmount() - oldNotificationFee); | ||
| paymentOption.setAmount(paymentOption.getAmount() + notificationFeeAmount); | ||
| paymentOption.setAmount(safeSubtract(paymentOption.getAmount(), oldNotificationFee, organizationFiscalCode)); | ||
| paymentOption.setAmount(safeAdd(paymentOption.getAmount(), notificationFeeAmount, organizationFiscalCode)); | ||
|
|
||
| // Subtracting the old value and adding the new one | ||
| validTransfer.ifPresent( | ||
| elem -> { | ||
| elem.setAmount(elem.getAmount() - oldNotificationFee); | ||
| elem.setAmount(elem.getAmount() + notificationFeeAmount); | ||
| elem.setAmount(safeSubtract(elem.getAmount(), oldNotificationFee, organizationFiscalCode)); | ||
| elem.setAmount(safeAdd(elem.getAmount(), notificationFeeAmount, organizationFiscalCode)); | ||
| }); | ||
| } | ||
|
|
|
2 participants
List of Changes
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: