Fix open redirect via protocol-relative URLs (#60215)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: heiskr

eslint.config.ts

@@ -102,6 +102,16 @@ export default [
 
       // Custom rules (disabled by default for now)
       'custom-rules/use-custom-logger': 'off',
+
+      // Prevent direct res.redirect() usage — use res.safeRedirect() instead
+      // to avoid open redirect vulnerabilities via protocol-relative URLs.
+      'no-restricted-syntax': [
+        'error',
+        {
+          selector: "CallExpression[callee.object.name='res'][callee.property.name='redirect']",
+          message: 'Use res.safeRedirect() instead of res.redirect() to prevent open redirects.',
+        },
+      ],
     },
   },
 

src/archives/middleware/archived-asset-redirects.ts

@@ -29,7 +29,7 @@ export default function archivedAssetRedirects(
 ) {
   if (req.path in REDIRECTS) {
     const redirect = REDIRECTS[req.path].replace('/assets/', '/assets/cb-0000/')
-    return res.redirect(308, redirect)
+    return res.safeRedirect(308, redirect)
   }
 
   return next()

src/archives/middleware/archived-enterprise-versions.ts

@@ -117,7 +117,7 @@ export default async function archivedEnterpriseVersions(
         languageCacheControl(res) // call first to get `vary`
       }
       archivedCacheControl(res) // call second to extend duration
-      return res.redirect(redirectCode, redirectTo)
+      return res.safeRedirect(redirectCode, redirectTo)
     }
 
     const redirectJson = (await getRemoteJSON(getProxyPath('redirects.json', requestedVersion), {
@@ -137,7 +137,7 @@ export default async function archivedEnterpriseVersions(
         languageCacheControl(res) // call first to get `vary`
       }
       archivedCacheControl(res) // call second to extend duration
-      return res.redirect(redirectCode, `/${language}${newRedirectTo}`)
+      return res.safeRedirect(redirectCode, `/${language}${newRedirectTo}`)
     }
   }
   // For releases 2.13 and lower, redirect language-prefixed URLs like /en/enterprise/2.10 -> /enterprise/2.10
@@ -146,7 +146,7 @@ export default async function archivedEnterpriseVersions(
     versionSatisfiesRange(requestedVersion, `<${firstVersionDeprecatedOnNewSite}`)
   ) {
     archivedCacheControl(res)
-    return res.redirect(redirectCode, req.baseUrl + req.path.replace(/^\/en/, ''))
+    return res.safeRedirect(redirectCode, req.baseUrl + req.path.replace(/^\/en/, ''))
   }
 
   // Redirects for releases 2.13 - 2.17
@@ -170,7 +170,7 @@ export default async function archivedEnterpriseVersions(
       // new destination.
       const redirect = `/${language || 'en'}${newPath || withoutLanguagePath}`
       cacheAggressively(res)
-      return res.redirect(redirectCode, redirect)
+      return res.safeRedirect(redirectCode, redirect)
     }
   }
   // Redirects for 2.18 - 3.0. Starting with 2.18, we updated the archival
@@ -193,7 +193,7 @@ export default async function archivedEnterpriseVersions(
     if (redirectJson[req.path]) {
       res.set('x-robots-tag', 'noindex')
       cacheAggressively(res)
-      return res.redirect(redirectCode, redirectJson[req.path])
+      return res.safeRedirect(redirectCode, redirectJson[req.path])
     }
   }
   // Retrieve the page from the archived repo
@@ -260,7 +260,7 @@ export default async function archivedEnterpriseVersions(
     const staticRedirect = body.match(patterns.staticRedirect)
     if (staticRedirect) {
       cacheAggressively(res)
-      return res.redirect(redirectCode, staticRedirect[1])
+      return res.safeRedirect(redirectCode, staticRedirect[1])
     }
 
     res.set('content-type', r.headers.get('content-type') || '')
@@ -373,7 +373,7 @@ export default async function archivedEnterpriseVersions(
       statsTags.push(`fallback:${fallbackRedirect}`)
       statsd.increment('middleware.trying_fallback_redirect_success', 1, statsTags)
       cacheAggressively(res)
-      return res.redirect(redirectCode, fallbackRedirect)
+      return res.safeRedirect(redirectCode, fallbackRedirect)
     }
     statsd.increment('middleware.trying_fallback_redirect_failure', 1, statsTags)
   }

src/article-api/middleware/pagelist.ts

@@ -92,7 +92,7 @@ router.get(
   '/',
   pagelistValidationMiddleware as RequestHandler,
   catchMiddlewareError(async function (req: ExtendedRequest, res: Response) {
-    res.redirect(
+    res.safeRedirect(
       308,
       req.originalUrl.replace(
         '/pagelist',
@@ -108,7 +108,7 @@ router.get(
   pagelistValidationMiddleware as RequestHandler,
   catchMiddlewareError(async function (req: ExtendedRequest, res: Response) {
     const { someParam } = req.params
-    res.redirect(
+    res.safeRedirect(
       308,
       req.originalUrl.replace(
         `/pagelist/${someParam}`,

src/assets/middleware/asset-preprocessing.ts

@@ -30,7 +30,7 @@ export default function assetPreprocessing(
       // By forcing this to be a redirect, it means we only serve
       // 1 single file. All other requests will be redirects.
       // Otherwise someone might trigger too much bypassing of the CDN.
-      return res.redirect(req.url.toLowerCase())
+      return res.safeRedirect(req.url.toLowerCase())
     }
 
     // We're only confident enough to set the *manual* surrogate key if the

src/assets/middleware/dynamic-assets.ts

@@ -76,7 +76,7 @@ export default async function dynamicAssets(
     //    < 302
     //    < location: /assets/images/site/logo.web
     //
-    return res.redirect(302, req.path)
+    return res.safeRedirect(302, req.path)
   }
 
   // From PNG to WEBP, if the PNG exists

src/frame/middleware/index.ts

@@ -66,6 +66,7 @@ import mockVaPortal from './mock-va-portal'
 import dynamicAssets from '@/assets/middleware/dynamic-assets'
 import generalSearchMiddleware from '@/search/middleware/general-search-middleware'
 import shielding from '@/shielding/middleware'
+import safeRedirect from './safe-redirect'
 import { MAX_REQUEST_TIMEOUT } from '@/frame/lib/constants'
 import { initLoggerContext } from '@/observability/logger/lib/logger-context'
 import { getAutomaticRequestLogger } from '@/observability/logger/middleware/get-automatic-request-logger'
@@ -125,6 +126,10 @@ export default function index(app: Express) {
   // for static assets as well.
   app.use(setDefaultFastlySurrogateKey)
 
+  // Attaches res.safeRedirect() to every response. Must appear before
+  // any middleware that redirects.
+  app.use(safeRedirect)
+
   // archivedEnterpriseVersionsAssets must come before static/assets
   app.use(asyncMiddleware(archivedEnterpriseVersionsAssets))
 

src/frame/middleware/manifest-json.ts

@@ -32,7 +32,7 @@ export default async function manifestJson(req: Request, res: Response, next: Ne
   if (req.url !== '/manifest.json') {
     // E.g. `/manifest.json/anything` or `/manifest.json?foo=bar`
     defaultCacheControl(res)
-    return res.redirect(302, '/manifest.json')
+    return res.safeRedirect(302, '/manifest.json')
   }
 
   const icons: Icon[] = []

src/frame/middleware/safe-redirect.ts

@@ -0,0 +1,29 @@
+import type { Response, NextFunction } from 'express'
+
+import type { ExtendedRequest } from '@/types'
+
+// Normalizes a redirect URL to prevent open redirects via protocol-relative
+// URLs (e.g. "//evil.com" which browsers interpret as "https://evil.com").
+export function safeRedirectUrl(url: string): string {
+  return url.replace(/^\/\/+/, '/')
+}
+
+// Matches the overloaded signature of Express's res.redirect().
+export type SafeRedirect = {
+  (url: string): void
+  (status: number, url: string): void
+}
+
+// Attaches res.safeRedirect() to the response for all downstream middleware.
+// Same signature as res.redirect() but normalizes the URL first.
+export default function safeRedirect(req: ExtendedRequest, res: Response, next: NextFunction) {
+  res.safeRedirect = function (statusOrUrl: number | string, url?: string) {
+    if (typeof statusOrUrl === 'number' && url !== undefined) {
+      // eslint-disable-next-line no-restricted-syntax
+      return res.redirect(statusOrUrl, safeRedirectUrl(url))
+    }
+    // eslint-disable-next-line no-restricted-syntax
+    return res.redirect(safeRedirectUrl(statusOrUrl as string))
+  }
+  return next()
+}

src/frame/middleware/trailing-slashes.ts

@@ -17,7 +17,7 @@ export default function trailingSlashes(req: ExtendedRequest, res: Response, nex
       }
       url = url.replace(/\/+/g, '/') // Prevent multiple slashes
       defaultCacheControl(res)
-      return res.redirect(301, url)
+      return res.safeRedirect(301, url)
     }
   }