On Thu, Feb 24, 2022 at 3:11 PM Tim Düsterhus, WoltLab GmbH <
duester...@woltlab.com> wrote:
> Hi Internals!
>
> during code review of the "Redacting parameters in back traces" RFC [1]
> an issue with the proposed serialization behavior of
> SensitiveParameterValue objects became apparent that was not noticed
> before the RFC went into voting:
>
> The RFC proposed that serialization was allowed, but without including
> the inner value in the serialization data:
>
> public function __serialize(): array { return []; }
>
> As this operation is lossy, it was proposed that unserialization fails
> and this is what was implemented in the PoC patch:
>
> public function __unserialize(array $data): void {
> throw new \Exception('...');
> }
>
> The decision to allow serialization was to allow existing error handlers
> to work without needing to special case SensitiveParameterValue. However
> it is clearly not useful, if unserialization does not work after all.
> Any error during unserialization is not recoverable.
>
> Please find the thread in the GitHub PR at:
>
> https://github.com/php/php-src/pull/7921#discussion_r813743903
>
> As per Ilija Tovilo's suggestion I'm looping in the Internals list as well.
>
> 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.
>
> 2. Allow unserialization, but poison the unserialized object and
> disallow calling ->getValue() on it.
>
> This would be closer to the original intent of the RFC, but moves the
> issue just somewhere else: The object would not be usable either way.
>
> -------
>
> What would be your preferred option? Feel free to either reply on the
> list or add to the discussion on GitHub.
>
> Thanks!
>
> [1] https://wiki.php.net/rfc/redact_parameters_in_back_traces
>
Hi Tim,
I would prefer option 2 (if possible), to avoid potentially breaking
existing code.
Calls to ->getValue() will be in new code written specifically for
SensitiveParameterValue anyway, and can be wrapped into try-catch, I think?
Best regards,
--
Guilliam Xavier
