WordPress.org

Make WordPress Core

Opened 15 months ago

Last modified 2 weeks ago

#44038 new defect (bug)

Change personal data export path stored in request meta to relative paths

Reported by: allendav Owned by:
Milestone: 5.3 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: needs-unit-tests has-patch needs-refresh good-first-bug
Focuses: administration Cc:

Description

Props @sergey for the idea.

I'd like to change it so we store file names relative to the exports directory rather than full path names (the latter would break when moving to another server, and I don't think we store full paths anywhere else in core, except for the list of recent files in plugin/theme editor), but I guess that can wait for 4.9.7, and since the files expire in a few days, doesn't seem like a big issue.

Attachments (2)

44038.patch (1.1 KB) - added by mrTall 13 months ago.
On this diff it store only filename, cuz we have full path through $exports_dir = wp_privacy_exports_dir();
44038.2.diff (3.5 KB) - added by cameronamcintyre 2 weeks ago.

Download all attachments as: .zip

Change History (24)

#1 @desrosj
14 months ago

  • Milestone changed from Awaiting Review to 4.9.7

#2 @desrosj
14 months ago

  • Component changed from General to Privacy

Moving to the new Privacy component.

#3 @desrosj
14 months ago

  • Version set to 4.9.6

Marking privacy bugs as introduced in 4.9.6.

#4 @desrosj
14 months ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

#5 @desrosj
13 months ago

  • Keywords needs-unit-tests added

@mrTall
13 months ago

On this diff it store only filename, cuz we have full path through $exports_dir = wp_privacy_exports_dir();

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


13 months ago

#7 @desrosj
13 months ago

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

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


12 months ago

#9 @desrosj
12 months ago

  • Milestone changed from 4.9.8 to 4.9.9

4.9.8 has entered release candidate stages and this still needs testing and unit tests.

Last edited 12 months ago by desrosj (previous) (diff)

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


12 months ago

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


12 months ago

#12 @desrosj
11 months ago

  • Focuses administration added
  • Keywords needs-refresh added; needs-testing removed

Thanks for the patch, @mrTall! 44038.patch stores the relative patch for new exports, but it has one issue, and there are some additional scenarios that should be considered.

44038.patch assumes the value stored in meta is always relative and adds the $exports_dir to the beginning of the value without examining it. Say a site has an old export (created before this change) that includes the full path in the meta value (/srv/www/wordpress/public_html/wp-content/exports/exportfile.zip, for example). The patch will always add the path to the beginning, even if it is the same, becoming /srv/www/wordpress/public_html/wp-content/exports/srv/www/wordpress/public_html/wp-content/exports/exportfile.zip.

These are the scenarios that should also be accounted for:

  1. A relative path stored in meta (accounted for in 44038.patch).
  2. A relative path stored in meta that becomes invalid (maybe a new plugin changes the data export directory via a filter, for example)
  3. A full path stored in meta that remains valid after this change.
  4. A full path stored in meta that becomes invalid when a site is migrated (either to a new server, or moving WordPress to a new directory on the same server).
  5. A full path stored in meta that remains valid, but the export directory has changed (plugin changes the data export directory).

All these things considered, I think this is the best plan forward:

  • If a relative path is stored, assume it is in the current export directory. Unfortunately, old links will become dead, but this is how attachment files are currently handled (see _wp_attached_file meta key).
  • If a full path is stored, check if it is still valid and usable. If it is and it can be protected from being browsable, leave the full path in meta and continue to use that path to preserve export links.
  • If a full path is stored and it is not valid, usable, or cannot be protected from browsing, use the same filename but the current export directory and save the relative path to meta. Old links will also become dead here, though.

@SergeyBiryukov does this make sense? Did I miss anything?

The unit tests for this also depend on #44233. They should reside within the test class being created there.

Version 0, edited 11 months ago by desrosj (next)

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


11 months ago

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


10 months ago

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


10 months ago

#16 @pento
9 months ago

  • Milestone changed from 4.9.9 to Future Release

This ticket was mentioned in Slack in #core-privacy by webdevlaw. View the logs.


9 months ago

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


7 weeks ago

#19 @garrett-eclipse
7 weeks ago

  • Keywords good-first-bug added
  • Milestone changed from Future Release to 5.3

Thanks for the clear direction @desrosj as there's a straight-forward plan here going to milestone for 5.3 and label good-first-bug in preparation for wceu contributor day.

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


5 weeks ago

This ticket was mentioned in Slack in #core-privacy by postphotos. View the logs.


4 weeks ago

#22 @cameronamcintyre
2 weeks ago

@desrosj I think I was able to cover everything this way?

Note: See TracTickets for help on using tickets.