Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:117141
MIME-Version: 1.0
References: <b9d3740d-7664-44ea-318d-bcb516ad5c87@woltlab.com>
 <CAC5V6ghjiDN4ZOzJaikVrKoGoA3atLu4zL2zodvEHhZ2tW0ZAA@mail.gmail.com> <b682e187-f338-87fd-cfae-a9004d4a9955@woltlab.com>
In-Reply-To: <b682e187-f338-87fd-cfae-a9004d4a9955@woltlab.com>
Date: Sat, 26 Feb 2022 11:49:13 +0000
Message-ID: <CA+kxMuQZreLZvN47nvSF5_CFM5N=_dHvrXyO2XXG5+TWg7kY9Q@mail.gmail.com>
To: =?UTF-8?Q?Tim_D=C3=BCsterhus=2C_WoltLab_GmbH?= <duesterhus@woltlab.com>
Cc: Guilliam Xavier <guilliam.xavier@gmail.com>, PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] SensitiveParameterValue serialization behavior
From: Danack@basereality.com (Dan Ackroyd)

On Thu, 24 Feb 2022 at 14:11, Tim D=C3=BCsterhus, WoltLab GmbH
<duesterhus@woltlab.com> wrote:
>
> I see two possible options to remediate this issue:
>
> -------
>
> 1. Disallow both serialization and unserialization.
>
> This will make the serialization issue very obvious, but will require
> adjustments to exception handlers that serialize the stack traces.

That sounds the best option. Yes, people who serialize/unserialize
stack traces will need to take into account the new behaviour, but
they will need to do that anyway, and it's better for something to
fail early, when the data is serialized rather than it happen later
when someone goes to use that data. Programs continuing when they have
already failed is Bad=E2=84=A2.

> Allow unserialization, but poison the unserialized object and
> disallow calling ->getValue() on it.

That sounds like something to be instantly added to the list of PHP Sadness=
.

> I theoretically could add an additional public function isPoisoned(): boo=
l as well.

To me, that sounds like a workaround needed by a bad design aka a hack.

Guilliam Xavier wrote:
> to avoid potentially breaking existing code.

Technically, all existing code will continue to run, as no-one is
currently using SensitiveParameter and so that code will continue to
run. When people start using that feature, then yes they will need to
make sure that any stack serializer is aware of the new feature. But
that doesn't sound like a huge problem to me.

In general, I think we should only add surprising and awkward apis
when there is a really strong reason for doing so, not because there
might be a problem. If it's left as unserializable for now, people
would have the opportunity for saying why it needs to be relaxed
later, aka "no is temporary, yes is forever" for features.

cheers
Dan
Ack