[11.x] Hook collection is unbounded: it walks hidden directories and follows symlinks out of the module

Problem/Motivation

This wawa fixed on main but the fix is something we can't backport.

HookCollectorPass::collectModuleHookImplementations() scans each installed module's directory to find hook implementations. The scan has no upper bound on where it can go:

$iterator = new \RecursiveDirectoryIterator($dir, \FilesystemIterator::SKIP_DOTS | \FilesystemIterator::UNIX_PATHS | \FilesystemIterator::FOLLOW_SYMLINKS);
$iterator = new \RecursiveCallbackFilterIterator($iterator, static::filterIterator(...));

and filterIterator() prunes tests, js, css, whatever $settings['file_scan_ignore_directories'] names, and directories that contain an .info.yml. Two things get through:

1. Hidden directories are scanned. FilesystemIterator::SKIP_DOTS skips . and .., not .git, .devenv, .direnv or .venv. ExtensionDiscovery, which walks the same trees for the same purpose, skips them on the first lines of its filter, with a comment that says exactly why:

// RecursiveExtensionFilterCallback::accept()
// FilesystemIterator::SKIP_DOTS only skips '.' and '..', but not hidden
// directories (like '.git').
if ($name[0] === '.') {
  return FALSE;
}

2. Symlinks are followed however far out of the module they lead. A symlink several levels down inside a module is followed into whatever it points at — another checkout, a package store, a home directory.

What the existing setting already covers

Worth ruling out first, because it is the obvious objection. #3564112: Cache rebuild triggers file_get_contents Permission denied in theme node_modules despite file_scan_ignore_directories taught filterIterator() to honour $settings['file_scan_ignore_directories'], and every site's settings.php ships node_modules and bower_components in it. That works. Driving the collector over a module with a hook planted in three places, on main without this change:

So a module shipping a front-end build is handled, and a site can add its own names to that list. What the setting does not reach is the two cases above: its default names two directories and nothing in it covers .git or .devenv, and no list of names stops a symlink that leaves the module altogether.

#3564112: Cache rebuild triggers file_get_contents Permission denied in theme node_modules despite file_scan_ignore_directories is worth reading alongside this: it came in from a theme's node_modules, which is the same shape of problem from a different direction.

Where it bites

A module symlinked in from a working checkout — the ordinary way to develop contrib against a site, and the case #3482283 added FOLLOW_SYMLINKS for. The checkout carries whatever tooling state the developer has, none of it named in file_scan_ignore_directories:

Core's own test tooling trips on the same tree, which is worth mentioning as corroboration: core/phpunit.xml.dist declares testsuites over ../modules/*/**/tests/src/Kernel and friends, and PHPUnit's directory globbing follows symlinks too. On the site this was found on, running any kernel test hung until the offending directory was moved out of the tree. So the assumption that a module directory is bounded is not confined to HookCollectorPass — though the collector is where it costs every rebuild.

Only drush cr and other container rebuilds are affected, since that is when the collector runs. That also makes it easy to misdiagnose: drush cache:clear bin discovery stays instant while a newly added hook stays undiscovered, so function_exists() reports TRUE while hasImplementations() reports FALSE, which sends you looking in the wrong place.

Steps to reproduce

1. Check out a module beside a site and symlink it into place: ln -s ../../../../mymodule web/modules/contrib/mymodule
2. In the checkout, create tooling state whose symlinks leave the tree. devenv and nix-direnv do this by themselves; by hand: mkdir -p .devenv && ln -s /nix/store/… .devenv/profile
3. time drush cr

Adding .devenv to $settings['file_scan_ignore_directories'] works around it, which is the point: a developer has to know to do that, for each name their tooling leaves behind.

Measured

A symlinked module checkout with .devenv in it. The module is 926 directories; following one symlink in .devenv reaches 38,676.

Almost all of it is system time — readdir/stat, not work. A stack sample during the hang sits in RecursiveCallbackFilterIterator. find -L over the same tree also reports 15 Too many levels of symbolic links errors.

Proposed resolution

Two guards in filterIterator(), in the merge request:

// FilesystemIterator::SKIP_DOTS only skips '.' and '..', but not hidden
// directories (like '.git', or a developer's '.devenv'). ExtensionDiscovery
// skips them too, and a module cannot be installed from one anyway.
if (str_starts_with($fileInfo->getFilename(), '.')) {
  return FALSE;
}
// Follow symlinks only at the top level of the module directory. A module
// may be symlinked into place, and items may be symlinked into a module
// directory — see HookCollectorPassTest::testSymlink — but a symlink deeper
// in is as likely to lead out of the module altogether, into a store
// closure or a package cache, and the scan has no business following it
// there.
if ($fileInfo->isLink() && str_contains($sub_path_name, '/')) {
  return FALSE;
}

Verified against the real collector on fixture modules:

The second row is the one that matters for #3482283: symlinks at the top level are still followed, so testSymlink() — which symlinks a module's individual items into the scanned directory — keeps passing.

Remaining tasks

• [ ] Decide whether vendor belongs in the prune list, as it is in ExtensionDiscovery::$skippedFolders. A module shipping its own vendor/ has it walked on every rebuild. Possibly a follow-up.

User interface changes

None.

API changes

None. filterIterator() is protected static on a compiler pass.

Behaviour change: a hook implementation inside a hidden directory, or behind a symlink below a module's top level, stops being registered. Neither is a documented place to put one, and ExtensionDiscovery already refuses to find an extension in a hidden directory at all, so a module could not be installed from one.

Data model changes

None.

Alternatives considered

Drop FOLLOW_SYMLINKS and realpath() the scan root. Tempting, because a symlinked module directory iterates correctly without the flag — passing the symlink as the root works either way, and the module's own files are still found (measured: 5,039 files reached, .info.yml among them). But testSymlink() symlinks the module's individual items into the scanned directory, so those are symlinks inside the root; dropping the flag breaks that test. The flag has to stay. What needs bounding is the depth.

Confine the scan to the web root. Cannot work: the module directory is inside the web root, and it is the symlink target that lies outside, so the check would have to reject exactly what #3482283 enabled.

A general setMaxDepth(). An arbitrary depth limit would silently stop finding hooks in legitimately deep structures. A hook quietly not registering is worse than scanning too much.

Leaving it to file_scan_ignore_directories. Already honoured since #3564112: Cache rebuild triggers file_get_contents Permission denied in theme node_modules despite file_scan_ignore_directories, and it is the right mechanism for names a site knows about — the table above shows it working. But it does not close this: the default names two directories, so .git and .devenv still get walked on a stock site, and no list of names stops a symlink that leaves the module. The guards bound the scan; the setting lets a site trim it further.

Adding .devenv and friends to the shipped default. Playing whack-a-mole with tool names, and it would still miss the symlink case. Skipping hidden directories covers the whole class at once, and matches what ExtensionDiscovery already does.

Tested

On main at 11.2.0-rc1-2036-g2bee6d53513, PHP 8.5.9, PHPUnit 12.5.17, MariaDB.

On unpatched main the two new tests fail, and the existing testSymlink() passes:

.FF                                                                 3 / 3 (100%)

1) Drupal\KernelTests\Core\Hook\HookCollectorPassTest::testHiddenDirectoryIsNotScanned
Failed asserting that an array does not have the key 'hidden_thing'.

2) Drupal\KernelTests\Core\Hook\HookCollectorPassTest::testSymlinkBelowModuleRootIsNotFollowed
Failed asserting that an array does not have the key 'outside_thing'.

Tests: 3, Assertions: 6, Failures: 2.

With the change, the whole class passes:

..................                                                18 / 18 (100%)

OK (18 tests, 62 assertions)

So the tests fail for the reason they are meant to, and nothing else in the class regresses — testSymlink() included, which is the one that has to keep working. core/phpcs.xml.dist is clean on both changed files.

AI-Generated

Yes (Used Opus 5 to draft and revise).