Address seer reviews

Author: onurtemizkan

packages/remix/src/client/remixRouteParameterization.ts

@@ -82,10 +82,9 @@ function getManifest(): RouteManifest | null {
  };
 
  try {
- // The manifest is double-stringified for XSS prevention in the Vite plugin,
- // so we need to parse twice: once to get the JSON string, then again to get the object
- const manifestJsonString = JSON.parse(currentManifestString);
- manifest = JSON.parse(manifestJsonString);
+ // The manifest string is JSON-stringified in the Vite plugin for safe injection into JavaScript.
+ // We parse once to convert the JSON string back to an object.
+ manifest = JSON.parse(currentManifestString);
  if (!Array.isArray(manifest.staticRoutes) || !Array.isArray(manifest.dynamicRoutes)) {
  return null;
  }

packages/remix/src/config/createRemixRouteManifest.ts

@@ -36,12 +36,14 @@ function isRouteFile(filename: string): boolean {
  * - users/$id.tsx (nested folder) -> /users/:id
  * - users/$id/posts.tsx (nested folder) -> /users/:id/posts
  * - users/index.tsx (nested folder) -> /users
+ * - _layout.tsx -> null (pathless layout route, not URL-addressable)
+ * - _auth.tsx -> null (pathless layout route, not URL-addressable)
  *
  * @param filename - The route filename or path (can include directory separators for nested routes)
- * @returns Object containing the parameterized path and whether it's dynamic
+ * @returns Object containing the parameterized path and whether it's dynamic, or null for pathless layout routes
  * @internal Exported for testing purposes
  */
-export function convertRemixRouteToPath(filename: string): { path: string; isDynamic: boolean } {
+export function convertRemixRouteToPath(filename: string): { path: string; isDynamic: boolean } | null {
  // Remove file extension
  const basename = filename.replace(/\.(tsx?|jsx?)$/, '');
 
@@ -85,6 +87,12 @@ export function convertRemixRouteToPath(filename: string): { path: string; isDyn
  }
  }
 
+ // If all segments were skipped (pathless layout route like _layout.tsx, _auth.tsx),
+ // return null to indicate this file should not be added to the route manifest
+ if (pathSegments.length === 0) {
+ return null;
+ }
+
  const path = `/${pathSegments.join('/')}`;
  return { path, isDynamic };
 }
@@ -151,7 +159,14 @@ function scanRoutesDirectory(
  staticRoutes.push(...nested.staticRoutes);
  } else if (stat.isFile() && isRouteFile(entry)) {
  const routeName = prefix ? `${prefix}/${entry}` : entry;
- const { path: routePath, isDynamic } = convertRemixRouteToPath(routeName);
+ const result = convertRemixRouteToPath(routeName);
+
+ // Skip pathless layout routes (e.g., _layout.tsx, _auth.tsx)
+ if (result === null) {
+ continue;
+ }
+
+ const { path: routePath, isDynamic } = result;
 
  if (isDynamic) {
  const { regex, paramNames } = buildRegexForDynamicRoute(routePath);

packages/remix/src/config/vite.ts

@@ -2,6 +2,16 @@ import * as path from 'path';
 import type { Plugin } from 'vite';
 import { createRemixRouteManifest } from './createRemixRouteManifest';
 
+/**
+ * Escapes a JSON string for safe embedding in HTML script tags.
+ * JSON.stringify alone doesn't escape </script> or <!-- which can break out of script context.
+ */
+function escapeJsonForHtml(jsonString: string): string {
+ return jsonString
+ .replace(/<\//g, '<\\/') // Escape </ to prevent </script> injection
+ .replace(/<!--/g, '<\\!--'); // Escape <!-- to prevent HTML comment injection
+}
+
 export type SentryRemixVitePluginOptions = {
  /**
  * Path to the app directory (where routes folder is located).
@@ -87,11 +97,11 @@ export function sentryRemixVitePlugin(options: SentryRemixVitePluginOptions = {}
  }
 
  /**
- * XSS Prevention: Double-stringify strategy prevents injection from malicious route filenames.
- * routeManifestJson is already stringified once, stringifying again escapes special chars.
- * Client-side code parses once to recover the manifest object.
+ * XSS Prevention: JSON.stringify escapes quotes/backslashes, but we also need to escape
+ * HTML-dangerous sequences like </script> and <!-- that could break out of the script context.
  */
- const script = `<script>window.${MANIFEST_GLOBAL_KEY} = ${JSON.stringify(routeManifestJson)};</script>`;
+ const safeJsonValue = escapeJsonForHtml(JSON.stringify(routeManifestJson));
+ const script = `<script>window.${MANIFEST_GLOBAL_KEY} = ${safeJsonValue};</script>`;
 
  if (/<head>/i.test(html)) {
  return html.replace(/<head>/i, match => `${match}\n ${script}`);
@@ -125,11 +135,12 @@ export function sentryRemixVitePlugin(options: SentryRemixVitePluginOptions = {}
  /(^|\/)server\.[jt]sx?$/.test(id);
 
  if (isClientEntry) {
- // XSS Prevention: Double-stringify strategy (same as transformIndexHtml above)
+ // XSS Prevention: Escape HTML-dangerous sequences in addition to JSON escaping
+ const safeJsonValue = escapeJsonForHtml(JSON.stringify(routeManifestJson));
  const injectedCode = `
 // Sentry Remix Route Manifest - Auto-injected
 if (typeof window !== 'undefined') {
- window.${MANIFEST_GLOBAL_KEY} = window.${MANIFEST_GLOBAL_KEY} || ${JSON.stringify(routeManifestJson)};
+ window.${MANIFEST_GLOBAL_KEY} = window.${MANIFEST_GLOBAL_KEY} || ${safeJsonValue};
 }
 ${code}`;
 
@@ -142,10 +153,12 @@ ${code}`;
  if (isServerEntry) {
  // Inject into server entry for server-side transaction naming
  // Use globalThis for Cloudflare Workers/Hydrogen compatibility
+ // XSS Prevention: Escape HTML-dangerous sequences (important if server renders this)
+ const safeJsonValue = escapeJsonForHtml(JSON.stringify(routeManifestJson));
  const injectedCode = `
 // Sentry Remix Route Manifest - Auto-injected
 if (typeof globalThis !== 'undefined') {
- globalThis.${MANIFEST_GLOBAL_KEY} = globalThis.${MANIFEST_GLOBAL_KEY} || ${JSON.stringify(routeManifestJson)};
+ globalThis.${MANIFEST_GLOBAL_KEY} = globalThis.${MANIFEST_GLOBAL_KEY} || ${safeJsonValue};
 }
 ${code}`;
 

packages/remix/src/utils/utils.ts

@@ -1,7 +1,7 @@
 import type { ActionFunctionArgs, LoaderFunctionArgs, ServerBuild } from '@remix-run/node';
 import type { AgnosticRouteObject } from '@remix-run/router';
 import type { Span, TransactionSource } from '@sentry/core';
-import { debug, GLOBAL_OBJ } from '@sentry/core';
+import { debug } from '@sentry/core';
 import { DEBUG_BUILD } from './debug-build';
 import { getRequestMatch, matchServerRoutes } from './vendor/response';
 
@@ -103,21 +103,6 @@ export function convertRemixRouteIdToPath(routeId: string): string {
  return routePath;
 }
 
-/**
- * Check if the Vite plugin manifest is available.
- * @returns True if the manifest is available, false otherwise.
- */
-function hasManifest(): boolean {
- const globalWithInjectedManifest = GLOBAL_OBJ as typeof GLOBAL_OBJ & {
- _sentryRemixRouteManifest: string | undefined;
- };
-
- return (
- !!globalWithInjectedManifest?._sentryRemixRouteManifest &&
- typeof globalWithInjectedManifest._sentryRemixRouteManifest === 'string'
- );
-}
-
 /**
  * Get transaction name from routes and url
  */
@@ -131,15 +116,10 @@ export function getTransactionName(routes: AgnosticRouteObject[], url: URL): [st
 
  const routeId = match.route.id || 'no-route-id';
 
- // Only use parameterized path if the Vite plugin manifest is available
- // This ensures backward compatibility - without the plugin, we use the old behavior
- if (hasManifest()) {
- const parameterizedPath = convertRemixRouteIdToPath(routeId);
- return [parameterizedPath, 'route'];
- }
-
- // Fallback to old behavior (route ID) when manifest is not available
- return [routeId, 'route'];
+ // Convert route ID to parameterized path (e.g., "routes/users.$id" -> "/users/:id")
+ // This is a pure string transformation that works without the Vite plugin manifest
+ const parameterizedPath = convertRemixRouteIdToPath(routeId);
+ return [parameterizedPath, 'route'];
 }
 
 /**

packages/remix/test/client/remixRouteParameterization.test.ts

@@ -7,14 +7,6 @@ const globalWithInjectedManifest = GLOBAL_OBJ as typeof GLOBAL_OBJ & {
  _sentryRemixRouteManifest: string | undefined;
 };
 
-/**
- * Helper to simulate the Vite plugin's double-stringify behavior for XSS prevention.
- * The Vite plugin stringifies the manifest once, then stringifies again when injecting into JS.
- */
-function doubleStringify(manifest: RouteManifest): string {
- return JSON.stringify(JSON.stringify(manifest));
-}
-
 describe('maybeParameterizeRemixRoute', () => {
  const originalManifest = globalWithInjectedManifest._sentryRemixRouteManifest;
 
@@ -38,7 +30,7 @@ describe('maybeParameterizeRemixRoute', () => {
  staticRoutes: [{ path: '/' }, { path: '/about' }, { path: '/contact' }, { path: '/blog/posts' }],
  dynamicRoutes: [],
  };
- globalWithInjectedManifest._sentryRemixRouteManifest = doubleStringify(manifest);
+ globalWithInjectedManifest._sentryRemixRouteManifest = JSON.stringify(manifest);
 
  expect(maybeParameterizeRemixRoute('/')).toBeUndefined();
  expect(maybeParameterizeRemixRoute('/about')).toBeUndefined();
@@ -69,7 +61,7 @@ describe('maybeParameterizeRemixRoute', () => {
  },
  ],
  };
- globalWithInjectedManifest._sentryRemixRouteManifest = doubleStringify(manifest);
+ globalWithInjectedManifest._sentryRemixRouteManifest = JSON.stringify(manifest);
 
  expect(maybeParameterizeRemixRoute('/users/123')).toBe('/users/:id');
  expect(maybeParameterizeRemixRoute('/users/john-doe')).toBe('/users/:id');
@@ -90,7 +82,7 @@ describe('maybeParameterizeRemixRoute', () => {
  },
  ],
  };
- globalWithInjectedManifest._sentryRemixRouteManifest = doubleStringify(manifest);
+ globalWithInjectedManifest._sentryRemixRouteManifest = JSON.stringify(manifest);
 
  expect(maybeParameterizeRemixRoute('/')).toBeUndefined();
  expect(maybeParameterizeRemixRoute('/about')).toBeUndefined();
@@ -112,7 +104,7 @@ describe('maybeParameterizeRemixRoute', () => {
  },
  ],
  };
- globalWithInjectedManifest._sentryRemixRouteManifest = doubleStringify(manifest);
+ globalWithInjectedManifest._sentryRemixRouteManifest = JSON.stringify(manifest);
 
  expect(maybeParameterizeRemixRoute('/docs/intro')).toBe('/docs/:*');
  expect(maybeParameterizeRemixRoute('/docs/guide/getting-started')).toBe('/docs/:*');
@@ -130,7 +122,7 @@ describe('maybeParameterizeRemixRoute', () => {
  },
  ],
  };
- globalWithInjectedManifest._sentryRemixRouteManifest = doubleStringify(manifest);
+ globalWithInjectedManifest._sentryRemixRouteManifest = JSON.stringify(manifest);
 
  expect(maybeParameterizeRemixRoute('/users/user-with-dashes/settings')).toBe('/users/:id/settings');
  expect(maybeParameterizeRemixRoute('/users/user_with_underscores/settings')).toBe('/users/:id/settings');
@@ -153,7 +145,7 @@ describe('maybeParameterizeRemixRoute', () => {
  },
  ],
  };
- globalWithInjectedManifest._sentryRemixRouteManifest = doubleStringify(manifest);
+ globalWithInjectedManifest._sentryRemixRouteManifest = JSON.stringify(manifest);
 
  // Should prefer more specific route over catch-all
  expect(maybeParameterizeRemixRoute('/users/123')).toBe('/users/:id');
@@ -170,7 +162,7 @@ describe('maybeParameterizeRemixRoute', () => {
  },
  ],
  };
- globalWithInjectedManifest._sentryRemixRouteManifest = doubleStringify(manifest);
+ globalWithInjectedManifest._sentryRemixRouteManifest = JSON.stringify(manifest);
 
  expect(maybeParameterizeRemixRoute('/users/123')).toBeUndefined();
  });
@@ -186,7 +178,7 @@ describe('maybeParameterizeRemixRoute', () => {
  },
  ],
  };
- globalWithInjectedManifest._sentryRemixRouteManifest = doubleStringify(manifest);
+ globalWithInjectedManifest._sentryRemixRouteManifest = JSON.stringify(manifest);
 
  expect(maybeParameterizeRemixRoute('/users/123')).toBeUndefined();
  });
@@ -204,7 +196,7 @@ describe('maybeParameterizeRemixRoute', () => {
  },
  ],
  };
- globalWithInjectedManifest._sentryRemixRouteManifest = doubleStringify(manifest);
+ globalWithInjectedManifest._sentryRemixRouteManifest = JSON.stringify(manifest);
 
  expect(maybeParameterizeRemixRoute('/unknown')).toBeUndefined();
  expect(maybeParameterizeRemixRoute('/posts/123')).toBeUndefined();
@@ -218,7 +210,7 @@ describe('maybeParameterizeRemixRoute', () => {
  staticRoutes: [],
  dynamicRoutes: [],
  };
- globalWithInjectedManifest._sentryRemixRouteManifest = doubleStringify(manifest);
+ globalWithInjectedManifest._sentryRemixRouteManifest = JSON.stringify(manifest);
 
  expect(maybeParameterizeRemixRoute('')).toBeUndefined();
  });
@@ -228,7 +220,7 @@ describe('maybeParameterizeRemixRoute', () => {
  staticRoutes: [{ path: '/' }],
  dynamicRoutes: [],
  };
- globalWithInjectedManifest._sentryRemixRouteManifest = doubleStringify(manifest);
+ globalWithInjectedManifest._sentryRemixRouteManifest = JSON.stringify(manifest);
 
  expect(maybeParameterizeRemixRoute('/')).toBeUndefined();
  });
@@ -244,7 +236,7 @@ describe('maybeParameterizeRemixRoute', () => {
  },
  ],
  };
- globalWithInjectedManifest._sentryRemixRouteManifest = doubleStringify(manifest);
+ globalWithInjectedManifest._sentryRemixRouteManifest = JSON.stringify(manifest);
 
  expect(maybeParameterizeRemixRoute('/api/v1/users/123/posts/456/comments/789')).toBe(
  '/api/v1/users/:id/posts/:postId/comments/:commentId',
@@ -297,7 +289,7 @@ describe('maybeParameterizeRemixRoute', () => {
  },
  ],
  };
- globalWithInjectedManifest._sentryRemixRouteManifest = doubleStringify(manifest);
+ globalWithInjectedManifest._sentryRemixRouteManifest = JSON.stringify(manifest);
 
  if (expectedRoute === undefined) {
  expect(maybeParameterizeRemixRoute(inputRoute)).toBeUndefined();
@@ -324,7 +316,7 @@ describe('maybeParameterizeRemixRoute', () => {
  },
  ],
  };
- globalWithInjectedManifest._sentryRemixRouteManifest = doubleStringify(manifest);
+ globalWithInjectedManifest._sentryRemixRouteManifest = JSON.stringify(manifest);
 
  // Single segment should match the specific route, not the catch-all
  expect(maybeParameterizeRemixRoute('/123')).toBe('/:parameter');
@@ -355,7 +347,7 @@ describe('maybeParameterizeRemixRoute', () => {
  },
  ],
  };
- globalWithInjectedManifest._sentryRemixRouteManifest = doubleStringify(manifest);
+ globalWithInjectedManifest._sentryRemixRouteManifest = JSON.stringify(manifest);
 
  // More specific route with static segments should win
  expect(maybeParameterizeRemixRoute('/api/users/123')).toBe('/api/users/:id');
@@ -380,7 +372,7 @@ describe('maybeParameterizeRemixRoute', () => {
  },
  ],
  };
- globalWithInjectedManifest._sentryRemixRouteManifest = doubleStringify(manifest);
+ globalWithInjectedManifest._sentryRemixRouteManifest = JSON.stringify(manifest);
 
  const result1 = maybeParameterizeRemixRoute('/users/123');
  const result2 = maybeParameterizeRemixRoute('/users/123');
@@ -400,7 +392,7 @@ describe('maybeParameterizeRemixRoute', () => {
  },
  ],
  };
- globalWithInjectedManifest._sentryRemixRouteManifest = doubleStringify(manifest1);
+ globalWithInjectedManifest._sentryRemixRouteManifest = JSON.stringify(manifest1);
 
  expect(maybeParameterizeRemixRoute('/users/123')).toBe('/users/:id');
 
@@ -415,7 +407,7 @@ describe('maybeParameterizeRemixRoute', () => {
  },
  ],
  };
- globalWithInjectedManifest._sentryRemixRouteManifest = doubleStringify(manifest2);
+ globalWithInjectedManifest._sentryRemixRouteManifest = JSON.stringify(manifest2);
 
  expect(maybeParameterizeRemixRoute('/users/123')).toBeUndefined();
  expect(maybeParameterizeRemixRoute('/members/123')).toBe('/members/:id');