Security fix: Introduce file type check for tax rate importer

[timbocode]

Backport for woocommerce/woocommerce@737f6af

See also #214

Files affected:
includes/admin/importers/class-wc-product-csv-importer-controller.php
includes/admin/importers/class-wc-tax-rate-importer.php
includes/wc-conditional-functions.php

Files not included:

tests/unit-tests/util/conditional-functions.php

[codecov-io]

Codecov Report

Merging #222 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master     #222      +/-   ##
============================================
- Coverage     40.83%   40.81%   -0.02%     
- Complexity    13447    13449       +2     
============================================
  Files           367      367              
  Lines         51275    51292      +17     
============================================
- Hits          20938    20936       -2     
- Misses        30337    30356      +19

Impacted Files	Coverage Δ	Complexity Δ
...rters/class-wc-product-csv-importer-controller.php	12.69% <ø> (ø)	69 <0> (ø)	⬇️
includes/wc-conditional-functions.php	7.86% <0%> (-1%)	0 <0> (ø)
...des/admin/importers/class-wc-tax-rate-importer.php	40.7% <0%> (-1.89%)	34 <0> (+2)
includes/class-wc-tax.php	79.4% <0%> (-0.55%)	130% <0%> (ø)
includes/class-wc-update-client.php	0% <0%> (ø)	64% <0%> (ø)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6180699...41ba9c0. Read the comment docs.

[bahiirwa]

This change 3c35038 doesn’t belong here. Please remove it, it’s already handled in another PR #220

EDIT - this was addressed in a later commit.

[timbocode]

Testing

• Manually tested the CSV import using a badly formatted CSV file. It successfully rejected the file.

[bahiirwa]

On manual test of this file. I tried to add a .jpeg file and received the message below.

Upload jpeg

Message below.

Message expected: "Invalid file type. The importer supports CSV and TXT file formats."

The change itself has no problem the expected changes for the user reflect otherwise in messaging.

Uploading a CSV and txt allows for full upload as expected.

We might have to re-fix this as CC.

And also fix this issue in classic-commerce/includes/admin/importers/class-wc-product-csv-importer-controller.phpon line 89 where

/**
 * Check whether a file is a valid CSV file.
 *
 * @todo Replace this method with wc_is_file_valid_csv() function.
 * @param string $file File path.
 * @param bool   $check_path Whether to also check the file is located in a valid location (Default: true).
 * @return bool
 */
public static function is_file_valid_csv( $file, $check_path = true ) {

If this change is adopted, we need to introduce the new file checker for csv and txt from includes/wc-conditional-functions.php in to classic-commerce/includes/admin/importers/class-wc-product-csv-importer-controller.php

[nylen]

1. Why was tests/unit-tests/util/conditional-functions.php excluded from this change?

2. I see the same behavior as @bahiirwa (the message is not the expected one), but otherwise the diff looks the same as woocommerce/woocommerce@737f6af. It's probably a good idea to test the same action using WooCommerce. If it behaves the same way then we should merge this and investigate separately.

[timbocode]

1. Yes, sorry. I meant to leave a note about that. The reason I left the test file out is because the new test appears to be looking for assets on a local c: drive and on other servers I'm not sure we have access to:

public function test_wc_is_file_valid_csv() {
  $this->assertTrue( wc_is_file_valid_csv( 'C:/wamp64/www/test.local/wp-content/uploads/2018/10/products_all_gg-1.csv' ) );
  $this->assertTrue( wc_is_file_valid_csv( '/srv/www/woodev/wp-content/uploads/2018/10/1098488_single.csv' ) );
  $this->assertFalse( wc_is_file_valid_csv( '/srv/www/woodev/wp-content/uploads/2018/10/img.jpg' ) );
  $this->assertFalse( wc_is_file_valid_csv( 'file:///srv/www/woodev/wp-content/uploads/2018/10/1098488_single.csv' ) );
}

2. I did compare the operation of CC with WC 3.9.2 at the time, with a variety of file types (not just malformed CSV). WC gives the same message:

[bahiirwa]

I agree on the decision @timbocode took here. However, there a sample files in the repo that can be tested on the fly. This could be its on implementation.

[bahiirwa]

Outside this #222 (comment) which we should consider seperately as well as add better message for the invalid file types

Manual tests passed:

• Add taxes manually ne by one
• Upload a csv and txt file with tax rates passes adding new tax rates.