From 5d07167222c2e4055686c70316c115a1ef75dc89 Mon Sep 17 00:00:00 2001 From: Fedor Indutny <79877362+indutny-signal@users.noreply.github.com> Date: Wed, 8 Mar 2023 16:58:54 -0800 Subject: [PATCH] Use libsignal-client validation for nicknames --- ts/services/username.ts | 35 ++++++++++++- ts/state/ducks/username.ts | 51 +++---------------- ts/test-electron/state/ducks/username_test.ts | 30 ----------- ts/test-node/util/Username_test.ts | 36 ------------- ts/types/Username.ts | 6 +++ ts/util/Username.ts | 26 ---------- ts/util/lookupConversationWithoutUuid.ts | 17 +++---- 7 files changed, 56 insertions(+), 145 deletions(-) delete mode 100644 ts/test-node/util/Username_test.ts diff --git a/ts/services/username.ts b/ts/services/username.ts index 2a3fb92cc4de..08fcadaa2222 100644 --- a/ts/services/username.ts +++ b/ts/services/username.ts @@ -1,7 +1,11 @@ // Copyright 2021 Signal Messenger, LLC // SPDX-License-Identifier: AGPL-3.0-only -import { usernames } from '@signalapp/libsignal-client'; +import { + usernames, + LibSignalErrorBase, + ErrorCode, +} from '@signalapp/libsignal-client'; import { singleProtoJobQueue } from '../jobs/singleProtoJobQueue'; import { strictAssert } from '../util/assert'; @@ -102,6 +106,35 @@ export async function reserveUsername( return reserveUsername(options); } } + if (error instanceof LibSignalErrorBase) { + if ( + error.code === ErrorCode.CannotBeEmpty || + error.code === ErrorCode.NicknameTooShort + ) { + return { + ok: false, + error: ReserveUsernameError.NotEnoughCharacters, + }; + } + if (error.code === ErrorCode.NicknameTooLong) { + return { + ok: false, + error: ReserveUsernameError.TooManyCharacters, + }; + } + if (error.code === ErrorCode.CannotStartWithDigit) { + return { + ok: false, + error: ReserveUsernameError.CheckStartingCharacter, + }; + } + if (error.code === ErrorCode.BadNicknameCharacter) { + return { + ok: false, + error: ReserveUsernameError.CheckCharacters, + }; + } + } throw error; } } diff --git a/ts/state/ducks/username.ts b/ts/state/ducks/username.ts index 1a6f78e92529..91fe43620e00 100644 --- a/ts/state/ducks/username.ts +++ b/ts/state/ducks/username.ts @@ -11,11 +11,6 @@ import { } from '../../types/Username'; import * as usernameServices from '../../services/username'; import type { ReserveUsernameResultType } from '../../services/username'; -import { - isValidNickname, - getMinNickname, - getMaxNickname, -} from '../../util/Username'; import { missingCaseError } from '../../util/missingCaseError'; import { sleep } from '../../util/sleep'; import { assertDev } from '../../util/assert'; @@ -166,17 +161,6 @@ export function reserveUsername( return; } - if (!isValidNickname(nickname)) { - const error = getNicknameInvalidError(nickname); - if (error) { - dispatch(setUsernameReservationError(error)); - } else { - assertDev(false, 'This should not happen'); - dispatch(setUsernameReservationError(UsernameReservationError.General)); - } - return; - } - const { username } = getMe(getState()); const abortController = new AbortController(); @@ -364,6 +348,14 @@ export function reducer( stateError = UsernameReservationError.CheckCharacters; } else if (error === ReserveUsernameError.Conflict) { stateError = UsernameReservationError.UsernameNotAvailable; + } else if (error === ReserveUsernameError.NotEnoughCharacters) { + stateError = UsernameReservationError.NotEnoughCharacters; + } else if (error === ReserveUsernameError.TooManyCharacters) { + stateError = UsernameReservationError.TooManyCharacters; + } else if (error === ReserveUsernameError.CheckStartingCharacter) { + stateError = UsernameReservationError.CheckStartingCharacter; + } else if (error === ReserveUsernameError.CheckCharacters) { + stateError = UsernameReservationError.CheckCharacters; } else { throw missingCaseError(error); } @@ -485,30 +477,3 @@ export function reducer( return state; } - -// Helpers - -function getNicknameInvalidError( - nickname: string | undefined -): UsernameReservationError | undefined { - if (!nickname) { - return undefined; - } - - if (nickname.length < getMinNickname()) { - return UsernameReservationError.NotEnoughCharacters; - } - - if (!/^[0-9a-z_]+$/.test(nickname)) { - return UsernameReservationError.CheckCharacters; - } - if (!/^[a-z_]/.test(nickname)) { - return UsernameReservationError.CheckStartingCharacter; - } - - if (nickname.length > getMaxNickname()) { - return UsernameReservationError.TooManyCharacters; - } - - return undefined; -} diff --git a/ts/test-electron/state/ducks/username_test.ts b/ts/test-electron/state/ducks/username_test.ts index 04cc6cee864c..6f7d8dafa745 100644 --- a/ts/test-electron/state/ducks/username_test.ts +++ b/ts/test-electron/state/ducks/username_test.ts @@ -131,36 +131,6 @@ describe('electron/state/ducks/username', () => { ); }); - const NICKNAME_ERROR_COMBOS = [ - ['x', UsernameReservationError.NotEnoughCharacters], - ['x'.repeat(128), UsernameReservationError.TooManyCharacters], - ['#$&^$)(', UsernameReservationError.CheckCharacters], - ['1abcdefg', UsernameReservationError.CheckStartingCharacter], - ]; - for (const [nickname, error] of NICKNAME_ERROR_COMBOS) { - // eslint-disable-next-line no-loop-func - it(`should dispatch ${error} error for "${nickname}"`, async () => { - const clock = sandbox.useFakeTimers(); - - const doReserveUsername = sinon.stub().resolves(DEFAULT_RESERVATION); - const dispatch = sinon.spy(); - - actions.reserveUsername(nickname, { - doReserveUsername, - })(dispatch, () => emptyState, null); - - await clock.runToLastAsync(); - - sinon.assert.calledOnce(dispatch); - sinon.assert.calledWith(dispatch, { - type: 'username/SET_RESERVATION_ERROR', - payload: { - error, - }, - }); - }); - } - it('should update reservation on success', () => { let state = emptyState; diff --git a/ts/test-node/util/Username_test.ts b/ts/test-node/util/Username_test.ts deleted file mode 100644 index cdec18126736..000000000000 --- a/ts/test-node/util/Username_test.ts +++ /dev/null @@ -1,36 +0,0 @@ -// Copyright 2021 Signal Messenger, LLC -// SPDX-License-Identifier: AGPL-3.0-only - -import { assert } from 'chai'; - -import * as Username from '../../util/Username'; - -describe('Username', () => { - describe('isValidUsername', () => { - const { isValidUsername } = Username; - - it('does not match invalid username searches', () => { - assert.isFalse(isValidUsername('username!')); - assert.isFalse(isValidUsername('1username')); - assert.isFalse(isValidUsername('u')); - assert.isFalse(isValidUsername('username9012345678901234567890123')); - assert.isFalse(isValidUsername('username.abc')); - }); - - it('matches valid usernames', () => { - assert.isTrue(isValidUsername('username_34')); - assert.isTrue(isValidUsername('u5ername')); - assert.isTrue(isValidUsername('_username')); - assert.isTrue(isValidUsername('use')); - assert.isTrue(isValidUsername('username901234567890123456789012')); - assert.isTrue(isValidUsername('username.0123')); - }); - - it('does not match valid and invalid usernames with @ prefix or suffix', () => { - assert.isFalse(isValidUsername('@username_34')); - assert.isFalse(isValidUsername('@1username')); - assert.isFalse(isValidUsername('username_34@')); - assert.isFalse(isValidUsername('1username@')); - }); - }); -}); diff --git a/ts/types/Username.ts b/ts/types/Username.ts index a5f59495e10a..26347f55b4e2 100644 --- a/ts/types/Username.ts +++ b/ts/types/Username.ts @@ -10,6 +10,12 @@ export type UsernameReservationType = Readonly<{ export enum ReserveUsernameError { Unprocessable = 'Unprocessable', Conflict = 'Conflict', + + // Maps to UsernameReservationError in state/ducks/usernameEnums.ts + NotEnoughCharacters = 'NotEnoughCharacters', + TooManyCharacters = 'TooManyCharacters', + CheckStartingCharacter = 'CheckStartingCharacter', + CheckCharacters = 'CheckCharacters', } export enum ConfirmUsernameResult { diff --git a/ts/util/Username.ts b/ts/util/Username.ts index d29215720eb1..477447ca0f63 100644 --- a/ts/util/Username.ts +++ b/ts/util/Username.ts @@ -13,29 +13,3 @@ export function getMaxNickname(): number { export function getMinNickname(): number { return parseIntWithFallback(RemoteConfig.getValue('global.nicknames.min'), 3); } - -export function isValidNickname(nickname: string): boolean { - if (!/^[a-z_][0-9a-z_]*$/i.test(nickname)) { - return false; - } - - if (nickname.length < getMinNickname()) { - return false; - } - - if (nickname.length > getMaxNickname()) { - return false; - } - - return true; -} - -export function isValidUsername(username: string): boolean { - const match = username.match(/^([a-z_][0-9a-z_]*)(\.\d+)?$/i); - if (!match) { - return false; - } - - const [, nickname] = match; - return isValidNickname(nickname); -} diff --git a/ts/util/lookupConversationWithoutUuid.ts b/ts/util/lookupConversationWithoutUuid.ts index a1c5e0a570ab..a29258325348 100644 --- a/ts/util/lookupConversationWithoutUuid.ts +++ b/ts/util/lookupConversationWithoutUuid.ts @@ -1,7 +1,7 @@ // Copyright 2022 Signal Messenger, LLC // SPDX-License-Identifier: AGPL-3.0-only -import { usernames } from '@signalapp/libsignal-client'; +import { usernames, LibSignalErrorBase } from '@signalapp/libsignal-client'; import { ToastFailedToFetchUsername } from '../components/ToastFailedToFetchUsername'; import { ToastFailedToFetchPhoneNumber } from '../components/ToastFailedToFetchPhoneNumber'; @@ -15,7 +15,6 @@ import { showToast } from './showToast'; import { strictAssert } from './assert'; import type { UUIDFetchStateKeyType } from './uuidFetchState'; import { getUuidsForE164s } from './getUuidsForE164s'; -import { isValidUsername } from './Username'; export type LookupConversationWithoutUuidActionsType = Readonly<{ lookupConversationWithoutUuid: typeof lookupConversationWithoutUuid; @@ -137,10 +136,6 @@ export async function lookupConversationWithoutUuid( async function checkForUsername( username: string ): Promise { - if (!isValidUsername(username)) { - return undefined; - } - const { server } = window.textsecure; if (!server) { throw new Error('server is not available!'); @@ -161,11 +156,15 @@ async function checkForUsername( username, }; } catch (error) { - if (!(error instanceof HTTPError)) { - throw error; + if (error instanceof HTTPError) { + if (error.code === 404) { + return undefined; + } } - if (error.code === 404) { + // Invalid username + if (error instanceof LibSignalErrorBase) { + log.error('checkForUsername: invalid username'); return undefined; }