Skip to content

fix: [M3-7078] codeQL warning with analytics.ts#9700

Merged
abailly-akamai merged 3 commits intolinode:developfrom
abailly-akamai:fix-M3-7078
Sep 22, 2023
Merged

fix: [M3-7078] codeQL warning with analytics.ts#9700
abailly-akamai merged 3 commits intolinode:developfrom
abailly-akamai:fix-M3-7078

Conversation

@abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Sep 19, 2023

Description 📝

This pr fixes this codeQL warning and type the _satellite object a bit better

Major Changes 🔄

  • Escape expression in eventPayload replaced values
  • Ensure all occurrences of | are replaced (as opposed to one before)

How to test 🧪

Setup:

  1. Pull code locally
  2. Follow the local analytics debugging setup

Steps:

  1. Navigate to /linodes
  2. Open any action menu and observe the analytic event in the console
  3. Modify the sendLinodeActionEvent method to include one or several | (pipe) characters
  4. Confirm they all get removed from the event payload

@abailly-akamai abailly-akamai self-assigned this Sep 19, 2023
@abailly-akamai abailly-akamai marked this pull request as ready for review September 19, 2023 20:41
@abailly-akamai abailly-akamai requested a review from a team as a code owner September 19, 2023 20:41
@abailly-akamai abailly-akamai requested review from dwiley-akamai and tyler-akamai and removed request for a team September 19, 2023 20:41
Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do extra spaces in the category name impact the integrity of the analytics? Two examples below:

Example 1

Change in code:
Screenshot 2023-09-19 at 5 13 39 PM

Logged to console:
Screenshot 2023-09-19 at 5 14 01 PM

Example 2

Change in code:
Screenshot 2023-09-19 at 5 14 52 PM

Logged to console:
Screenshot 2023-09-19 at 5 15 03 PM

Would those end up getting classified as different categories?

@abailly-akamai
Copy link
Contributor Author

@dwiley-akamai I don't think so and couldn't find a reference to extra in the docs and elsewhere.

It should be noted that the pipe delimiter in custom events does is already a bit of an edge case we document (we are the ones defining those).

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the improvements!

Yeah. | is an edge case of our own because of the way that our Analytics team is reading and delimiting these custom event attributes in Adobe, and so while we should avoid introducing pipes into our custom events in the first place, it's great to fix the warning and correctly prevent developer error.

Comment on lines +30 to +32
action: eventPayload.action.replace(/\|/g, ''),
category: eventPayload.category.replace(/\|/g, ''),
label: eventPayload.label?.replace(/\|/g, ''),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 😅

Comment on lines +10 to +12
type DTMSatellite = {
track: (eventName: string, eventPayload: AnalyticsEvent) => void;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed that the .replace works! ✅

Nice improvement to the types!

@jaalah-akamai jaalah-akamai added the Approved Multiple approvals and ready to merge! label Sep 22, 2023
@abailly-akamai abailly-akamai merged commit 37f08e0 into linode:develop Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Approved Multiple approvals and ready to merge!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants