revisit configurability options and defaults

[mozfreddyb]

No description provided.

[jonathanKingston]

When using the soon to be deprecated https://github.com/jonathanKingston/eslint-plugin-no-unescaped I have the ability to create classes of checks, there isn't a prefect setup for this as I also need the same configuration for my somewhat unrelated rule: https://github.com/jonathanKingston/eslint-plugin-no-unescaped/blob/master/lib/rules/no-key-assignment.js

The idea of this setup is for something like:

"no-unescaped/enforce": ["error",
    {
      html: {
        taggedTemplates: ["escaped"],
        methods: ["escapeHTML"]
      },
      jQuery: {
        taggedTemplates: ["escapejQuery"],
        methods: ["$.escape"]
      },
    },
    {
      properties: {
        innerHTML: {
          type: "html"
        },
        outerHTML: {
          type: "html"
        },
      },
      methods: {
        html: {
          type: "jQuery",
          properties: [0]
        },
        insertAdjacentHTML: {
          type: "html",
          properties: [1]
        },
        writeln: {
          type: "html",
          properties: [0]
        },
        write: {
          type: "html",
          properties: [0]
        },
        createContextualFragment: {
          type: "html",
          properties: [0]
        }
      }
    }
  ]

This gives an author of a large project the flexibility to have many different types of escaping without the ability to mix up escaping techniques by mistake (which I saw from fxos happened at least once).

What do you think @mozfreddyb?

[mozfreddyb]

I want this simple but I also want this secure. I am torn.
My argument against doing that in the past was that a good enough escaper (especially with template strings!) can get the context of the parameters and perform its escaping in a context-sensitive way.
There's a nice demo called safetemplate in https://github.com/mikesamuel/sanitized-jquery-templates to showcase this.

[mozfreddyb]

Note to freddy, that he might want to look at https://www.nczonline.net/blog/2012/08/01/a-critical-review-of-ecmascript-6-quasi-literals/ and http://stackoverflow.com/questions/5395813/auto-escape-to-prevent-xss#5399435 as a follow-up from this.

[jonathanKingston]

@mozfreddyb because of the different subsystems like react I worry several things:

• Users will want to use tagged template strings for different use cases
• Because you can't chain them a developer might want to ensure all methods are safe in each system, context

So you might end up with methods like: i18n, escapeHTML, escapeJSX which might not be compatible in each code base.

Perhaps we can come up with a better way which would allow this level of specificity but also simple by default.

[mozfreddyb]

Hm yes. React is a "problem" where every can mean something we don't know. That's one of the reasons I wanted JSX to live in another plugin 😇

[jonathanKingston]

I'm not sure if it is really any different, certainly a lot of the code would be shared.

So the options we have are:

• Escaping
  • Tagged template
  • Method
• Supported checks
  • Methods
    • Optional map of objects to restrict on (Eg: document, element)
    • Property id
  • Properties
    • Optional map of objects to restrict on (Eg: document, element)

The thing is methods, properties and JSXAttribute each could be their own rule which would likely get rid of my fear of the need for different checks. The plugin could could share all the same code with each having their own config like:

"no-unescaped/property": ["error",
    {
      escape: {
        taggedTemplate: ["escaped"],
        method: ["escapeHTML"]
      }
      properties: {
        innerHTML: {},
        outerHTML: {
          // this could add granularity and override the parent escape
          escape: {
            taggedTemplate: ["myThing"]
          },
          objects: ["document"]
        }
      }
  }
],
"no-unescaped/method": ["error",
  {
    escape: {
      taggedTemplates: ["escaped"],
      methods: ["escapeHTML"]
    },
    methods: {
      html: {
        properties: [0]
      },
      insertAdjacentHTML: {
        properties: [1]
      },
      writeln: {
        properties: [0]
      },
      write: {
        objects: ["document", "aDocument", "iframeDocument"],
        properties: [0]
      },
      createContextualFragment: {
        properties: [0]
      }
    }
  ]

By making `no-unescaped/property.properties" an object each key could be extended in the future, we could also accept an array of keys here too.

Separating out the code paths for each rule shouldn't slow down ESLint much as they are different rules internally for each method, property and JSX.

[jonathanKingston]

Reconsidered default with two rules rather than one:

"no-unescaped/property": ["error",
    {
      escape: {
        taggedTemplates: ["Sanitizer.escapeHTML", "escapeHTML"],
        methods: ["Sanitizer.unwrapSafeHTML", "unwrapSafeHTML"]
      }
      properties: {
        innerHTML: {},
        outerHTML: {}
      }
  }
],
"no-unescaped/method": ["error",
  {
    escape: {
      taggedTemplates: ["Sanitizer.escapeHTML", "escapeHTML"],
      methods: ["Sanitizer.unwrapSafeHTML", "unwrapSafeHTML"]
    },
    methods: {
      insertAdjacentHTML: {
        properties: [1]
      },
      writeln: {
        objectFilters: ["document"],
        properties: [0]
      },
      write: {
        objectFilters: ["document"],
        properties: [0]
      },
      createContextualFragment: { // Currently this isn't checked at all
        properties: [0]
      }
    }
  ]

objectFilters ['?document', '.*frame.*'] // list of regexes, parsed with |new RegExp(yourString, 'ui');

The only thing that annoys me somewhat is the inability of overriding the methods without touching the default properties/methods.

Maybe it could be {config}, {filters}:

"no-unescaped/property": ["error",
    {
      escape: {
        taggedTemplates: ["Sanitizer.escapeHTML", "escapeHTML"],
        methods: ["Sanitizer.unwrapSafeHTML", "unwrapSafeHTML"]
      }
  }, {
      innerHTML: {},
      outerHTML: {}
  }
],
"no-unescaped/method": ["error",
  {
    escape: {
      taggedTemplates: ["Sanitizer.escapeHTML", "escapeHTML"],
      methods: ["Sanitizer.unwrapSafeHTML", "unwrapSafeHTML"]
    },
  }, {
      insertAdjacentHTML: {
        properties: [1]
      },
      writeln: {
        objectFilters: ["document"],
        properties: [0]
      },
      write: {
        objectFilters: ["document"],
        properties: [0]
      },
      createContextualFragment: { // Currently this isn't checked at all
        properties: [0]
      }
    }
  ]

As most users are unlikely want to change the second, it allows us to provide the safest default. Maybe object filters should be part of the first config too with an optional local override (this would weaken the default insertAdjacentHTML check though).

Schema-ish:

"$pluginname/rulename": ["errorlevel",
  { // optional would use default config for all default ruleChecks
    // optional, omitting or length 0 would be considered ".*"
    objectFilters: ["stringRegex"],
    escape: {  // optional, if omitted would disallow use of matched "$ruleCheckName" prop/method
      taggedTemplates: ["taggedTemplateFunctionName"], // optional: Tagged template permitted as arguments/assignments
      methods: ["MethodName"] // optional: method permitted as arguments/assignments
    }
  },
  "$ruleChecks": { // "methods"/"properties" optional would use default ruleChecks
     "$ruleCheckName": {
       // optional, same as above and overrides parent even if blank array
       objectFilters: ["stringRegex"],
       // optional, same as above, overrides parent even if blank object
       escape: {...}
       // indices to check for $ruleChecks="methods"
       properties: [...]
     }
     ...
   }
]