Rename exports folder to avoid deleting other files

[iandunn]

#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.

[iandunn]

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).

[allendav]

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

[iandunn]

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]

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

[allendav]

Fix the folder name

[Ov3rfly]

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?

[allendav]

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)

[iandunn]

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.

[Ov3rfly]

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.

[iandunn]

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.

[iandunn]

Reopening to consider backporting to 4.9.

[azaozz]

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.

[allendav]

Unit tests for wp_privacy_delete_old_export_files

[allendav]

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