Class finder loads files it should not load

[enricobono]

We were in the process of upgrading our application from graphqlite v6 to v8, and we discovered an issue with v8.

This is a consequence of what was introduces here: thecodingmachine/graphqlite#657.
With this PR, graphqlite will now look for Types not only in the src/ space, but in the vendors as well. Which bwt is totally legit. To reach this goal, the class explorer package was replaced with kcs/class-finder.

Now, we notices that, in the dev environment, kcs/class-finder was looking for all the classes in the vendor/ directory and in the tests/ directory as well. It iterates over all the .php files, looking for classes:

//class-finder/lib/Iterator/Psr0Iterator.php::62

static function (string $path, string $class): void {
    class_exists($class, true);
}

The issue now is that class_exists will include the file, if not already loaded.

In our case, we have a tests/bootstrap.php file which contains plain code, no class declarations.

So kcs/class-finder will do class_exists('tests/bootstrap.php', true), the file will be included and its content executed. So we are basically executing every php file (which does not contain a class) in both vendor/ and tests/ . Which should not be the case. For example, in our case, a simple run of:

bin/console cache:clear --env dev

will execute the tests/bootstrap.php, which has implementation specific for the test env.

And, moreover, it may pose some security issue, given it will execute any code in any plain php file in any vendor/ subfolder.

Moreover, as per the current configuration, kcs/class-finder is called several times, so it requests the lists of files many times in each session, which means our tests/bootstrap.php file is included more than once, which causes other issues and makes the process slower.

Has anybody else experienced similar issues related to this?

[andrew-demb]

@enricobono see discussion in this issue alekitto/class-finder#24

[enricobono]

Thanks Andrii.
I would suggest to do the opposite of what AleKitto suggested. He's suggesting to explicitly exclude problematic files. But still, chances are graphqlite-bundle is loading files it should not load.

I'd propose the opposite:

• by default graphqlite-bundle searches for types on userland src/. If the developer wants to look for Types in some additional directory, the developer will need to explicitly add those directories. For instance, we can add this info in a config param.

What do you think?

[andrew-demb]

@enricobono seems like a useful configuration. But I suggest preserving the current behavior by default and making it opt-out (configured to use a path like an src/)

[enricobono]

Make sense, thanks.
I guess the default behaviour, right know, is to search everywhere.

[andrew-demb]

@enricobono it's not searching everywhere, but in relevant namespaces according to the dependencies (and project itself) - autoload config.

If package configured autoload like this [1], it leads to "full scan" - so this is architecture limitation of class-finder's method ComposerFinder::inNamespace

[1]

"autoload" : {
    "psr-4" : {
      "TheCodingMachine\\GraphQLite\\Bundle\\" : ""
    }
  },

[github-actions]

This issue is stale because it has been open 6 months with no activity. Remove stale label or comment or this will be closed in 5 days.

[github-actions]

This issue was closed because it has been stalled for 5 days with no activity.

[homersimpsons]

I believe this is not fixed yet.

[github-actions]

This issue is stale because it has been open 6 months with no activity. Remove stale label or comment or this will be closed in 5 days.