Feat/webhooks for disputes revisited

[alexanmtz]

This pull request introduces enhancements to the handling of payment request disputes, particularly around dispute closure events, and refactors related code for improved maintainability and type safety. The main changes include adding new email notifications for dispute closures, updating webhook handlers to support additional Stripe events, and refactoring the mail module to TypeScript.

Dispute Handling and Email Notifications:

• Added new localized email templates and strings for notifying users when a dispute for a payment request is closed, including status-specific details for "won" and "lost" outcomes.
• Implemented the newDisputeClosedForPaymentRequest email method in paymentRequest.ts, which sends users a detailed summary when a dispute is closed, including status, reason, and customer information.

Webhook and Event Handling:

• Updated the webhook controller to support the new charge.dispute.funds_withdrawn Stripe event and refactored dispute-related webhook handler imports for better organization. [1] [2]
• Removed the old chargeDisputeClosedWebhookHandler implementation file, as dispute closure handling is now centralized and improved.

Codebase Refactoring and Type Safety:

• Migrated paymentRequest.js to TypeScript (paymentRequest.ts), adding type annotations for function parameters and improving currency handling with a new resolveCurrencyKey helper. [1] [2] [3] [4] [5]
• Improved the calculation and display of dispute-related fees and net impact in email templates, including platform fees and more robust handling of monetary values. [1] [2] [3]

Template and Utility Updates:

• Standardized usage of the tableContentEmailTemplate and improved localization and formatting for all relevant user notification emails. [1] [2] [3] [4]

These changes collectively improve user communication around payment disputes and modernize the codebase for better maintainability and reliability.

In general, to fix externally-controlled format string issues in Node.js logging, ensure that untrusted data is never part of the format string argument. Instead, pass a constant, literal format string (or use string concatenation to pre-build a single string) and supply untrusted values as additional arguments or as already-rendered data. This prevents % sequences in user data from being interpreted by util.format.

For this specific case, the vulnerable line is:

console.error(`Error handling charge.dispute.closed for Dispute ID: ${id}`, error)

Here, the interpolated template literal becomes the first argument to console.error, i.e., the format string. To fix it without changing observable behavior, we can either:

1. Move id into a separate argument and use a constant format string, e.g.:

   console.error('Error handling charge.dispute.closed for Dispute ID: %s', id, error)

   This uses %s explicitly for the untrusted value, which is exactly the CodeQL recommendation; or

2. Pre-concatenate into a single safe string (which is then a constant from util.format’s perspective):

   console.error('Error handling charge.dispute.closed for Dispute ID: ' + id, error)

Option 1 is the clearest alignment with the recommendation and keeps essentially the same log structure. The only change needed is to update that single console.error call in src/modules/webhooks/charges/chargeDisputeClosed/chargeDisputeClosed.ts. No new imports or helper methods are required.

---

General approach: Avoid passing user-controlled data as part of the format string (first argument) to console.error/console.log. Instead, keep the format string constant and pass untrusted values as separate arguments (using %s if you want explicit formatting), or pre-format the message with string concatenation/template literals and pass it as a non-format-string by ensuring no additional arguments are supplied that would trigger formatting.

Best fix here: Change the console.error call so that the first argument is a constant string and the dynamic dispute ID is passed as a separate argument. This preserves all existing behavior (message content and error logging) while eliminating the format-string risk. Concretely, replace:

console.error(`Error handling charge.dispute.created for Dispute ID: ${data.object.id}`, error)

with something like:

console.error('Error handling charge.dispute.created for Dispute ID: %s', data.object.id, error)

This keeps the structure of the log (message, then ID, then error) and ensures that any % sequences in data.object.id are treated as a string argument, not as format specifiers.

Changes needed:

• File: src/modules/webhooks/charges/chargeDisputeCreated/chargeDisputeCreated.ts
  • On line 27, adjust the console.error invocation as above.
• No new imports or helper methods are needed.
• No changes are needed in src/app/controllers/webhooks/webhook-platform.ts.

---

In general, to fix an externally-controlled format string with console.log / console.error, ensure that the untrusted data is not part of the format string itself. Instead, use a constant format string and pass untrusted values as additional arguments (or pre-concatenate them into a single string that contains no % characters you care about as format specifiers).

The best minimal change here is to:

• Remove the template-literal interpolation of id in the first argument to console.error.
• Replace it with a constant message string and use %s (or simply a separate argument) to include id.
• This keeps the same information in the logs but ensures the format string is not influenced by untrusted input.

Concretely, in src/modules/webhooks/charges/chargeDisputeFundsWithdrawn/chargeDisputeFundsWithdrawn.ts, change:

console.error(`Error handling charge.dispute.funds_withdrawn for Dispute ID: ${id}`, error)

to something like:

console.error('Error handling charge.dispute.funds_withdrawn for Dispute ID: %s', id, error)

or, equivalently:

console.error('Error handling charge.dispute.funds_withdrawn for Dispute ID:', id, error)

Both approaches ensure that the first argument is a constant and that any % characters inside id are treated as plain text. No new imports or dependencies are needed, and existing behavior (logging the ID and error) is preserved.

---

General fix approach: Never let tainted / user‑controlled data form the format string (first argument) to console.log/console.error/util.format, etc. Instead, use a constant format string and pass untrusted data as subsequent arguments, or sanitize such data before embedding it into a template literal.

Best fix here without changing functionality: Replace the uses of template literals that inject source_id directly into the first argument position of console.error with calls where the first argument is a static string that uses %s, and source_id is passed as a separate argument. This preserves all existing log information and behavior, but ensures the format string itself is not influenced by untrusted data.

Concretely in src/services/payments/disputes/disputeService.ts:

• In createDisputeForPaymentRequest, change:

console.error(`Failed to send email for Dispute ID: ${source_id}`, mailError)

to:

console.error('Failed to send email for Dispute ID: %s', source_id, mailError)

• In withDrawnDisputeForPaymentRequest, change:

console.error(`Failed to send email for Dispute ID: ${source_id}`, mailError)

to:

console.error('Failed to send email for Dispute ID: %s', source_id, mailError)

This addresses all alert variants, since they point to the same pattern. No new imports or dependencies are needed.

---

General fix approach: Ensure that untrusted data is never used as part of the format string argument to variadic logging/formatting functions (console.log/error, util.format, etc.). Instead, keep the format string static and pass untrusted values as subsequent arguments, typically using %s placeholders if you want interpolation.

Best fix here: On line 105 in src/services/payments/disputes/disputeService.ts, change the console.error call so that the first argument is a constant string with a %s placeholder for the dispute ID, and pass source_id as a separate argument before mailError. This keeps the log message semantics identical while preventing any format specifiers inside source_id from being interpreted. We will also make the same style change for the similar log on line 26 in createDisputeForPaymentRequest (line 26) and the non-error logs that embed source_id and similar tainted values, to address all related variants in one go and keep logging consistent.

Concretely, in disputeService.ts, update:

• Line 26:
  From

  console.error(`Failed to send email for Dispute ID: ${source_id}`, mailError)

  To

  console.error('Failed to send email for Dispute ID: %s', source_id, mailError)

• Line 44:

  console.error(`Payment Request Payment not found for source ID: ${source_id}`)

  →

  console.error('Payment Request Payment not found for source ID: %s', source_id)

• Lines 46–48 (the success log with source_id):

  console.log(
    `Found Payment Request Payment with ID: ${paymentRequestPayment.id} for source ID: ${source_id}`
  )

  →

  console.log(
    'Found Payment Request Payment with ID: %s for source ID: %s',
    paymentRequestPayment.id,
    source_id
  )

• Lines 55 and 57 (user ID and error):

  console.log(`Dispute User ID: ${paymentRequestUser.id}`)
  console.error(`Payment Request User not found for source ID: ${source_id}`)

  →

  console.log('Dispute User ID: %s', paymentRequestUser.id)
  console.error('Payment Request User not found for source ID: %s', source_id)

• Lines 63–64 and 67 (balance logs):

  console.log(
    `Found Payment Request Balance with ID: ${paymentRequestBalance.id} for User ID: ${paymentRequestUser.id}`
  )
  console.error(`Payment Request Balance not found for User ID: ${paymentRequestUser.id}`)

  →

  console.log(
    'Found Payment Request Balance with ID: %s for User ID: %s',
    paymentRequestBalance.id,
    paymentRequestUser.id
  )
  console.error('Payment Request Balance not found for User ID: %s', paymentRequestUser.id)

• Lines 95 and 97 (transaction logs with tainted source_id):

  console.log(`Created PaymentRequestBalanceTransaction for Dispute ID: ${source_id}`)
  console.error(`Failed to create PaymentRequestBalanceTransaction for Dispute ID: ${source_id}`)

  →

  console.log('Created PaymentRequestBalanceTransaction for Dispute ID: %s', source_id)
  console.error(
    'Failed to create PaymentRequestBalanceTransaction for Dispute ID: %s',
    source_id
  )

• Line 105 (the specific flagged one):

  console.error(`Failed to send email for Dispute ID: ${source_id}`, mailError)

  →

  console.error('Failed to send email for Dispute ID: %s', source_id, mailError)

No new imports or dependencies are required; we only change how the logging function is called.

General approach: Avoid using strings that contain untrusted data as the format string to console.error (or other util.format consumers). Instead, use a constant format string with %s placeholders and pass the untrusted value as a separate argument. That way, any % characters in untrusted data are escaped as plain text and cannot alter formatting.

Best fix here: In src/services/payments/disputes/disputeService.ts, the problematic line is

console.error(`Failed to send email for Dispute ID: ${source_id}`, mailError)

Change it so that the first argument is a literal format string, and source_id is passed as a separate argument. This preserves behavior (message + error object), still logs the source_id, and removes the externally controlled format string:

console.error('Failed to send email for Dispute ID: %s', source_id, mailError)

Similarly, to address the same pattern elsewhere in the same file and avoid future alerts, we should adjust the analogous console.error usage in createDisputeForPaymentRequest and in the balance-transaction email catch block. These currently interpolate source_id into template literals that then serve as format strings; we’ll convert those to static format strings with %s. No new imports or dependencies are required, and functionality is unchanged apart from making the formatting behavior robust to malicious % content.