Closed Bug 1371568 Opened 3 years ago Closed 2 years ago

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

Categories

(DevTools :: Storage Inspector, enhancement, P2)

51 Branch
enhancement

Tracking

(firefox56 fixed)

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: miker, Assigned: miker)

References

Details

(Whiteboard: [todo-mr][t1])

Attachments

(1 file)

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.
You can run the xpcshell tests locally using:
./mach xpcshell-test devtools/shared/stringvalidator/
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"}
Summary: Add Chris O"Hara's validator.js library for string validation in the storage inspector → Add Chris O'Hara's validator.js library for string validation in the storage inspector
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.
Attachment #8876081 - Flags: review?(pbrosset)
(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.
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.
(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.
Flags: needinfo?(pbrosset)
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.
Attachment #8876081 - Flags: review?(pbrosset) → review+
Flags: needinfo?(pbrosset)
(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.
Pushed by mratcliffe@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/eb46683ba2f8
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.