Skip to content

Validate Square payment arguments - #625

Merged
ralyodio merged 1 commit into
profullstack:masterfrom
rissrice2105-agent:codex/square-payment-validation
Jun 6, 2026
Merged

Validate Square payment arguments#625
ralyodio merged 1 commit into
profullstack:masterfrom
rissrice2105-agent:codex/square-payment-validation

Conversation

@rissrice2105-agent

Copy link
Copy Markdown
Contributor

Fixes #624.

Changes:

  • validate Square create amount as a positive integer
  • normalize and validate three-letter currency codes
  • require sourceId for create commands
  • require paymentId for get, cancel, and refund commands
  • return contract-compliant BuildResult and ShipResult shapes
  • add unit tests proving invalid configs do not call Square

Validation:

  • vitest run packages/targets/payment-square/src/index.test.ts
  • tsc -p packages/targets/payment-square/tsconfig.json --noEmit

@greptile-apps

greptile-apps Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds input validation to the Square payment adapter: amounts are checked to be positive integers, currency codes are normalized and validated, and sourceId/paymentId are required before any network call is made. Return shapes are also updated to match the contract's BuildResult/ShipResult types.

  • requireCurrency silently defaults to 'USD' when the value is not a string (e.g. undefined or a mistyped number), unlike the other validators which always throw — a caller that omits or misspells currency will unknowingly charge in USD.
  • The refund case passes config.args?.amount (a bare number) as amount_money, but Square expects { amount, currency }; any partial-refund request will be rejected by the API, and no test covers this path.

Confidence Score: 3/5

Not safe to merge without fixing the refund amount_money shape and the silent currency default.

The refund path sends amount_money as a bare integer instead of the { amount, currency } object Square requires, meaning every partial refund will fail at the API level — and no test exercises this path to catch it. The requireCurrency function also silently accepts non-string inputs by substituting 'USD', which could cause a caller that omits currency to charge in the wrong denomination without any error signal.

Both changed files warrant attention: index.ts for the two logic defects, and index.test.ts because it lacks a partial-refund happy-path test that would have caught the amount_money issue.

Important Files Changed

Filename Overview
packages/targets/payment-square/src/index.ts Adds input validation helpers and contract-compliant return shapes; requireCurrency has a silent-default bug, and the refund path passes amount_money as a bare number instead of the required { amount, currency } object.
packages/targets/payment-square/src/index.test.ts Replaces the stub-only test suite with meaningful validation and happy-path tests; no test covers the partial-refund path, so the amount_money shape bug goes undetected.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[build called] --> B{SQUARE_ACCESS_TOKEN set?}
    B -- No --> C[throw Error]
    B -- Yes --> D{command}
    D -- create --> E[requirePositiveInteger amount]
    E --> F[requireCurrency currency]
    F --> G[requireText sourceId]
    G --> H[POST /v2/payments]
    H --> I[return artifact: square-payment-create]
    D -- get --> J[requireText paymentId]
    J --> K[GET /v2/payments/:id]
    K --> L[return artifact: square-payment-get]
    D -- cancel --> M[requireText paymentId]
    M --> N[POST /v2/payments/:id/cancel]
    N --> O[return artifact: square-payment-cancel]
    D -- list --> P[GET /v2/payments]
    P --> Q[return artifact: square-payment-list]
    D -- refund --> R[requireText paymentId]
    R --> S["POST /v2/refunds\namount_money: args.amount ⚠️ bare number"]
    S --> T[return artifact: square-payment-refund]
    D -- other --> U[throw Unknown command]
    style S fill:#ffcccc,stroke:#cc0000
Loading

Reviews (1): Last reviewed commit: "Validate Square payment arguments" | Re-trigger Greptile

Comment on lines +25 to +29
function requireCurrency(value: unknown): string {
const currency = typeof value === 'string' ? value.trim().toUpperCase() : 'USD';
if (!/^[A-Z]{3}$/.test(currency)) throw new Error('currency must be a three-letter ISO code');
return currency;
}

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.

P1 requireCurrency silently defaults instead of rejecting

When value is not a string — e.g. undefined, null, or a number — the function substitutes 'USD' and passes the ^[A-Z]{3}$ check without throwing. Every other require* helper in this file throws on an invalid/missing input, so callers expecting a validation error when they forget to supply currency (or pass the wrong type) will silently receive a USD charge instead. The inconsistency also means the function name "require" is misleading; a missing currency is accepted rather than rejected.

Comment on lines 97 to 105
@@ -85,7 +103,7 @@ export default defineTarget<Config>({
reason: (config.args?.reason as string) || 'requested_by_customer',
}),
});

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.

P1 Refund amount_money sends a bare number instead of a { amount, currency } object

Square's refund endpoint expects amount_money to be an object shaped { amount: number, currency: string }. The current code passes config.args?.amount directly — a bare integer like 1500 — so any partial-refund request will fail with a Square API validation error. When amount is omitted the field becomes undefined, which JSON.stringify drops entirely (triggering a full refund), so the bug surfaces only for partial refunds. Currency is also never threaded through to the refund body.

Suggested change
const data = await sq('/refunds', {
method: 'POST',
body: JSON.stringify({
idempotency_key: `sh1pt-${Date.now()}`,
payment_id: id,
...(config.args?.amount !== undefined && {
amount_money: {
amount: requirePositiveInteger(config.args.amount, 'amount'),
currency: requireCurrency(config.args?.currency),
},
}),
reason: (config.args?.reason as string) || 'requested_by_customer',
}),
});

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: git fetch upstream master && git rebase upstream/master.

10 similar comments
@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: git fetch upstream master && git rebase upstream/master.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: git fetch upstream master && git rebase upstream/master.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: git fetch upstream master && git rebase upstream/master.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: git fetch upstream master && git rebase upstream/master.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: git fetch upstream master && git rebase upstream/master.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: git fetch upstream master && git rebase upstream/master.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: git fetch upstream master && git rebase upstream/master.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: git fetch upstream master && git rebase upstream/master.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: git fetch upstream master && git rebase upstream/master.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: git fetch upstream master && git rebase upstream/master.

@ralyodio
ralyodio merged commit 2bd4222 into profullstack:master Jun 6, 2026
5 checks passed
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.

payment-square accepts invalid payment arguments

2 participants