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

Open
wants to merge 33 commits into
base: next
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
14dca7e
set Bugsnag-Integrity header in delivery-fetch
Oct 15, 2024
b2d4819
add jest dir to docker copy
Oct 15, 2024
4044c31
try fix testEnvironment path resolution
Oct 15, 2024
ebad38a
add jest dir to docker copy
Oct 15, 2024
af9731d
set jest env
Oct 15, 2024
c81a572
set Bugsnag-Integrity header in delivery-xml-http-request
Oct 16, 2024
7177ddf
do not use async syntax
Oct 16, 2024
9de0065
handle no promises in ie11
Oct 17, 2024
97f8402
handle no promises in ie11
Oct 17, 2024
a6d6e67
do not use promise finally
Oct 17, 2024
c06add0
bump jest
Oct 17, 2024
ff5cd1b
Revert "bump jest"
Oct 18, 2024
503eae3
add sendPayloadChecksums to browser
Oct 18, 2024
b1e0b1f
fix types
Oct 18, 2024
c415a67
Merge pull request #2228 from bugsnag/browser-integrity
djskinner Oct 18, 2024
a801c57
tidy test suite
Oct 18, 2024
6409517
Merge branch 'next' into web-worker-integrity
Oct 18, 2024
c8f48cc
add integrity header to delivery-fetch sendSession
Oct 18, 2024
277bdde
Merge branch 'next' into web-worker-integrity
Nov 13, 2024
cfd61b2
Merge branch 'https' into web-worker-integrity
Nov 15, 2024
79cac2d
add e2e tests for integrity headers
Nov 15, 2024
2db23b7
Merge branch 'next' into web-worker-integrity
Nov 20, 2024
59ec656
respect sendPayloadChecksums in delivery-fetch
Nov 20, 2024
983fcb4
move sendPayloadChecksums to core
Nov 20, 2024
5b3788c
move sendPayloadChecksums to core
Nov 20, 2024
acda730
add web worker integration tests for sendPayloadChecksums
Nov 20, 2024
283b77d
add e2e tests for integrity header on web workers
Nov 20, 2024
1eec649
update changelog
Nov 20, 2024
9d7c759
skip integrity tests on unsupported browsers
Nov 20, 2024
b46beae
skip integrity tests on unsupported browsers
Nov 20, 2024
190e226
skip integrity tests on unsupported browsers
Nov 20, 2024
b68c890
rename fixture documents
Nov 26, 2024
d38cc52
use ternary
Nov 26, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ module.exports = {
],
projects: [
project('core', ['core']),
project('web workers', ['web-worker']),
project('web workers', ['web-worker'], {
testEnvironment: './jest/FixJSDOMEnvironment.js'
}),
project('shared plugins', ['plugin-app-duration', 'plugin-stackframe-path-normaliser']),
project('browser', [
'browser',
Expand Down
18 changes: 18 additions & 0 deletions jest/FixJSDOMEnvironment.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
const { TextDecoder, TextEncoder } = require('node:util')
const crypto = require('crypto')

const JSDOMEnvironment = require('jest-environment-jsdom')

class FixJSDOMEnvironment extends JSDOMEnvironment {
constructor (...args) {
super(...args)

this.global.TextEncoder = TextEncoder
this.global.TextDecoder = TextDecoder
this.global.crypto = {
subtle: crypto.webcrypto.subtle
}
}
}
Comment on lines +6 to +16
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


module.exports = FixJSDOMEnvironment
40 changes: 29 additions & 11 deletions packages/delivery-fetch/delivery.js
Original file line number Diff line number Diff line change
@@ -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.

const msgUint8 = new TextEncoder().encode(requestBody)
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.


headers['Bugsnag-Integrity'] = 'sha1 ' + hashHex
}
}

const delivery = (client, fetch = global.fetch, windowOrWorkerGlobalScope = window) => ({
sendEvent: (event, cb = () => {}) => {
const url = client._config.endpoints.notify

fetch(url, {
method: 'POST',
headers: {
'Content-Type': 'application/json',
'Bugsnag-Api-Key': event.apiKey || client._config.apiKey,
'Bugsnag-Payload-Version': '4',
'Bugsnag-Sent-At': (new Date()).toISOString()
},
body: payload.event(event, client._config.redactedKeys)
}).then(() => {
const body = payload.event(event, client._config.redactedKeys)
const headers = {
'Content-Type': 'application/json',
'Bugsnag-Api-Key': event.apiKey || client._config.apiKey,
'Bugsnag-Payload-Version': '4',
'Bugsnag-Sent-At': (new Date()).toISOString()
}

addIntegrityHeader(windowOrWorkerGlobalScope, body, headers).then(() =>
fetch(url, {
method: 'POST',
headers,
body
})
).then(() => {
cb(null)
}).catch(err => {
client._logger.error(err)
Expand Down
29 changes: 27 additions & 2 deletions packages/delivery-fetch/test/delivery.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ const globalAny: any = global

describe('delivery:fetch', () => {
it('sends events successfully', done => {
window.isSecureContext = true

globalAny.fetch = jest.fn(() => Promise.resolve({
json: () => Promise.resolve()
}))
Expand All @@ -18,7 +20,7 @@ describe('delivery:fetch', () => {

const payload = { sample: 'payload' } as unknown as EventDeliveryPayload

delivery({ logger: {}, _config: config } as unknown as Client).sendEvent(payload, (err) => {
delivery({ logger: { }, _config: config } as unknown as Client).sendEvent(payload, (err) => {
expect(err).toBeNull()
expect(globalAny.fetch).toHaveBeenCalled()
expect(globalAny.fetch).toHaveBeenCalledWith('/echo/', expect.objectContaining({
Expand All @@ -28,11 +30,34 @@ describe('delivery:fetch', () => {
'Bugsnag-Api-Key': 'aaaaaaaa',
'Bugsnag-Payload-Version': '4',
'Bugsnag-Sent-At': expect.stringMatching(/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$/),
'Content-Type': 'application/json'
'Content-Type': 'application/json',
'Bugsnag-Integrity': 'sha1 14faf2461b0519f9d9d62cfb8d79483fcc8f825c'
})
}))
done()
})

window.isSecureContext = false
})

it('omits the bugsnag integrity header when not in a secure context', done => {
globalAny.fetch = jest.fn(() => Promise.resolve({
json: () => Promise.resolve()
}))

const config = {
apiKey: 'aaaaaaaa',
endpoints: { notify: '/echo/' },
redactedKeys: []
}

const payload = { sample: 'payload' } as unknown as EventDeliveryPayload

delivery({ logger: { }, _config: config } as unknown as Client).sendEvent(payload, (err) => {
expect(err).toBeNull()
expect(globalAny.fetch.mock.calls[0][1].headers['Bugsnag-Integrity']).toBeUndefined()
done()
})
})

it('returns an error for failed event delivery', done => {
Expand Down
2 changes: 1 addition & 1 deletion packages/web-worker/src/notifier.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)


bugsnag._logger.debug('Loaded!')

Expand Down
Loading