Skip to content
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.

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

chore: prevent unchecked index access #1612

Closed
wants to merge 16 commits into from
6 changes: 3 additions & 3 deletions packages/core/src/lib/filter/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ class Subscription {
async (source) => await all(source)
);

if (!res || !res.length) {
if (!res || !res.length || !res[0]) {
weboko marked this conversation as resolved.
Show resolved Hide resolved
throw Error(
`No response received for request ${request.requestId}: ${res}`
);
Expand Down Expand Up @@ -203,7 +203,7 @@ class Subscription {
async (source) => await all(source)
);

if (!res || !res.length) {
if (!res || !res.length || !res[0]) {
throw Error(
`No response received for request ${request.requestId}: ${res}`
);
Expand Down Expand Up @@ -246,7 +246,7 @@ class Subscription {
async (source) => await all(source)
);

if (!res || !res.length) {
if (!res || !res.length || !res[0]) {
throw Error(
`No response received for request ${request.requestId}: ${res}`
);
Expand Down
5 changes: 4 additions & 1 deletion packages/core/src/lib/filterPeers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,10 @@ export function filterPeersByDiscovery(
// Fill up to numPeers with remaining random peers if needed
while (selectedPeers.length < numPeers && nonBootstrapPeers.length > 0) {
const randomIndex = Math.floor(Math.random() * nonBootstrapPeers.length);
const randomPeer = nonBootstrapPeers.splice(randomIndex, 1)[0];
const randomPeer = nonBootstrapPeers[randomIndex];
if (!randomPeer) {
continue;
}
selectedPeers.push(randomPeer);
}

Expand Down
9 changes: 8 additions & 1 deletion packages/core/src/lib/store/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -237,9 +237,12 @@ class Store extends BaseProtocol implements IStore {
);
}

// we can be certain that there is only one pubsub topic in the query
const pubsubTopicForQuery = uniquePubsubTopicsInQuery[0];

if (!pubsubTopicForQuery) {
throw Error("Cannot find a pubsub topic to use for query.");
}

ensurePubsubTopicIsConfigured(pubsubTopicForQuery, this.pubsubTopics);

// check that the pubsubTopic from the Cursor and Decoder match
Expand Down Expand Up @@ -287,6 +290,10 @@ class Store extends BaseProtocol implements IStore {
})
)[0];

if (!peer) {
throw Error("Failed executing query: missing a peer.");
}

for await (const messages of paginate<T>(
this.getStream.bind(this, peer),
queryOpts,
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/lib/wait_for_remote_peer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@

if (peers.length) {
if (!metadataService) {
log.info(`${codec} peer found: `, peers[0].id.toString());
log.info(`${codec} peer found: `, peers[0]?.id.toString());
return;
}

Expand All @@ -99,7 +99,7 @@
);
return;
} catch (e) {
if ((e as any).code === "ERR_CONNECTION_BEING_CLOSED")

Check warning on line 102 in packages/core/src/lib/wait_for_remote_peer.ts

View workflow job for this annotation

GitHub Actions / proto

Unexpected any. Specify a different type

Check warning on line 102 in packages/core/src/lib/wait_for_remote_peer.ts

View workflow job for this annotation

GitHub Actions / check

Unexpected any. Specify a different type
log.error(
`Connection with the peer was closed and possibly because it's on a different shard. Error: ${e}`
);
Expand Down
34 changes: 24 additions & 10 deletions packages/dns-discovery/src/dns.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import type {
import { Logger } from "@waku/utils";

import { DnsOverHttps } from "./dns_over_https.js";
import { ENRTree } from "./enrtree.js";
import { ENRTree, ENRTreeValues } from "./enrtree.js";
import {
fetchNodesUntilCapabilitiesFulfilled,
yieldNodesUntilCapabilitiesFulfilled
Expand Down Expand Up @@ -41,8 +41,7 @@ export class DnsNodeDiscovery {
enrTreeUrls: string[],
wantedNodeCapabilityCount: Partial<NodeCapabilityCount>
): Promise<IEnr[]> {
const networkIndex = Math.floor(Math.random() * enrTreeUrls.length);
const { publicKey, domain } = ENRTree.parseTree(enrTreeUrls[networkIndex]);
const { publicKey, domain } = DnsNodeDiscovery.parseTree(enrTreeUrls);
const context: SearchContext = {
domain,
publicKey,
Expand Down Expand Up @@ -78,8 +77,7 @@ export class DnsNodeDiscovery {
enrTreeUrls: string[],
wantedNodeCapabilityCount: Partial<NodeCapabilityCount>
): AsyncGenerator<IEnr> {
const networkIndex = Math.floor(Math.random() * enrTreeUrls.length);
const { publicKey, domain } = ENRTree.parseTree(enrTreeUrls[networkIndex]);
const { publicKey, domain } = DnsNodeDiscovery.parseTree(enrTreeUrls);
const context: SearchContext = {
domain,
publicKey,
Expand Down Expand Up @@ -149,8 +147,9 @@ export class DnsNodeDiscovery {
subdomain: string,
context: SearchContext
): Promise<string> {
if (this._DNSTreeCache[subdomain]) {
return this._DNSTreeCache[subdomain];
const currentCache = this._DNSTreeCache[subdomain];
if (currentCache) {
return currentCache;
}

// Location is either the top level tree entry host or a subdomain of it.
Expand All @@ -161,9 +160,13 @@ export class DnsNodeDiscovery {

const response = await this.dns.resolveTXT(location);

if (!response.length)
if (!response.length) {
throw new Error("Received empty result array while fetching TXT record");
if (!response[0].length) throw new Error("Received empty TXT record");
}

if (!response[0]?.length) {
throw new Error("Received empty TXT record");
}

// Branch entries can be an array of strings of comma delimited subdomains, with
// some subdomain strings split across the array elements
Expand All @@ -172,6 +175,17 @@ export class DnsNodeDiscovery {
this._DNSTreeCache[subdomain] = result;
return result;
}

private static parseTree(enrTreeUrls: string[]): ENRTreeValues {
const networkIndex = Math.floor(Math.random() * enrTreeUrls.length);
const enrTree = enrTreeUrls[networkIndex];

if (!enrTree) {
throw Error(`Failed to read ENR tree for the network: ${networkIndex}`);
}

return ENRTree.parseTree(enrTree);
}
}

function getEntryType(entry: string): string {
Expand Down Expand Up @@ -211,5 +225,5 @@ function selectRandomPath(branches: string[], context: SearchContext): string {
index = Math.floor(Math.random() * branches.length);
} while (circularRefs[index]);

return branches[index];
return branches[index] as string;
}
17 changes: 14 additions & 3 deletions packages/dns-discovery/src/enrtree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ export class ENRTree {
// (Trailing recovery bit must be trimmed to pass `ecdsaVerify` method)
const signedComponent = root.split(" sig")[0];

if (!signedComponent) {
throw Error(`Signature component is missing, got: ${signedComponent}`);
}

const signedComponentBuffer = utf8ToBytes(signedComponent);
const signatureBuffer = fromString(rootValues.signature, "base64url").slice(
0,
Expand Down Expand Up @@ -113,11 +117,18 @@ export class ENRTree {
* either further branch entries or ENR records.
*/
static parseBranch(branch: string): string[] {
if (!branch.startsWith(this.BRANCH_PREFIX))
throw new Error(
if (!branch.startsWith(this.BRANCH_PREFIX)) {
throw Error(
`ENRTree branch entry must start with '${this.BRANCH_PREFIX}'`
);
}

const enrPart = branch.split(this.BRANCH_PREFIX)[1];

if (!enrPart) {
throw Error(`ENRTree branch does not have data`);
}

return branch.split(this.BRANCH_PREFIX)[1].split(",");
return enrPart.split(",");
}
}
2 changes: 1 addition & 1 deletion packages/enr/src/decoder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ async function fromValues(values: Uint8Array[]): Promise<ENR> {
const obj: Record<ENRKey, ENRValue> = {};
for (let i = 0; i < kvs.length; i += 2) {
try {
obj[bytesToUtf8(kvs[i])] = kvs[i + 1];
obj[bytesToUtf8(kvs[i] as Uint8Array)] = kvs[i + 1] as Uint8Array;
} catch (e) {
log.error("Failed to decode ENR key to UTF-8, skipping it", kvs[i], e);
}
Expand Down
14 changes: 6 additions & 8 deletions packages/enr/src/enr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,25 +73,23 @@ export class ENR extends RawEnr implements IEnr {

setLocationMultiaddr(multiaddr: Multiaddr): void {
const protoNames = multiaddr.protoNames();
if (
protoNames.length !== 2 &&
protoNames[1] !== "udp" &&
protoNames[1] !== "tcp"
) {
const protocol = protoNames[1];
if (protoNames.length !== 2 && protocol !== "udp" && protocol !== "tcp") {
throw new Error("Invalid multiaddr");
}

const tuples = multiaddr.tuples();
if (!tuples[0][1] || !tuples[1][1]) {
if (!tuples[0]?.[1] || !tuples[1]?.[1] || !protocol) {
throw new Error("Invalid multiaddr");
}

// IPv4
if (tuples[0][0] === 4) {
this.set("ip", tuples[0][1]);
this.set(protoNames[1], tuples[1][1]);
this.set(protocol, tuples[1][1]);
} else {
this.set("ip6", tuples[0][1]);
this.set(protoNames[1] + "6", tuples[1][1]);
this.set(protocol + "6", tuples[1][1]);
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/enr/src/raw_enr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ export class RawEnr extends Map<ENRKey, ENRValue> {
*/
get waku2(): Waku2 | undefined {
const raw = this.get("waku2");
if (raw) return decodeWaku2(raw[0]);
if (raw && raw[0]) return decodeWaku2(raw[0]);

return;
}
Expand Down
6 changes: 5 additions & 1 deletion packages/message-encryption/src/crypto/ecies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,12 @@ export async function decrypt(
(macKey) => [hash.slice(0, 16), macKey]
);

if (!encryptionKey || !macKey) {
throw Error("Failed to get parameters from the hash.");
}

if (!(await hmacSha256Verify(macKey, cipherAndIv, msgMac))) {
throw new Error("Incorrect MAC");
throw Error("Incorrect MAC");
}

return aesCtrDecrypt(iv, encryptionKey, ciphertext);
Expand Down
2 changes: 1 addition & 1 deletion packages/relay/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ export const minute = 60 * second;
/**
* RelayCodec is the libp2p identifier for the waku relay protocol
*/
export const RelayCodecs = ["/vac/waku/relay/2.0.0"];
export const RelayCodec = "/vac/waku/relay/2.0.0";

/**
* RelayGossipFactor affects how many peers we will emit gossip to at each heartbeat.
Expand Down
6 changes: 3 additions & 3 deletions packages/relay/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import {
import { pushOrInitMapSet } from "@waku/utils";
import { Logger } from "@waku/utils";

import { RelayCodecs } from "./constants.js";
import { RelayCodec } from "./constants.js";
import { messageValidator } from "./message_validator.js";
import { TopicOnlyDecoder } from "./topic_only_message.js";

Expand All @@ -54,7 +54,7 @@ class Relay implements IRelay {
public readonly pubsubTopics: Set<PubsubTopic>;
private defaultDecoder: IDecoder<IDecodedMessage>;

public static multicodec: string = RelayCodecs[0];
public static multicodec: string = RelayCodec;
public readonly gossipSub: GossipSub;

/**
Expand Down Expand Up @@ -300,7 +300,7 @@ export function wakuGossipSub(
fallbackToFloodsub: false
};
const pubsub = new GossipSub(components, init);
pubsub.multicodecs = RelayCodecs;
pubsub.multicodecs = [RelayCodec];
return pubsub;
};
}
4 changes: 2 additions & 2 deletions packages/tests/tests/store/multiple_pubsub.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@ import type { ContentTopicInfo, IMessage, LightNode } from "@waku/interfaces";
import { createLightNode, Protocols } from "@waku/sdk";
import {
contentTopicToPubsubTopic,
pubsubTopicToSingleShardInfo,
singleShardInfosToShardInfo
pubsubTopicToSingleShardInfo
} from "@waku/utils";
import { expect } from "chai";

Expand All @@ -29,6 +28,7 @@ import {
sendMessagesAutosharding,
shardInfo1,
shardInfoBothShards,
singleShardInfosToShardInfo,
startAndConnectLightNode,
totalMsgs
} from "./utils.js";
Expand Down
20 changes: 20 additions & 0 deletions packages/tests/tests/store/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,3 +148,23 @@ export const adjustDate = (baseDate: Date, adjustMs: number): Date => {
adjusted.setTime(adjusted.getTime() + adjustMs);
return adjusted;
};

export const singleShardInfosToShardInfo = (
singleShardInfos: SingleShardInfo[]
): ShardInfo => {
if (singleShardInfos.length === 0) throw new Error("Invalid shard");

const clusterIds = singleShardInfos.map((shardInfo) => shardInfo.clusterId);
if (new Set(clusterIds).size !== 1) {
throw new Error("Passed shard infos have different clusterIds");
}

const shards = singleShardInfos
.map((shardInfo) => shardInfo.shard)
.filter((shard): shard is number => shard !== undefined);

return {
clusterId: singleShardInfos[0].clusterId,
shards
};
};
3 changes: 2 additions & 1 deletion packages/tests/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
"compilerOptions": {
"outDir": "dist/",
"rootDir": "src",
"tsBuildInfoFile": "dist/.tsbuildinfo"
"tsBuildInfoFile": "dist/.tsbuildinfo",
"noUncheckedIndexedAccess": false,
},
"include": ["src", "src/run-tests.js"],
"exclude": ["src/**/*.spec.ts", "src/test_utils"]
Expand Down
4 changes: 2 additions & 2 deletions packages/utils/src/common/random_subset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ function shuffle<T>(arr: T[]): T[] {

for (let i = 0; i < arr.length; i++) {
const j = randInt();
const tmp = arr[i];
arr[i] = arr[j];
const tmp = arr[i] as T;
arr[i] = arr[j] as T;
Comment on lines +25 to +26
Copy link
Collaborator

Choose a reason for hiding this comment

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

does it not automatically infer the type? arr is of the type T[] 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

noUncheckedIndexedAccess makes so anything that is accepted by index becomes T | undefined

arr[j] = tmp;
}
return arr;
Expand Down
2 changes: 1 addition & 1 deletion packages/utils/src/common/relay_shard_codec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export const encodeRelayShard = (shardInfo: ShardInfo): Uint8Array => {
// rs format (Index List)
view.setUint8(2, shards.length);
for (let i = 0, offset = 3; i < shards.length; i++, offset += 2) {
view.setUint16(offset, shards[i]);
view.setUint16(offset, shards[i] as number);
}
}

Expand Down
Loading
Loading