fix: remove unsafe-eval from typescript sdk#5003
Open
ferntheplant wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
disclaimer: this is vibe-slop. I only manually reviewed the typescript code which seems acceptable - unsure about the rust side. Still working on verifying by running this build against my actual private project but it works on the
test-appDescription of Changes
Removes every use of
new Function(...)from the TypeScript SDK in favor of static, statically-analyzable code.crates/bindings-typescript/src/lib/algebraic_type.tspreviously dynamically synthesized serializer/deserializer functions at runtime usingnew Function('writer', 'value', '...'). This was both a CSP / security hazard and a maintenance burden (the inlined source had to handle every primitive and special case by string concatenation). It also meant downstream consumers couldn't statically inspect or tree-shake what their generated types serialize to.This PR replaces that with two complementary mechanisms:
ProductType.makeSerializer/makeDeserializerandSumType.makeSerializer/makeDeserializerare rewritten to use closures that cache themselves before populating their child (de)serializers — so recursive types still terminate, and the SDK still works for anyAlgebraicTypethe user constructs at runtime (e.g. viat.object(...), internal protocol types, the reducer-args path).TypeBuilder.withSerde(serialize, deserialize)helper installs precomputedserialize/deserializefunctions on a builder and registers them in the globalSERIALIZERS/DESERIALIZERScaches (keyed by the underlyingProductType/SumType). The TypeScript code generator incrates/codegen/src/typescript.rsnow emits a.withSerde((writer, value) => { … }, reader => { … })chained onto every generated__t.object(...),__t.row(...), and__t.enum(...). New helper functions (write_serialize_value,write_deserialize_into,write_product_with_serde,write_sum_with_serde) emit inline JS for everyAlgebraicTypeUsevariant, including the special wrapper types (Identity,ConnectionId,Timestamp,TimeDuration,Uuid) andScheduleAt.Both paths share the runtime caches, so any caller of
ProductType.makeDeserializer(table.rowType)(e.g. the SDK row pipeline indb_connection_impl.ts) automatically picks up the static serializer for codegen-known types and falls back to the closure walker for everything else.The last leftover
new Function('m', 'return import(m)')shim inws.ts(used to hide the dynamicimport('undici')from bundlers) is replaced with a plainawait import(...)that uses a variable specifier plus/* webpackIgnore: true */and/* @vite-ignore */magic comments — same "don't prebundleundici" behavior, noFunction.Other notable touches:
crates/cli/src/subcommands/generate.rs: bug fix — the CLI's--module-defflag was previously unusable becauseprepare_generate_run_configsalways required a module source path even when the schema was supplied as JSON. Now the module-source check is skipped when--module-defis set. This unblocked re-runningcargo run -p generate-client-api.crates/bindings-typescript/src/lib/type_builders.tsre-exportsConnectionId,Identity,TimeDuration,Timestamp,Uuidso generated code (import { … as __Identity, … } from "../../lib/type_builders") resolves the special-type wrappers.crates/bindings-typescript/src/lib/autogen/types.ts,crates/bindings-typescript/src/sdk/client_api/{index,types}.ts,crates/bindings-typescript/src/sdk/client_api/types/{reducers,procedures}.ts, and the typescript codegen snapshot.Out of scope for this PR: reducer-args / procedure-params / procedure-return still use the runtime closure walker. Those are built at runtime in
reducerSchema()from a brand-newProductTypethat has no static identity codegen can register against, so making them static would require API shape changes. They no longer useFunction, just the new closure walker, so they're already safe — just not optimized.API and ABI breaking changes
None.
TypeBuilder.withSerde(...)is a new, additive public method..withSerde(...)call chained onto existing__t.object(...)/__t.row(...)/__t.enum(...)expressions — a strict superset of the previous output; nothing was renamed or removed.generate --module-deffix only relaxes a previously over-broad validation; existing flag combinations continue to work.Expected complexity level and risk
3 / 5.
The diff is large (mostly mechanical: every codegen-emitted type gains a
withSerdeblock, and the regeneratedautogen/types.ts+sdk/client_api/types.tsmake up the bulk of the line count). The interesting surface is small but subtle:AlgebraicType(sum ofSum/Product/Array/...) would stack-overflow. The new code does this carefully and is exercised by the protocol's own recursiveAlgebraicTypeinautogen/types.ts.Identity/ConnectionId/Timestamp/TimeDuration/Uuid(1-field product types whose JS representation is a class instance, not an object literal);ScheduleAt(2-variant sum with bespoke wrapping);Option(usestagbyte but notagfield in JS);Result(wire is{ok}|{err}but TS inference isOk | Err— handled by typing the generated(writer, value: any)to bypass the mismatch).undefinedbehavior on out-of-range tags, since some callers may rely on it.prepare_generate_run_configstest suite (21 tests, all passing).If something regresses, it's most likely a wire-format mismatch on one of the special types — but the snapshot test plus the bindings-typescript test suite (which round-trips real protocol frames against the generated
client_api) make that highly visible.Testing
cargo test -p spacetimedb-codegen --test codegen test_codegen_typescript— snapshot accepted.cargo test -p spacetimedb-cli --lib generate::— 21 / 21 passed. Confirms the--module-defvalidation fix didn't regress anything.pnpm --filter spacetimedb run build:types(i.e.tsc -p tsconfig.build.json) — passes. Confirms the regeneratedautogen/types.tsandsdk/client_api/types.ts(with their new.withSerde(...)blocks) typecheck under strict mode.pnpm --filter spacetimedb run test— 182 / 182 passed (20 files). Includestests/serde.test.ts,tests/db_connection.test.ts(round-trips reducer + table updates through the real WebSocket-frame parser, so it exercises both the static codegen path onclient_api/types.tsand the runtime closure walker for reducer args),tests/algebraic_type.test.ts, and the binary reader/writer tests.pnpm --filter spacetimedb exec tsup— all 18 build entrypoints build cleanly.dist/sdk/index.mjspreserves the dynamicimport(undiciSpecifier)with both magic comments intact, so downstream bundlers also leaveundicialone.dist/min/index.browser.mjscontains zero references toundici(tree-shaken away on the browser build target).rg 'new Function\(|Function\([\x27"]' crates/bindings-typescript/src sdks/typescript/src— no matches.Function(...)is fully gone from both SDK source trees.crates/bindings-typescript/src/sdk/client_api/types.ts— especiallyOneOffQueryResult(Result-typed) andTableUpdateRows(sum with recursive refs) — to confirm the generated.withSerde(...)blocks are readable and look correct.(writer, value: any)/(reader): anyparameter types in the generatedwithSerdecallbacks. The escape hatch is intentional: the wire shape (e.g.{ok}|{err}forResult) doesn't always coincide with the inferred TS shape, and the generated code is opaque to consumers anyway.pnpm generatecycle on one of the example modules (e.g.pnpm generate:test-app) and confirm the output diff matches the codegen snapshot.