No or inadequate Custom Link URL validation

[JanR]

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.

[celloexpressions]

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

[celloexpressions]

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.

[netweb]

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

[westonruter]

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]

WP_Error error message

[RMarks]

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

[RMarks]

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

[EGregor]

Patch for invalid URL sanitization.

[celloexpressions]

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]

Updated the sanitization test since url defaults to an empty string

[westonruter]

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

[andrew.taylor]

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

[RMarks]

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

[EGregor]

Fixes to previous patch, tests.

[EGregor]

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.

[andrew.taylor]

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.

[westonruter]

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.

[westonruter]

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.

[Presskopp]

related: #37758

[westonruter]

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

[westonruter]

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.

[arvindsinghu]

@celloexpressions  i totally agree you

[jbpaul17]

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

[netweb]

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]

Fix #32816 issue

[voldemortensen]

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

[westonruter]

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

[westonruter]

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.

[westonruter]

Regression: #42506

[westonruter]

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.

[westonruter]

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.