[RUMF-766] prevent request duration override by wrong matching timing (…

…#604)

Reduce risk of matching error by not considering invalid or incomplete timing due to cross origin restrictions.

packages/rum/src/domain/assemblyV2.spec.ts

@@ -1,5 +1,5 @@
 import { Context } from '@datadog/browser-core'
-import { createRawRumEvent } from '../../test/createRawRumEvent'
+import { createRawRumEvent } from '../../test/fixtures'
 import { setup, TestSetupBuilder } from '../../test/specHelper'
 import { RumEventType } from '../typesV2'
 import { LifeCycle, LifeCycleEventType } from './lifeCycle'

packages/rum/src/domain/rumEventsCollection/action/trackActions.spec.ts

@@ -1,5 +1,5 @@
 import { DOM_EVENT } from '@datadog/browser-core'
-import { createRawRumEvent } from '../../../../test/createRawRumEvent'
+import { createRawRumEvent } from '../../../../test/fixtures'
 import { setup, TestSetupBuilder } from '../../../../test/specHelper'
 import { RumErrorEvent, RumEventCategory } from '../../../types'
 import { RumEventType } from '../../../typesV2'

packages/rum/src/domain/rumEventsCollection/resource/matchRequestTiming.spec.ts

@@ -1,4 +1,5 @@
 import { isIE } from '@datadog/browser-core'
+import { createResourceEntry } from '../../../../test/fixtures'
 import { RumPerformanceResourceTiming } from '../../../browser/performanceCollection'
 import { RequestCompleteEvent } from '../../requestCollection'
 
@@ -17,52 +18,61 @@ describe('matchRequestTiming', () => {
   })
 
   it('should match single timing nested in the request ', () => {
-    const match: Partial<RumPerformanceResourceTiming> = { startTime: 200, duration: 300 }
-    entries.push(match as RumPerformanceResourceTiming)
+    const match = createResourceEntry({ startTime: 200, duration: 300 })
+    entries.push(match)
 
     const timing = matchRequestTiming(FAKE_REQUEST as RequestCompleteEvent)
 
-    expect(timing).toEqual(match as RumPerformanceResourceTiming)
+    expect(timing).toEqual(match)
   })
 
   it('should not match single timing outside the request ', () => {
-    const match: Partial<RumPerformanceResourceTiming> = { startTime: 0, duration: 300 }
-    entries.push(match as RumPerformanceResourceTiming)
+    const match = createResourceEntry({ startTime: 0, duration: 300 })
+    entries.push(match)
 
     const timing = matchRequestTiming(FAKE_REQUEST as RequestCompleteEvent)
 
     expect(timing).toEqual(undefined)
   })
 
   it('should match two following timings nested in the request ', () => {
-    const optionsTiming: Partial<RumPerformanceResourceTiming> = { startTime: 200, duration: 50 }
-    const actualTiming: Partial<RumPerformanceResourceTiming> = { startTime: 300, duration: 100 }
-    entries.push(optionsTiming as RumPerformanceResourceTiming, actualTiming as RumPerformanceResourceTiming)
+    const optionsTiming = createResourceEntry({ startTime: 150, duration: 50 })
+    const actualTiming = createResourceEntry({ startTime: 200, duration: 100 })
+    entries.push(optionsTiming, actualTiming)
 
     const timing = matchRequestTiming(FAKE_REQUEST as RequestCompleteEvent)
 
-    expect(timing).toEqual(actualTiming as RumPerformanceResourceTiming)
+    expect(timing).toEqual(actualTiming)
   })
 
   it('should not match two not following timings nested in the request ', () => {
-    const match1: Partial<RumPerformanceResourceTiming> = { startTime: 200, duration: 100 }
-    const match2: Partial<RumPerformanceResourceTiming> = { startTime: 250, duration: 100 }
-    entries.push(match1 as RumPerformanceResourceTiming, match2 as RumPerformanceResourceTiming)
+    const match1 = createResourceEntry({ startTime: 150, duration: 100 })
+    const match2 = createResourceEntry({ startTime: 200, duration: 100 })
+    entries.push(match1, match2)
 
     const timing = matchRequestTiming(FAKE_REQUEST as RequestCompleteEvent)
 
     expect(timing).toEqual(undefined)
   })
 
   it('should not match multiple timings nested in the request', () => {
-    const match1: Partial<RumPerformanceResourceTiming> = { startTime: 100, duration: 100 }
-    const match2: Partial<RumPerformanceResourceTiming> = { startTime: 200, duration: 100 }
-    const match3: Partial<RumPerformanceResourceTiming> = { startTime: 300, duration: 100 }
-    entries.push(
-      match1 as RumPerformanceResourceTiming,
-      match2 as RumPerformanceResourceTiming,
-      match3 as RumPerformanceResourceTiming
-    )
+    const match1 = createResourceEntry({ startTime: 100, duration: 50 })
+    const match2 = createResourceEntry({ startTime: 150, duration: 50 })
+    const match3 = createResourceEntry({ startTime: 200, duration: 50 })
+    entries.push(match1, match2, match3)
+
+    const timing = matchRequestTiming(FAKE_REQUEST as RequestCompleteEvent)
+
+    expect(timing).toEqual(undefined)
+  })
+
+  it('should match invalid timing nested in the request ', () => {
+    const match = createResourceEntry({
+      duration: 100,
+      fetchStart: 0,
+      startTime: 200,
+    })
+    entries.push(match)
 
     const timing = matchRequestTiming(FAKE_REQUEST as RequestCompleteEvent)
 

packages/rum/src/domain/rumEventsCollection/resource/matchRequestTiming.ts

@@ -1,5 +1,6 @@
 import { RumPerformanceResourceTiming } from '../../../browser/performanceCollection'
 import { RequestCompleteEvent } from '../../requestCollection'
+import { toValidEntry } from './resourceUtils'
 
 interface Timing {
   startTime: number
@@ -14,6 +15,7 @@ interface Timing {
  * - Browsers generate a timing entry for OPTIONS request
  *
  * Strategy:
+ * - from valid nested entries
  * - if a single timing match, return the timing
  * - if two following timings match (OPTIONS request), return the timing for the actual request
  * - otherwise we can't decide, return undefined
@@ -22,11 +24,11 @@ export function matchRequestTiming(request: RequestCompleteEvent) {
   if (!performance || !('getEntriesByName' in performance)) {
     return
   }
-  const candidates = (performance
+  const candidates = performance
     .getEntriesByName(request.url, 'resource')
-    .filter((entry) =>
-      isBetween(entry, request.startTime, endTime(request))
-    ) as unknown) as RumPerformanceResourceTiming[]
+    .map((entry) => entry.toJSON() as RumPerformanceResourceTiming)
+    .filter(toValidEntry)
+    .filter((entry) => isBetween(entry, request.startTime, endTime(request)))
 
   if (candidates.length === 1) {
     return candidates[0]

packages/rum/src/domain/rumEventsCollection/resource/resourceCollection.spec.ts

@@ -1,6 +1,6 @@
 import { RequestType, ResourceType } from '@datadog/browser-core'
+import { createResourceEntry } from '../../../../test/fixtures'
 import { setup, TestSetupBuilder } from '../../../../test/specHelper'
-import { RumPerformanceResourceTiming } from '../../../browser/performanceCollection'
 import { RumEventCategory, RumResourceEvent } from '../../../types'
 import { RumEventType, RumResourceEventV2 } from '../../../typesV2'
 import { LifeCycleEventType } from '../../lifeCycle'
@@ -424,29 +424,6 @@ describe('resourceCollection V2', () => {
   })
 })
 
-function createResourceEntry(details?: Partial<RumPerformanceResourceTiming>): RumPerformanceResourceTiming {
-  const entry: Partial<RumPerformanceResourceTiming> = {
-    connectEnd: 0,
-    connectStart: 0,
-    decodedBodySize: 0,
-    domainLookupEnd: 0,
-    domainLookupStart: 0,
-    duration: 100,
-    entryType: 'resource',
-    fetchStart: 0,
-    name: 'https://resource.com/valid',
-    redirectEnd: 0,
-    redirectStart: 0,
-    requestStart: 0,
-    responseEnd: 0,
-    responseStart: 0,
-    secureConnectionStart: 0,
-    startTime: 1234,
-    ...details,
-  }
-  return entry as RumPerformanceResourceTiming
-}
-
 function createCompletedRequest(details?: Partial<RequestCompleteEvent>): RequestCompleteEvent {
   const request: Partial<RequestCompleteEvent> = {
     duration: 100,

packages/rum/src/domain/rumEventsCollection/resource/resourceUtils.ts

@@ -89,9 +89,16 @@ export function computePerformanceResourceDuration(entry: RumPerformanceResource
 export function computePerformanceResourceDetails(
   entry: RumPerformanceResourceTiming
 ): PerformanceResourceDetails | undefined {
+  const validEntry = toValidEntry(entry)
+
+  if (!validEntry) {
+    return undefined
+  }
   const {
     startTime,
     fetchStart,
+    redirectStart,
+    redirectEnd,
     domainLookupStart,
     domainLookupEnd,
     connectStart,
@@ -100,47 +107,7 @@ export function computePerformanceResourceDetails(
     requestStart,
     responseStart,
     responseEnd,
-  } = entry
-  let { redirectStart, redirectEnd } = entry
-
-  // Ensure timings are in the right order.  On top of filtering out potential invalid
-  // RumPerformanceResourceTiming, it will ignore entries from requests where timings cannot be
-  // collected, for example cross origin requests without a "Timing-Allow-Origin" header allowing
-  // it.
-  if (
-    !areInOrder(
-      startTime,
-      fetchStart,
-      domainLookupStart,
-      domainLookupEnd,
-      connectStart,
-      connectEnd,
-      requestStart,
-      responseStart,
-      responseEnd
-    )
-  ) {
-    return undefined
-  }
-
-  // The only time fetchStart is different than startTime is if a redirection occurred.
-  const hasRedirectionOccurred = fetchStart !== startTime
-
-  if (hasRedirectionOccurred) {
-    // Firefox doesn't provide redirect timings on cross origin requests.  Provide a default for
-    // those.
-    if (redirectStart < startTime) {
-      redirectStart = startTime
-    }
-    if (redirectEnd < startTime) {
-      redirectEnd = fetchStart
-    }
-
-    // Make sure redirect timings are in order
-    if (!areInOrder(startTime, redirectStart, redirectEnd, fetchStart)) {
-      return undefined
-    }
-  }
+  } = validEntry
 
   const details: PerformanceResourceDetails = {
     download: formatTiming(startTime, responseStart, responseEnd),
@@ -162,13 +129,65 @@ export function computePerformanceResourceDetails(
     details.dns = formatTiming(startTime, domainLookupStart, domainLookupEnd)
   }
 
-  if (hasRedirectionOccurred) {
+  if (hasRedirection(entry)) {
     details.redirect = formatTiming(startTime, redirectStart, redirectEnd)
   }
 
   return details
 }
 
+export function toValidEntry(entry: RumPerformanceResourceTiming) {
+  // Ensure timings are in the right order. On top of filtering out potential invalid
+  // RumPerformanceResourceTiming, it will ignore entries from requests where timings cannot be
+  // collected, for example cross origin requests without a "Timing-Allow-Origin" header allowing
+  // it.
+  if (
+    !areInOrder(
+      entry.startTime,
+      entry.fetchStart,
+      entry.domainLookupStart,
+      entry.domainLookupEnd,
+      entry.connectStart,
+      entry.connectEnd,
+      entry.requestStart,
+      entry.responseStart,
+      entry.responseEnd
+    )
+  ) {
+    return undefined
+  }
+
+  if (!hasRedirection(entry)) {
+    return entry
+  }
+
+  let { redirectStart, redirectEnd } = entry
+  // Firefox doesn't provide redirect timings on cross origin requests.
+  // Provide a default for those.
+  if (redirectStart < entry.startTime) {
+    redirectStart = entry.startTime
+  }
+  if (redirectEnd < entry.startTime) {
+    redirectEnd = entry.fetchStart
+  }
+
+  // Make sure redirect timings are in order
+  if (!areInOrder(entry.startTime, redirectStart, redirectEnd, entry.fetchStart)) {
+    return undefined
+  }
+
+  return {
+    ...entry,
+    redirectEnd,
+    redirectStart,
+  }
+}
+
+function hasRedirection(entry: RumPerformanceResourceTiming) {
+  // The only time fetchStart is different than startTime is if a redirection occurred.
+  return entry.fetchStart !== entry.startTime
+}
+
 function formatTiming(origin: number, start: number, end: number) {
   return {
     duration: msToNs(end - start),

packages/rum/src/domain/rumEventsCollection/view/trackViews.spec.ts

@@ -1,4 +1,4 @@
-import { createRawRumEvent } from '../../../../test/createRawRumEvent'
+import { createRawRumEvent } from '../../../../test/fixtures'
 import { setup, TestSetupBuilder } from '../../../../test/specHelper'
 import { RumPerformanceNavigationTiming, RumPerformancePaintTiming } from '../../../browser/performanceCollection'
 

packages/rum/test/createRawRumEvent.ts → packages/rum/test/fixtures.ts

@@ -1,4 +1,5 @@
 import { combine, Context, ErrorSource, ResourceType } from '@datadog/browser-core'
+import { RumPerformanceResourceTiming } from '../src/browser/performanceCollection'
 import { ActionType } from '../src/domain/rumEventsCollection/action/trackActions'
 import { ViewLoadingType } from '../src/domain/rumEventsCollection/view/trackViews'
 import { RawRumEventV2, RumEventType } from '../src/typesV2'
@@ -76,3 +77,29 @@ export function createRawRumEvent(type: RumEventType, overrides?: Context): RawR
       )
   }
 }
+
+export function createResourceEntry(
+  overrides?: Partial<RumPerformanceResourceTiming>
+): RumPerformanceResourceTiming & PerformanceResourceTiming {
+  const entry: Partial<RumPerformanceResourceTiming & PerformanceResourceTiming> = {
+    connectEnd: 200,
+    connectStart: 200,
+    decodedBodySize: 200,
+    domainLookupEnd: 200,
+    domainLookupStart: 200,
+    duration: 100,
+    entryType: 'resource',
+    fetchStart: 200,
+    name: 'https://resource.com/valid',
+    redirectEnd: 200,
+    redirectStart: 200,
+    requestStart: 200,
+    responseEnd: 300,
+    responseStart: 200,
+    secureConnectionStart: 200,
+    startTime: 200,
+    ...overrides,
+  }
+  entry.toJSON = () => entry
+  return entry as RumPerformanceResourceTiming & PerformanceResourceTiming
+}