[meta] Overarching code audit of differences

[jonathanKingston]

As mentioned I would check through the code and see for obvious differences and pull them out. I'm going to raise everything here and we can decide which differences we care about and should import.
Rather than question the differences etc, I will just state them as fact and we can dismiss and add comments (none of this should be considered criticism, other than I'm an idiot for making a competing code base etc 😬).

• Difference in export of rule - I think I am using the new/suggested format here
• Use strict Do we care any more about this?
• Only addition supported in expressions Is there a chance other expressions would matter (I just treated them all the same for paranoia sake).
• Permitted ecaping I allowed each member and assignment we are checking to have it's own type. This would allow for jQuery, react and html checks all within the same codebase but ensure the author is using the correct sanitizer for the job (Something I notice was an issue sometimes in fxos).
• Checks are completed on exit - this didn't seem to be the case for mine, did I miss something? If so can we test for it?
• If we don't need to complete on exit, the code will look simpler as we can potentially use ES2016+ formatting
• Switch statement vs if's - Personally I would prefer a different object call actually see code below.
• Use of ES2016+ formatting
• Hard coding of method and property checks
• Missing argument number in error reported
• Check doesn't explicitly return false
• By making the rule configurable, we should provide defaults here ideally we could allow users to just add in react, jQuery and DOM into their defaults.
• Linting should probably be added too space around if statement.
• Explicitly whitelist expression types rather than check for properties here the check if for .callee.properties or .callee.name rather than checking for if it is an Identifier or MemberExpression in this code this means new ESNext expression or something we missed breaks the extension rather than working (Hopefully getting other people to patch etc).
• Handle native methods when making configurable just as a reminder. The toString of the config object itself then gets checked as appears it's part of the array/objects etc.
• More checking for props over this being explicit
• Generalised method to get identifier of the function for stuff we don't know about it throws too. Because of this my CallExpression and TaggedTemplateExpression could be collapsed into one which is a TODO.
• allowedExpression wouldn't need parentNode without comments, it would however need the type/key as isSafeNode uses if we were to make it configurable.

That should mostly be it...

---

Different code example over using switch or if statements:

checkThing(node) {
  if (node.type in checkNode) {
    return checkNode[node.type](node);
  } else {
    throw new Error('Argh something happened with a node we didn\'t expect');
  }
};

const checkNode = {
  TaggedTemplateExpression(node) {
    // do something...
    return isValid;
  },
  CallExpression(node) {
    // do something...
    return isValid;
  }
};

[mozfreddyb]

I'll go through your points one by one:

• difference in export rule: I'll use whatever eslint prefers, I don't want to get philosophical on that. If both works, I'm fine with making this a low-priority change later
• use strict: I like the benefits of strict mode. I want to keep using it.
• expression operators: innerhtml -=, **= *= etc, dont make a lot of sense to me. Strings turn into NaN when operated with -, *, etc..
• permitted escaping: yes, I think we should have specific escapers. If things get too "weird" for * jquery/jsx, we could even throw that in its own rules. These areas are mostly unknown to me, so I'd like to dig deeper in the future.
• checks are completed on AssignmentExpression:exit, I think I wanted some subnodes. Maybe this is because I checked comments and not required anymore. is this a problem? let's remove and see if it breaks tests! trying in Try checking AssignmentExpression, not :exit. #21.
• es2016+: yeah sure :) let's see how good nodejs compat is though
• non-hardcoding property checks and stuff: aye!
• argument information in error report: agreed
• not explicitly returning false: good catch
• configurability & defaults: ack
• I think I have linting. maybe I'm missing a rule.
• EsNext expression concerns: also a good catch, we should be careful of those things indeed, I have been broken by this before.
• I don't understand the native code check
• props checking, aye. should become its own rule mayyyyyybe
• getting identifier, yeah.
• allowedExpression doesnt need parentNode anymore: agreed

[mozfreddyb]

I'm in a bit of a rush and I feel bad for doing this bad multi-response thing.
I will likely turn most of those into single issues in the next week, as tracking it like this is horrible.
Thank you for your input!!

[mozfreddyb]

I'll go through your points one by one:

• use new style to export a rule, rewrite export statement #24
• configurable escaping make name of escape functions configurable (defaulting to the existing ones) #22
• investigate jsx,jquery support investigate jquery api / jsx support #23
• checks are completed on AssignmentExpression:exit, I think I wanted some subnodes. Maybe this is because I checked comments and not required anymore. is this a problem? let's remove and see if it breaks tests! trying in Try checking AssignmentExpression, not :exit. #21.
• es2016+ support (depending on nodejs compat and nodejs uptake) support es2016+ syntax #25
• non-hardcoding property checks and stuff: aye! un-hardcode property names #26
• argument information in error report supply argument information in error name for function call block targeting specific arguments #27
• explicitly return false here, is return false explicitly in... #28
• configurability & defaults: revisit configurability options and defaults #29
• linting checks is investigate linting #30
• EsNext expression concerns: also a good catch, we should be careful of those things indeed, I have been broken by this before. this is new "esnext" expression means new bypass? #31.
• native code check native code check #32
• More property checking is moar property checking? #33, maybe this is its own rule.
• function identifier sanity is Reduce code-duplication when checking function calls on right-hand side #13.
• combine TaggedTemplateExpression and CallExpression code. likely also Reduce code-duplication when checking function calls on right-hand side #13.
• allowedExpression wouldn't need parentNode, that's rewrite allowexpression part to not require parent node #34

[jonathanKingston]

@mozfreddyb can we consider making the expression operators use an allow list of permitted expressions rather than a block list of checked operators. Such that we would have allowedExpressions = ["-=", "**=", "*=", "/="]; or similar? So if fancy new operator comes along that adds an exploit we don't need to remember to fix out code in future etc. #42

[jonathanKingston]

@mozfreddyb also I notice you are only checking for context.getSource(node.callee) for write and writeln but node.callee.property.name for insertAdjacentHTML was this intentional to avoid valid write functions? This might be a very breaking code change to add to make write use the property.name (I remember now this is exactly why I forked, when I mentioned breaking changes). #41