Reject unsafe @func names in program validator and interpreter - #394
Merged
Conversation
Add a shared isValidFunctionName predicate used by both createModuleTextFromProgram and evaluateJsonProgram so a program can never be validated by one path but dispatched by the other. - createModuleTextFromProgram now rejects @func values that aren't strict identifiers, closing a source-injection hole where comment tokens (/* ... */) in @func could comment out later steps so the type-checker-based validator never saw them. - evaluateJsonProgram now throws for @func values that aren't strict identifiers or that resolve to an Object.prototype member (constructor, __proto__, toString, valueOf, hasOwnProperty, etc.), instead of forwarding them to the host onCall dispatcher. @ref bounds checking is unrelated and already handled by #393. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens TypeChat’s JSON “program” validator and interpreter against unsafe @func names that could enable TypeScript source injection during validation and prototype-member dispatch during interpretation.
Changes:
- Added a shared
isValidFunctionNamepredicate and applied it in bothcreateModuleTextFromProgram(validation) andevaluateJsonProgram(interpreter dispatch). - Updated interpreter behavior to throw on invalid
@funcnames rather than dispatching them. - Added tests covering comment-injection
@funcvalues,Object.prototypemember names, and a normal valid program path.
Show a summary per file
| File | Description |
|---|---|
typescript/src/ts/program.ts |
Introduces isValidFunctionName and uses it to reject unsafe @func names in both module generation and runtime evaluation. |
typescript/test/program.test.ts |
Adds regression tests for @func name injection/prototype-member cases and verifies valid programs still work. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (5)
typescript/test/program.test.ts:124
- This
it(...)block’s body isn’t indented, which diverges from the style used in the rest of the test file and makes it harder to scan.
it("createModuleTextFromProgram rejects a comment-injection @func value", () => {
const result = createModuleTextFromProgram(maliciousProgram);
assert.equal(result.success, false);
});
typescript/test/program.test.ts:130
- This
it(...)block’s contents aren’t indented consistently with surrounding tests. Re-indent to match the file’s existing style.
it("createModuleTextFromProgram accepts a normal valid program", () => {
const result = createModuleTextFromProgram({
"@steps": [
{ "@func": "first", "@args": [] },
{ "@func": "second", "@args": [{ "@ref": 0 }] },
typescript/test/program.test.ts:142
- The body of this async
it(...)test isn’t indented, unlike the rest of the file. Re-indent for consistency/readability.
it("evaluateJsonProgram throws instead of dispatching a comment-injection @func value", async () => {
const calls: Array<{ func: string; args: unknown[] }> = [];
await assert.rejects(
evaluateJsonProgram(maliciousProgram, async (func, args) => {
calls.push({ func, args });
typescript/test/program.test.ts:154
- The
forloop and nestedit(...)block need consistent indentation (compare to earlier tests). This section is currently flush-left inside thedescribe, which hurts readability.
for (const badName of ["constructor", "__proto__", "toString", "valueOf", "hasOwnProperty"]) {
it(`evaluateJsonProgram throws instead of dispatching prototype member "${badName}"`, async () => {
const program: Program = { "@steps": [{ "@func": badName, "@args": [] }] };
let dispatched = false;
await assert.rejects(
typescript/test/program.test.ts:169
- This final
it(...)test body isn’t indented consistently with the rest of the file, making it harder to read and maintain. Re-indent to match existing tests.
it("evaluateJsonProgram dispatches a normal valid program correctly", async () => {
const calls: Array<{ func: string; args: unknown[] }> = [];
const result = await evaluateJsonProgram(
{
"@steps": [
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Low
…nt tests - Reword isValidFunctionName JSDoc to describe the actual ASCII identifier constraint instead of implying full JS identifier support. - evaluateJsonProgram's @func branch now mirrors the generator's shape checks (args must be an array when present, no unexpected extra keys) and throws instead of silently returning undefined when they don't hold. - Re-indent the @func name validation test block to 4 spaces. - Add tests covering non-array @Args and extra keys for both createModuleTextFromProgram and evaluateJsonProgram. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
robgruen
approved these changes
Jul 27, 2026
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.
Vulnerability
createModuleTextFromProgramconcatenated the model-supplied@funcname raw into generated TypeScript (api.${func}(...)) with no identifier check. A@funcvalue containing comment tokens (/* ... */) could comment out later generated steps, so the type-checker-based validator (which is the sole validation gate for programs) would report success even though a later step called an undeclared function.Separately,
evaluateJsonProgramonly checkedtypeof func === "string"before forwarding the name to the host'sonCalldispatcher, so names likeconstructor,__proto__,toString,valueOf,hasOwnPropertycould reach hosts that dispatch viaapi[func](...), resolving toObject.prototypemembers the schema never declared.Fix
Added one shared
isValidFunctionNamepredicate (strict identifier regex + rejectsObject.prototypemembers) used by both:createModuleTextFromProgram— invalid@funcnames are now treated as an invalid expression, so the whole program fails validation.evaluateJsonProgram— invalid@funcnames now throw instead of being dispatched.This guarantees the two code paths can never disagree about which programs are legal.
@refbounds checking is unrelated and already fixed in #393 (already merged tomain).Tests
Added
typescript/test/program.test.tscases:createModuleTextFromProgramvalidation.evaluateJsonProgramthrows (and never dispatches) for the comment-injection name and forconstructor/__proto__/toString/valueOf/hasOwnProperty.Wired
out/program.test.jsinto thetestnpm script.npm run build(tsc -p src, tsc -p test) passes;node --test out/validate.test.js out/zod.test.js out/program.test.js tests/model.test.mjs→ 97/97 tests pass. Also manually verified the exact malicious repro from the report is now rejected bycreateModuleTextFromProgramand throws inevaluateJsonProgramrather than dispatching.Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com