Navigation Block: Properly decode URL-encoded links #46435

kozer · 2022-12-09T15:13:33Z

Refers to: #46159

What?

The link is encoded when adding a URL with a special character (eg: %) in a navigation block link. However, the URL is not decoded properly when visiting the page, breaking the navigation.

Why?

This pr solving the above issue, decoding the URL properly.

How?

This was an easy fix. Just using PHP's urldecode function, seems to solve the underlying issue.

Testing Instructions

Start Gutenberg's dev environment
Open site editor (Appearance -> Editor)
Add a navigation block
Add a link (eg: Example
Save changes
Preview the page
Click on the link you just added
You should be able to visit the link without problems.

Testing Instructions for Keyboard

Screenshots or screencast

Some context on why this bug was introduced.

User enters a new URL via the editor.
Before encodeURI was introduced the URL was stored as is.
After saving, when someone navigates to the page with the inserted URL, the URL is passed in the render_block_core_navigation_link function.
When rendered, the URL is passed through esc_url. esc_url uses the preg_replace function that seems to discard < and > characters:

	$url = preg_replace( '|[^a-z0-9-~+_.?#=!&;,/:%@$\|*\'()\[\]\\x80-\\xff]|i', '', $url );

Using encodeURI in a URL that has those characters, (eg: encodeURI('http:example.com?s=<>') encodes it, and produces an encoded URL (in the above example the URL is encoded to http:example.com?s=%3C%3E, and so, esc_url no longer strips out those characters.

This is the reason why encodeURI was introduced originally in #19679, and produce the bug we are now facing.

github-actions · 2022-12-09T15:13:58Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @kozer! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

kozer · 2022-12-09T15:16:33Z

@danielbachhuber when I run those tests using: npm run test:php the test fails.
It seems (from the echo command you see - will be removed after I fix this issue) that the URL isn't affected by my change.
I also checked the file inside the container and the change is there.
How is that possible? Any ideas?
Thanks!

danielbachhuber · 2022-12-09T15:45:21Z

when I run those tests using: npm run test:php the test fails.
It seems (from the echo command you see - will be removed after I fix this issue) that the URL isn't affected by my change.
I also checked the file inside the container and the change is there.
How is that possible? Any ideas?

@kozer For me, the tests load the version of render_block_core_navigation_link() in WordPress core:

https://github.com/WordPress/wordpress-develop/blob/0cb8475c0d07d23893b1d73d755eda5f12024585/src/wp-includes/blocks/navigation-link.php#L123-L238

One thing that might not be obvious: the JavaScript and PHP in this repository is regularly synced into WordPress core.

It looks like there was a function rename in #40657

@adamziel @gziolo Any suggestions on how we should proceed? Should we submit this change against WordPress core instead?

kozer · 2022-12-09T17:55:14Z

@danielbachhuber I replaced wp.org file with the change I made here inside the WordPress container, and re-run the tests. They passed.
So your theory was correct. I'm waiting for @adambasa-dp and @gziolo's response.

danielbachhuber · 2022-12-09T18:08:57Z

@kozer While we're waiting, can you update the pull request description with details on the fix, how to test, etc?

danielbachhuber · 2022-12-19T17:25:23Z

Asking about how to proceed in Slack https://wordpress.slack.com/archives/C02QB2JS7/p1671470350718839

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

kozer · 2022-12-20T09:05:57Z

@danielbachhuber it seems that e2e tests are not passing, however, I don't see how my change is relevant. Should I update the test to make it pass?

adamziel · 2022-12-20T10:59:50Z

#46662 looks like a reasonable way forward. In hindsight I don't think removing the prefix wasn't a good decision. Only the e2e tests are failing now. They tend to be flakey so I just restarted them 🤞

danielbachhuber

Interestingly, the behavior works as expected on trunk and breaks for me on this PR.

I opened the demonstration link from the original issue, and then copied the link from my browser:

https://api.payaconnect.com/hostedpaymentpage?id=11eacdb9c9fc0d2c989d2057&data=U2FsdGVkX18xMWVhY2RiOa8xisZ0ulzB%2FzlaRnFN0dWAM1rLyZL%2BQpUc9oo2fp616ICqcykrs%2BjPzDfEPLd%2FZA%3D%3D

It ends up stored in the database as this:

<!-- wp:navigation-link {"label":"Sample Page","id":2,"url":"https://wordpress-develop.test/?page_id=2","kind":"post-type","isTopLevelLink":true} /-->

<!-- wp:navigation-link {"label":"Testing5","url":"https://api.payaconnect.com/hostedpaymentpage?id=11eacdb9c9fc0d2c989d2057\u0026data=U2FsdGVkX18xMWVhY2RiOa8xisZ0ulzB%2FzlaRnFN0dWAM1rLyZL%2BQpUc9oo2fp616ICqcykrs%2BjPzDfEPLd%2FZA%3D%3D","kind":"custom","isTopLevelLink":true} /-->

On this branch, the rendered HTML ends up as this:

<a class="wp-block-navigation-item__content"  href="https://api.payaconnect.com/hostedpaymentpage?id=11eacdb9c9fc0d2c989d2057&data=U2FsdGVkX18xMWVhY2RiOa8xisZ0ulzB/zlaRnFN0dWAM1rLyZL+QpUc9oo2fp616ICqcykrs+jPzDfEPLd/ZA=="><span class="wp-block-navigation-item__label">Testing5</span></a>

Clicking on the link produces this error:

On trunk, the rendered HTML ends up as this:

<a class="wp-block-navigation-item__content"  href="https://api.payaconnect.com/hostedpaymentpage?id=11eacdb9c9fc0d2c989d2057&#038;data=U2FsdGVkX18xMWVhY2RiOa8xisZ0ulzB%2FzlaRnFN0dWAM1rLyZL%2BQpUc9oo2fp616ICqcykrs%2BjPzDfEPLd%2FZA%3D%3D"><span class="wp-block-navigation-item__label">Testing5</span></a>

Clicking the link opens the page as expected.

I was curious why this data is escaped in the first place, given JSON serialization should be sufficient. It looks like it was added in #19679

Can you figure out these discrepancies? It'd be great to have more test cases too, covering the various different URLs we need to support.

kozer · 2022-12-21T10:55:38Z

@danielbachhuber , this should not happen now. It's fixed. The problem was that we were testing the same thing, differently on the same component.

More specifically, textControl was calling setAttributes to set the URL (and this wasn't encoding the URL), but adding the URL the way I did in the pr was calling updateAttributes (which was escaping the URL).

Given that I have to propose the following:

replace all direct calls to setAttributes to updateAttributes
update the function name to processAttributes or something, to make it more apparent that this has to be called every time we want to send some props outside of the component. By doing that, we will make sure that all the data that will "go out" from this component, will be processed as needed.

danielbachhuber · 2022-12-21T12:03:57Z

@kozer Can you make sure there are test cases that cover the before and after of your changes, as well as the variety of links we want to support?

kozer · 2022-12-23T16:53:52Z

@adamziel @gziolo I update the pr trying to have a fallback for users that have stored URLs that are not encoded. I also updated the tests.
If you think this is enough, let's go ahead and merge it.
I'll be off the next week, however, @sejas is aware of the ticket if anything comes up.

danielbachhuber · 2023-01-03T12:48:38Z

packages/block-library/src/navigation-link/index.php

+        return rawurldecode($url);
+    }
+    return $url;
+}


Can we consolidate these two functions into one function called block_core_navigation_link_maybe_urldecode()?

For better or for worse, maybe_* functions are a common pattern in WordPress core: https://developer.wordpress.org/?s=maybe

It seems like there are some PHPCS issues that need to be fixed too.

I prefer having them as two separate functions, as it's clear what each one of them is doing, and we can reuse is_url_encoded for testing encoded URLs elsewhere if we need to instead of tying those two functions together and producing a "side effect" if the URL is encoded.

However, I'll rename the urldecode_once to maybe_urldecode to match the common pattern in WordPress core.

Thanks for pointing this out!

It seems like there are some PHPCS issues that need to be fixed too.

Can you elaborate a bit more on this? I don't see any PHPCS issues locally for those two functions.

@kozer Given this will eventually be included in WordPress core, I'd like them consolidated to one function to match the pattern already established in WordPress core.

@danielbachhuber Doesn't the maybe_urldecode renaming establish that pattern?

@kozer If it's only one block_core_navigation_link_maybe_urldecode() function, then yes 😊

In the context of a large open source project, we shouldn't introduce two functions if we only need one.

@danielbachhuber I made the change as requested!
A side note:
I'm not sure if the phpcs errors you mentioned are related to tabs vs spaces. Neither vscode or neovim show any warnings/errors to me.
Are tabs the way to go in PHP-related files?

I'm not sure if the phpcs errors you mentioned are related to tabs vs spaces. Neither vscode or neovim show any warnings/errors to me.
Are tabs the way to go in PHP-related files?

@kozer Here's the failing test: https://github.com/WordPress/gutenberg/actions/runs/3830602130/jobs/6518686126

The failing test runs npm run lint:php:

You can run npm run lint:php in your local environment to see all of the same errors.

I use this VS Code extension to integrate PHPCS:

Name: PHP Sniffer & Beautifier
Id: valeryanm.vscode-phpsab
Description: PHP Sniffer & Beautifier for Visual Studio Code
Version: 0.0.15
Publisher: Samuel Hilson
VS Marketplace Link: https://marketplace.visualstudio.com/items?itemName=ValeryanM.vscode-phpsab

You can see all of WordPress' PHP coding standards here: https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/

danielbachhuber · 2023-01-03T12:55:20Z

packages/block-library/src/navigation-link/update-attributes.js

@@ -89,7 +89,7 @@ export const updateAttributes = (

 	setAttributes( {
 		// Passed `url` may already be encoded. To prevent double encoding, decodeURI is executed to revert to the original string.
-		...( newUrl && { url: encodeURI( safeDecodeURI( newUrl ) ) } ),
+		...{ url: newUrl ? encodeURI( safeDecodeURI( newUrl ) ) : newUrl },


I'm still not super clear why it was necessary to encodeURI() here in the first place. It seems like https://example.com?s=<> is JSON-encoded just fine:

>> var test = {}; undefined >> test.url = 'https://example.com?s=<>'; "https://example.com/?s=<>" >> JSON.stringify(test); "{\"url\":\"https://example.com/?s=<>\"}"

Maybe because the <> gets stripped out by kses? Can you debug and document why #19679 was necessary in the first place? If it's not necessary, maybe we can remove it entirely.

@danielbachhuber you were right. The reason that this was stripped out is because of esc_url function that uses kses under the hood.
So I think this is the way to go here.

The reason that this was stripped out is because of esc_url function that uses kses under the hood.

@kozer For posterity, can you provide a full step-by-step documentation of what's going on?

For sure @danielbachhuber! So, the process is the following:

User enters a new URL via the editor.

Before encodeURI introduced the URL was stored as is.

After saving, when someone navigates to the page with the inserted URL, the URL is passed in the render_block_core_navigation_link function.

When rendered, the URL is passed through esc_url. esc_url uses the preg_replace function that seems to discard < and > characters:

$url = preg_replace( '|[^a-z0-9-~+_.?#=!&;,/:%@$\|*\'()\[\]\\x80-\\xff]|i', '', $url );

Using encodeURI in a URL that has those characters, (eg: encodeURI('http:example.com?s=<>') encodes it, and produce an encoded URL (in the above example the URL is encoded to http:example.com?s=%3C%3E, and so, esc_url no longer strips out those characters.

This is the reason why encodeURI was introduced originally in #19679, and produce the bug we are now facing.

I also put those steps in the description of this pr. Given that, I assume this is ok to be merged.

danielbachhuber

Thanks for your persistence on this, @kozer !

I appreciate that you updated the PR description with "Some context on why this bug was introduced" too.

github-actions · 2023-01-05T20:32:22Z

Congratulations on your first merged pull request, @kozer! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

fix: issue with already encoded urls

f1f750c

kozer requested a review from tellthemachines as a code owner December 9, 2022 15:13

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Dec 9, 2022

fix: test

8256ad3

kozer changed the title ~~fix: issue with already encoded urls~~ fix Navigation Block: Special characters in Links Are Improperly decoded Dec 12, 2022

Merge branch 'WordPress:trunk' into fix_navigation_block_encoded_links

4283088

kozer mentioned this pull request Dec 19, 2022

Navigation Block: Percentage Character (%) in Links Are Improperly Encoded #46159

Closed

danielbachhuber added [Type] Bug An existing feature does not function as intended [Block] Navigation Link Affects the Navigation Link Block labels Dec 19, 2022

danielbachhuber mentioned this pull request Dec 19, 2022

Restore gutenberg_ prefix to function calls in PHPUnit tests #46662

Merged

danielbachhuber reviewed Dec 19, 2022

View reviewed changes

phpunit/class-block-library-navigation-link-test.php Outdated Show resolved Hide resolved

kozer added 2 commits December 20, 2022 10:01

fix: test

4ae395e

Merge branch 'WordPress:trunk' into fix_navigation_block_encoded_links

8150dad

danielbachhuber self-requested a review December 20, 2022 12:40

danielbachhuber reviewed Dec 20, 2022

View reviewed changes

fix: Always encode a url before saving

4704426

Try to decode only urls that are encoded

3a29c0e

danielbachhuber assigned kozer Jan 1, 2023

Merge branch 'WordPress:trunk' into fix_navigation_block_encoded_links

3505a5f

kozer requested review from danielbachhuber and removed request for tellthemachines January 3, 2023 11:03

danielbachhuber reviewed Jan 3, 2023

View reviewed changes

danielbachhuber requested a review from WunderBart January 3, 2023 13:11

kozer added 4 commits January 3, 2023 16:52

change the naming to match WordPress core naming pattern

9430754

change the naming to match WordPress core naming pattern

4cfabbb

fix: Fix lint issues

2cb90f0

Merge branch 'WordPress:trunk' into fix_navigation_block_encoded_links

ddd1f9e

kozer requested review from danielbachhuber and removed request for WunderBart January 5, 2023 08:54

danielbachhuber changed the title ~~fix Navigation Block: Special characters in Links Are Improperly decoded~~ Navigation Block: Properly decode URL-encoded links Jan 5, 2023

danielbachhuber approved these changes Jan 5, 2023

View reviewed changes

danielbachhuber added this to the Gutenberg 15.0 milestone Jan 5, 2023

danielbachhuber merged commit 622b829 into WordPress:trunk Jan 5, 2023

danielbachhuber mentioned this pull request Jan 5, 2023

Navigation Block: Percentage Character (%) in Links Are Improperly Encoded Automattic/wp-calypso#69839

Closed

t-hamano mentioned this pull request Oct 23, 2023

Custom Link: Decode value in URL input field #55549

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Navigation Block: Properly decode URL-encoded links #46435

Navigation Block: Properly decode URL-encoded links #46435

kozer commented Dec 9, 2022 •

edited

Loading

github-actions bot commented Dec 9, 2022

kozer commented Dec 9, 2022

danielbachhuber commented Dec 9, 2022

kozer commented Dec 9, 2022 •

edited

Loading

danielbachhuber commented Dec 9, 2022

danielbachhuber commented Dec 19, 2022

kozer commented Dec 20, 2022

adamziel commented Dec 20, 2022 •

edited

Loading

danielbachhuber left a comment

kozer commented Dec 21, 2022

danielbachhuber commented Dec 21, 2022

kozer commented Dec 23, 2022

danielbachhuber Jan 3, 2023

kozer Jan 3, 2023 •

edited

Loading

danielbachhuber Jan 3, 2023

kozer Jan 3, 2023

danielbachhuber Jan 3, 2023

kozer Jan 3, 2023

danielbachhuber Jan 3, 2023

danielbachhuber Jan 3, 2023

kozer Jan 4, 2023

danielbachhuber Jan 4, 2023

kozer Jan 5, 2023 •

edited

Loading

danielbachhuber left a comment

github-actions bot commented Jan 5, 2023

Navigation Block: Properly decode URL-encoded links #46435

Navigation Block: Properly decode URL-encoded links #46435

Conversation

kozer commented Dec 9, 2022 • edited Loading

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

Some context on why this bug was introduced.

github-actions bot commented Dec 9, 2022

kozer commented Dec 9, 2022

danielbachhuber commented Dec 9, 2022

kozer commented Dec 9, 2022 • edited Loading

danielbachhuber commented Dec 9, 2022

danielbachhuber commented Dec 19, 2022

kozer commented Dec 20, 2022

adamziel commented Dec 20, 2022 • edited Loading

danielbachhuber left a comment

Choose a reason for hiding this comment

kozer commented Dec 21, 2022

danielbachhuber commented Dec 21, 2022

kozer commented Dec 23, 2022

Choose a reason for hiding this comment

kozer Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kozer Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

danielbachhuber left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 5, 2023

kozer commented Dec 9, 2022 •

edited

Loading

kozer commented Dec 9, 2022 •

edited

Loading

adamziel commented Dec 20, 2022 •

edited

Loading

kozer Jan 3, 2023 •

edited

Loading

kozer Jan 5, 2023 •

edited

Loading