Enable ESLint for dom/crypto

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P2
normal
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: standard8, Assigned: klymenkodp)

Tracking

(Blocks 1 bug)

Trunk
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 wontfix, firefox66 fixed)

Details

(Whiteboard: [seneca-eslint][domsecurity-active])

Attachments

(2 attachments)

As part of rolling out ESLint across the tree, we should enable it for dom/crypto.
I would be happy to take this issue.
Assignee: standard8 → klymenkodp
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [seneca-eslint] → [seneca-eslint][domsecurity-active]
I am confused with the following eslint error:
Function 'next' expected no return value.  consistent-return (test_WebCrypto_HKDF.html, 159:7)

This is the function:

function next() {
      if (!tests.length) {
        return;
      }

      var test = tests.shift();
      var {key} = test;

      return crypto.subtle.importKey("raw", key, "HKDF", false, ["deriveBits"])
        .then(function(baseKey) {
          return crypto.subtle.deriveBits({
            name: "HKDF",
            hash: test.prf,
            salt: test.salt,
            info: test.info,
          }, baseKey, test.data.byteLength * 8);
        })
        .then(function(data) {
          if (!util.memcmp(data, test.data)) {
            throw new Error("derived bits don't match expected value");
          }

          // Next test vector.
          return next();
        });
    }



I thought the problem was with the first conditional statement: "if (!tests.length) { return; }" 
When I remove "return;" , there is no eslint error, but the test fails.
The issue here is the inconsistent return value. So the one in the if statement returns nothing, but the main one (`return crypto.subtle.importKey`) is likely returning a promise (you can tell this due to the '.then' calls after it.

Also notice that in the last .then section, next() is called itself, iteratively.

So the !tests.length check is to end the iteration.

I think the best way to solve this is to get the if statement to return an already resolved promise:

```
if (!test.length) {
  return Promise.resolve();
}
```

That will give a consistent return value, and will complete correctly.

The only slight compromise here, is that if `tests` is empty at the start of the test, then it will no longer return undefined, and so the call below the function definition (`next().then(complete(that), error(that));`) won't fail.

To work around this, we can also add an explicit check for length just after tests is defined:

```
var tests = tv.hkdf.slice();
if (!tests.length) {
  error(that)("No tests found");
  return;
}

function next() {
...
```

Note: the error call is slightly awkward due to how the tests are constructed, but I've tested it and it does work.
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2b3549145fc3
Enable ESLint for dom/crypto/ (automatic changes) r=Standard8,Ehsan
https://hg.mozilla.org/integration/autoland/rev/04a31c0080dc
Enable ESLint for dom/crypto (manual changes) r=Standard8,Ehsan
https://hg.mozilla.org/mozilla-central/rev/2b3549145fc3
https://hg.mozilla.org/mozilla-central/rev/04a31c0080dc
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Volodymyr, thank you for all your work on this. It is now landed & incorporated into the source code, it is great to get this enabled.
You need to log in before you can comment on or make changes to this bug.