Skip to content
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.

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

Handling panics in encrypt/decrypt #78

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
22 changes: 20 additions & 2 deletions webcrypto/subtle_crypto.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,16 @@ func (sc *SubtleCrypto) Encrypt( //nolint:dupl // we have two similar methods

callback := sc.vu.RegisterCallback()
go func() {
result, err := encrypter.Encrypt(plaintext, ck)
result, err := func() (result []byte, err error) {
defer func() {
if r := recover(); r != nil {
err = NewError(OperationError, fmt.Sprintf("an unexpected error during encrypting happened: %s", r))
}
}()

result, err = encrypter.Encrypt(plaintext, ck)
return
}()
Comment on lines +97 to +106
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that the issue in the concrete case is about the input not being what is expected, I do wonder if it is not better to check the input instead of using recover.

I guess we can still have this code, but I am not a fan of "exceptional workflow".

AFAIK the reason this is a panic in the crypto module is because there is no reason for someone to provide it incorrect data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general, I totally agree with you, and we should catch this on input/validation. My issue is that if we don't catch such input rule, e.g. that algorithm was part of the web API test suite and still, k6 ended up with hard interruption which isn't great :sad:


callback(func() error {
if err != nil {
Expand Down Expand Up @@ -186,7 +195,16 @@ func (sc *SubtleCrypto) Decrypt( //nolint:dupl // we have two similar methods

callback := sc.vu.RegisterCallback()
go func() {
result, err := decrypter.Decrypt(ciphertext, ck)
result, err := func() (result []byte, err error) {
defer func() {
if r := recover(); r != nil {
err = NewError(OperationError, fmt.Sprintf("an unexpected error during decrypting happened: %s", r))
}
}()

result, err = decrypter.Decrypt(ciphertext, ck)
return
}()

callback(func() error {
if err != nil {
Expand Down
19 changes: 19 additions & 0 deletions webcrypto/tests/cmd_run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,25 @@ func TestExamplesInputOutput(t *testing.T) {
}
}

func TestHandlePanic(t *testing.T) {
t.Parallel()

script, err := os.ReadFile("handle-panic.js") //nolint:forbidigo // it's a test case
require.NoError(t, err)

ts := getSingleFileTestState(t, string(script), []string{"-v", "--log-output=stdout"}, 0)

cmd.ExecuteWithGlobalState(ts.GlobalState)

stdout := ts.Stdout.String()

assert.Contains(t, stdout, "1 complete and 0 interrupted iterations")
// We expect the panic message to be printed
assert.Contains(t, stdout, "Uncaught")

assert.Empty(t, ts.Stderr.String())
}

func getFiles(t *testing.T, path string) []string {
t.Helper()

Expand Down
40 changes: 40 additions & 0 deletions webcrypto/tests/handle-panic.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// this example demonstrates how to handle panic in the decrypt function
// we intentionally cut transmittedData from 12 to the end, so it will panic
// the panic will be caught
import { crypto } from "k6/experimental/webcrypto";

export default async function () {
const keyData = new Uint8Array([
7, 152, 164, 45, 255, 169, 164, 66, 164, 163, 20, 197, 194, 223, 48, 213,
93, 115, 173, 86, 215, 81, 128, 188, 45, 237, 156, 92, 163, 197, 248, 114,
]);

const transmittedData = new Uint8Array([
167, 9, 89, 202, 97, 13, 137, 77, 223, 24, 226, 161, 225, 228, 121, 248,
181, 4, 25, 202, 215, 230, 193, 94, 143, 77, 187, 231, 84, 3, 198, 75, 22,
211, 83, 101, 241, 159, 117, 124, 155, 229, 244, 173, 58, 149, 57, 18,
]);

const iv = new Uint8Array(transmittedData.slice(0, 16));

// we intentionally cut incorrectly transmittedData from 12 to the end
const encryptedData = new Uint8Array(transmittedData.slice(12));

const importedKey = await crypto.subtle.importKey(
"raw",
keyData,
{ name: "AES-CBC", length: "256" },
true,
["encrypt", "decrypt"]
);

// if we pass such transmittedData to decrypt function, it will panic
await crypto.subtle.decrypt(
{
name: "AES-CBC",
iv: iv,
},
importedKey,
encryptedData
);
}
Loading