Nyholm / psr7

A super lightweight PSR-7 implementation
MIT License
1.16k stars 75 forks source link

Broken PSR-7 immutability #198

Closed zonuexe closed 2 years ago

zonuexe commented 2 years ago

This issue is related to https://github.com/php/php-src/issues/8351 and https://github.com/vimeo/psalm/issues/7857.

Most PSR-7 interfaces ensure immutability by providing withXXX instead of setXXX, but many PSR-7 implementations expose constructors so objects can be easily modified.

$uri = $psr17Factory->createURI('http://tnyholm.se');
var_dump([spl_object_id($uri) => (string)$uri]);

$uri->__construct('https://evil.example.com/');
var_dump([spl_object_id($uri) => (string)$uri]);

This issue seems to have been mentioned in a 2017 article. https://www.simonholywell.com/post/2017/03/php-and-immutability/

And yesterday I learned about this issue by @twada at a conference in Japan called "PHPerKaigi 2022". https://speakerdeck.com/twada/growing-reliable-code-phperkaigi-2022?slide=84

PSR-7 interfaces do not define those constructors, so it's just an implementation package issue.

I don't think there are any security holes due to this issue, but it makes it easier for application implementers to implement against the PSR-7 and PSR-15 concepts.

nicolas-grekas commented 2 years ago

I replied on https://github.com/php/php-src/issues/8351 and I think this applies here to:

"Encapsulation" relates to the public API of some software design. A constructor doesn't belong to what "encapsulation" refers to. Yes, you can mutate the object. So does reflection with private properties. Calling the constructor is like using reflection to me: do it if you know why you shouldn't :shrug: ...

I'd suggest closing this as "won't fix".

Nyholm commented 2 years ago

You are very correct. But I think we should not really care in this case.

Thank you for reporting.