Merge pull request #12281 from overleaf/jpa-tweak-event-segmentation-filter

[web] tweak analytics event segmentation filter

GitOrigin-RevId: e00fef0ac74edfd7fbace33bf9289f1c6f905b57

services/web/app/src/Features/Analytics/AnalyticsManager.js

@@ -6,7 +6,7 @@ const Queues = require('../../infrastructure/Queues')
 const crypto = require('crypto')
 const _ = require('lodash')
 const { expressify } = require('../../util/promises')
-const { logger } = require('@overleaf/logger')
+const logger = require('@overleaf/logger')
 const { getAnalyticsIdFromMongoUser } = require('./AnalyticsHelper')
 
 const analyticsEventsQueue = Queues.getQueue('analytics-events')
@@ -132,6 +132,10 @@ function updateEditingSession(userId, projectId, countryCode, segmentation) {
     return
   }
   if (!_isSegmentationValid(segmentation)) {
+    logger.info(
+      { userId, projectId, segmentation },
+      'rejecting analytics editing session due to bad segmentation'
+    )
     return
   }
   Metrics.analyticsQueue.inc({
@@ -165,9 +169,17 @@ function _recordEvent(
   { delay } = {}
 ) {
   if (!_isAttributeValid(event)) {
+    logger.info(
+      { analyticsId, event, segmentation },
+      'rejecting analytics event due to bad event name'
+    )
     return
   }
   if (!_isSegmentationValid(segmentation)) {
+    logger.info(
+      { analyticsId, event, segmentation },
+      'rejecting analytics event due to bad segmentation'
+    )
     return
   }
   Metrics.analyticsQueue.inc({ status: 'adding', event_type: 'event' })
@@ -193,10 +205,18 @@ function _recordEvent(
 }
 
 function _setUserProperty({ analyticsId, propertyName, propertyValue }) {
-  if (
-    !_isAttributeValid(propertyName) ||
-    !_isAttributeValueValid(propertyValue)
-  ) {
+  if (!_isAttributeValid(propertyName)) {
+    logger.info(
+      { analyticsId, propertyName, propertyValue },
+      'rejecting analytics user property due to bad name'
+    )
+    return
+  }
+  if (!_isAttributeValueValid(propertyValue)) {
+    logger.info(
+      { analyticsId, propertyName, propertyValue },
+      'rejecting analytics user property due to bad value'
+    )
     return
   }
   Metrics.analyticsQueue.inc({
@@ -254,8 +274,8 @@ function _isAttributeValueValid(attributeValue) {
 }
 
 function _isSegmentationValueValid(attributeValue) {
-  // spaces are allowed for segmentation values
-  return !attributeValue || /^[a-zA-Z0-9-_.:;,/ ]+$/.test(attributeValue)
+  // spaces and %-escaped values are allowed for segmentation values
+  return !attributeValue || /^[a-zA-Z0-9-_.:;,/ %]+$/.test(attributeValue)
 }
 
 function _isSegmentationValid(segmentation) {

services/web/test/acceptance/src/helpers/InitApp.js

@@ -2,14 +2,14 @@ const App = require('../../../../app.js')
 const QueueWorkers = require('../../../../app/src/infrastructure/QueueWorkers')
 const MongoHelper = require('./MongoHelper')
 const RedisHelper = require('./RedisHelper')
-const { logger } = require('@overleaf/logger')
+const logger = require('@overleaf/logger')
 const Settings = require('@overleaf/settings')
 const MockReCAPTCHAApi = require('../mocks/MockReCaptchaApi')
 const {
   gracefulShutdown,
 } = require('../../../../app/src/infrastructure/GracefulShutdown')
 
-logger.level('error')
+logger.logger.level('error')
 
 MongoHelper.initialize()
 RedisHelper.initialize()

services/web/test/unit/src/Analytics/AnalyticsManagerTests.js

@@ -114,6 +114,7 @@ describe('AnalyticsManager', function () {
         'fr',
         { key: '<alert>' }
       )
+      sinon.assert.called(this.logger.info)
       sinon.assert.notCalled(this.analyticsEditingSessionQueue.add)
     })
 
@@ -122,6 +123,7 @@ describe('AnalyticsManager', function () {
         this.fakeUserId,
         'not an event!'
       )
+      sinon.assert.called(this.logger.info)
       sinon.assert.notCalled(this.analyticsEventsQueue.add)
     })
 
@@ -131,6 +133,7 @@ describe('AnalyticsManager', function () {
         'an_event',
         { not_a: 'Valid Segmentation!' }
       )
+      sinon.assert.called(this.logger.info)
       sinon.assert.notCalled(this.analyticsEventsQueue.add)
     })
 
@@ -140,6 +143,7 @@ describe('AnalyticsManager', function () {
         'an invalid property',
         'a_value'
       )
+      sinon.assert.called(this.logger.info)
       sinon.assert.notCalled(this.analyticsUserPropertiesQueue.add)
     })
 
@@ -149,6 +153,7 @@ describe('AnalyticsManager', function () {
         'a_property',
         'an invalid value'
       )
+      sinon.assert.called(this.logger.info)
       sinon.assert.notCalled(this.analyticsUserPropertiesQueue.add)
     })
   })
@@ -157,6 +162,7 @@ describe('AnalyticsManager', function () {
     it('identifyUser', function () {
       const analyticsId = 'bd101c4c-722f-4204-9e2d-8303e5d9c120'
       this.AnalyticsManager.identifyUser(this.fakeUserId, analyticsId, true)
+      sinon.assert.notCalled(this.logger.info)
       sinon.assert.calledWithMatch(
         this.Queues.createScheduledJob,
         'analytics-events',
@@ -180,6 +186,7 @@ describe('AnalyticsManager', function () {
         event,
         null
       )
+      sinon.assert.notCalled(this.logger.info)
       sinon.assert.calledWithMatch(this.analyticsEventsQueue.add, 'event', {
         analyticsId: this.analyticsId,
         event,
@@ -198,6 +205,7 @@ describe('AnalyticsManager', function () {
         countryCode,
         segmentation
       )
+      sinon.assert.notCalled(this.logger.info)
       sinon.assert.calledWithMatch(
         this.analyticsEditingSessionQueue.add,
         'editing-session',
@@ -217,6 +225,7 @@ describe('AnalyticsManager', function () {
         'an_event',
         { compileTime: timings?.compileE2E }
       )
+      sinon.assert.notCalled(this.logger.info)
       sinon.assert.calledWithMatch(this.analyticsEventsQueue.add, 'event', {
         analyticsId: this.analyticsId,
         event: 'an_event',
@@ -231,6 +240,7 @@ describe('AnalyticsManager', function () {
         'an_event',
         { segment: 'a value with spaces' }
       )
+      sinon.assert.notCalled(this.logger.info)
       sinon.assert.calledWithMatch(this.analyticsEventsQueue.add, 'event', {
         analyticsId: this.analyticsId,
         event: 'an_event',
@@ -239,12 +249,28 @@ describe('AnalyticsManager', function () {
       })
     })
 
+    it('percent sign in event segmentation value', async function () {
+      await this.AnalyticsManager.recordEventForUser(
+        this.fakeUserId,
+        'an_event',
+        { segment: 'a value with escaped comma %2C' }
+      )
+      sinon.assert.notCalled(this.logger.info)
+      sinon.assert.calledWithMatch(this.analyticsEventsQueue.add, 'event', {
+        analyticsId: this.analyticsId,
+        event: 'an_event',
+        segmentation: { segment: 'a value with escaped comma %2C' },
+        isLoggedIn: true,
+      })
+    })
+
     it('boolean field in event segmentation', async function () {
       await this.AnalyticsManager.recordEventForUser(
         this.fakeUserId,
         'an_event',
         { isAutoCompile: false }
       )
+      sinon.assert.notCalled(this.logger.info)
       sinon.assert.calledWithMatch(this.analyticsEventsQueue.add, 'event', {
         analyticsId: this.analyticsId,
         event: 'an_event',