Tests: Make open_basedir_linkinfo.phpt test open_basedir again, and remove its XFAIL - #23672
Open
rayblair06 wants to merge 1 commit into
Open
Tests: Make open_basedir_linkinfo.phpt test open_basedir again, and remove its XFAIL#23672rayblair06 wants to merge 1 commit into
rayblair06 wants to merge 1 commit into
Conversation
Member
|
Per https://bugs.php.net/bug.php?id=29145 I think this is correct. But not sure. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.