Use SensitiveParameter class

[strider72]

In PHP now you can mark parameters as Sensitive, which prevents them from being logged. This is good practice with things such as passwords and 2FA secrets.

I would suggest using this any time the 2FA Secret is passed to a function as a parameter, such as in TwoFactorAuth->getQRCodeImageAsDataUri()

https://www.php.net/manual/en/class.sensitiveparameter.php

[strider72]

So:

public function getQRCodeImageAsDataUri(string $label, #[\SensitiveParameter] string $secret, int $size = 200): string

[willpower232]

good idea, would you like to make a PR?

[NicolasCARPi]

Note that this is only available in PHP 8.2. I'm not sure it's worth adding while we still support 8.1. It seems to be harmless, but might have side effects in static analysis for instance. Otherwise I'm all for it, of course!

PHP 8.1 end of security support is in 7 months, active support ended 4 months ago, so we could even go forward and require 8.2 along with this change. Users of PHP 8.1 will simply not update this library, and should already start planning upgrading their PHP version.

[willpower232]

that is a fair point, as attributes became a thing in 8.0, this would presumably generate an error rather than being ignored as a comment in earlier versions

also it looks like PHP 8.1 might get another year of support so this might have to thoroughly get stuck on the back burner

[NicolasCARPi]

so this might have to thoroughly get stuck on the back burner

Given the stable state of this library (we don't commit bugfixes every day), going with it earlier is no issue. I've recently (6 hours ago) upgraded my app to use PHP 8.3, so I would benefit from this change. Users stuck on 8.1 will simply continue using the current version, until they upgrade, and then they'll already have a fresh version with this change waiting for them!

[RobThree]

Users stuck on 8.1 will simply continue using the current version

But if we change/fix something they will be forced to upgrade first before they can use the newer version, right? And since 8.1 might get another year of support I guess we have to consider if the attribute is worth it.

Having said that it does look like a cool feature. And should any issues arise we can always backport to the attribute-less version, right?

[NicolasCARPi]

they will be forced to upgrade first before they can use the newer version, right?

No. We'll emit a bugfix on the 8.1 supported branch.

[RobThree]

That's what I meant with "And should any issues arise we can always backport to the attribute-less version, right?"

[NicolasCARPi]

Yeah sorry I didn't understand correctly the second sentence, but yeah.

[NicolasCARPi]

PR opened #119

[koreirozzu]

As #119 has been merged, this issue should be closed as resolved.

[willpower232]

Yeah we'd probably forget to close when v3 is actually released