Larastan: User code is getting executed

[canvural]

• Larastan Version: 0.4.3
• --level used: 5

I have a project with Laravel Passport installed.

And when I analyze it with larastan it gives me an error like this Internal error: Key path "file:///var/www/html/storage/oauth-public.key" does not exist or is not readable This is an exception from either Passport or League\OAuth2 package.

@CyberiaResurrection could this be because of your change to use ClassMapGenerator ?

[CyberiaResurrection]

@canvural , I'm not sure off the top of my head - been in other things recently. Does pain persist with the last version before #318 landed?

[szepeviktor]

Actually phpstan and larastan are not totally static analyzers.

A fully static analyzer should use e.g. the PHP AST extension and not load class files.

[canvural]

Ok I have some more information. @CyberiaResurrection yes issue is there also in previous versions. 0.4.0, 0.4.1

But strange thing is I only get this error when level is 3 or higher. In level 2 everything is fine.

[CyberiaResurrection]

@canvural , the obvious question would seem to be what does level 3 do (on version 0.4.0 or whatever) that level 2 does not?

[nunomaduro]

cc @ondrejmirtes

[canvural]

Ok. I tracked down the error to this line. We are using the actual Laravel container to resolve the implementation of the interface. And yeah, this is bad :smile:

Maybe we can just return the interface here.

[CyberiaResurrection]

@canvural , @nunomaduro , and others who know a lot more than I do:

Would it be possible to do a "dry" resolve? Rather than the whole three-ring circus, go far enough to dig out the resolved object's class name?

[canvural]

No, it's dynamic.

Here, it's better to just use the interface. Since the all actual implementations implement this interface.

[canvural]

@CyberiaResurrection do you wanna take this? We just need to change this line to use the interface. And update the tests.

[CyberiaResurrection]

ok, @canvural , I'll have a bang at this. Does anyone have any objections to me doing what I can to push unit test coverage up while I'm at it?

[canvural]

Maybe you can do that in a seperate PR. Not in this one.

[CyberiaResurrection]

Makes sense.

[canvural]

I did some more digging. And this issue is more than just the Auth extension. Basically it happens everywhere where we use resolve

So one idea is to add another catch here. Which would catch any exception. So \Exception or \Throwable

Another idea is to refactor every place resolve is used. But this is much harder to do.

I'd go with the first option. Seems like a good idea. But i'm not sure.

cc @nunomaduro, @szepeviktor, @CyberiaResurrection

[CyberiaResurrection]

@canvural , with your "catch all the things" approach, why not _replace_ the existing catch blocks with a Throwable block, to save on cyclomatic complexity and better express your apparent intent of "we squish any and all kabooms".

[canvural]

Yeah, that's the idea.

[canvural]

So from the conversation #343

That's not a good solution. We should understand, instead, why are we resolving things from the container that can fail. Not that, if resolving things from the container a static analysis time, they may also fail at runtime for the user.

Let's make sure we address this issue properly.

So potentially every use of the resolve method is dangerous. In the code we use resolve to resolve an interface and get the concrete implementation. Or resolve the abstract manager and get the correct implementation.

First idea is, we can try to reduce the usage of resolve We can just return the interface without resolving for example.

Second idea is, maybe we can catch the exception. And try to give a better error message to the user. @nunomaduro made a good point with

if resolving things from the container a static analysis time, they may also fail at runtime for the user.

So this idea can be solution for that.

[nunomaduro]

First idea is, we can try to reduce the usage of resolve We can just return the interface without resolving for example.

The problem about this is: Laravel developers don't think that way. When they make Auth::user() they don't expect to receive a Authenticable object but instead they expect to receive the user model from it.

[canvural]

Yeah, I agree. So we can consider the second option. Trying to wrap the exception with a nice user friendly message, and don't break the analyses

[nunomaduro]

Lets won't forget to revert: https://github.com/nunomaduro/larastan/commit/12ee2d6f603afc3b67fa2e11e4c1293d3833b8f5.

Sounds good, like a warning or wtv.

[canvural]

239 is related, I think.

[bytestream]

Just ran into this.

What's the purpose of bootstrap.php? This seems to be the culprit behind code being executed.

Actually phpstan and larastan are not totally static analyzers.

This should no longer be true in phpstan >= 0.12.4 (https://github.com/phpstan/phpstan/issues/67)

[ondrejmirtes]

 This should no longer be true in phpstan >= 0.12.4

It's still true because static reflection isn't fully deployed. As the last comment in that thread says.

But Larastan's bootstrap serves a little bit different purpose than reflection, so it will still need to execute parts of Laravel to find out useful information about your application for the static analysis. There's no other way to obtain that information.

[bytestream]

The bootstrap file should be configurable or possible to override. It's essentially a copy of the artisan file which you're obviously free to edit in Laravel.

Is that possible? It looks to be added to the autoloader files so before you'd get the chance to change it.

I basically have a requirements check that happens before Laravel is loaded, and some later code (inside Laravel) expects those requirements to have ran.

[ondrejmirtes]

Yes, you can override it: https://phpstan.org/config-reference#bootstrap

But you should make sure to include Larastan's bootstrap in your bootstrap.