Surpress chmod warnings

[peterrehm]

See https://github.com/doctrine/common/issues/381

This avoids the error chmod(): Operation not permitted when your current user has not created the file but is in the same group.

[Ocramius]

This requires testing though

[peterrehm]

I expected no other response, but when chmod was introduced with https://github.com/Ocramius/PackageVersions/commit/faf066e07c1295724d83445048895596abbb1dfb there was also no test?

How would you like it to be tested?

[Ocramius]

I would suggest an end-to-end test that does an explicit chown on a a different user.

On 21 Sep 2017 18:47, "Peter Rehm" notifications@github.com wrote:

I expected no other response, but when chmod was introduced with faf066e https://github.com/Ocramius/PackageVersions/commit/faf066e07c1295724d83445048895596abbb1dfb there was also no test?

How would you like it to be tested?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Ocramius/PackageVersions/pull/47#issuecomment-331215543, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJakJpQPveeEvoxqVqbaL74qPM2yU2hks5skpMZgaJpZM4Pfg3y .

[Ocramius]

And yeah, my bad for not adding the test in first place, sorry 😥

On 21 Sep 2017 19:36, "Marco Pivetta" ocramius@gmail.com wrote:

I would suggest an end-to-end test that does an explicit chown on a a different user.

On 21 Sep 2017 18:47, "Peter Rehm" notifications@github.com wrote:

I expected no other response, but when chmod was introduced with faf066e https://github.com/Ocramius/PackageVersions/commit/faf066e07c1295724d83445048895596abbb1dfb there was also no test?

How would you like it to be tested?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Ocramius/PackageVersions/pull/47#issuecomment-331215543, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJakJpQPveeEvoxqVqbaL74qPM2yU2hks5skpMZgaJpZM4Pfg3y .

[peterrehm]

Hehe 😄

As this more or less reverts the previous change or at least does not throw an exception when the chmod breaks I think this could be accepted without the overhead of a full E2E test.

This is no functionality just the supression of errors.

What do you think?

[Ocramius]

No, sorry, I no longer accept patches without clear verification of the change: each of these changes can lead to further bugs, so I want to have my own free time backed by automated checks.

On 21 Sep 2017 20:27, "Peter Rehm" notifications@github.com wrote:

Hehe 😄

As this more or less reverts the previous change or at least does not throw an exception when the chmod breaks I think this could be accepted without the overhead of a full E2E test.

This is no functionality just the supression of errors.

What do you think?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Ocramius/PackageVersions/pull/47#issuecomment-331241844, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJakEVaSG-8OBSJlKxACsYuLOJj9PJTks5skqp2gaJpZM4Pfg3y .

[peterrehm]

Okay, then feel free to close the PR.

[Ocramius]

@peterrehm I'm fine with keeping it open until somebody else comes up with a valid test scenario ;-)

[peterrehm]

Btw. another solution would have been deleting the file prior to the chown.

[peterrehm]

And this is the reason for the issue: https://unix.stackexchange.com/a/125787

[peterrehm]

It has been fixed thiws way e.g. in Twig:

https://github.com/twigphp/Twig/pull/705

Another option would be to remove the file prior to writing it, a user in the same group can do this and afterwards the file has the current user as the owner and a chmod is permitted.

Prior to introducing the chmod command I had no issues.

[Ocramius]

Yep

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

On Fri, Sep 22, 2017 at 11:22 AM, Claudio Zizza notifications@github.com wrote:

@SenseException commented on this pull request.

In src/PackageVersions/Installer.php https://github.com/Ocramius/PackageVersions/pull/47#discussion_r140448106 :

@@ -124,7 +124,7 @@ private static function writeVersionClassToFile(string $versionClassSource, Comp $io->write('ocramius/package-versions: Generating version class...');

     file_put_contents($installPath, $versionClassSource);

• chmod($installPath, 0664);
• @chmod($installPath, 0664);

"Hard" as in \RuntimeException and a $io->write('ocramius/package-versions: You ruined my life!!1!') before the exception?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Ocramius/PackageVersions/pull/47#discussion_r140448106, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJakJkGym2eJpHYcnbPU6Xxl-d7YE-Sks5sk3xGgaJpZM4Pfg3y .

[Ocramius]

Yes, not every lib applied that patch, including composer.

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

On Tue, Nov 28, 2017 at 8:15 AM, Sergei Morozov notifications@github.com wrote:

@morozov commented on this pull request.

In src/PackageVersions/Installer.php https://github.com/Ocramius/PackageVersions/pull/47#discussion_r153410047 :

@@ -124,7 +124,7 @@ private static function writeVersionClassToFile(string $versionClassSource, Comp $io->write('ocramius/package-versions: Generating version class...');

     file_put_contents($installPath, $versionClassSource);

• chmod($installPath, 0664);
• @chmod($installPath, 0664);

So if I set umask 0 and run composer install, then the resulting files look like this:

↪ ls -l vendor/ocramius/package-versions/src/PackageVersions/ total 20 -rw-rw-rw- 1 morozov morozov 2582 Sep 6 08:24 FallbackVersions.php -rw-rw-rw- 1 morozov morozov 5468 Sep 6 08:24 Installer.php -rw-rw-r-- 1 morozov morozov 4558 Nov 27 23:06 Versions.php

The auto-generated file is indeed not writable by others but the rest of the files are. The attacker still can write code into other files.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Ocramius/PackageVersions/pull/47#discussion_r153410047, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJakPsSSiBq3tySRBoVV7PAd2Gy8iCKks5s67MigaJpZM4Pfg3y .

[peterrehm]

I am closing this as it looks like this wont be fixed.

[Ocramius]

@peterrehm thanks for the feedback anyway!