Merge pull request #1598 from bugsnag/fix-network-breadcrumb-crash

Fix a crash when XMLHttpRequest URL isn't a string
bugsnag · Nov 25, 2021 · 26f58b6 · 26f58b6
2 parents d83339e + 1e8b8e0
commit 26f58b6
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 5 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,7 @@
 
 ### Fixed
 
+- (plugin-network-breadcrumbs): Fix a crash when request URL is not a string [#1598](https://github.com/bugsnag/bugsnag-js/pull/1598)
 - (in-flight): Fix Typescript definition exporting a type instead of a value [skirsten](https://github.com/skirsten) [#1587](https://github.com/bugsnag/bugsnag-js/pull/1587)
 - (plugin-electron-net-breadcrumbs): Don't leave breadcrumbs for requests to the minidumps endpoint [#1597](https://github.com/bugsnag/bugsnag-js/pull/1597)
 

diff --git a/packages/plugin-network-breadcrumbs/network-breadcrumbs.js b/packages/plugin-network-breadcrumbs/network-breadcrumbs.js
@@ -60,12 +60,16 @@ module.exports = (_ignoredUrls = [], win = window) => {
       }
 
       function handleXHRLoad () {
-        if (this[REQUEST_URL_KEY] === undefined) {
+        const url = this[REQUEST_URL_KEY]
+
+        if (url === undefined) {
           client._logger.warn('The request URL is no longer present on this XMLHttpRequest. A breadcrumb cannot be left for this request.')
           return
         }
 
-        if (includes(ignoredUrls, this[REQUEST_URL_KEY].replace(/\?.*$/, ''))) {
+        // an XMLHttpRequest's URL can be an object as long as its 'toString'
+        // returns a URL, e.g. a HTMLAnchorElement
+        if (typeof url === 'string' && includes(ignoredUrls, url.replace(/\?.*$/, ''))) {
           // don't leave a network breadcrumb from bugsnag notify calls
           return
         }
@@ -82,12 +86,14 @@ module.exports = (_ignoredUrls = [], win = window) => {
       }
 
       function handleXHRError () {
-        if (this[REQUEST_URL_KEY] === undefined) {
+        const url = this[REQUEST_URL_KEY]
+
+        if (url === undefined) {
           client._logger.warn('The request URL is no longer present on this XMLHttpRequest. A breadcrumb cannot be left for this request.')
           return
         }
 
-        if (includes(ignoredUrls, this[REQUEST_URL_KEY].replace(/\?.*$/, ''))) {
+        if (typeof url === 'string' && includes(ignoredUrls, url.replace(/\?.*$/, ''))) {
           // don't leave a network breadcrumb from bugsnag notify calls
           return
         }

diff --git a/packages/plugin-network-breadcrumbs/test/network-breadcrumbs.test.ts b/packages/plugin-network-breadcrumbs/test/network-breadcrumbs.test.ts
@@ -13,7 +13,7 @@ class XMLHttpRequest {
     this.status = null
   }
 
-  open (method: string, url: string) {
+  open (method: string, url: string | { toString: () => any }) {
   }
 
   send (fail: boolean, status: number | null = null) {
@@ -225,6 +225,61 @@ describe('plugin: network breadcrumbs', () => {
     )
   })
 
+  it('should leave a breadcrumb when the request URL is not a string', () => {
+    const window = { XMLHttpRequest } as unknown as Window & typeof globalThis
+
+    const logger = {
+      debug: jest.fn(),
+      info: jest.fn(),
+      warn: jest.fn(),
+      error: jest.fn()
+    }
+
+    p = plugin([], window)
+    const client = new Client({ apiKey: 'abcabcabcabcabcabcabc1234567890f', logger, plugins: [p] })
+
+    const request = new window.XMLHttpRequest() as unknown as XMLHttpRequest
+    request.open('GET', { toString: () => 'https://example.com' })
+    request.send(false, 200)
+
+    expect(client._breadcrumbs.length).toBe(1)
+    expect(client._breadcrumbs[0]).toEqual(expect.objectContaining({
+      type: 'request',
+      message: 'XMLHttpRequest succeeded',
+      metadata: {
+        status: 200,
+        request: 'GET https://example.com'
+      }
+    }))
+  })
+
+  it('should leave a breadcrumb when the request URL is not a string for a request that errors', () => {
+    const window = { XMLHttpRequest } as unknown as Window & typeof globalThis
+
+    const logger = {
+      debug: jest.fn(),
+      info: jest.fn(),
+      warn: jest.fn(),
+      error: jest.fn()
+    }
+
+    p = plugin([], window)
+    const client = new Client({ apiKey: 'abcabcabcabcabcabcabc1234567890f', logger, plugins: [p] })
+
+    const request = new window.XMLHttpRequest() as unknown as XMLHttpRequest
+    request.open('GET', { toString: () => 'https://example.com' })
+    request.send(true)
+
+    expect(client._breadcrumbs.length).toBe(1)
+    expect(client._breadcrumbs[0]).toEqual(expect.objectContaining({
+      type: 'request',
+      message: 'XMLHttpRequest error',
+      metadata: {
+        request: 'GET https://example.com'
+      }
+    }))
+  })
+
   it('should leave a breadcrumb when a fetch() resolves', (done) => {
     const window = { XMLHttpRequest, fetch } as unknown as Window & typeof globalThis