feat(tracing): Wrap Expo Router push, replace, navigate, back, dismiss in addition to prefetch#6221
Draft
alwx wants to merge 2 commits into
Draft
feat(tracing): Wrap Expo Router push, replace, navigate, back, dismiss in addition to prefetch#6221alwx wants to merge 2 commits into
push, replace, navigate, back, dismiss in addition to prefetch#6221alwx wants to merge 2 commits into
Conversation
Contributor
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog.
🤖 This preview updates automatically when you update the PR. |
Contributor
|
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
alwx
changed the title
push, replace, navigate, back, dismiss in addition to prefetchpush, replace, navigate, back, dismiss in addition to prefetch
…nd sendDefaultPii
Expo Router `push`/`replace`/`navigate`/`back`/`dismiss` (and `prefetch`)
wrappers previously emitted the raw `href` and route `params` on both
the navigation breadcrumb and span attributes regardless of the client
options. Dynamic segments like `/users/123` and parameter values such
as `{ id: '123' }` can carry user identifiers and other PII, so the
existing `reactnavigation.ts` integration already gates them behind
`sendDefaultPii`. The new Expo Router wrappers should follow the same
contract.
This change:
- Reads `sendDefaultPii` from the client options and only attaches
`href` (raw string or stringified object form) and `params` to the
breadcrumb `data` and span attributes when it is enabled.
- Keeps the non-PII fields — `method`, `pathname` (route template),
and `route.name` — unconditional so navigations remain visible and
groupable in Sentry without leaking user data.
- Applies the same guard to the `prefetch` span's `route.href`
attribute, which was newly added in this PR.
- Adds dedicated `sendDefaultPii` test cases and updates the existing
default-off assertions for all wrapped methods.
- Adds a changelog entry for #6221.
alwx
changed the title
push, replace, navigate, back, dismiss in addition to prefetchpush, replace, navigate, back, dismiss in addition to prefetch
Comment on lines
+60
to
+74
| 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']>; | ||
| } |
Contributor
There was a problem hiding this comment.
Navigation pathname leaks PII to Sentry regardless of sendDefaultPii setting
In wrapNavigationMethod, parsed.pathname is unconditionally included in breadcrumb messages, breadcrumb data.pathname, span names, and route.name attributes — but for string hrefs (e.g. router.push('/users/42')), parseHref sets pathname === href, so the full resolved path is always sent to Sentry even when sendDefaultPii is false, defeating the guard that protects href and params.
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.- The code comment explicitly labels
/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
Comment on lines
+150
to
+175
| 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), | ||
| }, | ||
| }); | ||
|
|
||
| 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(); |
Contributor
There was a problem hiding this comment.
Stale pending navigation set before original.apply() can corrupt next navigation's span attributes
If original.apply(router, args) throws synchronously (e.g. invalid route, navigation not ready), setPendingExpoRouterNavigation has already stored the failed call's data. The catch block does not clear it, so the next successful navigation will have consumePendingExpoRouterNavigation() return this stale value and incorrectly tag that span with the failed method's navigation.method and route info.
Evidence
setPendingExpoRouterNavigationis called unconditionally at line 150, before thetryblock at line 168.original.apply(router, args)at line 169 can throw synchronously (e.g. bad route arg).- The
catchblock at lines 173–177 only records the span error and re-throws; it never callsclearPendingExpoRouterNavigation()or otherwise resets the pending value. - In
reactnavigation.tsline 456,consumePendingExpoRouterNavigation()is called on the next navigation state change, where it would dequeue the stale value and wrongly attribute the new span to the failed navigation. - The error-path test in
expoRouter.test.tsdoes not assert that the pending value is cleared after a throw, confirming this is unguarded.
Also found at 2 additional locations
packages/core/src/js/tracing/expoRouter.ts:59-74packages/core/src/js/tracing/reactnavigation.ts:437-440
Identified by Warden code-review · X3Y-GQX
Comment on lines
+145
to
+164
| ...(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), |
Contributor
There was a problem hiding this comment.
JSON.stringify in serializeHref can throw and abort navigation before the original method is called
When sendDefaultPii is enabled and href.params contains a non-serializable value (e.g. BigInt, Symbol, or a circular reference), serializeHref throws a TypeError from JSON.stringify. Both call sites (inside addBreadcrumb and startInactiveSpan) are executed before the try { original.apply(router, args) } block, so the exception propagates uncaught and the original navigation method is never invoked — silently dropping the navigation.
Evidence
paramsinExpoRouterHrefandParsedHrefis typedRecord<string, unknown>, allowing any value including non-serializable types.serializeHref(line 204) callsJSON.stringify(href)with no try/catch; BigInt/Symbol/circular refs all throwTypeError.- Lines 145 and 164 call
serializeHrefinsideaddBreadcrumbandstartInactiveSpanargument objects, both located before thetryblock at line 170 that wrapsoriginal.apply(router, args). - If either throws, execution exits the wrapper without ever reaching
original.apply, silently killing the navigation. - No existing test covers a non-serializable param value with
sendDefaultPiienabled.
Identified by Warden find-bugs · 9U9-FWQ
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📢 Type of change
📜 Description
Extends the Expo Router instrumentation beyond
prefetchso that programmatic navigation calls produce breadcrumbs and spans, and the resulting idle navigation transaction can be attributed back to the call that triggered it.What changed:
wrapExpoRouternow wrapspush,replace,navigate,back, anddismissin addition toprefetch.navigationbreadcrumb (category: 'navigation',data.method,data.pathname),navigation.<method>span tagged withsentry.origin = auto.navigation.expo_router,navigation.method, androute.name,pendingExpoRouterNavigation) that the next React Navigation idle navigation span consumes to set its ownnavigation.methodattribute, attributing the transaction back to the originating call.__sentryWrappedflag guards against double-wrapping ifwrapExpoRouteris called multiple times on the same router.reactnavigation.ts: rawhrefand routeparams(which can encode user identifiers like/users/123or{ id: '123' }) are only attached to breadcrumbs and span attributes whensendDefaultPiiis enabled.method,pathname(the route template), androute.nameremain unconditional so navigations stay visible and groupable.💡 Motivation and Context
Fixes #6158.
Previously only
prefetchwas instrumented, so any programmaticrouter.push(...)/router.replace(...)/router.back()etc. was invisible in Sentry — no breadcrumb trail, no span, and the resulting React Navigation idle transaction had no way to know which call site initiated it. This PR closes that gap while keeping the existingprefetchbehavior intact and aligning PII handling with the rest of the navigation tracing.💚 How did you test it?
packages/core/test/tracing/expoRouter.test.tscovering:push/replace/navigateviadescribe.each— span and breadcrumb shape with string and object hrefs, error reporting, and the pending-navigation hand-off,back()anddismiss(count)with no/optional args,href, noparams) and dedicatedsendDefaultPii: truecases that verifyhrefandparamsreappear on both the breadcrumb and the span.yarn jest— 113 suites / 1481 tests passing.yarn lint:lernaclean.yarn api-report:checkup to date.📝 Checklist
sendDefaultPiiis enabled🔮 Next steps