Replace ua-parser-js with a reduced in-house version #3720

davidje13 · 2021-10-30T15:00:13Z

As discussed in #3715

This replaces the external library ua-parser-js with an in-house version which has a significantly reduced scope.

The parser has been written from scratch with a focus on passing the existing tests, but should be easy to extend with more browsers and OSes if required (though I would recommend keeping it minimal and maybe even removing some entries). The method used is a bit more robust than the wildcard searches used in the other project, and should be less prone to accidental catastrophic backtracking issues, but the main point of this is to reduce the breadth of dependencies to help avoid upstream attacks in the future.

Currently it is not possible for users to plug in their own function; the recommendation would be for people who need "friendly" names for a wide selection of browsers to use the fullName property instead and do their own parsing (e.g. via ua-parser-js if they wish).

google-cla · 2021-10-30T15:00:19Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

davidje13 · 2021-10-30T15:03:43Z

@googlebot I signed it

jginsburgn · 2021-11-01T21:15:19Z

Nice CL!

jginsburgn · 2021-11-01T21:20:40Z

lib/helper.js

+    }
+  }
+  if (!result.firstEntry) {
+    // ensure firstEntry is always set no matter what (null object pattern)
+    result.firstEntry = {
+      version: null,
+      details: [],
+      hasDetail: () => false,
+      getDetail: () => []
+    }
+  }
+  return result
+}
+
+const extractFromParts = (parts, checks) => checks
+  .map((check) => check(parts))
+  .filter((result) => result)[0] || []
+
+const extractMacVersion = (m) => m[1].replace(/_/g, '.')
+
+const WINDOWS_NT_VERSION_MAP = {
+  5.1: 'XP',
+  5.2: 'XP',
+  '6.0': 'Vista',
+  6.1: '7',
+  6.2: '8',
+  6.3: '8.1',
+  6.4: '10',
+  '10.0': '10'
+}
+const extractWindowsVersion = (m) => WINDOWS_NT_VERSION_MAP[m[1]]
+
+const UA_BROWSERS = [
+  ({ phantomjs }) => phantomjs && // also contains Safari
+    ['PhantomJS', phantomjs.version],
+
+  ({ headlesschrome }) => headlesschrome && // also contains Safari
+    ['Chrome Headless', headlesschrome.version],
+
+  ({ opera, version }) => opera &&
+    ['Opera', version && version.version],
+
+  ({ firefox }) => firefox &&
+    ['Firefox', firefox.version],
+
+  ({ edg }) => edg && // also contains Chrome, Safari
+    ['Edge', edg.version],
+
+  ({ chrome }) => chrome && // also contains Safari
+    ['Chrome', chrome.version],
+
+  ({ firstEntry, version }) => firstEntry.hasDetail(/^iphone/i) && // also contains Safari
+    ['Mobile Safari', version && version.version],
+
+  ({ firstEntry, version }) => firstEntry.hasDetail(/^android/i) && // also contains Safari
+    ['Android Browser', version && version.version],
+
+  ({ safari, version }) => safari &&
+    ['Safari', version && version.version],
+
+  ({ firstEntry }) => firstEntry.hasDetail(/^msie/i) &&
+    ['IE', firstEntry.getDetail(/^msie ([\d.]+)/i)[1]]
+]
+
+const UA_SYSTEMS = [
+  ({ firstEntry }) => firstEntry.hasDetail(/^android/i) &&
+    ['Android', firstEntry.getDetail(/^android ([\d.]+)/i)[1]],
+
+  ({ firstEntry }) => firstEntry.hasDetail(/^iphone/i) &&
+    ['iOS', firstEntry.getDetail(/iphone os ([\d._]+)/i, extractMacVersion)],
+
+  ({ ubuntu }) => ubuntu &&
+    ['Ubuntu', ubuntu.version],
+
+  ({ firstEntry }) => firstEntry.hasDetail(/^freebsd/i) &&
+    ['FreeBSD', null],
+
+  ({ firstEntry }) => firstEntry.hasDetail(/^linux/i) &&
+    ['Linux', firstEntry.getDetail(/^linux (.+)/i)[1]],
+
+  ({ firstEntry }) => firstEntry.hasDetail(/mac os/i) &&
+    ['Mac OS', firstEntry.getDetail(/mac os(?: x)? ([\d._]+)/i, extractMacVersion)],
+
+  ({ firstEntry }) => firstEntry.hasDetail(/^windows/i) &&
+    ['Windows', firstEntry.getDetail(/windows nt ([\d.]+)/i, extractWindowsVersion)]
+]
+
 exports.browserFullNameToShort = (fullName) => {
-  const ua = useragent(fullName)
-  if (!ua.browser.name && !ua.browser.version && !ua.os.name && !ua.os.version) {
-    return fullName
+  const parts = extractUAParts(fullName)
+  const [browserName, browserVersion] = extractFromParts(parts, UA_BROWSERS)
+  const [osName, osVersion] = extractFromParts(parts, UA_SYSTEMS)
+  if (browserName || osName) {
+    return `${browserName || 'unknown'} ${browserVersion || '0.0.0'} (${osName || 'unknown'} ${osVersion || '0.0.0'})`
  }
-  return `${ua.browser.name} ${ua.browser.version || '0.0.0'} (${ua.os.name} ${ua.os.version || '0.0.0'})`
+  return fullName || 'unknown'
 }


I wonder if we should move this logic to its own file. @devoto13 WDYT?

jginsburgn · 2021-11-01T21:21:29Z

lib/helper.js

 const mm = require('minimatch')

+const extractUAParts = (ua) => {


Can we add some unit tests for this? @davidje13

jginsburgn · 2021-11-01T21:24:29Z

lib/helper.js

+const WINDOWS_NT_VERSION_MAP = {
+  5.1: 'XP',
+  5.2: 'XP',
+  '6.0': 'Vista',
+  6.1: '7',
+  6.2: '8',
+  6.3: '8.1',
+  6.4: '10',
+  '10.0': '10'
+}
+const extractWindowsVersion = (m) => WINDOWS_NT_VERSION_MAP[m[1]]
+
+const UA_BROWSERS = [
+  ({ phantomjs }) => phantomjs && // also contains Safari
+    ['PhantomJS', phantomjs.version],
+
+  ({ headlesschrome }) => headlesschrome && // also contains Safari
+    ['Chrome Headless', headlesschrome.version],
+
+  ({ opera, version }) => opera &&
+    ['Opera', version && version.version],
+
+  ({ firefox }) => firefox &&
+    ['Firefox', firefox.version],
+
+  ({ edg }) => edg && // also contains Chrome, Safari
+    ['Edge', edg.version],
+
+  ({ chrome }) => chrome && // also contains Safari
+    ['Chrome', chrome.version],
+
+  ({ firstEntry, version }) => firstEntry.hasDetail(/^iphone/i) && // also contains Safari
+    ['Mobile Safari', version && version.version],
+
+  ({ firstEntry, version }) => firstEntry.hasDetail(/^android/i) && // also contains Safari
+    ['Android Browser', version && version.version],
+
+  ({ safari, version }) => safari &&
+    ['Safari', version && version.version],
+
+  ({ firstEntry }) => firstEntry.hasDetail(/^msie/i) &&
+    ['IE', firstEntry.getDetail(/^msie ([\d.]+)/i)[1]]
+]
+
+const UA_SYSTEMS = [
+  ({ firstEntry }) => firstEntry.hasDetail(/^android/i) &&
+    ['Android', firstEntry.getDetail(/^android ([\d.]+)/i)[1]],
+
+  ({ firstEntry }) => firstEntry.hasDetail(/^iphone/i) &&
+    ['iOS', firstEntry.getDetail(/iphone os ([\d._]+)/i, extractMacVersion)],
+
+  ({ ubuntu }) => ubuntu &&
+    ['Ubuntu', ubuntu.version],
+
+  ({ firstEntry }) => firstEntry.hasDetail(/^freebsd/i) &&
+    ['FreeBSD', null],
+
+  ({ firstEntry }) => firstEntry.hasDetail(/^linux/i) &&
+    ['Linux', firstEntry.getDetail(/^linux (.+)/i)[1]],
+
+  ({ firstEntry }) => firstEntry.hasDetail(/mac os/i) &&
+    ['Mac OS', firstEntry.getDetail(/mac os(?: x)? ([\d._]+)/i, extractMacVersion)],
+
+  ({ firstEntry }) => firstEntry.hasDetail(/^windows/i) &&
+    ['Windows', firstEntry.getDetail(/windows nt ([\d.]+)/i, extractWindowsVersion)]
+]
+


Where did you get all this info? Probably from the implementation of ua-parser-js? Can we add a reference?

Also, can you please add comments to the functions?

source of this is actually the test user-agent strings which were already in this project. The code and regular expressions here have no relation to ua-parser-js's code.

As for documentation: if the team is happy with this approach, I can extract the code into its own file, add finer-grained tests, and document. Wanted to get a quick first pass submitted without all that so that I didn't waste too much time if the team was opposed to the whole idea.

I am happy with your approach. It is very elegant! :)
I'd like to hear from @devoto13 as well.

Approach overall LGTM!
I'll try to look more into details in the coming days.

jginsburgn · 2021-11-01T21:27:43Z

@davidje13 also please take a look at the checks as there are some failures :)

davidje13 · 2021-11-02T02:39:58Z

@davidje13 also please take a look at the checks as there are some failures :)

seems Node 10 doesn't have matchAll. Easy to replace. I'll update it when I get time.

jginsburgn · 2021-11-02T17:29:10Z

@davidje13 also please take a look at the checks as there are some failures :)

seems Node 10 doesn't have matchAll. Easy to replace. I'll update it when I get time.

Oh yeah! That is minor. :)

devoto13 · 2021-11-02T17:33:40Z

seems Node 10 doesn't have matchAll. Easy to replace. I'll update it when I get time.

I would be more comfortable landing this change in a major release, so it's probably okay to keep using matchAll as we're going to drop Node 10 in the next major.

jginsburgn · 2021-11-02T17:37:59Z

seems Node 10 doesn't have matchAll. Easy to replace. I'll update it when I get time.

I would be more comfortable landing this change in a major release, so it's probably okay to keep using matchAll as we're going to drop Node 10 in the next major.

Or, can we add a TODO where appropriate to replace the Node < 10 equivalent with matchAll when we release the next major (#3503)?

Maybe something in the lines of:

// TODO(#3503): Replace the following logic with `matchAll`.
...

Replace ua-parser-js with a reduced in-house version

b9d0e4c

jginsburgn force-pushed the minimal-ua-parsing branch from 64b7c81 to b9d0e4c Compare November 1, 2021 21:14

jginsburgn requested changes Nov 1, 2021

View reviewed changes

jginsburgn requested a review from devoto13 November 1, 2021 21:26

jginsburgn assigned davidje13 Nov 1, 2021

jginsburgn assigned devoto13 Nov 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace ua-parser-js with a reduced in-house version #3720

Replace ua-parser-js with a reduced in-house version #3720

davidje13 commented Oct 30, 2021

google-cla bot commented Oct 30, 2021

davidje13 commented Oct 30, 2021

jginsburgn commented Nov 1, 2021

jginsburgn Nov 1, 2021

jginsburgn Nov 1, 2021

jginsburgn Nov 1, 2021

jginsburgn Nov 1, 2021

davidje13 Nov 2, 2021

davidje13 Nov 2, 2021

jginsburgn Nov 2, 2021

devoto13 Nov 2, 2021

jginsburgn commented Nov 1, 2021

davidje13 commented Nov 2, 2021

jginsburgn commented Nov 2, 2021

devoto13 commented Nov 2, 2021

jginsburgn commented Nov 2, 2021 •

edited

		const mm = require('minimatch')

		const extractUAParts = (ua) => {

Replace ua-parser-js with a reduced in-house version #3720

Are you sure you want to change the base?

Replace ua-parser-js with a reduced in-house version #3720

Conversation

davidje13 commented Oct 30, 2021

google-cla bot commented Oct 30, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

davidje13 commented Oct 30, 2021

jginsburgn commented Nov 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jginsburgn commented Nov 1, 2021

davidje13 commented Nov 2, 2021

jginsburgn commented Nov 2, 2021

devoto13 commented Nov 2, 2021

jginsburgn commented Nov 2, 2021 • edited

jginsburgn commented Nov 2, 2021 •

edited