Fix url escaping for array parameters in Navigation links

[draganescu]

What?

Fixes #58020

Why?

It's a bug fix.

How?

Check if the parameter is a string before attempting to url encode /decode.

Testing Instructions

• in a post
• in a navigation block
• add a link
• set the link to 'https://example.com/index.php?param[0]=test&param[1]=another-test'`
• save the saving should work
• preview the post
• the post should render with no errors

Testing Instructions for Keyboard

N/A

Screenshots or screencast

N/A

[github-actions]

Flaky tests detected in aaf5586.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7711192512
📝 Reported issues:

• [Flaky Test] directives inside islands should not be hydrated twice #58332 in /test/e2e/specs/interactivity/tovdom-islands.spec.ts

[getdave]

cc @mreishus - does this resolve the Issue you reported? 🙏

[ironprogrammer]

Thanks for the patch, @draganescu! This bug has the potential to bring down a site's frontend if the problematic nav isn't addressed immediately.

Test Report

Environment

• Hardware: MacBook Pro Apple M1 Pro
• OS: macOS 14.3
• Browser: Safari 17.3
• Server: nginx/1.25.3
• PHP: 8.3.2
• WordPress: 6.5-alpha-56966-src
• Theme: twentytwentyfour v1.0
• Active Plugins:
  • gutenberg v17.6.0-rc.2

Actual Results

Fatal no longer occurs when adding link with array query params via:

• ✅ Adding or modifying the link in the site editor navigation.
• ✅ Adding the link in the post editor (Navigation block).

[mreishus]

cc @mreishus - does this resolve the Issue you reported? 🙏

Sure does! Thank you @draganescu!

[anton-vlasenko]

The code looks good to me, but the function requires unit tests and further improvements.
I attempted to intentionally cause it to fail, and here are the results I got:

// See the results below.
var_export( block_core_navigation_link_maybe_urldecode( $url ) );

// 1.
$url = "http://www.example.com/example%20path?param=value";
// Result:
'http://www.example.com/example%20path?param=value' // Not decoded.

// 2.
$url = "http://www.example.com/example%20path?param=value%20with%20space";
// Result:
'http://www.example.com/example%20path?param=value%20with%20space' // Not decoded.

// 3.
$url = "http://www.example.com/path?arrayParam[]=value1&arrayParam[]=value2";
// Result:
Fatal error: Uncaught TypeError: rawurldecode(): Argument #1 ($string) must be of type string, // Fatal error!

// 4.
$url = null;
// Result:
Deprecated: parse_url(): Passing null to parameter #1 ($url) of type string is deprecated in src/wp-includes/blocks/navigation-link.php on line 131
NULL // PHP warning.

[getdave]

Agreed. We need to land this one asap so it's in Gutenberg prior to 17.6.0 on 31st Jan.

Confirming that it will not require a backport in order to land in WP 6.5 Beta 1 because the PHP code resides within the block-library package and thus will be automatically synced.

[draganescu]

While my patch fixes the issue, indeed looking at @anton-vlasenko 's tests I doubt the way the function works:it checks only if one of the params of the URL is encoded and then if yes returns the whole URL decoded.

Even the doc comment is misleading "Decodes a url if it's encoded, returning the same url if not." since that would be true if we'd have a check like:

PHP
function block_core_navigation_link_maybe_urldecode( $url ) {
...
  if ( rawurldecode( $url ) === $url ) {
    return;
  }
}

which we don't - we return the fully decoded URL only if one of the params are encoded. Moreover, in an URL like http://www.example.com/example%20path?param=value%20with%20space the call for wp_parse_args on the query args, decodes the args because it internally uses PHP's parse_strwhich automagically urldecode-s the values.

I wonder what was the problem with a function like:

function block_core_navigation_link_maybe_urldecode( $url ) {
	if ( ! is_string( $url ) ) {
		return $url;
	}

	if ( rawurldecode( $url ) === $url ) {
		return $url;
	}

	return rawurldecode( $url );
}

Since in the block encodeURI is applied to the full URL I am not clear on why not the above. @kozer @danielbachhuber if it rings any bells that'd be great.

OTOH for the purposes of landing this sooner rather than later since it fixes a bug that can take down the front end @anton-vlasenko do you think we have any blockers?

[anton-vlasenko]

OTOH for the purposes of landing this sooner rather than later since it fixes a bug that can take down the front end @anton-vlasenko do you think we have any blockers?

@draganescu Yes, I didn't mean to block your PR. Rather, I wanted to point out the issues that I found. Sure, I think the fixes for the issues mentioned in my previous comment can be implemented as a follow-up task when there is enough time for it.

Also, adding PHPUnit tests would be good, but, IMO, that can also wait for a future task.

[github-actions]

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files

❔ phpunit/class-block-library-navigation-link-test.php

[anton-vlasenko]

The code looks good to me.

[peterwilsoncc]

@draganescu Were array parameters with url encoded values tested with this, something along the lines of example.com?q[]=lzB%2Fzd%FZA%3D%3D&q[]=lzB%2Fzd%FZA%3D%3D?

Sorry the question came in after the commit, there's a lot to monitor so I tend to track commits rather than try and catch all the things as they're in PRs.

[draganescu]

@peterwilsoncc I tested and the URL you offered as an example is saved as is, which should be ok, right?