Skip to content
This repository has been archived by the owner. It is now read-only.

fix(metrics): drop invalid utm_ params from flow data #4431

Closed
wants to merge 1 commit into from

Conversation

@philbooth
Copy link
Contributor

@philbooth philbooth commented Nov 21, 2016

Fixes #4415, silently dropping utm_* parameters that contain any dodgy-looking characters. Note that this is different treatment than the rest of the flow data, where the whole event is dropped for invalid data. The values for those params are under our control, whereas these aren't.

Can be tested by manually adding utm_ params to the URL. Those containing valid characters should show up in the flow event, like so:

{"event":"flow.signup.begin","flow_id":"698f1e55b51d763f4dc94fdecd8fc79e683b35e2dcbf48a5a942788c79cfc901","flow_time":0,"hostname":"pbooth-22205.local","op":"flowEvent","pid":58285,"time":"2016-11-21T14:18:07.774Z","userAgent":"Firefox-iOS-FxA/5.3 (Firefox)","v":1,"context":"fx_desktop_v3","entrypoint":"menupanel","service":"sync","utm_campaign":"THIS_IS_GOOD_DATA"}

Any that contain invalid characters should not be present in the flow event.

There is also a secondary fix contained herein; we agreed to ditch utm_term entirely because it's completely free-form and we have don't envisage wanting to analyse it in this way.

@vladikoff r?

@@ -28,7 +27,7 @@ const FLOW_BEGIN_EVENT_TYPES = /^flow\.[a-z_-]+\.begin$/;
const FLOW_ID_KEY = config.get('flow_id_key');
const FLOW_ID_EXPIRY = config.get('flow_id_expiry');

const ENTRYPOINT_PATTERN = /^[\w\.-]+$/;
const ENTRYPOINT_PATTERN = /^[\w.-]+$/;

This comment has been minimized.

@philbooth

philbooth Nov 21, 2016
Author Contributor

The period doesn't need escaping here because it's inside a character class. Updated to be consistent with the new regex below.

@philbooth
Copy link
Contributor Author

@philbooth philbooth commented Nov 23, 2016

Only just realised there's failing tests on this PR, sorry. Closing, will re-open when fixed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants