Make WordPress Core

Opened 4 years ago

Last modified 4 years ago

#51368 new defect (bug)

Remove windows path check from validate_file

Reported by: zieladam's profile 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 4 years ago.
51368.2.diff (402 bytes) - added by whyisjake 4 years ago.

Download all attachments as: .zip

Change History (7)

#1 @zieladam
4 years ago

cc @dd32

@whyisjake
4 years ago

#2 in reply to: ↑ description @SergeyBiryukov
4 years 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
4 years ago

  • Component changed from General to Filesystem API

@whyisjake
4 years ago

#4 @whyisjake
4 years 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
4 years 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.