1371568 - Add Chris O'Hara's validator.js library for string validation in the storage inspector

Status: RESOLVED FIXED

(DevTools :: Storage Inspector, enhancement, P2)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Chris's validator.js has a 3 second startup time inside Firefox so I have had to make a bunch of changes to get this to work.

I have also ported the tests to xpcshell tests as even external library code should contain tests, especially after a large amount of modification.

This library is derived from https://github.com/chriso/validator.js but it is **NOT** the same as the original. We have had to make the following changes:
  - Use lazyRequireGetters to get each type to greatly speed up the library.
  - Remove various require hacks e.g. Object.defineProperty(exports, "__esModule", ...), interopRequireDefault etc.
  - Fixed various bugs.
  - Changed mocha tests to xpcshell based tests.
  - Various fixes to tests to remove node dependencies.
  - Merged the following pull requests:
    - [isMobileNumber] Added Lithuanian number pattern #667
    - Hongkong mobile number #665
    - Added option to validate any phone locale #663
    - Added validation for ISRC strings #660
  - Added isRFC5646 for rfc 5646 #572
  - Added isSemVer for version numbers.
  - Added isRGBColor for RGB colors.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1371568 - Add Chris O"Hara's validator.js library for string validation in the storage inspector

Review commit: https://reviewboard.mozilla.org/r/147516/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147516/

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

You can run the xpcshell tests locally using:
./mach xpcshell-test devtools/shared/stringvalidator/

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Try is green with the xpcshell tests running:

{"status": "PASS", "thread": "Thread-551", "pid": 1692, "source": "XPCShell", "test": "devtools/shared/stringvalidator/tests/unit/test_sanitizers.js", "time": 1497009775273, "action": "test_end", "message": "xpcshell return code: 0"}

{"status": "PASS", "thread": "Thread-552", "pid": 1692, "source": "XPCShell", "test": "devtools/shared/stringvalidator/tests/unit/test_validators.js", "time": 1497009780508, "action": "test_end", "message": "xpcshell return code: 0"}

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8876081 [details]
Bug 1371568 - Add Chris O"Hara's validator.js library for string validation in the storage inspector

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147516/diff/1-2/

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8876081 [details]
Bug 1371568 - Add Chris O"Hara's validator.js library for string validation in the storage inspector

https://reviewboard.mozilla.org/r/147516/#review153350

Sorry for taking so long to get to this review Mike.
I have 2 comments about this:
- why no just copying the validator.js file from https://github.com/chriso/validator.js/blob/master/validator.js instead of the whole content of the lib directory? Much easier to update in the future.
- Please move this to devtools/client/shared/vendor/ As you can see, this directory already has things like d3.js or React. It's also a good idea to copy the license file as a new file in this directory as well as creating a new file that explains how we're supposed to update the library.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Patrick Brosset <:pbro> from comment #5)
> Comment on attachment 8876081 [details]
> Bug 1371568 - Add Chris O"Hara's validator.js library for string validation
> in the storage inspector
> 
> https://reviewboard.mozilla.org/r/147516/#review153350
> 
> Sorry for taking so long to get to this review Mike.
> I have 2 comments about this:
> - why no just copying the validator.js file from
> https://github.com/chriso/validator.js/blob/master/validator.js instead of
> the whole content of the lib directory? Much easier to update in the future.

I will try that but the whole point of the changes was to dramatically shorten a 3 second startup time.

> - Please move this to devtools/client/shared/vendor/ As you can see, this
> directory already has things like d3.js or React. It's also a good idea to
> copy the license file as a new file in this directory as well as creating a
> new file that explains how we're supposed to update the library.

Will do.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8876081 [details]
Bug 1371568 - Add Chris O"Hara's validator.js library for string validation in the storage inspector

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147516/diff/2-3/

Comment 8

[Patrick Brosset <:pbro>]

As discussed in a meeting today, I'm ok to R+ this but would like a list of justifications for the many changes described in the updating.md file. I want to be sure we do need all these manual changes, because they're going to make it harder for us to update the library in the future.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Patrick Brosset <:pbro> from comment #8)
> As discussed in a meeting today, I'm ok to R+ this but would like a list of
> justifications for the many changes described in the updating.md file. I
> want to be sure we do need all these manual changes, because they're going
> to make it harder for us to update the library in the future.

The following are common string formats stored in cookies that should not be displayed as a tree structure along with their validator function names:

- isISRC(): International Standard Recording Codes (ISRC) are used in the music industry to identify a particular recording by a particular artist. It identifies a particular recording, not the work (composition and lyrical content) itself. Therefore, different recordings, edits, and remixes of the same work should each have their own ISRC. These are often used by online apps to identify a particular recording so they are often used in cookies.
- isRFC5646(): These are country codes paired with language codes e.g. en-GB, en-US, fr-FR etc. and are often used by cookies to identify a users country and language settings.
- isSemVer(): Semantic version numbers e.g. 1.0.1, 1.0.2, 1.0.3beta, 1.0.4rc2 etc. are stored in cookies created by .NET apps to show the cookie version number. Different cookie versions in .NET apps have different properties, hence including the version number inside the cookie.
- isRGBColor(): Should need no introduction, often stored in cookies by themes and many online apps.
- isMobilePhone(): Obviously often stored in cookies. This method has been corrected to work with all Hong Kong mobile numbers. Support has also been added for Korean and Lithuanian mobile phone numbers.
- isBase64(): I have made use of atob() to use Firefox's inbuilt base64 check rather than relying on a regular expression.

Comment 10

[Patrick Brosset <:pbro>]

Comment on attachment 8876081 [details]
Bug 1371568 - Add Chris O"Hara's validator.js library for string validation in the storage inspector

https://reviewboard.mozilla.org/r/147516/#review156266

Thank you for comment 9.
It still feels to me like this is going to be too hard to maintain on the long run, but I don't think we'll actually have to maintain this library much.
Thank you for adding the tests too, I like those.

A thought: aren't there fewer cases where we want to parse the value rather than cases where we don't want to? If so, then it would actually be simpler to whitelist certain cases rather than blacklist a bunch of random things like phone numbers, etc.

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Patrick Brosset <:pbro> from comment #10)
> Comment on attachment 8876081 [details]
> Bug 1371568 - Add Chris O"Hara's validator.js library for string validation
> in the storage inspector
> 
> https://reviewboard.mozilla.org/r/147516/#review156266
> 
> Thank you for comment 9.
> It still feels to me like this is going to be too hard to maintain on the
> long run, but I don't think we'll actually have to maintain this library
> much.
> Thank you for adding the tests too, I like those.
> 
> A thought: aren't there fewer cases where we want to parse the value rather
> than cases where we don't want to? If so, then it would actually be simpler
> to whitelist certain cases rather than blacklist a bunch of random things
> like phone numbers, etc.

If only life was so simple. The problem is that storage values, especially cookie values are free text. This means that they are often URLEncoded and use random things as separators e.g. "~", "#", ".", ",", "=" or pretty much anything. Most people that I have asked say they like the tree but complain that it gets it wrong too often, hence this bug.

The things this fixes are not so random but I see the list is missing from this bug:

  - Base64
  - dataURI
  - email
  - FQDN
  - IP
  - ISBN
  - ISIN
  - ISSN
  - Mac Address
  - MD5 & SHA1 (SHA1 has 40 chars and MD5 32)
  - Mobile Phone
  - URL
  - UUID
  - Credit card number

I think the most common cases are Base64, dataURI, email, FQDN, IP, Mac Address, MD5, SHA1, UUID and URL so I have covered them and a little more... I don't think they are random but rather common strings that have commonly used separators used by cookies but shouldn't be displayed as a tree-type structure.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb46683ba2f8
Add Chris O"Hara's validator.js library for string validation in the storage inspector r=pbro

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/eb46683ba2f8