WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 2 months ago

#51368 new defect (bug)

Remove windows path check from validate_file

Reported by: zieladam Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Filesystem API Keywords:
Focuses: Cc:

Description (last modified by SergeyBiryukov)

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

Attachments (2)

51368.diff (912 bytes) - added by whyisjake 2 months ago.
51368.2.diff (402 bytes) - added by whyisjake 2 months ago.

Download all attachments as: .zip

Change History (7)

#1 @zieladam
2 months ago

cc @dd32

@whyisjake
2 months ago

#2 in reply to: ↑ description @SergeyBiryukov
2 months ago

  • Description modified (diff)

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.

#3 @SergeyBiryukov
2 months ago

  • Component changed from General to Filesystem API

@whyisjake
2 months ago

#4 @whyisjake
2 months ago

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.

#5 @zieladam
2 months ago

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

Note: See TracTickets for help on using tickets.