Add Jest TaggedTemplateExpression Support

[tylerkrupicka-stripe]

Closes https://github.com/mozilla/eslint-plugin-no-unsanitized/issues/154

Adds support for the Jest describe.each and similar syntaxes for declaring a series of tests as a table in a template string.

Let me know if there's anything I need to fix or improve! Thanks for maintaining the project.

[tylerkrupicka-stripe]

@mozfreddyb it looks like you were assigned in the other ticket if you want to review!

[tylerkrupicka-stripe]

@mozfreddyb Thank you for the thorough review. Before looking at customizing available tags, I tried to see if I could figure out how to still test the expressions inside of callee tagged templates. This turned out to be more straightforward than I expected, so I updated the change to do that.

I've added tests to show that expressions inside of tagged templates are checked as normal, and I ran this on a codebase that uses this Jest syntax to make sure it all functions properly.

Let me know if this is the wrong approach, or if you see any other opportunities for improvement. Thanks!

[tylerkrupicka-stripe]

@mozfreddyb thanks for merging https://github.com/mozilla/eslint-plugin-no-unsanitized/pull/156! I have merged this branch with master and verified the tests are still passing. Let me know if there's anything else I need to address!

Also, what is the usual release cadence for changes like this?

[mozfreddyb]

Let's briefly take a step back and dissect what you are allowlisting here. The code in question, describe.each`table`(...) performs two function calls

First, the tagged template expression describe.each`table` is executing the describe.each function with the template expression (and interpolations) as parameters. Secondly, the return value of that tagged template expression is then executed again.

Your patch allows based on the expressions within template expression, which is a useful addition.

However, I'm not so sure if we want to allow calling a unknown function (the return value of describe.each`table`) without any additional checks.... I know that we can't check it any further and this is mostly a shortcoming of dynamic typing and static analysis in general, but I am honestly a bit unsure how to proceed.

Do people want us to "give up checking", when we can't follow anymore? Do we want people to explicitly disable the linter for test-only code?

I understand this issue is blocking you from using thorough linting and I don't like that, but I also don't want to decide on this issue too hastily.

[mozfreddyb]

However, I'm not so sure if we want to allow calling a unknown function (the return value of describe.eachtable) without any additional checks.... I know that we can't check it any further and this is mostly a shortcoming of dynamic typing and static analysis in general, but I am honestly a bit unsure how to proceed.

I realize this sentence was missing an example case that we'd let through..

function getwrite() { return document.write }
getwrite``(evil);

On the other hand, we're not trying to protect against intentionally bad code authors...

[tylerkrupicka-stripe]

Great points! Thanks for taking a closer look at the impact for the change. It seems like all code that "disguises" the function being called will be hard to detect, for example:

const evil = '<img src=x onerror=alert("hi");>'
function getwrite() { return document.write.bind(document) };
getwrite()(evil);

Seems to pass through currently as well. I'm open to any suggestions for fixing this or documenting it better.

[mozfreddyb]

That's fair. I shall not block the work you're doing here. I'll file a follow-up issue instead.

[mozfreddyb]

Sorry this is causing another back-and-forth, but I'd like to ask you to add checks & tests for using a "disallowed" function as the callee of the tagged template expression. I.e.,

document.write`blah${evil}`

If possible, the following should still be allowed, I mean no sensible person should call that particular function as a tagged template strings. But what if a custom defined "bad function" needs to be used as a tagged template string, I guess we need to support that.

document.write`blah${'static string'}`

[tylerkrupicka-stripe]

@mozfreddyb thanks again for pointing this out, I had to do a bit of research to determine possible risk in the format you identified. It's not completely straightforward but I think I handled it in the safest way possible, so I'll try to document my findings below.

So one thing that is not intuitive about the TaggedTemplate syntax, is that the template string itself is not passed as the argument to the function.

In your examples:

document.write`blah${evil}`;
// gets passed as
document.write( ['blah', ''], evil );

document.write`blah${'static string'}`
// gets passed as
document.write( ['blah', ''], 'static string' );

Here's another sanity check of that behavior in Firefox:

Because of this behavior there's actually very little risk, because the first argument ends up being the static string parts of the template, not the embedded variables. It will also just cause weird behaviors with the functions which probably aren't expecting arrays. That being said, in the case of:

node.insertAdjacentHTML`text ${variable}`
// gets passed as
node.insertAdjacentHTML( ['text', ''],  variable);

The variable does end up in the "injected" portion of the function parameter, with the caveat that it seems to also cause an understandable DOMException because the first parameter is an array.

In order to properly model these I had to also check TaggedTemplateExpression nodes, and pull out the parameters into the format they will end up in. That way, the rule will only error if there is actually a risk that a variable will end up in the XSS vulnerable argument to one of the functions. This probably won't be used with the browser XSS functions much in practice, but I also added tests for custom functions which could occur.

// passes
{
  code: "custom`text ${'string'}`",
  options: [
      {},
      {
          "custom": {
              "properties": [1]
          }
      }
  ]
},
// does not
{
  code: "custom`text ${evil}`",
  options: [
      {},
      {
          "custom": {
              "properties": [1]
          }
      }
  ]
},

I don't think my changes added too much complexity, but let me know if you see any opportunity for improvement. It definitely required me to learn a bit more about the internals of the argument checking code. I can't imagine anyone would (or should) ever call the HTML insertion functions with a tagged template, but this plugin should now be able to reason about the safety of doing so, along with the safety of calling custom functions this way.

Thanks again!

• More info about the babel template nodes

[mozfreddyb]

Wow, yeah. This was getting rather odd and funky. I realize I could have been a bit more explicit about how parameters get passed into tagged template expressions and that the document.write case is certainly beyond what's realistically done, but hey, you solved it rather nicely :-)

Thank you for another pass!

[tylerkrupicka-stripe]

@mozfreddyb Thank you for the help!

[mozfreddyb]

See https://github.com/mozilla/eslint-plugin-no-unsanitized/pull/162