[9.x] Fix Illuminate Filesystem replace() leaves file executable #45856

kumarravisingh · 2023-01-29T21:07:22Z

The issue #45779 is because of changing the permission of file stored in /tmp directory giving 0775 permission via chmod() method call with umask() subtraction.

chmod($tempPath, 0777 - umask());

Here we are subtracting the umask() value from 0777
i.e 0777 - 002 = 0775 which in permission is rwxrwxr-x i.e others is getting read and execute permission

if we change the permission given to chmod($tempPath, 0776 - umask()); file will have permission
of 0776 - 002 = 0774 which in permission is rwxrwxr-- i.e other is getting read only permission

passing values in test is a bit confusing for me but I tried doing my best :

in testFilePermissionRestoredAfterReplace line 100 I have converted 774 into decimal which is after subtracting 2 of umask() from 776, I had to do the assertion with decimal value since $this->getFilePermissions($tempFile) returns permission value in decimal. ( I think this function name should be getFilePermissionsInDecimal to avoid confusion).

After that I updated the permission value in testReplaceWhenUnixSymlinkExists() test.

… executable

It seems OS is taking precedence over test permission making the permission wrxw-xr--

…s file and updated test

taylorotwell · 2023-01-30T15:26:06Z

Changing it to 776 doesn't make much sense to me. From my understanding, you should subtract from 777 for directories and 666 for files. So, changing it to 666 is more sensible to me and fixes the issue on my machine. However, a test is failing where the original test (made by a contributor some years ago) uses a "weird" umask of 131. 666 - 131 is not even a real permission afaik so the test is failing. I'm unsure of the value of this particular test or what it is even trying to test.

I have fixed the test.

dwightwatson · 2023-02-01T05:15:41Z

Just wanted to flag that this PR appears to be the cause of #45890 preventing Laravel apps from building on Heroku. Not sure exactly if this is an issue with the fix in the PR or an underlying issue with Heroku's PHP build pipeline.

potsky · 2023-02-01T13:47:33Z

Just wanted to flag that this PR appears to be the cause of #45890 preventing Laravel apps from building on Heroku. Not sure exactly if this is an issue with the fix in the PR or an underlying issue with Heroku's PHP build pipeline.

Hi @dwightwatson !

We run on an other PAAS than Heroku and we have the same behavior.

taylorotwell · 2023-02-01T14:09:05Z

I would like to get to the bottom of this. This change was correct IMO but is apparently causing build issues on PAAS providers. @dwightwatson - what is the value of "umask()" on your Heroku build pipeline? @potsky?

potsky · 2023-02-01T14:12:01Z

I cannot say because it is ran during the build and we have no way to know what is really happening. Here is the end of the build log if it can help:

  - Installing voku/simple_html_dom (4.8.7): Extracting archive
   0/237 [>---------------------------]   0%
  30/237 [===>------------------------]  12%
  50/237 [=====>----------------------]  21%
  83/237 [=========>------------------]  35%
 102/237 [============>---------------]  43%
 125/237 [==============>-------------]  52%
 145/237 [=================>----------]  61%
 172/237 [====================>-------]  72%
 196/237 [=======================>----]  82%
 219/237 [=========================>--]  92%
 237/237 [============================] 100%
Generating optimized autoload files
       composer/package-versions-deprecated: Generating version class...
       composer/package-versions-deprecated: ...done generating version class
> Illuminate\Foundation\ComposerScripts::postAutoloadDump
> @php artisan package:discover --ansi
[2023-01-31 17:23:15] production.ERROR: file_put_contents(/build/f7933c11-5b28-46c6-97d2-a5d8c78366b3/bootstrap/cache/packages.php3rSdmw): Failed to open stream: Permission denied {"exception":"[object] (ErrorException(code: 0): file_put_contents(/build/f7933c11-5b28-46c6-97d2-a5d8c78366b3/bootstrap/cache/packages.php3rSdmw): Failed to open stream: Permission denied at /build/f7933c11-5b28-46c6-97d2-a5d8c78366b3/vendor/laravel/framework/src/Illuminate/Filesystem/Filesystem.php:212)
[stacktrace]
#0 /build/f7933c11-5b28-46c6-97d2-a5d8c78366b3/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php(266): Illuminate\\Foundation\\Bootstrap\\HandleExceptions->handleError(2, 'file_put_conten...', '/build/f7933c11...', 212)
#1 [internal function]: Illuminate\\Foundation\\Bootstrap\\HandleExceptions->Illuminate\\Foundation\\Bootstrap\\{closure}(2, 'file_put_conten...', '/build/f7933c11...', 212)
#2 /build/f7933c11-5b28-46c6-97d2-a5d8c78366b3/vendor/laravel/framework/src/Illuminate/Filesystem/Filesystem.php(212): file_put_contents('/build/f7933c11...', '<?php return ar...')
#3 /build/f7933c11-5b28-46c6-97d2-a5d8c78366b3/vendor/laravel/framework/src/Illuminate/Foundation/PackageManifest.php(182): Illuminate\\Filesystem\\Filesystem->replace('/build/f7933c11...', '<?php return ar...')
#4 /build/f7933c11-5b28-46c6-97d2-a5d8c78366b3/vendor/laravel/framework/src/Illuminate/Foundation/PackageManifest.php(137): Illuminate\\Foundation\\PackageManifest->write(Array)
#5 /build/f7933c11-5b28-46c6-97d2-a5d8c78366b3/vendor/laravel/framework/src/Illuminate/Foundation/PackageManifest.php(107): Illuminate\\Foundation\\PackageManifest->build()
#6 /build/f7933c11-5b28-46c6-97d2-a5d8c78366b3/vendor/laravel/framework/src/Illuminate/Foundation/PackageManifest.php(90): Illuminate\\Foundation\\PackageManifest->getManifest()
#7 /build/f7933c11-5b28-46c6-97d2-a5d8c78366b3/vendor/laravel/framework/src/Illuminate/Foundation/PackageManifest.php(79): Illuminate\\Foundation\\PackageManifest->config('aliases')
#8 /build/f7933c11-5b28-46c6-97d2-a5d8c78366b3/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/RegisterFacades.php(26): Illuminate\\Foundation\\PackageManifest->aliases()
#9 /build/f7933c11-5b28-46c6-97d2-a5d8c78366b3/vendor/laravel/framework/src/Illuminate/Foundation/Application.php(242): Illuminate\\Foundation\\Bootstrap\\RegisterFacades->bootstrap(Object(Illuminate\\Foundation\\Application))
#10 /build/f7933c11-5b28-46c6-97d2-a5d8c78366b3/vendor/laravel/framework/src/Illuminate/Foundation/Console/Kernel.php(383): Illuminate\\Foundation\\Application->bootstrapWith(Array)
#11 /build/f7933c11-5b28-46c6-97d2-a5d8c78366b3/vendor/laravel/framework/src/Illuminate/Foundation/Console/Kernel.php(153): Illuminate\\Foundation\\Console\\Kernel->bootstrap()
#12 /build/f7933c11-5b28-46c6-97d2-a5d8c78366b3/artisan(37): Illuminate\\Foundation\\Console\\Kernel->handle(Object(Symfony\\Component\\Console\\Input\\ArgvInput), Object(Symfony\\Component\\Console\\Output\\ConsoleOutput))
#13 {main}
"} 
       
       In Filesystem.php line 212:
                                                                                      
         file_put_contents(/build/f7933c11-5b28-46c6-97d2-a5d8c78366b3/bootstrap/cac  
         he/packages.php3rSdmw): Failed to open stream: Permission denied             
                                                                                      
       
Script @php artisan package:discover --ansi handling the post-autoload-dump event returned with error code 1
!     An error occurred during buildpack compilation
 !   Error deploying the application
 !   → Invalid return code from buildpack

@taylorotwell I can ask to the PAAS team to make inspections on the build machines if you want. There are very reactive.

I can then add some commands in the build on staging to display outputs like this for example in composer.json :

    "scripts": {
        "post-autoload-dump": [
++            "@php -r \"echo 'UMASK = '.umask();\"",
            "Illuminate\\Foundation\\ComposerScripts::postAutoloadDump",
            "@php artisan package:discover --ansi"

stevenmaguire · 2023-02-01T15:47:03Z

what is the value of "umask()" on your Heroku build pipeline?

I can then add some commands in the build on staging to display outputs like this for example in composer.json :

I did exactly that on Heroku CI, here is the result:

UMASK = 63

stevenmaguire · 2023-02-01T15:53:39Z

FWIW - Heroku offers an interactive shell for the CI session. It is possible to remote in and poke around a bit if that would be useful.

taylorotwell · 2023-02-01T16:02:17Z

Thanks for sharing the umask. That's helpful. I'm used to the umask being 22 on most Linux distributions and on macOS. It being 63 leads to some weird permissions when we try to subtract it as you normally would.

stevenmaguire · 2023-02-01T16:08:17Z

My macOS shell is currently reporting UMASK = 18

¯\_(ツ)_/¯

taylorotwell · 2023-02-01T16:13:13Z

I'm curious if someone can temporarily hardcode Illuminate\Filesystem\Filesystem's chmod call in replace to this:

chmod($tempPath, 0644);

Does Heroku fail then?

stevenmaguire · 2023-02-01T16:15:59Z

I'll remote in, update the class, then run the discover command, and see what happens.

potsky · 2023-02-01T16:25:55Z

UMASK = 63 on Scalingo PAAS too → 077 in octal → so decoct(0777 - 0077); result is 700. I presume on PAAS, owner of directories is not the owner running the build so it fails. Write access for the group is a minimal requirement.

I am going to build with this line in composer.json

"@php -r \"file_put_contents('vendor/laravel/framework/src/Illuminate/Filesystem/Filesystem.php', str_replace('chmod($tempPath, 0777 - umask());', 'chmod($tempPath, 0644);', file_get_contents('vendor/laravel/framework/src/Illuminate/Filesystem/Filesystem.php')));\"",

result in a few minutes but I am quite sure it will work.

taylorotwell · 2023-02-01T16:29:47Z

OK - my hunch would be to update the replace method to just accept a third argument for the permissions you want on the file after replacement. Then update the only two places we use this framework method to explicitly pass in 0644, which is what all the other files in Laravel are.

potsky · 2023-02-01T16:31:38Z

It works

> @php -r "file_put_contents('vendor/laravel/framework/src/Illuminate/Filesystem/Filesystem.php', str_replace('chmod($tempPath, 0777 - umask());', 'chmod($tempPath, 0644);', file_get_contents('vendor/laravel/framework/src/Illuminate/Filesystem/Filesystem.php')));"
> Illuminate\Foundation\ComposerScripts::postAutoloadDump
> @php artisan package:discover --ansi
       
          INFO  Discovering packages.  
       
         ashallendesign/short-url .............................................. DONE

stevenmaguire · 2023-02-01T16:39:51Z

Yeah, Heroku dynos don't ship with a text editor, so I punted and followed @potsky's technique and it built as well.

taylorotwell · 2023-02-01T16:41:52Z

Thanks for letting me know.

potsky · 2023-02-01T16:44:56Z

@taylorotwell in PackageManifest class, the caller method is:

    protected function write(array $manifest)
    {
        if (! is_writable($dirname = dirname($this->manifestPath))) {
            throw new Exception("The {$dirname} directory must be present and writable.");
        }

        $this->files->replace(
            $this->manifestPath, '<?php return '.var_export($manifest, true).';'
        );
    }

So before replacing or writing the file in $this->files→replace, we check if the directory is writable and it is.

So why not doing something like this in Filesystem class if the goal of the last change on chmod was to leave permissions correctly on the file system ?

        $tempPath = tempnam(dirname($path), basename($path));

        chmod($tempPath, 0644);

        file_put_contents($tempPath, $content);

        // AND THEN RESTORE CORRECT PERMISSIONS 
        chmod($tempPath, 0776 - umask());

kumarravisingh added 4 commits January 29, 2023 22:32

Fixed issue laravel#45779 Illuminate Filesystem replace() leaves file…

44298c3

… executable

Added some descriptive comment in test

9a70a54

Passed permission value as string

853bf93

Changed permission to 754

8cbda39

It seems OS is taking precedence over test permission making the permission wrxw-xr--

kumarravisingh closed this Jan 29, 2023

reverted permission to 775 give read only permission to group for thi…

1f37a05

…s file and updated test

kumarravisingh reopened this Jan 30, 2023

driesvints changed the title ~~Fixed issue #45779 Illuminate Filesystem replace() leaves file executable~~ [9.x] Fix Illuminate Filesystem replace() leaves file executable Jan 30, 2023

taylorotwell added 3 commits January 30, 2023 09:16

Update Filesystem.php

f99a6e2

fix one test

cf292d2

fix test

82fffea

taylorotwell merged commit ce00bd4 into laravel:9.x Jan 30, 2023

kumarravisingh deleted the fix-illuminate-filesystem-replace-leaves-file-executable branch January 30, 2023 18:57

dwightwatson mentioned this pull request Feb 1, 2023

Build Issues with v9.49.0 #45890

Closed

nunomaduro added a commit that referenced this pull request Feb 1, 2023

Reverts #45856

634f57e

nunomaduro mentioned this pull request Feb 1, 2023

[9.x] Reverts https://github.com/laravel/framework/pull/45856 tests changes. #45905

Merged

taylorotwell pushed a commit that referenced this pull request Feb 1, 2023

Reverts #45856 (#45905)

209957e

[9.x] Fix Illuminate Filesystem replace() leaves file executable #45856

[9.x] Fix Illuminate Filesystem replace() leaves file executable #45856

Uh oh!

Conversation

kumarravisingh commented Jan 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taylorotwell commented Jan 30, 2023

Uh oh!

dwightwatson commented Feb 1, 2023

Uh oh!

potsky commented Feb 1, 2023

Uh oh!

taylorotwell commented Feb 1, 2023

Uh oh!

potsky commented Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevenmaguire commented Feb 1, 2023

Uh oh!

stevenmaguire commented Feb 1, 2023

Uh oh!

taylorotwell commented Feb 1, 2023

Uh oh!

stevenmaguire commented Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taylorotwell commented Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevenmaguire commented Feb 1, 2023

Uh oh!

potsky commented Feb 1, 2023

Uh oh!

taylorotwell commented Feb 1, 2023

Uh oh!

potsky commented Feb 1, 2023

Uh oh!

stevenmaguire commented Feb 1, 2023

Uh oh!

taylorotwell commented Feb 1, 2023

Uh oh!

potsky commented Feb 1, 2023

Uh oh!

Uh oh!

kumarravisingh commented Jan 29, 2023 •

edited

Loading

potsky commented Feb 1, 2023 •

edited

Loading

stevenmaguire commented Feb 1, 2023 •

edited

Loading

taylorotwell commented Feb 1, 2023 •

edited

Loading