From fd607b10954dd04fa357c943c68c1e5c36ca81ed Mon Sep 17 00:00:00 2001 From: Mikkel Snitker Date: Mon, 4 Oct 2021 08:18:03 +0200 Subject: [PATCH] chore: propagate 'Invalid header' errors (#7020) Enable developers to handle 'Invalid header' errors instead of hiding them to make sure they can address them properly. Co-authored-by: Jan Scheffler --- src/common/Connection.ts | 32 ++++++++++++++++++++++-------- src/common/Errors.ts | 11 ++++++++++- src/common/HTTPRequest.ts | 38 ++++++++++++++---------------------- src/common/NetworkManager.ts | 5 +---- test/network.spec.ts | 24 +++++++++++++++++++++++ 5 files changed, 74 insertions(+), 36 deletions(-) diff --git a/src/common/Connection.ts b/src/common/Connection.ts index 9b3a861fe0a..f5471b7f2ee 100644 --- a/src/common/Connection.ts +++ b/src/common/Connection.ts @@ -22,6 +22,7 @@ import { Protocol } from 'devtools-protocol'; import { ProtocolMapping } from 'devtools-protocol/types/protocol-mapping.js'; import { ConnectionTransport } from './ConnectionTransport.js'; import { EventEmitter } from './EventEmitter.js'; +import { ProtocolError } from './Errors.js'; /** * @public @@ -34,7 +35,7 @@ export { ConnectionTransport, ProtocolMapping }; export interface ConnectionCallback { resolve: Function; reject: Function; - error: Error; + error: ProtocolError; method: string; } @@ -99,7 +100,12 @@ export class Connection extends EventEmitter { const params = paramArgs.length ? paramArgs[0] : undefined; const id = this._rawSend({ method, params }); return new Promise((resolve, reject) => { - this._callbacks.set(id, { resolve, reject, error: new Error(), method }); + this._callbacks.set(id, { + resolve, + reject, + error: new ProtocolError(), + method, + }); }); } @@ -206,7 +212,7 @@ export interface CDPSessionOnMessageObject { id?: number; method: string; params: Record; - error: { message: string; data: any }; + error: { message: string; data: any; code: number }; result?: any; } @@ -288,7 +294,12 @@ export class CDPSession extends EventEmitter { }); return new Promise((resolve, reject) => { - this._callbacks.set(id, { resolve, reject, error: new Error(), method }); + this._callbacks.set(id, { + resolve, + reject, + error: new ProtocolError(), + method, + }); }); } @@ -348,13 +359,13 @@ export class CDPSession extends EventEmitter { * @returns {!Error} */ function createProtocolError( - error: Error, + error: ProtocolError, method: string, - object: { error: { message: string; data: any } } + object: { error: { message: string; data: any; code: number } } ): Error { let message = `Protocol error (${method}): ${object.error.message}`; if ('data' in object.error) message += ` ${object.error.data}`; - return rewriteError(error, message); + return rewriteError(error, message, object.error.message); } /** @@ -362,7 +373,12 @@ function createProtocolError( * @param {string} message * @returns {!Error} */ -function rewriteError(error: Error, message: string): Error { +function rewriteError( + error: ProtocolError, + message: string, + originalMessage?: string +): Error { error.message = message; + error.originalMessage = originalMessage; return error; } diff --git a/src/common/Errors.ts b/src/common/Errors.ts index cbfd301ee93..5b9a09a092c 100644 --- a/src/common/Errors.ts +++ b/src/common/Errors.ts @@ -18,7 +18,7 @@ * @public */ export class CustomError extends Error { - constructor(message: string) { + constructor(message?: string) { super(message); this.name = this.constructor.name; Error.captureStackTrace(this, this.constructor); @@ -36,6 +36,15 @@ export class CustomError extends Error { * @public */ export class TimeoutError extends CustomError {} +/** + * ProtocolError is emitted whenever there is an error from the protocol. + * + * @public + */ +export class ProtocolError extends CustomError { + public code?: number; + public originalMessage: string; +} /** * @public */ diff --git a/src/common/HTTPRequest.ts b/src/common/HTTPRequest.ts index 6f9cc20a644..d4ba66333f7 100644 --- a/src/common/HTTPRequest.ts +++ b/src/common/HTTPRequest.ts @@ -19,6 +19,7 @@ import { HTTPResponse } from './HTTPResponse.js'; import { assert } from './assert.js'; import { helper, debugError } from './helper.js'; import { Protocol } from 'devtools-protocol'; +import { ProtocolError } from './Errors.js'; /** * @public @@ -229,11 +230,7 @@ export class HTTPRequest { async finalizeInterceptions(): Promise { await this._interceptActions.reduce( (promiseChain, interceptAction) => - promiseChain.then(interceptAction).catch((error) => { - // This is here so cooperative handlers that fail do not stop other handlers - // from running - debugError(error); - }), + promiseChain.then(interceptAction).catch(handleError), Promise.resolve() ); const [resolution] = this.interceptResolution(); @@ -443,12 +440,7 @@ export class HTTPRequest { postData: postDataBinaryBase64, headers: headers ? headersArray(headers) : undefined, }) - .catch((error) => { - // In certain cases, protocol will return error if the request was - // already canceled or the page was closed. We should tolerate these - // errors. - debugError(error); - }); + .catch(handleError); } /** @@ -540,12 +532,7 @@ export class HTTPRequest { responseHeaders: headersArray(responseHeaders), body: responseBody ? responseBody.toString('base64') : undefined, }) - .catch((error) => { - // In certain cases, protocol will return error if the request was - // already canceled or the page was closed. We should tolerate these - // errors. - debugError(error); - }); + .catch(handleError); } /** @@ -594,12 +581,7 @@ export class HTTPRequest { requestId: this._interceptionId, errorReason, }) - .catch((error) => { - // In certain cases, protocol will return error if the request was - // already canceled or the page was closed. We should tolerate these - // errors. - debugError(error); - }); + .catch(handleError); } } @@ -666,6 +648,16 @@ function headersArray( return result; } +async function handleError(error: ProtocolError) { + if (['Invalid header'].includes(error.originalMessage)) { + throw error; + } + // In certain cases, protocol will return error if the request was + // already canceled or the page was closed. We should tolerate these + // errors. + debugError(error); +} + // List taken from // https://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml // with extra 306 and 418 codes. diff --git a/src/common/NetworkManager.ts b/src/common/NetworkManager.ts index 73ae84a9d51..a3cfb81306a 100644 --- a/src/common/NetworkManager.ts +++ b/src/common/NetworkManager.ts @@ -388,10 +388,7 @@ export class NetworkManager extends EventEmitter { ); this._requestIdToRequest.set(event.requestId, request); this.emit(NetworkManagerEmittedEvents.Request, request); - request.finalizeInterceptions().catch((error) => { - // This should never happen, but catch just in case. - debugError(error); - }); + request.finalizeInterceptions(); } _onRequestServedFromCache( diff --git a/test/network.spec.ts b/test/network.spec.ts index 6775fdce199..9450a76be28 100644 --- a/test/network.spec.ts +++ b/test/network.spec.ts @@ -68,6 +68,30 @@ describe('network', function () { }); }); + describeFailsFirefox('Request.continue', () => { + it('should split a request header at new line characters and add the header multiple times instead', async () => { + const { page, server } = getTestState(); + + let resolve; + const errorPromise = new Promise((r) => { + resolve = r; + }); + + await page.setRequestInterception(true); + page.on('request', async (request) => { + await request + .continue({ + headers: { + 'X-Test-Header': 'a\nb', + }, + }) + .catch(resolve); + }); + page.goto(server.PREFIX + '/empty.html'); + const error = await errorPromise; + expect(error).toBeTruthy(); + }); + }); describe('Request.frame', function () { it('should work for main frame navigation request', async () => { const { page, server } = getTestState();