-
-
Notifications
You must be signed in to change notification settings - Fork 358
feat(tracing): Wrap Expo Router push, replace, navigate, back, dismiss in addition to prefetch
#6221
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
base: main
Are you sure you want to change the base?
feat(tracing): Wrap Expo Router push, replace, navigate, back, dismiss in addition to prefetch
#6221
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,64 +1,103 @@ | ||
| import { SPAN_STATUS_ERROR, SPAN_STATUS_OK, startInactiveSpan } from '@sentry/core'; | ||
| import { addBreadcrumb, getClient, SPAN_STATUS_ERROR, SPAN_STATUS_OK, startInactiveSpan } from '@sentry/core'; | ||
|
|
||
| import { SPAN_ORIGIN_AUTO_EXPO_ROUTER_PREFETCH } from './origin'; | ||
| import { SPAN_ORIGIN_AUTO_EXPO_ROUTER_NAVIGATION, SPAN_ORIGIN_AUTO_EXPO_ROUTER_PREFETCH } from './origin'; | ||
| import { setPendingExpoRouterNavigation } from './pendingExpoRouterNavigation'; | ||
|
|
||
| type ExpoRouterHref = string | { pathname?: string; params?: Record<string, unknown> }; | ||
|
|
||
| /** | ||
| * Type definition for Expo Router's router object | ||
| * Type definition for Expo Router's router object. | ||
| */ | ||
| export interface ExpoRouter { | ||
| prefetch?: (href: string | { pathname?: string; params?: Record<string, unknown> }) => void | Promise<void>; | ||
| // Other router methods can be added here if needed | ||
| prefetch?: (href: ExpoRouterHref) => void | Promise<void>; | ||
| push?: (...args: unknown[]) => void; | ||
| replace?: (...args: unknown[]) => void; | ||
| back?: () => void; | ||
| navigate?: (...args: unknown[]) => void; | ||
| back?: () => void; | ||
| dismiss?: (count?: number) => void; | ||
| } | ||
|
|
||
| type NavigationMethod = 'push' | 'replace' | 'navigate' | 'back' | 'dismiss'; | ||
|
|
||
| interface ParsedHref { | ||
| href?: unknown; | ||
| routeName: string; | ||
| pathname?: string; | ||
| params?: Record<string, unknown>; | ||
| } | ||
|
|
||
| /** | ||
| * Wraps Expo Router. It currently only does one thing: extends prefetch() method | ||
| * to add automated performance monitoring. | ||
| * Wraps Expo Router methods to add automated performance monitoring and breadcrumbs. | ||
| * | ||
| * Currently wraps: | ||
| * - `prefetch` โ wraps the call in a `navigation.prefetch` span. | ||
| * - `push` / `replace` / `navigate` / `back` / `dismiss` โ adds a navigation | ||
| * breadcrumb, wraps the call in a short-lived span that mirrors prefetch's | ||
| * error/status handling, and tags the subsequent idle navigation transaction | ||
| * with the initiating `navigation.method` so the resulting span can be | ||
| * attributed back to the call site. | ||
| * | ||
| * This function instruments the `prefetch` method of an Expo Router instance | ||
| * to create performance spans that measure how long route prefetching takes. | ||
| * Safe to call repeatedly โ guarded by a single `__sentryWrapped` flag. | ||
| * | ||
| * @param router - The Expo Router instance from `useRouter()` hook | ||
| * @returns The same router instance with an instrumented prefetch method | ||
| * @returns The same router instance with instrumented methods | ||
| */ | ||
| export function wrapExpoRouter<T extends ExpoRouter>(router: T): T { | ||
| if (!router?.prefetch) { | ||
| if (!router) { | ||
| return router; | ||
| } | ||
|
|
||
| // Check if already wrapped to avoid double-wrapping | ||
| if ((router as T & { __sentryPrefetchWrapped?: boolean }).__sentryPrefetchWrapped) { | ||
| const wrappedRouter = router as T & { __sentryWrapped?: boolean }; | ||
| if (wrappedRouter.__sentryWrapped) { | ||
| return router; | ||
| } | ||
|
|
||
| const originalPrefetch = router.prefetch.bind(router); | ||
| if (router.prefetch) { | ||
| wrapPrefetch(router); | ||
| } | ||
|
|
||
| router.prefetch = ((href: Parameters<NonNullable<ExpoRouter['prefetch']>>[0]) => { | ||
| // Extract route name from href for better span naming | ||
| let routeName = 'unknown'; | ||
| if (typeof href === 'string') { | ||
| routeName = href; | ||
| } else if (href && typeof href === 'object' && 'pathname' in href && href.pathname) { | ||
| routeName = href.pathname; | ||
| } | ||
| if (router.push) { | ||
| router.push = wrapNavigationMethod(router, 'push', router.push.bind(router)); | ||
| } | ||
| if (router.replace) { | ||
| router.replace = wrapNavigationMethod(router, 'replace', router.replace.bind(router)); | ||
| } | ||
| if (router.navigate) { | ||
| router.navigate = wrapNavigationMethod(router, 'navigate', router.navigate.bind(router)); | ||
| } | ||
| if (router.back) { | ||
| router.back = wrapNavigationMethod(router, 'back', router.back.bind(router)) as NonNullable<T['back']>; | ||
| } | ||
| if (router.dismiss) { | ||
| const originalDismiss = router.dismiss.bind(router) as (...args: unknown[]) => unknown; | ||
| router.dismiss = wrapNavigationMethod(router, 'dismiss', originalDismiss) as NonNullable<T['dismiss']>; | ||
| } | ||
|
Check warning on line 74 in packages/core/src/js/tracing/expoRouter.ts
|
||
|
|
||
| wrappedRouter.__sentryWrapped = true; | ||
| return router; | ||
| } | ||
|
|
||
| function wrapPrefetch<T extends ExpoRouter>(router: T): void { | ||
|
sentry-warden[bot] marked this conversation as resolved.
|
||
| const originalPrefetch = router.prefetch!.bind(router); | ||
|
|
||
| router.prefetch = ((href: ExpoRouterHref) => { | ||
| const { routeName } = parseHref(href); | ||
|
|
||
| const span = startInactiveSpan({ | ||
| op: 'navigation.prefetch', | ||
| name: `Prefetch ${routeName}`, | ||
| attributes: { | ||
| 'sentry.origin': SPAN_ORIGIN_AUTO_EXPO_ROUTER_PREFETCH, | ||
| 'route.href': typeof href === 'string' ? href : JSON.stringify(href), | ||
| 'route.name': routeName, | ||
|
Check warning on line 91 in packages/core/src/js/tracing/expoRouter.ts
|
||
| // `route.href` may contain dynamic segment values (e.g. `/users/123`) | ||
| // or stringified `params`, so it is gated behind `sendDefaultPii`. | ||
| ...(isSendDefaultPiiEnabled() ? { 'route.href': serializeHref(href) } : undefined), | ||
| }, | ||
| }); | ||
|
|
||
| try { | ||
| const result = originalPrefetch(href); | ||
|
|
||
| // Handle both promise and synchronous returns | ||
| if (result && typeof result === 'object' && 'then' in result && typeof result.then === 'function') { | ||
| return result | ||
| .then(res => { | ||
|
|
@@ -71,21 +110,101 @@ | |
| span?.end(); | ||
| throw error; | ||
| }); | ||
| } else { | ||
| // Synchronous completion | ||
| span?.setStatus({ code: SPAN_STATUS_OK }); | ||
| span?.end(); | ||
| return result; | ||
| } | ||
|
|
||
| span?.setStatus({ code: SPAN_STATUS_OK }); | ||
| span?.end(); | ||
| return result; | ||
| } catch (error) { | ||
| span?.setStatus({ code: SPAN_STATUS_ERROR, message: String(error) }); | ||
| span?.end(); | ||
| throw error; | ||
| } | ||
| }) as NonNullable<T['prefetch']>; | ||
| } | ||
|
|
||
| // Mark as wrapped to prevent double-wrapping | ||
| (router as T & { __sentryPrefetchWrapped?: boolean }).__sentryPrefetchWrapped = true; | ||
| function wrapNavigationMethod( | ||
| router: ExpoRouter, | ||
| method: NavigationMethod, | ||
| original: (...args: unknown[]) => unknown, | ||
| ): (...args: unknown[]) => unknown { | ||
| return (...args: unknown[]) => { | ||
| const parsed = parseMethodArgs(method, args); | ||
| const sendPii = isSendDefaultPiiEnabled(); | ||
|
|
||
| return router; | ||
| addBreadcrumb({ | ||
| category: 'navigation', | ||
| type: 'navigation', | ||
| message: `Expo Router ${method}${parsed.pathname ? ` to ${parsed.pathname}` : ''}`, | ||
| data: { | ||
| method, | ||
| ...(parsed.pathname ? { pathname: parsed.pathname } : undefined), | ||
| // `href` (raw URL form) and `params` may contain user identifiers or | ||
| // other PII (e.g. `/users/42`, `{ id: '42' }`). Mirror the behavior of | ||
| // `reactnavigation.ts` and only include them when `sendDefaultPii` is on. | ||
| ...(sendPii && parsed.href !== undefined ? { href: serializeHref(parsed.href) } : undefined), | ||
| ...(sendPii && parsed.params ? { params: parsed.params } : undefined), | ||
| }, | ||
| }); | ||
|
|
||
| setPendingExpoRouterNavigation({ | ||
| method, | ||
| href: parsed.href, | ||
| pathname: parsed.pathname, | ||
| params: parsed.params, | ||
| }); | ||
|
|
||
| const span = startInactiveSpan({ | ||
| op: `navigation.${method}`, | ||
| name: `Navigation ${method}${parsed.pathname ? ` to ${parsed.pathname}` : ''}`, | ||
| attributes: { | ||
| 'sentry.origin': SPAN_ORIGIN_AUTO_EXPO_ROUTER_NAVIGATION, | ||
| 'navigation.method': method, | ||
| ...(parsed.routeName ? { 'route.name': parsed.routeName } : undefined), | ||
| ...(sendPii && parsed.href !== undefined ? { 'route.href': serializeHref(parsed.href) } : undefined), | ||
|
Check warning on line 164 in packages/core/src/js/tracing/expoRouter.ts
|
||
|
Comment on lines
+145
to
+164
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. JSON.stringify in serializeHref can throw and abort navigation before the original method is called When Evidence
Identified by Warden find-bugs ยท 9U9-FWQ |
||
| }, | ||
| }); | ||
|
|
||
| try { | ||
| const result = original.apply(router, args); | ||
| span?.setStatus({ code: SPAN_STATUS_OK }); | ||
| span?.end(); | ||
| return result; | ||
| } catch (error) { | ||
| span?.setStatus({ code: SPAN_STATUS_ERROR, message: String(error) }); | ||
| span?.end(); | ||
|
Check warning on line 175 in packages/core/src/js/tracing/expoRouter.ts
|
||
|
Comment on lines
+150
to
+175
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Stale pending navigation set before If Evidence
Also found at 2 additional locations
Identified by Warden code-review ยท X3Y-GQX |
||
| throw error; | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| function parseMethodArgs(method: NavigationMethod, args: unknown[]): ParsedHref { | ||
| if (method === 'back' || method === 'dismiss') { | ||
| return { routeName: method }; | ||
| } | ||
| return parseHref(args[0] as ExpoRouterHref | undefined); | ||
| } | ||
|
|
||
| function parseHref(href: ExpoRouterHref | undefined): ParsedHref { | ||
| if (typeof href === 'string') { | ||
| return { href, routeName: href, pathname: href }; | ||
| } | ||
| if (href && typeof href === 'object') { | ||
| const pathname = typeof href.pathname === 'string' ? href.pathname : undefined; | ||
| return { | ||
| href, | ||
| routeName: pathname ?? 'unknown', | ||
| pathname, | ||
| params: href.params, | ||
| }; | ||
| } | ||
| return { routeName: 'unknown' }; | ||
| } | ||
|
|
||
| function serializeHref(href: unknown): string { | ||
| return typeof href === 'string' ? href : JSON.stringify(href); | ||
| } | ||
|
|
||
| function isSendDefaultPiiEnabled(): boolean { | ||
| return getClient()?.getOptions()?.sendDefaultPii ?? false; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| /** | ||
| * Cross-module hand-off between {@link wrapExpoRouter} and the | ||
| * {@link reactNavigationIntegration} idle navigation span. | ||
| * | ||
| * When an Expo Router method (push / replace / navigate / back / dismiss) is | ||
| * called, it stores the initiating method here. The next idle navigation span | ||
| * consumes (and clears) this value so the span can be attributed to the call | ||
| * site via the `navigation.method` attribute. | ||
| */ | ||
|
|
||
| export interface PendingExpoRouterNavigation { | ||
| /** The Expo Router method that initiated the navigation. */ | ||
| method: 'push' | 'replace' | 'navigate' | 'back' | 'dismiss'; | ||
| /** The target href (string or object), if any. */ | ||
| href?: unknown; | ||
| /** Parsed pathname from the href, if any. */ | ||
| pathname?: string; | ||
| /** Parsed params from the href, if any. */ | ||
| params?: Record<string, unknown>; | ||
| } | ||
|
|
||
| let pending: PendingExpoRouterNavigation | undefined; | ||
|
|
||
| /** Stores the initiating Expo Router navigation call. Overwrites any previous pending value. */ | ||
| export function setPendingExpoRouterNavigation(value: PendingExpoRouterNavigation): void { | ||
| pending = value; | ||
| } | ||
|
|
||
| /** Returns and clears the pending Expo Router navigation, if any. */ | ||
| export function consumePendingExpoRouterNavigation(): PendingExpoRouterNavigation | undefined { | ||
| const value = pending; | ||
| pending = undefined; | ||
| return value; | ||
| } | ||
|
|
||
| /** Test helper โ clears the pending value without consuming it. */ | ||
| export function clearPendingExpoRouterNavigation(): void { | ||
| pending = undefined; | ||
| } |
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.
Navigation pathname leaks PII to Sentry regardless of
sendDefaultPiisettingIn
wrapNavigationMethod,parsed.pathnameis unconditionally included in breadcrumb messages, breadcrumbdata.pathname, span names, androute.nameattributes โ but for string hrefs (e.g.router.push('/users/42')),parseHrefsetspathname === href, so the full resolved path is always sent to Sentry even whensendDefaultPiiisfalse, defeating the guard that protectshrefandparams.Evidence
parseHref(href: string)returns{ href, routeName: href, pathname: href }โ all three fields hold the identical value (e.g./users/42).wrapNavigationMethodguardsparsed.hrefandparsed.paramsbehindisSendDefaultPiiEnabled(), butparsed.pathname(same value for string hrefs) is written to the breadcrumb message (to ${parsed.pathname}),data.pathname, span name, androute.namewith no guard at all./users/42as PII and states it should be gated, yetpathnameโ which equals that value โ bypasses the gate entirely.wrapPrefetchhas the same issue:route.nameis always set torouteNamewhich equals the full string href.Identified by Warden security-review ยท VL9-XWJ