Use CONTEXT_DOCUMENT_ROOT for scanning dir tree

[Ragnoroct]

This allows people to use the CONTEXT_DOCUMENT_ROOT variable to scan recursively for .user.ini files.

There is a bug report open here https://bugs.php.net/bug.php?id=64865 and plenty of other references to this issue on stackoverflow and other forums.

I'm not sure if this should be an RFC or not. So I just created a pull request to start the process.

[nikic]

Looks reasonable to me for master, but I don't think we should land this change on stable versions. It seems quite likely that people rely on the current behavior as a workaround if nothing else.

[Ragnoroct]

Okay, I've rebased my PHP-7.3 to master and changed the pull request target.
In hindsight I should have created a new branch to avoid this confusing setup. I can always create another pull request and close this one.

[bukka]

Just noticed this. Can someone tell why was this was add it to php-fpm (just noticed that in changes for 8.0)? This is just CGI, right so this shouldn't impact fastcgi.

Please let me know. Otherwise I will remove the FPM specific changes.

[bukka]

CC @Ragnoroct @nikic

[nikic]

I don't think cgi vs fcgi makes a difference here. Not sure whether it's relevant for fpm specifically though.

[bukka]

Ok it looks that Apache actually passes that variable. I guess it probably makes some sense.

[bukka]

I mean it sets it for fcgi as well...

[Ragnoroct]

The bug report said it was happening on PHP-CGI. I've also experienced it on fcgi.

[Ragnoroct]

I'm not sure what the question is.

[nikic]

It looks like using CONTEXT_DOCUMENT_ROOT is not always desirable: https://bugs.php.net/bug.php?id=80113

[Ragnoroct]

What should we do in this situation? Is it possible to scan both?..

[RealityRipple]

Can a check be introduced to see if the user_ini_filename value in question is even in a subdirectory of either DOCUMENT_ROOT or CONTEXT_DOCUMENT_ROOT? If both, use CONTEXT_. If one and not the other, you have your answer. If neither... well, hopefully that's functionally impossible (or at least unlikely), but maybe don't traverse any directories at all because something's already not right?

[wf-mattr]

Or alternately, check if CONTEXT_DOCUMENT_ROOT is a substring of SCRIPT_FILENAME before choosing to use it instead of DOCUMENT_ROOT? I don't have a site with mod_userdir to check the behavior that way, but this would fix the sites I see with CGI configured as described in the PHP bug 80113. @Ragnoroct, is it still possible for this to be addressed before PHP 8.0 is released?

[Ragnoroct]

Yes, I think so. I'll see if I can try your script_filename solution. Is script_filename always guaranteed to be set?

…

On Thu, Oct 15, 2020, 12:03 PM wf-mattr ***@***.***> wrote: Or alternately, check if CONTEXT_DOCUMENT_ROOT is a substring of SCRIPT_FILENAME before choosing to use it instead of DOCUMENT_ROOT? I don't have a site with mod_userdir to check the behavior that way, but this would fix the sites I see with CGI configured as described in the PHP bug 80113. @Ragnoroct <https://github.com/Ragnoroct>, is it still possible for this to be addressed before PHP 8.0 is released? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5051 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AESESBNTYEDMUDDLSQNXSW3SK4TN3ANCNFSM4KCO5XYQ> .

[wf-mattr]

I have never seen SCRIPT_FILENAME unset myself on cgi, fcgi, mod_php, or the CLI; but I don't know if it is possible in different configurations from what I'm used to. (It is actually an empty string if I run the CLI interactively, but that is expected.)

[bukka]

There is already a check if doc_root is prefix for path (which is from paht_translated which is effectively SCRIPT_FILENAME without SCRIPT_NAME). What should be done is to refactore the php_cgi_ini_activate_user_config to get context root and document root and compare the prefix first for the context and if it doesn't match, then for the doc root. Then I think it should address the reported issue.

The thing that I don't really like is that this got in without any test. It should be doable to write a test for this in FPM as we have got user.ini tests already. Especially if there is more logic coming, the test is really required IMO.

[nikic]

I have reverted this change in c97da0f. We can reland once there is a solution that satisfies all scenarios. But for now, let's get back to the status quo of previous versions.