WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 4 months ago

Last modified 4 months ago

#44038 closed defect (bug) (fixed)

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

Reported by: allendav Owned by: xkon
Milestone: 5.5 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch dev-feedback 2nd-opinion
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 (6)

44038.patch (1.1 KB) - added by mrTall 2 years 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 16 months ago.
44038.3.patch (4.1 KB) - added by nmenescardi 15 months ago.
It contains some tests an changes in 44038.2 patch
44038.4.diff (1.7 KB) - added by garrett-eclipse 13 months ago.
Refresh to have indentation conform to CS
44038.5.diff (4.2 KB) - added by xkon 10 months ago.
44038.6.diff (8.7 KB) - added by davidbaumwald 4 months ago.
Patch with unit tests updates

Download all attachments as: .zip

Change History (47)

#1 @desrosj
2 years ago

  • Milestone changed from Awaiting Review to 4.9.7

#2 @desrosj
2 years ago

  • Component changed from General to Privacy

Moving to the new Privacy component.

#3 @desrosj
2 years ago

  • Version set to 4.9.6

Marking privacy bugs as introduced in 4.9.6.

#4 @desrosj
2 years ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

#5 @desrosj
2 years ago

  • Keywords needs-unit-tests added

@mrTall
2 years 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.


2 years ago

#7 @desrosj
2 years ago

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

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


2 years ago

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

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


2 years ago

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


2 years ago

#12 @desrosj
2 years 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 2 years ago by desrosj (previous) (diff)

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


2 years ago

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


2 years ago

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


2 years ago

#16 @pento
2 years ago

  • Milestone changed from 4.9.9 to Future Release

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


2 years ago

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


17 months ago

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


17 months ago

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


17 months ago

#22 @cameronamcintyre
16 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.


15 months ago

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


15 months ago

#25 @nmenescardi
15 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
15 months ago

It contains some tests an changes in 44038.2 patch

#26 @davidbaumwald
13 months ago

  • Keywords needs-testing added; needs-unit-tests needs-refresh removed

@garrett-eclipse
13 months ago

Refresh to have indentation conform to CS

#27 @garrett-eclipse
13 months ago

  • Keywords dev-feedback added; good-first-bug needs-testing removed
  • Milestone changed from 5.3 to 5.4

Thanks for the patches @nmenescardi / @cameronamcintyre / @mrTall

I've refreshed the patch in 44038.4.diff for minor CS items. It's currently passing unit tests as well.

As there are some concerns with the current implementation (thanks @nmenescardi) I'm flagging with dev-feedback and moving to 5.4 since we're already in 5.3-beta2.

@SergeyBiryukov or @desrosj would you mind reviewing the implementation and providing further direction and feedback.

Thank you

@xkon
10 months ago

#28 @xkon
10 months ago

  • Keywords 2nd-opinion added

I've tested the 44038.4.diff but it was generating extra .zip files for me in some instances depending on the if statements that are already present.

44038.5.diff takes a different approach and removes all previous postmeta used for the files _export_file_url and _export_file_path while keeping/creating the new _export_file_name that .4 patch introduced.

This way the database only keeps a filename and no records of hard paths/links at all so everything has to pass via the wp_privacy_exports_dir() & wp_privacy_exports_url() to build a full path and url which are following the upload_dir() so they should be fully filterable and updatable from installation to installation.

We still need to get the _export_file_url and _export_file_path via get_post_meta() for the sanity checks and to gradually move away from them.

To test this:

1] Add some random export requests ( no need to have actual data as the zips are getting generated either way ). On some, proceed to the download so the "original" postmeta can be created and on some others leave them as pending.

2] Apply the patch and click on a Download Personal Data or send an e-mail etc normally.

The database should be fully updated to use only _export_file_name and the links, as well as .zips, should be unique and generated properly without leaving any extra files or database trails to not-used .zips.

I think that this gets everything on what @SergeyBiryukov proposed and it should be fully backward compatible due to the addition/removal of the postmeta on each case if I didn't miss anything :D.

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


10 months ago

#30 @xkon
10 months ago

  • Owner set to xkon
  • Status changed from new to assigned

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


10 months ago

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


9 months ago

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


9 months ago

#34 @xkon
9 months ago

  • Milestone changed from 5.4 to 5.5

Marking this for 5.5 as 5.4 beta is close, the patch works but I'd like some 2nd eyes first and more testing for edge cases before this goes for a commit :-).

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


4 months ago

#36 @whyisjake
4 months ago

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

In 48087:

Privacy: Use relative paths for exported personal data.

Ensures back-compat while moving to paths off of the /exports directory.

Fixes: #44038.

Props: allendav, mrTall, desrosj, garrett-eclipse, cameronamcintyre, nmenescardi, xkon, whyisjake.

#37 @whyisjake
4 months ago

In 48088:

Privacy: Revert use relative paths for exported personal data.

Tests need to be updated to pass.

See: #44038.

Unprops: whyisjake.

#38 @whyisjake
4 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#39 @whyisjake
4 months ago

Tests need to be updated, @xkon can you take a look?

@davidbaumwald
4 months ago

Patch with unit tests updates

#40 @whyisjake
4 months ago

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

In 48127:

Privacy: Use relative paths for exported personal data.

Ensures back-compat while moving to paths off of the /exports directory.

Fixes: #44038.

Props: allendav, mrTall, desrosj, garrett-eclipse, cameronamcintyre, nmenescardi, xkon, whyisjake, davidbaumwald.

#41 @SergeyBiryukov
4 months ago

In 48330:

Privacy: Simplify the logic for updating the meta values for personal data export requests from absolute to relative paths.

Follow-up to [48127].

See #44038.

Note: See TracTickets for help on using tickets.