Add symlink support to Composer Stager 2.0, require that version, and simplify UX & tests accordingly

The question of symlinks is simply rife with unknowns. We don't know how symlinks behave on different OS-es and filesystems, how soft links differ from hard links, the implications of absolute vs. relative links, if Windows junctions count or whether PHP handles them differently--just to name a few. There are simply so many (untested) possibilities, and what's worse, even the ones we do understand can't necessarily be detected at runtime. Therefore, we've taken the conservative approach of banning them across the board and explicitly allowing only known, tested cases.

All risks associated with symlinks essentially boil down to the codebase or external dependencies getting corrupted at some point in the update process and in the worst case, breaking the live site. This could occur because the file sync operation failed in either direction due to platform errors (OS or PHP), because changes to symlinks have unexpected side effects (such as changing or overwriting their targets), because of interdependencies with logical or temporal coupling (such as an update that changes both a symlink and its target so that changes to the two have to be synced in the correct order), pollution of production symlink targets due to accidental sharing of linked directories between active and stage environments (such as a link that is absolute and so targets the same files or assets directory from active and stage), or by broken relative symlinks that assume a target that's present in active but absent in stage or vice versa.

One important thing we do know is that symlinks on Linux don't care if their target exists or not, whereas trying to create a symlink on Windows whose target is absent causes a PHP fatal error. That fact alone vastly expands the number of potential problems on Windows and renders many of the issues above not only possible but likely.

In light of all this, really there is no way to confidently handle symlinks other than to exclude them altogether--and even that introduces the risk of breaking symlinks from excluded directories (e.g., sites/files) by changing or removing target files in included directories.

As to the specific cases you ask about, @Wim Leers, symlinks within packages (i.e., relative symlinks with both the source and the target inside the package) are the safest--though even they could be vulnerable to coupling problems on Windows.

Symlinks in sites/files, sites, etc. are the next safest as long as they're excluded--unless they contain symlinks to files in the codebase that get changed in stage, which could result in broken symlinks or missing HTTP assets.

"Symlinks elsewhere" is an unknown quantity that we can only speculate about. Any symlink, relative or absolute, that points outside the codebase is inherently risky an can at best be excluded--granting the afore-mentioned qualification that even excluded symlinks could theoretically cause corruption or failure.

In short, "good luck, cowboy!" 🙂️ Whatever we say about symlinks in the UI or documentation should strongly warn against using them and clearly state the risks of working around the validator with exclusions.

@TravisCarden thanks for the through explicationation!

Would it be possible for an advanced user, who has read the documentation that will be added and knows there own system very well, to remove the precondition to composer stager, possibly replacing with their own custom symlink pre-condition?
Here is what I am thinking.

1. An advanced user, possibly 1 that hosts hundreds
2. Always on linux so no windows only concerns
3. Using rsync copier and has tested rsync symlink behavior
4. Finds that they commonly have composer dependencies that have relative symlinks inside the same package that prevent Automatic Updates from working. These packages may be present in their active site or be added in the staged update and prevents the update from being applied
5. Writes their own precondition that allows symlink only if they are relative within the same package(or uses someone else's
   contrib module that provides this)
6. Knows that even with all that there are still risks to allowing the relative in package symlinks but determines those risks are less than the risks not running automatic updates on the hundreds of sites for security updates and it would be to cost prohibitive apply security updates on those sites in a timely manner manually

On the Drupal side they could removethe package_manager.validator.symlink service and replace it with their own but I think when they call composer stager the current precondition will still be called before the operation and prevent it from working.

This question is based around a conversation I had at Drupalcon Prague with someone from an agency that hosts hundreds sites on their own hosting. I mentioned they would have to make the codebases writable to use automatic updates but they said that would probably be ok because currently it was cost prohibitive to manually apply updates on all these site because, a bulk of these sites were lowing paying customers and currently they just don't get security updates in a timely manner. Their higher paying customers might be a on Drupal specific hosting elsewhere and pay enough to warrant the time to update the sites manually.

I wonder if an agency like that would be willing to take the risks on their own known hosting to allow limited symlinks. I am not sure if they could swap out the precondition on composer stager with their own.

re: #6.3 regarding relative symlinks being common in packages

That's a very common case — for example https://github.com/drush-ops/drush/blob/11.4.0/docs/misc/icon_PhpStorm.png.

I think there are common in drupal project because of drush but I am not sure if they are common in general in composer packages. If we find that drush was uses a practice that is not common we could try to get drush and the 1 dependency it has that uses symlinks, to change before AU is stable in core.

UPDATE: the 1 drush dependency grasmash/yaml-expander that was using symlinks is no longer using symlinks. Also FWIW the 1 folder that was previously using symlinks was added by a drush maintainer(not saying it is bad thing, just if we are trying to determine how common this practice is, this is relevant)

I tried installing other Composer based cms's.

1. composer create-project october/october myoctober "^2.0"
2. composer create-project craftcms/craft=^1
3. composer create-project typo3/cms-base-distribution:"^10" YourNewProjectFolder
4. composer create-project -n concrete5/composer dreamrs
5. composer require joomla/application (couldn't find a project)
6. composer create-project codeigniter4/appstarter project-root
7. composer create-project goalgorilla/social_template opensocial --stability dev (to test a complex drupal application)

None of these projects had symlinks. So I don't think we can assume symlink uses inside packages are very common in the php world.

@TravisCarden I think it would be good to update the issue summary to explicitly list the types of symlink and the problems each one would have

for instance:

1. Relative symlinks inside a package where the target is inside the same package: Can't be supported because X
2. Relative symlinks inside a package where the target is outside the same package: Can't be supported because Y
3. Relative symlinks outside a package but inside a project where the target is inside the project, but not in a package: Can't be supported because Z

]

4. Relative symlinks outside a package but inside a project where the target is inside the project and in a package: Can't be supported because Z2
5. .....

I think if Windows has unique problems we don't need to list them on every different type of link.

@TravisCarden as part this list maybe you could explain the complexities that could happen if the target of the symlink changed during the update. I think I remember in our discussions there were potential problems if a symlink target changed during the update(either to another link location or to a non-symlink)

Thanks

Re: @tedbow (#5)

tl;dr: Yes, anyone could swap out the "no symlinks" precondition with their own.

Would it be possible for an advanced user, who has read the documentation that will be added and knows there own system very well, to remove the precondition to composer stager, possibly replacing with their own custom symlink pre-condition?

Yes. In fact at one point this module was doing the very same thing with the "Composer is available" precondition. It's not exactly hard to do if you know how, but that's by no means obvious. Perhaps we should document it.^[1]

On the Drupal side they could remove the package_manager.validator.symlink service and replace it with their own but I think when they call composer stager the current precondition will still be called before the operation and prevent it from working.

Actually, that would work just fine. If they wire CodebaseContainsNoSymlinksInterface to their own implementation, Composer Stager will only use theirs. (Of course, they could conditionally fall back to Composer Stager's if they wanted to.)

I wonder if an agency... could swap out the precondition on composer stager with their own.

They could even write a custom module that provided the new precondition class and altered the service definitions and just add that to all their hosted sites.

Or, if we wanted to provide the option right in our module itself to disable the check altogether, we could replace the Composer Stager precondition with our own that checks for an "ignore symlinks" config switch, and skips the check if it's on or delegates back to Composer Stager if it's not.^[2]

Re: @Wim Leers (#6)

1. I'm very sorry, but this just tells me that we should restrict this functionality to only the operating systems this was tested on

   I'm fine with that--it's a business decision. If the initiative owners accept it, I have no objections. In which case, I think the question becomes, do we create a validator for symlinks on Windows and fail if any exist? Or do we just document our lack of support?^[3]

2. composer-stager should be protecting against hard links too, and it's not! That means we should raise this to critical

   I agree. It may indeed be the case that PHP (is_link()) doesn't consider hard links to be symlinks. Composer Stager definitely needs test coverage for that case and any functional change necessary.

3. I think it's reasonable to never overwrite nor add absolute symlinks to the active directory.

   Does that mean we should test for and forbid hard links or just document that we don't support them?^[4]

4. Our primary concern should be supporting symlinks... that are relative within the current package

   You probably won't get much pushback on that. We actually talked about that early on before you came and "opened Pandora's box". 😂

5. when you use path repositories, composer will symlink it by default. This is very likely to occur in the real world.

   That's a very good point. You clarify elsewhere that "path repos won't work anyway because "active" and "stage" would both point to the same version." It's surely very edge case-y, but it may be worth considering.^[5]

6. interdependencies with logical or temporal coupling... would have to be pretty nasty composer packages to be fair. And that's exactly why I've argued to allow relative symlinks within the current package only.

   Actually, it wouldn't be a problem with the Composer packages at all--they could neither cause it nor prevent it. It's about whether the filesystem support for symlinks fails if their targets don't exist. That could occur very simply like this, for example:

   • Two files are created in staging: link.txt pointing to target_1.txt.
   • When the PHP file syncer copies them to the active directory, it needs to rename target_1.txt first or at the moment when link.txt is created it will be invalid.
   • On filesystems that fail if symlink targets are missing, that will cause a PHP exception and stop the whole process mid-operation, corrupting the live site.

   So this actually goes back to the platform-level support. And again, we know Windows would fail, but we have no idea whether other systems (including flavors of Linux) might suffer from the same vulnerability. (Ugh, which makes me think of another problem.^[6])

7. broken relative symlinks that assume a target that's present in active but absent in stage or vice versa... are not a concern anymore if we only allow relative symlinks within the current package?

   Not so. This would be another instance of the previous point.

8. I don't see how this even matters [that "trying to create a symlink on Windows whose target is absent causes a PHP fatal error"] if we don't know what we can rely on for Windows. I think making all of this *nix-only is totally fine, since 99.9% of hosting is on *nix anyway.

   That could be the case--except that the problem isn't Windows per-se, it's the filesystem Windows happens to use. Which means that the support isn't at the OS level, it's at the filesystem level. And surely there are some filesystems out there other than Windows that are *nix-compatible but also fail on "broken" symlinks. In other words, a *nix OS is no guarantee of a compatible fileystem and therefore not a strictly adequate test. Maybe we accept this risk, too, but we have to decide again whether it's enough to document the risk or actually test for it, c.f. [4].

9. I do not agree [that "really there is no way to confidently handle symlinks"] until I've seen my proposal (only allow relative symlinks within the current package) proven to not be viable.

   I'm sure your proposal is viable. The question is whether it is adequate to achieve confidence. Absolute confidence, of course, is not feasible, much less required. I'm not criticizing your proposal, by any means. But I think I have shown that it doesn't solve all of our possible problems. In other words, I think it can serve as a mitigation strategy, but we'll still have use cases left that we have to make the same business decisions about^[1], including the inherent risk of the unknown.

10. That's exactly what I'm proposing here (that "symlinks within packages are the safest--though even they could be vulnerable to coupling problems on Windows.")

    I know. 😁

11. This is going too far in the direction of "it's the user's problem" [to "strongly warn against using them and clearly state the risks of working around the validator with exclusions"]

    Yes, I understand your point. But I'm not suggesting we put the whole burden on our users and "throw it over the wall". I'm just saying that the number of risks with symlinks, known and unknown, that we can actually protect them against is unusually high, and the costs of something going wrong is very high; and we should let them know that. Drupal core does the same thing when it tells users that they should back up their site before applying database updates. We should, by all means, make things as safe as we can. But we already know that the whole thing has inherent risk, or we wouldn't have invented failure markers. I'm just saying that users need to understand this so they can make an informed decision and have a recovery strategy.

Re: @tedbow (#9)

1. I think it would be good to update the issue summary to explicitly list the types of symlink and the problems each one would have... as part this list maybe you could explain the complexities that could happen if the target of the symlink changed during the update.

   Sure. Can we make a comprehensive update once we've hashed out all the details in the comments?

2. I think I remember in our discussions there were potential problems if a symlink target changed during the update (either to another link location or to a non-symlink)

   Yes, I believe I addressed that above.

Summary of questions and takeaways

1. [1] Should we document how to override validators in the module (and/or preconditions in Composer Stager)? It's always been on my wish list to make overriding preconditions easier, but it's never been a priority. It wouldn't hurt my feelings if we wanted to do that. 🙂️
2. [2] Should we test for and forbid hard links or just document that we don't support them? Either one would probably best be done in Composer Stager.
3. [3] Shall we go ahead and prioritize adding hard link support to Composer Stager?
4. [4] Should we drop support for symlinks on Windows (filesystems) altogether? And if so, should we add a validator to forbid them or just document the risk? Also, is a blacklist good enough (i.e., forbidding Windows only)? Or do we need to take a whitelist approach (i.e., allowing known/tested cases only)?
5. [5] If we exclude some directories (as we do), it's possible that there will be inbound symlinks--from outside excluded directories pointing to targets inside directories that won't exist in the staging directory (because they've been excluded)--resulting in the afore-mentioned problem of (at least) Windows file systems corrupting live sites.
6. [6] If we exclude some directories (as we do), it's possible that there will be inbound symlinks--from outside excluded directories pointing to targets inside directories that won't exist in the staging directory (because they've been excluded)--resulting in the afore-mentioned problem of (at least) Windows file systems corrupting live sites.
7. Just thought of this: Do we need to worry about the active site root directory itself being a symlink? I'm not sure off the top of my head what Composer Stager would do in that case. Too edge case-y to worry about?

I think that covers everything--and just in time, too. The Internet ran out of blockquotes right as I finished. 😉 Back to you guys.

I chatted with @TravisCarden today about this again.

1 idea that I came up with 1 idea regarding #10

It's about whether the filesystem support for symlinks fails if their targets don't exist.

One option would be if we decided at some point to support some symlink scenarios to instead of checking for Windows specifically we could check for the problematic behavior.

For instance built into the precondition to allow only certain symlinks we could do a check where we attempt to create a symlink where the target doesn't exist yet. If we get an exception we could disallow allow symlinks. If we don't get an exception we could support symlinks in some limit scenarios(tbd what those would be)

Re #8

Since shows that common 6 PHP CMS when installed with their base dependencies do not contain any symlinks and goalgorilla/social_template as a complex drupal distro also does not contain any symlinks I think it is likely that the symlink use in drush is not a common type of usage.

I am thinking about filing an issue with drush to remove these symlinks as I don't think they are integral to drush but just used as a convenience.

It think this make it so many drupal sites would never encounter this problem and at least would mean the benefit of supporting in package relative symlink much less important.

I just holding off to see if we get more consensus here. I don't want to ask for the change in drush if we end up not needing it

Since the vast majority of these problems come from the Windows-like symlink behavior of failing if a symlink's target doesn't exist, I support a precondition for that case and a little documentation.

Hard links are also a critical problem because they can result in overwritten target files, so we should have a precondition for them, too.

Absolute links create the possibility of changing/overwriting files shared by other applications, including the live site. So we should probably forbid those, too. It probably wouldn't be too hard to detect them.

With those cases out of the way, I don't think there are any remaining known issues that could cause actual failure or corruption--even links that cross the boundary of a package. The risk in that case is just a link with a missing target. That could theoretically cause a problem, but it seems like a low probability, low impact risk. It would also be the most expensive to test for--both at design time and runtime. I don't know if we need to worry about this one at all anymore.

With all that in mind, I make the following proposal:

1. Add a precondition to requires the filesystem to allow symlinks with missing targets.
2. Add a precondition to forbid hard links.
3. Add a precondition to forbid absolute links.
4. Add a precondition to forbid links that point outside the codebase.
5. Completely forget about links that cross package boundaries, i.e., "inter-package" links.

I propose adding the new preconditions upstream in Composer Stager. (We could use it as an opportunity for knowledge transfer.)

• [1] @Wim Leers, I confirmed this while creating Add support to Infrastructure\Service\Precondition\CodeBaseContainsNoSymlinks for exclusions by TravisCarden · Pull Request #51 · php-tuf/composer-stager--Windows tests failed because of a test symlink that didn't have a valid target and passed once I fixed the symlink.
• Re: @Wim Leers, "I'm surprised you even ask this". Don't you give me your lip, mister! 😉

Per offline discussion with @tedbow, we will proceed with my proposal in #14. Updates to follow.

Here you go, @Wim Leers: https://github.com/php-tuf/composer-stager/issues/58. The rules will be documented in the new code first and then translated into a user-facing form--in the wiki, probably, we'll see.

The PR (https://github.com/php-tuf/composer-stager/pull/61) has been reviewed and is ready to be tested with the module here. (Note on testing.) Let me know how I can help.

Okay, I'm not worried about the test failures in the most recent commit. That's just straight-up due to the branch name, as far as I can tell. Everything else passes.

What I'm stuck on is...I don't understand what Wim is asking for in #26. What would these scenarios look like?

not complain about intra-package symlinks: simulate the Drush example, because that's very common

✅

not complain about inter-package symlinks

✅, but 🤔...this is really the same, if you look at it, as intra-package symlinks. The validator has no awareness of packages, or the boundaries between them. So IMHO this is not really adding anything and can be removed.

complain about a hard link

🐞 🐛 Composer Stager does something wrong here. Several of the more specific preconditions that check for unsupported symlinks will throw an IOException if the path they get is not a symlink (i.e., they will throw if they're given a hard link). This would normally not be an issue, except that the precondition that checks for hard links does NOT run first, and therefore these other preconditions might hit the hard link first and throw an exception.

The proper fix here is upstream; \PhpTuf\ComposerStager\Infrastructure\Aggregate\PreconditionsTree\NoUnsupportedLinksExist::__construct() needs to put $noHardLinksExist first.

complain about a symlink to an absolute target

✅

complain about a symlink in active to a relative target outside active

✅

complain about a symlink in stage to a relative target outside stage

✅

Hah … so this is why I sort of was wanting to have that test written here before the upstream PR was merged 🙈 But … no big deal of course! We can get that fixed upstream 😊 Can you report it? 🙏

Eh, I was pretty sure we'd encounter at least one small thing. It's literally a one-line fix, though, so that's still a pretty excellent outcome.

Clarifying #31: the thing we need to check here is that, if any .git or node_modules directory are existing under a symlink, they are still detected and excluded.

So, this is blocked on three things that need to be fixed upstream.

1. The hard link thing I detailed in #32. This change should be trivial to fix, but it's gotta be fixed upstream.
2. When I wrote test coverage for the case that Wim detailed in #31, Composer Stager started complaining. The complaint really arose from Symfony Filesystem. Pasting from a Slack conversation with @TravisCarden:
   

   When I try to copy a directory with a supported symlink, I’m getting an error from Symfony’s filesystem component: Failed to copy "/private/tmp/package_manager_testing_roottest18634147/active/custom/example" because file does not exist . custom, in this case, is a symlink. Symfony is checking is_file($path), and throwing because custom/example is indeed a directory, not a file…

   When I said this, Travis said he didn't have any test coverage for symlinks to directories. It's quite possible (likely?) that this is an actual bug in Composer Stager that has never been tested. This needs to be dealt with before this issue can move forward, since it's likely to be something we see in real sites.

3. There needs to be a tagged version of Composer Stager with symlink support before we can merge this MR.

Filed a specific issue for the hard link bug: https://github.com/php-tuf/composer-stager/issues/98

And https://github.com/php-tuf/composer-stager/issues/99 for the symlinks-to-directories weirdness.

Well...not quite.

I have yet to get the full details on this, but according to @TravisCarden:

• We are trying to test symlinks that point to directories.
• It's not working.
• Composer Stager has no test coverage for that either.
• When Travis tried to add coverage for that, he ran into some kind of rabbit hole that he says is very complicated and will take a while to fix.
• So we might have to scale back what we've got here.

Point is, this may be somewhat dragon-shaped. Will update when I have a clearer picture of where we stand.

From a conversation with @TravisCarden in Slack:

the problem with directory symlinks, as I currently understand it, is that if you copy() a symlink to a file, it copies the symlink (the source). But if you copy() a symlink to a directory, it tries to copy the directory it points to (the destination). More precisely, it refuses to try to copy it because, to quote the error, The first argument to copy() function cannot be a directory. My current thought is that in order to do this, for every file in the directory tree, we'll have to do all of the following:

1. Ask if the file is a symlink. (Disk hit.)
2. If it is, ask if it points to a directory. (Disk hit.)
3. If it does, ask what directory it points to. (Disk hit.)
4. Check whether it's a supported type, i.e., relative, within the codebase.
5. If it is supported recursively delete the target directory, because rmdir() only works on an empty directory. (Disk hit, disk hit, disk hit...)
6. Delete the directory. (Disk hit.)
7. Finally, instead of copying the symlink, create a new one in the destination. (Disk hit, obviously.)

So it can be done, but obviously, it'll be a lot of engineering effort, and it'll adversely impact performance.

After some discussion, here's the plan.

• @TravisCarden will add an additional precondition in Composer Stager which scans for symlinks to directories, and complains if it finds any. We're postponed until that's done.
• When it's done, though, we'll bring that change in, and add test coverage on our end.
• We'll create a decorated version of that precondition which exits without complaint if Package Manager is explicitly configured to use the rsync file syncer, since symlinks to directories pose no problem for rsync.

This will net us broad support for common symlinking scenarios (including the Drush problem), with very clear rules about what kind of symlinks aren't supported. That's good enough to land Package Manager in core as alpha experimental.

Given the complexity detailed in #43, it's unclear if we'll want to add support for directory symlinks later on. If we do, here's one way we could do it in Package Manager, for sites that don't have rsync:

• On pre-apply, scan the staging directory for any symlinks to directories within the codebase.
• Store that information - the paths of the symlinks, and their relative targets.
• On post-apply, re-create all of those symlinks.

However, if we do this, it's not an alpha blocker, and we can detail it in a follow-up later.

Travis merged the change I detailed in #44, so this is no longer blocked.

I think this is reviewable now.

Oooh, good catch. Yeah, I'll apply the code from #22 and also ensure we're linking to the relevant documentation in Composer Stager's code base.

Applying patch #22 means that all symlink-related documentation is removed from Package Manager's help. Which means SymlinkValidator no longer needs to link to the online help if it finds problems. The individual parts of Composer Stager's preconditions are clear and explicit enough that I don't think any documentation is needed!

This greatly simplifies SymlinkValidator -- it's now just a wrapper around the precondition, nothing more -- and allows me to remove the test coverage for its linking to the online help. 🎉

Back to Wim to confirm everything looks right.

Actually postponing this, just so nobody accidentally commits it or counts it as a "true" RTBC.

On second thought, this will need to be updated when Composer Stager tags, so maybe it's just plain postponed.

OK, found another bug while manually testing this. Luckily it's not too bad, but it does need to be fixed upstream.

(The problem is that symlink targets are read and resolved relative to the current working directory, not the directory in which the symlink itself lives, which produces false positives.)

Composer Stager 2.0.0-alpha1 has been tagged, so this is now unblocked.

To be clear: I manually tested this in a site with Drush installed. I was able to get most of the way through the update; I encountered a problem that is completely unrelated to symlinking, but I got far enough to convince me that this is where it needs to be.

Finally.

Wow, just seeing this. Feel free to reach out to me in the future. I might be grumpy sometimes but i do support this initiative.