Skip to content

Commit

Permalink
Introduce new configuration 'variableTracing', default to disabled. A…
Browse files Browse the repository at this point in the history 
…dding tests for both states (fix #178)
  • Loading branch information
@mozfreddyb
mozfreddyb committed Sep 28, 2021
1 parent 4e5ba48 commit 07c343c
Show file tree
Hide file tree
Showing 4 changed files with 139 additions and 7 deletions.
21 changes: 20 additions & 1 deletion docs/rules/customization.md
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,31 @@
# Customization

## Variable Tracing

The plugin allows a limit back-tracing of variables.
This will be used to check code like here:
```js
const greeting_template = `<p>Hello World!</p>`;
// ... lots of other code in between ...
someElemenet.innerHTML = greeting_template;
```

Currently, backtracing will only allow const and let variables that contain string literals only.
Further assignments to these variables will also be checked for validation.

**Backtracing can be disabled by setting the boolean
option `variableTracing` to `false`.**

Both values are supported and tested in CI.

## Customization Examples
You can customize the way this rule works in various ways.
* Add to the list of properties or functions to be checked for potentially
dangers variable input
* Add to the list of allowed escaping functions to mitigate security concerns
* Besides adding to the list, you may override the defaults and provide an exhaustive list yourself

## Examples

### Disallow the `html` function by specifically checking input for the first function parameter
```json
{
Expand Down
13 changes: 13 additions & 0 deletions lib/ruleHelper.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ RuleHelper.prototype = {

/**
* Check if an identifier is allowed
* - only if variableTracing is enabled in the first place.
* - find its declarations and see if it's const or let
* - if so, allow if the declaring statement is an allowed expression
* - ensure that following assignments to that identifier are also allowed
Expand All @@ -114,6 +115,12 @@ RuleHelper.prototype = {
* @returns {boolean} Returns whether the Identifier is deemed safe.
*/
isAllowedIdentifier(expression, escapeObject, details) {

// respect the custom config property `variableTracing`:
if (!this.ruleChecks["variableTracing"]) {
return false;
}

// find declared variables and see which are literals
const scope = this.context.getScope(expression);
const variableInfo = scope.set.get(expression.name);
Expand Down Expand Up @@ -330,6 +337,12 @@ RuleHelper.prototype = {
childRuleChecks = Object.assign({}, defaultRuleChecks, childRuleChecks);
}

// default to no backtracing.
ruleCheckOutput["variableTracing"] = false;
if ("variableTracing" in parentRuleChecks) {
ruleCheckOutput["variableTracing"] = !!parentRuleChecks["variableTracing"];
}

// If we have defined child rules lets ignore default rules
Object.keys(childRuleChecks).forEach((ruleCheckKey) => {
// However if they have missing keys merge with default
Expand Down
30 changes: 30 additions & 0 deletions tests/rules/method.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -833,6 +833,11 @@ eslintTester.run("method", rule, {
type: "CallExpression"
}
],
options: [
{
variableTracing: true
}
]
},
{
code: "let copies = evil; n.insertAdjacentHTML('beforebegin', copies);",
Expand All @@ -842,6 +847,11 @@ eslintTester.run("method", rule, {
type: "CallExpression"
}
],
options: [
{
variableTracing: true
}
],
parserOptions: { ecmaVersion: 6 }
},
{
Expand All @@ -852,6 +862,11 @@ eslintTester.run("method", rule, {
type: "CallExpression"
}
],
options: [
{
variableTracing: true
}
],
parserOptions: { ecmaVersion: 6 }
},

Expand All @@ -864,6 +879,11 @@ eslintTester.run("method", rule, {
type: "CallExpression"
}
],
options: [
{
variableTracing: true
}
],
parserOptions: { ecmaVersion: 6 }
},

Expand All @@ -876,6 +896,11 @@ eslintTester.run("method", rule, {
type: "CallExpression"
}
],
options: [
{
variableTracing: true
}
],
parserOptions: { ecmaVersion: 6 }
},

Expand All @@ -888,6 +913,11 @@ eslintTester.run("method", rule, {
type: "CallExpression"
}
],
options: [
{
variableTracing: true
}
],
parserOptions: { ecmaVersion: 6 }
},
{
Expand Down
82 changes: 76 additions & 6 deletions tests/rules/property.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,27 +210,57 @@ eslintTester.run("property", rule, {
// support for variables that are declared elsewhere
{
code: "let literalFromElsewhere = '<b>safe</b>'; y.innerHTML = literalFromElsewhere;",
parserOptions: { ecmaVersion: 6 }
parserOptions: { ecmaVersion: 6 },
options: [
{
variableTracing: true
}
],
},
{
code: "const literalFromElsewhereWithInnerExpr = '<b>safe</b>'+'yo'; y.innerHTML = literalFromElsewhereWithInnerExpr;",
parserOptions: { ecmaVersion: 6 }
parserOptions: { ecmaVersion: 6 },
options: [
{
variableTracing: true
}
],
},
{
code: "let multiStepVarSearch = '<b>safe</b>'+'yo'; const copy = multiStepVarSearch; y.innerHTML = copy;",
parserOptions: { ecmaVersion: 6 }
parserOptions: { ecmaVersion: 6 },
options: [
{
variableTracing: true
}
],
},
{
code: "let copies = '<b>safe</b>'; copies = 'stillOK'; y.innerHTML = copies;",
parserOptions: { ecmaVersion: 6 }
parserOptions: { ecmaVersion: 6 },
options: [
{
variableTracing: true
}
],
},
{
code: "let copies = '<b>safe</b>'; if (monday) { copies = 'stillOK'; }; y.innerHTML = copies;",
parserOptions: { ecmaVersion: 6 }
parserOptions: { ecmaVersion: 6 },
options: [
{
variableTracing: true
}
],
},
{
code: "let msg = '<b>safe</b>'; let altMsg = 'also cool'; if (monday) { msg = altMsg; }; y.innerHTML = msg;",
parserOptions: { ecmaVersion: 6 }
parserOptions: { ecmaVersion: 6 },
options: [
{
variableTracing: true
}
],
},
],

Expand Down Expand Up @@ -574,6 +604,11 @@ eslintTester.run("property", rule, {
type: "AssignmentExpression"
}
],
options: [
{
variableTracing: true
}
],
parserOptions: { ecmaVersion: 6 }
},
{
Expand All @@ -584,6 +619,11 @@ eslintTester.run("property", rule, {
type: "AssignmentExpression"
}
],
options: [
{
variableTracing: true
}
],
parserOptions: { ecmaVersion: 6 }
},
{
Expand All @@ -594,6 +634,11 @@ eslintTester.run("property", rule, {
type: "AssignmentExpression"
}
],
options: [
{
variableTracing: true
}
],
parserOptions: { ecmaVersion: 6 }
},
{
Expand All @@ -609,6 +654,11 @@ eslintTester.run("property", rule, {
type: "AssignmentExpression"
}
],
options: [
{
variableTracing: true
}
],
parserOptions: { ecmaVersion: 6 }
},
{
Expand All @@ -623,8 +673,28 @@ eslintTester.run("property", rule, {
type: "AssignmentExpression"
}
],
options: [
{
variableTracing: true
}
],
parserOptions: { ecmaVersion: 6 }
},
{
code: "let msg = '<b>safe</b>'; let altMsg = 'also cool'; if (monday) { msg = altMsg; }; y.innerHTML = msg;",
parserOptions: { ecmaVersion: 6 },
errors: [
{
message: /Unsafe assignment to innerHTML/,
type: "AssignmentExpression"
}
],
options: [
{
variableTracing: false
}
],
},
{
code: "a.innerHTML §= b;",
errors: [
Expand Down

0 comments on commit 07c343c

Please sign in to comment.