Skip to content

[Filesystem] Safeguard (sym)link calls #46407

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 20, 2022

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented May 19, 2022

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets #46402
License MIT
Doc PR N/A

This PR let's link and symlink operations fail if the corresponding PHP functions are unavailable, e.g. because they have been disabled. Without the patch, the user receives a cryptic TypeError in such cases.

I'm not really sure, if I can write tests for this scenario because we cannot disable those functions at runtime, can we?

@nicolas-grekas
Copy link
Member

This is trading an exception for another one, isn't it? Would this really solve the linked issue, if it's solvable?

@derrabus
Copy link
Member Author

Yes, instead of a TypeError that confuses the user by telling them that something is not a callable, they would now get an exception simply saying that linking failed. I don't think that the issue is actually solvable.

@nicolas-grekas
Copy link
Member

We could then throw a more specific exception, telling that symlink is disabled.
And I also think Composer should consider not trying to symlink when the function doesn't exist.

@derrabus
Copy link
Member Author

Alternatively, we could replace the passed 'symlink' string with a closure:

$symlink = static function($originDir, $targetDir) {
    return symlink($originDir, $targetDir);
};

if (!self::box($symlink, $originDir, $targetDir)) {
    $this->linkException($originDir, $targetDir, 'symbolic');
}

This way, the user would get a fatal error Call to undefined function symlink(). PHP 8.1 would allow us to achieve the same thing more elegantly, see #46408 for a patch against the 6.1 branch.

@derrabus
Copy link
Member Author

I also think Composer should consider not trying to symlink when the function doesn't exist.

Symlinking could fail for other reasons, so attempting the symlink and catching Symfony's IOException should be the proper workflow. Let's throw this exception also when the function is disabled.

@derrabus derrabus force-pushed the bugfix/safeguard-symlink branch from 67df83c to 9647d10 Compare May 19, 2022 11:47
@derrabus
Copy link
Member Author

I've added a more specific error message.

@nicolas-grekas
Copy link
Member

If we go with #46408, we should ensure that the behavior will be the same in both branches.

@nicolas-grekas
Copy link
Member

Using Closure::fromCallable will throw TypeError: Failed to create closure from callable: function "foo" not found or invalid function name
Using foo(...) will throw Error: Call to undefined function foo()

We need to chose one from TypeError, Error or IOException :)

@nicolas-grekas
Copy link
Member

nicolas-grekas commented May 19, 2022

I think we could remove the callable type hint on the box() method (replace it by string), and do the check there, with a nice IOException.

@derrabus derrabus force-pushed the bugfix/safeguard-symlink branch from 9647d10 to 17e612a Compare May 19, 2022 13:04
@derrabus
Copy link
Member Author

What I like about the current approach is that we'd do an early exit and skip the whole operation if the function is unavailable.

@nicolas-grekas
Copy link
Member

Let's do both then: add the same check in box after changing its type hint, just in case any other methods are disabled?

@nicolas-grekas nicolas-grekas force-pushed the bugfix/safeguard-symlink branch from 17e612a to c15028f Compare May 20, 2022 08:49
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I've updated the patch to include my suggestion)

@derrabus
Copy link
Member Author

Agreed. 👍🏻

@derrabus derrabus merged commit 2ec626f into symfony:4.4 May 20, 2022
@derrabus derrabus deleted the bugfix/safeguard-symlink branch May 20, 2022 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants