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-Integrit… #532

Open
wants to merge 12 commits into
base: next
Choose a base branch
from

Conversation

djskinner
Copy link
Contributor

@djskinner djskinner commented Nov 21, 2024

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 endpoint 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

Testing

  • unit tests on delivery-fetch
  • integration tests for browser
  • e2e tests for browser

Comment on lines +76 to +79
// 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
}
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

Copy link

github-actions bot commented Nov 21, 2024

Browser bundle size

NPM build

Package
Before 210.63 kB
After 211.41 kB
± +771 bytes ⚠️

CDN build

Unminified Minfied Minified + gzipped
Before 107.59 kB 40.60 kB 11.89 kB
After 109.22 kB 41.21 kB 12.10 kB
± +1,635 bytes ⚠️ +617 bytes ⚠️ +206 bytes ⚠️

Code coverage

Ok File (✨=New File) Lines Branches Functions Statements
🔴 /home/runner/work/bugsnag-js-performance/bugsnag-js-performance/packages/core/lib/core.ts 100%
(+0%)
90.9%
(-2.85%)
92.85%
(+0%)
98.5%
(+0.04%)
🔴 /home/runner/work/bugsnag-js-performance/bugsnag-js-performance/packages/delivery-fetch/lib/delivery.ts 97.05%
(+1.4%)
94.73%
(-5.27%)
100%
(+0%)
97.05%
(+1.4%)

Total:

Lines Branches Functions Statements
87.9%(+0.08%) 78.98%(+0.22%) 86.78%(+0.08%) 86%(+0.08%)

Generated against 906d8c6 on 29 November 2024 at 17:11:19 UTC

Base automatically changed from fix-delivery-fetch-tests to next November 22, 2024 11:42
@djskinner djskinner changed the base branch from next to gingerbenw/pin-maze-runner-8.7.0 November 26, 2024 13:33
@djskinner djskinner changed the base branch from gingerbenw/pin-maze-runner-8.7.0 to next November 26, 2024 13:33
@djskinner djskinner marked this pull request as ready for review November 26, 2024 14:45
Comment on lines +76 to +79
// 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
}
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

}
}

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

Copy link
Member

@gingerbenw gingerbenw left a comment

Choose a reason for hiding this comment

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

looks good to me - a rebase from next should resolve the ios https tests

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.

2 participants