When regexp:true, should have option to permit ^ to negate character classes #555

Closed
Xeriar opened this Issue Jun 20, 2012 · 9 comments

Comments

Projects
None yet
7 participants
@Xeriar

Xeriar commented Jun 20, 2012

I usually use this to prune invalid characters as people type them for e.g. phone numbers, birthdays, etc. and such. On the other hand, if I put an unescaped period into a regular expression, it's almost always a mistake on my part, so I like regexp:true. However the complaints about my starting a character class with ^ are mildly annoying.

My preferred solution would be to break regexp:true up into two options, one just for periods and the other for the negation, or just remove:

if (option.regexp) {
    warningAt("Insecure '{a}'.", line, from + l, c);
} else 

It's not actually documented as being flagged for regexp:true and this is the first I've heard of class negations confusing people.

@Krinkle

This comment has been minimized.

Show comment
Hide comment
@Krinkle

Krinkle Jul 3, 2012

Can you paste an example of the regexp code that fails jshint?

Krinkle commented Jul 3, 2012

Can you paste an example of the regexp code that fails jshint?

@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Jul 3, 2012

Contributor

I just had the same problem with this regex /[^\r]\n/, while trying to match Unix lineendings, but not Windows.

Contributor

sindresorhus commented Jul 3, 2012

I just had the same problem with this regex /[^\r]\n/, while trying to match Unix lineendings, but not Windows.

@archiecobbs

This comment has been minimized.

Show comment
Hide comment
@archiecobbs

archiecobbs Jul 18, 2012

I agree there are times when the Insecure '^' check is overkill. For example, I'm trying to to this:

var digits = s.replace(/[^0-9]/g, "")

Clearly there's nothing unsafe about stripping away all the non-digit characters from a string.

But currently there's no /* jshint: */ option for disabling just this check.

Request adding one.

I agree there are times when the Insecure '^' check is overkill. For example, I'm trying to to this:

var digits = s.replace(/[^0-9]/g, "")

Clearly there's nothing unsafe about stripping away all the non-digit characters from a string.

But currently there's no /* jshint: */ option for disabling just this check.

Request adding one.

@Krinkle

This comment has been minimized.

Show comment
Hide comment
@Krinkle

Krinkle Jul 18, 2012

http://www.jshint.com/
With all the default settings (all enabled) it doesn't show a warning, but when setting regexp:true in the code it does yield the following warning.

/*jshint regexp:true */
var digits = ''.replace(/[^0-9]/g, '');
Line 2: var digits = ''.replace(/[^0-9]/g, '');
Insecure '^'.

Krinkle commented Jul 18, 2012

http://www.jshint.com/
With all the default settings (all enabled) it doesn't show a warning, but when setting regexp:true in the code it does yield the following warning.

/*jshint regexp:true */
var digits = ''.replace(/[^0-9]/g, '');
Line 2: var digits = ''.replace(/[^0-9]/g, '');
Insecure '^'.
@archiecobbs

This comment has been minimized.

Show comment
Hide comment
@archiecobbs

archiecobbs Jul 18, 2012

Yes, sorry for the confusion. I am using regexp: true but since the Insecure '^' is not documented on the jshint web site under that option, I didn't know that this option was causing the check to be enabled.

Although it would be nice to have separate options, perhaps the more immediate problem then is the documentation bug.

Yes, sorry for the confusion. I am using regexp: true but since the Insecure '^' is not documented on the jshint web site under that option, I didn't know that this option was causing the check to be enabled.

Although it would be nice to have separate options, perhaps the more immediate problem then is the documentation bug.

@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Jul 18, 2012

Contributor

I want to be warned on insecure regex use like ., but I still would like an option to turn of the ^ warnings.

Contributor

sindresorhus commented Jul 18, 2012

I want to be warned on insecure regex use like ., but I still would like an option to turn of the ^ warnings.

@Skalman

This comment has been minimized.

Show comment
Hide comment
@Skalman

Skalman Aug 9, 2012

I don't believe warnings about ^ are warranted, ever.

Skalman commented Aug 9, 2012

I don't believe warnings about ^ are warranted, ever.

@Xeriar

This comment has been minimized.

Show comment
Hide comment
@Xeriar

Xeriar Aug 12, 2012

If no one thinks it's warranted (I certainly don't), could just strip the offending code as described above.

Xeriar commented Aug 12, 2012

If no one thinks it's warranted (I certainly don't), could just strip the offending code as described above.

@valueof valueof closed this in 53e862e Aug 29, 2012

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc Dec 19, 2012

Anton,

You should probably remove ^ from the comment found at 53e862e#L1R836

cowwoc commented Dec 19, 2012

Anton,

You should probably remove ^ from the comment found at 53e862e#L1R836

jugglinmike added a commit to jugglinmike/jshint that referenced this issue Oct 21, 2014

Don't warn about insecure ^ when regexp is set.
This option was supposed to be only about insecure '.' and lots of
people seem to thnk that warning about ^ is unwarranted.

Closes GH-555.

danielctull pushed a commit to danielctull-forks/jshint that referenced this issue Jan 14, 2018

danielctull pushed a commit to danielctull-forks/jshint that referenced this issue Jan 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment