fix(core): fall back to node:crypto so randomUUID cannot throw on Node - #599
fix(core): fall back to node:crypto so randomUUID cannot throw on Node#599adilburaksen wants to merge 2 commits into
Conversation
randomUUID() consults only globalThis.crypto. That global was added in Node v17.4.0 and stayed behind --experimental-global-webcrypto until v19.0.0, so on a default Node 18 or earlier neither branch matches and the function throws, where it previously returned a weak UUID. That surfaces as a hard failure in createSession, AuthHandler.generateAuthUri and every A2A message id. node:crypto's randomUUID has existed since v14.17.0 and does not depend on the global, so using it as the last resort makes the throw unreachable on any Node this package could plausibly target. The two globalThis.crypto branches keep precedence, so browsers and Node 19+ are unaffected. randomUUID reaches the web bundle via index_web.ts -> common.ts -> events/event.js, so the node:crypto import is aliased to a browser shim, following the existing node:async_hooks precedent in build.js. The shim throws the message the function used to throw: it is reached only once both globalThis.crypto branches have been ruled out, which in a browser means the Web Crypto API is genuinely absent. Fixes google#598.
AmaadMartin
left a comment
There was a problem hiding this comment.
Checked the load-bearing claim rather than taking it: walking the import graph from dist/web/index_web.js, node:async_hooks (via utils/client_labels.js), node:fs/promises and node:path (via skills/loader.js, utils/file_utils.js) are already reachable from the published browser entry — so node:crypto joins a path that is already unresolvable rather than opening a new one. Your caveat is accurate, and the distinction between the output directory and what is reachable from the entry is the part that actually matters, so thank you for flagging it. Also confirmed nothing else in core/src imports node:crypto, which means the shim's single export is sufficient today and a future importer of, say, createHash would fail loudly at bundle time rather than silently. One nit inline, on the doc comment rather than the code.
For the follow-up you offered: I checked integrations/ as well — it also publishes browser: ./dist/web/index_web.js, but its entry reaches only two modules and pulls in no node: builtins, so core is the only package that needs the fix. Worth folding a guard into that PR too: nothing currently fails if the alias is dropped from build.js, and there is no test behind the node:async_hooks alias either, so both are held in place only by manual inspection.
| * its `randomUUID` has existed since v14.17.0 — so it is the last resort. In | ||
| * the web build that import is aliased to `crypto_shim.ts`, which throws, | ||
| * because a browser without the Web Crypto API has no secure source left. |
There was a problem hiding this comment.
Nit. The doc comment claims more coverage than the alias actually gives: it says "the web build", but only the bundled web build is aliased, and that is not the one the package publishes.
* its `randomUUID` has existed since v14.17.0 — so it is the last resort. In
* the web build that import is aliased to `crypto_shim.ts`, which throws,
* because a browser without the Web Crypto API has no secure source left. * its `randomUUID` has existed since v14.17.0 — so it is the last resort. In
* the bundled web build the import is aliased to `crypto_shim.ts`, which
* throws, because a browser without the Web Crypto API has no secure source
* left. The non-bundle `dist/web` output that `package.json#browser` points
* at keeps the import verbatim, as it already does for `node:async_hooks`
* and `node:path`.Your PR description is careful about exactly this distinction — the code comment isn't, and the comment is what the next reader gets. I confirmed the gap rather than assuming it: from dist/web/index_web.js, node:async_hooks is reachable through utils/client_labels.js, and node:path/node:fs/promises through skills/loader.js and utils/file_utils.js. So the sentence is wrong only about scope, not about the mechanism.
Same wording question applies to crypto_shim.ts's own header, though "does not pull a Node builtin into the web bundle" there is already accurate as written.
The comment said the import is aliased in "the web build", but the alias in build.js applies only when platform is browser and bundle is set. The output package.json#browser points at, dist/web/index_web.js, comes from the non-bundle path and keeps the import verbatim, as it already does for node:async_hooks and node:path.
Fixes #598.
randomUUID()consults onlyglobalThis.crypto. That global was added in Node v17.4.0 and stayed behind--experimental-global-webcryptountil v19.0.0, so on a default Node 18 or earlier neither branch matches and the function throws, where it previously returned a weak UUID — a hard failure increateSession,AuthHandler.generateAuthUriand every A2A message id.node:crypto'srandomUUIDhas existed since v14.17.0 and does not depend on the global, so it becomes the last resort and the throw becomes unreachable on any Node this package could plausibly target. The twoglobalThis.cryptobranches keep precedence, so browsers and Node 19+ behave exactly as they do today.Browser build
randomUUIDdoes reach the web bundle —index_web.tsre-exports./common.js, andcommon.ts:145re-exports./events/event.js, an existing caller. So thenode:cryptoimport is aliased to a shim, following the existingnode:async_hooksprecedent atbuild.js:53-57.core/src/utils/crypto_shim.tsthrows the messagerandomUUIDused to throw. In a browser it is reached only after bothglobalThis.cryptobranches have been ruled out, which means the Web Crypto API is genuinely absent and there is nothing left to fall back to — so failing is still the correct move there.Verified on the output rather than assumed: after
npm run build:bundle,dist/web/index.jscontains nonode:cryptoimport and does contain the shim's message.One thing the alias does not cover
Flagging this rather than leaving it to be discovered later. esbuild's
aliasonly takes effect while it is resolving imports, i.e. whenbundleis true. Butnpm run build— the non-bundle path, which is whatprepublishOnlyruns — also emitsdist/web/, andcore/package.jsonpointsbrowserat./dist/web/index_web.js. That output keeps its imports verbatim:This is a pre-existing gap rather than one this PR opens — the same non-bundle web output already ships unaliased
node:async_hooks,node:path,node:os,node:netandnode:child_process, and theformat === 'esm'banner injectsimport {createRequire} from 'module'into it as well. Closing it needs a different mechanism thanalias(a resolve plugin, or abrowserfield map inpackage.json). Happy to send that separately — I left it out here to keep this PR to what #598 describes.Tests
falls back to node:crypto when globalThis.crypto is absent— the regression this fixes.does not repeat itself across calls without globalThis.crypto— 1000 ids, all distinct, so the fallback is not a constant or a stub.throws instead of degrading when no secure source existsnow targets the shim directly, since that is the only place the throw is still reachable.Negative control run rather than assumed: with
return nodeRandomUUID()reverted to the oldthrow, exactly the two new assertions fail and the other eleven pass.tsc --noEmit, prettier and eslint are clean, andvitest --project unit:coreis green — 168 files, 2358 tests.Not included
No
enginesfield. As discussed on #577 it is advisory unless the consumer setsengine-strict, so it would have made the mismatch visible without preventing the throw. After this change the supported-Node floor is a documentation question rather than a correctness one, which is the point of doing this one first.