From 54fe070f85397eb4d7c300d6a1ece3971fe0ba62 Mon Sep 17 00:00:00 2001 From: sea-snake <104725312+sea-snake@users.noreply.github.com> Date: Mon, 9 Dec 2024 20:40:55 +0100 Subject: [PATCH] fix: Make pollForResponse typesafe to avoid exceptions from unknown requests (#958) - Make pollForResponse typesafe - Change signed request typing from any to unknown - Remove await where requestId method is used --- docs/CHANGELOG.md | 3 +- packages/agent/src/actor.test.ts | 2 +- packages/agent/src/agent/api.ts | 9 +-- packages/agent/src/agent/http/http.test.ts | 12 ++-- packages/agent/src/agent/http/index.ts | 16 +++--- packages/agent/src/auth.ts | 2 +- packages/agent/src/polling/index.ts | 58 ++++++++++++++++++-- packages/agent/src/request_id.test.ts | 8 +-- packages/agent/src/request_id.ts | 3 +- packages/identity/src/identity/delegation.ts | 2 +- 10 files changed, 78 insertions(+), 37 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 161251413..bb4341b37 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -8,7 +8,8 @@ ### Changed -- chore: Removes warning that users found unhelpful, when a message originates from other sources than the identity provider in `AuthClient` during authentication. +- chore: Removes warning that users found unhelpful, when a message originates from other sources than the identity provider in `AuthClient` during authentication. +- fix: Make pollForResponse typesafe to avoid exceptions from unknown requests ## [2.1.3] - 2024-10-23 diff --git a/packages/agent/src/actor.test.ts b/packages/agent/src/actor.test.ts index 8a87b5249..5f12d9465 100644 --- a/packages/agent/src/actor.test.ts +++ b/packages/agent/src/actor.test.ts @@ -132,7 +132,7 @@ describe('makeActor', () => { }, } as UnSigned; - const expectedCallRequestId = await requestIdOf(expectedCallRequest.content); + const expectedCallRequestId = requestIdOf(expectedCallRequest.content); const httpAgent = new HttpAgent({ fetch: mockFetch }); diff --git a/packages/agent/src/agent/api.ts b/packages/agent/src/agent/api.ts index b521116bb..6d6cd9061 100644 --- a/packages/agent/src/agent/api.ts +++ b/packages/agent/src/agent/api.ts @@ -160,11 +160,7 @@ export interface Agent { * `readState` uses this internally. * Useful to avoid signing the same request multiple times. */ - createReadStateRequest?( - options: ReadStateOptions, - identity?: Identity, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - ): Promise; + createReadStateRequest?(options: ReadStateOptions, identity?: Identity): Promise; /** * Send a read state query to the replica. This includes a list of paths to return, @@ -179,8 +175,7 @@ export interface Agent { effectiveCanisterId: Principal | string, options: ReadStateOptions, identity?: Identity, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - request?: any, + request?: unknown, ): Promise; call(canisterId: Principal | string, fields: CallOptions): Promise; diff --git a/packages/agent/src/agent/http/http.test.ts b/packages/agent/src/agent/http/http.test.ts index c388531d4..83b862302 100644 --- a/packages/agent/src/agent/http/http.test.ts +++ b/packages/agent/src/agent/http/http.test.ts @@ -99,13 +99,13 @@ test('call', async () => { ingress_expiry: new Expiry(300000), }; - const mockPartialsRequestId = await requestIdOf(mockPartialRequest); + const mockPartialsRequestId = requestIdOf(mockPartialRequest); const expectedRequest = { content: mockPartialRequest, }; - const expectedRequestId = await requestIdOf(expectedRequest.content); + const expectedRequestId = requestIdOf(expectedRequest.content); expect(expectedRequestId).toEqual(mockPartialsRequestId); const { calls } = mockFetch.mock; @@ -150,7 +150,7 @@ test('queries with the same content should have the same signature', async () => const methodName = 'greet'; const arg = new Uint8Array([]); - const requestId = await requestIdOf({ + const requestId = requestIdOf({ request_type: SubmitRequestType.Call, nonce, canister_id: Principal.fromText(canisterIdent).toString(), @@ -213,7 +213,7 @@ test('readState should not call transformers if request is passed', async () => const methodName = 'greet'; const arg = new Uint8Array([]); - const requestId = await requestIdOf({ + const requestId = requestIdOf({ request_type: SubmitRequestType.Call, nonce, canister_id: Principal.fromText(canisterIdent).toString(), @@ -314,13 +314,13 @@ test('use anonymous principal if unspecified', async () => { ingress_expiry: new Expiry(300000), }; - const mockPartialsRequestId = await requestIdOf(mockPartialRequest); + const mockPartialsRequestId = requestIdOf(mockPartialRequest); const expectedRequest: Envelope = { content: mockPartialRequest, }; - const expectedRequestId = await requestIdOf(expectedRequest.content); + const expectedRequestId = requestIdOf(expectedRequest.content); expect(expectedRequestId).toEqual(mockPartialsRequestId); const { calls } = mockFetch.mock; diff --git a/packages/agent/src/agent/http/index.ts b/packages/agent/src/agent/http/index.ts index c8f5b6572..5deee1b90 100644 --- a/packages/agent/src/agent/http/index.ts +++ b/packages/agent/src/agent/http/index.ts @@ -437,7 +437,7 @@ export class HttpAgent implements Agent { ): Promise { // TODO - restore this value const callSync = options.callSync ?? true; - const id = await(identity !== undefined ? await identity : await this.#identity); + const id = await (identity !== undefined ? await identity : await this.#identity); if (!id) { throw new IdentityInvalidError( "This identity has expired due this application's security policy. Please refresh your authentication.", @@ -468,8 +468,7 @@ export class HttpAgent implements Agent { ingress_expiry, }; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - let transformedRequest: any = (await this._transform({ + let transformedRequest = (await this._transform({ request: { body: null, method: 'POST', @@ -493,7 +492,7 @@ export class HttpAgent implements Agent { } // Apply transform for identity. - transformedRequest = await id.transformRequest(transformedRequest); + transformedRequest = (await id.transformRequest(transformedRequest)) as HttpAgentSubmitRequest; const body = cbor.encode(transformedRequest.body); const backoff = this.#backoffStrategy(); @@ -528,9 +527,9 @@ export class HttpAgent implements Agent { backoff, tries: 0, }); + const requestId = requestIdOf(submit); - const [response, requestId] = await Promise.all([request, requestIdOf(submit)]); - + const response = await request; const responseBuffer = await response.arrayBuffer(); const responseBody = ( response.status === 200 && responseBuffer.byteLength > 0 @@ -780,7 +779,7 @@ export class HttpAgent implements Agent { this.log.print(`ecid ${ecid.toString()}`); this.log.print(`canisterId ${canisterId.toString()}`); const makeQuery = async () => { - const id = await(identity !== undefined ? identity : this.#identity); + const id = await (identity !== undefined ? identity : this.#identity); if (!id) { throw new IdentityInvalidError( "This identity has expired due this application's security policy. Please refresh your authentication.", @@ -799,7 +798,7 @@ export class HttpAgent implements Agent { ingress_expiry: new Expiry(this.#maxIngressExpiryInMinutes * MINUTE_TO_MSECS), }; - const requestId = await requestIdOf(request); + const requestId = requestIdOf(request); // eslint-disable-next-line @typescript-eslint/no-explicit-any let transformedRequest: HttpAgentRequest = await this._transform({ @@ -1176,4 +1175,3 @@ export class HttpAgent implements Agent { return p; } } - diff --git a/packages/agent/src/auth.ts b/packages/agent/src/auth.ts index 9c4c57d06..47b58a551 100644 --- a/packages/agent/src/auth.ts +++ b/packages/agent/src/auth.ts @@ -88,7 +88,7 @@ export abstract class SignIdentity implements Identity { */ public async transformRequest(request: HttpAgentRequest): Promise { const { body, ...fields } = request; - const requestId = await requestIdOf(body); + const requestId = requestIdOf(body); return { ...fields, body: { diff --git a/packages/agent/src/polling/index.ts b/packages/agent/src/polling/index.ts index 1518992ea..3e8bf344a 100644 --- a/packages/agent/src/polling/index.ts +++ b/packages/agent/src/polling/index.ts @@ -7,6 +7,7 @@ import { toHex } from '../utils/buffer'; export * as strategy from './strategy'; import { defaultStrategy } from './strategy'; import { DEFAULT_INGRESS_EXPIRY_DELTA_IN_MSECS } from '../constants'; +import { ReadRequestType, ReadStateRequest } from '../agent/http/types'; export { defaultStrategy } from './strategy'; export type PollStrategy = ( canisterId: Principal, @@ -15,6 +16,52 @@ export type PollStrategy = ( ) => Promise; export type PollStrategyFactory = () => PollStrategy; +interface SignedReadStateRequestWithExpiry { + body: { + content: Pick; + }; +} + +/** + * Check if an object has a property + * @param value the object that might have the property + * @param property the key of property we're looking for + */ +function hasProperty( + value: O, + property: P, +): value is O & Record { + return Object.prototype.hasOwnProperty.call(value, property); +} + +/** + * Check if value is a signed read state request with expiry + * @param value to check + */ +function isSignedReadStateRequestWithExpiry( + value: unknown, +): value is SignedReadStateRequestWithExpiry { + return ( + value !== null && + typeof value === 'object' && + hasProperty(value, 'body') && + value.body !== null && + typeof value.body === 'object' && + hasProperty(value.body, 'content') && + value.body.content !== null && + typeof value.body.content === 'object' && + hasProperty(value.body.content, 'request_type') && + value.body.content.request_type === ReadRequestType.ReadState && + hasProperty(value.body.content, 'ingress_expiry') && + typeof value.body.content.ingress_expiry === 'object' && + value.body.content.ingress_expiry !== null && + hasProperty(value.body.content.ingress_expiry, 'toCBOR') && + typeof value.body.content.ingress_expiry.toCBOR === 'function' && + hasProperty(value.body.content.ingress_expiry, 'toHash') && + typeof value.body.content.ingress_expiry.toHash === 'function' + ); +} + /** * Polls the IC to check the status of the given request then * returns the response bytes once the request has been processed. @@ -22,7 +69,7 @@ export type PollStrategyFactory = () => PollStrategy; * @param canisterId The effective canister ID. * @param requestId The Request ID to poll status for. * @param strategy A polling strategy. - * @param request Request for the readState call. + * @param request Request for the repeated readState call. * @param blsVerify - optional replacement function that verifies the BLS signature of a certificate. */ export async function pollForResponse( @@ -30,8 +77,7 @@ export async function pollForResponse( canisterId: Principal, requestId: RequestId, strategy: PollStrategy = defaultStrategy(), - // eslint-disable-next-line - request?: any, + request?: unknown, blsVerify?: CreateCertificateOptions['blsVerify'], ): Promise<{ certificate: Certificate; @@ -40,8 +86,10 @@ export async function pollForResponse( const path = [new TextEncoder().encode('request_status'), requestId]; const currentRequest = request ?? (await agent.createReadStateRequest?.({ paths: [path] })); - // Use a fresh expiry for the readState call. - currentRequest.body.content.ingress_expiry = new Expiry(DEFAULT_INGRESS_EXPIRY_DELTA_IN_MSECS); + // Use a fresh expiry for the repeated readState call + if (request && isSignedReadStateRequestWithExpiry(currentRequest)) { + currentRequest.body.content.ingress_expiry = new Expiry(DEFAULT_INGRESS_EXPIRY_DELTA_IN_MSECS); + } const state = await agent.readState(canisterId, { paths: [path] }, undefined, currentRequest); if (agent.rootKey == null) throw new Error('Agent root key not initialized before polling'); diff --git a/packages/agent/src/request_id.test.ts b/packages/agent/src/request_id.test.ts index 4fcdc9661..deaa00905 100644 --- a/packages/agent/src/request_id.test.ts +++ b/packages/agent/src/request_id.test.ts @@ -70,7 +70,7 @@ test('requestIdOf', async () => { arg: new Uint8Array([68, 73, 68, 76, 0, 253, 42]), }; - const requestId = await requestIdOf(request); + const requestId = requestIdOf(request); expect(toHex(requestId)).toEqual( '8781291c347db32a9d8c10eb62b710fce5a93be676474c42babc74c51858f94b', @@ -90,7 +90,7 @@ test.skip('requestIdOf for sender_delegation signature', async () => { new Uint8Array([0, 0, 0, 0, 0, 32, 0, 43, 1, 1]), ], }; - const delegation1ActualHashBytes = await requestIdOf(delegation1); + const delegation1ActualHashBytes = requestIdOf(delegation1); expect(toHex(delegation1ActualHashBytes)).toEqual(expectedHashBytes); // Note: this uses `bigint` and blobs, which the rest of this lib uses too. @@ -101,7 +101,7 @@ test.skip('requestIdOf for sender_delegation signature', async () => { targets: delegation1.targets.map(t => t), expiration: BigInt(delegation1.expiration.toString()), }; - const delegation2ActualHashBytes = await requestIdOf(delegation2); + const delegation2ActualHashBytes = requestIdOf(delegation2); expect(toHex(delegation2ActualHashBytes)).toEqual(toHex(delegation1ActualHashBytes)); // This one uses Principals as targets @@ -109,7 +109,7 @@ test.skip('requestIdOf for sender_delegation signature', async () => { ...delegation1, targets: delegation1.targets.map(t => Principal.fromText(t.toString())), }; - const delegation3ActualHashBytes = await requestIdOf(delegation3); + const delegation3ActualHashBytes = requestIdOf(delegation3); expect(toHex(delegation3ActualHashBytes)).toEqual(toHex(delegation1ActualHashBytes)); }); diff --git a/packages/agent/src/request_id.ts b/packages/agent/src/request_id.ts index d73f263bf..9c56d5ac3 100644 --- a/packages/agent/src/request_id.ts +++ b/packages/agent/src/request_id.ts @@ -74,8 +74,7 @@ const hashString = (value: string): ArrayBuffer => { * https://sdk.dfinity.org/docs/interface-spec/index.html#hash-of-map * @param request - ic-ref request to hash into RequestId */ -// eslint-disable-next-line @typescript-eslint/no-explicit-any -export function requestIdOf(request: Record): RequestId { +export function requestIdOf(request: Record): RequestId { return hashOfMap(request) as RequestId; } diff --git a/packages/identity/src/identity/delegation.ts b/packages/identity/src/identity/delegation.ts index 99d296e37..72f875db5 100644 --- a/packages/identity/src/identity/delegation.ts +++ b/packages/identity/src/identity/delegation.ts @@ -110,7 +110,7 @@ async function _createSingleDelegation( // a user gesture if you await an async call thats not fetch, xhr, or setTimeout. const challenge = new Uint8Array([ ...domainSeparator, - ...new Uint8Array(requestIdOf(delegation)), + ...new Uint8Array(requestIdOf({ ...delegation })), ]); const signature = await from.sign(challenge);