Skip to content

Notify on unsecure commands #731

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
thboop merged 11 commits into from
Oct 2, 2020
Merged

Notify on unsecure commands #731

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1738c7d
notify on unsecure commands
thboop Oct 1, 2020
944c9e7
notify on unsecure commands
thboop Oct 1, 2020
bab52e2
Correct text
thboop Oct 1, 2020
de01119
better messaging
thboop Oct 1, 2020
f242b50
make generic opt into unsecure commands
thboop Oct 1, 2020
7933f1a
Respond to Julio's feedback!
thboop Oct 1, 2020
947680d
slight verbiage update
thboop Oct 1, 2020
91ebfe7
Pr feedback
thboop Oct 2, 2020
204d934
swap to env context to get step level variables
thboop Oct 2, 2020
072047b
re-add runner check
thboop Oct 2, 2020
daafd3a
verbiage update
thboop Oct 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/Runner.Common/Constants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,9 @@ public static class ReturnCode

public static readonly string InternalTelemetryIssueDataKey = "_internal_telemetry";
public static readonly string WorkerCrash = "WORKER_CRASH";
public static readonly string UnsupportedCommand = "UNSUPPORTED_COMMAND";
public static readonly string UnsupportedCommandMessage = "The `{0}` command is deprecated and will be disabled soon. Please upgrade to using Environment Files. For more information see: https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/";
public static readonly string UnsupportedCommandMessageDisabled = "The `{0}` command is disabled. Please upgrade to using Environment Files or opt into unsecure command execution by setting the `ACTIONS_ALLOW_UNSECURE_COMMANDS` environment variable to `true`. For more information see: https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/";
}

public static class RunnerEvent
Expand Down Expand Up @@ -198,6 +201,7 @@ public static class Actions
//
// Keep alphabetical
//
public static readonly string AllowUnsupportedCommands = "ACTIONS_ALLOW_UNSECURE_COMMANDS";
public static readonly string RunnerDebug = "ACTIONS_RUNNER_DEBUG";
public static readonly string StepDebug = "ACTIONS_STEP_DEBUG";
}
Expand Down
69 changes: 69 additions & 0 deletions src/Runner.Worker/ActionCommandManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using GitHub.DistributedTask.Pipelines;
using GitHub.DistributedTask.Pipelines.ContextData;
using GitHub.DistributedTask.WebApi;
using GitHub.Runner.Common.Util;
using GitHub.Runner.Worker.Container;
Expand Down Expand Up @@ -183,6 +184,40 @@ public sealed class SetEnvCommandExtension : RunnerService, IActionCommandExtens

public void ProcessCommand(IExecutionContext context, string line, ActionCommand command, ContainerInfo container)
{
var configurationStore = HostContext.GetService<IConfigurationStore>();
var isHostedServer = configurationStore.GetSettings().IsHostedServer;

var allowUnsecureCommands = false;
bool.TryParse(Environment.GetEnvironmentVariable(Constants.Variables.Actions.AllowUnsupportedCommands), out allowUnsecureCommands);
Copy link
Copy Markdown
Collaborator Author

@thboop thboop Oct 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ericsciple this pulls from the .env file in the layout folder of the runner. Users won't be able to set it in their workflow job env.


// Apply environment from env context, env context contains job level env and action's env block
#if OS_WINDOWS
var envContext = context.ExpressionValues["env"] as DictionaryContextData;
#else
var envContext = context.ExpressionValues["env"] as CaseSensitiveDictionaryContextData;
#endif
if (!allowUnsecureCommands && envContext.ContainsKey(Constants.Variables.Actions.AllowUnsupportedCommands))
{
bool.TryParse(envContext[Constants.Variables.Actions.AllowUnsupportedCommands].ToString(), out allowUnsecureCommands);
}

// TODO: Eventually remove isHostedServer and apply this to dotcom customers as well
if (!isHostedServer && !allowUnsecureCommands)
{
throw new Exception(String.Format(Constants.Runner.UnsupportedCommandMessageDisabled, this.Command));
}
else if (!allowUnsecureCommands)
{
// Log Telemetry and let user know they shouldn't do this
var issue = new Issue()
{
Type = IssueType.Warning,
Message = String.Format(Constants.Runner.UnsupportedCommandMessage, this.Command)
};
issue.Data[Constants.Runner.InternalTelemetryIssueDataKey] = Constants.Runner.UnsupportedCommand;
context.AddIssue(issue);
}

if (!command.Properties.TryGetValue(SetEnvCommandProperties.Name, out string envName) || string.IsNullOrEmpty(envName))
{
throw new Exception("Required field 'name' is missing in ##[set-env] command.");
Expand Down Expand Up @@ -282,6 +317,40 @@ public sealed class AddPathCommandExtension : RunnerService, IActionCommandExten

public void ProcessCommand(IExecutionContext context, string line, ActionCommand command, ContainerInfo container)
{
var configurationStore = HostContext.GetService<IConfigurationStore>();
var isHostedServer = configurationStore.GetSettings().IsHostedServer;

var allowUnsecureCommands = false;
bool.TryParse(Environment.GetEnvironmentVariable(Constants.Variables.Actions.AllowUnsupportedCommands), out allowUnsecureCommands);

// Apply environment from env context, env context contains job level env and action's env block
#if OS_WINDOWS
var envContext = context.ExpressionValues["env"] as DictionaryContextData;
#else
var envContext = context.ExpressionValues["env"] as CaseSensitiveDictionaryContextData;
#endif
if (!allowUnsecureCommands && envContext.ContainsKey(Constants.Variables.Actions.AllowUnsupportedCommands))
{
bool.TryParse(envContext[Constants.Variables.Actions.AllowUnsupportedCommands].ToString(), out allowUnsecureCommands);
}

// TODO: Eventually remove isHostedServer and apply this to dotcom customers as well
if (!isHostedServer && !allowUnsecureCommands)
{
throw new Exception(String.Format(Constants.Runner.UnsupportedCommandMessageDisabled, this.Command));
}
else if (!allowUnsecureCommands)
{
// Log Telemetry and let user know they shouldn't do this
var issue = new Issue()
{
Type = IssueType.Warning,
Message = String.Format(Constants.Runner.UnsupportedCommandMessage, this.Command)
};
issue.Data[Constants.Runner.InternalTelemetryIssueDataKey] = Constants.Runner.UnsupportedCommand;
context.AddIssue(issue);
}

ArgUtil.NotNullOrEmpty(command.Data, "path");
context.Global.PrependPath.RemoveAll(x => string.Equals(x, command.Data, StringComparison.CurrentCulture));
context.Global.PrependPath.Add(command.Data);
Expand Down