search

segment-integrations / analytics.js-integration-intercom

The Intercom analytics.js integration.
https://segment.com/docs/integrations/intercom/
MIT License
7 stars 12 forks source link

Type-check the type of traits before formatting it #9

Closed bbeaird closed 8 years ago

bbeaird commented 8 years ago

Moved from here - https://github.com/segmentio/integration-intercom/issues/12

We've seen a nasty bug where somehow our traits was not a JS object, but some HTML string. This was coming from our API endpoint that at one point—due to a bug—returned some HTML and we passed it through as the traits.

***This repo below is wrong as it's server-side. This led to the following block of code being executed: https://github.com/segmentio/integration-intercom/blob/b522c0c5a4d7bed8c12c340c913402c790e83ce2/lib/index.js#L126-L140

Since traits here isn't type-checked, the string was iterated over as a list of chars, leading to the "formatted" result objects that looked like this:

{
  "0": "<",
  "1": "h",
  "2": "t",
  "3": "m",
  "4": "l",
  "5": ">",
  ...
}

Since this is a perfectly valid object, Intercom happily accepted it. The resulting object had almost 12K keys like this, and it crippled our Intercom dashboard (all users now has to load over 12,000 properties).

This could have been prevented by adding a type check to the input param here to be an object. I'm not sure if this is a more generic problem, or only exists in the Intercom integration.

f2prateek commented 8 years ago

This likely affects other integrations, client side and server side (but not all).

This should be fixed in facade which is the right place to do validations like this.

The other obvious solution is to not send bad data like this - that's why we have a spec documented with what we expect certain properties to be.

hankim813 commented 8 years ago

So the proposal for the validation in the segmentio/facade is to just check if the traits passed inside .identify() call is an object? I've added this to our list of updates on the platform team so will close this since this is not necessarily an issue regarding Intercom itself.