[X] I have checked my logs and I'm sure is a bug in this package.
[X] I can reproduce this bug in isolation (vanilla Laravel install)
[X] I can suggest a workaround as a Pull Request
Expectation
The label is properly URL encoded when generating the url.
The label does not contain the issuer twice.
Description
Hi,
I found 2 bugs during testing on an Apple device (IPhone 6S, Ios 15.7.1)
When using generating the QR code
\Laragear\TwoFactor\Models\Concerns\SerializesSharedSecret::toQr It uses \Laragear\TwoFactor\Models\Concerns\SerializesSharedSecret::toUri to build the otpauth uri.
It returns the url like this:
return 'otpauth://totp/'.rawurlencode($issuer).'%3A'.$this->attributes['label']."?$query";
The $this->attributes['label'] is not urlencoded like the issuer and this causes an issue when testing generating a QR code on an Apple device.
Instead of 'App name:user@example.org' the user was rendered as 'App%20name:user@example.org'.
There's a second issue to this, of which I think also is unwanted behavior, it by defaults creates a label based on the issuer + the e-mail of the user. https://github.com/Laragear/TwoFactor/blob/v1.2.2/src/TwoFactorAuthentication.php#L106
But the specs (think these are the official specs, could not find any other)
Instead of just passing on the label or using label + email, the module combines label + (label + email), which includes the label unnecessary twice. So I think the default label twoFactorLabel() should not include the issuer by default and think it would be a good practice to always include it in the otpauth url.
Can't find label as an official parameter for otpauth://TYPE/LABEL?PARAMETERS either, although the module adds it to the query parameters, I don't think it's used.
Summary;
Encode the full concat string: rawurlencode($issuer . ':' . $this->attributes['label']).
Remove issuer from label when always adding / forcing an issuer on the url.
I've mitigated this issue by overriding the following method on the model
protected function twoFactorLabel(): string
Reproduction
* Set APP name with space like: 'The Application'
* Create TwoFactorAuthentication model
* Create QR code for the model
* Scan the code
* View result on a IOS device
DarkGhostHunter
commented
6 months ago
PHP & Platform
8.1 - Ubuntu 22.04
Database
No response
Laravel version
10.x
Have you done this?
Expectation
Description
Hi,
I found 2 bugs during testing on an Apple device (IPhone 6S, Ios 15.7.1)
When using generating the QR code
\Laragear\TwoFactor\Models\Concerns\SerializesSharedSecret::toQrIt uses\Laragear\TwoFactor\Models\Concerns\SerializesSharedSecret::toUrito build the otpauth uri.It returns the url like this:
return 'otpauth://totp/'.rawurlencode($issuer).'%3A'.$this->attributes['label']."?$query";The
$this->attributes['label']is not urlencoded like the issuer and this causes an issue when testing generating a QR code on an Apple device.Instead of 'App name:user@example.org' the user was rendered as 'App%20name:user@example.org'.
There's a second issue to this, of which I think also is unwanted behavior, it by defaults creates a label based on the issuer + the e-mail of the user.
https://github.com/Laragear/TwoFactor/blob/v1.2.2/src/TwoFactorAuthentication.php#L106But the specs (think these are the official specs, could not find any other)
State: otpauth://TYPE/LABEL?PARAMETERS
Instead of just passing on the label or using label + email, the module combines label + (label + email), which includes the label unnecessary twice. So I think the default label
twoFactorLabel()should not include the issuer by default and think it would be a good practice to always include it in the otpauth url.Can't find label as an official parameter for otpauth://TYPE/LABEL?PARAMETERS either, although the module adds it to the query parameters, I don't think it's used.
Summary;
rawurlencode($issuer . ':' . $this->attributes['label']).I've mitigated this issue by overriding the following method on the model
protected function twoFactorLabel(): stringReproduction
Stack trace & logs
No response