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

[xkon]

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?

[subrataemfluence]

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.

[birgire]

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 :)

[xkon]

@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(?) ).

[desrosj]

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

[desrosj]

#44102 was marked as a duplicate.

[desrosj]

Moving to the new Privacy component.

[desrosj]

Marking privacy bugs as introduced in 4.9.6.

[desrosj]

Moving all tickets in 4.9.7 to 4.9.8.

[saimonh]

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

[birgire]

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.

[idea15]

#44518 was marked as a duplicate.

[desrosj]

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

[desrosj]

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

[desrosj]

Related: #44707.

[desrosj]

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

[garrett-eclipse]

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]

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

[garrett-eclipse]

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

[garrett-eclipse]

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]

Removed unintended newline from this version

[birgire]

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'] in a string.

[garrett-eclipse]

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.

[birgire]

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.

[garrett-eclipse]

Commit Candidate - This is the .clean. version

[garrett-eclipse]

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.

[xkon]

@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]

Updated patch to use admin_url and tools.php

[garrett-eclipse]

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

[xkon]

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

[garrett-eclipse]

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

[pento]

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]

Adjustment for the admin url output 44047-3.diff​

[desrosj]

In 45154:

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

Introduced in [45149].

Props: birgire.
Fixes #44047.