[php-src] PR #23672: Fix open_basedir_linkinfo.phpt: restore open_basedir enforcement and remove XFAIL
| From: | rayblair06 | Date: | Sat, 12 Sep 2026 09:37:57 +0000 |
| Subject: | [php-src] PR #23672: Fix open_basedir_linkinfo.phpt: restore open_basedir enforcement and remove XFAIL | ||
| Groups: | php.git-pulls | ||
| Request: | Send a blank email to git-pulls+get-38457@lists.php.net to get a copy of this message | ||
Pull Request: https://github.com/php/php-src/pull/23672
Author: rayblair06
This is my first patch to
php-src, so please tell me if I've misunderstood
something here.
I was looking through the tests, and saw that tests/security/open_basedir_linkinfo.phpt
isn't actualyt checking open_basedir at all any more.
What I found, in commit 2459296ff50, the test used to set the limit with --INI--
open_basedir=. and was changed to call ini_set("open_basedir",
"."). The XFAIL was added in the same commit. As far as I can tell those
aren't the same thing:
- with --INI--, the . is compared against the current working directory
every time a check happens, so the chdir() into test/ok further down in
the test makes test/bad off-limits.
- with ini_set(), OnUpdateBaseDir() expands . to an absolute
path once, at the moment it is called, so after the chdir() the test/bad
directory is still inside the limit.
The way I convinced myself was to run the test's code twice, once with
open_basedir set to . and once set to / (so the limit allows
everything). With the current ini_set() version the output is exactly the same both
times, which I think means the test would still look fine even if open_basedir stopped working
completely. It also explains the one line that differs today. The warning is symlink(): File
exists, which is just the link already existing and has nothing to do with open_basedir.
So in this patch I put the --INI-- section back, so it matches the other
open_basedir_* tests in that directory, removed the XFAIL, and updated the expected
output to what PHP does now. With the limit actually applied, a symlink inside the allowed directory
whose target is outside it behaves like this: linkinfo() on the link works, but
symlink() and unlink() on it are refused, because the check follows the
link to its target.
The XFAIL text described that unlink() behaviour as a bug and mentioned two reports. I
looked both up: bugs.php.net/48111 asks for exactly this and was closed as Won't fix, because
special-casing unlink() for symlinks had already been turned down in
bugs.php.net/29145. bugs.php.net/52176 looks unrelated to me. It's about Windows, and this test
skips Windows. So my understanding is that the current behaviour is intentional and the test can
just assert it, which also means the test would catch a future change in this area instead of
staying silent.
This only touches the test file, no C code.
How I tested it (macOS, --enable-debug, NTS):
- tests/security/ on PHP-8.4: 48 passed, 0 failed.
- the same on master (8.6.0-dev): 48 passed, 0 failed.
- also with opcache.enable_cli=1, and with opcache.jit=tracing: same
result.
- the --CLEAN-- section still deletes the test/ directory the test
creates.
I don't have a Linux machine to try it on, so I'd appreciate it if either CI or someone
could confirm that, since this test deals with symlinks and path resolution.
Two things I'm unsure about and would like a second opinion on:
1. Is asserting the current unlink() behaviour the right call, or would you rather keep
an XFAIL that documents the behaviour someone might still want to change one day?
2. I dropped the chdir(__DIR__) calls because the sibling tests don't have them
and the --INI-- section makes them unnecessary. Let me know if they were there for a
reason I've missed.