Skip to content

Add regression coverage for command argument injection safeguards#53359

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
pelikhan merged 2 commits into from
Aug 17, 2026
+49 −0
Merged

Add regression coverage for command argument injection safeguards #53359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2fd4ba3
Initial plan
Copilot Aug 17, 2026
86f894e
test command argument injection safeguards
Copilot Aug 17, 2026
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
29 changes: 29 additions & 0 deletions pkg/cli/upgrade_command_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -4,6 +4,7 @@ package cli

import (
"context"
"io"
"os"
"testing"

Expand DownExpand Up@@ -165,6 +166,34 @@ func TestRelaunchWithSameArgsAllowsEmptyForwardedArgument(t *testing.T) {
require.NoError(t, err)
}

func TestRelaunchWithSameArgsPassesShellMetacharactersLiterally(t *testing.T) {
origArgs := os.Args
origStdout := os.Stdout
t.Cleanup(func() {
os.Args = origArgs
os.Stdout = origStdout
})
os.Args = []string{"gh-aw", "upgrade", ";", "$(whoami)"}

readOutput, writeOutput, err := os.Pipe()
require.NoError(t, err)
writeOutputClosed := false
t.Cleanup(func() {
_ = readOutput.Close()
if !writeOutputClosed {
_ = writeOutput.Close()
}
})
os.Stdout = writeOutput

require.NoError(t, relaunchWithSameArgs("--skip-extension-upgrade", "/bin/echo"))
require.NoError(t, writeOutput.Close())
writeOutputClosed = true
output, err := io.ReadAll(readOutput)
require.NoError(t, err)
require.Equal(t, "upgrade ; $(whoami) --skip-extension-upgrade\n", string(output))
}

func TestRelaunchWithSameArgsRejectsUnknownExtraFlag(t *testing.T) {
err := relaunchWithSameArgs("--unknown-flag", "/bin/echo")
require.Error(t, err)
Expand Down
20 changes: 20 additions & 0 deletions pkg/workflow/argument_injection_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -370,6 +370,26 @@ func TestValidatePipPackageName(t *testing.T) {
expectError: true,
errContains: "PyPI names must start and end with a letter or digit, with hyphens, underscores, or dots allowed inside (e.g. \"requests\" or \"my-package\")",
},
{
name: "shell separator is rejected",
pkg: "pkg;whoami",
expectError: true,

@github-actionsgithub-actionsBotAug 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] The new injection test cases omit errContains, so any error message satisfies the check — the test does not verify the rejection reason.

💡 Suggestion

Adding errContains for each case pins the behaviour and turns the test into a specification:

{
name: "shell separator is rejected",
pkg: "pkg;whoami",
expectError: true,
errContains: "PyPI names must start and end",
},

The same pattern applies to the command substitution, newline option injection, and empty name cases. Without it, a silent refactor that changes the rejection path leaves these cases green while the intended guard disappears.

@copilot please address this.

},
{
name: "command substitution is rejected",
pkg: "pkg$(whoami)",
expectError: true,
},
{
name: "newline option injection is rejected",
pkg: "pkg\n--index-url",
expectError: true,
},
{
name: "empty name is rejected",
pkg: "",
expectError: true,
},
}

for _, tt := range tests {
Expand Down
Loading