Skip to content

Harden against undefined index in _load_script_textdomain_from_src() and add types for wp_parse_url()#11690

Closed
westonruter wants to merge 6 commits intoWordPress:trunkfrom
westonruter:fix/load-script-textdomain-from-src-hardening
Closed

Harden against undefined index in _load_script_textdomain_from_src() and add types for wp_parse_url()#11690
westonruter wants to merge 6 commits intoWordPress:trunkfrom
westonruter:fix/load-script-textdomain-from-src-hardening

Conversation

@westonruter
Copy link
Copy Markdown
Member

@westonruter westonruter commented Apr 30, 2026

See Trac comment for full description of issue: https://core.trac.wordpress.org/ticket/65015#comment:47

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

Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Opus 4.7
Used for: PHPStan type research


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
Copy link
Copy Markdown

github-actions Bot commented Apr 30, 2026

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.

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

Props westonruter, manzoorwanijk, mukesh27.

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

Comment thread src/wp-includes/l10n.php
if ( ! $src_url ) {
return false;
}
$src_url['path'] ??= '';
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the crux for fixing the undefined index error.

Copy link
Copy Markdown
Member

@manzoorwanijk manzoorwanijk May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we cover these bail-outs with unit tests?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that would be idea. I've added full coverage in b230450

@github-actions
Copy link
Copy Markdown

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

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

@westonruter
Copy link
Copy Markdown
Member Author

@manzoorwanijk Please review.

Comment thread src/wp-includes/l10n.php
}

$path = untrailingslashit( $path );
if ( $path ) {
Copy link
Copy Markdown
Member Author

@westonruter westonruter Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conditional added because the $path can still be false here since $wp_textdomain_registry->get() returns string|false.

Comment thread src/wp-includes/l10n.php

// If the source is not from WP.
if ( false === $relative ) {
if ( ! is_string( $relative ) ) {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since apply_filters() can return anything, safer to check for a string specifically than for equality with false.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aims to harden script translation path resolution against edge cases in URL parsing (preventing undefined index / invalid offsets) and to improve static analysis by adding PHPStan typing for wp_parse_url() and its helper.

Changes:

  • Adds guards/defaults in _load_script_textdomain_from_src() to avoid undefined index access when working with parsed URLs.
  • Adjusts handling of unresolved relative paths to be more type-safe (! is_string( $relative )).
  • Adds PHPStan conditional return types and array-shape annotations for wp_parse_url(), _get_component_from_parsed_url_array(), and _wp_translate_php_url_constant_to_key().

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/wp-includes/l10n.php Adds URL-parse hardening and path handling tweaks for script/module translation resolution.
src/wp-includes/http.php Introduces PHPStan parameter/return typing for URL parsing helpers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/wp-includes/l10n.php Outdated
Comment thread src/wp-includes/http.php
Comment thread src/wp-includes/http.php
…content URL

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@westonruter
Copy link
Copy Markdown
Member Author

westonruter commented May 1, 2026

The addition of PHPStan types here (cf. Core-64238) ensures that all of the scenarios are accounted for which could cause errors.

Copy link
Copy Markdown
Member

@manzoorwanijk manzoorwanijk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I just wonder whether we should add new test cases for the new bailout paths.

Comment thread src/wp-includes/l10n.php
if ( ! $src_url ) {
return false;
}
$src_url['path'] ??= '';
Copy link
Copy Markdown
Member

@manzoorwanijk manzoorwanijk May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we cover these bail-outs with unit tests?

Comment thread src/wp-includes/l10n.php
* @return string|false The JSON-encoded translated strings on success, false otherwise.
*/
function _load_script_textdomain_from_src( string $handle, string $src, string $domain, string $path, bool $is_module ) {
/** @var WP_Textdomain_Registry $wp_textdomain_registry */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something that globally fix in #11692. Can we leave this for this PR

…om_src()

Cover the three new short-circuit branches added in 8f38b36 (unparseable
$src, unparseable content_url(), non-string $relative filter return) plus
the $src_url['path'] default added in bfe0eca. Each test uses a
pre_load_script_translations spy to assert the early-return branch was the
one actually taken, rather than the result coincidentally matching the
fallback-path behavior.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Member

@manzoorwanijk manzoorwanijk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect. Thanks

westonruter and others added 2 commits May 1, 2026 16:44
Collapse the data-provider variants (null/true/array/int) into a single
test using `__return_null` — all four hit the same one-line guard, so one
case is enough to catch a regression.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the bespoke by-ref-closure spy (not used elsewhere in
tests/phpunit/) with the idiomatic MockAction helper. Keep the spy
only on test_unparseable_content_url_returns_false, where the early
return is the only thing distinguishing the new branch from the
fallback path tail-end load_script_translations(false, ...) call.

The other two tests do not need a spy: removing the early return for
unparseable src triggers the "Automatic conversion of false to array"
deprecation, and reverting the is_string guard triggers a TypeError
in str_ends_with() -- both surface as test failures already.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
pento pushed a commit that referenced this pull request May 2, 2026
…_src()`.

This guards against an undefined index warning being raised when a script or script module is registered with a URL that lacks a path component. 

This also adds full PHPStan type definitions for `wp_parse_url()`, ensuring that the `_load_script_textdomain_from_src()` function has no PHPStan errors at rule level 10.

Developed in #11690

Follow-up to r62278.

Props westonruter, manzoorwanijk, mukesh27.
See #65015, #64238.


git-svn-id: https://develop.svn.wordpress.org/trunk@62293 602fd350-edb4-49c9-b593-d223f7449a82
@westonruter
Copy link
Copy Markdown
Member Author

Committed in r62293 (0fced49)

@westonruter westonruter closed this May 2, 2026
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request May 2, 2026
…_src()`.

This guards against an undefined index warning being raised when a script or script module is registered with a URL that lacks a path component. 

This also adds full PHPStan type definitions for `wp_parse_url()`, ensuring that the `_load_script_textdomain_from_src()` function has no PHPStan errors at rule level 10.

Developed in WordPress/wordpress-develop#11690

Follow-up to r62278.

Props westonruter, manzoorwanijk, mukesh27.
See #65015, #64238.

Built from https://develop.svn.wordpress.org/trunk@62293


git-svn-id: http://core.svn.wordpress.org/trunk@61573 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants