Skip to content
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

[Filesystem] Safeguard (sym)link calls #46407

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

nicolas-grekas commented May 19, 2022

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

derrabus commented May 19, 2022

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

nicolas-grekas commented May 19, 2022

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

derrabus commented May 19, 2022

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

derrabus commented May 19, 2022

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
@derrabus
Copy link
Member Author

derrabus commented May 19, 2022

I've added a more specific error message.

xabbuh
xabbuh approved these changes May 19, 2022
@nicolas-grekas
Copy link
Member

nicolas-grekas commented May 19, 2022

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

@nicolas-grekas
Copy link
Member

nicolas-grekas commented May 19, 2022

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
@derrabus
Copy link
Member Author

derrabus commented May 19, 2022

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

nicolas-grekas commented May 19, 2022

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

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

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

@derrabus
Copy link
Member Author

derrabus commented May 20, 2022

Agreed. 👍🏻

@derrabus derrabus merged commit 2ec626f into symfony:4.4 May 20, 2022
9 of 10 checks passed
@derrabus derrabus deleted the bugfix/safeguard-symlink branch May 20, 2022
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants