Opened 19 months ago
Closed 11 months ago
#58226 closed defect (bug) (fixed)
HTML attribute "action" on element "form": Must be non-empty.
Reported by: | Malae | Owned by: | joedolson |
---|---|---|---|
Milestone: | 6.5 | Priority: | normal |
Severity: | normal | Version: | 5.9 |
Component: | Administration | Keywords: | needs-testing has-testing-info has-patch commit |
Focuses: | coding-standards | Cc: |
Description
If checking HTML validation , it appears that the core file wp-login.php line 331
<form id="language-switcher" action="" method="get">
throws a warning: Bad value "" for attribute "action" on element "form": Must be non-empty.
However, it appears that this applies to HTML 4, but apparently in HTML 5 the use of action="" is not supported.
Checking further, I saw a statement that not including the action attribute opens the page up to iframe clickjacking attacks,
If action="" is left in, I saw suggestions to use action="#", or action="?".
Does this core file use the best valid, secure way to submit this form, or does it need changing?
Attachments (1)
Change History (22)
This ticket was mentioned in PR #4572 on WordPress/wordpress-develop by bartkleinreesink.
18 months ago
#4
- Keywords has-patch added; needs-patch removed
Added attribute "action" to language switcher form on the login page as suggested in trac ticket.
Trac ticket: https://core.trac.wordpress.org/ticket/58226
This ticket was mentioned in Slack in #core by sathyapulse. View the logs.
18 months ago
#6
@
16 months ago
- Milestone changed from Awaiting Review to 6.4
Looks good to me, putting this for 6.4
This ticket was mentioned in Slack in #core by nicolefurlan. View the logs.
14 months ago
#9
@
14 months ago
- Keywords needs-testing has-testing-info added
We discussed this ticket in today's bug scrub.
@costdev mentioned that these forms are often included elsewhere by extenders, so we need to test to make sure this solution is compatible.
#10
@
14 months ago
I've tested it on the regular login page (wp-login.php) and also checked it with plugins like 'WPS Hide Login,' which change the way the login page works. It seems to be working well with these setups.
#11
@
14 months ago
It would be great if we could get some further testing to make sure this solution works for various extenders.
@costdev I remember discussing this with you in a bug scrub a couple of weeks ago – do you have any thoughts on the extenders it should be tested with? If you could add some examples to this ticket I think that might help testers.
Thanks :)
#12
@
14 months ago
@nicolefurlan After some more thinking, this may be okay. I can't seem to find a plugin that would be susceptible to this, and for those that use a modal, the $interim_login
global should be set to a truthy value. When this is the case, the language selector isn't shown.
Pinging @joedolson who may have more knowledge on whether this patch is likely to cause any issues.
#14
@
14 months ago
Setting the action to site_url( 'wp-login.php' )
or wp_login_form()
will remove the redirect destination for the login, ie the redirect_to
query string parameter. On the reset password page, it will return users to the login form.
Rather than populate the action
attribute, I suggest it be removed as the default value is to use the current URL. This approach passes when run through validator.w3.org.
#15
@
14 months ago
- Keywords changes-requested added
- Milestone changed from 6.4 to 6.5
Thank you for your input @peterwilsoncc!
Since this PR will need updating and then testing, let's bump it to 6.5.
Of course, if we feel that we can get this done by RC1 tomorrow, feel free to do so! :)
#16
@
13 months ago
- Keywords 2nd-opinion added; has-patch removed
IMO, I guess we should just remove the action attribute as it will end up using the current page's URL anyway. If that is done, we might need to update the same thing in other instances as well, where the action is set to an empty value.
- wp-login.php
- options-privacy.php
- widgets-form.php
#17
@
11 months ago
- Keywords has-patch added; dev-feedback changes-requested 2nd-opinion removed
Given the title of this ticket, I think it's fairly clear that it intends to cover all instances of invalid action attributes, even though the only example documented in the description is on wp-login.php. Patch 58226.2.diff removes the four empty action attributes I found in core (in the files noted by @rajinsharwar)
To test:
- Verify the action attribute is gone in the language selection control at /wp-login.php, and verify the form works as expected.
- Verify the action attribute is gone and you can create a new Privacy Policy page at Settings > Privacy
- Verify the action attribute is gone and you can change your Privacy Policy page at Settings > Privacy
- In the classic widget editor, verify that the form wrapping "Clear Inactive Widgets" does not have an action attribute, and that you are able to clear inactive widgets.
All tested and verified in Chrome/Windows 11.
#18
@
11 months ago
Thanks for updating the patch, @joedolson.
I've tested your patch in Firefox 121/macOS 12.7 and it works as expected.
On the classic widgets screen I tested both with and without JavaScript enabled.
Hello and thank you for opening this ticket,
I'd suggest to use something like
action="<?php echo esc_url( site_url( 'wp-login.php' ) ); ?>"