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

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open
31 changes: 20 additions & 11 deletions karma.conf.cjs
Original file line number Diff line number Diff line change
@@ -1,49 +1,58 @@
const webpack = require("webpack");
const playwright = require('playwright');
const playwright = require("playwright");

process.env.CHROME_BIN = playwright.chromium.executablePath();
process.env.FIREFOX_BIN = playwright.firefox.executablePath();

const path = require("path");
const tsConfigFile = path.resolve(__dirname, "./tsconfig.dev.json");

module.exports = function (config) {
config.set({
frameworks: ["webpack", "mocha"],
files: ["src/**/!(node).spec.ts"],
preprocessors: {
"src/**/!(node).spec.ts": ["webpack"]
"src/**/!(node).spec.ts": ["webpack"],
},
envPreprocessor: ["CI"],
reporters: ["progress"],
browsers: ["ChromeHeadless", "FirefoxHeadless"],
singleRun: true,
client: {
mocha: {
timeout: 6000 // Default is 2s
}
timeout: 6000, // Default is 2s
},
},
webpack: {
mode: "development",
module: {
rules: [{ test: /\.([cm]?ts|tsx)$/, loader: "ts-loader" }]
rules: [
{
test: /\.([cm]?ts|tsx)$/,
loader: "ts-loader",
options: { configFile: tsConfigFile },
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change makes tests running in with tsconfig.dev.json which makes sense as we do have a lower bar for linting tests.

@waku-org/js-waku-developers

},
],
},
plugins: [
new webpack.DefinePlugin({
"process.env.CI": process.env.CI || false,
"process.env.DISPLAY": "Browser",
}),
new webpack.ProvidePlugin({
process: "process/browser.js"
})
process: "process/browser.js",
}),
],
resolve: {
extensions: [".ts", ".tsx", ".js"],
extensionAlias: {
".js": [".js", ".ts"],
".cjs": [".cjs", ".cts"],
".mjs": [".mjs", ".mts"]
}
".mjs": [".mjs", ".mts"],
},
},
stats: { warnings: false },
devtool: "inline-source-map"
}
devtool: "inline-source-map",
},
});
};
6 changes: 3 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@
"check": "run-s check:*",
"check:workspaces": "npm run check --workspaces --if-present",
"check:ws": "[ $(ls -1 ./packages|wc -l) -eq $(cat package.json | jq '.workspaces | length') ] || exit 1 # check no packages left behind",
"test": "NODE_ENV=test npm run test --workspaces --if-present",
"test:browser": "NODE_ENV=test npm run test:browser --workspaces --if-present",
"test:node": "NODE_ENV=test npm run test:node --workspaces --if-present",
"test": "npm run test --workspaces --if-present",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is not needed as each of internal commands should be worrying about supplying ENV flags
fyi @danisharora099

"test:browser": "npm run test:browser --workspaces --if-present",
"test:node": "npm run test:node --workspaces --if-present",
"proto": "npm run proto --workspaces --if-present",
"deploy": "node ci/deploy.js",
"doc": "run-s doc:*",
Expand Down
6 changes: 3 additions & 3 deletions packages/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,12 @@
"fix": "run-s fix:*",
"fix:lint": "eslint src *.js --fix",
"check": "run-s check:*",
"check:tsc": "tsc -p tsconfig.dev.json",
"check:tsc": "tsc -p tsconfig.json",
"check:lint": "eslint src *.js",
"check:spelling": "cspell \"{README.md,src/**/*.ts}\"",
"test": "NODE_ENV=test run-s test:*",
"test": "NODE_ENV=test TS_NODE_PROJECT=./tsconfig.dev.json run-s test:*",
"test:node": "NODE_ENV=test TS_NODE_PROJECT=./tsconfig.dev.json mocha",
"test:browser": "NODE_ENV=test karma start karma.conf.cjs",
"test:browser": "NODE_ENV=test TS_NODE_PROJECT=./tsconfig.dev.json karma start karma.conf.cjs",
"watch:build": "tsc -p tsconfig.json -w",
"watch:test": "mocha --watch",
"prepublish": "npm run build",
Expand Down
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?.[0]) {
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?.[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?.[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 @@ -89,7 +89,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 @@ -100,7 +100,7 @@
);
return;
} catch (e) {
if ((e as any).code === "ERR_CONNECTION_BEING_CLOSED")

Check warning on line 103 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
log.error(
`Connection with the peer was closed and possibly because it's on a different shard. Error: ${e}`
);
Expand Down
6 changes: 3 additions & 3 deletions packages/dns-discovery/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,12 @@
"check": "run-s check:*",
"check:lint": "eslint src --ext .ts",
"check:spelling": "cspell \"{README.md,src/**/*.ts}\"",
"check:tsc": "tsc -p tsconfig.dev.json",
"check:tsc": "tsc -p tsconfig.json",
"prepublish": "npm run build",
"reset-hard": "git clean -dfx -e .idea && git reset --hard && npm i && npm run build",
"test": "NODE_ENV=test run-s test:*",
"test": "NODE_ENV=test TS_NODE_PROJECT=./tsconfig.dev.json run-s test:*",
"test:node": "NODE_ENV=test TS_NODE_PROJECT=./tsconfig.dev.json mocha",
"test:browser": "NODE_ENV=test karma start karma.conf.cjs"
"test:browser": "NODE_ENV=test TS_NODE_PROJECT=./tsconfig.dev.json karma start karma.conf.cjs"
},
"engines": {
"node": ">=18"
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(",");
}
}
6 changes: 3 additions & 3 deletions packages/enr/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,10 @@
"check": "run-s check:*",
"check:lint": "eslint src --ext .ts",
"check:spelling": "cspell \"{README.md,src/**/*.ts}\"",
"check:tsc": "tsc -p tsconfig.dev.json",
"test": "NODE_ENV=test run-s test:*",
"check:tsc": "tsc -p tsconfig.json",
"test": "NODE_ENV=test TS_NODE_PROJECT=./tsconfig.dev.json run-s test:*",
"test:node": "NODE_ENV=test TS_NODE_PROJECT=./tsconfig.dev.json mocha",
"test:browser": "NODE_ENV=test karma start karma.conf.cjs",
"test:browser": "NODE_ENV=test TS_NODE_PROJECT=./tsconfig.dev.json karma start karma.conf.cjs",
"prepublish": "npm run build",
"reset-hard": "git clean -dfx -e .idea && git reset --hard && npm i && npm run build"
},
Expand Down
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) return decodeWaku2(raw[0] as number);

return;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/interfaces/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
"check": "run-s check:*",
"check:lint": "eslint src",
"check:spelling": "cspell \"{README.md,src/**/*.ts}\"",
"check:tsc": "tsc -p tsconfig.dev.json",
"check:tsc": "tsc -p tsconfig.json",
"prepublish": "npm run build",
"reset-hard": "git clean -dfx -e .idea && git reset --hard && npm i && npm run build"
},
Expand Down
6 changes: 3 additions & 3 deletions packages/message-encryption/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,10 @@
"check": "run-s check:*",
"check:lint": "eslint src *.js",
"check:spelling": "cspell \"{README.md,src/**/*.ts}\"",
"check:tsc": "tsc -p tsconfig.dev.json",
"test": "NODE_ENV=test run-s test:*",
"check:tsc": "tsc -p tsconfig.json",
"test": "NODE_ENV=test TS_NODE_PROJECT=./tsconfig.dev.json run-s test:*",
"test:node": "NODE_ENV=test TS_NODE_PROJECT=./tsconfig.dev.json mocha",
"test:browser": "NODE_ENV=test karma start karma.conf.cjs",
"test:browser": "NODE_ENV=test TS_NODE_PROJECT=./tsconfig.dev.json karma start karma.conf.cjs",
"prepublish": "npm run build",
"reset-hard": "git clean -dfx -e .idea && git reset --hard && npm i && npm run build"
},
Expand Down
Loading
Loading