add sendPayloadChecksums config option and implement Bugsnag-Integrity header #2221

djskinner · 2024-10-15T14:06:18Z

Goal

Add Bugsnag-Integrity request header (where required APIs are available) and implement the sendPayloadChecksums core config option to allow opting out of this behavior.

Bugsnag-Integrity headers are set by default unless the endpoints configuration option is set, in which case they are disabled. This behavior can be overriden with the new sendPayloadChecksums config option.

Design

Bugsnag-Integrity headers are set where the environment supports it (requires native promises, subtle crypto, and a secure context)
The behaviour of sendPayloadChecksums was chosen to avoid possible CORS issues where custom endpoints are being used and they are not configured to accept the new Bugsnag-Integrity in requests.

Changeset

Add sendPayloadChecksums core config option
Set Bugsnag-Integrity in delivery-fetch, which is used in @bugsnag/web-worker.
Set Bugsnag-Integrity in delivery-xml-http-request, which is used in @bugsnag/browser.

Testing

unit tests on delivery-fetch and delivery-xml-http-request
manually tested using the web-worker example: event appears when integrity header is correct, event does not appear in the dashboard when the integrity header is wrong
integration tests for browser and web worker
e2e tests for browser and web worker

djskinner · 2024-10-15T14:07:43Z

jest/FixJSDOMEnvironment.js

+class FixJSDOMEnvironment extends JSDOMEnvironment {
+  constructor (...args) {
+    super(...args)
+
+    this.global.TextEncoder = TextEncoder
+    this.global.TextDecoder = TextDecoder
+    this.global.crypto = {
+      subtle: crypto.webcrypto.subtle
+    }
+  }
+}


More info: https://mswjs.io/docs/migrations/1.x-to-2.x/#requestresponsetextencoder-is-not-defined-jest

We could also use https://github.com/mswjs/jest-fixed-jsdom but would still need to manually add the crypto APIs

I think this is fairly simple and clear

For future reference, when we come to upgrade jest, it will need rewriting like this:

const { TextDecoder, TextEncoder } = require('node:util') const crypto = require('crypto') const JSDOMEnvironment = require('jest-environment-jsdom').default function FixJSDOMEnvironment (...args) { var _this = Reflect.construct(JSDOMEnvironment, args) _this.global.TextEncoder = TextEncoder _this.global.TextDecoder = TextDecoder Object.defineProperty(_this.global, 'crypto', { value: { subtle: crypto.webcrypto.subtle } }) return _this } FixJSDOMEnvironment.prototype = Object.create(JSDOMEnvironment.prototype) FixJSDOMEnvironment.prototype.constructor = FixJSDOMEnvironment module.exports = FixJSDOMEnvironment

This is because JSDOMEnvironment is an ES6 class but we transpile classes in our code down to es5. This is how I managed to get it to work having an es5 class extend from an es6 class

github-actions · 2024-10-15T14:09:22Z

`@bugsnag/browser` bundle size diff

	Minified	Minfied + Gzipped
Before	`45.89 kB`	`13.78 kB`
After	`46.96 kB`	`14.02 kB`
±	⚠️ `+1,073 bytes`	⚠️ `+242 bytes`

code coverage diff

<temporarily disabled>

Generated by 🚫 dangerJS against d38cc52

djskinner · 2024-10-15T14:09:24Z

packages/delivery-fetch/delivery.js

@@ -1,19 +1,37 @@
 import payload from '@bugsnag/core/lib/json-payload'

-const delivery = (client, fetch = global.fetch) => ({
+async function addIntegrityHeader (windowOrWorkerGlobalScope, requestBody, headers) {
+  if (windowOrWorkerGlobalScope.isSecureContext && windowOrWorkerGlobalScope.crypto && windowOrWorkerGlobalScope.crypto.subtle && windowOrWorkerGlobalScope.crypto.subtle.digest && typeof TextEncoder === 'function') {


see https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto and https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts

djskinner · 2024-10-15T14:09:43Z

packages/delivery-fetch/delivery.js

+    const hashBuffer = await windowOrWorkerGlobalScope.crypto.subtle.digest('SHA-1', msgUint8)
+    const hashArray = Array.from(new Uint8Array(hashBuffer))
+    const hashHex = hashArray
+      .map((b) => b.toString(16).padStart(2, '0'))
+      .join('')


See https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/digest#converting_a_digest_to_a_hex_string

djskinner · 2024-10-15T14:10:40Z

packages/web-worker/src/notifier.js

@@ -36,7 +36,7 @@ export const Bugsnag = {
    // configure a client with user supplied options
    const bugsnag = new Client(opts, schema, internalPlugins, { name, version, url })

-    bugsnag._setDelivery(delivery)
+    bugsnag._setDelivery(client => delivery(client, undefined, self))


Same way the global self is passed to plugin, see above, e.g. pluginWindowUnhandledRejection(self)

This reverts commit c06add0.

set Bugsnag-Integrity header in delivery-xml-http-request

djskinner · 2024-10-18T15:20:42Z

packages/browser/test/index.test.ts

@@ -4,38 +4,55 @@ const DONE = window.XMLHttpRequest.DONE



I reworked the mocking in this test suite to get a callback on the xhr send where I could call done. The existing mocking wasn't able to handle the awaiting of the getIntegrity promise.

djskinner · 2024-10-18T15:24:47Z

packages/browser/test/index.test.ts

+    it('includes the integrity header by default', (done) => {
+      const onSessionSend = (session: MockXHR) => {
+        expect(session.open).toHaveBeenCalledWith('POST', 'https://sessions.bugsnag.com')
+        expect(session.setRequestHeader).toHaveBeenCalledWith('Bugsnag-Integrity', expect.any(String))


I spent a lot of time here trying to mock out cuid to get a stable value but for reasons I don't understand jest.mock('@bugsnag/cuid') was just not doing anything. It does work if I update jest to v29 but that causes a load of other problems elsewhere and it was too much work to upgrade for this, hence the expect.any(String).

Note for when we upgrade jest, a stable value can be obtained with:

jest.mock('@bugsnag/cuid', () => () => 'abc123') jest.useFakeTimers() jest.setSystemTime(new Date(...))

djskinner · 2024-10-18T15:51:04Z

packages/delivery-fetch/delivery.js

+      const hashArray = Array.from(new Uint8Array(hashBuffer))
+      const hashHex = hashArray
+        .map((b) => b.toString(16).padStart(2, '0'))
+        .join('')


See https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/digest#converting_a_digest_to_a_hex_string

djskinner · 2024-11-20T15:09:00Z

packages/react-native/src/config.js

+  'featureFlags',
+  'reportUnhandledPromiseRejectionsAsHandled',
+  // sendPayloadChecksums gets set in core because its 'default value' depends on other config values and so cannot remain unset
+  'sendPayloadChecksums'


this doesn't seem ideal but I'm not sure there is a better way to get the "default value" behaviour of sendPayloadChecksums without actually setting its value when the client is created, i.e. this part:

// sendPayloadChecksums is false by default unless custom endpoints are not specified if (!opts.endpoints) { opts.sendPayloadChecksums = 'sendPayloadChecksums' in opts ? opts.sendPayloadChecksums : true }

I think it would be better if the defaultValue part of config could produce its value based on the rest of the config but that seemed way out of scope here,

djskinner · 2024-11-20T15:10:13Z

packages/core/client.js

+    // sendPayloadChecksums is false by default unless custom endpoints are not specified
+    if (!opts.endpoints) {
+      opts.sendPayloadChecksums = 'sendPayloadChecksums' in opts ? opts.sendPayloadChecksums : true
+    }


the sendPayloadChecksums is added to core so the behaviour is the same for all clients i.e. browser and web worker. It doesn't apply to react native as that uses native delivery so they would need separate support and configuration in the native notifiers (which I believe they already have)

djskinner · 2024-11-20T15:21:03Z

packages/delivery-fetch/delivery.js

@@ -1,18 +1,44 @@
 import payload from '@bugsnag/core/lib/json-payload'

-const delivery = (client, fetch = global.fetch) => ({
+function getIntegrityHeaderValue (sendPayloadChecksums, windowOrWorkerGlobalScope, requestBody, headers) {
+  if (sendPayloadChecksums && windowOrWorkerGlobalScope.isSecureContext && windowOrWorkerGlobalScope.crypto && windowOrWorkerGlobalScope.crypto.subtle && windowOrWorkerGlobalScope.crypto.subtle.digest && typeof TextEncoder === 'function') {


see https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto and https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts

gingerbenw

Do we also need to set this in the delivery-x-domain-request package? I suppose IE8 and 9 won't support an integrity header?

gingerbenw · 2024-11-21T11:12:42Z

packages/delivery-xml-http-request/test/delivery.test.ts

+    // mock XMLHttpRequest class
+    function XMLHttpRequest (this: MockXMLHttpRequest) {
+      this.method = null
+      this.url = null
+      this.data = null
+      this.headers = {}
+      this.readyState = XMLHttpRequest.UNSENT
+      requests.push(this)
+    }
+    XMLHttpRequest.UNSENT = 0
+    XMLHttpRequest.OPENED = 1
+    XMLHttpRequest.HEADERS_RECEIVED = 2
+    XMLHttpRequest.LOADING = 3
+    XMLHttpRequest.DONE = 4
+    XMLHttpRequest.prototype.open = function (method: string, url: string) {
+      this.method = method
+      this.url = url
+      this.readyState = XMLHttpRequest.OPENED
+      this.onreadystatechange()
+    }
+    XMLHttpRequest.prototype.setRequestHeader = function (key: string, val: string) {
+      this.headers[key] = val
+    }
+    XMLHttpRequest.prototype.send = function (data: string) {
+      this.data = data
+      this.readyState = XMLHttpRequest.HEADERS_RECEIVED
+      this.onreadystatechange()
+
+      setTimeout(() => {
+        this.status = 200
+        this.readyState = XMLHttpRequest.DONE
+        this.onreadystatechange()
+      }, 500)
+    }


can this code be shared between tests?

gingerbenw · 2024-11-21T11:15:32Z

test/browser/features/integrity.feature

+Feature: Bugsnag-Integrity header
+
+Scenario: Integrity headers are set when setPayloadChecksums is true
+  When I navigate to the test URL "/integrity/script/a.html"


could we name the html something closer to what the test actually does? otherwise we have to reference between the feature and the fixture to figure out what a.html does

I was following the precedent of the majority of other fixtures here. I suppose it can be sometimes difficult to come up with a suitable description in a filename format. how about sendpayloadchecksums-true.html and sendpayloadchecksums-false.html?

yeah, that sounds good

djskinner · 2024-11-21T11:23:45Z

Do we also need to set this in the delivery-x-domain-request package? I suppose IE8 and 9 won't support an integrity header?

No, XDomainRequest is only available on ie8 and ie9 neither which support the necessary APIs for calculating and setting the integrity header. even ie11 is excluded because it doesn't have full native promise support, hence the typeof Promise !== 'undefined' && Promise.toString().indexOf('[native code]') !== -1 check.

set Bugsnag-Integrity header in delivery-fetch

14dca7e

djskinner commented Oct 15, 2024

View reviewed changes

Dan Skinner added 4 commits October 15, 2024 15:23

add jest dir to docker copy

b2d4819

try fix testEnvironment path resolution

4044c31

add jest dir to docker copy

ebad38a

set jest env

af9731d

djskinner marked this pull request as ready for review October 15, 2024 15:56

Dan Skinner added 6 commits October 16, 2024 10:16

set Bugsnag-Integrity header in delivery-xml-http-request

c81a572

do not use async syntax

7177ddf

handle no promises in ie11

9de0065

handle no promises in ie11

97f8402

do not use promise finally

a6d6e67

bump jest

c06add0

djskinner marked this pull request as draft October 18, 2024 12:18

Dan Skinner and others added 4 commits October 18, 2024 14:39

Revert "bump jest"

ff5cd1b

This reverts commit c06add0.

add sendPayloadChecksums to browser

503eae3

fix types

b1e0b1f

Merge pull request #2228 from bugsnag/browser-integrity

c415a67

set Bugsnag-Integrity header in delivery-xml-http-request

djskinner changed the title ~~set Bugsnag-Integrity header in delivery-fetch~~ add sendPayloadChecksums config option and implement Bugsnag-Integrity header Oct 18, 2024

Dan Skinner added 2 commits October 18, 2024 16:17

tidy test suite

a801c57

Merge branch 'next' into web-worker-integrity

6409517

djskinner commented Oct 18, 2024

View reviewed changes

add integrity header to delivery-fetch sendSession

c8f48cc

djskinner commented Oct 18, 2024

View reviewed changes

Merge branch 'next' into web-worker-integrity

277bdde

Dan Skinner added 8 commits November 15, 2024 10:11

Merge branch 'https' into web-worker-integrity

cfd61b2

add e2e tests for integrity headers

79cac2d

Merge branch 'next' into web-worker-integrity

2db23b7

respect sendPayloadChecksums in delivery-fetch

59ec656

move sendPayloadChecksums to core

983fcb4

move sendPayloadChecksums to core

5b3788c

add web worker integration tests for sendPayloadChecksums

acda730

add e2e tests for integrity header on web workers

283b77d

djskinner commented Nov 20, 2024

View reviewed changes

Dan Skinner added 3 commits November 20, 2024 15:21

update changelog

1eec649

skip integrity tests on unsupported browsers

9d7c759

skip integrity tests on unsupported browsers

b46beae

djskinner marked this pull request as ready for review November 20, 2024 17:35

djskinner requested review from gingerbenw, tomlongridge and yousif-bugsnag November 20, 2024 17:35

skip integrity tests on unsupported browsers

190e226

gingerbenw reviewed Nov 21, 2024

View reviewed changes

Dan Skinner added 2 commits November 26, 2024 14:53

rename fixture documents

b68c890

use ternary

d38cc52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add sendPayloadChecksums config option and implement Bugsnag-Integrity header #2221

add sendPayloadChecksums config option and implement Bugsnag-Integrity header #2221

djskinner commented Oct 15, 2024 •

edited

Loading

djskinner Oct 15, 2024

djskinner Oct 18, 2024

github-actions bot commented Oct 15, 2024 •

edited

Loading

djskinner Oct 15, 2024

djskinner Oct 15, 2024

djskinner Oct 15, 2024

djskinner Oct 18, 2024

djskinner Oct 18, 2024

djskinner Oct 18, 2024 •

edited

Loading

djskinner Nov 20, 2024

djskinner Nov 20, 2024

djskinner Nov 20, 2024

gingerbenw left a comment

gingerbenw Nov 21, 2024

gingerbenw Nov 21, 2024

djskinner Nov 21, 2024

gingerbenw Nov 21, 2024

djskinner commented Nov 21, 2024

add sendPayloadChecksums config option and implement Bugsnag-Integrity header #2221

Are you sure you want to change the base?

add sendPayloadChecksums config option and implement Bugsnag-Integrity header #2221

Conversation

djskinner commented Oct 15, 2024 • edited Loading

Goal

Design

Changeset

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 15, 2024 • edited Loading

@bugsnag/browser bundle size diff

code coverage diff

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djskinner Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gingerbenw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djskinner commented Nov 21, 2024

djskinner commented Oct 15, 2024 •

edited

Loading

github-actions bot commented Oct 15, 2024 •

edited

Loading

`@bugsnag/browser` bundle size diff

djskinner Oct 18, 2024 •

edited

Loading