Late-escaping of output, removal of $_REQUEST - #4
stevegrunwell wants to merge 20 commits into
Conversation
…provider label, whereas print_label() runs said label through esc_html before echoing
…c keypads on supported devices, but the usage of input[type='tel'] could cause accessibility concerns
… on how they are being used
…necessary to use verbose placeholders like %2$s
…$_POST['redirect_to'] was the value being set
|
Hmm, seem to be encountering an infinite login loop when I hit mysite.com/wp-login.php directly; will troubleshoot and have a patch shortly, as it may be connected to my |
|
That's exactly what it was, the 'redirect_to' value was getting lost, resulting in an infinite login loop. |
There was a problem hiding this comment.
This I had intentionally left as $_REQUEST in case we later modify the emails to include a link they can follow with the $_GET argument pre-filled.
… a comment explaining why
There was a problem hiding this comment.
Good catch. I don't know if it will ever be useful in practice, as the content will only ever be a substr of a md5 hash, but it's still a good practice to be in.
There was a problem hiding this comment.
Depending on how the AJAX processing endpoints are written, this may be better left as $_REQUEST to let one endpoint do both. Maybe. I'd need to see it built out first.
|
I skipped a few remarks at the end between $_REQUEST and $_POST -- but I'd prefer if for the moment anyways, we could leave all of those as $_REQUEST until I get the backup methods branch sorted. |
…t's not necessary to use verbose placeholders like %2$s" This reverts commit 04be14a.
…ant them to be clean), use wp_strip_all_tags() instead of esc_html() in get_columns()
…esc_html() isn't really the ideal escaping method for email content
…epending on how they are being used" This reverts commit fa8e5a9 at the request of @georgestephanis while he works on some of the fallback code (see #4) Conflicts: class.two-factor-core.php providers/class.two-factor-email.php
|
@georgestephanis I've gone through and addressed your feedback on the PR, thank you for being so thorough! I've also reverted the |
There was a problem hiding this comment.
This is escaped inside login_html() here: https://github.com/georgestephanis/two-factor/blob/41bed06cc04ec0683ce34ac826cc8a29dd2e5790/class.two-factor-core.php#L144 -- doesn't need double escaping.
…everything as it outputs it
|
Can you tell I've been burned by under-escaping before? ;) |
|
👍 |
|
I'm not sure if this PR is still needed anymore. There are aspects of it that made it into Master already. @stevegrunwell would you like to fix the merge conflicts so we can see what's different, or should we close the PR? |
Conflicts: class.application-passwords-list-table.php class.application-passwords.php class.two-factor-core.php providers/class.two-factor-email.php providers/class.two-factor-provider.php
|
@valendesigns It looks like the only thing left in the PR that hasn't been merged is converting the two-factor-input-code input from |
|
We'll want to make sure to include the backup codes provider if we take another look at |
4 participants
We're looking to use this on a platform that champions "escape all the things!", so I've gone through and added late-escaping wherever it was missing, following the WordPress VIP standards (as they tend to know a thing or two about securing WordPress 😉).
This PR also replaces instances of
$_REQUESTwith$_POSTor$_GET, based on how the data is being received.Thank you for putting this together!