From 3f435a16ac14649ace9156fd359ff4855ab13bcd Mon Sep 17 00:00:00 2001 From: James Titcumb Date: Tue, 4 Mar 2025 09:06:21 +0000 Subject: [PATCH 1/3] Fixing various INI permissions issues --- src/File/FailedToCreateFile.php | 47 +++++++++++++++ src/File/SudoCreate.php | 58 +++++++++++++++++++ src/File/SudoUnlink.php | 2 +- src/Installing/Ini/OndrejPhpenmod.php | 4 +- .../Ini/StandardAdditionalPhpIniDirectory.php | 4 +- test/unit/File/SudoCreateTest.php | 42 ++++++++++++++ 6 files changed, 152 insertions(+), 5 deletions(-) create mode 100644 src/File/FailedToCreateFile.php create mode 100644 src/File/SudoCreate.php create mode 100644 test/unit/File/SudoCreateTest.php diff --git a/src/File/FailedToCreateFile.php b/src/File/FailedToCreateFile.php new file mode 100644 index 00000000..651f6fea --- /dev/null +++ b/src/File/FailedToCreateFile.php @@ -0,0 +1,47 @@ +getMessage(), + ), + previous: $processFailed, + ); + } +} diff --git a/src/File/SudoCreate.php b/src/File/SudoCreate.php new file mode 100644 index 00000000..e7ba4899 --- /dev/null +++ b/src/File/SudoCreate.php @@ -0,0 +1,58 @@ + Update the access and modification times of each FILE to the current time. + * + * But, the way we are using `touch` is to just create the file, hence + * the class naming is to reflect that. So; if the file already exists + * we can exit early (as we're not actually interested in updating the + * access/modification times of the file currently). + */ + if (file_exists($filename)) { + return; + } + + if (is_writable(dirname($filename))) { + $capturedErrors = []; + $touchSuccessful = CaptureErrors::for( + static fn () => touch($filename), + $capturedErrors, + ); + + if (! $touchSuccessful) { + throw FailedToCreateFile::fromTouchErrors($filename, $capturedErrors); + } + + return; + } + + if (! Sudo::exists()) { + throw FailedToCreateFile::fromNoPermissions($filename); + } + + try { + Process::run([Sudo::find(), 'touch', $filename]); + } catch (ProcessFailedException $processFailedException) { + throw FailedToCreateFile::fromSudoTouchProcessFailed($filename, $processFailedException); + } + } +} diff --git a/src/File/SudoUnlink.php b/src/File/SudoUnlink.php index 6e7efb9d..4f1bb542 100644 --- a/src/File/SudoUnlink.php +++ b/src/File/SudoUnlink.php @@ -48,6 +48,6 @@ public static function singleFile(string $filename): void return; } - FailedToUnlinkFile::fromNoPermissions($filename); + throw FailedToUnlinkFile::fromNoPermissions($filename); } } diff --git a/src/Installing/Ini/OndrejPhpenmod.php b/src/Installing/Ini/OndrejPhpenmod.php index 7d3865dc..bc39ab67 100644 --- a/src/Installing/Ini/OndrejPhpenmod.php +++ b/src/Installing/Ini/OndrejPhpenmod.php @@ -8,6 +8,7 @@ use Php\Pie\Downloading\DownloadedPackage; use Php\Pie\File\BinaryFile; use Php\Pie\File\Sudo; +use Php\Pie\File\SudoCreate; use Php\Pie\File\SudoUnlink; use Php\Pie\Platform\TargetPlatform; use Php\Pie\Util\Process; @@ -21,7 +22,6 @@ use function preg_match; use function rtrim; use function sprintf; -use function touch; use const DIRECTORY_SEPARATOR; @@ -139,7 +139,7 @@ public function setup( OutputInterface::VERBOSITY_VERY_VERBOSE, ); $pieCreatedTheIniFile = true; - touch($expectedIniFile); + SudoCreate::file($expectedIniFile); } $addingExtensionWasSuccessful = ($this->checkAndAddExtensionToIniIfNeeded)( diff --git a/src/Installing/Ini/StandardAdditionalPhpIniDirectory.php b/src/Installing/Ini/StandardAdditionalPhpIniDirectory.php index b2744360..7e668ea5 100644 --- a/src/Installing/Ini/StandardAdditionalPhpIniDirectory.php +++ b/src/Installing/Ini/StandardAdditionalPhpIniDirectory.php @@ -7,7 +7,7 @@ use Php\Pie\Downloading\DownloadedPackage; use Php\Pie\File\BinaryFile; use Php\Pie\File\Sudo; -use Php\Pie\File\SudoFilePut; +use Php\Pie\File\SudoCreate; use Php\Pie\File\SudoUnlink; use Php\Pie\Platform\TargetPlatform; use Symfony\Component\Console\Output\OutputInterface; @@ -87,7 +87,7 @@ public function setup( OutputInterface::VERBOSITY_VERY_VERBOSE, ); $pieCreatedTheIniFile = true; - SudoFilePut::contents($expectedIniFile, ''); + SudoCreate::file($expectedIniFile); } $addingExtensionWasSuccessful = ($this->checkAndAddExtensionToIniIfNeeded)( diff --git a/test/unit/File/SudoCreateTest.php b/test/unit/File/SudoCreateTest.php new file mode 100644 index 00000000..d37362cb --- /dev/null +++ b/test/unit/File/SudoCreateTest.php @@ -0,0 +1,42 @@ +remove($path); + } +} From 537bb7c13b66dc6a3025fe7176a93af2d16eb3fc Mon Sep 17 00:00:00 2001 From: James Titcumb Date: Tue, 4 Mar 2025 22:07:05 +0000 Subject: [PATCH 2/3] Write to a temporary file in SudoFilePut to avoid chmod+write-in-place approach --- src/File/FailedToWriteFile.php | 8 +++ src/File/SudoFilePut.php | 67 ++++++++++--------- test/unit/File/SudoFilePutTest.php | 50 +++++++++++++- .../Ini/AddExtensionToTheIniFileTest.php | 40 +++++++++-- .../Installing/Ini/OndrejPhpenmodTest.php | 32 ++++++--- 5 files changed, 152 insertions(+), 45 deletions(-) diff --git a/src/File/FailedToWriteFile.php b/src/File/FailedToWriteFile.php index b856ea48..134a5cb2 100644 --- a/src/File/FailedToWriteFile.php +++ b/src/File/FailedToWriteFile.php @@ -23,4 +23,12 @@ public static function fromFilePutContentErrors(string $filename, array $recorde implode("\n - ", array_column($recorded, 'message')), )); } + + public static function fromNoPermissions(string $filename): self + { + return new self(sprintf( + 'Failed to write file %s as PIE does not have enough permissions', + $filename, + )); + } } diff --git a/src/File/SudoFilePut.php b/src/File/SudoFilePut.php index 32cdcc19..fba4f94f 100644 --- a/src/File/SudoFilePut.php +++ b/src/File/SudoFilePut.php @@ -6,61 +6,64 @@ use Php\Pie\Util\CaptureErrors; use Php\Pie\Util\Process; -use Symfony\Component\Process\Exception\ProcessFailedException; +use function dirname; use function file_exists; use function file_put_contents; -use function fileperms; -use function is_string; use function is_writable; -use function sprintf; -use function substr; +use function sha1; +use function sys_get_temp_dir; + +use const DIRECTORY_SEPARATOR; /** @internal This is not public API for PIE, so should not be depended upon unless you accept the risk of BC breaks */ final class SudoFilePut { public static function contents(string $filename, string $content): void { - $didChangePermissions = false; - if (file_exists($filename)) { - $previousPermissions = substr(sprintf('%o', fileperms($filename)), -4); + $fileWritable = file_exists($filename) && is_writable($filename); + $pathWritable = ! file_exists($filename) && file_exists(dirname($filename)) && is_writable(dirname($filename)); - $didChangePermissions = self::attemptToMakeFileEditable($filename); - } + if ($fileWritable || $pathWritable) { + $capturedErrors = []; + $writeSuccessful = CaptureErrors::for( + static fn () => file_put_contents($filename, $content), + $capturedErrors, + ); - $capturedErrors = []; - $writeSuccessful = CaptureErrors::for( - static fn () => file_put_contents($filename, $content), - $capturedErrors, - ); + if ($writeSuccessful === false) { + throw FailedToWriteFile::fromFilePutContentErrors($filename, $capturedErrors); + } - if ($writeSuccessful === false) { - throw FailedToWriteFile::fromFilePutContentErrors($filename, $capturedErrors); + return; } - if (! isset($previousPermissions) || ! is_string($previousPermissions) || ! $didChangePermissions || ! Sudo::exists()) { - return; + if (! Sudo::exists()) { + throw FailedToWriteFile::fromNoPermissions($filename); } - Process::run([Sudo::find(), 'chmod', $previousPermissions, $filename]); + self::writeWithSudo($filename, $content); } - private static function attemptToMakeFileEditable(string $filename): bool + private static function writeWithSudo(string $filename, string $content): void { - if (! Sudo::exists()) { - return false; - } + $tempFilename = sys_get_temp_dir() . DIRECTORY_SEPARATOR . 'pie_tmp_' . sha1($content); - if (! is_writable($filename)) { - try { - Process::run([Sudo::find(), 'chmod', '0777', $filename]); + $capturedErrors = []; + $writeSuccessful = CaptureErrors::for( + static fn () => file_put_contents($tempFilename, $content), + $capturedErrors, + ); - return true; - } catch (ProcessFailedException) { - return false; - } + if ($writeSuccessful === false) { + throw FailedToWriteFile::fromFilePutContentErrors($tempFilename, $capturedErrors); + } + + if (file_exists($filename)) { + Process::run([Sudo::find(), 'chmod', '--reference=' . $filename, $tempFilename]); + Process::run([Sudo::find(), 'chown', '--reference=' . $filename, $tempFilename]); } - return false; + Process::run([Sudo::find(), 'mv', $tempFilename, $filename]); } } diff --git a/test/unit/File/SudoFilePutTest.php b/test/unit/File/SudoFilePutTest.php index 64707640..96f866b0 100644 --- a/test/unit/File/SudoFilePutTest.php +++ b/test/unit/File/SudoFilePutTest.php @@ -4,6 +4,7 @@ namespace Php\PieUnitTest\File; +use Composer\Util\Filesystem; use Php\Pie\File\Sudo; use Php\Pie\File\SudoFilePut; use PHPUnit\Framework\Attributes\CoversClass; @@ -11,6 +12,7 @@ use function chmod; use function file_get_contents; +use function mkdir; use function sys_get_temp_dir; use function touch; use function uniqid; @@ -20,7 +22,32 @@ #[CoversClass(SudoFilePut::class)] final class SudoFilePutTest extends TestCase { - public function testSudoFilePutContents(): void + public function testSudoFilePutContentsWithExistingWritableFile(): void + { + $file = sys_get_temp_dir() . DIRECTORY_SEPARATOR . uniqid('pie_test_sudo_file_put_contents_', true); + touch($file); + + SudoFilePut::contents($file, 'the content'); + + self::assertSame('the content', file_get_contents($file)); + + (new Filesystem())->remove($file); + } + + public function testSudoFilePutContentsWithNewFileInWritablePath(): void + { + $path = sys_get_temp_dir() . DIRECTORY_SEPARATOR . uniqid('pie_test_sudo_file_put_contents_', true); + mkdir($path); + $file = $path . DIRECTORY_SEPARATOR . 'testfile'; + + SudoFilePut::contents($file, 'the content'); + + self::assertSame('the content', file_get_contents($file)); + + (new Filesystem())->remove($path); + } + + public function testSudoFilePutContentsWithExistingUnwritableFile(): void { if (! Sudo::exists()) { self::markTestSkipped('Cannot test sudo file_put_contents without sudo'); @@ -34,5 +61,26 @@ public function testSudoFilePutContents(): void chmod($file, 777); self::assertSame('the content', file_get_contents($file)); + + (new Filesystem())->remove($file); + } + + public function testSudoFilePutContentsWithNewFileInUnwritablePath(): void + { + if (! Sudo::exists()) { + self::markTestSkipped('Cannot test sudo file_put_contents without sudo'); + } + + $path = sys_get_temp_dir() . DIRECTORY_SEPARATOR . uniqid('pie_test_sudo_file_put_contents_', true); + mkdir($path); + chmod($path, 0444); + $file = $path . DIRECTORY_SEPARATOR . 'testfile'; + + SudoFilePut::contents($file, 'the content'); + + chmod($path, 0777); + self::assertSame('the content', file_get_contents($file)); + + (new Filesystem())->remove($path); } } diff --git a/test/unit/Installing/Ini/AddExtensionToTheIniFileTest.php b/test/unit/Installing/Ini/AddExtensionToTheIniFileTest.php index 66820d77..baad3e8d 100644 --- a/test/unit/Installing/Ini/AddExtensionToTheIniFileTest.php +++ b/test/unit/Installing/Ini/AddExtensionToTheIniFileTest.php @@ -8,6 +8,7 @@ use Php\Pie\DependencyResolver\Package; use Php\Pie\ExtensionName; use Php\Pie\ExtensionType; +use Php\Pie\File\Sudo; use Php\Pie\Installing\Ini\AddExtensionToTheIniFile; use Php\Pie\Platform\TargetPhp\Exception\ExtensionIsNotLoaded; use Php\Pie\Platform\TargetPhp\PhpBinaryPath; @@ -43,15 +44,15 @@ public function setUp(): void $this->mockPhpBinary = $this->createMock(PhpBinaryPath::class); } - public function testReturnsFalseWhenFileIsNotWritable(): void + public function testReturnsFalseWhenFileIsNotWritableAndSudoDoesNotExist(): void { - if (TargetPlatform::isRunningAsRoot()) { - self::markTestSkipped('Test cannot be run as root, as root can always write files'); + if (Sudo::exists()) { + self::markTestSkipped('Test needs sudo to NOT exist'); } $unwritableFilename = tempnam(sys_get_temp_dir(), 'PIE_unwritable_ini_file'); touch($unwritableFilename); - chmod($unwritableFilename, 000); + chmod($unwritableFilename, 0444); try { self::assertFalse((new AddExtensionToTheIniFile())( @@ -79,6 +80,37 @@ public function testReturnsFalseWhenFileIsNotWritable(): void } } + public function testReturnsTrueWhenFileIsNotWritableAndSudoExists(): void + { + if (! Sudo::exists()) { + self::markTestSkipped('Test needs sudo to exist'); + } + + $unwritableFilename = tempnam(sys_get_temp_dir(), 'PIE_unwritable_ini_file'); + touch($unwritableFilename); + chmod($unwritableFilename, 0444); + + try { + self::assertTrue((new AddExtensionToTheIniFile())( + $unwritableFilename, + new Package( + $this->createMock(CompletePackage::class), + ExtensionType::PhpModule, + ExtensionName::normaliseFromString('foobar'), + 'foo/bar', + '1.0.0', + null, + ), + $this->mockPhpBinary, + $this->output, + null, + )); + } finally { + chmod($unwritableFilename, 644); + unlink($unwritableFilename); + } + } + #[RequiresOperatingSystemFamily('Linux')] public function testReturnsFalseWhenExistingIniCouldNotBeRead(): void { diff --git a/test/unit/Installing/Ini/OndrejPhpenmodTest.php b/test/unit/Installing/Ini/OndrejPhpenmodTest.php index efa76863..6d957cca 100644 --- a/test/unit/Installing/Ini/OndrejPhpenmodTest.php +++ b/test/unit/Installing/Ini/OndrejPhpenmodTest.php @@ -270,15 +270,36 @@ public function testSetupReturnsFalseWhenModsAvailablePathNotWritable(): void unlink($modsAvailablePath); mkdir($modsAvailablePath, 000, true); + $expectedIniFile = $modsAvailablePath . DIRECTORY_SEPARATOR . 'foobar.ini'; + $this->mockPhpBinary ->method('additionalIniDirectory') ->willReturn('/value/does/not/matter'); $this->checkAndAddExtensionToIniIfNeeded - ->expects(self::never()) - ->method('__invoke'); + ->expects(self::once()) + ->method('__invoke') + ->with( + $expectedIniFile, + $this->targetPlatform, + $this->downloadedPackage, + $this->output, + self::isType(IsType::TYPE_CALLABLE), + ) + ->willReturnCallback( + /** @param callable():bool $additionalEnableStep */ + static function ( + string $iniFile, + TargetPlatform $targetPlatform, + DownloadedPackage $downloadedPackage, + OutputInterface $output, + callable $additionalEnableStep, + ): bool { + return $additionalEnableStep(); + }, + ); - self::assertFalse( + self::assertTrue( (new OndrejPhpenmod( $this->checkAndAddExtensionToIniIfNeeded, self::GOOD_PHPENMOD, @@ -291,11 +312,6 @@ public function testSetupReturnsFalseWhenModsAvailablePathNotWritable(): void ), ); - self::assertStringContainsString( - 'Mods available path ' . $modsAvailablePath . ' is not writable', - $this->output->fetch(), - ); - rmdir($modsAvailablePath); } From 41007cc20f3e531dfee0c7f2377b0d9a06d80d5b Mon Sep 17 00:00:00 2001 From: James Titcumb Date: Wed, 5 Mar 2025 19:55:49 +0000 Subject: [PATCH 3/3] Use tempnam for temporary file writing --- src/File/SudoFilePut.php | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/File/SudoFilePut.php b/src/File/SudoFilePut.php index fba4f94f..d95cf571 100644 --- a/src/File/SudoFilePut.php +++ b/src/File/SudoFilePut.php @@ -11,10 +11,8 @@ use function file_exists; use function file_put_contents; use function is_writable; -use function sha1; use function sys_get_temp_dir; - -use const DIRECTORY_SEPARATOR; +use function tempnam; /** @internal This is not public API for PIE, so should not be depended upon unless you accept the risk of BC breaks */ final class SudoFilePut @@ -47,7 +45,10 @@ public static function contents(string $filename, string $content): void private static function writeWithSudo(string $filename, string $content): void { - $tempFilename = sys_get_temp_dir() . DIRECTORY_SEPARATOR . 'pie_tmp_' . sha1($content); + $tempFilename = tempnam(sys_get_temp_dir(), 'pie_tmp_'); + if ($tempFilename === false) { + throw FailedToWriteFile::fromNoPermissions($filename); + } $capturedErrors = []; $writeSuccessful = CaptureErrors::for(