mirror of
https://github.com/Cleanuparr/Cleanuparr.git
synced 2026-10-28 10:48:49 -04:00
Fix 2FA code rejected when it has spaces (#743)
This commit is contained in:
1 parent
237b02b1f1
commit
d7d8ff8afd
13 files changed
+110
-12
No files matched your search
@@ -5,6 +5,6 @@ namespace Cleanuparr.Api.Features.Auth.Contracts.Requests;
|
||||
public sealed record VerifyTotpRequest
|
||||
{
|
||||
[Required]
|
||||
[StringLength(6, MinimumLength = 6)]
|
||||
[StringLength(32)]
|
||||
public required string Code { get; init; }
|
||||
}
|
||||
@@ -68,6 +68,24 @@ public sealed class TotpServiceTests
|
||||
_sut.VerifySecondFactor(user, codes[0].Replace("-", string.Empty).ToLowerInvariant()).ShouldBeTrue();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void VerifySecondFactor_WithPaddedTotpCode_ReturnsTrue()
|
||||
{
|
||||
string secret = _sut.GenerateSecret();
|
||||
User user = CreateUser(secret);
|
||||
|
||||
_sut.VerifySecondFactor(user, $" {ComputeTotp(secret)} ").ShouldBeTrue();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void VerifySecondFactor_WithPaddedRecoveryCode_ReturnsTrue()
|
||||
{
|
||||
List<string> codes = _sut.GenerateRecoveryCodes();
|
||||
User user = CreateUser(_sut.GenerateSecret(), codes);
|
||||
|
||||
_sut.VerifySecondFactor(user, $" {codes[0]}\n").ShouldBeTrue();
|
||||
}
|
||||
|
||||
private User CreateUser(string secret, List<string>? recoveryCodes = null)
|
||||
{
|
||||
return new User
|
||||
|
||||
@@ -24,7 +24,14 @@ public sealed class TotpService : ITotpService
|
||||
/// <inheritdoc/>
|
||||
public bool ValidateCode(string secret, string code)
|
||||
{
|
||||
if (string.IsNullOrWhiteSpace(code) || code.Length != 6)
|
||||
if (string.IsNullOrWhiteSpace(code))
|
||||
{
|
||||
return false;
|
||||
}
|
||||
|
||||
string trimmedCode = code.Trim();
|
||||
|
||||
if (trimmedCode.Length != 6)
|
||||
{
|
||||
return false;
|
||||
}
|
||||
@@ -33,7 +40,7 @@ public sealed class TotpService : ITotpService
|
||||
{
|
||||
var keyBytes = Base32Encoding.ToBytes(secret);
|
||||
var totp = new Totp(keyBytes);
|
||||
return totp.VerifyTotp(code, out _, new VerificationWindow(previous: 1, future: 1));
|
||||
return totp.VerifyTotp(trimmedCode, out _, new VerificationWindow(previous: 1, future: 1));
|
||||
}
|
||||
catch
|
||||
{
|
||||
@@ -72,7 +79,7 @@ public sealed class TotpService : ITotpService
|
||||
{
|
||||
try
|
||||
{
|
||||
var normalized = code.Replace("-", "").ToUpperInvariant();
|
||||
string normalized = code.Trim().Replace("-", "").ToUpperInvariant();
|
||||
return BCrypt.Net.BCrypt.Verify(normalized, hash);
|
||||
}
|
||||
catch
|
||||
|
||||
@@ -102,7 +102,7 @@
|
||||
<app-button
|
||||
variant="primary"
|
||||
class="login-submit"
|
||||
[disabled]="totpCode().length !== 6 || loading()"
|
||||
[disabled]="totpCode().trim().length !== 6 || loading()"
|
||||
type="submit"
|
||||
>
|
||||
@if (loading()) {
|
||||
@@ -135,7 +135,7 @@
|
||||
<app-button
|
||||
variant="primary"
|
||||
class="login-submit"
|
||||
[disabled]="!recoveryCode() || loading()"
|
||||
[disabled]="!recoveryCode().trim() || loading()"
|
||||
type="submit"
|
||||
>
|
||||
@if (loading()) {
|
||||
|
||||
@@ -185,6 +185,24 @@ describe('LoginComponent', () => {
|
||||
expect(navigations).toEqual([['/dashboard']]);
|
||||
});
|
||||
|
||||
it('accepts a pasted code that carries surrounding whitespace', () => {
|
||||
const { fixture, verify2faCalls } = setup({
|
||||
login: of({ requiresTwoFactor: true, loginToken: 'login-token' }),
|
||||
});
|
||||
const component = fixture.componentInstance;
|
||||
|
||||
component.submitLogin();
|
||||
component.totpCode.set(' 123456 ');
|
||||
fixture.detectChanges();
|
||||
|
||||
expect(submitButton(fixture).disabled).toBe(false);
|
||||
|
||||
submitButton(fixture).click();
|
||||
fixture.detectChanges();
|
||||
|
||||
expect(verify2faCalls).toEqual([['login-token', '123456', undefined]]);
|
||||
});
|
||||
|
||||
it('verifies a recovery code and reports a rejected one on the recovery view', () => {
|
||||
const { fixture, verify2faCalls, navigations } = setup({
|
||||
login: of({ requiresTwoFactor: true, loginToken: 'login-token' }),
|
||||
|
||||
@@ -117,7 +117,7 @@ export class LoginComponent implements OnInit {
|
||||
this.loading.set(true);
|
||||
this.error.set('');
|
||||
|
||||
this.auth.verify2fa(this.loginToken(), this.totpCode()).subscribe({
|
||||
this.auth.verify2fa(this.loginToken(), this.totpCode().trim()).subscribe({
|
||||
next: () => {
|
||||
this.router.navigate(['/dashboard']);
|
||||
},
|
||||
@@ -137,7 +137,7 @@ export class LoginComponent implements OnInit {
|
||||
this.loading.set(true);
|
||||
this.error.set('');
|
||||
|
||||
this.auth.verify2fa(this.loginToken(), this.recoveryCode(), true).subscribe({
|
||||
this.auth.verify2fa(this.loginToken(), this.recoveryCode().trim(), true).subscribe({
|
||||
next: () => {
|
||||
this.router.navigate(['/dashboard']);
|
||||
},
|
||||
|
||||
@@ -153,7 +153,7 @@
|
||||
<app-button
|
||||
variant="primary"
|
||||
class="submit-btn"
|
||||
[disabled]="verificationCode().length !== 6"
|
||||
[disabled]="verificationCode().trim().length !== 6"
|
||||
type="submit"
|
||||
>
|
||||
Verify Code
|
||||
|
||||
@@ -191,6 +191,24 @@ describe('SetupComponent', () => {
|
||||
expect(fixture.nativeElement.querySelector('.recovery-codes')).toBeNull();
|
||||
});
|
||||
|
||||
it('accepts a padded verification code and submits it trimmed', () => {
|
||||
const { fixture, verifyTotpCalls } = setup();
|
||||
const component = fixture.componentInstance;
|
||||
|
||||
fillAccount(fixture);
|
||||
component.createAccount();
|
||||
component.verificationCode.set(' 123456 ');
|
||||
fixture.detectChanges();
|
||||
|
||||
expect(submitButton(fixture).disabled).toBe(false);
|
||||
|
||||
submitButton(fixture).click();
|
||||
fixture.detectChanges();
|
||||
|
||||
expect(verifyTotpCalls).toEqual(['123456']);
|
||||
expect(component.totpVerified()).toBe(true);
|
||||
});
|
||||
|
||||
it('gates the step three advance behind the saved recovery codes checkbox', () => {
|
||||
const { fixture } = setup();
|
||||
const component = fixture.componentInstance;
|
||||
|
||||
@@ -143,7 +143,7 @@ export class SetupComponent {
|
||||
this.loading.set(true);
|
||||
this.error.set('');
|
||||
|
||||
this.auth.verifyTotpSetup(this.verificationCode()).subscribe({
|
||||
this.auth.verifyTotpSetup(this.verificationCode().trim()).subscribe({
|
||||
next: () => {
|
||||
this.totpVerified.set(true);
|
||||
this.loading.set(false);
|
||||
|
||||
@@ -134,7 +134,7 @@
|
||||
<div class="form-actions">
|
||||
<app-button
|
||||
variant="primary"
|
||||
[disabled]="enableVerificationCode().length !== 6 || enabling2fa()"
|
||||
[disabled]="enableVerificationCode().trim().length !== 6 || enabling2fa()"
|
||||
(clicked)="verifyEnable2fa()"
|
||||
>
|
||||
@if (enabling2fa()) {
|
||||
|
||||
@@ -207,6 +207,20 @@ describe('TwoFactorCardComponent', () => {
|
||||
expect(secret(fixture)).toBeNull();
|
||||
});
|
||||
|
||||
it('enables 2FA on a pasted code that carries surrounding whitespace', () => {
|
||||
const { fixture, verifiedCodes } = setup();
|
||||
|
||||
type(fixture, 'Enter your password to enable 2FA', 'my-password');
|
||||
click(fixture, 'Enable 2FA');
|
||||
type(fixture, 'Enter 6-digit code from your app', ' 123456 ');
|
||||
|
||||
expect(button(fixture, 'Verify & Enable 2FA').disabled).toBe(false);
|
||||
|
||||
click(fixture, 'Verify & Enable 2FA');
|
||||
|
||||
expect(verifiedCodes).toEqual(['123456']);
|
||||
});
|
||||
|
||||
it('keeps the recovery codes on screen after enabling until they are dismissed', () => {
|
||||
const { fixture } = setup();
|
||||
|
||||
|
||||
@@ -109,7 +109,7 @@ export class TwoFactorCardComponent {
|
||||
|
||||
verifyEnable2fa(): void {
|
||||
this.enabling2fa.set(true);
|
||||
this.api.verifyEnable2fa(this.enableVerificationCode()).subscribe({
|
||||
this.api.verifyEnable2fa(this.enableVerificationCode().trim()).subscribe({
|
||||
next: () => {
|
||||
this.toast.success('Two-factor authentication enabled. Save your recovery codes before dismissing them!');
|
||||
this.enableSetup.set(false);
|
||||
|
||||
@@ -174,6 +174,29 @@ test.describe('Account — 2FA with recovery codes', () => {
|
||||
}
|
||||
});
|
||||
|
||||
test('codes pasted with surrounding whitespace are accepted', async ({ api, anonymousApi }) => {
|
||||
const enable = await api.account.enable2fa(TEST_CONFIG.adminPassword);
|
||||
expect(enable.ok).toBe(true);
|
||||
|
||||
const setup: TwoFactorSetup = await enable.json();
|
||||
const verify = await api.account.enable2faVerify(` ${generateTotpCode(setup.secret)} `);
|
||||
expect(verify.status).toBe(200);
|
||||
|
||||
try {
|
||||
const recoveryApi = await loginWithRecoveryCode(anonymousApi, ` ${setup.recoveryCodes[0]}\n`);
|
||||
|
||||
const disable = await recoveryApi.account.disable2fa(
|
||||
TEST_CONFIG.adminPassword,
|
||||
` ${generateTotpCode(setup.secret)} `,
|
||||
);
|
||||
|
||||
expect(disable.status).toBe(200);
|
||||
expect(await isTwoFactorEnabled(api)).toBe(false);
|
||||
} finally {
|
||||
await forceDisable(api, setup);
|
||||
}
|
||||
});
|
||||
|
||||
test('repeated bad codes at the 2FA login step are rate limited', async ({ api, anonymousApi }) => {
|
||||
const setup = await enableTwoFactor(api);
|
||||
|
||||
|
||||
Reference in new issue
Block a user