From 5e2ce84f0af288762866333122918e66dcdcfc0d Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 16 May 2026 00:20:36 +0000 Subject: [PATCH 1/3] fix: draft() emits ciceromark Formula node without required value field (#146) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An inline formula written as a bare expression — e.g. `{{% amount / 2.0 %}}` without `return` — was compiled to a TS function whose body was an expression statement, so the function returned undefined. JSON.stringify(undefined) is undefined, which left the `value` field unset on the ciceromark Formula node and caused the post-generation validator to reject the document with `ValidationException: missing the required field "value"`. Fix: `writeFunctionToString` now wraps user code with `return` when no explicit `return` keyword is present (strings/comments stripped to avoid false positives), restoring the implicit-expression semantics that Cicero/Ergo formulas have always had. As a defense in depth, the formula visitor in `generateAgreement` now throws a clear error if it ever still sees an undefined result, instead of producing an invalid document. Adds a `helloformula-implicit-return` good-template test that reproduces the bug and asserts the Formula node carries the evaluated `value`. Signed-off-by: Claude --- src/TemplateMarkInterpreter.ts | 7 +- src/utils.ts | 29 +++++- .../TemplateMarkInterpreter.test.ts.snap | 96 ++++++++++++++----- .../helloformula-implicit-return/data.json | 5 + .../helloformula-implicit-return/model.cto | 7 ++ .../helloformula-implicit-return/template.md | 1 + 6 files changed, 119 insertions(+), 26 deletions(-) create mode 100644 test/templates/good/helloformula-implicit-return/data.json create mode 100644 test/templates/good/helloformula-implicit-return/model.cto create mode 100644 test/templates/good/helloformula-implicit-return/template.md diff --git a/src/TemplateMarkInterpreter.ts b/src/TemplateMarkInterpreter.ts index 02dc6d4..67e1b65 100644 --- a/src/TemplateMarkInterpreter.ts +++ b/src/TemplateMarkInterpreter.ts @@ -393,7 +393,12 @@ async function generateAgreement(modelManager: ModelManager, clauseLibrary: obje else if (FORMULA_DEFINITION_RE.test(nodeClass)) { if (context.code) { const result = userCodeResults[this.path.join('/')]; - if (result === null) { + if (result === undefined) { + // JSON.stringify(undefined) is undefined, which would leave the + // required `value` field unset and fail downstream validation. + throw new Error(`Formula '${context.name}' did not return a value. Formulas must be an expression or use 'return' to produce a value.`); + } + else if (result === null) { context.value = ''; } else if (typeof result === 'string') { diff --git a/src/utils.ts b/src/utils.ts index ca23c2d..610e22f 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -40,13 +40,40 @@ export function writeFunctionToString(templateClass:ClassDeclaration, functionNa templateClass.getProperties().forEach((p: Property) => { result += ` const ${p.getName()} = data.${p.getName()};\n`; }); - result += ' ' + code.trim() + '\n'; + result += ' ' + wrapExpressionWithReturn(code) + '\n'; result += '}\n'; result += '\n'; return result; } +const RETURN_KEYWORD_RE = /\breturn\b/; +const LINE_COMMENT_RE = /\/\/.*$/gm; +const BLOCK_COMMENT_RE = /\/\*[\s\S]*?\*\//g; +const STRING_LITERAL_RE = /(['"`])(?:\\.|(?!\1)[^\\])*\1/g; + +/** + * Wraps user-supplied formula/condition code with `return` when the user did + * not write an explicit `return`, so that an inline expression like + * `{{% amount / 2.0 %}}` produces a value rather than an undefined result that + * fails downstream validation. Strings and comments are stripped before the + * keyword scan to avoid false positives like a literal `'return foo'`. + */ +function wrapExpressionWithReturn(code: string): string { + const trimmed = code.trim(); + if (trimmed.length === 0) { + return trimmed; + } + const sanitized = trimmed + .replace(BLOCK_COMMENT_RE, '') + .replace(LINE_COMMENT_RE, '') + .replace(STRING_LITERAL_RE, '""'); + if (RETURN_KEYWORD_RE.test(sanitized)) { + return trimmed; + } + return `return ${trimmed.replace(/;\s*$/, '')};`; +} + export function nameUserCode(templateMarkDom: any) { return traverse(templateMarkDom).map(function (x) { if (x && ((x.$class === `${TemplateMarkModel.NAMESPACE}.ConditionalDefinition` && x.condition) || diff --git a/test/__snapshots__/TemplateMarkInterpreter.test.ts.snap b/test/__snapshots__/TemplateMarkInterpreter.test.ts.snap index 7d82ebc..f62e0c9 100644 --- a/test/__snapshots__/TemplateMarkInterpreter.test.ts.snap +++ b/test/__snapshots__/TemplateMarkInterpreter.test.ts.snap @@ -7,53 +7,43 @@ exports[`templatemark interpreter should fail to generate formula-invalid 1`] = "errors": [ { "category": 1, - "character": 3, + "character": 10, "code": 2304, - "id": "err-2304-468-4", + "id": "err-2304-475-4", "length": 4, "line": 82, "renderedMessage": "Cannot find name 'THIS'.", - "start": 1785, + "start": 1792, }, { "category": 1, - "character": 8, + "character": 15, "code": 2304, - "id": "err-2304-473-2", + "id": "err-2304-480-2", "length": 2, "line": 82, "renderedMessage": "Cannot find name 'IS'.", - "start": 1790, + "start": 1797, }, { "category": 1, - "character": 11, + "character": 18, "code": 2304, - "id": "err-2304-476-7", + "id": "err-2304-483-7", "length": 7, "line": 82, "renderedMessage": "Cannot find name 'GARBAGE'.", - "start": 1793, - }, - { - "category": 1, - "character": 3, - "code": 1435, - "id": "err-1435-468-4", - "length": 4, - "line": 82, - "renderedMessage": "Unknown keyword or identifier. Did you mean 'this'?", - "start": 1785, + "start": 1800, }, { "category": 1, - "character": 8, - "code": 1434, - "id": "err-1434-473-2", + "character": 15, + "code": 1005, + "id": "err-1005-480-2", "length": 2, "line": 82, - "renderedMessage": "Unexpected keyword or identifier.", - "start": 1790, + "renderedMessage": "';' expected.", + "start": 1797, }, ], "nodeId": "formula_adef0feb9fc1b6e7513409c93d86aa4d9999d35608e1aaeecc07dca99bc591c8", @@ -1300,6 +1290,64 @@ exports[`templatemark interpreter should generate helloformula 1`] = ` } `; +exports[`templatemark interpreter should generate helloformula-implicit-return 1`] = ` +{ + "$class": "org.accordproject.commonmark@0.5.0.Document", + "nodes": [ + { + "$class": "org.accordproject.commonmark@0.5.0.Paragraph", + "nodes": [ + { + "$class": "org.accordproject.commonmark@0.5.0.Paragraph", + "nodes": [ + { + "$class": "org.accordproject.commonmark@0.5.0.Text", + "text": "Hello ", + }, + { + "$class": "org.accordproject.ciceromark@0.6.0.Variable", + "elementType": "String", + "name": "message", + "value": "World", + }, + { + "$class": "org.accordproject.commonmark@0.5.0.Text", + "text": "! Half of ", + }, + { + "$class": "org.accordproject.ciceromark@0.6.0.Variable", + "elementType": "Double", + "name": "importerLOCAmount", + "value": "1000.0", + }, + { + "$class": "org.accordproject.commonmark@0.5.0.Text", + "text": " is ", + }, + { + "$class": "org.accordproject.commonmark@0.5.0.Strong", + "nodes": [ + { + "$class": "org.accordproject.ciceromark@0.6.0.Formula", + "dependencies": [], + "name": "formula_6a1c6cfe8791ac1da78736898d0ee7ead395e709aa1c4f3e890f2fff0b51497f", + "value": "500", + }, + ], + }, + { + "$class": "org.accordproject.commonmark@0.5.0.Text", + "text": ".", + }, + ], + }, + ], + }, + ], + "xmlns": "http://commonmark.org/xml/1.0", +} +`; + exports[`templatemark interpreter should generate helloworld 1`] = ` { "$class": "org.accordproject.commonmark@0.5.0.Document", diff --git a/test/templates/good/helloformula-implicit-return/data.json b/test/templates/good/helloformula-implicit-return/data.json new file mode 100644 index 0000000..4dbe629 --- /dev/null +++ b/test/templates/good/helloformula-implicit-return/data.json @@ -0,0 +1,5 @@ +{ + "$class" : "helloformula@1.0.0.TemplateData", + "message": "World", + "importerLOCAmount": 1000.0 +} diff --git a/test/templates/good/helloformula-implicit-return/model.cto b/test/templates/good/helloformula-implicit-return/model.cto new file mode 100644 index 0000000..e88f6fb --- /dev/null +++ b/test/templates/good/helloformula-implicit-return/model.cto @@ -0,0 +1,7 @@ +namespace helloformula@1.0.0 + +@template +concept TemplateData { + o String message + o Double importerLOCAmount +} diff --git a/test/templates/good/helloformula-implicit-return/template.md b/test/templates/good/helloformula-implicit-return/template.md new file mode 100644 index 0000000..8304b63 --- /dev/null +++ b/test/templates/good/helloformula-implicit-return/template.md @@ -0,0 +1 @@ +Hello {{message}}! Half of {{importerLOCAmount}} is **{{% importerLOCAmount / 2.0 %}}**. From fc7364b78efa07912c0082f776d8a6405d1bc030 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 16 May 2026 00:23:42 +0000 Subject: [PATCH 2/3] fix: replace ReDoS-prone regex stripping with linear scanner CodeQL flagged the block-comment regex `/\/\*[\s\S]*?\*\//g` (and the nested-quantifier string-literal regex) as polynomial on uncontrolled input. The formula/condition source text comes from user-authored templates and is uncontrolled, so the alert is real. Replace the three regex-based replace calls with a single O(n) char-by-char scanner that drops line comments, block comments, and string literals. Functionally equivalent for the existing test cases and immune to ReDoS by construction. Signed-off-by: Claude --- src/utils.ts | 54 ++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 44 insertions(+), 10 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index 610e22f..cf77d70 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -48,27 +48,61 @@ export function writeFunctionToString(templateClass:ClassDeclaration, functionNa } const RETURN_KEYWORD_RE = /\breturn\b/; -const LINE_COMMENT_RE = /\/\/.*$/gm; -const BLOCK_COMMENT_RE = /\/\*[\s\S]*?\*\//g; -const STRING_LITERAL_RE = /(['"`])(?:\\.|(?!\1)[^\\])*\1/g; + +/** + * Linear, ReDoS-safe pass that drops string literals, line comments, and block + * comments from JS/TS source. Used to scan for the `return` keyword without + * false positives from quoted/commented occurrences. + */ +function stripStringsAndComments(s: string): string { + let out = ''; + const n = s.length; + let i = 0; + while (i < n) { + const c = s[i]; + const next = i + 1 < n ? s[i + 1] : ''; + if (c === '/' && next === '/') { + i += 2; + while (i < n && s[i] !== '\n') i++; + continue; + } + if (c === '/' && next === '*') { + i += 2; + while (i < n - 1 && !(s[i] === '*' && s[i + 1] === '/')) i++; + i = Math.min(n, i + 2); + continue; + } + if (c === '\'' || c === '"' || c === '`') { + const quote = c; + i++; + while (i < n && s[i] !== quote) { + if (s[i] === '\\' && i + 1 < n) { + i += 2; + } else { + i++; + } + } + i++; + continue; + } + out += c; + i++; + } + return out; +} /** * Wraps user-supplied formula/condition code with `return` when the user did * not write an explicit `return`, so that an inline expression like * `{{% amount / 2.0 %}}` produces a value rather than an undefined result that - * fails downstream validation. Strings and comments are stripped before the - * keyword scan to avoid false positives like a literal `'return foo'`. + * fails downstream validation. */ function wrapExpressionWithReturn(code: string): string { const trimmed = code.trim(); if (trimmed.length === 0) { return trimmed; } - const sanitized = trimmed - .replace(BLOCK_COMMENT_RE, '') - .replace(LINE_COMMENT_RE, '') - .replace(STRING_LITERAL_RE, '""'); - if (RETURN_KEYWORD_RE.test(sanitized)) { + if (RETURN_KEYWORD_RE.test(stripStringsAndComments(trimmed))) { return trimmed; } return `return ${trimmed.replace(/;\s*$/, '')};`; From 7af4d246d5acbadc1c73153bb1298332b8960dee Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 16 May 2026 07:05:01 +0000 Subject: [PATCH 3/3] refactor: use TypeScript AST to detect explicit return in formula bodies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the regex-on-source approach in wrapExpressionWithReturn with an AST walk over top-level statements via ts.createSourceFile. The regex matched any `return` keyword anywhere in the user code, which gave a false positive when a nested function/arrow contained its own return (e.g. `[1,2,3].map(x => { return x*2 })[0]`) — the outer expression was then never wrapped and the Formula node ended up missing its `value`. The new logic inspects only sourceFile.statements: if any top-level statement is a ReturnStatement the code is kept verbatim; if the last top-level statement is an ExpressionStatement it is rewritten as `return ;`; otherwise the code is left unchanged and the existing "did not return a value" runtime error fires. The stripStringsAndComments helper and RETURN_KEYWORD_RE constant are removed. typescript is already a direct dependency so no package.json changes were needed. Adds focused unit tests for wrapExpressionWithReturn (bare expression, explicit return, nested-return regression, multi-statement trailing expression, statements-only, empty input). The formula-invalid snapshot is regenerated: only the character offsets shift and a spurious "';' expected." syntax error disappears because the new wrapping shape is syntactically valid; the semantic "Cannot find name" errors are unchanged. Signed-off-by: Claude --- src/utils.ts | 71 +++++++------------ .../TemplateMarkInterpreter.test.ts.snap | 28 +++----- test/utils.test.ts | 32 +++++++++ 3 files changed, 65 insertions(+), 66 deletions(-) create mode 100644 test/utils.test.ts diff --git a/src/utils.ts b/src/utils.ts index cf77d70..2043c0c 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -20,6 +20,7 @@ import { templatemarkutil } from '@accordproject/markdown-template'; import { existsSync, mkdirSync, rmSync } from 'fs'; import traverse from 'traverse'; +import * as ts from 'typescript'; export function ensureDirSync(path:string) { if(!existsSync(path)) { @@ -47,65 +48,41 @@ export function writeFunctionToString(templateClass:ClassDeclaration, functionNa return result; } -const RETURN_KEYWORD_RE = /\breturn\b/; - -/** - * Linear, ReDoS-safe pass that drops string literals, line comments, and block - * comments from JS/TS source. Used to scan for the `return` keyword without - * false positives from quoted/commented occurrences. - */ -function stripStringsAndComments(s: string): string { - let out = ''; - const n = s.length; - let i = 0; - while (i < n) { - const c = s[i]; - const next = i + 1 < n ? s[i + 1] : ''; - if (c === '/' && next === '/') { - i += 2; - while (i < n && s[i] !== '\n') i++; - continue; - } - if (c === '/' && next === '*') { - i += 2; - while (i < n - 1 && !(s[i] === '*' && s[i + 1] === '/')) i++; - i = Math.min(n, i + 2); - continue; - } - if (c === '\'' || c === '"' || c === '`') { - const quote = c; - i++; - while (i < n && s[i] !== quote) { - if (s[i] === '\\' && i + 1 < n) { - i += 2; - } else { - i++; - } - } - i++; - continue; - } - out += c; - i++; - } - return out; -} - /** * Wraps user-supplied formula/condition code with `return` when the user did * not write an explicit `return`, so that an inline expression like * `{{% amount / 2.0 %}}` produces a value rather than an undefined result that * fails downstream validation. + * + * Uses the TypeScript AST to inspect only top-level statements, so a nested + * function/arrow that contains its own `return` does not confuse the outer + * detection (e.g. `[1,2,3].map(x => { return x*2 })[0]`). */ -function wrapExpressionWithReturn(code: string): string { +export function wrapExpressionWithReturn(code: string): string { const trimmed = code.trim(); if (trimmed.length === 0) { return trimmed; } - if (RETURN_KEYWORD_RE.test(stripStringsAndComments(trimmed))) { + const sourceFile = ts.createSourceFile('formula.ts', trimmed, ts.ScriptTarget.Latest, false, ts.ScriptKind.TS); + const statements = sourceFile.statements; + if (statements.length === 0) { + return trimmed; + } + for (const stmt of statements) { + if (ts.isReturnStatement(stmt)) { + return trimmed; + } + } + const last = statements[statements.length - 1]; + if (!ts.isExpressionStatement(last)) { return trimmed; } - return `return ${trimmed.replace(/;\s*$/, '')};`; + const prefix = statements + .slice(0, -1) + .map(s => s.getText(sourceFile)) + .join('\n'); + const expr = last.expression.getText(sourceFile); + return prefix.length > 0 ? `${prefix}\nreturn ${expr};` : `return ${expr};`; } export function nameUserCode(templateMarkDom: any) { diff --git a/test/__snapshots__/TemplateMarkInterpreter.test.ts.snap b/test/__snapshots__/TemplateMarkInterpreter.test.ts.snap index f62e0c9..134b85a 100644 --- a/test/__snapshots__/TemplateMarkInterpreter.test.ts.snap +++ b/test/__snapshots__/TemplateMarkInterpreter.test.ts.snap @@ -7,44 +7,34 @@ exports[`templatemark interpreter should fail to generate formula-invalid 1`] = "errors": [ { "category": 1, - "character": 10, + "character": 3, "code": 2304, - "id": "err-2304-475-4", + "id": "err-2304-468-4", "length": 4, "line": 82, "renderedMessage": "Cannot find name 'THIS'.", - "start": 1792, + "start": 1785, }, { "category": 1, - "character": 15, + "character": 0, "code": 2304, - "id": "err-2304-480-2", + "id": "err-2304-473-2", "length": 2, - "line": 82, + "line": 83, "renderedMessage": "Cannot find name 'IS'.", - "start": 1797, + "start": 1790, }, { "category": 1, - "character": 18, + "character": 7, "code": 2304, "id": "err-2304-483-7", "length": 7, - "line": 82, + "line": 84, "renderedMessage": "Cannot find name 'GARBAGE'.", "start": 1800, }, - { - "category": 1, - "character": 15, - "code": 1005, - "id": "err-1005-480-2", - "length": 2, - "line": 82, - "renderedMessage": "';' expected.", - "start": 1797, - }, ], "nodeId": "formula_adef0feb9fc1b6e7513409c93d86aa4d9999d35608e1aaeecc07dca99bc591c8", }, diff --git a/test/utils.test.ts b/test/utils.test.ts new file mode 100644 index 0000000..389a8fd --- /dev/null +++ b/test/utils.test.ts @@ -0,0 +1,32 @@ +import { wrapExpressionWithReturn } from '../src/utils'; + +describe('wrapExpressionWithReturn', () => { + test('wraps a bare expression with return', () => { + expect(wrapExpressionWithReturn('importerLOCAmount / 2.0')).toBe('return importerLOCAmount / 2.0;'); + }); + + test('leaves explicit top-level return unchanged', () => { + const code = 'const x = 1;\nreturn x + 2;'; + expect(wrapExpressionWithReturn(code)).toBe(code); + }); + + test('wraps when only a nested function/arrow contains return (regression for #146 follow-up)', () => { + const code = '[1,2,3].map(x => { return x*2 })[0]'; + expect(wrapExpressionWithReturn(code)).toBe('return [1,2,3].map(x => { return x*2 })[0];'); + }); + + test('wraps only the trailing expression of a multi-statement body', () => { + const code = 'const x = 1;\nconst y = 2;\nx + y'; + expect(wrapExpressionWithReturn(code)).toBe('const x = 1;\nconst y = 2;\nreturn x + y;'); + }); + + test('leaves a statements-only body (no trailing expression) unchanged', () => { + const code = 'const x = 1;\nconst y = 2;'; + expect(wrapExpressionWithReturn(code)).toBe(code); + }); + + test('returns an empty string as-is', () => { + expect(wrapExpressionWithReturn('')).toBe(''); + expect(wrapExpressionWithReturn(' \n\t ')).toBe(''); + }); +});