From 9c2552c8157b7700cfb1d95c7af56d33d162e23a Mon Sep 17 00:00:00 2001 From: Ry Racherbaumer Date: Tue, 28 Nov 2023 15:12:02 -0600 Subject: [PATCH 1/6] fix: filter out conversations with invalid topics --- src/conversations/Conversations.ts | 35 ++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/src/conversations/Conversations.ts b/src/conversations/Conversations.ts index efabd1e86..df7fa205a 100644 --- a/src/conversations/Conversations.ts +++ b/src/conversations/Conversations.ts @@ -25,6 +25,21 @@ const messageHasHeaders = (msg: MessageV1): boolean => { return Boolean(msg.recipientAddress && msg.senderAddress) } +// validate that a topic only contains ASCII characters +const isValidTopic = (topic: string): boolean => { + // eslint-disable-next-line no-control-regex + const regex = /^[\x00-\x7F]+$/ + const index = topic.indexOf('0/') + if (index !== -1) { + const unwrappedTopic = topic.substring( + index + 2, + topic.lastIndexOf('/proto') + ) + return regex.test(unwrappedTopic) + } + return false +} + /** * Conversations allows you to view ongoing 1:1 messaging sessions with another wallet */ @@ -81,14 +96,14 @@ export default class Conversations { }) await this.client.keystore.saveV1Conversations({ - conversations: Array.from(seenPeers).map( - ([peerAddress, createdAt]) => ({ + conversations: Array.from(seenPeers) + .map(([peerAddress, createdAt]) => ({ peerAddress, createdNs: dateToNs(createdAt), topic: buildDirectMessageTopic(peerAddress, this.client.address), context: undefined, - }) - ), + })) + .filter((c) => isValidTopic(c.topic)), }) return ( @@ -158,11 +173,13 @@ export default class Conversations { shouldThrow = false ): Promise[]> { const { responses } = await this.client.keystore.saveInvites({ - requests: envelopes.map((env) => ({ - payload: env.message as Uint8Array, - timestampNs: Long.fromString(env.timestampNs as string), - contentTopic: env.contentTopic as string, - })), + requests: envelopes + .map((env) => ({ + payload: env.message as Uint8Array, + timestampNs: Long.fromString(env.timestampNs as string), + contentTopic: env.contentTopic as string, + })) + .filter((req) => isValidTopic(req.contentTopic)), }) const out: ConversationV2[] = [] From 9e6bbffc06daf2b0ebb5ad14429f23552d354ae3 Mon Sep 17 00:00:00 2001 From: Ry Racherbaumer Date: Tue, 28 Nov 2023 20:10:50 -0600 Subject: [PATCH 2/6] refactor: move isValidTopic to utils/topic --- src/conversations/Conversations.ts | 16 +--------------- src/utils/topic.ts | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/conversations/Conversations.ts b/src/conversations/Conversations.ts index df7fa205a..ab1f5a4d4 100644 --- a/src/conversations/Conversations.ts +++ b/src/conversations/Conversations.ts @@ -12,6 +12,7 @@ import { buildUserIntroTopic, buildUserInviteTopic, dateToNs, + isValidTopic, nsToDate, } from '../utils' import { PublicKeyBundle } from '../crypto' @@ -25,21 +26,6 @@ const messageHasHeaders = (msg: MessageV1): boolean => { return Boolean(msg.recipientAddress && msg.senderAddress) } -// validate that a topic only contains ASCII characters -const isValidTopic = (topic: string): boolean => { - // eslint-disable-next-line no-control-regex - const regex = /^[\x00-\x7F]+$/ - const index = topic.indexOf('0/') - if (index !== -1) { - const unwrappedTopic = topic.substring( - index + 2, - topic.lastIndexOf('/proto') - ) - return regex.test(unwrappedTopic) - } - return false -} - /** * Conversations allows you to view ongoing 1:1 messaging sessions with another wallet */ diff --git a/src/utils/topic.ts b/src/utils/topic.ts index 5290cbf51..6bff0ec57 100644 --- a/src/utils/topic.ts +++ b/src/utils/topic.ts @@ -35,3 +35,18 @@ export const buildUserPrivateStoreTopic = (addrPrefixedKey: string): string => { // e.g. "0x1111111111222222222233333333334444444444/key_bundle" return buildContentTopic(`privatestore-${addrPrefixedKey}`) } + +// validate that a topic only contains ASCII characters +export const isValidTopic = (topic: string): boolean => { + // eslint-disable-next-line no-control-regex + const regex = /^[\x00-\x7F]+$/ + const index = topic.indexOf('0/') + if (index !== -1) { + const unwrappedTopic = topic.substring( + index + 2, + topic.lastIndexOf('/proto') + ) + return regex.test(unwrappedTopic) + } + return false +} From d22918d3e21e25e1ed87f6717bbc8c80f3135b68 Mon Sep 17 00:00:00 2001 From: Ry Racherbaumer Date: Tue, 28 Nov 2023 20:30:08 -0600 Subject: [PATCH 3/6] refactor: narrow character range --- src/utils/topic.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/utils/topic.ts b/src/utils/topic.ts index 6bff0ec57..7e4b25e63 100644 --- a/src/utils/topic.ts +++ b/src/utils/topic.ts @@ -36,10 +36,10 @@ export const buildUserPrivateStoreTopic = (addrPrefixedKey: string): string => { return buildContentTopic(`privatestore-${addrPrefixedKey}`) } -// validate that a topic only contains ASCII characters +// validate that a topic only contains ASCII characters 33-127 export const isValidTopic = (topic: string): boolean => { // eslint-disable-next-line no-control-regex - const regex = /^[\x00-\x7F]+$/ + const regex = /^[\x21-\x7F]+$/ const index = topic.indexOf('0/') if (index !== -1) { const unwrappedTopic = topic.substring( From bfbb7f30ae5d095051dd77c4b2a821b3675c2584 Mon Sep 17 00:00:00 2001 From: Ry Racherbaumer Date: Tue, 28 Nov 2023 20:30:39 -0600 Subject: [PATCH 4/6] test: add tests --- test/utils/topic.test.ts | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 test/utils/topic.test.ts diff --git a/test/utils/topic.test.ts b/test/utils/topic.test.ts new file mode 100644 index 000000000..d28bc379b --- /dev/null +++ b/test/utils/topic.test.ts @@ -0,0 +1,26 @@ +import { buildContentTopic, isValidTopic } from '../../src/utils/topic' + +describe('topic utils', () => { + describe('isValidTopic', () => { + it('validates topics correctly', () => { + expect(isValidTopic(buildContentTopic('foo'))).toBe(true) + expect(isValidTopic(buildContentTopic('123'))).toBe(true) + expect(isValidTopic(buildContentTopic('bar987'))).toBe(true) + expect(isValidTopic(buildContentTopic('*&+-)'))).toBe(true) + expect(isValidTopic(buildContentTopic('%#@='))).toBe(true) + expect(isValidTopic(buildContentTopic('<;.">'))).toBe(true) + expect(isValidTopic(buildContentTopic(String.fromCharCode(33)))).toBe( + true + ) + expect(isValidTopic(buildContentTopic('∫ß'))).toBe(false) + expect(isValidTopic(buildContentTopic('\xA9'))).toBe(false) + expect(isValidTopic(buildContentTopic('\u2665'))).toBe(false) + expect(isValidTopic(buildContentTopic(String.fromCharCode(1)))).toBe( + false + ) + expect(isValidTopic(buildContentTopic(String.fromCharCode(23)))).toBe( + false + ) + }) + }) +}) From 01757fba14bb55ed00a9745fd9d46285b6641fe1 Mon Sep 17 00:00:00 2001 From: Ry Racherbaumer Date: Wed, 29 Nov 2023 14:12:21 -0600 Subject: [PATCH 5/6] test: add random topic testing --- test/utils/topic.test.ts | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/test/utils/topic.test.ts b/test/utils/topic.test.ts index d28bc379b..1f0879754 100644 --- a/test/utils/topic.test.ts +++ b/test/utils/topic.test.ts @@ -1,4 +1,9 @@ -import { buildContentTopic, isValidTopic } from '../../src/utils/topic' +import { + buildContentTopic, + buildDirectMessageTopicV2, + isValidTopic, +} from '../../src/utils/topic' +import crypto from '../../src/crypto/crypto' describe('topic utils', () => { describe('isValidTopic', () => { @@ -22,5 +27,22 @@ describe('topic utils', () => { false ) }) + + it('validates random topics correctly', () => { + const topics = Array.from({ length: 100 }).map(() => + buildDirectMessageTopicV2( + Buffer.from(crypto.getRandomValues(new Uint8Array(32))) + .toString('base64') + .replace(/=*$/g, '') + // Replace slashes with dashes so that the topic is still easily split by / + // We do not treat this as needing to be valid Base64 anywhere + .replace('/', '-') + ) + ) + + topics.forEach((topic) => { + expect(isValidTopic(topic)).toBe(true) + }) + }) }) }) From b4836dff46f0f2a4b6df69769459cf252bf3d32a Mon Sep 17 00:00:00 2001 From: Ry Racherbaumer Date: Thu, 30 Nov 2023 13:51:48 -0600 Subject: [PATCH 6/6] fix: ensure slashes aren't part of topics --- src/Invitation.ts | 2 +- test/utils/topic.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Invitation.ts b/src/Invitation.ts index b9286d2b7..07cecd8f5 100644 --- a/src/Invitation.ts +++ b/src/Invitation.ts @@ -47,7 +47,7 @@ export class InvitationV1 implements invitation.InvitationV1 { .replace(/=*$/g, '') // Replace slashes with dashes so that the topic is still easily split by / // We do not treat this as needing to be valid Base64 anywhere - .replace('/', '-') + .replace(/\//g, '-') ) const keyMaterial = crypto.getRandomValues(new Uint8Array(32)) diff --git a/test/utils/topic.test.ts b/test/utils/topic.test.ts index 1f0879754..2baf3d6e0 100644 --- a/test/utils/topic.test.ts +++ b/test/utils/topic.test.ts @@ -36,7 +36,7 @@ describe('topic utils', () => { .replace(/=*$/g, '') // Replace slashes with dashes so that the topic is still easily split by / // We do not treat this as needing to be valid Base64 anywhere - .replace('/', '-') + .replace(/\//g, '-') ) )