Make WordPress Core

Opened 11 months ago

Closed 2 months ago

#58226 closed defect (bug) (fixed)

HTML attribute "action" on element "form": Must be non-empty.

Reported by: malae's profile Malae Owned by: joedolson's profile 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)

58226.2.diff (1.7 KB) - added by joedolson 2 months ago.
Remove all empty form action attributes

Download all attachments as: .zip

Change History (22)

#1 @audrasjb
11 months ago

  • Component changed from HTML API to Login and Registration
  • Version 6.2 deleted

#2 @audrasjb
11 months ago

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' ) ); ?>"

#3 @krupalpanchal
11 months ago

  • Keywords needs-patch added

@Malae Would you like to add the patch for this?

This ticket was mentioned in PR #4572 on WordPress/wordpress-develop by bartkleinreesink.


9 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.


9 months ago

#6 @rajinsharwar
7 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.


6 months ago

#8 @joedolson
6 months ago

  • Owner set to joedolson
  • Status changed from new to accepted
  • Version set to 5.9

#9 @nicolefurlan
6 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 @shubhamsedani
6 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 @nicolefurlan
5 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 @costdev
5 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.

Last edited 5 months ago by costdev (previous) (diff)

#13 @nicolefurlan
5 months ago

Wonderful! Maybe this one is ready for commit :)

#14 @peterwilsoncc
5 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 @nicolefurlan
5 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 @rajinsharwar
4 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.

  1. wp-login.php
  2. options-privacy.php
  3. widgets-form.php

@joedolson
2 months ago

Remove all empty form action attributes

#17 @joedolson
2 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 @peterwilsoncc
2 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.

#19 @peterwilsoncc
2 months ago

  • Keywords commit added

#20 @joedolson
2 months ago

  • Component changed from Login and Registration to Administration

Changing component now that this is broader than just the login form.

#21 @joedolson
2 months ago

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

In 57295:

Administration: Remove empty form action attributes.

Remove the action attribute in the login language selector, privacy forms, and classic widget forms.

An empty action attribute is invalid HTML4 and unsupported HTML5. The action attribute is optional, but must have a valid URL when provided.

Props Malae, audrasjb, bartkleinreesink, nicolefurlan, shubhamsedani, costdev, peterwilsoncc, rajinsharwar, joedolson.
Fixes #58226.

Note: See TracTickets for help on using tickets.