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

fix: Exclude data url requests as captured AJAX events #1012

Merged
merged 2 commits into from
May 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 6 additions & 7 deletions src/common/deny-list/deny-list.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,9 @@ var denyList = []
* @returns {boolean} `true` if request does not match any entries of {@link denyList|deny list}; else `false`
*/
export function shouldCollectEvent (params) {
if (denyList.length === 0) {
return true
}
if (hasUndefinedHostname(params)) return false

// XHR requests with an undefined hostname (e.g., data URLs) should not be collected.
if (params.hostname === undefined) {
return false
}
if (denyList.length === 0) return true

for (var i = 0; i < denyList.length; i++) {
var parsed = denyList[i]
Expand All @@ -35,6 +30,10 @@ export function shouldCollectEvent (params) {
return true
}

export function hasUndefinedHostname (params) {
return (params.hostname === undefined) // requests with an undefined hostname (e.g., data URLs) should not be collected.
}

/**
* Initializes the {@link denyList|XHR deny list} by extracting hostname and pathname from an array of filter strings.
* @param {string[]} denyListConfig - array of URL filters to identify XHR requests to be excluded from collection
Expand Down
20 changes: 11 additions & 9 deletions src/features/ajax/instrument/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { FEATURE_NAME } from '../constants'
import { FEATURE_NAMES } from '../../../loaders/features/features'
import { SUPPORTABILITY_METRIC } from '../../metrics/constants'
import { now } from '../../../common/timing/now'
import { hasUndefinedHostname } from '../../../common/deny-list/deny-list'

var handlers = ['load', 'error', 'abort', 'timeout']
var handlersLen = handlers.length
Expand Down Expand Up @@ -338,18 +339,18 @@ function subscribeToEvents (agentIdentifier, ee, handler, dt) {
// eslint-disable-next-line handle-callback-err
function onFetchDone (_, res) {
this.endTime = now()
if (!this.params) {
this.params = {}
}
if (!this.params) this.params = {}
if (hasUndefinedHostname(this.params)) return // don't bother with fetch to url with no hostname

this.params.status = res ? res.status : 0

// convert rxSize to a number
var responseSize
let responseSize
if (typeof this.rxSize === 'string' && this.rxSize.length > 0) {
responseSize = +this.rxSize
}

var metrics = {
const metrics = {
txSize: this.txSize,
rxSize: responseSize,
duration: now() - this.startTime
Expand All @@ -360,17 +361,18 @@ function subscribeToEvents (agentIdentifier, ee, handler, dt) {

// Create report for XHR request that has finished
function end (xhr) {
var params = this.params
var metrics = this.metrics

const params = this.params
const metrics = this.metrics
if (this.ended) return
this.ended = true

for (var i = 0; i < handlersLen; i++) {
for (let i = 0; i < handlersLen; i++) {
xhr.removeEventListener(handlers[i], this.listener, false)
}

if (params.aborted) return
if (hasUndefinedHostname(params)) return // don't bother with XHR of url with no hostname

metrics.duration = now() - this.startTime
if (!this.loadCaptureCalled && xhr.readyState === 4) {
captureXhrData(this, xhr)
Expand Down
31 changes: 0 additions & 31 deletions tests/assets/xhr-data-url.html

This file was deleted.

6 changes: 3 additions & 3 deletions tests/components/ajax/instrument.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ beforeAll(() => {
baseEE = ajaxInstrument.ee
})

describe('Data url DOES generates telemetry for', () => {
describe('Ajax event is not captured or buffered for data urls', () => {
let xhrEventLogged
beforeAll(() => { baseEE.on('xhr', monitorXhrEvent) })
beforeEach(() => { xhrEventLogged = false })
Expand All @@ -37,7 +37,7 @@ describe('Data url DOES generates telemetry for', () => {
xhr.onreadystatechange = function () {
if (xhr.readyState === XMLHttpRequest.DONE) {
setTimeout(() => { // the following has to wait for next loop so monitorXhrEvent can run
expect(xhrEventLogged).toEqual(true)
expect(xhrEventLogged).toEqual(false)
baseEE.removeEventListener('send-xhr-start', validateIsDataProtocol)
done()
}, 0)
Expand All @@ -50,7 +50,7 @@ describe('Data url DOES generates telemetry for', () => {
baseEE.on('fetch-done', validateIsDataProtocol)

fetch('data:,dataUrl').then(() => {
expect(xhrEventLogged).toEqual(true)
expect(xhrEventLogged).toEqual(false)
baseEE.removeEventListener('fetch-done', validateIsDataProtocol)
done()
})
Expand Down
60 changes: 0 additions & 60 deletions tests/functional/xhr/xhr-data-url.test.js

This file was deleted.

18 changes: 18 additions & 0 deletions tests/specs/ajax/deny-list.e2e.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,4 +75,22 @@ describe('xhr events deny list', () => {
})
]))
})

it.withBrowsersMatching(supportsFetch)('does not capture data URLs (or events with undefined hostname) at all', async () => {
const url = await browser.testHandle.assetURL('instrumented.html') // has no deny list
await browser.url(url).then(() => browser.waitForAgentLoad())

const [ajaxEvents, ajaxMetrics] = await Promise.all([
browser.testHandle.expectAjaxEvents(),
browser.testHandle.expectAjaxTimeSlices(),
browser.execute(function () {
fetch('data:,Hello%2C%20World%21')
})
])

const undefinedDomainEvt = ajaxEvents.request.body.find(obj => obj.domain.startsWith('undefined'))
expect(undefinedDomainEvt).toBeUndefined()
const undefinedHostMetric = ajaxMetrics.request.body.xhr.find(obj => obj.params.host.startsWith('undefined'))
expect(undefinedHostMetric).toBeUndefined()
})
})
71 changes: 0 additions & 71 deletions tests/specs/ajax/fetch.e2e.js
Original file line number Diff line number Diff line change
Expand Up @@ -200,77 +200,6 @@ describe.withBrowsersMatching(supportsFetch)('Fetch Ajax', () => {
})
})

it('creates event and metric data for fetch using data uri', async () => {
await browser.url(await browser.testHandle.assetURL('ajax/fetch-data-uri.html'))
.then(() => browser.waitForAgentLoad())

const [ajaxEventsHarvest, ajaxTimeSlicesHarvest] = await Promise.all([
browser.testHandle.expectAjaxEvents(),
browser.testHandle.expectAjaxTimeSlices(),
browser.execute(function () {
// We don't want the spa feature to pick up the ajax call
window.disableAjaxHashChange = true
}).then(() => $('#sendAjax').click())
])

const ajaxEvent = ajaxEventsHarvest.request.body.find(event => event.domain === 'undefined:undefined')
expect(ajaxEvent).toEqual({
type: 'ajax',
children: [],
start: expect.toBeWithin(1, Infinity),
end: expect.toBeWithin(1, Infinity),
callbackEnd: expect.toBeWithin(1, Infinity),
callbackDuration: 0,
method: 'GET',
status: 200,
domain: 'undefined:undefined',
path: '',
requestBodySize: 0,
// Only firefox captures the data response size
responseBodySize: browserMatch(onlyFirefox) ? 8 : 0,
requestedWith: 'fetch',
nodeId: '0',
guid: null,
traceId: null,
timestamp: null
})

const ajaxMetric = ajaxTimeSlicesHarvest.request.body.xhr.find(metric => metric.params.host === 'undefined:undefined')
expect(ajaxMetric).toEqual({
params: {
protocol: 'data',
host: 'undefined:undefined',
method: 'GET',
status: 200
},
metrics: {
count: 1,
duration: {
t: expect.toBeWithin(0, Infinity)
},
// Only firefox captures the data response size
...(browserMatch(onlyFirefox)
? {
rxSize: {
t: 8
}
}
: {
rxSize: {
c: 1
}
}
),
time: {
t: expect.toBeWithin(1, Infinity)
},
txSize: {
t: 0
}
}
})
})

;[
{ title: 'promise resolved callbacks', url: 'ajax/fetch-promise-resolve.html' },
{ title: 'promise rejected callbacks', url: 'ajax/fetch-promise-reject.html' },
Expand Down
62 changes: 1 addition & 61 deletions tests/specs/ajax/xhr.e2e.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { notIE, onlyFirefox } from '../../../tools/browser-matcher/common-matchers.mjs'
import { onlyFirefox } from '../../../tools/browser-matcher/common-matchers.mjs'

describe('XHR Ajax', () => {
it('creates event and metric data for xhr', async () => {
Expand Down Expand Up @@ -196,66 +196,6 @@ describe('XHR Ajax', () => {
})
})

it.withBrowsersMatching(notIE)('creates event and metric data for xhr using data uri', async () => {
await browser.url(await browser.testHandle.assetURL('ajax/xhr-data-uri.html'))
.then(() => browser.waitForAgentLoad())

const [ajaxEventsHarvest, ajaxTimeSlicesHarvest] = await Promise.all([
browser.testHandle.expectAjaxEvents(),
browser.testHandle.expectAjaxTimeSlices(),
browser.execute(function () {
// We don't want the spa feature to pick up the ajax call
window.disableAjaxHashChange = true
}).then(() => $('#sendAjax').click())
])

const ajaxEvent = ajaxEventsHarvest.request.body.find(event => event.domain === 'undefined:undefined')
expect(ajaxEvent).toEqual({
type: 'ajax',
children: [],
start: expect.toBeWithin(1, Infinity),
end: expect.toBeWithin(1, Infinity),
callbackEnd: expect.toBeWithin(1, Infinity),
callbackDuration: 0,
method: 'GET',
status: 200,
domain: 'undefined:undefined',
path: '',
requestBodySize: 0,
responseBodySize: 8,
requestedWith: 'XMLHttpRequest',
nodeId: '0',
guid: null,
traceId: null,
timestamp: null
})

const ajaxMetric = ajaxTimeSlicesHarvest.request.body.xhr.find(metric => metric.params.host === 'undefined:undefined')
expect(ajaxMetric).toEqual({
params: {
protocol: 'data',
host: 'undefined:undefined',
method: 'GET',
status: 200
},
metrics: {
cbTime: {
t: expect.toBeWithin(75, 126)
},
count: 1,
duration: {
t: expect.toBeWithin(0, Infinity)
},
rxSize: {
t: 8
},
time: {
t: expect.toBeWithin(1, Infinity)
}
}
})
})

it('only includes load handlers in metric timings', async () => {
await browser.url(await browser.testHandle.assetURL('ajax/xhr-callback-duration.html'))
.then(() => browser.waitForAgentLoad())
Expand Down
Loading