diff --git a/services/web/app/src/Features/Analytics/AnalyticsUTMTrackingMiddleware.mjs b/services/web/app/src/Features/Analytics/AnalyticsUTMTrackingMiddleware.mjs index bfdc32771c..7b5d6a2b63 100644 --- a/services/web/app/src/Features/Analytics/AnalyticsUTMTrackingMiddleware.mjs +++ b/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', { - query, - }) - logger.warn({ error }, error.message) + logger.warn({ error, rawQuery, req }, 'failed to track UTM tags') + } + + 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() diff --git a/services/web/app/src/Features/Analytics/RequestHelper.mjs b/services/web/app/src/Features/Analytics/RequestHelper.mjs index 11d21ea0a0..ca030721ca 100644 --- a/services/web/app/src/Features/Analytics/RequestHelper.mjs +++ b/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, } diff --git a/services/web/test/acceptance/src/AnalyticsUTMTrackingMiddlewareTests.mjs b/services/web/test/acceptance/src/AnalyticsUTMTrackingMiddlewareTests.mjs new file mode 100644 index 0000000000..b77835bc40 --- /dev/null +++ b/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') + }) +})