Fix: Allow text fragments in kses for Trac ticket #60347 by ShaneMuir · Pull Request #7890 · WordPress/wordpress-develop

ShaneMuir · 2024-11-25T15:32:21Z

This PR addresses the issue in Trac ticket #60347, where wp_kses() removes valid text fragment links (e.g., #:~:text=highlight) by treating them as invalid protocols.

Changes:

•	Updated wp_kses_bad_protocol() in kses.php to explicitly allow text fragments (#:~:text).
•	Added a unit test to verify that text fragments are preserved in the href attribute.

Testing:

1.	Run npm run test:php to confirm all tests pass.
2.	Apply the patch and manually test with an anchor tag containing #:~:text in the href attribute.
•	Example: <a href="#:~:text=highlight">Link</a> should pass validation.

Impact:

This change ensures that text fragment links are correctly preserved by wp_kses(), aligning with modern URL standards and avoiding unnecessary sanitization.

Trac ticket: https://core.trac.wordpress.org/ticket/60347

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

github-actions · 2024-11-25T15:32:41Z

Hi @ShaneMuir! 👋

Thank you for your contribution to WordPress! 💖

It looks like this is your first pull request to wordpress-develop. Here are a few things to be aware of that may help you out!

No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description.

Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making.

More information about how GitHub pull requests can be used to contribute to WordPress can be found in the Core Handbook.

Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook.

If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook.

The Developer Hub also documents the various coding standards that are followed:

Thank you,
The WordPress Project

github-actions · 2024-11-25T15:32:43Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @shane.muirhead@tribusdigital.com.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

Core Committers: Use this line as a base for the props when committing in SVN:

Props shanemuir, westonruter, azaozz.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2024-11-25T15:45:25Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

azaozz · 2025-03-26T17:03:22Z

 		( str_starts_with( $content, 'https://' ) && in_array( 'https', $allowed_protocols, true ) ) ||
-		( str_starts_with( $content, 'http://' ) && in_array( 'http', $allowed_protocols, true ) )
+		( str_starts_with( $content, 'http://' ) && in_array( 'http', $allowed_protocols, true ) ) ||
+		( str_starts_with( $content, '#:~:text' ) )


This will make URLs starting with a text fragment impossible to disable. Is there any specific reason for that? If not, thinking the last line should be replaced with:
( str_starts_with( $content, '#:~:text' ) && in_array( '#:~:text', $allowed_protocols, true ) )

Then the #:~:text string should be added to the default $allowed_protocols.

azaozz · 2025-03-26T17:06:59Z

 		( str_starts_with( $content, 'https://' ) && in_array( 'https', $allowed_protocols, true ) ) ||
-		( str_starts_with( $content, 'http://' ) && in_array( 'http', $allowed_protocols, true ) )
+		( str_starts_with( $content, 'http://' ) && in_array( 'http', $allowed_protocols, true ) ) ||
+		( str_starts_with( $content, '#:~:text' ) )


And a nitpick :)
Looking at the specs, should the identifier string be #:~:text or just #:~:?

I think you are correct here it could just be fragment directive I will make amends.

Thank you @azaozz for reviewing. I've made some amends based on your recommendations, added check so that the protocol can be disabled through removing the protocol from the default array. Removed the 'text' part of the directive string and added a test to check fragment directive is removed if removed from default protocol array.

azaozz · 2025-03-26T17:13:10Z

This patch looks generally okay. Left couple questions for possible changes/improvements.

Looking at the specs: https://wicg.github.io/scroll-to-text-fragment/#syntax and https://wicg.github.io/scroll-to-text-fragment/#the-fragment-directive it seems there aren't any security implications. URLs starting with #:~: should conform to the existing standards for URLs starting with #. On the other hand there is no bypass for the latter so thinking a more in depth review by the security team is in order.

…otocol() to ensure it checks for the fragment directive in allowed protocols. Added extra units tests to check fragment directive is removed if not in allowed protocols.

…ix/kses-allow-text-fragments

…ments. Remove the text fragment from since it was advised these aren't protocols. Updated unit tests

ShaneMuir · 2025-07-09T15:38:39Z

Updated PR on Ticket: #60347

westonruter · 2026-09-10T19:31:45Z

+	if ( is_allowed_text_fragment( $content ) ) {
+		return $content;
+	}


I don't see the need for introducing a new function for this. I think it should be just:

Suggested change

if ( is_allowed_text_fragment( $content ) ) {

return $content;

}

if ( str_starts_with( $content, '#:~:text=' ) ) {

return $content;

}

However, for completeness, should it also take into account $allowed_protocols? What if you called wp_kses_bad_protocol( $content, array( 'mailto' ) )? This should get rightfully rejected.

westonruter · 2026-09-10T19:31:56Z

+
+/**
+ * Checks if a content string is a valid text fragment link.
+ *
+ * This function handles the text fragment links (e.g., #:~:text=highlight) separately.
+ *
+ * @param string $content
+ * @return bool
+ */
+function is_allowed_text_fragment( string $content ): bool {
+	return str_starts_with( $content, '#:~:text=' );
+}


Suggested change

/**

* Checks if a content string is a valid text fragment link.

*

* This function handles the text fragment links (e.g., #:~:text=highlight) separately.

*

* @param string $content

* @return bool

*/

function is_allowed_text_fragment( string $content ): bool {

return str_starts_with( $content, '#:~:text=' );

}

westonruter · 2026-09-10T19:33:14Z

+	/**
+	 * Test to ensure wp_kses() allows text fragment links.
+	 *
+	 * This test verifies that wp_kses() correctly handles and preserves
+	 * text fragment links (e.g., #:~:text=highlight) in the href attribute.
+	 *
+	 * @ticket 60347
+	 *
+	 * @return void
+	 */
+	public function test_wp_kses_allows_text_fragments() {
+		$html         = '<a href="#:~:text=highlight">Text Fragment</a>';
+		$allowed_html = array(
+			'a' => array(
+				'href' => true,
+			),
+		);
+
+		$result = wp_kses( $html, $allowed_html );
+		$this->assertSame( $html, $result );
+	}


Let's refactor this to use a data provider. The data provider should include cases for:

#:~:text=highlight

/absolute/path/#:~:text=highlight

https://example.com/#:~:text=highlight

Maybe others.

Also, it should check for non-text fragment anchor links as well, like #footer.

westonruter · 2026-09-10T19:34:01Z

+		$this->assertNotSame( $html, $result );
+		$this->assertStringNotContainsString( 'javascript:', $result );


Let's use assertEqualHTML here.

westonruter · 2026-09-10T19:34:40Z

+		);
+
+		$result = wp_kses( $html, $allowed_html );
+		$this->assertSame( $html, $result );


Using assertSame works, but assertEqualHTML is probably better.

ShaneMuir · 2026-09-11T16:07:22Z

Updated the PR to address @westonruter’s review comments: 3b3ad66

…fragments

dhrupo

Tested 374f911. The earlier review feedback looks addressed: data provider, assertEqualHTML, the #footer case, and honouring $allowed_protocols.

Results (PHP 7.4.33, MySQL 8.4.5):

--group 60347: 16/16 pass. Run against trunk's kses.php, 7 of them fail, so the tests cover the bug.
--group kses: 486 tests pass.
PHPCS (--warning-severity=1) on both files: clean.
javascript:, javascript:, javascript&colon;, javascript: and : (with and without the semicolon), a tab inside the scheme, and data: are all still stripped when followed by #:~:text=.

One gap: a relative URL with more than one query parameter is still broken, because [^:&]* stops at the first &:

/page?a=1&b=2#:~:text=foo   → href="text=foo"
?p=1&lang=en#:~:text=foo    → href="text=foo"

Excluding & is what keeps encoded colons out, so the suggestion below only excludes the & sequences that can produce a colon (&#… numeric references and &colon;). With it, both URLs above are preserved, every encoded-scheme case above is still stripped, and the existing 16 tests and the kses group still pass.

Possible extra data provider cases:

'relative URL with query string'   => array(
	'/page?a=1&b=2#:~:text=highlight',
	'/page?a=1&amp;b=2#:~:text=highlight',
	array( 'http', 'https' ),
),
'named entity colon before fragment' => array(
	'javascript&colon;alert(1)#:~:text=highlight',
	'text=highlight',
	array( 'http', 'https' ),
),
'hex entity colon before fragment'   => array(
	'javascript&#x3a;alert(1)#:~:text=highlight',
	'text=highlight',
	array( 'http', 'https' ),
),

Not for this PR, but related: a fragment containing a colon that isn't a text directive, such as #section:2 or /page#section:2, still comes out as 2 (same on trunk). That seems worth its own ticket.

AI disclosure: I used Claude Code to help write the probe cases and draft this review. I ran and checked every result above myself.

dhrupo · 2026-10-16T04:52:51Z

+	// Preserve relative text fragment URLs when web protocols are allowed.
+	if (
+		( in_array( 'http', $allowed_protocols, true ) || in_array( 'https', $allowed_protocols, true ) ) &&
+		preg_match( '/^[^:&]*#:~:text=/', $content )


Allows & (e.g. a query string) before the fragment while still rejecting &#… and &colon;, which can decode to a colon:

Suggested change

preg_match( '/^[^:&]*#:~:text=/', $content )

preg_match( '/^(?:[^:&]|&(?!#|colon;))*#:~:text=/i', $content )

Fix: Allow text fragments in kses for Trac ticket #60347

080885e

azaozz reviewed Mar 26, 2025

View reviewed changes

Shane Muirhead and others added 6 commits March 27, 2025 12:36

Fix: Allow text fragments in kses for Trac ticket #60347

33105b2

Added fragment directive to default protocols. Updated wp_kses_bad_pr…

f9ddc00

…otocol() to ensure it checks for the fragment directive in allowed protocols. Added extra units tests to check fragment directive is removed if not in allowed protocols.

Merge branch 'trunk' of github.com:ShaneMuir/wordpress-develop into f…

57777e1

…ix/kses-allow-text-fragments

Merge

8032d86

60347 - Updated kses.php to include independent handling of text frag…

067c31d

…ments. Remove the text fragment from since it was advised these aren't protocols. Updated unit tests

Added line back that was removed

17a0b10

pento force-pushed the trunk branch from 3af2d27 to 64ede28 Compare July 6, 2026 03:10

Merge branch 'trunk' into fix/kses-allow-text-fragments

9a01f29

westonruter requested changes Sep 10, 2026

View reviewed changes

Refine KSES text fragment handling and tests

3b3ad66

Merge remote-tracking branch 'origin/trunk' into fix/kses-allow-text-…

374f911

…fragments

dhrupo reviewed Oct 16, 2026

View reviewed changes

dhrupo mentioned this pull request Oct 16, 2026

Formatting: Keep URLs whose colon follows a path, query or fragment in KSES. #14320

Open

		$this->assertNotSame( $html, $result );
		$this->assertStringNotContainsString( 'javascript:', $result );

	preg_match( '/^[^:&]*#:~:text=/', $content )
	preg_match( '/^(?:[^:&]\|&(?!#\|colon;))*#:~:text=/i', $content )

Conversation

ShaneMuir commented Nov 25, 2024

Uh oh!

github-actions Bot commented Nov 25, 2024

Uh oh!

github-actions Bot commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unlinked Accounts

Uh oh!

github-actions Bot commented Nov 25, 2024

Test using WordPress Playground

Some things to be aware of

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azaozz commented Mar 26, 2025

Uh oh!

ShaneMuir commented Jul 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShaneMuir commented Sep 11, 2026

Uh oh!

dhrupo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions Bot commented Nov 25, 2024 •

edited

Loading