WordPress.org

Make WordPress Core

Opened 17 months ago

Last modified 2 months 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 (3)

44038.patch (1.1 KB) - added by mrTall 15 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 3 months ago.
44038.3.patch (4.1 KB) - added by nmenescardi 2 months ago.
It contains some tests an changes in 44038.2 patch

Download all attachments as: .zip

Change History (28)

#1 @desrosj
17 months ago

  • Milestone changed from Awaiting Review to 4.9.7

#2 @desrosj
16 months ago

  • Component changed from General to Privacy

Moving to the new Privacy component.

#3 @desrosj
16 months ago

  • Version set to 4.9.6

Marking privacy bugs as introduced in 4.9.6.

#4 @desrosj
16 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
15 months ago

  • Keywords needs-unit-tests added

@mrTall
15 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.


15 months ago

#7 @desrosj
15 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.


14 months ago

#9 @desrosj
14 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 14 months ago by desrosj (previous) (diff)

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


14 months ago

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


14 months ago

#12 @desrosj
14 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 for backwards compatibility.

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.

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

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


13 months ago

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

#16 @pento
12 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.


12 months ago

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


4 months ago

#19 @garrett-eclipse
4 months 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.


3 months ago

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


3 months ago

#22 @cameronamcintyre
3 months ago

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

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


2 months ago

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


2 months ago

#25 @nmenescardi
2 months ago

I’ve been reviewing this ticket. I applied last patch (44038.2) and ran existing tests in privacy group, they all passed.

Also, I created two more tests related with this ticket using absolute paths. I tried to follow some of the points highlighted on @desrosj comment: https://core.trac.wordpress.org/ticket/44038#comment:12

I found two possible issues with this patch:
1) if a not valid path is saved (eg: change hosting) I believe it doesn’t resolve a new valid directory and file name.

2) If an old valid absolute path was saved, it doesn’t keep that value. Instead, it generates and saves a relative path as new value. It works with new links but probably it doesn’t preserve old ones.

Tests are added in a new patch: 44038.3 along with previous changes.

@nmenescardi
2 months ago

It contains some tests an changes in 44038.2 patch

Note: See TracTickets for help on using tickets.