WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 months ago

Last modified 5 weeks ago

#32816 closed defect (bug) (fixed)

No or inadequate Custom Link URL validation

Reported by: JanR Owned by: westonruter
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords: needs-testing has-patch needs-screenshots ui-feedback
Focuses: ui, javascript Cc:

Description

When you add a Custom Link through the Menu Customizer, an invalid URL (non existing protocol for instance) is allowed, but stripped on the front page (by esc_url_raw?). An example of such an invalid URL protocol is: hhttps://myurl.com or http://;.

I would expect some some input validation and alert on error. The Menu location doesn't matter (Primary or Social Links). Please see attached images.

Attachments (12)

WordPress 4.3 Customizer Custom Links 02.png (531.4 KB) - added by JanR 2 years ago.
WordPress 4.3 Customizer Custom Links 01.png (69.9 KB) - added by JanR 2 years ago.
WP_Error_32816.PNG (19.0 KB) - added by RMarks 13 months ago.
WP_Error error message
32816.diff (685 bytes) - added by RMarks 13 months ago.
return a WP_Error when the return value of esc_url_raw() is empty
class-wp-customize-nav-menu-item-setting.diff (813 bytes) - added by EGregor 13 months ago.
Patch for invalid URL sanitization.
rmarks-32816-2.diff (1.1 KB) - added by RMarks 13 months ago.
Updated the sanitization test since url defaults to an empty string
32816-ataylor-js-validation.diff (2.3 KB) - added by andrew.taylor 13 months ago.
Here is a diff with @RMarks PHP validation adding JavaScript validation.
32816-egregor-fixes-tests.diff (3.5 KB) - added by EGregor 13 months ago.
Fixes to previous patch, tests.
32816.6.diff (5.2 KB) - added by westonruter 12 months ago.
#32816.patch (877 bytes) - added by umangvaghela123 6 months ago.
Fix #32816 issue
32816.2.diff (5.3 KB) - added by voldemortensen 2 months ago.
32816.10000.diff (6.8 KB) - added by westonruter 2 months ago.
Combine PHP validation with JS-based validation for adding new items

Download all attachments as: .zip

Change History (49)

#1 @stevenkword
2 years ago

  • Summary changed from No or inadequatee Custom Link URL validation to No or inadequate Custom Link URL validation

#2 @celloexpressions
2 years ago

  • Keywords needs-patch added

FWIW, I don't think the menus admin screen does this either. Would need to export the allowed protocols to JS.

#3 @celloexpressions
2 years ago

  • Milestone changed from Awaiting Review to Future Release

In a patch for this, I'd like to see the allowed protocols be available in JS and checked when items are added using the links section in the available items panel (currently it just checks for empty strings or http://, the default value, and prevents adding the item and adds a visual indicator that the option is invalid).

Not sure whether we could/should add validation for when links are changed in existing items, since the other fields there also get validated only when savee/previewed/on the server side, but adding better validation on the add-item field would be a nice improvement.

#4 @netweb
2 years ago

At the same this gets worked on, defaulting to https:// for HTTPS sites would be an additional bonus.

#5 @westonruter
13 months ago

  • Keywords good-first-bug added
  • Milestone changed from Future Release to 4.8

We can do this now easily with setting validation, where the sanitize method for WP_Customize_Nav_Menu_Item_Setting class can return a WP_Error when the return value of esc_url_raw() is empty. This will cause the setting to be blocked from being saved and the error message will be displayed in the control.

@RMarks
13 months ago

WP_Error error message

#6 @RMarks
13 months ago

@westonruter Is there a value that should be passed into WP_Error? I tried 'nav_menu_setting_failure' and it did highlight the bad menu item in red shown above.

This ticket was mentioned in Slack in #core-customize by rmarks. View the logs.


13 months ago

@RMarks
13 months ago

return a WP_Error when the return value of esc_url_raw() is empty

#8 @RMarks
13 months ago

  • Keywords has-patch needs-unit-tests added; needs-patch removed

@EGregor
13 months ago

Patch for invalid URL sanitization.

#9 @RMarks
13 months ago

  • Keywords needs-testing added; needs-unit-tests removed

#10 @celloexpressions
13 months ago

In addition to using setting validation, this should really be checked on the form where the custom URL is most often entered in the first place, in the available menu items panel. That would provide a better user experience by blocking invalid items from being added in the first place, and could be backed up with this setting validation in case users edit the value in the menu item properties.

There is already some basic JS validation on that input, making sure it isn't blank or left at the default http:// in case users forget to add a link. That should be extended to make similar checks to what will happen in php with the recent patches.

@RMarks
13 months ago

Updated the sanitization test since url defaults to an empty string

#11 @westonruter
13 months ago

  • Keywords needs-unit-tests added

Still needs unit tests. The JS validation is an optional enhancement.

@andrew.taylor
13 months ago

Here is a diff with @RMarks PHP validation adding JavaScript validation.

#12 @RMarks
13 months ago

FYI, @EGregor created a working unit test at WCUS Contributor Day. He said he'd post a diff later today.

@EGregor
13 months ago

Fixes to previous patch, tests.

#13 @EGregor
13 months ago

  • Keywords needs-unit-tests removed

Did some standards tweaks for @andrew.taylor's patch, added phpunit tests for @RMarks's work. Wasn't able to find qunit tests for the JS.

#14 @andrew.taylor
13 months ago

The URL JavaScript regex validation I used is from the jQuery validate source. It had an attribution to a GitHub Gist with an MIT license.

I included the attribution and according to this license compatibility list I think it's okay but wanted to comment in case it can't be used.

#15 @westonruter
12 months ago

The PHP-based sanitization/validation needs to remain, as it is only the server-side validation that ultimately matters for ensuring valid values are saved. The JS validation can be easily bypassed.

I'm wary of attempting to include a full URL sanitization in JS that completely matches and replicates what is in PHP. I think the JS validation should be very rudimentary, just checking for ^https?://.+.

Also, we need to ensure that mailto: links work.

#16 @westonruter
12 months ago

  • Keywords needs-patch added; has-patch removed

Another reason to defer to PHP-only validation is that calls to esc_url_raw() will return with the value passed through the clean_url filter, and so we have no idea at all what ultimately PHP via plugins will decide is a valid URL.

#18 @westonruter
12 months ago

@Presskopp This problem shouldn't be evident here because there is a whitelist of $protocols (wp_allowed_protocols()) that esc_url_raw() will allow.

#19 @westonruter
12 months ago

  • Keywords has-patch added; needs-patch removed

32816.6.diff adds more unit tests, removes the JS-validation since I think we should keep it DRY, and also adds styling to the URL field when there is an invalid_url error.

#20 @arvindsinghu
10 months ago

@celloexpressions  i totally agree you

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


7 months ago

#22 @jbpaul17
7 months ago

  • Milestone changed from 4.8 to 4.8.1

Per yesterday's bug scrub, we're going to punt this to 4.8.1.

#23 @westonruter
6 months ago

  • Milestone changed from 4.8.1 to 4.9

#24 @netweb
6 months ago

  • Owner set to westonruter
  • Status changed from new to assigned

This ticket has a patch, setting the owner to that of the person who added the good-first-bug keyword, this ticket update causes the ticket to be removed from the "Unclaimed" section of the Good First Bug Trac Report 44.

@umangvaghela123
6 months ago

Fix #32816 issue

#25 @umangvaghela123
6 months ago

  • Keywords good-first-bug removed

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


6 months ago

#27 @melchoyce
3 months ago

  • Keywords needs-screenshots ui-feedback added

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


3 months ago

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


3 months ago

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


2 months ago

#31 @westonruter
2 months ago

  • Type changed from enhancement to defect (bug)

#32 @voldemortensen
2 months ago

32816.2.diff adds another test case to @westonruter's patch.

@westonruter
2 months ago

Combine PHP validation with JS-based validation for adding new items

#33 @westonruter
2 months ago

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

In 41697:

Customize: Provide validation feedback for invalid Custom Link URLs in nav menu items.

Props RMarks, EGregor, umangvaghela123, andrew.taylor, celloexpressions, westonruter, voldemortensen.
Fixes #32816.

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


5 weeks ago

#35 @westonruter
5 weeks ago

Regression: #42506

#36 @westonruter
5 weeks ago

In 42153:

Customize: Restore ability to add Custom Link nav menu items for jump links and other URLs that were previously allowed.

Simplify regular expression for checking URL validity to just do basic checks to confirm the value looks like a URL. Leave the complete validation to the server-side logic in WP_Customize_Nav_Menu_Item_Setting::sanitize() to avoid having to maintain two separate codebases for validating URLs.

Props westonruter, SergeyBiryukov for testing.
Amends [41697].
See #32816.
Fixes #42506 for trunk.

#37 @westonruter
5 weeks ago

In 42154:

Customize: Restore ability to add Custom Link nav menu items for jump links and other URLs that were previously allowed.

Simplify regular expression for checking URL validity to just do basic checks to confirm the value looks like a URL. Leave the complete validation to the server-side logic in WP_Customize_Nav_Menu_Item_Setting::sanitize() to avoid having to maintain two separate codebases for validating URLs.

Props westonruter, SergeyBiryukov for testing.
Amends [41697].
See #32816.
Fixes #42506 for 4.9.

Note: See TracTickets for help on using tickets.