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

Already on GitHub? Sign in to your account

Resubmission (fixed whitespace): Crude fix to identify Annex B deprecated functions #1397

Closed
wants to merge 11 commits into
from

Conversation

Projects
None yet
2 participants

smikes commented Dec 3, 2013

This is a crude quick fix to identify functions noted in ES5 Annex B as nonstandard or deprecated: notably 'escape', 'unescape', 'substr' and the non-Y2K-friendly 'getYear' and 'setYear'

This addresses an open issue in the 2.x codebase ( #1092 )

Although this is contrary to the goals for current (3.x) jshint development, I believe this should be included in 2.x jshint for the following reasons:

  1. substantial interest including small ($50) bug bounty
  2. Implementing this change and running it against jshint source identified ~10 uses of the nonstandard 'substr' in jshint sources, which were replaced by 'substring' (see pull request)

The option to enable this behavior is named 'legacy'; the behavior is enabled by setting legacy: true

Changes 2013-Dec-03:

  1. Undo extraneous changes to whitespace in src/jshint.js
  2. Change 'pretest' script to use current, not installed, jshint, for bootstrapping new option 'legacy'; previous PR failed CI because option 'legacy' was not understood
Owner

smikes commented on 4e1e881 Dec 3, 2013

Merge from upstream

@valueof valueof commented on an outdated diff Dec 5, 2013

@@ -27,7 +27,7 @@
"data": "node scripts/generate-identifier-data",
"build": "node bin/build",
"test": "nodeunit tests tests/regression tests/unit",
- "pretest": "jshint src"
+ "pretest": "node bin/jshint src"
@valueof

valueof Dec 5, 2013

Owner

This isn't needed. We want to test JSHint with a previous stable version of JSHint.

@valueof valueof commented on an outdated diff Dec 5, 2013

@@ -63,7 +63,8 @@
"undef": true,
"unused": true,
"onecase": true,
- "indent": 2
+ "indent": 2,
+ "legacy": true
@valueof

valueof Dec 5, 2013

Owner

We can enable this option once it ships with the stable version.

smikes commented Dec 5, 2013

OK. Would you like me to modify the pull request, or can you deal with the changes on your side?

Owner

valueof commented Dec 21, 2013

Unfortunately, this patch will have to wait. I feel like simply checking method names will generate too many false-positives.

smikes commented Dec 21, 2013

Okay.. is there a false positive rate you're willing to accept, if I can
show on a corpus of commonly-used javascript?

Alternatively, is there any way to implement this feature short of a full
execution? Without a list of active frames at execution time and the live
runtime type of all objects, we can't prove that an annex B method is being
called.

On Fri, Dec 20, 2013 at 6:17 PM, Anton Kovalyov notifications@github.comwrote:

Unfortunately, this patch will have to wait. I feel like simply checking
method names will generate too many false-positives.


Reply to this email directly or view it on GitHubhttps://github.com/jshint/jshint/pull/1397#issuecomment-31053273
.

Owner

valueof commented Dec 21, 2013

I'm going to do a 2.4.0 release next week and then will think harder about this patch and the problem in general.

Owner

valueof commented Mar 27, 2014

I tried this patch out on our relatively large web app project at Medium and it generates way too many false positives. I'm not sure what the solution is here—but the crude fix won't make it I'm afraid.

@valueof valueof closed this Mar 27, 2014

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