#44038 closed defect (bug) (fixed)
Change personal data export path stored in request meta to relative paths
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (47)
@
3 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.
3 years ago
This ticket was mentioned in Slack in #core by pbiron. View the logs.
3 years ago
#9
@
3 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.
This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.
3 years ago
This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.
3 years ago
#12
@
3 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:
- A relative path stored in meta (accounted for in 44038.patch).
- A relative path stored in meta that becomes invalid (maybe a new plugin changes the data export directory via a filter, for example)
- A full path stored in meta that remains valid after this change.
- 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).
- 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.
This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.
3 years ago
This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.
3 years ago
This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.
3 years ago
This ticket was mentioned in Slack in #core-privacy by webdevlaw. View the logs.
3 years ago
This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.
2 years ago
#19
@
2 years 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.
2 years ago
This ticket was mentioned in Slack in #core-privacy by postphotos. View the logs.
2 years ago
This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.
2 years ago
This ticket was mentioned in Slack in #core-privacy by nmenescardi. View the logs.
2 years ago
#25
@
2 years 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.
#27
@
2 years 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
#28
@
21 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.
21 months ago
This ticket was mentioned in Slack in #core by xkon. View the logs.
21 months ago
This ticket was mentioned in Slack in #core-privacy by xkon. View the logs.
20 months ago
This ticket was mentioned in Slack in #core-privacy by xkon. View the logs.
20 months ago
#34
@
20 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 :-).
Moving to the new Privacy component.