deps: [email protected]

[joerx]

Got some code for us? Awesome 🎊!

Please include a description of your change & check your PR against this list, thanks!

• Commit message has a short title & issue references
• Commits are squashed
• The build will pass (run npm test).

More info can be found by clicking the "guidelines for contributing" link above.

closes #6462

• monkey-patch validator.extends() since it was dropped by validator @5.0.0
• coerce input to string prior to validation (custom toString func)
• need to handle boolean validation based on column type not isIn()

[joerx]

validator 5.0.0 breaks validation of non-string input values. this has some interesting consequences:

• to maintain backwards compat, the safe approach is to manually coerce input using toString()
• problematic are validations like 'isIn' with the reference value(s) being non-string - should we consider equal(0, '0') === true? (assuming yes, for compat)
• the schema had a few validations of the form isIn: [[0, 1, false, true]] to validate booleans. toString(true) would return 'true' which is not in the target collection - I modified validation code to validate boolean cols via validator.isBoolean()
• validation for empty values using validator.isEmpty() only works when input is converted to string before, since lodash's _.isEmpty() behaves unexpected when dealing with booleans and numbers

Moreover, validator dropped extend() without replacement (see their #496) - their reasoning is they dislike the global validator object being extended. Since we need exactly that, I monkey patched extend() back in.

[ErisDS]

This is looking good thanks 👍 Having the boolean validations be automatic rather than explicit seems sensible. Did you do much testing to see if there were cases where the assertion/deprecation message gets spat out?

[joerx]

Well, it wouldn't create warnings anymore, since validator 5.0 it would just throw up and fail miserably. Didn't do much manual testing since I assumed the tests would sort it all out - as long as code is going via core/server/data/validation and not using validator directly it should be alright. Can do some additional smoke testing tomorrow though, if you want.

[ErisDS]

@joerx would be excellent if you could check for any code paths using validator directly (hope not!) and run a couple of tests - perhaps try sending weird values to the API?

Other than that this looks good to go :)

[joerx]

OK, so I did some searching and I actually found quite a few direct uses of 'validate' in the code:

• api/authentication (email in generateResetToken) -
• config/index (haven't understood config well enough to qualify)
• sitemap/user-generator (imageUrl - can't qualify but looks safe)
• mail/index (from address - loaded from config = safe?)
• models/user (password)
• models/base (sanitize but disabled)
• models/post (via sanitize but disabled)

Manually testing Posts/Settings APIs produced 500s - ironically not related to the validator at all, but caused by trim() or replace() being called on the title and markdown properties. I added a few tests and the related fixes - I only tested cases where validator is involved though.

Another thing I noticed: validator.sanitize() is not used by models (anymore?) but still required. Maybe some cleanup due?

[joerx]

I'm still wondering what's the best option to avoid '' + foo - lodash's toString() seems to work well, but it's only available in 3.10.1. 😞

[ErisDS]

This looks like it's probably catching and fixing some gnarly edge cases!

Another thing I noticed: validator.sanitize() is not used by models (anymore?) but still required. Maybe some cleanup due?

Yes, it's always good to remove dead code, even if it might be brought back later.

I'm still wondering what's the best option to avoid '' + foo - lodash's toString() seems to work well, but it's only available in 3.10.1. 😞

Reading this stackoverflow on the subject, highlights why _.toString() is superior to any of the native versions as it handles nulls and undefined correctly. We're stuck on an older version of lodash atm, but we could include this module to get access?

[joerx]

Any reason why lodash can't be upgraded? Using lodash.tostring for the time being. Looks good to merge from my side.

[ErisDS]

I left a note on this PR as to why I didn't upgrade it, but I can't say that I remember what the note means 😞

[ErisDS]

Ok, I think this is ready to merge, with one tiny exception - can you please remove the caret from the new dependency version in package.json? Thanks!

[joerx]

Done

[ErisDS]

@joerx This is all looking good, unfortunately all GitHub notifications started going into my spam folder over the weekend and so I didn't see this before it got a merge conflict 😞

If you have time to rebase that'd be great otherwise I'll look at doing it myself sometime later this week.