![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png){kind=logo}
Opened 5 weeks ago
Last modified 3 weeks ago
#65448 new enhancement
Use the new Uri\Rfc3986\Uri::parse in wp_parse_url() if PHP 8.5 is available
| Reported by: | zodiac1978 | Owned by: | |
|---|---|---|---|
| Priority: | normal | Milestone: | Awaiting Review |
| Component: | Security | Version: | |
| Severity: | normal | Keywords: | changes-requested has-patch |
| Cc: | Focuses: |
Description
After seeing the talk from Milan Petrović at WordCamp Europe 2026 in Krakow, I was wondering if we should follow his recommendation to use the new Uri\Rfc3986\Uri::parse (available since PHP 8.5) instead of parse_url() if it is available.
Code snippet: https://github.com/dev4press/vulnerability-lab/blob/main/forms/tab-ssrf.php
As we already have a wrapper function for this with wp_parse_url() we could internally use the new PHP 8.5 feature if it is there to make this even more safe and reliable. This could prevent SSRF attacks.
I'm just not sure if something maybe is trusting the old feature set and does not expect that malformed URLs would break with this change. We could add a new parameter for opting in to this (or opt-out). Another idea could be to add this change very early in the release cycle, to get early feedback if things break.
What do you think?
Attachments (5)
Change History (20)
#2
in reply to: ↑ 1
@
5 weeks ago
Replying to dmsnell:
[...] We want to use the
\Uri\WhatWg\Urlclasses instead [...]
The parse_url() page on PHP.net is recommending both, but provided no more insights which is to use in which case and as Milan suggested Uri\Rfc3986\Uri::parse I was using this, but your explanation sounds good. I'm fine with Uri\WhatWg\Url.
To that end, and given the utility of the classes, I don’t think the best approach will end up being
wp_parse_url()but per-callsite uses of\Uri\WhatWg\Url. Still, updating that class to at least eliminate the most-egregious parse failures is welcome.
As we are recommending the usage of wp_parse_url() via WordPress Coding standards, I think fixing it in the wrapper would be the easiest way for everyone (core, plugins, themes) to just get the best and safest version for parsing without changing their code, as we should use wp_parse_url() everywhere.
If you are volunteering to work on this, I recommend conditionally defining
wp_parse_url()based on the existence of theWhatWgclass, similar to howutf8_decode()is conditionally defined based on the presence of thembstringextension
I'm not sure if I understand this paragraph correctly. Do you mean to check if the WhatWg class is available and if yes, use it in wp_parse_url() instead of using parse_url()? Because this is exactly what I am suggesting.
#3
@
5 weeks ago
More context from https://www.php.net/manual/en/function.parse-url.php
Caution
This function does not follow any established URI or URL standard. It will return incorrect or non-sense results for relative or malformed URLs. Even for valid URLs the result may differ from that of a different URL parser, since there are multiple different URL-related standards that target different use cases and that differ in their requirements.
Processing an URL with parsers following different URL standards is a common source of security vulnerabilities. As an example, validating an URL against an allow-list of acceptable hostnames with parser A might be ineffective when the actual retrieval of the resource uses parser B that extracts hostnames differently.
The Uri\Rfc3986\Uri and Uri\WhatWg\Url classes strictly follow the RFC 3986 and WHATWG URL Standards respectively. It is strongly recommended to use these classes for all newly written code and to migrate existing uses of the parse_url() function to these classes, unless the parse_url() behavior needs to be preserved for compatibility reasons.
#4
@
5 weeks ago
- Keywords has-patch dev-feedback added; needs-patch removed
The patch is untested and quickly made with help from ChatGPT. I just want to check if this is the way forward here or not.
#5
@
4 weeks ago
Hi @zodiac1978 — thanks for working on this. You might want to fork wordpress/wordpress-develop and create a PR for this, referencing Trac ticket: Core-65448 in the description, and then the test suite will run against it.
While the code in the patch will work, it imposes an unnecessary runtime cost on every invocation, which is to check if the class exists, but we only ever expect the class to exist or not at the start and never be added later. So take a look at how the utf8_decode() polyfill in src/wp-includes/compat.php is conditionally defined based on the presence of mbstring and only pays the cost of that once, on initialization.
The parse_url() page on PHP.net is recommending both, but provided no more insights which is to use in which case
This is because each standard should be used in the situations in which they are relevant. Since we are working with web browsers and receive paths from web browsers, WhatWG is what we will use to make sure that WordPress understands what those browsers send, and sends what browsers will interpret.
There are nuance differences in handling things like empty path segments, percent-encoding, authentication parts, and failure modes. If we end up disagreeing with the browser on these things we open up opportunities to exploit those differences in unwanted ways.
There could be parts of WordPress where RFC 3986 URLs are appropriate, but I’m not sure where they are. It would likely be in something like an XML exchange with a well-specified message, or some JSON interchange format, again with a well-specified URL field listed as RFC 3986 rather than just "URL" or a WHAT-WG URL. This duality is generally confusing and I think there is insufficient material online to clarify; the RFC follows a more idealistic pursuit to normalize what should exist, while the WHAT-WG is more pragmatic and oriented towards ensuring that everyone agrees on what actually exists.
#6
@
4 weeks ago
- Keywords changes-requested added; has-patch dev-feedback removed
Hi @dmsnell
it is so great to get such detailed and timely feedback. Thank you!
I will look at the implementation and will work on a new patch.
#7
follow-up:
↓ 8
@
4 weeks ago
To be honest, I don't think this is really a good idea. parse_url() and \Uri\WhatWg\Url::parse() are quite different functions, you can't really just replace one with the other.
For example, \Uri\WhatWg\Url::parse() doesn't seem to handle relative URLs at all? (Unless you pass a second argument to it.)
<?php $url = '/foo'; $parsed = parse_url( $url ); var_dump( $parsed ); $parsed = \Uri\WhatWg\Url::parse( $url ); var_dump( $parsed );
If I run the above I get:
array(1) {
["path"]=>
string(4) "/foo"
}
NULL
If I try to apply the suggested patch and run the test suite, it causes a huge number of failures:
$ vendor/bin/phpunit --filter Tests_HTTP_HTTP ... ..FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF 63 / 122 ( 51%) FFFFFFFFFFFFFFF..FFF....................................... 122 / 122 (100%) ... There were 79 failures...
#8
in reply to: ↑ 7
;
follow-up:
↓ 10
@
4 weeks ago
Replying to siliconforks:
To be honest, I don't think this is really a good idea.
The PHP documentation is quite clear, that everything should use the new function and all code should be migrated and parse_url should only be used for compatibility reasons.
It is strongly recommended to use these classes for all newly written code and to migrate existing uses of the parse_url() function to these classes, unless the parse_url() behavior needs to be preserved for compatibility reasons.
From: https://www.php.net/manual/en/function.parse-url.php
If I try to apply the suggested patch and run the test suite, it causes a huge number of failures:
As the new function is more restrictive, this is correct and expected. Those tests need to be revisited if there are still relevant.
I have a new patch (created with Codex) to fix the relative URLs.
This ticket was mentioned in PR #12205 on WordPress/wordpress-develop by @zodiac1978.
4 weeks ago
#9
- Keywords has-patch added
Refactor wp_parse_url to use WHATWG URL parser introduced in PHP 8.5.
Trac ticket: https://core.trac.wordpress.org/ticket/65448
Trac ticket: Core-65448
## Use of AI Tools
AI assistance: Yes
Tool(s): Codex
Model(s): GPT-5.5 (Medium)
Used for: Implementation - reviewed and edited by me.
#10
in reply to: ↑ 8
@
4 weeks ago
Replying to zodiac1978:
The PHP documentation is quite clear, that everything should use the new function and all code should be migrated and
parse_urlshould only be used for compatibility reasons.
The documentation is not suggesting you can simply replace each call to parse_url() with \Uri\WhatWg\Url::parse() and it will work in every case. They are two different functions.
As the new function is more restrictive, this is correct and expected.
The problem is that wp_parse_url() is part of the public API of WordPress. If you make it more restrictive, that will break backward compatibility.
Those tests need to be revisited if there are still relevant.
It's not just a matter of fixing tests - there are places in WordPress which call wp_parse_url() with a relative URL and expect it to work. (For example, see WP_Http::make_absolute_url().) You could modify those places too, but even fixing those would not be sufficient - for example, there are over 5000 plugins in the plugin directory using wp_parse_url(). Some of those are probably expecting it to work with relative URLs.
#11
@
4 weeks ago
Some of those are probably expecting it to work with relative URLs.
Try the latest patch from GitHub. It *is* working now with relative URLs.
#12
@
4 weeks ago
@siliconforks @zodiac1978 part of the basic requirement for updating the function is to maintain the intended interface for wp_parse_url(), which means that relative URLs need to be handled. that was never going to slip through, but thank you for raising the issue explicitly.
We should be able to pass a sentinel base URL in to Url::parse( $given_uri, 'https://example.com' ) and then perform a check if that came as part of the given URI or not. If it wasn’t, we can assume this is a relative URL. That may be wrong, but I think the general idea will hold.
That is to say, this should be a tractable problem we only need to make sure to implement.
The problem is that wp_parse_url() is part of the public API of WordPress. If you make it more restrictive, that will break backward compatibility.
@siliconforks this is always good to remember. Still, I think we might have to consider some backwards compatible changes for the good of the ecosystem where the existing behaviors are known to be broken and unintentional. We can target a patch early in the release cycle to maximize testing, but parse_url() is horribly broken and many of its existing behaviors are the cause of further breakage.
As with HTML parsing, this is going to lead us to choosing tradeoffs and we might have to examine some of the existing test cases and try to determine if they were intentional or accidental, whether they are worth revising. None of this will relate to the interface of what the function accepts or returns, but only on the understanding of the URLs.
One criteria we’ve used with HTML parsing is that if an existing breakage means that downstream code is also broken, we can take some liberty in fixing those cases because we can know for sure that something cannot be working with the existing code either. One example of this is how many functions conflate HTML tags and HTML comments; code modifying tags that receive an HTML comment fails no matter what, so it’s fine to update functionality to only return tags, or change behavior in some way. That’s me sharing the approach I take as I examine a lot of legacy systems which have varying levels of invalid behaviors.
@zodiac1978 commented on PR #12205:
4 weeks ago
#13
@dmsnell I am still learning some things with PHP, but hopefully I got your intention now.
I know that the PR is not ready. It seems that we could use an internal helper function to just replace the moving parts, as there is code above and below that stays.
And I'm not sure where to put the DocBlocks in such a case. WPCS in my editor is still complaining.
Very excited if I am on track now :)
@zodiac1978 commented on PR #12205:
4 weeks ago
#14
Hi @dmsnell
perhaps we are missing the point here and instead of defining two wp_parse_url() what we want is to create a new lower-level helper to do the URL parsing.
That was my idea too:
It seems that we could use an internal helper function to just replace the moving parts
I will work a new PR implementing this idea!
Thanks for the ticket, @zodiac1978. This is closely related to #53938 and something that myself, @jonsurrell, and @zieladam have all been working adjacent to. We shared design feedback from WordPress on the development of the new
Uriclasses and indeed, the goal is to migrate towards those updated interfaces.Given the dismal array of problems with
parse_url(), it’s my assessment that we should replace it and accept behavioral changes as bug fixes.However, RFC 3986 is not what I recommend as that creates a host of new problems which stem from the fact that RFC 3986 URLs are not useful when communicating with browsers. We want to use the
\Uri\WhatWg\Urlclasses instead because that will allow WordPress to understand a given URL the way a browser would, to agree on how to parse it, and to be able to produce URLs that will do what developers expect them to when interacted with through a browser.There may be a place for RFC 3986 in some machine-oriented XML APIs, but largely we will be causing confusion and breakage if we pursue that form. The two exist side-by-side because there are use-cases for each. It just so happens that in WordPress we’re interfacing most frequently with browsers.
To that end, and given the utility of the classes, I don’t think the best approach will end up being
wp_parse_url()but per-callsite uses of\Uri\WhatWg\Url. Still, updating that class to at least eliminate the most-egregious parse failures is welcome.If you are volunteering to work on this, I recommend conditionally defining
wp_parse_url()based on the existence of theWhatWgclass, similar to howutf8_decode()is conditionally defined based on the presence of thembstringextension