WordPress.org

Make WordPress Core

Opened 8 days ago

Closed 8 days ago

Last modified 7 days ago

#44091 closed defect (bug) (fixed)

Rename exports folder to avoid deleting other files

Reported by: iandunn Owned by: iandunn
Milestone: 4.9.6 Priority: high
Severity: normal Version: trunk
Component: Privacy Keywords: gdpr commit fixed-major dev-feedback
Focuses: Cc:

Description

#43546 created the wp-content/uploads/exports folder in order to store personal data exports. @johnjamesjacoby pointed out that that's likely to conflict with existing folders, and that the wp_privacy_delete_old_export_files() cron job will delete any files in that folder.

To avoid unintentionally deleting files created by plugins, we should probably rename the folder to something more specific, like wp-content/uploads/wp-personal-data-exports.

Attachments (4)

44091.diff (2.6 KB) - added by allendav 8 days ago.
44091.2.diff (2.6 KB) - added by allendav 8 days ago.
Fix the folder name
44091.3.diff (2.5 KB) - added by allendav 8 days ago.
44091-unit-tests.diff (2.9 KB) - added by allendav 7 days ago.
Unit tests for wp_privacy_delete_old_export_files

Download all attachments as: .zip

Change History (22)

#1 @iandunn
8 days ago

  • Status changed from new to assigned
  • Summary changed from Rename exports folder to minimize chance of conflicts to Rename exports folder to avoid deleting other files

Since the folder it set in 3 places, it might be nice to have a central DRY location where it gets created, and then apply a filter. That'll minimize the chance of human error with redundant code, and also give people a way to move the folder outside the document root, in case they want additional hardening (see ticket:44091#comment:63).

This ticket was mentioned in Slack in #gdpr-compliance by iandunn. View the logs.


8 days ago

#3 @allendav
8 days ago

I agree. This needs a more specific name like @iandunn suggests.

#4 @iandunn
8 days ago

A constant would be the most consistent with other folders, like WP_LANG_DIR and WPMU_PLUGIN_DIR, but would require modifying wp-config.php instead of letting plugins customize it.

The easiest option would be to just continue manually hardcoding it in all 3 locations. That seems like it's hard to maintain and prone to human error, though.

I think a function that applies a filter to the return value might be the best way to go. If we do that, we'll need to make sure that the file it's contained in is always require_once()'d from the callers, unless the file is one that is always loaded on the front and back ends.

Some alternate names were discussed in Slack.

@allendav
8 days ago

#5 @allendav
8 days ago

To test

  • IMPORTANT: YOU CANNOT USE ANY EXISTING REQUESTS AFTER THIS CHANGE - SINCE WE PRESERVE THE PATH FOR THE SAKE OF EMAIL LINKS NOT BREAKING - YOU SHOULD DELETE ALL EXISTING EXPORT REQUESTS
  • WP_DEBUG true
  • tail error log (e.g. tail -f /usr/local/var/log/httpd/error_log )
  • open wp-admin and go to Tools > Export Personal Data
  • run an export for any request
  • ensure you can receive the file
  • open wp-content/uploads/wp-personal-data-exports and make sure you see your file
  • copy in a file into that folder
  • use touch -mt 201804110000 filename

to change it to > 3 days old

  • use wp-crontrol to force wp_privacy_delete_old_export_files to run immediately (instead of waiting an hour)
  • make sure that old file is immediately deleted (i.e. ala cron)
  • make sure no errors are logged
Last edited 8 days ago by allendav (previous) (diff)

@allendav
8 days ago

Fix the folder name

#6 @allendav
8 days ago

  • Keywords has-patch needs-testing added; needs-patch removed

This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.


8 days ago

#8 follow-up: @Ov3rfly
8 days ago

Attachment 44091.2.diff​ added

New version uses trailingslashit() in a different way. Shouldn't the whole path be "trailingslashed"?

And shouldn't 'basedir' be used with DIRECTORY_SEPARATOR instead of forward slash?

Last edited 8 days ago by Ov3rfly (previous) (diff)

#9 in reply to: ↑ 8 @allendav
8 days ago

Replying to Ov3rfly:

Attachment 44091.2.diff​ added

New version uses trailingslashit() in a different way. Shouldn't the whole path be "trailingslashed"?

True. I changed that on purpose to enforce the ending of $upload_dirbaseurl? and $upload_dirbasedir? before concatenation with the folder name

And shouldn't 'basedir' be used with DIRECTORY_SEPARATOR instead of forward slash?

Good idea.

Also moved require_once to the actual place it is required (inside the cron job)

@allendav
8 days ago

#10 @iandunn
8 days ago

44091.3.diff looks good at first glance, testing it deeper now.

I don't think we typically use DIRECTORY_SEPARATOR, see ticket:32519#comment:1 and ticket:29726#comment:2.

#11 @Ov3rfly
8 days ago

That went a bit wrong, DIRECTORY_SEPARATOR (if at all) should be used only for basedir, not baseurl.

Just saw the different usage of trailingslashit() between the versions and wanted to ask for the reason.

Not sure if trailingslashit() is necessary here at all though, wp_upload_dir() results usually do not have any trailing slashes or backslashes, so hardcoding slash (or the php seperator) between and at end should be fine.

#12 @iandunn
8 days ago

  • Owner set to iandunn
  • Resolution set to fixed
  • Status changed from assigned to closed

In 43284:

Privacy: Rename exports folder to avoid deleting other files.

Previously, personal data exports were stored in wp-content/uploads/exports, which is generic enough that it's likely there are existing folders with that name, either created by plugins or manually by administrators. If that folder were reused by Core, then wp_privacy_delete_old_export_files() would delete all of the existing files inside it, which is almost certainly not what the site owner wants or expects.

To avoid that, the folder is being renamed to include a specific reference to Core, and a more verbose description of its purpose. With those factored in, it's very unlikely that there will be any conflicts with existing folders.

The wp_privacy_exports_dir() and wp_privacy_exports_url() functions were introduced to provide a canonical source for the location, and the wp_privacy_exports_dir and wp_privacy_exports_url filters were introduced to allow plugins to customize it.

Props johnjamesjacoby, allendav.
Fixes #44091.

#13 @iandunn
8 days ago

  • Keywords commit fixed-major dev-feedback added; has-patch needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to consider backporting to 4.9.

#14 @azaozz
8 days ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 43285:

Privacy: Rename exports folder to avoid deleting other files.

Previously, personal data exports were stored in wp-content/uploads/exports, which is generic enough that it's likely there are existing folders with that name, either created by plugins or manually by administrators. If that folder were reused by Core, then wp_privacy_delete_old_export_files() would delete all of the existing files inside it, which is almost certainly not what the site owner wants or expects.

To avoid that, the folder is being renamed to include a specific reference to Core, and a more verbose description of its purpose. With those factored in, it's very unlikely that there will be any conflicts with existing folders.

The wp_privacy_exports_dir() and wp_privacy_exports_url() functions were introduced to provide a canonical source for the location, and the wp_privacy_exports_dir and wp_privacy_exports_url filters were introduced to allow plugins to customize it.

Props johnjamesjacoby, allendav.
Merges [43284] to the 4.9 branch.
Fixes #44091.

This ticket was mentioned in Slack in #gdpr-compliance by allendav. View the logs.


8 days ago

This ticket was mentioned in Slack in #core by desrosj. View the logs.


8 days ago

#17 @desrosj
7 days ago

  • Component changed from Administration to Privacy

@allendav
7 days ago

Unit tests for wp_privacy_delete_old_export_files

#18 @allendav
7 days ago

Nevermind that unit test - i meant to add that to 43546 - will do so in a second.

Note: See TracTickets for help on using tickets.