diff --git a/src/Sdk/DTExpressions2/Expressions2/ExpressionParser.cs b/src/Sdk/DTExpressions2/Expressions2/ExpressionParser.cs index 43b9b4eaf9f..f0dac3d24aa 100644 --- a/src/Sdk/DTExpressions2/Expressions2/ExpressionParser.cs +++ b/src/Sdk/DTExpressions2/Expressions2/ExpressionParser.cs @@ -20,7 +20,7 @@ public IExpressionNode CreateTree( IEnumerable functions, Boolean allowCaseFunction = true) { - var context = new ParseContext(expression, trace, namedValues, functions, allowCaseFunction); + var context = new ParseContext(expression, trace, namedValues, functions, allowCaseFunction: allowCaseFunction); context.Trace.Info($"Parsing expression: <{expression}>"); return CreateTree(context); } diff --git a/src/Test/L0/Sdk/ExpressionParserL0.cs b/src/Test/L0/Sdk/ExpressionParserL0.cs new file mode 100644 index 00000000000..6512ec9e74a --- /dev/null +++ b/src/Test/L0/Sdk/ExpressionParserL0.cs @@ -0,0 +1,104 @@ +using GitHub.DistributedTask.Expressions2; +using GitHub.DistributedTask.Expressions2.Sdk; +using GitHub.DistributedTask.ObjectTemplating; +using System; +using System.Collections.Generic; +using Xunit; + +namespace GitHub.Runner.Common.Tests.Sdk +{ + /// + /// Regression tests for ExpressionParser.CreateTree to verify that + /// allowCaseFunction does not accidentally set allowUnknownKeywords. + /// + public sealed class ExpressionParserL0 + { + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Sdk")] + public void CreateTree_RejectsUnrecognizedNamedValue() + { + // Regression: allowCaseFunction was passed positionally into + // the allowUnknownKeywords parameter, causing all named values + // to be silently accepted. + var parser = new ExpressionParser(); + var namedValues = new List + { + new NamedValueInfo("inputs"), + }; + + var ex = Assert.Throws(() => + parser.CreateTree("github.event.repository.private", null, namedValues, null)); + + Assert.Contains("Unrecognized named-value", ex.Message); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Sdk")] + public void CreateTree_AcceptsRecognizedNamedValue() + { + var parser = new ExpressionParser(); + var namedValues = new List + { + new NamedValueInfo("inputs"), + }; + + var node = parser.CreateTree("inputs.foo", null, namedValues, null); + + Assert.NotNull(node); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Sdk")] + public void CreateTree_CaseFunctionWorks_WhenAllowed() + { + var parser = new ExpressionParser(); + var namedValues = new List + { + new NamedValueInfo("github"), + }; + + var node = parser.CreateTree("case(github.event_name, 'push', 'Push Event')", null, namedValues, null, allowCaseFunction: true); + + Assert.NotNull(node); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Sdk")] + public void CreateTree_CaseFunctionRejected_WhenDisallowed() + { + var parser = new ExpressionParser(); + var namedValues = new List + { + new NamedValueInfo("github"), + }; + + var ex = Assert.Throws(() => + parser.CreateTree("case(github.event_name, 'push', 'Push Event')", null, namedValues, null, allowCaseFunction: false)); + + Assert.Contains("Unrecognized function", ex.Message); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Sdk")] + public void CreateTree_CaseFunctionDoesNotAffectUnknownKeywords() + { + // The key regression test: with allowCaseFunction=true (default), + // unrecognized named values must still be rejected. + var parser = new ExpressionParser(); + var namedValues = new List + { + new NamedValueInfo("inputs"), + }; + + var ex = Assert.Throws(() => + parser.CreateTree("github.ref", null, namedValues, null, allowCaseFunction: true)); + + Assert.Contains("Unrecognized named-value", ex.Message); + } + } +} diff --git a/src/Test/L0/Worker/ActionManifestManagerL0.cs b/src/Test/L0/Worker/ActionManifestManagerL0.cs index b5da3b30498..9d707c3daa5 100644 --- a/src/Test/L0/Worker/ActionManifestManagerL0.cs +++ b/src/Test/L0/Worker/ActionManifestManagerL0.cs @@ -928,6 +928,58 @@ public void Evaluate_Default_Input() } } + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void Load_ContainerAction_RejectsInvalidExpressionContext() + { + try + { + // Arrange + Setup(); + + var actionManifest = new ActionManifestManager(); + actionManifest.Initialize(_hc); + + // Act & Assert — github is not a valid context for container-runs-env (only inputs is allowed) + var ex = Assert.Throws(() => + actionManifest.Load(_ec.Object, Path.Combine(TestUtil.GetTestDataPath(), "dockerfileaction_env_invalid_context.yml"))); + + Assert.Contains("Failed to load", ex.Message); + } + finally + { + Teardown(); + } + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void Load_ContainerAction_AcceptsValidExpressionContext() + { + try + { + // Arrange + Setup(); + + var actionManifest = new ActionManifestManager(); + actionManifest.Initialize(_hc); + + // Act — inputs is a valid context for container-runs-env + var result = actionManifest.Load(_ec.Object, Path.Combine(TestUtil.GetTestDataPath(), "dockerfileaction_arg_env_expression.yml")); + + // Assert + var containerAction = result.Execution as ContainerActionExecutionDataNew; + Assert.NotNull(containerAction); + Assert.Equal("${{ inputs.entryPoint }}", containerAction.Environment[1].Value.ToString()); + } + finally + { + Teardown(); + } + } + private void Setup([CallerMemberName] string name = "") { _ecTokenSource?.Dispose(); diff --git a/src/Test/L0/Worker/ActionManifestManagerLegacyL0.cs b/src/Test/L0/Worker/ActionManifestManagerLegacyL0.cs index c11d4b9b621..0166b87b201 100644 --- a/src/Test/L0/Worker/ActionManifestManagerLegacyL0.cs +++ b/src/Test/L0/Worker/ActionManifestManagerLegacyL0.cs @@ -926,6 +926,58 @@ public void Evaluate_Default_Input() } } + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void Load_ContainerAction_RejectsInvalidExpressionContext() + { + try + { + // Arrange + Setup(); + + var actionManifest = new ActionManifestManagerLegacy(); + actionManifest.Initialize(_hc); + + // Act & Assert — github is not a valid context for container-runs-env (only inputs is allowed) + var ex = Assert.Throws(() => + actionManifest.Load(_ec.Object, Path.Combine(TestUtil.GetTestDataPath(), "dockerfileaction_env_invalid_context.yml"))); + + Assert.Contains("Failed to load", ex.Message); + } + finally + { + Teardown(); + } + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void Load_ContainerAction_AcceptsValidExpressionContext() + { + try + { + // Arrange + Setup(); + + var actionManifest = new ActionManifestManagerLegacy(); + actionManifest.Initialize(_hc); + + // Act — inputs is a valid context for container-runs-env + var result = actionManifest.Load(_ec.Object, Path.Combine(TestUtil.GetTestDataPath(), "dockerfileaction_arg_env_expression.yml")); + + // Assert + var containerAction = result.Execution as ContainerActionExecutionData; + Assert.NotNull(containerAction); + Assert.Equal("${{ inputs.entryPoint }}", containerAction.Environment[1].Value.ToString()); + } + finally + { + Teardown(); + } + } + private void Setup([CallerMemberName] string name = "") { _ecTokenSource?.Dispose(); diff --git a/src/Test/L0/Worker/ActionManifestParserComparisonL0.cs b/src/Test/L0/Worker/ActionManifestParserComparisonL0.cs index d7039767cde..7551b0f57e6 100644 --- a/src/Test/L0/Worker/ActionManifestParserComparisonL0.cs +++ b/src/Test/L0/Worker/ActionManifestParserComparisonL0.cs @@ -379,6 +379,40 @@ public void EvaluateCompositeOutputs_BothParsersAgree() } } + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void Load_BothParsersRejectInvalidExpressionContext() + { + try + { + // Arrange — regression test: both parsers must reject github context + // in container-runs-env (only inputs is allowed per schema) + Setup(); + _ec.Object.Global.Variables.Set(Constants.Runner.Features.CompareWorkflowParser, "true"); + + var legacyManager = new ActionManifestManagerLegacy(); + legacyManager.Initialize(_hc); + _hc.SetSingleton(legacyManager); + + var newManager = new ActionManifestManager(); + newManager.Initialize(_hc); + _hc.SetSingleton(newManager); + + var wrapper = new ActionManifestManagerWrapper(); + wrapper.Initialize(_hc); + + var manifestPath = Path.Combine(TestUtil.GetTestDataPath(), "dockerfileaction_env_invalid_context.yml"); + + // Act & Assert — both parsers should reject, wrapper should throw + Assert.Throws(() => wrapper.Load(_ec.Object, manifestPath)); + } + finally + { + Teardown(); + } + } + private string GetFullExceptionMessage(Exception ex) { var messages = new List(); diff --git a/src/Test/TestData/dockerfileaction_env_invalid_context.yml b/src/Test/TestData/dockerfileaction_env_invalid_context.yml new file mode 100644 index 00000000000..0762d69e24e --- /dev/null +++ b/src/Test/TestData/dockerfileaction_env_invalid_context.yml @@ -0,0 +1,13 @@ +name: 'Action With Invalid Context' +description: 'Docker action that uses github context in env (only inputs is allowed)' +inputs: + my-input: + description: 'A test input' + required: false + default: 'hello' +runs: + using: 'docker' + image: 'Dockerfile' + env: + VALID: '${{ inputs.my-input }}' + INVALID: '${{ github.event.repository.private }}'