Tests: Make open_basedir_linkinfo.phpt test open_basedir again, and remove its XFAIL - #23672
Tests: Make open_basedir_linkinfo.phpt test open_basedir again, and remove its XFAIL#23672rayblair06 wants to merge 1 commit into
Conversation
|
Per https://bugs.php.net/bug.php?id=29145 I think this is correct. But not sure. |
|
Thanks for digging that up. I think if I understand 29145 correctly this is the exact case this test hits. A symlink inside the allowed paths pointing outside them, which unlink() refuses. Worth saying, this isn't a behaviour change and only touches the test file. What mainly caught my eye is that the test currently can't tell whether open_basedir works at all, set it to That said, if you'd rather keep an XFAIL to record that someone might still want this changed one day, I'm happy to do that but I'd still put the |
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.phptisn't actualyt checking open_basedir at all any more.What I found, in commit 2459296, the test used to set the limit with
--INI-- open_basedir=.and was changed to callini_set("open_basedir", "."). The XFAIL was added in the same commit. As far as I can tell those aren't the same thing:--INI--, the.is compared against the current working directory every time a check happens, so thechdir()intotest/okfurther down in the test makestest/badoff-limits.ini_set(),OnUpdateBaseDir()expands.to an absolute path once, at the moment it is called, so after thechdir()thetest/baddirectory is still inside the limit.The way I convinced myself was to run the test's code twice, once with
open_basedirset to.and once set to/(so the limit allows everything). With the currentini_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 issymlink(): 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 otheropen_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, butsymlink()andunlink()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-casingunlink()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.opcache.enable_cli=1, and withopcache.jit=tracing: same result.--CLEAN--section still deletes thetest/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:
unlink()behaviour the right call, or would you rather keep an XFAIL that documents the behaviour someone might still want to change one day?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.