Remove windows path check from validate_file

[zieladam]

Let's talk about validate_file function, and specifically about this fragment here:

​https://github.com/WordPress/wordpress-develop/blob/b984a64c987ae259109bcb08776b1ed22f1dc98f/src/wp-includes/functions.php#L5373-L5376

It checks whether or not the path is a Windows drive path. Why is this logic needed? It doesn't seem to play any role or even make sense - why allow arbitrary unix paths, but not windows paths? It was introduced 16 years ago when the function was first created, and even then there was no clear explanation why is it even being checked: [2019].

[zieladam]

cc @dd32

[SergeyBiryukov]

Replying to zieladam:

It was introduced 16 years ago when the function was first created, and even then there was no clear explanation why is it even being checked: [2019].

Just noting the ':' === substr( $file, 1, 1 ) check itself is even older and comes from b2/cafelog:
https://core.trac.wordpress.org/browser/trunk/b2template.php?rev=3&marks=79-80#L68

That is the line that ended up in validate_file() after being moved around quite a few times.

Looking at the "Sorry, can't call files with their real path" message, it was probably added as some sort of a security precaution, though perhaps no longer relevant.

[whyisjake]

I think that rather than removing the windows check, we can just be a little more explicit in the checking of the file paths. I could go either way here.

[zieladam]

@whyisjake

The hard part here is that not only are all of these are valid paths on windows:

C:Projects\apilibrary\apilibrary.sln
D:\FY2018
\\system07\C$\
\Server2\Share\Test\Foo.txt
c:\temp\test-file.txt
\\127.0.0.1\c$\temp\test-file.txt
\\LOCALHOST\c$\temp\test-file.txt
\\.\c:\temp\test-file.txt
\\?\c:\temp\test-file.txt
\\.\UNC\LOCALHOST\c$\temp\test-file.txt
\\127.0.0.1\c$\temp\test-file.txt
\?\Volume{b75e2c83-0000-0000-0000-602f00000000}\Test\Foo.txt

But also that:

On Windows, both slash (/) and backslash (\) are used as directory separator character. In other environments, it is the forward slash (/).
– PHP docs ​https://www.php.net/manual/en/function.basename.php

In this context, these are valid paths on both windows and linux:

//system07/C$/
//./c:/temp/test-file.txt
//LOCALHOST/c$/temp/test-file.txt

This one is either absolute or relative depending on the context: C:Projects/apilibrary/apilibrary.sln.

[whyisjake]

Attaching 51368.3.diff​, which reverses my two earlier patches and the ticket title: rather than removing the Windows check, it widens it. 51368.2.diff tried to make the existing check more explicit, along the lines of my comment:4. Tracing what the check actually does today convinced me both of my earlier attempts were aimed at the wrong defect.

To be clear about size and nature before the detail: this is a contract-accuracy fix, not a security fix. It closes no vulnerability, and the caller audit below shows no core code path depends on the check. The patch is one condition. What I am asking for is a decision on whether making the docblock's claim true is worth a behavior change, or whether the ticket should instead be re-scoped to retract the claim.

A separate problem, which cuts against removal as much as for it

@zieladam asked why we reject Windows paths but allow arbitrary Unix paths. There is a second problem, orthogonal to that one: we do not actually reject Windows paths. We reject one of several forms.

validate_file() runs wp_normalize_path() before the drive-letter test. That folds backslashes to forward slashes, but its slash-collapsing regex |(?<=.)/+| deliberately spares a leading // ("allowing for network shares having two slashes"). So every backslash-prefixed Windows form arrives with no colon in the second byte, and validate_file() returns 0 (acceptable) rather than 2 (Windows drive path). The table in the next section has the enumeration.

Meanwhile the docblock says a return of 2 "means the file path contains a Windows drive path," which reads as a guarantee that Windows absolute paths are screened. Most of them are not. That gap is the part worth fixing, and neither of my earlier patches addressed it.

Answering comment:5

@zieladam noted, correctly, that the Windows path space is too large to parse from a substring offset. It does not need parsing: after normalization folds the separators, every form on that list is either caught by the existing colon test or begins with //. Checked against the list as given, plus the three forward-slash variants from the same comment:

form	after wp_normalize_path()	trunk	patched
C:Projects\apilibrary\apilibrary.sln	C:Projects/apilibrary/apilibrary.sln	2	2
D:\FY2018	D:/FY2018	2	2
c:\temp\test-file.txt	C:/temp/test-file.txt	2	2
\\system07\C$\	//system07/C$/	0	2
\\127.0.0.1\c$\temp\test-file.txt	//127.0.0.1/c$/temp/test-file.txt	0	2
\\LOCALHOST\c$\temp\test-file.txt	//LOCALHOST/c$/temp/test-file.txt	0	2
\\.\c:\temp\test-file.txt	//./c:/temp/test-file.txt	0	2
\\?\c:\temp\test-file.txt	//?/c:/temp/test-file.txt	0	2
\\.\UNC\LOCALHOST\c$\temp\test-file.txt	//./UNC/LOCALHOST/c$/temp/test-file.txt	0	2
//system07/C$/	//system07/C$/	0	2
//./c:/temp/test-file.txt	//./c:/temp/test-file.txt	0	2
//LOCALHOST/c$/temp/test-file.txt	//LOCALHOST/c$/temp/test-file.txt	0	2
\Server2\Share\Test\Foo.txt	/Server2/Share/Test/Foo.txt	0	0
\?\Volume{b75e2c83-...}\Test\Foo.txt	/?/Volume{b75e2c83-...}/Test/Foo.txt	0	0

The last two are the single-backslash entries, and they stay at 0: after normalization they are indistinguishable from POSIX absolute paths.

Why widen rather than remove

The strongest case for removal is not the one about Unix paths. It is the one this patch's own findings supply: the check is not a security boundary, no core caller depends on it, its rationale has never been reconstructed, and on a Windows-hosted site its only observable effect is to reject paths that are legitimate on that platform. Against that, "keep it because a plugin might rely on the rejection" is answering a narrower question than the one being asked. So let me take the real one.

The reason I land on widening is that the docblock is a published contract and third-party code reads it. That leaves two coherent options: make the contract true, or retract it. Retracting it is the larger and less reversible change, because anyone depending on code 2 depends on it firing. They wrote if ( validate_file( $f ) ) { reject; }. Delete the branch and their rejection becomes an acceptance. The available deprecation paths for a return value are all awkward — you could keep returning 2 while documenting it as deprecated, but that is indistinguishable from leaving it in place, which is not what the ticket asks for.

Widening has the opposite profile:

if ( ':' === substr( $file, 1, 1 ) || str_starts_with( $file, '//' ) ) {
        return 2;
}

Code 2 fires on strictly more inputs and never fewer, so no existing rejection can turn into an acceptance. The exposure runs the other way — callers relying on a 0 — and I take that up at the end.

What this does and does not change for POSIX paths

validate_file( '/etc/passwd' ) returns 0 on trunk and still returns 0 here. Rejecting all absolute POSIX paths would be a far larger change: plugins pass absolute template paths through this function and get 0 today. That is a separate discussion, and bundling it here would give reviewers two arguments to weigh instead of one.

One caveat that belongs in this section rather than buried in the tests, because it is the obvious hole in the paragraph above. Any path arriving with a leading // is now rejected regardless of platform, and that includes doubled-slash POSIX paths. //home/user/file.php returns 2 under this patch. POSIX.1 says a pathname beginning with two successive slashes may be interpreted in an implementation-defined manner, and Linux resolves it the same as a single slash, so this is a real if unusual path that used to validate and no longer does.

I think that is acceptable, but it is a judgement rather than a non-issue, so it should be made deliberately. The argument for it: wp_normalize_path() already preserves a leading // specifically on the assumption that it denotes a network share, so core has taken that position for as long as that comment has been there. This patch is consistent with that existing assumption rather than introducing a new one. If component owners disagree, the fix is to make the condition platform-aware, which I would rather not do in a string-validation helper.

The check is defense-in-depth, not a boundary

No core caller depends on code 2 for safety. I traced all 24 validate_file() call sites in src/: every one either concatenates the validated value onto a trusted base directory and stats the result, or looks it up in a registry. This patch closes no vulnerability — an absolute path already fails one step later. A representative sample:

call site	handling after validation
wp-admin/admin.php:266	WP_PLUGIN_DIR . "/$plugin_page" then file_exists()
wp-admin/admin.php:313	$wp_importers[ $importer ] lookup
wp-settings.php:716	WP_LANG_DIR . "/$locale.php" then is_readable()
wp-admin/includes/file.php:419,480	relative allowlist, then WP_PLUGIN_DIR . '/' . $file
wp-includes/template.php:488,560	passed to locate_template()

I should name the tension rather than leave it for someone to find: this cuts toward removal as far as core is concerned. The case for keeping the check rests entirely on external code reading that docblock, which is also why I am not comfortable removing it on core's behalf. What the patch buys is a true docblock, which matters for plugins relying on it.

Tests

The patch adds 15 cases to data_validate_file() in three groups: the UNC and device forms above, the POSIX paths that must stay at 0, and both stream-wrapper branches.

The wrapper cases are there because anchoring the condition to the start of the string keeps wrappers working by two separate mechanisms, and I wanted both pinned. A registered wrapper keeps its :// through wp_normalize_path()'s scheme split, so php://memory has its slashes at offset 3, not 0. An unregistered scheme never enters that branch, so myapp://foo collapses to myapp:/foo. Both stay at 0.

Disabling the added clause fails exactly the nine new rejection assertions, all as 0 is identical to 2; every other assertion, including the POSIX and wrapper cases and all pre-existing entries, still passes.

tests/phpunit/tests/functions.php, admin/includesPlugin.php, admin/includesFile.php, theme.php, rest-api/rest-plugins-controller.php and blocks/renderBlockTemplatePart.php all pass. phpcs is clean on both changed files.

Three open questions for the component owners

@since. Written as 7.2.0 assuming this misses 7.1, which is in beta. Happy to move it.

Should validate_file() reject network-share paths that are legitimate on the host platform? This is the one genuine back-compat exposure. A Windows-hosted site passing a UNC path and relying on the 0 would now get 2. There are two readings and I do not think it is my call which one governs: either that caller was depending on a documented rejection failing to fire, or a UNC path on a Windows host is simply a valid path and rejecting it is the bug. The same question applies to the doubled-slash POSIX case above.

Allowlist precedence. I need to be accurate about what the patch did here rather than claim I left it alone. The existing C:/WINDOWS/system32 data set documents that an explicitly allowlisted absolute path still returns 2, because the drive check runs after the allowlist test. The patch extends that precedence to UNC paths and locks it in with a new assertion — '//system07/C$/foo.php' with that exact path allowlisted returns 2. Matching the existing drive-path behavior seemed better than having two rules, but it is a new decision made in this patch, not inherited. If you think an explicit allowlist entry should win, that assertion is the line to change.

---

AI assistance: Yes. Tool(s): Claude Code (Claude Opus 5).

Used for: tracing the wp_normalize_path() interaction that produces the gap, auditing the validate_file() call sites, drafting the patch and its test cases, and drafting this comment.

Tests run: tests/phpunit/tests/functions.php, admin/includesPlugin.php, admin/includesFile.php, theme.php, rest-api/rest-plugins-controller.php, blocks/renderBlockTemplatePart.php, plus phpcs on both changed files. I have reviewed the patch and tests and can speak to the approach in review.

[whyisjake]

In 63312:

Filesystem API: Reject UNC and device paths in validate_file().

validate_file() runs wp_normalize_path() before testing for a drive letter at offset 1. That call folds backslashes to forward slashes, but its slash-collapsing regex deliberately spares a leading // to allow for network shares. UNC paths such as //server/share, and the device namespaces //./ and //?/, therefore arrived with no colon in the second byte and returned 0 rather than 2.

The docblock described a return value of 2 as meaning the path contains a Windows drive path, which reads as a guarantee that Windows absolute paths are screened. Most forms were not. Anchoring a second test to the start of the string closes that gap. Stream wrappers stay at 0 by two separate mechanisms: a registered wrapper keeps its :// through the scheme split, placing those slashes past the second byte, while an unregistered scheme has its // collapsed.

Code 2 now fires on strictly more inputs and never fewer, so no existing rejection becomes an acceptance.

Absolute POSIX paths such as /etc/passwd continue to return 0; screening those would be a larger change affecting plugins that pass absolute template paths. Paths beginning with two successive slashes are now rejected regardless of platform, which includes the implementation-defined POSIX doubled-slash form, consistent with the assumption wp_normalize_path() already makes about a leading //.

Props zieladam, SergeyBiryukov.
Fixes #51368.