Fix positional arg bug in ExpressionParser.CreateTree (#4279)
This commit is contained in:
@@ -20,7 +20,7 @@ namespace GitHub.DistributedTask.Expressions2
|
|||||||
IEnumerable<IFunctionInfo> functions,
|
IEnumerable<IFunctionInfo> functions,
|
||||||
Boolean allowCaseFunction = true)
|
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}>");
|
context.Trace.Info($"Parsing expression: <{expression}>");
|
||||||
return CreateTree(context);
|
return CreateTree(context);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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
|
||||||
|
{
|
||||||
|
/// <summary>
|
||||||
|
/// Regression tests for ExpressionParser.CreateTree to verify that
|
||||||
|
/// allowCaseFunction does not accidentally set allowUnknownKeywords.
|
||||||
|
/// </summary>
|
||||||
|
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<INamedValueInfo>
|
||||||
|
{
|
||||||
|
new NamedValueInfo<ContextValueNode>("inputs"),
|
||||||
|
};
|
||||||
|
|
||||||
|
var ex = Assert.Throws<ParseException>(() =>
|
||||||
|
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<INamedValueInfo>
|
||||||
|
{
|
||||||
|
new NamedValueInfo<ContextValueNode>("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<INamedValueInfo>
|
||||||
|
{
|
||||||
|
new NamedValueInfo<ContextValueNode>("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<INamedValueInfo>
|
||||||
|
{
|
||||||
|
new NamedValueInfo<ContextValueNode>("github"),
|
||||||
|
};
|
||||||
|
|
||||||
|
var ex = Assert.Throws<ParseException>(() =>
|
||||||
|
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<INamedValueInfo>
|
||||||
|
{
|
||||||
|
new NamedValueInfo<ContextValueNode>("inputs"),
|
||||||
|
};
|
||||||
|
|
||||||
|
var ex = Assert.Throws<ParseException>(() =>
|
||||||
|
parser.CreateTree("github.ref", null, namedValues, null, allowCaseFunction: true));
|
||||||
|
|
||||||
|
Assert.Contains("Unrecognized named-value", ex.Message);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -928,6 +928,58 @@ namespace GitHub.Runner.Common.Tests.Worker
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[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<ArgumentException>(() =>
|
||||||
|
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 = "")
|
private void Setup([CallerMemberName] string name = "")
|
||||||
{
|
{
|
||||||
_ecTokenSource?.Dispose();
|
_ecTokenSource?.Dispose();
|
||||||
|
|||||||
@@ -926,6 +926,58 @@ namespace GitHub.Runner.Common.Tests.Worker
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[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<ArgumentException>(() =>
|
||||||
|
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 = "")
|
private void Setup([CallerMemberName] string name = "")
|
||||||
{
|
{
|
||||||
_ecTokenSource?.Dispose();
|
_ecTokenSource?.Dispose();
|
||||||
|
|||||||
@@ -379,6 +379,40 @@ namespace GitHub.Runner.Common.Tests.Worker
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[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<IActionManifestManagerLegacy>(legacyManager);
|
||||||
|
|
||||||
|
var newManager = new ActionManifestManager();
|
||||||
|
newManager.Initialize(_hc);
|
||||||
|
_hc.SetSingleton<IActionManifestManager>(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<ArgumentException>(() => wrapper.Load(_ec.Object, manifestPath));
|
||||||
|
}
|
||||||
|
finally
|
||||||
|
{
|
||||||
|
Teardown();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
private string GetFullExceptionMessage(Exception ex)
|
private string GetFullExceptionMessage(Exception ex)
|
||||||
{
|
{
|
||||||
var messages = new List<string>();
|
var messages = new List<string>();
|
||||||
|
|||||||
@@ -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 }}'
|
||||||
Reference in New Issue
Block a user