mozilla / eslint-plugin-no-unsanitized

Custom ESLint rule to disallows unsafe innerHTML, outerHTML, insertAdjacentHTML and alike
Mozilla Public License 2.0
228 stars 37 forks source link

Cannot read property 'name' of undefined #7

Closed LockeLamora closed 8 years ago

LockeLamora commented 8 years ago

With certain JS files I'm getting this error, It's a pretty massive codebase so I haven't isolated the exact file(s) yet to give any more information, is an assumption being made about the structure of the JS?

Cannot read property 'name' of undefined TypeError: Cannot read property 'name' of undefined at allowedExpression (/usr/local/lib/node_modules/eslint-plugin-no-unsafe-innerhtml/lib/rules/no-unsafe-innerhtml.js:62:78) at EventEmitter.AssignmentExpression:exit (/usr/local/lib/node_modules/eslint-plugin-no-unsafe-innerhtml/lib/rules/no-unsafe-innerhtml.js:98:30) at emitOne (events.js:82:20) at EventEmitter.emit (events.js:169:7) at NodeEventGenerator.leaveNode (/usr/local/lib/node_modules/eslint/lib/util/node-event-generator.js:49:22) at CodePathAnalyzer.leaveNode (/usr/local/lib/node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:628:23) at CommentEventGenerator.leaveNode (/usr/local/lib/node_modules/eslint/lib/util/comment-event-generator.js:110:23) at Controller.leave (/usr/local/lib/node_modules/eslint/lib/eslint.js:901:36) at Controller.__execute (/usr/local/lib/node_modules/eslint/node_modules/estraverse/estraverse.js:397:31) at Controller.traverse (/usr/local/lib/node_modules/eslint/node_modules/estraverse/estraverse.js:491:28)

mozfreddyb commented 8 years ago

Hey, thanks for reporting that error! The stack trace seems to link to line 62 of lib/rules/no-unsafe-innerhtml.js, but that lines does not contain any attempt to read a property 'name'. This means that I can not proceed debugging this problem.

Did you modify your version of the script? If so, could you please respond with additional details or possibly a link to a gist that shows your version of the file?

LockeLamora commented 8 years ago

Hi, thanks for the reply, nope nothing should be customised

It's odd, I've got the latest and did another pull just to be safe, and it's still showing up as line 62, but when i view the code through this web ui, It's showing up as being on line 74

} else if (expression.type === "CallExpression") {

      var funcName = expression.callee.name || expression.callee.property.name;

      if (VALID_UNWRAPPERS.indexOf(funcName) !== -1) {

        allowed = true;
      }

the 2nd line there is line 62 for my code

mozfreddyb commented 8 years ago

OK, I think I found the bug. This is likely due to an arrow function expression being called. In this case, both expression.callee.name and expression.callee.property.name are null/undefined.

I guess we should be checking funcName being truthy before calling VALID_UNWRAPPERS.indexOf like so:

 if ( funcName && VALID_UNWRAPPERS.indexOf(funcName) !== -1) {

Would you be willing to create a pull request, @LockeLamora?

LockeLamora commented 8 years ago

sure! I've nuked my repo and re-ran it and it's behaving properly and showign as line 74 now

it was installed as part of the scanjs install.sh, so i'm guessing it's not finding latest

LockeLamora commented 8 years ago

hm I'm not sure that fix will work, it's failing trying to define funcName due to the null expression.callee parameters it tries to assign with so it'll fail before that check still

I've just tried running with that change and still failing at 74. I'm trying a new check in there, just waiting to see if the job completes :)

LockeLamora commented 8 years ago

ok my fix works but i'm denied access to create a branch, it's just a one line change if you want me to paste it here?

mozfreddyb commented 8 years ago

You have to fork this repository and create a branch over there. The Pull Request should come from your fork.

LockeLamora commented 8 years ago

Thanks very much for your help!

There's some wacky JS in our project, so now i have thousands of errors and warnings to log with the devs.