cloudflare / workerd

The JavaScript / Wasm runtime that powers Cloudflare Workers
https://blog.cloudflare.com/workerd-open-source-workers-runtime/
Apache License 2.0
6.25k stars 300 forks source link

🐛 Bug Report — Runtime APIs - nodejs compat - crypto - `getRandomBytes()` #2716

Closed petebacondarwin closed 1 month ago

petebacondarwin commented 1 month ago

The getRandomBytes() function will fail when called if it is not bound to the webcrypto object. See https://github.com/cloudflare/workers-sdk/pull/6721 for a test case. Currently the calls fail with: TypeError: Illegal invocation: function called with incorrectthisreference..

This has been fixed in unenv via https://github.com/unjs/unenv/pull/309 but I think that the getRandomBytes() function should not require any particular this to work.

kentonv commented 1 month ago

What's getRuntimeBytes()? Did you mean crypto.getRandomValues()?

Assuming so, it has the same behavior in Chrome:

Screenshot from 2024-09-16 15-47-53

petebacondarwin commented 1 month ago

To be more concrete. Using nodejs_compat (not v2, so no polyfills) running this Worker:

import { getBuiltinModule } from "node:process";

export default {
    async fetch(
        request: Request,
        env: Env,
        ctx: ExecutionContext
    ): Promise<Response> {
        const results: string[] = [];
        const nodeCrypto = getBuiltinModule("crypto");
        const webcrypto = nodeCrypto.webcrypto;
        const getRandomValues = webcrypto.getRandomValues;
        results.push(crypto.getRandomValues(new Uint8Array(6)).toString()); // global
        results.push(webcrypto.getRandomValues(new Uint8Array(6)).toString()); // webcrypto
        results.push(nodeCrypto.getRandomValues(new Uint8Array(6)).toString()); // namespace import
        results.push(getRandomValues(new Uint8Array(6)).toString()); // free standing
        return Response.json(results);
    },
};

Results in exceptions on these lines:

nodeCrypto.getRandomValues(new Uint8Array(6)).toString(); // namespace import
getRandomValues(new Uint8Array(6)).toString(); // free standing

I could accept that the bare getRandomValues() call might not work. But I would expect the nodeCrypto.getRandomValues() call to work, which is the point of this issue.

petebacondarwin commented 1 month ago

Also given that this is for Node.js compatibility, I tested it on vanilla Node.js (18):

> const crypto = require("crypto")
undefined

> crypto.getRandomValues(new Uint8Array(6))
Uint8Array(6) [ 56, 123, 166, 65, 220, 221 ]

> crypto.webcrypto.getRandomValues(new Uint8Array(6))
Uint8Array(6) [ 89, 169, 107, 187, 129, 83 ]

> const getRandomValues = crypto.getRandomValues
undefined

> getRandomValues(new Uint8Array(6))
Uint8Array(6) [ 82, 122, 19, 15, 222, 155 ]