Merge pull request #91 from github/cschleiden/improve-expression-validation

Validate expressions even when calling `fromJson`

actions-expressions/src/errors.ts

@@ -50,3 +50,5 @@ function errorDescription(typ: ErrorType): string {
       return "Unknown error";
   }
 }
+
+export class ExpressionEvaluationError extends Error {}

actions-expressions/src/evaluator.test.ts

@@ -1,27 +1,42 @@
 import * as data from "./data";
+import {ExpressionEvaluationError} from "./errors";
 import {Evaluator} from "./evaluator";
 import {Lexer} from "./lexer";
 import {Parser} from "./parser";
 
-test("evaluator", () => {
-  const input = "foo['']";
+describe("evaluator", () => {
+  const lexAndParse = (input: string) => {
+    const lexer = new Lexer(input);
+    const result = lexer.lex();
 
-  const lexer = new Lexer(input);
-  const result = lexer.lex();
+    // Parse
+    const parser = new Parser(result.tokens, ["foo"], []);
+    const expr = parser.parse();
+    return expr;
+  };
 
-  // Parse
-  const parser = new Parser(result.tokens, ["foo"], []);
-  const expr = parser.parse();
+  it("basic evaluation", () => {
+    const expr = lexAndParse("foo['']");
 
-  // Evaluate expression
-  const evaluator = new Evaluator(
-    expr,
-    new data.Dictionary({
-      key: "foo",
-      value: new data.Dictionary({key: "bar", value: new data.NumberData(42)})
-    })
-  );
-  const eresult = evaluator.evaluate();
+    // Evaluate expression
+    const evaluator = new Evaluator(
+      expr,
+      new data.Dictionary({
+        key: "foo",
+        value: new data.Dictionary({key: "bar", value: new data.NumberData(42)})
+      })
+    );
+    const eresult = evaluator.evaluate();
 
-  expect(eresult.kind).toBe(data.Kind.Null);
+    expect(eresult.kind).toBe(data.Kind.Null);
+  });
+
+  it("handle runtime errors", () => {
+    const expr = lexAndParse("fromJson('test') == 123");
+    const evaluator = new Evaluator(expr, new data.Dictionary());
+
+    expect(() => evaluator.evaluate()).toThrowError(
+      new ExpressionEvaluationError("Error parsing JSON when evaluating fromJson")
+    );
+  });
 });

actions-expressions/src/evaluator.ts

@@ -14,14 +14,19 @@ import {
 import * as data from "./data";
 import {FilteredArray} from "./filtered_array";
 import {wellKnownFunctions} from "./funcs";
+import {FunctionDefinition} from "./funcs/info";
 import {idxHelper} from "./idxHelper";
 import {TokenType} from "./lexer";
 import {equals, falsy, greaterThan, lessThan, truthy} from "./result";
 
-export class EvaluationError extends Error {}
-
 export class Evaluator implements ExprVisitor<data.ExpressionData> {
-  constructor(private n: Expr, private context: data.Dictionary) {}
+  /**
+   * Creates a new evaluator
+   * @param n Parsed expression to evaluate
+   * @param context Context data to use
+   * @param functions Optional map of function implementations. If given, these will be preferred over the built-in functions.
+   */
+  constructor(private n: Expr, private context: data.Dictionary, private functions?: Map<string, FunctionDefinition>) {}
 
   public evaluate(): data.ExpressionData {
     return this.eval(this.n);
@@ -109,7 +114,7 @@ export class Evaluator implements ExprVisitor<data.ExpressionData> {
       try {
         idxResult = this.eval(ia.index);
       } catch (e) {
-        throw new Error(`could not evaluate index for index access: ${e}`);
+        throw new Error(`could not evaluate index for index access: ${e}`, {cause: e});
       }
       idx = new idxHelper(false, idxResult);
     }
@@ -150,16 +155,14 @@ export class Evaluator implements ExprVisitor<data.ExpressionData> {
     // Evaluate arguments
     const args = functionCall.args.map(arg => this.eval(arg));
 
-    return fcall(functionCall, args);
+    // Get function definitions
+    const functionName = functionCall.functionName.lexeme.toLowerCase();
+    const f = this.functions?.get(functionName) || wellKnownFunctions[functionName];
+
+    return f.call(...args);
   }
 }
 
-function fcall(fc: FunctionCall, args: data.ExpressionData[]): data.ExpressionData {
-  const f = wellKnownFunctions[fc.functionName.lexeme.toLowerCase()];
-
-  return f.call(...args);
-}
-
 function filteredArrayAccess(fa: FilteredArray, idx: idxHelper): data.ExpressionData {
   const result = new FilteredArray();
 

actions-expressions/src/funcs/fromjson.ts

@@ -1,5 +1,6 @@
 import {ExpressionData} from "../data";
 import {reviver} from "../data/reviver";
+import {ExpressionEvaluationError} from "../errors";
 import {FunctionDefinition} from "./info";
 
 export const fromjson: FunctionDefinition = {
@@ -16,7 +17,10 @@ export const fromjson: FunctionDefinition = {
       throw new Error("empty input");
     }
 
-    const d = JSON.parse(is, reviver);
-    return d;
+    try {
+      return JSON.parse(is, reviver);
+    } catch (e) {
+      throw new ExpressionEvaluationError("Error parsing JSON when evaluating fromJson", {cause: e});
+    }
   }
 };

actions-expressions/src/index.ts

@@ -2,7 +2,8 @@ export {Expr} from "./ast";
 export {complete, CompletionItem} from "./completion";
 export {DescriptionDictionary, DescriptionPair, isDescriptionDictionary} from "./completion/descriptionDictionary";
 export * as data from "./data";
-export {ExpressionError} from "./errors";
+export {ExpressionError, ExpressionEvaluationError} from "./errors";
 export {Evaluator} from "./evaluator";
+export {wellKnownFunctions} from "./funcs";
 export {Lexer, Result} from "./lexer";
 export {Parser} from "./parser";

actions-expressions/tsconfig.json

@@ -3,6 +3,7 @@
   "compilerOptions": {
     "module": "esnext",
     "target": "ES2020",
+    "lib": ["ES2022"],
     "esModuleInterop": true,
     "forceConsistentCasingInFileNames": true,
     "sourceMap": true,

actions-languageservice/src/validate.expressions.test.ts

@@ -359,7 +359,7 @@ jobs:
     runs-on: ubuntu-latest
     steps:
     - name: step a
-      env: 
+      env:
         step_env: job_a_env
       run: echo "hello \${{ env.step_env }}
 `;
@@ -376,7 +376,7 @@ env:
 jobs:
   a:
     runs-on: ubuntu-latest
-    env: 
+    env:
       envjoba: job_a_env
     steps:
     - name: step a
@@ -395,7 +395,7 @@ env:
 jobs:
   a:
     runs-on: ubuntu-latest
-    env: 
+    env:
       envjoba: job_a_env
     steps:
     - name: step a
@@ -1185,6 +1185,9 @@ jobs:
     - run: echo "hello \${{ github.event.inputs.name }}"
     - run: echo "hello \${{ github.event.inputs.third-name }}"
     - run: echo "hello \${{ github.event.inputs.random }}"
+    - run: echo \${{ fromJSON(inputs.random2) }}
+    - run: echo "hello \${{ inputs.random }}"
+      name: "\${{ fromJSON('test') == inputs.name }}"
 `;
       const result = await validate(createDocument("wf.yaml", input));
 
@@ -1202,6 +1205,34 @@ jobs:
             }
           },
           severity: DiagnosticSeverity.Warning
+        },
+        {
+          message: "Context access might be invalid: random2",
+          range: {
+            end: {
+              character: 47,
+              line: 20
+            },
+            start: {
+              character: 16,
+              line: 20
+            }
+          },
+          severity: 2
+        },
+        {
+          message: "Context access might be invalid: random",
+          range: {
+            end: {
+              character: 43,
+              line: 21
+            },
+            start: {
+              character: 23,
+              line: 21
+            }
+          },
+          severity: 2
         }
       ]);
     });

actions-languageservice/src/validate.ts

@@ -1,4 +1,11 @@
-import {Evaluator, Lexer, Parser} from "@github/actions-expressions";
+import {
+  data,
+  Evaluator,
+  ExpressionEvaluationError,
+  Lexer,
+  Parser,
+  wellKnownFunctions
+} from "@github/actions-expressions";
 import {Expr} from "@github/actions-expressions/ast";
 import {
   convertWorkflowTemplate,
@@ -202,7 +209,7 @@ async function validateExpression(
     try {
       const context = await getContext(namedContexts, contextProviderConfig, workflowContext, Mode.Validation);
 
-      const e = new Evaluator(expr, wrapDictionary(context));
+      const e = new Evaluator(expr, wrapDictionary(context), validatorFunctions);
       e.evaluate();
 
       // Any invalid context access would've thrown an error via the `ErrorDictionary`, for now we don't have to check the actual
@@ -214,7 +221,25 @@ async function validateExpression(
           severity: DiagnosticSeverity.Warning,
           range: mapRange(expression.range)
         });
+      } else if (e instanceof ExpressionEvaluationError) {
+        diagnostics.push({
+          message: `Expression might be invalid: ${e.message}`,
+          severity: DiagnosticSeverity.Error,
+          range: mapRange(expression.range)
+        });
       }
     }
   }
 }
+
+// Custom implementations for standard actions-expression functions used during validation. For example,
+// for fromJson we'll most likely not have a valid input. In order to not throw, we'll always return an
+// empty dictionary.
+const validatorFunctions = new Map(
+  Object.entries({
+    fromjson: {
+      ...wellKnownFunctions.fromjson,
+      call: () => new data.Dictionary()
+    }
+  })
+);