-
Notifications
You must be signed in to change notification settings - Fork 142
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
🐛 allow untrusted event for httpRequest xhr event listeners #3123
Conversation
/to-staging |
Devflow running:
|
…ng-46 Integrated commit sha: 5f3f019 Co-authored-by: Thomas Lebeau <thomas.lebeau@datadoghq.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3123 +/- ##
==========================================
- Coverage 93.63% 93.17% -0.47%
==========================================
Files 276 276
Lines 7616 7615 -1
Branches 1709 1708 -1
==========================================
- Hits 7131 7095 -36
- Misses 485 520 +35 ☔ View full report in Codecov by Sentry. |
@@ -49,11 +49,12 @@ class MockEventEmitter { | |||
this.listeners[name] = this.listeners[name].filter((listener) => listener !== callback) | |||
} | |||
|
|||
protected dispatchEvent(name: string) { | |||
if (!this.listeners[name]) { | |||
dispatchEvent(evt: Event) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dispatchEvent
is normally exposed on the native xhr, so I see no reason to not expose it for mockXHR, especially as I a change the signature to be more standard.
It's just a tiny bit more verbose where dispatchEvent is called
Co-authored-by: Benoît <benoit.zugmeyer@datadoghq.com>
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
@@ -187,7 +187,7 @@ describe('httpRequest', () => { | |||
const onResponseSpy = jasmine.createSpy('xhrOnResponse') | |||
|
|||
interceptor.withMockXhr((xhr) => { | |||
const syntheticEvent = new Event('loadend') | |||
const syntheticEvent = createNewEvent('loadend', {}, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 suggestion: maybe leveraging the properties
argument like createNewEvent('loadend', { __ddIsTrusted: false })
could be simpler/more explicit...
71cfaae
to
9d3552e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Motivation
Fixes an issue where a non native XMLHttpRequest override is dispatching synthetic events (i.e. non-trusted) result in the callback not being executed, and breaking session replay ingestion.
Changes
allow untrusted event in that particular use-case. This is safe as we're not using any properties from the event.
Testing
I have gone over the contributing documentation.