Closed
Bug 1508825
Opened 6 years ago
Closed 6 years ago
Enable ESLint for dom/crypto
Categories
(Core :: DOM: Security, enhancement, P2)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla66
People
(Reporter: standard8, Assigned: klymenkodp)
References
Details
(Whiteboard: [seneca-eslint][domsecurity-active])
Attachments
(2 files)
As part of rolling out ESLint across the tree, we should enable it for dom/crypto.
Assignee | ||
Comment 1•6 years ago
|
||
I would be happy to take this issue.
Reporter | ||
Updated•6 years ago
|
Assignee: standard8 → klymenkodp
Updated•6 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [seneca-eslint] → [seneca-eslint][domsecurity-active]
Assignee | ||
Comment 2•6 years ago
|
||
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.
Reporter | ||
Comment 3•6 years ago
|
||
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.
Assignee | ||
Comment 4•6 years ago
|
||
Assignee | ||
Comment 5•6 years ago
|
||
Depends on D13693
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
Comment 7•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2b3549145fc3
https://hg.mozilla.org/mozilla-central/rev/04a31c0080dc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox66:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Reporter | ||
Comment 8•6 years ago
|
||
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.
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•