Skip to content

[WebCryptoAPI] Add tests for argument read order #57614

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
twiss merged 2 commits into from
Feb 26, 2026
Merged

[WebCryptoAPI] Add tests for argument read order #57614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
56594eb
[WebCryptoAPI] Add tests for argument read order
twiss Feb 6, 2026
ee59982
Restore data rather than flipping it again in argument name getter
twiss Feb 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 35 additions & 19 deletions WebCryptoAPI/digest/cshake.tentative.https.any.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,26 +171,42 @@ Object.keys(digestedData).forEach(function (alg) {
});
}, alg + ' with ' + length + ' bit output and ' + size + ' source data');

promise_test(function (test) {
var buffer = new Uint8Array(sourceData[size]);
return crypto.subtle
.digest({ name: alg, length: length }, buffer)
.then(function (result) {
// Alter the buffer after calling digest
if (buffer.length > 0) {
if (sourceData[size].length > 0) {
promise_test(function (test) {
Copy link
Copy Markdown
Member

@annevk annevk Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to reformat anyway, might as well adopt async/await?

Copy link
Copy Markdown
Member Author

@twiss twiss Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't really reformat anything here, I just added an if around it. I can refactor this and other tests in a followup.

var buffer = new Uint8Array(sourceData[size]);
Copy link
Copy Markdown
Member

@annevk annevk Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const

// Alter the buffer before calling digest
buffer[0] = ~buffer[0];
return crypto.subtle
.digest({
get name() {
// Alter the buffer back while calling digest
buffer[0] = sourceData[size][0];
return alg;
},
Comment on lines +181 to +185
Copy link
Copy Markdown
Contributor

@panva panva Feb 16, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this and all the others using the same pattern truly alter the buffer back instead of continuously switching it back and forth? Otherwise it's dependant on that the name is only accessed once.

Copy link
Copy Markdown
Member Author

@twiss twiss Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, interesting question. It's true that it wasn't my intention for this test to check that name is accessed only once, however, I do think WebIDL and WebCrypto are defined as such, because the property is accessed once in step 4 of normalizing an algorithm, and then never thereafter. So I think it should be fine to rely on this?

Then again, it might make it harder to debug what's the cause of this test failing if it does, and of course this is not quite a proper test of the property being accessed once as it would also pass if it's accessed three times :D So, we could also split that up, if we think the latter is worth testing.

Copy link
Copy Markdown
Contributor

@panva panva Feb 16, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might make it harder to debug what's the cause of this test failing if it does

Exactly where i'm coming from. I'm not interested in testing that algorithm properties are only accessed once. Nor in the order itself FWIW but here we are. Optimizations that we have in Node.js rule out this proposed test pattern.

Copy link
Copy Markdown
Member

@annevk annevk Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well we might need a separate test then because it sounds like Node.js might not have a conforming bindings implementation?

Copy link
Copy Markdown
Contributor

@panva panva Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All i'm asking is that the getter "switches back" instead of "switching back and forth".

Copy link
Copy Markdown
Member Author

@twiss twiss Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I've updated the tests. We can separately add more tests for the bindings / argument normalization later.

Copy link
Copy Markdown
Member

@annevk annevk Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand what you're asking for, but since you're supposed to only call these once and in order, we'd be hiding a bug of sorts by removing coverage for that.

length
}, buffer)
.then(function (result) {
assert_true(
equalBuffers(result, digestedData[alg][length][size]),
'digest matches expected'
);
});
}, alg + ' with ' + length + ' bit output and ' + size + ' source data and altered buffer during call');

promise_test(function (test) {
var buffer = new Uint8Array(sourceData[size]);
return crypto.subtle
.digest({ name: alg, length: length }, buffer)
.then(function (result) {
// Alter the buffer after calling digest
buffer[0] = ~buffer[0];
Copy link
Copy Markdown
Member

@annevk annevk Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also alter it immediately after calling digest. In fact, that would be the superior test as this would be redundant with that.

Copy link
Copy Markdown
Member Author

@twiss twiss Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this test is wrong, I fixed that in #57616.

}
assert_true(
equalBuffers(result, digestedData[alg][length][size]),
'digest matches expected'
);
});
}, alg +
' with ' +
length +
' bit output and ' +
size +
' source data and altered buffer after call');
assert_true(
equalBuffers(result, digestedData[alg][length][size]),
'digest matches expected'
);
});
}, alg + ' with ' + length + ' bit output and ' + size + ' source data and altered buffer after call');
}
});
});
});
Expand Down
44 changes: 31 additions & 13 deletions WebCryptoAPI/digest/digest.https.any.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,19 +83,37 @@
return promise;
}, mixedCase + " with " + size + " source data");

promise_test(function(test) {
var copiedBuffer = copyBuffer(sourceData[size]);
var promise = subtle.digest({name: upCase}, copiedBuffer)
.then(function(result) {
assert_true(equalBuffers(result, digestedData[alg][size]), "digest() yielded expected result for " + alg + ":" + size);
}, function(err) {
assert_unreached("digest() threw an error for " + alg + ":" + size + " - " + err.message);
});

copiedBuffer[0] = 255 - copiedBuffer;
return promise;
}, upCase + " with " + size + " source data and altered buffer after call");

if (sourceData[size].length > 0) {
promise_test(function(test) {
var copiedBuffer = copyBuffer(sourceData[size]);
copiedBuffer[0] = 255 - copiedBuffer[0];
var promise = subtle.digest({
get name() {
copiedBuffer[0] = sourceData[size][0];
return upCase;
}
}, copiedBuffer)
.then(function(result) {
assert_true(equalBuffers(result, digestedData[alg][size]), "digest() yielded expected result for " + alg + ":" + size);
}, function(err) {
assert_unreached("digest() threw an error for " + alg + ":" + size + " - " + err.message);
});
return promise;
}, upCase + " with " + size + " source data and altered buffer during call");

promise_test(function(test) {
var copiedBuffer = copyBuffer(sourceData[size]);
var promise = subtle.digest({name: upCase}, copiedBuffer)
.then(function(result) {
assert_true(equalBuffers(result, digestedData[alg][size]), "digest() yielded expected result for " + alg + ":" + size);
}, function(err) {
assert_unreached("digest() threw an error for " + alg + ":" + size + " - " + err.message);
});

copiedBuffer[0] = 255 - copiedBuffer[0];
return promise;
}, upCase + " with " + size + " source data and altered buffer after call");
}
});
});

Expand Down
44 changes: 32 additions & 12 deletions WebCryptoAPI/digest/sha3.tentative.https.any.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,19 +109,39 @@ Object.keys(sourceData).forEach(function (size) {
});
}, alg + ' with ' + size + ' source data');

promise_test(function (test) {
var buffer = new Uint8Array(sourceData[size]);
return crypto.subtle.digest(alg, buffer).then(function (result) {
// Alter the buffer after calling digest
if (buffer.length > 0) {
if (sourceData[size].length > 0) {
promise_test(function (test) {
var buffer = new Uint8Array(sourceData[size]);
// Alter the buffer before calling digest
buffer[0] = ~buffer[0];
return crypto.subtle
.digest({
get name() {
// Alter the buffer back while calling digest
buffer[0] = sourceData[size][0];
return alg;
}
}, buffer)
.then(function (result) {
assert_true(
equalBuffers(result, digestedData[alg][size]),
'digest matches expected'
);
});
}, alg + ' with ' + size + ' source data and altered buffer during call');

promise_test(function (test) {
var buffer = new Uint8Array(sourceData[size]);
return crypto.subtle.digest(alg, buffer).then(function (result) {
// Alter the buffer after calling digest
buffer[0] = ~buffer[0];
}
assert_true(
equalBuffers(result, digestedData[alg][size]),
'digest matches expected'
);
});
}, alg + ' with ' + size + ' source data and altered buffer after call');
assert_true(
equalBuffers(result, digestedData[alg][size]),
'digest matches expected'
);
});
}, alg + ' with ' + size + ' source data and altered buffer after call');
}
});
});

Expand Down
74 changes: 69 additions & 5 deletions WebCryptoAPI/encrypt_decrypt/aes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,38 @@ function run_test() {
all_promises.push(promise);
});

// Check for successful encryption even if the buffer is changed while calling encrypt.
passingVectors.forEach(function(vector) {
var plaintext = copyBuffer(vector.plaintext);
plaintext[0] = 255 - plaintext[0];
var promise = importVectorKey(vector, ["encrypt", "decrypt"])
.then(function(vector) {
promise_test(function(test) {
var operation = subtle.encrypt({
...vector.algorithm,
get name() {
plaintext[0] = vector.plaintext[0];
return vector.algorithm.name;
}
}, vector.key, plaintext)
.then(function(result) {
assert_true(equalBuffers(result, vector.result), "Should return expected result");
}, function(err) {
assert_unreached("encrypt error for test " + vector.name + ": " + err.message);
});
return operation;
}, vector.name + " with altered plaintext during call");
}, function(err) {
// We need a failed test if the importVectorKey operation fails, so
// we know we never tested encryption
promise_test(function(test) {
assert_unreached("importKey failed for " + vector.name);
}, "importKey step: " + vector.name + " with altered plaintext during call");
});

all_promises.push(promise);
});

// Check for successful encryption even if the buffer is changed after calling encrypt.
passingVectors.forEach(function(vector) {
var plaintext = copyBuffer(vector.plaintext);
Expand All @@ -49,13 +81,13 @@ function run_test() {
});
plaintext[0] = 255 - plaintext[0];
return operation;
}, vector.name + " with altered plaintext");
}, vector.name + " with altered plaintext after call");
}, function(err) {
// We need a failed test if the importVectorKey operation fails, so
// we know we never tested encryption
promise_test(function(test) {
assert_unreached("importKey failed for " + vector.name);
}, "importKey step: " + vector.name + " with altered plaintext");
}, "importKey step: " + vector.name + " with altered plaintext after call");
});

all_promises.push(promise);
Expand Down Expand Up @@ -84,7 +116,39 @@ function run_test() {
all_promises.push(promise);
});

// Check for successful decryption even if ciphertext is altered.
// Check for successful decryption even if ciphertext is altered while calling encrypt.
passingVectors.forEach(function(vector) {
var ciphertext = copyBuffer(vector.result);
ciphertext[0] = 255 - ciphertext[0];
var promise = importVectorKey(vector, ["encrypt", "decrypt"])
.then(function(vector) {
promise_test(function(test) {
var operation = subtle.decrypt({
...vector.algorithm,
get name() {
ciphertext[0] = vector.result[0];
return vector.algorithm.name;
}
}, vector.key, ciphertext)
.then(function(result) {
assert_true(equalBuffers(result, vector.plaintext), "Should return expected result");
}, function(err) {
assert_unreached("decrypt error for test " + vector.name + ": " + err.message);
});
return operation;
}, vector.name + " decryption with altered ciphertext during call");
}, function(err) {
// We need a failed test if the importVectorKey operation fails, so
// we know we never tested encryption
promise_test(function(test) {
assert_unreached("importKey failed for " + vector.name);
}, "importKey step for decryption: " + vector.name + " with altered ciphertext during call");
});

all_promises.push(promise);
});

// Check for successful decryption even if ciphertext is altered after calling encrypt.
passingVectors.forEach(function(vector) {
var ciphertext = copyBuffer(vector.result);
var promise = importVectorKey(vector, ["encrypt", "decrypt"])
Expand All @@ -98,13 +162,13 @@ function run_test() {
});
ciphertext[0] = 255 - ciphertext[0];
return operation;
}, vector.name + " decryption with altered ciphertext");
}, vector.name + " decryption with altered ciphertext after call");
}, function(err) {
// We need a failed test if the importVectorKey operation fails, so
// we know we never tested encryption
promise_test(function(test) {
assert_unreached("importKey failed for " + vector.name);
}, "importKey step for decryption: " + vector.name + " with altered ciphertext");
}, "importKey step for decryption: " + vector.name + " with altered ciphertext after call");
});

all_promises.push(promise);
Expand Down
Loading