Skip to content

Added 'content' validator to security #490

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Aug 29, 2016

Conversation

jonuriarte
Copy link
Contributor

I've followed the suggestions made in #368 .

The optional attributes are:

  • data-validation-require-uc-letter
  • data-validation-require-lc-letter
  • data-validation-require-special-char
  • data-validation-require-numeral

You can set these attributes to the number of characters of each type you want in the password:

ex. data-validation-require-lc-letter="3"

@victorjonsson
Copy link
Owner

victorjonsson commented Aug 25, 2016

Hello!

Thanks for the contribution, it's very appreciated! There are some things that needs to be discussed and taken care of, before I can merge this pr.

Naming style of variables
Variables should be written in camelCase and abbreviations should be avoided. Consider naming require_lc to something like numRequiredLowercaseChars (being verbose isn't dangerous in my opinion).

Regex for special characters
I'm no regex guru but wouldn't [^a-zA-Z0-9] be a more correct way of matching special characters? I consider any character that is not alphanumeric to be a "special character".

Unit tests
Please add some tests to test/qunit.html confirming that the validator works. (The test coverage of this plugin is horrible, just adding some tests is better than none though).

Validator name
I think I understand the thought process behind naming the validator content. But aren't all validators confirming the validity of some "content"? The length-validator is validating the length of some content, the alphanumeric-validator is validating that some content only contains alhphanumeric characters and so on... Why should this particular validator be named "content"? This validator will mostly be used to validate passwords or passphrases. For that reason, it feels more logical to simply name the validator "password" or "passphrase". What do you think?

Localization
It's okey to not translate all dialogs, for all languages provided by the plugin, when introducing a new validator. The community will eventually translate the dialogs. But it's not okey to hardcode the dialogs in the source code of the validator. This will be a hazzle. You have to move all the dialogs into the global language object and also to the english language file.

@jonuriarte
Copy link
Contributor Author

Hi,

Naming style of variables
Ok, I'll correct that.

Regex for special characters
That was my mistake. I was doing some test and I forgot to correct it.
Instead of considering a valid character any character that is not alphanumeric, I think we should a "limited" list: https://www.owasp.org/index.php/Password_special_characters. So it should be:

[!"#$%&'()*+\,-./:;<=>?@[]^_`{|}~]

Unit tests
Ok, I'll add some tests.

Validator name
I agree with you, content is a very generic name for the validator. How about complexity or requirements?

Localization
Ok, I'll correct that.

More descriptive variable names.
Regex fix for special chars.
Validator name changed.
Localization added.
Added localization
Added some test for the complexity validator
@victorjonsson
Copy link
Owner

Great!

I like the name "complexity" and agree with you regarding the regex for special characters.

It looks ready for merge?

@victorjonsson victorjonsson merged commit 3c0e5c2 into victorjonsson:master Aug 29, 2016
@victorjonsson
Copy link
Owner

Superb work!

@jonuriarte
Copy link
Contributor Author

Thank you!!! Congrats for the plugin!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants