Skip to content

add sendPayloadChecksums config option and implement Bugsnag-Integrit… #532

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

Open
wants to merge 12 commits into
base: next
Choose a base branch
from
1 change: 1 addition & 0 deletions .tool-versions
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
nodejs 20.11.1
4 changes: 4 additions & 0 deletions jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,15 @@ module.exports = {
{
displayName: 'delivery-fetch',
testMatch: ['<rootDir>/packages/delivery-fetch/**/*.test.ts'],
testEnvironment: 'jsdom',
setupFilesAfterEnv: ['<rootDir>/jest/setup/crypto.ts'],
...defaultModuleConfig
},
{
displayName: 'browser',
testMatch: ['<rootDir>/packages/platforms/browser/**/*.test.ts'],
testEnvironment: 'jsdom',
setupFilesAfterEnv: ['<rootDir>/jest/setup/crypto.ts'],
...defaultModuleConfig
},
{
Expand Down
19 changes: 19 additions & 0 deletions jest/setup/crypto.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { TextDecoder, TextEncoder } from 'node:util'
import crypto from 'crypto'

Object.defineProperty(window, 'crypto', {
get () {
return {
getRandomValues: crypto.getRandomValues,
subtle: crypto.webcrypto.subtle
}
}
})

Object.defineProperty(window, 'TextEncoder', {
get () { return TextEncoder }
})

Object.defineProperty(window, 'TextDecoder', {
get () { return TextDecoder }
})
9 changes: 8 additions & 1 deletion packages/core/lib/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
ATTRIBUTE_STRING_VALUE_LIMIT_MAX
} from './custom-attribute-limits'
import type { Plugin } from './plugin'
import { isLogger, isNumber, isObject, isOnSpanEndCallbacks, isPluginArray, isString, isStringArray, isStringWithLength } from './validation'
import { isBoolean, isLogger, isNumber, isObject, isOnSpanEndCallbacks, isPluginArray, isString, isStringArray, isStringWithLength } from './validation'

type SetTraceCorrelation = (traceId: string, spanId: string) => void

Expand Down Expand Up @@ -53,6 +53,7 @@ export interface Configuration {
attributeStringValueLimit?: number
attributeArrayLengthLimit?: number
attributeCountLimit?: number
sendPayloadChecksums?: boolean
}

export interface TestConfiguration {
Expand Down Expand Up @@ -81,6 +82,7 @@ export interface CoreSchema extends Schema {
plugins: ConfigOption<Array<Plugin<Configuration>>>
bugsnag: ConfigOption<BugsnagErrorStatic | undefined>
samplingProbability: ConfigOption<number | undefined>
sendPayloadChecksums: ConfigOption<boolean>
}

export const schema: CoreSchema = {
Expand Down Expand Up @@ -153,6 +155,11 @@ export const schema: CoreSchema = {
defaultValue: ATTRIBUTE_COUNT_LIMIT_DEFAULT,
message: `should be a number between 1 and ${ATTRIBUTE_COUNT_LIMIT_MAX}`,
validate: (value: unknown): value is number => isNumber(value) && value > 0 && value <= ATTRIBUTE_COUNT_LIMIT_MAX
},
sendPayloadChecksums: {
defaultValue: false,
message: 'should be true|false',
validate: isBoolean
}
}

Expand Down
7 changes: 6 additions & 1 deletion packages/core/lib/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@ export function createClient<S extends CoreSchema, C extends Configuration, T> (
start: (config: C | string) => {
const configuration = validateConfig<S, C>(config, options.schema)

// sendPayloadChecksums is false by default unless custom endpoints are not specified
if (typeof config !== 'string' && !config.endpoint) {
configuration.sendPayloadChecksums = ('sendPayloadChecksums' in config && config.sendPayloadChecksums) || true
}
Comment on lines +76 to +79
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not keen on this but not sure of a better way. We need to work out what config options the user passed in to determine the behaviour. const configuration = validateConfig<S, C>(config, options.schema) does more than just validate, it creates a config with all values set based on the schema, using default values if not supplied.

Copy link
Member

Choose a reason for hiding this comment

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

right now this is the place we're performing similar manipulation on the configuration, perhaps we could refactor the validation to include this, possibly separating out validation and 'cleaning' but I think for now this is fine


// if using the default endpoint add the API key as a subdomain
// e.g. convert URL https://otlp.bugsnag.com/v1/traces to URL https://<project_api_key>.otlp.bugsnag.com/v1/traces
if (configuration.endpoint === schema.endpoint.defaultValue) {
Expand All @@ -92,7 +97,7 @@ export function createClient<S extends CoreSchema, C extends Configuration, T> (
}
}

const delivery = options.deliveryFactory(configuration.endpoint)
const delivery = options.deliveryFactory(configuration.endpoint, configuration.sendPayloadChecksums)

options.spanAttributesSource.configure(configuration)

Expand Down
4 changes: 3 additions & 1 deletion packages/core/lib/delivery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import type { JsonEvent } from './events'
import type { Kind, SpanEnded } from './span'
import { spanToJson } from './span'

export type DeliveryFactory = (endpoint: string) => Delivery
export type DeliveryFactory = (endpoint: string, sendPayloadChecksums: boolean) => Delivery

export type ResponseState = 'success' | 'failure-discard' | 'failure-retryable'

Expand Down Expand Up @@ -60,6 +60,8 @@ export interface TracePayload {
// therefore it's 'undefined' when passed to delivery, which adds a value
// immediately before initiating the request
'Bugsnag-Sent-At'?: string
// 'undefined' when passed to delivery, which adds a value before initiating the request
'Bugsnag-Integrity'?: string
}
}

Expand Down
4 changes: 2 additions & 2 deletions packages/core/tests/core.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ describe('Core', () => {

await jest.runOnlyPendingTimersAsync()

expect(deliveryFactory).toHaveBeenCalledWith('https://a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6.otlp.bugsnag.com/v1/traces')
expect(deliveryFactory).toHaveBeenCalledWith('https://a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6.otlp.bugsnag.com/v1/traces', false)
})
})

Expand All @@ -356,7 +356,7 @@ describe('Core', () => {

await jest.runOnlyPendingTimersAsync()

expect(deliveryFactory).toHaveBeenCalledWith('https://my-custom-otel-repeater.com')
expect(deliveryFactory).toHaveBeenCalledWith('https://my-custom-otel-repeater.com', false)
})
})
})
Expand Down
29 changes: 24 additions & 5 deletions packages/delivery-fetch/lib/delivery.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
import {

responseStateFromStatusCode
} from '@bugsnag/core-performance'
import { responseStateFromStatusCode } from '@bugsnag/core-performance'
import type { BackgroundingListener, Clock, Delivery, DeliveryFactory, TracePayload } from '@bugsnag/core-performance'

export type Fetch = typeof fetch
Expand Down Expand Up @@ -40,14 +37,20 @@ function createFetchDeliveryFactory (
})
}

return function fetchDeliveryFactory (endpoint: string): Delivery {
return function fetchDeliveryFactory (endpoint: string, sendPayloadChecksums?: boolean): Delivery {
return {
async send (payload: TracePayload) {
const body = JSON.stringify(payload.body)

payload.headers['Bugsnag-Sent-At'] = clock.date().toISOString()

try {
const integrityHeaderValue = await getIntegrityHeaderValue(sendPayloadChecksums ?? false, window, body)

if (integrityHeaderValue) {
payload.headers['Bugsnag-Integrity'] = integrityHeaderValue
}

const response = await fetch(endpoint, {
method: 'POST',
keepalive,
Expand All @@ -72,3 +75,19 @@ function createFetchDeliveryFactory (
}

export default createFetchDeliveryFactory

function getIntegrityHeaderValue (sendPayloadChecksums: boolean, windowOrWorkerGlobalScope: Window, requestBody: string) {
if (sendPayloadChecksums && windowOrWorkerGlobalScope.isSecureContext && windowOrWorkerGlobalScope.crypto && windowOrWorkerGlobalScope.crypto.subtle && windowOrWorkerGlobalScope.crypto.subtle.digest && typeof TextEncoder === 'function') {
const msgUint8 = new TextEncoder().encode(requestBody)
return windowOrWorkerGlobalScope.crypto.subtle.digest('SHA-1', msgUint8).then((hashBuffer) => {
const hashArray = Array.from(new Uint8Array(hashBuffer))
const hashHex = hashArray
.map((b) => b.toString(16).padStart(2, '0'))
.join('')

return 'sha1 ' + hashHex
})
}

return Promise.resolve()
}
144 changes: 129 additions & 15 deletions packages/delivery-fetch/tests/delivery.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,3 @@
/**
* @jest-environment jsdom
*/

import type { TracePayload } from '@bugsnag/core-performance'
import type { JsonEvent } from '@bugsnag/core-performance/lib'
import {
Expand All @@ -14,11 +10,15 @@ import createFetchDeliveryFactory from '../lib/delivery'
const SENT_AT_FORMAT = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$/

describe('Browser Delivery', () => {
it('delivers a span', async () => {
const fetch = jest.fn(() => Promise.resolve({ status: 200, headers: new Headers() } as unknown as Response))
const backgroundingListener = new ControllableBackgroundingListener()
const clock = new IncrementingClock('2023-01-02T00:00:00.000Z')
beforeAll(() => {
window.isSecureContext = true
})

afterAll(() => {
window.isSecureContext = false
})

it('delivers a span', async () => {
const deliveryPayload: TracePayload = {
body: {
resourceSpans: [{
Expand All @@ -45,8 +45,12 @@ describe('Browser Delivery', () => {
}
}

const fetch = jest.fn(() => Promise.resolve({ status: 200, headers: new Headers() } as unknown as Response))
const backgroundingListener = new ControllableBackgroundingListener()
const clock = new IncrementingClock('2023-01-02T00:00:00.000Z')

const deliveryFactory = createFetchDeliveryFactory(fetch, clock, backgroundingListener)
const delivery = deliveryFactory('/test')
const delivery = deliveryFactory('/test', true)
const response = await delivery.send(deliveryPayload)

expect(fetch).toHaveBeenCalledWith('/test', {
Expand All @@ -57,7 +61,8 @@ describe('Browser Delivery', () => {
'Bugsnag-Api-Key': 'test-api-key',
'Bugsnag-Span-Sampling': '1:1',
'Content-Type': 'application/json',
'Bugsnag-Sent-At': new Date(clock.timeOrigin + 1).toISOString()
'Bugsnag-Sent-At': new Date(clock.timeOrigin + 1).toISOString(),
'Bugsnag-Integrity': 'sha1 835f1ff603eb1be3bf91fc9716c0841e1b37ee64'
}
})

Expand Down Expand Up @@ -98,7 +103,7 @@ describe('Browser Delivery', () => {
}

const deliveryFactory = createFetchDeliveryFactory(fetch, new IncrementingClock(), backgroundingListener)
const delivery = deliveryFactory('/test')
const delivery = deliveryFactory('/test', false)

backgroundingListener.sendToBackground()

Expand Down Expand Up @@ -153,7 +158,7 @@ describe('Browser Delivery', () => {
}

const deliveryFactory = createFetchDeliveryFactory(fetch, new IncrementingClock(), backgroundingListener)
const delivery = deliveryFactory('/test')
const delivery = deliveryFactory('/test', false)

backgroundingListener.sendToBackground()
backgroundingListener.sendToForeground()
Expand Down Expand Up @@ -200,7 +205,7 @@ describe('Browser Delivery', () => {
const backgroundingListener = new ControllableBackgroundingListener()

const deliveryFactory = createFetchDeliveryFactory(fetch, new IncrementingClock(), backgroundingListener)
const delivery = deliveryFactory('/test')
const delivery = deliveryFactory('/test', false)
const deliveryPayload: TracePayload = {
body: { resourceSpans: [] },
headers: {
Expand Down Expand Up @@ -238,7 +243,7 @@ describe('Browser Delivery', () => {
const backgroundingListener = new ControllableBackgroundingListener()

const deliveryFactory = createFetchDeliveryFactory(fetch, new IncrementingClock(), backgroundingListener)
const delivery = deliveryFactory('/test')
const delivery = deliveryFactory('/test', false)
const payload: TracePayload = {
body: { resourceSpans: [] },
headers: {
Expand Down Expand Up @@ -293,10 +298,119 @@ describe('Browser Delivery', () => {
}

const deliveryFactory = createFetchDeliveryFactory(fetch, clock, backgroundingListener)
const delivery = deliveryFactory('/test')
const delivery = deliveryFactory('/test', false)

const { state } = await delivery.send(deliveryPayload)

expect(state).toBe('failure-discard')
})

it('omits the bugsnag integrity header when not in a secure context', async () => {
const deliveryPayload: TracePayload = {
body: {
resourceSpans: [{
resource: { attributes: [{ key: 'test-key', value: { stringValue: 'test-value' } }] },
scopeSpans: [{
spans: [{
name: 'test-span',
kind: 1,
spanId: 'test-span-id',
traceId: 'test-trace-id',
endTimeUnixNano: '56789',
startTimeUnixNano: '12345',
attributes: [{ key: 'test-span', value: { intValue: '12345' } }],
droppedAttributesCount: 0,
events: []
}]
}]
}]
},
headers: {
'Bugsnag-Api-Key': 'test-api-key',
'Content-Type': 'application/json',
'Bugsnag-Span-Sampling': '1:1'
}
}

window.isSecureContext = false
Copy link
Member

Choose a reason for hiding this comment

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

does this remain false for the next test? we're using beforeAll rather than beforeEach to revert this back to true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah 👍 fixed

const _fetch = jest.fn(() => Promise.resolve({ status: 200, headers: new Headers() } as unknown as Response))
const backgroundingListener = new ControllableBackgroundingListener()
const clock = new IncrementingClock('2023-01-02T00:00:00.000Z')

const deliveryFactory = createFetchDeliveryFactory(_fetch, clock, backgroundingListener)
const delivery = deliveryFactory('/test', true)
const response = await delivery.send(deliveryPayload)

expect(_fetch).toHaveBeenCalledWith('/test', {
method: 'POST',
keepalive: false,
body: JSON.stringify(deliveryPayload.body),
headers: {
'Bugsnag-Api-Key': 'test-api-key',
'Bugsnag-Span-Sampling': '1:1',
'Content-Type': 'application/json',
'Bugsnag-Sent-At': new Date(clock.timeOrigin + 1).toISOString()
}
})

expect(response).toStrictEqual({
state: 'success',
samplingProbability: undefined
})

window.isSecureContext = true
})

it('omits the bugsnag integrity header when sendPayloadChecksums is false', async () => {
const deliveryPayload: TracePayload = {
body: {
resourceSpans: [{
resource: { attributes: [{ key: 'test-key', value: { stringValue: 'test-value' } }] },
scopeSpans: [{
spans: [{
name: 'test-span',
kind: 1,
spanId: 'test-span-id',
traceId: 'test-trace-id',
endTimeUnixNano: '56789',
startTimeUnixNano: '12345',
attributes: [{ key: 'test-span', value: { intValue: '12345' } }],
droppedAttributesCount: 0,
events: []
}]
}]
}]
},
headers: {
'Bugsnag-Api-Key': 'test-api-key',
'Content-Type': 'application/json',
'Bugsnag-Span-Sampling': '1:1'
}
}

const fetch = jest.fn(() => Promise.resolve({ status: 200, headers: new Headers() } as unknown as Response))
const backgroundingListener = new ControllableBackgroundingListener()
const clock = new IncrementingClock('2023-01-02T00:00:00.000Z')

const deliveryFactory = createFetchDeliveryFactory(fetch, clock, backgroundingListener)
const delivery = deliveryFactory('/test', false)
const response = await delivery.send(deliveryPayload)

expect(fetch).toHaveBeenCalledWith('/test', {
method: 'POST',
keepalive: false,
body: JSON.stringify(deliveryPayload.body),
headers: {
'Bugsnag-Api-Key': 'test-api-key',
'Bugsnag-Span-Sampling': '1:1',
'Content-Type': 'application/json',
'Bugsnag-Sent-At': new Date(clock.timeOrigin + 1).toISOString()
}
})

expect(response).toStrictEqual({
state: 'success',
samplingProbability: undefined
})
})
})
Loading