-
Notifications
You must be signed in to change notification settings - Fork 4
Da/sdk 255 #884
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Da/sdk 255 #884
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a currency conversion bug in the employee payment method component where split amounts need to be stored in cents (not dollars) when communicating with the API.
Key Changes:
- Added utility functions to convert between dollars and cents
- Updated payment method data flow to convert split amounts when
splitByis set to "Amount" - Fixed display formatting to show correct values in the UI
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/components/Employee/PaymentMethod/PaymentMethod.tsx | Added conversion utilities and applied cent/dollar conversions when loading and saving amount-based split values |
| src/components/Employee/PaymentMethod/BankAccountsList.tsx | Fixed display formatting to convert cents to dollars for amount-based splits |
| src/components/Payroll/ConfirmWireDetails/ConfirmWireDetails.tsx | Removed unnecessary eslint-disable comment |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
This pull request introduces a potential currency rounding precision risk: using Math.round(dollars * 100) in dollarsToCents together with z.number() validation (which doesn’t restrict decimal places) can cause off-by-one-cent errors for certain floating-point inputs, risking financial integrity. The issue is in src/components/Employee/PaymentMethod/PaymentMethod.tsx (lines ~186–202) and should be fixed by using a safe decimal or integer-based approach or stricter input validation.
Currency Rounding Precision Risk in
|
| Vulnerability | Currency Rounding Precision Risk |
|---|---|
| Description | The implementation of dollarsToCents uses Math.round(dollars * 100), which relies on standard JavaScript floating-point arithmetic. The form validation for splitAmount uses z.number(), which does not restrict the number of decimal places. This combination creates a high risk of rounding errors (off-by-one cent) for certain inputs (e.g., a number that should round up but is represented internally as a value slightly less than X.5 after multiplication by 100). This violates the integrity required for financial transactions. |
embedded-react-sdk/src/components/Employee/PaymentMethod/PaymentMethod.tsx
Lines 186 to 202 in 608d786
| : (paymentMethod.splitBy ?? SPLIT_BY.percentage), | |
| splits: | |
| payload.isSplit && paymentMethod.splits | |
| ? paymentMethod.splits.map(split => { | |
| const splitAmountValue = payload.splitAmount[split.uuid] ?? null | |
| const isAmountSplit = payload.splitBy === SPLIT_BY.amount | |
| return { | |
| ...split, | |
| splitAmount: isAmountSplit | |
| ? dollarsToCents(splitAmountValue) | |
| : splitAmountValue, | |
| priority: payload.priority[split.uuid], | |
| } | |
| }) | |
| : (paymentMethod.splits ?? []), | |
| } | |
| const paymentMethodResponse = await paymentMethodMutation.mutateAsync({ |
All finding details can be found in the DryRun Security Dashboard.
Fixing employee payment method component - API expects split amount (fixed) in cents, not dollars.
Screen.Recording.2025-12-18.at.3.21.45.PM.mov