Skip to content

ensure seeker is valid #382

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
peterjah wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

ensure seeker is valid #382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions gossip-sdk/src/services/message.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,19 @@ export class MessageService {
seeker: Uint8Array,
ownerUserId: string
): Promise<Message | undefined> {
// Malformed payloads can carry an empty seeker in reply/forward metadata.
// fake-indexeddb rejects that as an IndexedDB key and throws DataError.
if (!(seeker instanceof Uint8Array) || seeker.byteLength === 0) {
logger
.forMethod('findMessageBySeeker')
.warn('skipping invalid seeker lookup', {
ownerUserId,
seekerLength:
seeker instanceof Uint8Array ? seeker.byteLength : 'not-uint8array',
});
Comment on lines +434 to +443
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logging pattern here creates the logger inline rather than storing it in a const variable like other methods in this file (e.g., fetchMessages, storeDecryptedMessages). While this works correctly, for consistency with the rest of the file, consider creating a const log variable at the start of the function: const log = logger.forMethod('findMessageBySeeker'); and then using log.warn(...). This is a minor stylistic preference based on the patterns seen elsewhere in this file.

Suggested change
// Malformed payloads can carry an empty seeker in reply/forward metadata.
// fake-indexeddb rejects that as an IndexedDB key and throws DataError.
if (!(seeker instanceof Uint8Array) || seeker.byteLength === 0) {
logger
.forMethod('findMessageBySeeker')
.warn('skipping invalid seeker lookup', {
ownerUserId,
seekerLength:
seeker instanceof Uint8Array ? seeker.byteLength : 'not-uint8array',
});
const log = logger.forMethod('findMessageBySeeker');
// Malformed payloads can carry an empty seeker in reply/forward metadata.
// fake-indexeddb rejects that as an IndexedDB key and throws DataError.
if (!(seeker instanceof Uint8Array) || seeker.byteLength === 0) {
log.warn('skipping invalid seeker lookup', {
ownerUserId,
seekerLength:
seeker instanceof Uint8Array ? seeker.byteLength : 'not-uint8array',
});

Copilot uses AI. Check for mistakes.
return undefined;
}
Comment on lines +436 to +445
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The defensive check validates that the seeker is a Uint8Array and has non-zero length, but does not verify that it has the expected length of 34 bytes (SEEKER_SIZE). While the deserialization validation should ensure seekers are always 34 bytes, adding a check for the exact size would make this function more robust and would catch any potential bugs in the deserialization logic or other code paths that might pass seekers to this function. Consider adding a check like: seeker.byteLength !== 34

Copilot uses AI. Check for mistakes.

return await this.db.messages
.where('[ownerUserId+seeker]')
.equals([ownerUserId, seeker])
Expand Down
19 changes: 19 additions & 0 deletions gossip-sdk/src/utils/messageSerialization.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,19 @@ export const MESSAGE_TYPE_KEEP_ALIVE = 0x03;
// Seeker size: 1 byte length prefix + 32 bytes hash + 1 byte key index
const SEEKER_SIZE = 34;

function assertAvailable(
buffer: Uint8Array,
offset: number,
neededBytes: number,
context: string
): void {
if (buffer.length < offset + neededBytes) {
throw new Error(
`Invalid ${context} message: expected ${neededBytes} bytes at offset ${offset}, got ${Math.max(buffer.length - offset, 0)}`
);
}
}

export interface DeserializedMessage {
content: string;
replyTo?: {
Expand Down Expand Up @@ -190,18 +203,21 @@ export function deserializeMessage(buffer: Uint8Array): DeserializedMessage {
let offset = 1;

// Read original content length (4 bytes)
assertAvailable(buffer, offset, 4, 'reply');
const originalContentLen = Number(
U32.fromBytes(buffer.slice(offset, offset + 4))
);
offset += 4;

// Read original content
assertAvailable(buffer, offset, originalContentLen, 'reply');
Comment on lines +206 to +213
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertAvailable function validates that enough bytes are available for the content length itself, but it does not validate that the content length value is reasonable before attempting to validate the content bytes. If originalContentLen or forwardContentLen is an extremely large number (e.g., due to corruption), this could lead to an unhelpful error message or potential performance issues. Consider adding a sanity check on the length value (e.g., maximum message size) before using it to validate content availability.

Copilot uses AI. Check for mistakes.
const originalContent = bytesToStr(
buffer.slice(offset, offset + originalContentLen)
);
offset += originalContentLen;

// Read seeker (34 bytes)
assertAvailable(buffer, offset, SEEKER_SIZE, 'reply');
const originalSeeker = buffer.slice(offset, offset + SEEKER_SIZE);
offset += SEEKER_SIZE;

Expand All @@ -223,18 +239,21 @@ export function deserializeMessage(buffer: Uint8Array): DeserializedMessage {
let offset = 1;

// Read forward content length (4 bytes)
assertAvailable(buffer, offset, 4, 'forward');
const forwardContentLen = Number(
U32.fromBytes(buffer.slice(offset, offset + 4))
);
offset += 4;

// Read forward content
assertAvailable(buffer, offset, forwardContentLen, 'forward');
Comment on lines +242 to +249
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertAvailable function validates that enough bytes are available for the content length itself, but it does not validate that the content length value is reasonable before attempting to validate the content bytes. If forwardContentLen is an extremely large number (e.g., due to corruption), this could lead to an unhelpful error message or potential performance issues. Consider adding a sanity check on the length value (e.g., maximum message size) before using it to validate content availability.

Copilot uses AI. Check for mistakes.
const originalContent = bytesToStr(
buffer.slice(offset, offset + forwardContentLen)
);
offset += forwardContentLen;

// Read seeker (34 bytes)
assertAvailable(buffer, offset, SEEKER_SIZE, 'forward');
const originalSeeker = buffer.slice(offset, offset + SEEKER_SIZE);
offset += SEEKER_SIZE;

Expand Down
20 changes: 20 additions & 0 deletions gossip-sdk/test/utils/serialization.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,4 +117,24 @@ describe('Deserialization Failure Handling', () => {
expect(true).toBe(true);
}
});

it('throws on reply payloads with truncated seeker', () => {
const valid = serializeReplyMessage('new', 'old', serializationSeeker);
const truncated = valid.slice(0, valid.length - 10);
expect(() => deserializeMessage(truncated)).toThrow(
'Invalid reply message'
);
Comment on lines +124 to +126
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message expectation does not match the actual error message thrown by the assertAvailable function. The assertAvailable function throws a more detailed error message that includes the expected bytes, offset, and actual bytes received (e.g., "Invalid reply message: expected 34 bytes at offset X, got Y"). The test expects only "Invalid reply message" which will cause this test to fail. Consider updating the expectation to match the full error message format or use a partial string match.

Copilot uses AI. Check for mistakes.
});

it('throws on forward payloads with truncated seeker', () => {
const valid = serializeForwardMessage(
'forward',
'note',
serializationSeeker
);
const truncated = valid.slice(0, valid.length - 10);
expect(() => deserializeMessage(truncated)).toThrow(
'Invalid forward message'
);
Comment on lines +136 to +138
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message expectation does not match the actual error message thrown by the assertAvailable function. The assertAvailable function throws a more detailed error message that includes the expected bytes, offset, and actual bytes received (e.g., "Invalid forward message: expected 34 bytes at offset X, got Y"). The test expects only "Invalid forward message" which will cause this test to fail. Consider updating the expectation to match the full error message format or use a partial string match.

Copilot uses AI. Check for mistakes.
});
});