fix: don't parse potentially invalid urls in event handlers (#5090)

src/cli/traceViewer/snapshotRouter.ts

@@ -18,6 +18,7 @@ import * as fs from 'fs';
 import * as path from 'path';
 import * as util from 'util';
 import type { Route } from '../../..';
+import { parsedURL } from '../../client/clientHelper';
 import type { FrameSnapshot, NetworkResourceTraceEvent, PageSnapshot } from '../../trace/traceTypes';
 import { ContextEntry } from './traceModel';
 
@@ -112,11 +113,9 @@ export class SnapshotRouter {
 }
 
 function removeHash(url: string) {
-  try {
+  const u = parsedURL(url);
-    const u = new URL(url);
+  if (!u)
-    u.hash = '';
-    return u.toString();
-  } catch (e) {
     return url;
-  }
+  u.hash = '';
+  return u.toString();
 }

src/cli/traceViewer/traceViewer.ts

@@ -132,8 +132,8 @@ class TraceViewer {
         this._document.snapshotRouter.route(route);
         return;
       }
-      const url = new URL(request.url());
       try {
+        const url = new URL(request.url());
         if (this._document && request.url().includes('action-preview')) {
           const fullPath = url.pathname.substring('/action-preview/'.length);
           const actionId = fullPath.substring(0, fullPath.indexOf('.png'));

src/client/clientHelper.ts

@@ -58,6 +58,14 @@ export async function evaluationScript(fun: Function | string | { path?: string,
   throw new Error('Either path or content property must be present');
 }
 
+export function parsedURL(url: string): URL | null {
+  try {
+    return new URL(url);
+  } catch (e) {
+    return null;
+  }
+}
+
 export function urlMatches(urlString: string, match: types.URLMatch | undefined): boolean {
   if (match === undefined || match === '')
     return true;
@@ -67,10 +75,11 @@ export function urlMatches(urlString: string, match: types.URLMatch | undefined)
     return match.test(urlString);
   if (typeof match === 'string' && match === urlString)
     return true;
-  const url = new URL(urlString);
+  const url = parsedURL(urlString);
+  if (!url)
+    return false;
   if (typeof match === 'string')
     return url.pathname === match;
-
   if (typeof match !== 'function')
     throw new Error('url parameter should be string, RegExp or function');
   return match(url);

src/server/network.ts

@@ -61,12 +61,18 @@ export function rewriteCookies(cookies: types.SetNetworkCookieParam[]): types.Se
   });
 }
 
-function stripFragmentFromUrl(url: string): string {
+export function parsedURL(url: string): URL | null {
-  if (!url.indexOf('#'))
+  try {
+    return new URL(url);
+  } catch (e) {
+    return null;
+  }
+}
+
+export function stripFragmentFromUrl(url: string): string {
+  if (!url.includes('#'))
     return url;
-  const parsed = new URL(url);
+  return url.substring(0, url.indexOf('#'));
-  parsed.hash = '';
-  return parsed.href;
 }
 
 export class Request {

src/server/page.ts

@@ -481,7 +481,9 @@ export class Page extends EventEmitter {
     const url = frame.url();
     if (!url.startsWith('http'))
       return;
-    this._browserContext.addVisitedOrigin(new URL(url).origin);
+    const purl = network.parsedURL(url);
+    if (purl)
+      this._browserContext.addVisitedOrigin(purl.origin);
   }
 
   allBindings() {

src/trace/harTracer.ts

@@ -134,7 +134,9 @@ class HarContextTracer {
 
   private _onRequest(page: Page, request: network.Request) {
     const pageEntry = this._pageEntries.get(page)!;
-    const url = new URL(request.url());
+    const url = network.parsedURL(request.url());
+    if (!url)
+      return;
 
     const harEntry: har.Entry = {
       pageref: pageEntry.id,

src/trace/snapshotter.ts

@@ -18,6 +18,7 @@ import { BrowserContext } from '../server/browserContext';
 import { Page } from '../server/page';
 import * as network from '../server/network';
 import { helper, RegisteredListener } from '../server/helper';
+import { stripFragmentFromUrl } from '../server/network';
 import { Progress, runAbortableTask } from '../server/progress';
 import { debugLogger } from '../utils/debugLogger';
 import { Frame } from '../server/frames';
@@ -115,7 +116,7 @@ export class Snapshotter {
         return frameResult;
       const frameSnapshot = {
         frameId: frame._id,
-        url: removeHash(frame.url()),
+        url: stripFragmentFromUrl(frame.url()),
         html: '<body>Snapshot is not available</body>',
         resourceOverrides: [],
       };
@@ -190,7 +191,7 @@ export class Snapshotter {
 
     const snapshot: FrameSnapshot = {
       frameId: frame._id,
-      url: removeHash(frame.url()),
+      url: stripFragmentFromUrl(frame.url()),
       html: data.html,
       resourceOverrides: [],
     };
@@ -216,16 +217,6 @@ export class Snapshotter {
   }
 }
 
-function removeHash(url: string) {
-  try {
-    const u = new URL(url);
-    u.hash = '';
-    return u.toString();
-  } catch (e) {
-    return url;
-  }
-}
-
 type FrameSnapshotAndMapping = {
   snapshot: FrameSnapshot,
   mapping: Map<Frame, string>,

test/page-goto.spec.ts

@@ -494,3 +494,8 @@ it('should report raw buffer for main resource', (test, { browserName, platform
   const body = await response.body();
   expect(body.toString()).toBe('Ü (lowercase ü)');
 });
+
+it('should not throw unhandled rejections on invalid url', async ({page, server}) => {
+  const e = await page.goto('https://www.youtube Panel Title.com/').catch(e => e);
+  expect(e.toString()).toContain('Panel Title');
+});