Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#44047 closed defect (bug) (fixed)

The link you followed has expired. - Export / Erasure admin screens

Reported by: xkon's profile xkon Owned by: pento's profile pento
Milestone: 5.2 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch commit
Focuses: ui, administration Cc:

Description

This happens to both Erasure & Export admin screens.

To reproduce:

  • Add a request and don't confirm the e-mail ( just for faster test )
  • Make a 'forced' export or erasure
  • Refresh the page so the Remove Request button gets activated
  • Press Remove Request
  • Add any e-mail without refreshing the page (I didn't use a different email on the preview but it happens with all)

Note: the new confirmation is actually sent & the email is working properly even though you get the The link you followed has expired.

@allendav any ideas or something that I might be missing on this one?

Attachments (8)

expired_link_issue.gif (410.5 KB) - added by xkon 6 years ago.
patch.diff (631 bytes) - added by saimonh 6 years ago.
Hello, I think we just can't simply reload the browser as there are some message to show. Meaning reloading the browser to fix the issue will prevent us to see the success message of what has been deleted or performed. So the best option I could think of is to remove the query args from the url using javascript. I've attached a patch please have a look. Regards
44047.clean.diff (1001 bytes) - added by garrett-eclipse 5 years ago.
Addresses the issue by explicitly placing the action="?page=remove_personal_data" and action="?page=remove_personal_data" onto the forms.
44047.getpage.diff (1.1 KB) - added by garrett-eclipse 5 years ago.
Dynamically sets the form action to ?page=$_GET['page'] to address the issue
44047.getpage.2.diff (1.1 KB) - added by garrett-eclipse 5 years ago.
Removed unintended newline from this version
44047.commit.diff (1001 bytes) - added by garrett-eclipse 5 years ago.
Commit Candidate - This is the .clean. version
44047.2.diff (1.1 KB) - added by garrett-eclipse 5 years ago.
Updated patch to use admin_url and tools.php
44047-3.diff (1.3 KB) - added by birgire 5 years ago.

Download all attachments as: .zip

Change History (51)

#1 @subrataemfluence
6 years ago

I confirm that I could reproduce the issue by following the steps.

However

"Add any e-mail without refreshing the page...",

if I do it even after refreshing the page the message is still coming.

Not sure if I am right, but when I Remove request, the process page URL is like this:

/wp-admin/tools.php?page=export_personal_data&action=delete&request_id%5B0%5D=136&_wpnonce=cb4afd7f30

and after removal, the page stays only there.

After the removal is done the value for _w_http_referrer is

/wp-admin/tools.php?page=export_personal_data&action=delete&request_id%5B0%5D=136&_wpnonce=cb4afd7f30

which is reported expired since we have already removed that request.

But when I load the page by clicking Tools > Export personal data link, the referrer is /wp-admin/tools.php?page=export_personal_data.

To my understanding after deleting a request the page should redirect back to /wp-admin/tools.php?page=export_personal_data, so that no Expired Link is reported.

I may be wrong!

Last edited 6 years ago by subrataemfluence (previous) (diff)

#2 @birgire
6 years ago

Thanks for the ticket @xkon

I thought I noticed something similar while testing request deleting for another ticket yesterday (#44000), but I suspected this to be something specific to my modified test site, at the time :)

#3 @xkon
6 years ago

@birgire np at all, it isn't actually causing any issues or breaking any newly generated links, it's just an error message from the URL not being updated properly as @subrataemfluence mentions although I didn't check the code as well to dig deeper.

So maybe yes, a nice and easy solution would be that after a Remove Request we could just redirect to the tool page so the url is clean etc ( or remove any extra vars from it with js so the next action starts with a clean one(?) ).

#4 @iandunn
6 years ago

  • Component changed from General to Administration
  • Keywords gdpr needs-patch added
  • Milestone changed from Awaiting Review to 4.9.6
  • Owner set to iandunn
  • Status changed from new to accepted

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


6 years ago

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


6 years ago

#7 @iandunn
6 years ago

  • Owner iandunn deleted
  • Status changed from accepted to assigned

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


6 years ago

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


6 years ago

#10 @desrosj
6 years ago

  • Milestone changed from 4.9.6 to 4.9.7

This still needs a patch. With RC2 in the next few hours, punting to 4.9.7.

#11 @desrosj
6 years ago

#44102 was marked as a duplicate.

#12 @desrosj
6 years ago

  • Component changed from Administration to Privacy

Moving to the new Privacy component.

#13 @desrosj
6 years ago

  • Version set to 4.9.6

Marking privacy bugs as introduced in 4.9.6.

#14 @desrosj
6 years ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

@saimonh
6 years ago

Hello, I think we just can't simply reload the browser as there are some message to show. Meaning reloading the browser to fix the issue will prevent us to see the success message of what has been deleted or performed. So the best option I could think of is to remove the query args from the url using javascript. I've attached a patch please have a look. Regards

#15 @birgire
6 years ago

There can be challenges using two or more forms, each with a different nonce, on the same page, when the nonce is added to the url as a GET parameter.

If ticket #43912 (Requests (new, pending, completed) should not be deletable) is the way to go, then this problem goes away :)

Ps: For non-javascript support, the core uses the message GET parameter, to be able to display the corresponding notice after the redirection. We can see this when a tag is deleted on the edit-tags.php screen when javascript is disabled. For example:

/wp-admin/edit-tags.php?taxonomy=category&message=2

that displays the Category deleted message.

#16 @idea15
6 years ago

#44518 was marked as a duplicate.

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


6 years ago

#18 @desrosj
6 years ago

  • Keywords gdpr removed

Removing the GDPR keyword. This has been replaced by the new Privacy component and privacy focuses in Trac.

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


6 years ago

#20 @desrosj
6 years ago

  • Milestone changed from 4.9.8 to 4.9.9

This still needs a patch and 4.9.8 RC has already been released.

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


6 years ago

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


6 years ago

#23 @desrosj
6 years ago

Related: #44707.

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


6 years ago

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


6 years ago

#26 @desrosj
6 years ago

  • Milestone changed from 4.9.9 to 5.0

Punting to 5.0 since it falls outside of the 4.9.9 focus.

#27 @pento
6 years ago

  • Milestone changed from 5.0 to 5.1

#28 @garrett-eclipse
5 years ago

  • Milestone changed from 5.1 to Future Release

I'm moving this ticket to 'Future Release' as it hasn't seen much attention. Once a patch has been worked on this can be re-milestoned.

@garrett-eclipse
5 years ago

Addresses the issue by explicitly placing the action="?page=remove_personal_data" and action="?page=remove_personal_data" onto the forms.

@garrett-eclipse
5 years ago

Dynamically sets the form action to ?page=$_GET['page'] to address the issue

#29 @garrett-eclipse
5 years ago

  • Focuses ui administration added
  • Keywords has-patch 2nd-opinion needs-testing added; needs-patch removed
  • Milestone changed from Future Release to 5.2

Hello all,

Thank you for raising the issue @xkon and for the discussion and investigation @birgire and @subrataemfluence, as well I appreciate the javascript based approach @saimonh

After reviewing and testing I found the issue is the forms themselves have no action and so take the entire url including the expired nonce. The forms only need the ?page= param applied as the action. By specifying an action we strip the expired nonce from the submission.

I've provided two working patchs to address this issue.
44047.clean.diff - Which just places the action="?page=remove_personal_data" and action="?page=remove_personal_data" onto the forms.
44047.getpage.diff - Which checks $_GET['page'] to populate the form actions.

In all honesty, the second one using $_GET is probably overkill as the page name is unlikely to change, and if it does the entirety of core would have to be swept anyway. So I would suggest going with the clean version.

Moving into 5.2 as an easy fix. Would love some additional testing to confirm the fix as well as thoughts on which option makes sense.

All the best

@garrett-eclipse
5 years ago

Removed unintended newline from this version

#30 @garrett-eclipse
5 years ago

  • Owner set to garrett-eclipse

#31 @birgire
5 years ago

Thanks for the patch and looking into it @garrett-eclipse

I will try to test it before the ticket scrub on Monday.

ps: we should avoid the injection possibility from the direct use of $_GET['page']

Version 0, edited 5 years ago by birgire (next)

#32 @garrett-eclipse
5 years ago

Thanks @birgire good point, guess that gives all the more reason to use the .clean version and ignore the .getpage version. Appreciate the testing and feedback.

#33 @birgire
5 years ago

I tested the 44047.clean.diff patch that adds the missing forms actions and it seems to fix the bug, when following the steps to reproduce the bug, in the ticket's description.

This was discussed during bug scrub and mentioned the possibility that the in future these pages might be moved to their own pages, e.g. /wp-admin/tools.php?page=remove_personal_data to /wp-admin/remove-personal-data.php. This bug would most likely still show up in that case and action='?' might also clear the GET parameters there, as mentioned by @garrett-eclipse.

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


5 years ago

@garrett-eclipse
5 years ago

Commit Candidate - This is the .clean. version

#35 @garrett-eclipse
5 years ago

  • Keywords commit added; 2nd-opinion needs-testing removed
  • Status changed from assigned to accepted

Thanks @birgire for testing this and your previous feedback concerning the getpage approach.

I've uploaded 44047.commit.diff as a final version, it's just the tested and chosen .clean. patch but wanted to upload as the most recent patch to avoid any confusion for committers.

Moving this forward to commit.

#36 @xkon
5 years ago

@garrett-eclipse / @birgire ,

I'd still be more comfortable by using tools.php?page= than just ?page= and we could change that to anything when the pages get moved, as well most forms use actions similar to tools.php?params or even with admin_url() and self_admin_url(). Maybe it would be better to be in-line with other files within the admin area :) .

I'm ok with either choice and 44047.commit.diff works as expected. I'd love an opinion from a committer though before this goes in ( even if this is a really minor thing ).

@garrett-eclipse
5 years ago

Updated patch to use admin_url and tools.php

#37 @garrett-eclipse
5 years ago

  • Keywords needs-testing added; commit removed

Thanks @xkon I appreciate the flag and feedback there. I've uploaded 44047.2.diff to use admin_url and tools.php?page= as all other references in the user.php file also used admin_url. I did take a look at self_admin_url but the tools are currently site specific and don't have a network tools yet...#43738.

@birgire / @xkon could you please take another round of testing and I'll return this to commit.
Cheers

#38 @xkon
5 years ago

  • Keywords needs-testing removed

Looks good to me @garrett-eclipse , thank you !

#39 @garrett-eclipse
5 years ago

  • Keywords commit added

Thanks @xkon I appreciate the feedback and testing. Marking for commit.

#40 @pento
5 years ago

  • Owner changed from garrett-eclipse to pento

#41 @pento
5 years ago

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

In 45149:

Privacy: Add a missing <form> action on the Export/Erasure screens.

This lack of action could cause an expired nonce to end up in the URL.

Props garrett-eclipse, saimonh, xkon.
Fixes #44047.

@birgire
5 years ago

#42 @birgire
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Adjustment for the admin url output 44047-3.diff

#43 @desrosj
5 years ago

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

In 45154:

Privacy: Pass admin URLs for data export and erase forms through esc_url().

Introduced in [45149].

Props: birgire.
Fixes #44047.

Note: See TracTickets for help on using tickets.