mozilla / eslint-plugin-no-unsanitized

Custom ESLint rule to disallows unsafe innerHTML, outerHTML, insertAdjacentHTML and alike

Disallow unsanitized code (no-unsanitized)

These rules disallow unsafe coding practices that may result into security vulnerabilities. We will disallow assignments (e.g., to innerHTML) as well as calls (e.g., to insertAdjacentHTML) without the use of a pre-defined escaping function. The escaping functions must be called with a template string. The function names are hardcoded as Sanitizer.escapeHTML and escapeHTML. The plugin also supports the Sanitizer API and calls to .setHTML() are also allowed by default.

This plugin is built for and used within Mozilla to maintain and improve the security of our products and services.

Rule Details

method

The method rule disallows certain function calls. E.g., document.write() or insertAdjacentHTML(). See docs/rules/method.md for more.

property

The property rule disallows certain assignment expressions, e.g., to innerHTML.

See docs/rules/property.md for more.

Examples

Here are a few examples of code that we do not want to allow:

foo.innerHTML = input.value;
bar.innerHTML = "<a href='" + url + "'>About</a>";

A few examples of allowed practices:

foo.innerHTML = 5;
bar.innerHTML = "<a href='/about.html'>About</a>";
bar.innerHTML = escapeHTML`<a href='${url}'>About</a>`;

Install

With yarn or npm:

$ yarn add -D eslint-plugin-no-unsanitized
$ npm install --save-dev eslint-plugin-no-unsanitized

Usage

In your .eslintrc.json file enable this rule with the following:

{
    "extends": ["plugin:no-unsanitized/DOM"]
}

Or:

{
    "plugins": ["no-unsanitized"],
    "rules": {
        "no-unsanitized/method": "error",
        "no-unsanitized/property": "error"
    }
}

Documentation

See docs/.

eslint-plugin-no-unsanitized's Issues

Husky throws a TypeError: context.reportUnsupported is not a function

I am using a husky pre-commit and prepush hook for Lightbeam WE, and this plug-in is throwing an error. Here are the git details:

bdanforth ~/projects/lightbeam-we (make-vars-dynamic) $ git commit -m "Partial fix per PR feedback"

> husky - npm run -s precommit
> husky - node v6.10.3

context.reportUnsupported is not a function
TypeError: context.reportUnsupported is not a function
    at EventEmitter.CallExpression (/Users/bdanforth/Projects/lightbeam-we/node_modules/eslint-plugin-no-unsanitized/lib/rules/method.js:77:29)
    at emitOne (events.js:101:20)
    at EventEmitter.emit (events.js:188:7)
    at NodeEventGenerator.applySelector (/Users/bdanforth/Projects/lightbeam-we/node_modules/eslint/lib/util/node-event-generator.js:265:26)
    at NodeEventGenerator.applySelectors (/Users/bdanforth/Projects/lightbeam-we/node_modules/eslint/lib/util/node-event-generator.js:294:22)
    at NodeEventGenerator.enterNode (/Users/bdanforth/Projects/lightbeam-we/node_modules/eslint/lib/util/node-event-generator.js:308:14)
    at CodePathAnalyzer.enterNode (/Users/bdanforth/Projects/lightbeam-we/node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:602:23)
    at CommentEventGenerator.enterNode (/Users/bdanforth/Projects/lightbeam-we/node_modules/eslint/lib/util/comment-event-generator.js:98:23)
    at Traverser.enter (/Users/bdanforth/Projects/lightbeam-we/node_modules/eslint/lib/eslint.js:929:36)
    at Traverser.__execute (/Users/bdanforth/Projects/lightbeam-we/node_modules/estraverse/estraverse.js:397:31)

> husky - pre-commit hook failed (add --no-verify to bypass)
> husky - to debug, use 'npm run precommit'

JSX Support (Was: Cannot read property 'innerHTML' of undefined)

Hey,

just wanted to let you know that we observed the following error while using this plugin in version 3.0.2. I'm not sure how to get more information about this issue.

Error while loading rule 'no-unsanitized/property': Cannot read property 'innerHTML' of undefined
TypeError: Error while loading rule 'no-unsanitized/property': Cannot read property 'innerHTML' of undefined
at Object.keys.forEach (/Users/.../node_modules/eslint-plugin-no-unsanitized/lib/ruleHelper.js:207:34)
at Array.forEach (<anonymous>)
at RuleHelper.combineRuleChecks (/Users/.../node_modules/eslint-plugin-no-unsanitized/lib/ruleHelper.js:204:38)
at new RuleHelper (/Users/.../node_modules/eslint-plugin-no-unsanitized/lib/ruleHelper.js:22:28)
at Object.create (/Users/.../node_modules/eslint-plugin-no-unsanitized/lib/rules/property.js:42:28)
at createRuleListeners (/Users/.../node_modules/eslint/lib/linter.js:682:21)
at Object.keys.forEach.ruleId (/Users/.../node_modules/eslint/lib/linter.js:841:31)
at Array.forEach (<anonymous>)
at runRules (/Users/.../node_modules/eslint/lib/linter.js:781:34)
at Linter._verifyWithoutProcessors (/Users/.../node_modules/eslint/lib/linter.js:1003:33)

Unsupported Callee for CallExpression (no-unsanitized/method) PROBLEMS OUTPUT DEBUG CONSOLE TERMINAL

no-unsanitized reports "Unexpected Callee" for

var f = new Function()(); // could give a string to the function constructor as well

my eslint configuration is:

{
  "plugins": ["no-unsanitized"],
  "rules": {
    "no-unsanitized/method": "error",
    "no-unsanitized/property": ["error", {
      "escape": {
        "methods": ["MyEncoder.encodeHTML"]
      }
    }]
  }
}

[typescript] Unsupported Callee for CallExpression (NonNullExpression)

Error in no-unsanitized: Unexpected Callee no-unsanitized/method

The error is when using TypeScript ! sign

const forTimeline = (id: string) => {
    if (!firebaseProvider.app) return;

    const timeline = firebaseProvider.app.database!()
        .ref('timelines')
        .child(id)
        .orderByChild('timestamp');

    return timeline;
};

This is my .eslintrc.js file

module.exports = {
    parser: '@typescript-eslint/parser',
    extends: [
        'plugin:@typescript-eslint/recommended',
        'prettier/@typescript-eslint',
        'plugin:prettier/recommended',
        'plugin:react/recommended',
        'plugin:security/recommended',
        'plugin:no-unsanitized/DOM',
    ],
    plugins: ['scanjs-rules', 'security', 'no-unsanitized', 'no-wildcard-postmessage'],
    parserOptions: {
        ecmaVersion: 2018,
        sourceType: 'module',
        ecmaFeatures: {
            jsx: true,
        },
    },
    rules: {
        '@typescript-eslint/explicit-function-return-type': 'off',
        '@typescript-eslint/no-explicit-any': 'off',
        '@typescript-eslint/no-empty-interface': [
            'error',
            {
                allowSingleExtends: true,
            },
        ],
        '@typescript-eslint/no-use-before-define': ['error', {functions: false, classes: true}],
        'security/detect-non-literal-require': 'off',
        'security/detect-object-injection': 'off',
        '@typescript-eslint/no-inferrable-types': 'off',
        '@typescript-eslint/no-non-null-assertion': 'off',
    },
    settings: {
        react: {
            version: 'detect',
        },
    },
};

Please include documentation on how to install/use

If there were documentation on how to call it, that would be helpful. I can't get no-unsanitized to warn when it should, even when I intentionally do not escape. Any idea what I am doing wrong?

As an aside, here is a first stab at some documentation you might include in the README:

---

Install

With yarn or npm:

$ yarn add -D git+https://github.com/mozilla/eslint-plugin-no-unsanitized.git
$ npm install --save-dev git+https://github.com/mozilla/eslint-plugin-no-unsanitized.git

Usage

With eslint -- enable the plugin in eslint.rc file by adding it to the plugins section:

plugins: ["no-unsanitized"]

(Or any other method in eslint's docs)

With standard:

standard --plugin no-unsanitized

(Or any other method in standardjs' docs)

A sanitizer you might find useful: https://www.npmjs.com/package/escape-html-template-tag

Consider allow list for assignment operators

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.

Block document.write and document.writeln

I should add document.write and document.writeln besides insertAdjacentHTML.

supply argument information in error name for function call block targeting specific arguments

revisit configurability options and defaults

Unsupported Callee for CallExpression no-unsanitized/method

I tested eslint-plugin-no-unsanitized and got the following output:

D:\dev\projects\wireapp\wire-webapp\app\script\view_model\bindings\CommonBindings.js
303:14 error Error in no-unsanitized: Unexpected Callee. Please report a minimal code snippet to the developers at https://github.com/mozilla/eslint-plugin-no-unsanitized/issues/new?title=Unsupported%20Callee%20for%20CallExpression no-unsan
itized/method
306:7 error Error in no-unsanitized: Unexpected Callee. Please report a minimal code snippet to the developers at https://github.com/mozilla/eslint-plugin-no-unsanitized/issues/new?title=Unsupported%20Callee%20for%20CallExpression no-unsan
itized/method

It seems like the plugin doesn't like return this().trim(); & this(value.trim()); in:

ko.subscribable.fn.trimmed = function() {
  return ko.computed({
    owner: this,
    read() {
      return this().trim();
    },
    write(value) {
      this(value.trim());
      this.valueHasMutated();
    },
  });
};

Here are the links to the specific lines of CommonBindings.js (which the plugin complains about):

• https://github.com/wireapp/wire-webapp/blob/3d2c7f7ba7694f988fc2e6736a2e89889aee4a09/app/script/view_model/bindings/CommonBindings.js#L303
• https://github.com/wireapp/wire-webapp/blob/3d2c7f7ba7694f988fc2e6736a2e89889aee4a09/app/script/view_model/bindings/CommonBindings.js#L306

native code check

@jonathanKingston said

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.

Can you explain more @jonathanKingston? I'm not sure I understand.

Cannot read property 'name' of undefined

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)

Wiki changes

FYI: The following changes were made to this repository's wiki:

• defacing spam has been removed

• the wiki has been disabled, as it was not used

These were made as the result of a recent automated defacement of publically writeable wikis.

treat `foo.bind(something).bar()` similar to `foo.bar()`

We can't do full type detection and it's (hard?) for us to detect stuff like

let hiding = eval
hiding(foo);

in fact, we don't think we ought to do so across the board as we're optimizing for well-intended code that is able to pass code review preferably.

Missing sinks

We are missing some sinks here I think:

partial interface DOMParser {
    Document parseFromString(HTMLString str, SupportedType type);
};

partial interface HTMLIFrameElement {
     DOMString srcdoc;
};

Source: https://github.com/WICG/trusted-types/blob/master/README.md

I'm not sure how feasible it would be to check for the HTMLIFrameElement.

/cc @mozfreddyb

Unsupported Callee for CallExpression

const ShippingContainer = Loadable({
    loader: () => import('../panels/shipping/ShippingContainer'),
    loading: LoadingComponent,
});

It doesn't like dynamic imports 👎

[meta] Overarching code audit of differences

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;
  }
};

investigate jsdoc in travis

we document functions, but only mostly, not always.
@jonathanKingston did this well for most of the functions he added, it would be a shame if we could not bind ourselves to that behavior.

new "esnext" expression means new bypass?

investigate future proofing!

investigate jquery api / jsx support

rewrite allowexpression part to not require parent node

Unexpected Callee: `AwaitExpression`

Getting another:

Unexpected Callee. Please report a minimal code snippet to the developers

(For no-unsanitized/method)

(async () => {
(await prom)();
})();

Thanks!

Fix or remove CHANGELOG.md

Not a big fan of change logs, to be honest, I just realized we had one and it's oudated.
We should either get it updated (maybe automatically, that'd be cool) or kill it.

Reduce code-duplication when checking function calls on right-hand side

When we check the expression on the right side of the innerHTML assignment (or the parameter within insertAdjacentHTML), we have some duplicate that looks for function names when the code contains a function call (see https://github.com/mozfreddyb/eslint-plugin-no-unsafe-innerhtml/blob/3a0ff0635a589b91b3dae3cf6affc56737ac90cb/lib/rules/no-unsafe-innerhtml.js#L66-L78).

We should remove the duplicate code and make it a bit simpler (and thus easier to read).

(@LockeLamora If you are looking to further contribute to this, please let me know :-))

Update documentation to include `createContextualFragment`

Discussed and implemented in #79, #80. But never implemented.

Should warn for use of createContextualFragment

What it says on the tin. Could probably base this off whatever we do for insertAdjacentHTML.

build demo to make contributions (and testing) easier

Would be nice if there was a demo similar to the ESLint demo.

It just so happened that ESLint on the browser is not a supported setup and they explicitly say that they don't want to support or maintain their demo setup, so copying it is discouraged.

Unsupported Callee for CallExpression (SequenceExpression)

Hi,

In order to examine the safety of dist code, which is often Babelified, one comes across numerous comma-operator expressions such as these which gives an "Unexpected Callee" message in eslint-plugin-no-unsanitized:

  const puncts = (0, _mainUmd.RegExtras)(sentenceEndGrouping).map(txt, punct => {
    return punct;
  });

The error is triggered as the above (0, ...) code is a SequenceExpression which is not a handled callee node type in the plugin's method.js file.

The reason for this approach is well-explained in the answers at https://stackoverflow.com/questions/32275135/why-does-babel-rewrite-imported-function-call-to-0-fn , namely to ensure this is set to a global (allowing Babel to use some namespacing without calling on said namespace).

While unlikely to be generated by Babel (e.g., the bind below which makes it work is not added by Babel), I do expect this would need to be handled as one could technically have this functional code and which could get through:

(0, document.body.insertAdjacentHTML.bind(document.body))('beforebegin', '<b>Boo</b>')

Consider allowing method name checks over getSource check

Currently the code checks for context.getSource(node.callee) for write and writeln but node.callee.property.name for insertAdjacentHTML.

This might be a very breaking code change to add to make write use the property.name as some code bases use this method name. Making this change however will catch any other reference to document using the write method which is somewhat a common pattern in non-malice™ code:

let d = document;
d.writeln("blah");
d.writeln("blah");
d.writeln("blah");
d.writeln("blah");

Without this making the code configurable will be harder #29

latest commit broke export

After installing and running I get the following.

$ /home/lutostag/src/webextension-formsave/node_modules/.bin/standard --plugin no-unsanitized                                                                                                                                    
standard: Unexpected linter output:
                           
Error: Failed to load plugin no-unsanitized: Cannot find module './lib/rules/no-unsanitized'

Latest commit broke index.js export by removing the lib/rules/no-unsanitized.js file.
HEAD~ does not have this problem.

rewrite export statement

the new eslint thing seems to be different from what we do.
I'm agnostic to those changes, but @jonathanKingston suggested.

investigate async/await

as noted by @jonathanKingston in #56

Migration to a new repo

Proposal

• Move this repo under https://github.com/mozilla/
• Make breaking changes to splitting properties, rules and config changes before submission
  • Create rules for each type: no-unsanitized/property, no-unsanitized/method, no-unsanitized/jsx
• Rename to be not specific about innerHTML. Proposed name: "eslint-plugin-no-unsanitized"

Justification

• Moving under a Mozilla namespace will gain recognition and maintainers
• Moving names solves ambiguity issues
• Allows us to expand beyond DOM sanitization, configurability would allow SVG, React, Glimmer etc level escaping specific to a framework
• Solves configuration bloat which would get developers linting incorrectly or choosing to disable a rule on an existing repo (#29)
• Allows other expansion for anything regarding catching sanitisation under other rules, for example
  • This also allows me to shut off this plugin to consolidate users, contributors etc
• Keeping all three proposed subrules under the same plugin means the code can share a lot of the same checks (which it currently does)
• no-unsanitized
  • matches the no prefix used by native rules: http://eslint.org/docs/rules/
  • unsanitized because native methods use en-us
  • Isn't specific to DOM

Further tasks

• Provide migration advice on this repo of how to configure using the exact same setup as this currently does.
• Publish npm
• Alert all dependencies tracked on npm
• Inform 18f of the move
• Ask this author for an update too
• Write a hacks post about the changes
• Migrate outstanding issues over

CC other parties involved in this plugin: @EnTeQuAk, @KevinGrandon, @LockeLamora, @marumari

I'm willing to do all of this in my own time, the only change I can't make is creating the repo on Mozilla org with the right permissions for maintainers etc.

we have hardcoded sanitizers, that likely nobody uses. We should move them to defaultRulesCheck empty object instead

See https://github.com/mozilla/eslint-plugin-no-unsanitized/blob/e3efa339e8a0aa1e4702af42db85008a20d8ea2c/lib/ruleHelper.js#L9-10

// names of escaping functions that we acknowledge
const VALID_ESCAPERS = ["Sanitizer.escapeHTML", "escapeHTML"];
const VALID_UNWRAPPERS = ["Sanitizer.unwrapSafeHTML", "unwrapSafeHTML"];

when instead we should remove it here and put it into the default configuration.

People can then override it and have better control of what they actually want to allow
This would not be a breaking change.

I've also noticed that we dont have tests for custom escapers (which is probably a super common use case 😱 )

CODE_OF_CONDUCT.md file missing

As of January 1 2019, Mozilla requires that all GitHub projects include this CODE_OF_CONDUCT.md file in the project root. The file has two parts:

1. Required Text - All text under the headings Community Participation Guidelines and How to Report, are required, and should not be altered.
2. Optional Text - The Project Specific Etiquette heading provides a space to speak more specifically about ways people can work effectively and inclusively together. Some examples of those can be found on the Firefox Debugger project, and Common Voice. (The optional part is commented out in the raw template file, and will not be visible until you modify and uncomment that part.)

If you have any questions about this file, or Code of Conduct policies and procedures, please see Mozilla-GitHub-Standards or email [email protected].

(Message COC001)

return false explicitly in...

https://github.com/mozfreddyb/eslint-plugin-no-unsafe-innerhtml/blob/0f2a40a75c6d7715019247224a00d221806becf6/lib/rules/no-unsafe-innerhtml.js#L78

un-hardcode property names

bump eslint to e.g., 6.x

make name of escape functions configurable (defaulting to the existing ones)

Comment code removal

As ESLint has it's own way of removing current and next lines via comments I would move to deprecate code specific to Bugzilla for turning off the code.

https://github.com/mozfreddyb/eslint-plugin-no-unsafe-innerhtml/blob/master/lib/rules/no-unsafe-innerhtml.js#L88

we can't properly look into assignment expressions

       code: "(Async.checkAppReady = document.writeln)(n);",

Shouldnt pass. This is a follow-up of #60.

Do we need to scan ternary or logical expressions?

Examples:

let endTime = (mapEnd || (e => e.delta))(this._data[this._data.length - 1]);

and

(text.endsWith("\n") ? document.write : document.writeln)(text)

Would be interesting if we could deep-dive into left/right attributes of the LogicalExpression (or the consequent/alternate attributes of the ConditionalExpression respectively)

add test for custom escaper (template strings)

as an aside from #107, we dont have tests for custom escapers that accept tempalte strings.

investigate linting

https://github.com/mozfreddyb/eslint-plugin-no-unsafe-innerhtml/blob/0f2a40a75c6d7715019247224a00d221806becf6/lib/rules/no-unsafe-innerhtml.js#L76

Consider renaming based upon extending functionality

I cyber-squated on eslint-plugin-no-unescaped for the reason I wanted customisation for methods, assignment, string assignment and potentially other checks too.

If the customisation and the separate rule for checking for key assignment were rolled in here then it could be worth renaming the extension all together.

I know this somewhat defeats the point of rolling the code in here (I have no desire to maintain this longterm anyway and ideally would push to have it under a Mozilla GitHub really) to prevent stranded users of this npm however I am pretty convinced we will need to do a breaking change anyway.

Anyway just thought I would raise the discussion point :).

Add links in README to other docs pages

As a follow up to #65 we should add links to all the sub pages of documentation.

So README would link to property and method pages so users find it easy to discover them.

Make a new rule "innerText" property when append script tag?

var script = document.createElement('script');
script.innerText = attack_var;
document.body.append(script);

I thought innerText could be harmful in this case when attack_var is unsanitized .

Add more explanation of default args to docs

Follow up to #65

There is likely a lot of expansion we could add to the customisation and also how to disable the defaults. I know for example we made it so you can override the sanitizers safely without having to disable the default innerHTML checks for example.

moar property checking?

More checking for props over this being explicit

support es2016+ syntax

(depending on nodejs compat and nodejs uptake)