Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 23 commits into
base: next
Choose a base branch
from

Conversation

djskinner
Copy link
Contributor

Goal

Add Bugsnag-Integrity request header (where required APIs are available)

Design

Changeset

  • Set Bugsnag-Integrity in delivery-fetch, which is used in @bugsnag/web-worker.

Testing

  • unit tests
  • 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
  • no e2e tests as these APIs require a secure context (i.e. HTTPs) and our e2e tests don't support that yet

Comment on lines +6 to +16
class FixJSDOMEnvironment extends JSDOMEnvironment {
constructor (...args) {
super(...args)

this.global.TextEncoder = TextEncoder
this.global.TextDecoder = TextDecoder
this.global.crypto = {
subtle: crypto.webcrypto.subtle
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

github-actions bot commented Oct 15, 2024

@bugsnag/browser bundle size diff

Minified Minfied + Gzipped
Before 45.89 kB 13.78 kB
After 46.95 kB 14.03 kB
± ⚠️ +1,067 bytes ⚠️ +248 bytes

code coverage diff

<temporarily disabled>

Generated by 🚫 dangerJS against 79cac2d

@@ -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') {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 6 to 10
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('')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -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))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@djskinner djskinner marked this pull request as ready for review October 15, 2024 15:56
@djskinner djskinner marked this pull request as draft October 18, 2024 12:18
Dan Skinner and others added 4 commits October 18, 2024 14:39
@djskinner djskinner changed the title set Bugsnag-Integrity header in delivery-fetch add sendPayloadChecksums config option and implement Bugsnag-Integrity header Oct 18, 2024
@@ -4,38 +4,55 @@ const DONE = window.XMLHttpRequest.DONE

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(...))

Comment on lines +7 to +10
const hashArray = Array.from(new Uint8Array(hashBuffer))
const hashHex = hashArray
.map((b) => b.toString(16).padStart(2, '0'))
.join('')
Copy link
Contributor Author

@djskinner djskinner Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant