ua-parser-js fork is vulnerable

[kevinyou]

Summary

Please update @amplitude/ua-parser-js, the amplitude fork of us-parser-js, to at least 0.7.33, and update consumers of that library accordingly

Motivations

ua-parser-js versions >=0.7.30 <0.7.33 are vulnerable: CVE-2022-25927 SNYK-JS-UAPARSERJS-3244450

This seems to be similar to https://github.com/amplitude/Amplitude-JavaScript/issues/158 with a new vulnerability

[kevinyou]

PR in https://github.com/amplitude/ua-parser-js/pull/7

[jk-clarabridge]

@liuyang1520 @justin-fiedler sorry to call you out but you've both been active recently 😅

Can you all help with this, or at least get it on the radar of someone who can help?

[yuhao900914]

@jk-clarabridge Thanks for your contribution. Do you mind merging the conflict on that pr?

[kevinyou]

I took the main ua-parser.js file from the original and this amplitude fork: https://github.com/faisalman/ua-parser-js/blob/0.7.x/src/ua-parser.js https://github.com/amplitude/ua-parser-js/blob/master/src/ua-parser.js

And then applied a common prettier formatter for uniformity, and then produced a diff: https://github.com/kevinyou/demo-ua-parser-comparison/blob/main/diff.diff

Most differences are strictly additive improvements - newer versions of ua-parser support more browsers.

This section is the security fix for the CVE

I don't know why amplitude originally decided to fork ua-parser-js but I'm not seeing any reasons with the latest 0.7.x version of ua-parser-js. It might be simpler to just archive the fork and use the latest ua-parser-js.

[jk-clarabridge]

Just to echo what Kevin said, the conflicts are difficult to merge due to the formatter being applied, as there are dozens of changes detected, but the majority are caused by linting/formatting preferences. It doesnt look like there's any compelling reason for this to be its own package. Is it possible to update amplitude to just use the base package instead of this fork?

We could also just apply the fix for the vulnerability that Kevin referenced and skip all of the other updates, but that feels like it's just kicking the can down the road, and next time there's a reason to update from the base repo you'll run into this same issue again.

[kevinyou]

@yuhao900914 I merged the conflicts in a new PR, can you review this? https://github.com/amplitude/ua-parser-js/pull/8

[kevinyou]

@liuyang1520 @justin-fiedler Sorry to call you out again but would like to get this resolved and i have a mergable PR now

[yuhao900914]

@kevinyou , I left a comment on your pr and request the changes. Thanks.

[kevinyou]

@yuhao900914 Could you also publish 0.7.33 to npmjs?

[kevinyou]

@yuhao900914 Could you also update these two dependents from 0.7.31 to something like ^0.7.33?

https://github.com/amplitude/Amplitude-JavaScript/blob/main/package.json#L18

https://github.com/amplitude/experiment-js-client/blob/main/packages/analytics-connector/package.json#L31