[web] fix open redirect via utm tags (#36019)

GitOrigin-RevId: 1304c1578adf1d3248abcaa01902467422e8d8f6

services/web/app/src/Features/Analytics/AnalyticsUTMTrackingMiddleware.mjs

@@ -1,18 +1,16 @@
-import _ from 'lodash'
 import RequestHelper from './RequestHelper.mjs'
 import AnalyticsManager from './AnalyticsManager.mjs'
-import querystring from 'node:querystring'
 import { URL } from 'node:url'
 import Settings from '@overleaf/settings'
-import OError from '@overleaf/o-error'
 import logger from '@overleaf/logger'
+import UrlHelper from '../Helpers/UrlHelper.mjs'
 
 function recordUTMTags() {
   return function (req, res, next) {
-    const query = req.query
+    const rawQuery = req.query
 
     try {
-      const utmValues = RequestHelper.parseUtm(query)
+      const utmValues = RequestHelper.parseUtm(rawQuery)
 
       if (utmValues) {
         const path = new URL(req.url, Settings.siteUrl).pathname
@@ -36,21 +34,32 @@ function recordUTMTags() {
           'utm-tags',
           propertyValue
         )
-
-        // redirect to URL without UTM query params
-        const queryWithoutUtm = _.omit(query, RequestHelper.UTM_KEYS)
-        const queryString =
-          Object.keys(queryWithoutUtm).length > 0
-            ? '?' + querystring.stringify(queryWithoutUtm)
-            : ''
-        return res.redirect(path + queryString)
       }
     } catch (error) {
       // log errors and fail silently
-      OError.tag(error, 'failed to track UTM tags', {
+      logger.warn({ error, rawQuery, req }, 'failed to track UTM tags')
-        query,
+    }
-      })
+
-      logger.warn({ error }, error.message)
+    try {
+      const strippedQuery = RequestHelper.stripUTMKeys(rawQuery)
+      const allFields = Object.keys(rawQuery).length
+      const withoutUTM = Object.keys(strippedQuery).length
+      if (allFields !== withoutUTM) {
+        let redirect =
+          UrlHelper.getSafeRedirectPath(
+            // Drop the query from URL, we will add our own below.
+            new URL(req.url, Settings.siteUrl).pathname
+          ) || '/'
+        if (withoutUTM > 0) redirect += `?${new URLSearchParams(strippedQuery)}`
+        res.redirect(redirect)
+        return
+      }
+    } catch (err) {
+      // log errors and fail silently
+      logger.warn(
+        { err, rawQuery, req },
+        'failed to remove UTM tags from query'
+      )
     }
 
     next()

services/web/app/src/Features/Analytics/RequestHelper.mjs

@@ -52,9 +52,16 @@ function parseReferrer(referrer, url) {
 
 const REGISTRATION_UTM_KEYS = UTM_KEYS.filter(k => k !== 'utm_id')
 
+function stripUTMKeys(query) {
+  return Object.fromEntries(
+    Object.entries(query).filter(([key]) => !UTM_KEYS.includes(key))
+  )
+}
+
 export default {
   UTM_KEYS,
   REGISTRATION_UTM_KEYS,
   parseUtm,
   parseReferrer,
+  stripUTMKeys,
 }

services/web/test/acceptance/src/AnalyticsUTMTrackingMiddlewareTests.mjs

@@ -0,0 +1,62 @@
+import { expect } from 'chai'
+import http from 'node:http'
+import { fetchRedirect } from '@overleaf/fetch-utils'
+
+const HTTP_TEST_HOST = process.env.HTTP_TEST_HOST || '127.0.0.1'
+const HTTP_TEST_PORT = 23000
+const BASE_URL = `http://${HTTP_TEST_HOST}:${HTTP_TEST_PORT}`
+
+// Sends a request with the given raw path, bypassing the WHATWG URL parsing
+// that fetch()/URL would otherwise use to resolve dot-segments client-side
+// before the request ever reaches the wire.
+function rawRequest(rawPath) {
+  return new Promise((resolve, reject) => {
+    const req = http.request(
+      {
+        hostname: HTTP_TEST_HOST,
+        port: HTTP_TEST_PORT,
+        path: rawPath,
+        method: 'GET',
+      },
+      res => {
+        res.resume()
+        res.on('end', () => resolve(res))
+      }
+    )
+    req.on('error', reject)
+    req.end()
+  })
+}
+
+describe('AnalyticsUTMTrackingMiddleware', function () {
+  it('redirects a normal request to the path without the utm query params', async function () {
+    const location = await fetchRedirect(`${BASE_URL}/project?utm_source=foo`)
+    expect(location).to.equal(`${BASE_URL}/project`)
+  })
+
+  it('redirects a normal request with other params without the utm query params', async function () {
+    const location = await fetchRedirect(
+      `${BASE_URL}/project?utm_source=foo&other=bar`
+    )
+    expect(location).to.equal(`${BASE_URL}/project?other=bar`)
+  })
+
+  it('redirects to / with other params without the utm query params', async function () {
+    const location = await fetchRedirect(
+      `${BASE_URL}/?utm_source=foo&other=bar`
+    )
+    expect(location).to.equal(`${BASE_URL}/?other=bar`)
+  })
+
+  it('should block open redirect with a raw path with a plain dot-segment', async function () {
+    const res = await rawRequest('/.//evil.com/x?utm_source=foo')
+    expect(res.statusCode).to.be.within(300, 399)
+    expect(res.headers.location).to.equal('/x')
+  })
+
+  it('should block open redirect with a raw path with a percent-encoded dot-segment', async function () {
+    const res = await rawRequest('/%2e//evil.com/x?utm_source=foo')
+    expect(res.statusCode).to.be.within(300, 399)
+    expect(res.headers.location).to.equal('/x')
+  })
+})