bug #19872 [Filesystem] Consider the umask setting when dumping a file (leofeyer)

fabpot · fabpot · commit da565de0afa1 · 2016-09-13T17:18:46.000-07:00
This PR was merged into the 3.1 branch. Discussion ---------- [Filesystem] Consider the umask setting when dumping a file | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #14246 , #17063 | License | MIT | Doc PR | - `Filesystem::dumpFile()` does not consider the umask setting and thus creates files with write permissions for everyone (`0666`). Other `chmod()` calls in Symfony do consider the umask setting (see e.g. [here](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/File/File.php#L101) or [here](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/File/UploadedFile.php#L230)), therefore I have adjusted the `dumpFile()` method accordingly. Commits ------- fdd266f Consider the umask setting when dumping a file.
diff --git a/src/Symfony/Component/Filesystem/Filesystem.php b/src/Symfony/Component/Filesystem/Filesystem.php
@@ -556,8 +556,7 @@ public function dumpFile($filename, $content)
             throw new IOException(sprintf('Failed to write file "%s".', $filename), 0, null, $filename);
         }
 
-        // Ignore for filesystems that do not support umask
-        @chmod($tmpFile, 0666);
+        @chmod($tmpFile, 0666 & ~umask());
         $this->rename($tmpFile, $filename, true);
     }
 
diff --git a/src/Symfony/Component/Filesystem/Tests/FilesystemTest.php b/src/Symfony/Component/Filesystem/Tests/FilesystemTest.php
@@ -1101,13 +1101,19 @@ public function testDumpFile()
     {
         $filename = $this->workspace.DIRECTORY_SEPARATOR.'foo'.DIRECTORY_SEPARATOR.'baz.txt';
 
+        // skip mode check on Windows
+        if ('\\' !== DIRECTORY_SEPARATOR) {
+            $oldMask = umask(0002);
+        }
+
         $this->filesystem->dumpFile($filename, 'bar');
         $this->assertFileExists($filename);
         $this->assertSame('bar', file_get_contents($filename));
 
         // skip mode check on Windows
         if ('\\' !== DIRECTORY_SEPARATOR) {
-            $this->assertFilePermissions(666, $filename);
+            $this->assertFilePermissions(664, $filename);
+            umask($oldMask);
         }
     }
 

Original file line number	Diff line number	Diff line change
`@@ -556,8 +556,7 @@ public function dumpFile($filename, $content)`
`556`	`556`	`throw new IOException(sprintf('Failed to write file "%s".', $filename), 0, null, $filename);`
`557`	`557`	`}`
`558`	`558`
`559`		`- // Ignore for filesystems that do not support umask`
`560`		`- @chmod($tmpFile, 0666);`
	`559`	`+ @chmod($tmpFile, 0666 & ~umask());`
`561`	`560`	`$this->rename($tmpFile, $filename, true);`
`562`	`561`	`}`
`563`	`562`
Original file line number	Diff line number	Diff line change
`@@ -1101,13 +1101,19 @@ public function testDumpFile()`
`1101`	`1101`	`{`
`1102`	`1102`	`$filename = $this->workspace.DIRECTORY_SEPARATOR.'foo'.DIRECTORY_SEPARATOR.'baz.txt';`
`1103`	`1103`
	`1104`	`+ // skip mode check on Windows`
	`1105`	`+ if ('\\' !== DIRECTORY_SEPARATOR) {`
	`1106`	`+ $oldMask = umask(0002);`
	`1107`	`+ }`
	`1108`	`+`
`1104`	`1109`	`$this->filesystem->dumpFile($filename, 'bar');`
`1105`	`1110`	`$this->assertFileExists($filename);`
`1106`	`1111`	`$this->assertSame('bar', file_get_contents($filename));`
`1107`	`1112`
`1108`	`1113`	`// skip mode check on Windows`
`1109`	`1114`	`if ('\\' !== DIRECTORY_SEPARATOR) {`
`1110`		`- $this->assertFilePermissions(666, $filename);`
	`1115`	`+ $this->assertFilePermissions(664, $filename);`
	`1116`	`+ umask($oldMask);`
`1111`	`1117`	`}`
`1112`	`1118`	`}`
`1113`	`1119`