Skip to content

Fix/issue WOOTAX-46 - Prevent unauthenticated downloads of tax rate backup CSV files#2932

Merged
iyut merged 10 commits intotrunkfrom
fix/issue-wootax-46
Apr 6, 2026
Merged

Fix/issue WOOTAX-46 - Prevent unauthenticated downloads of tax rate backup CSV files#2932
iyut merged 10 commits intotrunkfrom
fix/issue-wootax-46

Conversation

@iyut
Copy link
Copy Markdown
Collaborator

@iyut iyut commented Feb 20, 2026

Description

Tax rate backup CSV files were stored directly in the public wp-content/uploads/ directory with predictable filenames (e.g. taxjar-wc_tax_rates-04-04-2025-1743750602.csv). Anyone with the URL could download them without authentication.

This PR prevents unauthorized downloads by:

  1. Storing new backups in a protected subdirectory (wp-content/uploads/taxjar-backups/) with .htaccess (deny from all) and an index.php file to block direct HTTP access.
  2. Serving files through a secure AJAX endpoint (wcs_download_tax_backup) that verifies a nonce and checks for the manage_woocommerce capability before streaming the file.
  3. Automatically migrating existing backups from the public uploads root into the protected directory the first time the status page is loaded.

Changes

  • classes/class-wc-connect-functions.php

    • backup_existing_tax_rates(): Saves CSVs to uploads/taxjar-backups/ and creates protection files on first use.
    • protect_backup_directory() (new): Creates .htaccess and index.php in the backup directory.
    • get_backed_up_tax_rate_files(): Moves any legacy files from the public uploads root into the protected directory, then returns a flat array of filenames.
  • classes/class-wc-connect-help-view.php

    • Registers wp_ajax_wcs_download_tax_backup action.
    • get_form_data(): Passes a nonce-protected AJAX download URL to the frontend.
    • handle_tax_backup_download() (new): AJAX handler that verifies nonce + capability, validates the filename against a strict regex, and streams the file.
  • client/apps/plugin-status/view.js

    • Download links now point to the secure AJAX endpoint instead of direct file URLs.

Related issue(s)

Closes WOOTAX-46

Steps to reproduce & screenshots/GIFs

Before (vulnerable):

  1. Enable automated taxes so a backup CSV is created.
  2. Navigate to WooCommerce > Status > WooCommerce Tax.
  3. Copy the direct download URL of a backup file.
  4. Open that URL in an incognito/logged-out browser window.
  5. The CSV file downloads successfully without authentication.

After (fixed):

  1. Enable automated taxes so a backup CSV is created.
  2. Navigate to WooCommerce > Status > WooCommerce Tax.
  3. Click a backup file link -- it downloads via the secure AJAX endpoint.
  4. Copy that URL and open it in an incognito/logged-out browser window.
  5. The download is blocked (returns -1 / unauthorized).
  6. The old direct URL (/wp-content/uploads/taxjar-wc_tax_rates-*.csv) returns 404 because the file has been moved to the protected directory.

Checklist

  • unit tests
  • changelog.txt entry added
  • readme.txt entry added

@iyut iyut marked this pull request as draft February 20, 2026 08:28
@iyut iyut self-assigned this Feb 20, 2026
@iyut iyut marked this pull request as ready for review February 23, 2026 05:18
@iyut iyut requested review from bartech and dustinparker February 23, 2026 05:18
Copy link
Copy Markdown
Collaborator

@bartech bartech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested Changes

Suggestions to consider for improving the security hardening and robustness of this PR. Each section shows the proposed change as a diff and explains the reasoning.


1. Use woocommerce_uploads/taxes instead of taxjar-backups

Files: class-wc-connect-functions.php (lines 265, 333), class-wc-connect-help-view.php (line 386)

- $backup_dir = $upload_dir['basedir'] . '/taxjar-backups';
+ $backup_dir = $upload_dir['basedir'] . '/woocommerce_uploads/taxes';
- $file_path  = $upload_dir['basedir'] . '/taxjar-backups/' . $filename;
+ $file_path  = $upload_dir['basedir'] . '/woocommerce_uploads/taxes/' . $filename;

Why: WooCommerce already protects wp-content/uploads/woocommerce_uploads/ with its own .htaccess on most installs. Storing tax backups inside this directory means the files inherit that parent-level protection automatically — even if the child .htaccess is somehow deleted, the parent still blocks direct HTTP access. A custom taxjar-backups directory at the uploads root has no such inherited protection.


2. Re-create protection files when .htaccess is missing

File: class-wc-connect-functions.phpbackup_existing_tax_rates()

  if ( ! file_exists( $backup_dir ) ) {
      if ( ! wp_mkdir_p( $backup_dir ) ) {
          $backup_dir = $upload_dir['basedir'];
      } else {
          self::protect_backup_directory( $backup_dir );
      }
+ } elseif ( ! file_exists( $backup_dir . '/.htaccess' ) ) {
+     // Re-create protection files if they were removed.
+     self::protect_backup_directory( $backup_dir );
  }

Why: Currently protect_backup_directory() is only called when the directory is first created. If the directory already exists but its .htaccess was deleted — by an admin cleaning up, a hosting migration, or a prior run that crashed mid-way — the files will remain unprotected indefinitely. This elseif branch detects the missing .htaccess and re-creates all protection files on the next backup.


3. Add index.php alongside index.html in protect_backup_directory()

File: class-wc-connect-functions.phpprotect_backup_directory()

  $files = array(
      array(
          'base'    => $dir,
          'file'    => '.htaccess',
          'content' => 'DirectoryIndex index.php index.html' . PHP_EOL . 'deny from all',
      ),
      array(
          'base'    => $dir,
          'file'    => 'index.html',
          'content' => '',
      ),
+     array(
+         'base'    => $dir,
+         'file'    => 'index.php',
+         'content' => '<?php' . PHP_EOL . '// Silence is golden.',
+     ),
  );

Why: The .htaccess DirectoryIndex directive already references both index.php and index.html, but only index.html is created. On servers that ignore .htaccess (Nginx, or Apache with AllowOverride None), directory listing prevention falls back to whether a default index file exists. Most default web server configs look for index.php first. Adding it matches WooCommerce core's own wc_create_files() pattern and provides an extra layer against directory listing.

Also replaces raw file_put_contents() with fopen()/fwrite()/fclose() using @ error suppression. This is more robust on restricted filesystems — fopen() with @ returns false silently instead of emitting a PHP warning that could break AJAX responses.


4. Obfuscate filenames with wp_hash() suffix

File: class-wc-connect-functions.phpbackup_existing_tax_rates()

- $backed_up = file_put_contents(
-     $backup_dir . '/taxjar-wc_tax_rates-' . date( 'Y-m-d' ) . '-' . time() . '.csv',
-     $csv
- );
+ $base_name   = 'taxjar-wc_tax_rates-' . gmdate( 'Y-m-d' ) . '-' . time();
+ $hash_suffix = wp_hash( $base_name );
+ $backed_up   = file_put_contents(
+     $backup_dir . '/' . $base_name . '-' . $hash_suffix . '.csv',
+     $csv
+ );

Why: Two improvements:

  • date()gmdate(): date() depends on PHP's configured timezone, which can vary between servers and produce inconsistent filenames. gmdate() always uses UTC.
  • wp_hash() suffix: Even inside a protected directory, the filenames are predictable (taxjar-wc_tax_rates-2026-03-03-1709472000.csv). On Nginx — which ignores .htaccess entirely — anyone who guesses the URL can download the file. Appending a 32-character wp_hash() suffix (keyed to the site's salt) makes brute-forcing infeasible. This is the same pattern WooCommerce uses for log files.

5. Hash legacy filenames during migration

File: class-wc-connect-functions.phpget_backed_up_tax_rate_files()

- $dest = $backup_dir . '/' . basename( $old_file );
+ $old_basename = pathinfo( $old_file, PATHINFO_FILENAME );
+
+ // Add wp_hash() suffix if the file doesn't already have one (32-char hex at the end).
+ if ( ! preg_match( '/-[0-9a-f]{32}$/', $old_basename ) ) {
+     $hash_suffix  = wp_hash( $old_basename );
+     $new_filename = $old_basename . '-' . $hash_suffix . '.csv';
+ } else {
+     $new_filename = basename( $old_file );
+ }
+
+ $dest = $backup_dir . '/' . $new_filename;
  if ( ! file_exists( $dest ) ) {
-     rename( $old_file, $dest );
+     rename( $old_file, $dest );
  } else {
-     unlink( $old_file );
+     wp_delete_file( $old_file );
  }

Why: Migrating old files without adding a hash suffix defeats the purpose of filename obfuscation — the old predictable names would persist in the new directory. This checks whether the file already has a 32-char hex suffix and only appends one if missing, so files are never double-hashed.

Also replaces unlink() with wp_delete_file(). This uses the WordPress API which fires the wp_delete_file filter hook, allowing security plugins to log the deletion or perform cleanup.


6. Gate migration behind an option flag

File: class-wc-connect-functions.phpget_backed_up_tax_rate_files()

  // Attempt to migrate legacy files from the public uploads root into the protected directory.
- $old_files = glob( $upload_dir['basedir'] . '/taxjar-wc_tax_rates-*.csv' );
-
- if ( ! empty( $old_files ) ) {
+ if ( ! get_option( 'wcs_tax_backup_files_migrated' ) ) {
+     $old_files = glob( $upload_dir['basedir'] . '/taxjar-wc_tax_rates-*.csv' );
+
+     if ( ! empty( $old_files ) ) {
          // ... migration logic ...
+     }
+
+     // Mark migration complete only when no old files remain, so it retries if rename() failed.
+     if ( empty( glob( $upload_dir['basedir'] . '/taxjar-wc_tax_rates-*.csv' ) ) ) {
+         update_option( 'wcs_tax_backup_files_migrated', true, false );
+     }
  }

Why: get_backed_up_tax_rate_files() is called from get_form_data() on every WooCommerce Status page load. Without gating, the migration glob() and rename loop execute every time — even when there are no legacy files left to migrate. On most sites post-migration, this is a wasted glob() call scanning the uploads root on every admin page view.

The option flag is only set when the post-migration glob() confirms no old files remain. This means:

  • If rename() failed for some files, the flag stays unset and migration retries on the next page load.
  • The third false argument to update_option() disables autoloading, since this option is only checked on the status page, not on every request.

7. Tighten the filename validation regex

File: class-wc-connect-help-view.phphandle_tax_backup_download()

- // Validate filename matches expected pattern.
- if ( ! preg_match( '/^taxjar-wc_tax_rates-\d{2,4}-\d{2,4}-\d{2,4}-\d+\.csv$/', $filename ) ) {
+ // Validate filename matches expected pattern: YYYY-MM-DD or legacy MM-DD-YYYY, with optional hash suffix.
+ if ( ! preg_match( '/^taxjar-wc_tax_rates-(\d{4}-\d{2}-\d{2}|\d{2}-\d{2}-\d{4})-\d{1,10}(-[0-9a-f]{32})?\.csv$/', $filename ) ) {

Why: The original regex \d{2,4}-\d{2,4}-\d{2,4} is too permissive — it would match nonsensical patterns like 99-9999-99 or 9999-99-9999. The replacement uses an alternation group (\d{4}-\d{2}-\d{2}|\d{2}-\d{2}-\d{4}) that only accepts ISO dates (YYYY-MM-DD, used by new backups) or the legacy US format (MM-DD-YYYY, used by old TaxJar plugin).

Additionally:

  • \d{1,10} caps the Unix timestamp portion, preventing excessive backtracking (minor ReDoS protection).
  • (-[0-9a-f]{32})? is an optional group for the new wp_hash() suffix, so the regex accepts both old (unhashed) and new (hashed) filenames.

8. Fix whitespace alignment in get_form_data()

File: class-wc-connect-help-view.phpget_form_data()

- 'health_items'             => $this->get_health_items(),
- 'services'                 => $this->get_services_items(),
- 'logging_enabled'          => $this->logger->is_logging_enabled(),
- 'debug_enabled'            => $this->logger->is_debug_enabled(),
- 'logs'                     => array(
+ 'health_items'            => $this->get_health_items(),
+ 'services'                => $this->get_services_items(),
+ 'logging_enabled'         => $this->logger->is_logging_enabled(),
+ 'debug_enabled'           => $this->logger->is_debug_enabled(),
+ 'logs'                    => array(

Why: Cosmetic fix. The => operators were over-padded to align with 'tax_backup_download_url' key from a previous iteration. This normalizes alignment to the current longest key, following WordPress coding standards for associative arrays.


9. Add nocache_headers() and flush output buffers before streaming

File: class-wc-connect-help-view.phphandle_tax_backup_download()

+ nocache_headers();
  header( 'Content-Type: text/csv' );
  header( 'Content-Disposition: attachment; filename="' . $filename . '"' );
  header( 'Content-Length: ' . filesize( $file_path ) );
+
+ // Clean any output buffers to prevent corrupted downloads.
+ while ( ob_get_level() > 0 ) {
+     ob_end_clean();
+ }
+
  readfile( $file_path );
  exit;

Why: Two additions:

  • nocache_headers(): Prevents browsers and proxy caches from storing the authenticated download response. Without this, a shared proxy could serve a cached copy of the CSV to a different user, or the browser cache could serve a stale file after tax rates have changed.

  • ob_end_clean() loop: WordPress core, WooCommerce, and other plugins often have output buffers active during AJAX handling. If any buffered content (PHP notices, debug output, plugin HTML) exists when readfile() executes, that content is prepended to the CSV data. This causes two problems: (1) the Content-Length header becomes incorrect because it only counts the file's bytes, not the buffered output, and (2) the downloaded file is corrupted with non-CSV data at the beginning. This pattern is used by WooCommerce core in WC_Download_Handler::download_file_force().

iyut and others added 2 commits March 12, 2026 10:46
* Relocate tax backups to protected directory and harden file security

Move backup storage from taxjar-backups to woocommerce_uploads/taxes,
add wp_hash() suffix to filenames to prevent URL guessing, and improve
directory protection using the WC ReportCSVExporter pattern. Migration
logic now uses an idempotency flag (wcs_tax_backup_files_migrated) and
wp_delete_file() for safer cleanup of legacy files.

* Update download handler for new backup directory and add response hardening

Point file download path to woocommerce_uploads/taxes, tighten the
filename regex to accept hash-suffixed names and both date formats,
and add nocache_headers() plus output buffer cleanup for reliable
CSV downloads.

* fix the marking migration logic

---------

Co-authored-by: Iyut <iyut85@yahoo.com>
@iyut
Copy link
Copy Markdown
Collaborator Author

iyut commented Mar 12, 2026

@bartech , thanks a lot for helping hardening the security and I've merged your changes to this branch. When you are free, please kindly review this PR again.

@iyut iyut requested a review from bartech March 12, 2026 04:22
Copy link
Copy Markdown
Collaborator

@bartech bartech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@iyut iyut merged commit 0f824ea into trunk Apr 6, 2026
9 checks passed
@iyut iyut deleted the fix/issue-wootax-46 branch April 6, 2026 05:35
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.

2 participants