Fix exception, open_basedir restriction in effect

[chris-kruining]

this is a VERY crude fix for this exception is_file(): open_basedir restriction in effect. File([internal]) is not within the allowed path(s)

[Jean85]

Can you please tell us how this bug happens? Can you reproduce it locally?

[ste93cry]

Thank you for the contribution. We already had a few cases of this issue in 1.x version too (#653) and I believe that hardcoding a comparison between the file path and the [internal] string won't resolve the problem once for all. It would be great if we could finally pinpoint the root cause of the problem instead of just applying a fix blindly, so would you be so kind to help us debug the problem more in depth? Sharing a little script along with a minimal PHP/webserver configuration that reproduces the problem would be awesome!

[chris-kruining]

After some searching I think this is not as much an issue with sentry as it is php's file IO API. I think there are a couple of approaches we could take, but I have no clue which is most desirable.

1. use @ to suppress the error altogether (please don't :P)
2. add a try catch block, most like option 1. but with proper syntax
3. use ini_get('open_basedir') to read the conf, split on : and check if the given path is in the allowed path(s)
4. afaik file_exists also returns false if the given path is outside the allowed set of basedirs without throwing errors or warnings, ergo

if (!file_exixts($path) || !is_file($path) || !is_readable($path)) {
       return [];
}

imho the last option would be the most elegant

[ste93cry]

imho the last option would be the most elegant

I agree, with options 2 and 1 in the order of preference. I would like to stay away from parsing an INI setting complex as the open_basedir because we should handle absolute vs relative paths and/or expand paths before doing comparison (I believe at least) and this is a complex stuff. If you could try if file_exists works as expected and fixes the issue then we are more than happy to accept the PR with the changes. Still, it would be great if a simple script and configuration to reproduce the problem and assert that it's fixed would be more than welcome

[chris-kruining]

I have had the 'patch' with file_exists running for about half a day in production now, no issues thus far.
I'll try to provide some env information and setup soon

[ste93cry]

Looking a bit on the web it seems that this error is raised even with file_exists checks. I still didn't have the chance to setup a test project to reproduce the issue, however are you sure that the fix is working in your production environment?

[Jean85]

The fix is probably working but it's raising some kind of warning, that is probably not configured to be reported.

If file_exists raises a notice anyway, @file_exists will be the only way to go.

[ste93cry]

If file_exists raises a notice anyway, @file_exists will be the only way to go.

Probably it is, even though I believe that such core functions should not generate a warning in this case but just return false. Also the comparison with the [internal] string probably makes sense since (I didn't see it before) we are generating such filename when the real one is not available, so it's already hardcoded in some way

[Jean85]

Also the comparison with the [internal] string probably makes sense since (I didn't see it before) we are generating such filename when the real one is not available, so it's already hardcoded in some way

If that's the case, then we've already covered the issue elsewhere, and this PR is good as it is 👍

Apart from adding some tests, obviously 😄

[chris-kruining]

though I believe that such core functions should not generate a warning in this case but just return false

I disagree, I think a warning is fair game, as it is erroneous behaviour. That said, PHP should then offer a mechanism to check instead.

I'll try to create an isolated case from the rest of my code base and provide some server config soon.

[ste93cry]

If that's the case, then we've already covered the issue elsewhere, and this PR is good as it is

We didn't. If the name of the file is not available in the stacktrace frame then we set it to [internal], but we keep checking if the file exists on the filesystem, and we could avoid it because let's face it, I highly doubt someone will ever have a file with such name. But even if we decide to keep the existing behavior for correctness, the issue would not be solved because it would still be present with all other filenames 😉 So my suggestion at this point is to suppress errors from is_readable and swap the check with is_file. Since is_readable also checks if the file exists (of course), we should not have the need of call file_exists and if a warning is generate we don't care about it anyway since we should be as much fail-safe as possible. Summarizing:

-if (!is_file($path) || !is_readable($path)) {
+if (!@is_readable($path) || !is_file($path)) {

[chris-kruining]

alright, !@is_readable($path) || !is_file($path) would work, but can't really confirm as I can't reproduce the error any more. So I guess it was a fuck up in the php instance, as all that has changed in de setup are a couple of apache and php-fpm restarts...

[ste93cry]

So I guess it was a fuck up in the php instance, as all that has changed in de setup are a couple of apache and php-fpm restarts

Are you sure that the original error that was being captured by Sentry is still thrown and that the stacktrace is the expected one to reproduce the issue? Of course if there is no frame referencing the [internal] filename and/or the open_basedir option is disabled then you cannot reproduce anymore the error 😄 By the way, catching an error with try/catch is not possible so the only remaining viable option is suppressing the error. If you could make the changes to this PR I would gladly approve it

[chris-kruining]

Are you sure that the original error that was being captured by Sentry is still thrown

Well no, I resolved that issue. However I used to reproduce it with any exception, and that has become impossible. I think it is wise to keep this PR open until I have a solid reproducible case, so that, I can write a couple of test cases

[chris-kruining]

I can't for the life of me reproduce it, even when I roll back all of my changes, it still works normaly without errors. I tried both to directly trigger the error by manually calling is_file('[internal]') and by provoking the original exception that triggered it, but no dice.

So I'm at my wits end. Do we just merge this, suspend until reproducable, or totally drop it?

[ste93cry]

Since it's such a simple change LGTM, it cannot hurt anyway to suppress an error in this specific case. Just one thing: can you please fix the build?

[chris-kruining]

The build error seems to be a dependency issue, no clue where to start, maybe you could take a gander?

[ste93cry]

Fix should be pretty easy and should only involve changing the phpunit.xml.dist file to set the SYMFONY_DEPRECATIONS_HELPER environment variable to max[self]=0

[chris-kruining]

build is fixed

[ste93cry]

Thank you for the contribution, it's highly appreciated!