Automattic / woocommerce-payments

Accept payments via credit card. Manage transactions within WordPress.
https://wordpress.org/plugins/woocommerce-payments/
Other
173 stars 69 forks source link

Remove `'-` sign in fees column for plugin-generated transaction CSV files (plugin and service CSVs are inconsistent) #9303

Open wyter opened 1 month ago

wyter commented 1 month ago

Description

[!IMPORTANT]
This issue occurs upstream in the @woocommerce/csv-export package – I've reopened the related issue in the WC repo: https://github.com/woocommerce/woocommerce/issues/25379 I've marked this issue as blocked while we resolve the issue upstream.

We received this in one of our support requests:

When the report download is emailed to me, it is great (see screenshot attached called 'fine'). However, when the report downloads immediately, which is most of the time, there are the symbols '- in front of every figure in the 'fees' column (see screenshot titled 'not fine'). This is very frustrating, as Excel is not able to recognise the figures and automatically create a total for me underneath, so I have to manually remove the '- symbols from every single row of the fees column.

Here's the "fine" screenshot from emailed reports, doesn't have '- symbols:

fine, has no symbols

Here's the "not fine" screenshot from automatically downloaded transaction reports, has '- symbols:

has symbols, not fine

Request: Since the emailed transaction CSV files do not have the symbols '-, could we consider removing these symbols in the automatically-downloaded transaction reports?

Acceptance criteria

Removal of the symbols '- before transaction fees in automatically downloaded transaction CSV files.

Designs

The fees column in the automatically-downloaded transaction reports should look like this: fine, has no symbols

Testing instructions

  1. Go to WP-Admin → Payments → Transactions on a WooPayments account that doesn't have many transactions (you can use the advanced filters in the "show" dropdown if the account has many transactions to reduce the number of transactions shown to ~20 or so)
  2. Click on the download button to automatically download the WooPayments transactions download transactions
  3. Open the downloaded CSV file, check the Fees column and check if the fees has the symbols '-. Ideally, these shouldn't show if this improvement is made.

Additional context

Reported on 8635657-zen

Jinksi commented 1 week ago

🤔 It looks like the ' prepended to negative values is not a bug, it is expected as part of the escaping process from @woocommerce/csv-export to prevent a CSV injection exploit:

https://github.com/woocommerce/woocommerce/issues/25379

https://github.com/woocommerce/woocommerce/blob/dbb8ab8d1e875528da6c81469df7855792a85e5f/includes/export/abstract-wc-csv-exporter.php#L355-L378

To remediate it, ensure that no cells begin with any of the following characters: Equals to (=) Plus (+) Minus (-) At (@) Tab (0x09) Carriage return (0x0D)

Forcing all values to a string before @woocommerce/csv-export generateCSVDataFromTable() does not bypass this escaping process.

Note, this is only a problem for untrusted user input, which should not be a problem here since WooPayments is generating the values after fetching from the /transactions REST endpoint.

Jinksi commented 1 week ago

I've confirmed this issue also exists in WC Analytics and negative values.

An example of a WC Analytics → Revenue exported CSV opened with Apple Numbers:

image

revenue_2024-09-26_page-wc-admin_period-custom_compare-previous-year_chart-net-revenue_path--analytics-revenue_per-page-100_orderby-net-revenue_interval-week_order-asc_paged-1_after-2023-06-01_before-2023-06-30.csv

Jinksi commented 1 week ago

I've re-opened the upstream @woocommerce/csv-export issue, so we can move the discussion there: https://github.com/woocommerce/woocommerce/issues/25379.

I'll mark this as blocked until we can resolve upstream by potentially bypassing the escaping process if values are from a trusted source.

Jinksi commented 1 week ago

This is related to a prior WooPayments issue #7307, where tab chars were previously prepended rather than '. A subsequent PR to @woocommerce/csv-export changed the tab char to a ' char, as recommended by OWASP.