Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#43905 closed defect (bug) (fixed)

Personal data export link does not work

Reported by: desrosj's profile desrosj Owned by: iandunn's profile iandunn
Milestone: 4.9.6 Priority: normal
Severity: normal Version: 5.1
Component: Privacy Keywords: gdpr fixed-major
Focuses: Cc:

Description

After taking the following actions, a personal data export file is inaccessible:

  1. Initiate a personal data export request.
  2. Confirm the request by clicking the link in the confirmation email.
  3. As an admin, click the "Email Data" button.
  4. Click "Download Personal Data" in the admin by hovering over the list table item for that user.
  5. Click the link in the email.

You will see that the export file does not exist.

Attachments (2)

43905.diff (2.7 KB) - added by allendav 7 years ago.
Once a filename has been generated for a request export, keep using it.
43905.2.diff (2.5 KB) - added by allendav 7 years ago.
Refreshed patch; also now using wp_delete_file instead of unlink

Download all attachments as: .zip

Change History (20)

This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.


7 years ago

#2 in reply to: ↑ description @xkon
7 years ago

Replying to desrosj:

After taking the following actions, a personal data export file is inaccessible:

  1. Initiate a personal data export request.
  2. Confirm the request by clicking the link in the confirmation email.
  3. As an admin, click the "Email Data" button.
  4. Click "Download Personal Data" in the admin by hovering over the list table item for that user.
  5. Click the link in the email.

You will see that the export file does not exist.

I just tested this again and again but with no luck breaking it. I have a total of 23 exported .zip files (3 different emails as well) split in Download/Email and the files are all there + mails do work as supposed to ofc.

-So!-

Note 1: Every time you press either Email or Download a new zip is created so the link doesn't make sense not to work. Maybe the .zip didn't got generated on your end at all for some reason?

Note 2: The e-mail is in plain-text so the actual URL of the file is not enclosed in tags or anything. Since I'm not sure 'how' you're reading your sent e-mail, might that link just got broken since it gets on auto href from the client? ( I know silly note but sometimes happens :D ) .

#3 @TZ Media
7 years ago

  • Keywords 2nd-opinion added

I can confirm the bug itself (local install of trunk without patches).

What I believe happens is that every time a new file is generated, the old file is deleted.

  • On clicking "Email Data", a file is generated and a link to that file is sent to the user.
  • On clicking "Download Personal Data", that original file is replaced with a new file (with a different name).
  • When the user tries to download the first file, it does not exists anymore.

If my thoughts are correct, then this is broken by design, as the direct download in the backend invalidates the link sent via email. Question is: Is this a bug, or a feature?

#4 @allendav
7 years ago

@TZ Media - that is certainly a way to get the problem to occur. I hadn't anticipated the Email flow would be followed by a Download flow. I'll change the download flow to use the file generated for the email flow if it is present instead of regenerating it.

Patch forthcoming

This ticket was mentioned in Slack in #gdpr-compliance by allendav. View the logs.


7 years ago

@allendav
7 years ago

Once a filename has been generated for a request export, keep using it.

#6 @allendav
7 years ago

  • Keywords has-patch added

To test: Follow the steps in the original description of this ticket and verify you are able to download the file via the email link even after using the download flow. (The filename should not change even as the file gets re-generated.)

#7 @TZ Media
7 years ago

Tested. Works for me.

This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.


7 years ago

This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.


7 years ago

#10 @desrosj
7 years ago

  • Keywords needs-refresh added; 2nd-opinion removed

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


7 years ago

@allendav
7 years ago

Refreshed patch; also now using wp_delete_file instead of unlink

#12 @allendav
7 years ago

  • Keywords needs-refresh removed

#13 @iandunn
7 years ago

  • Owner set to iandunn
  • Resolution set to fixed
  • Status changed from new to closed

In 43180:

Privacy: Reuse existing archive filenames to maintain URLs.

Whenever an admin initiates a download or email of a personal data export, a fresh copy of the file is generated. Previously, a new filename was used each time, which could lead to situations where a URL that was emailed to a data subject is broken.

That can be avoided by reusing the same filename when building fresh archives.

Props desrosj, tz-media, allendav.
Fixes #43905.

#14 @iandunn
7 years ago

  • Keywords fixed-major added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version set to trunk

Reopening for backport to 4.9.

#15 @iandunn
7 years ago

FYI: I left out the bit from 43905.2.diff where the existing unlink() was changed to wp_delete_file(). It's good to change that, but it's not a bug, so I think it should wait until after 4.9.6.

#16 @desrosj
7 years ago

Created #44014 to keep track of the wp_delete_file() change that was too late for 4.9.6.

#17 @SergeyBiryukov
7 years ago

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

In 43186:

Privacy: Reuse existing archive filenames to maintain URLs.

Whenever an admin initiates a download or email of a personal data export, a fresh copy of the file is generated. Previously, a new filename was used each time, which could lead to situations where a URL that was emailed to a data subject is broken.

That can be avoided by reusing the same filename when building fresh archives.

Props desrosj, tz-media, allendav.
Merges [43180] to the 4.9 branch.
Fixes #43905.

#18 @desrosj
7 years ago

  • Component changed from General to Privacy

Moving to the new Privacy component.

Note: See TracTickets for help on using tickets.