gitlab
/
cli
mirror of https://gitlab.com/gitlab-org/cli.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix: Merging security fixes for v1.24.0 release

This commit is contained in:
Gary Holtz 2022-12-06 19:35:01 +00:00
parent 272666a547
commit c795f7e008
15 changed files with 582 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

50
commands/ci/artifact/artifact.go
View File

@ -2,8 +2,10 @@ package ci
import (
"archive/zip"
"fmt"
"io"
"os"
"path/filepath"
"strings"
"github.com/MakeNowJust/heredoc"
@ -14,6 +16,27 @@ import (
"gitlab.com/gitlab-org/cli/internal/config"
)
func ensurePathIsCreated(filename string) error {
dir, _ := filepath.Split(filename)
if _, err := os.Stat(filename); os.IsNotExist(err) {
err = os.MkdirAll(dir, 0o700) // Create your file
if err != nil {
return fmt.Errorf("could not create new path: %v", err)
}
}
return nil
}
func sanitizeAssetName(asset string) string {
if !strings.HasPrefix(asset, "/") {
// Prefix the asset with "/" ensures that filepath.Clean removes all `/..`
// See rule 4 of filepath.Clean for more information: https://pkg.go.dev/path/filepath#Clean
asset = "/" + asset
}
return filepath.Clean(asset)
}
func NewCmdRun(f *cmdutils.Factory) *cobra.Command {
jobArtifactCmd := &cobra.Command{
Use: "artifact <refName> <jobName> [flags]",
@ -60,8 +83,19 @@ func NewCmdRun(f *cmdutils.Factory) *cobra.Command {
}
for _, v := range zipReader.File {
sanitizedAssetName := sanitizeAssetName(v.Name)
destDir, err := filepath.Abs(path)
if err != nil {
return fmt.Errorf("resolving absolute download directory path: %v", err)
}
destPath := filepath.Join(destDir, sanitizedAssetName)
if !strings.HasPrefix(destPath, destDir) {
return fmt.Errorf("invalid file path name")
}
if v.FileInfo().IsDir() {
if err := os.Mkdir(path+v.Name, v.Mode()); err != nil {
if err := os.Mkdir(destPath, v.Mode()); err != nil {
return err
}
} else {
@ -70,7 +104,19 @@ func NewCmdRun(f *cmdutils.Factory) *cobra.Command {
return err
}
defer srcFile.Close()
dstFile, err := os.OpenFile(path+v.Name, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, v.Mode())
err = ensurePathIsCreated(destPath)
if err != nil {
return err
}
symlinkCheck, _ := os.Lstat(destPath)
if symlinkCheck != nil && symlinkCheck.Mode()&os.ModeSymlink != 0 {
return fmt.Errorf("file in artifact would overwrite a symbolic link- cannot extract")
}
dstFile, err := os.OpenFile(destPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, v.Mode())
if err != nil {
return err
}

255
commands/ci/artifact/artifact_test.go Normal file
View File

@ -0,0 +1,255 @@
package ci
import (
"archive/zip"
"bytes"
"io"
"net/http"
"os"
"path/filepath"
"testing"
"github.com/alecthomas/assert"
"github.com/stretchr/testify/require"
"gitlab.com/gitlab-org/cli/pkg/iostreams"
"github.com/xanzy/go-gitlab"
"gitlab.com/gitlab-org/cli/api"
"github.com/google/shlex"
"gitlab.com/gitlab-org/cli/commands/cmdutils"
"gitlab.com/gitlab-org/cli/internal/config"
"gitlab.com/gitlab-org/cli/internal/glrepo"
"gitlab.com/gitlab-org/cli/pkg/git"
"gitlab.com/gitlab-org/cli/pkg/httpmock"
)
func doesFileExist(fileName string) bool {
_, err := os.Stat(fileName)
return err == nil
}
func createZipFile(t *testing.T, filename string) (string, string) {
tempPath, err := os.MkdirTemp("/tmp", "testing_directory")
require.NoError(t, err)
archive, err := os.CreateTemp(tempPath, "test.*.zip")
require.NoError(t, err)
defer archive.Close()
zipWriter := zip.NewWriter(archive)
f1, err := os.Open("./testdata/file.txt")
if err != nil {
panic(err)
}
defer f1.Close()
w1, err := zipWriter.Create(filename)
if err != nil {
panic(err)
}
if _, err := io.Copy(w1, f1); err != nil {
panic(err)
}
zipWriter.Close()
return tempPath, archive.Name()
}
func makeTestFactory() (factory *cmdutils.Factory, fakeHTTP *httpmock.Mocker) {
fakeHTTP = &httpmock.Mocker{
MatchURL: httpmock.PathAndQuerystring,
}
io, _, _, _ := iostreams.Test()
io.IsaTTY = false
io.IsInTTY = false
io.IsErrTTY = false
client := func(token, hostname string) (*api.Client, error) {
return api.TestClient(&http.Client{Transport: fakeHTTP}, token, hostname, false)
}
// FIXME as mentioned in ./commands/auth/status/status_test.go,
// httpmock seems to require a quick test run before it will work
_, err := client("", "gitlab.com")
if err != nil {
return nil, nil
}
factory = &cmdutils.Factory{
IO: io,
Config: func() (config.Config, error) {
return config.NewBlankConfig(), nil
},
HttpClient: func() (*gitlab.Client, error) {
a, err := client("xxxx", "gitlab.com")
if err != nil {
return nil, err
}
return a.Lab(), err
},
BaseRepo: func() (glrepo.Interface, error) {
return glrepo.New("OWNER", "REPO"), nil
},
Remotes: func() (glrepo.Remotes, error) {
return glrepo.Remotes{
{
Remote: &git.Remote{Name: "origin"},
Repo: glrepo.New("OWNER", "REPO"),
},
}, nil
},
Branch: func() (string, error) {
return "feature", nil
},
}
return factory, fakeHTTP
}
func createSymlinkZip(t *testing.T) (string, string) {
tempPath, err := os.MkdirTemp("/tmp", "testing_directory")
require.NoError(t, err)
archive, err := os.CreateTemp(tempPath, "test.*.zip")
require.NoError(t, err)
defer archive.Close()
immutableFile, err := os.CreateTemp(tempPath, "immutable_file*.txt")
require.NoError(t, err)
immutableText := "Immutable text! GitLab is cool"
_, err = immutableFile.WriteString(immutableText)
require.NoError(t, err)
err = os.Symlink(immutableFile.Name(), tempPath+"/symlink_file.txt")
require.NoError(t, err)
zipWriter := zip.NewWriter(archive)
fixtureFile, err := os.Open("./testdata/file.txt")
if err != nil {
panic(err)
}
defer fixtureFile.Close()
zipFile, err := zipWriter.Create("symlink_file.txt")
if err != nil {
panic(err)
}
if _, err := io.Copy(zipFile, fixtureFile); err != nil {
panic(err)
}
zipWriter.Close()
return tempPath, archive.Name()
}
func Test_NewCmdRun(t *testing.T) {
tests := []struct {
name string
filename string
want string
customPath string
wantErr bool
}{
{
name: "A regular filename",
filename: "cli-v1.22.0.json",
want: "cli-v1.22.0.json",
},
{
name: "A regular filename in a directory",
filename: "cli/cli-v1.22.0.json",
want: "cli/cli-v1.22.0.json",
},
{
name: "A filename with directory traversal",
filename: "cli-v1.../../22.0.zip",
wantErr: true,
},
{
name: "A particularly nasty filename",
filename: "..././..././..././etc/password_file",
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
factory, fakeHTTP := makeTestFactory()
tempPath, tempFileName := createZipFile(t, tt.filename)
// defer os.Remove(tempFileName)
fakeHTTP.RegisterResponder("GET", `https://gitlab.com/api/v4/projects/OWNER%2FREPO/jobs/artifacts/main/download?job=secret_detection`,
httpmock.NewFileResponse(200, tempFileName))
cmd := NewCmdRun(factory)
argv, err := shlex.Split("main secret_detection")
if err != nil {
t.Fatal(err)
}
cmd.SetArgs(argv)
err = cmd.Flags().Set("path", tempPath)
if err != nil {
return
}
cmd.SetIn(&bytes.Buffer{})
cmd.SetOut(io.Discard)
cmd.SetErr(io.Discard)
_, err = cmd.ExecuteC()
filePathWanted := filepath.Join(tempPath, tt.want)
if tt.wantErr {
assert.Error(t, err, "Should error out if a path doesn't exist")
return
}
assert.NoError(t, err, "Should not have errors")
assert.True(t, doesFileExist(filePathWanted), "File should exist")
if err != nil {
t.Fatal(err)
}
})
}
t.Run("symlink can't overwrite", func(t *testing.T) {
factory, fakeHTTP := makeTestFactory()
tempPath, tempFileName := createSymlinkZip(t)
defer os.Remove(tempFileName)
fakeHTTP.RegisterResponder("GET", `https://gitlab.com/api/v4/projects/OWNER%2FREPO/jobs/artifacts/main/download?job=secret_detection`,
httpmock.NewFileResponse(200, tempFileName))
cmd := NewCmdRun(factory)
argv, err := shlex.Split("main secret_detection")
if err != nil {
t.Fatal(err)
}
cmd.SetArgs(argv)
err = cmd.Flags().Set("path", tempPath)
if err != nil {
return
}
cmd.SetIn(&bytes.Buffer{})
cmd.SetOut(io.Discard)
cmd.SetErr(io.Discard)
_, err = cmd.ExecuteC()
assert.Error(t, err, "file in artifact would overwrite a symbolic link- cannot extract")
})
}

1
commands/ci/artifact/testdata/file.txt vendored Normal file
View File

@ -0,0 +1 @@
a

1
commands/ci/symlink Normal file
View File

@ -0,0 +1 @@
GitLab is cool!

13
commands/issue/create/issue_create.go
View File

@ -443,9 +443,9 @@ func generateIssueWebURL(opts *CreateOpts) (string, error) {
// this uses the slash commands to add labels to the description
// See https://docs.gitlab.com/ee/user/project/quick_actions.html
// See also https://gitlab.com/gitlab-org/gitlab-foss/-/issues/19731#note_32550046
description += "\n/label "
description += "\n/label"
for _, label := range opts.Labels {
description += fmt.Sprintf("~%q", label)
description += fmt.Sprintf(" ~%q", label)
}
}
if len(opts.Assignees) > 0 {
@ -470,10 +470,11 @@ func generateIssueWebURL(opts *CreateOpts) (string, error) {
return "", err
}
u.Path += "/-/issues/new"
u.RawQuery = fmt.Sprintf(
"issue[title]=%s&issue[description]=%s",
strings.ReplaceAll(url.PathEscape(opts.Title), "+", "%2B"),
strings.ReplaceAll(url.PathEscape(description), "+", "%2B"))
q := u.Query()
q.Set("issue[title]", opts.Title)
q.Add("issue[description]", description)
u.RawQuery = q.Encode()
return u.String(), nil
}

24
commands/issue/create/issue_create_test.go
View File

@ -92,3 +92,27 @@ func Test_IssueCreate(t *testing.T) {
api.CreateIssue = oldCreateIssue
}
func TestGenerateIssueWebURL(t *testing.T) {
opts := &CreateOpts{
Labels: []string{"backend", "frontend"},
Assignees: []string{"johndoe", "janedoe"},
Milestone: 15,
Weight: 3,
IsConfidential: true,
BaseProject: &gitlab.Project{
ID: 101,
WebURL: "https://gitlab.example.com/gitlab-org/gitlab",
},
Title: "Autofill tests | for this @project",
}
u, err := generateIssueWebURL(opts)
expectedUrl := "https://gitlab.example.com/gitlab-org/gitlab/-/issues/new?" +
"issue%5Bdescription%5D=%0A%2Flabel+~%22backend%22+~%22frontend%22%0A%2Fassign+johndoe%2C+janedoe%0A%2Fmilestone+%2515%0A%2Fweight+3%0A%2Fconfidential&" +
"issue%5Btitle%5D=Autofill+tests+%7C+for+this+%40project"
assert.NoError(t, err)
assert.Equal(t, expectedUrl, u)
}

19
commands/mr/create/mr_create.go
View File

@ -6,6 +6,7 @@ import (
"net/url"
"os"
"regexp"
"strconv"
"strings"
"github.com/AlecAivazis/survey/v2"
@ -690,15 +691,17 @@ func generateMRCompareURL(opts *CreateOpts) (string, error) {
if err != nil {
return "", err
}
u.Path += "/-/merge_requests/new"
u.RawQuery = fmt.Sprintf(
"merge_request[title]=%s&merge_request[description]=%s&merge_request[source_branch]=%s&merge_request[target_branch]=%s&merge_request[source_project_id]=%d&merge_request[target_project_id]=%d",
strings.ReplaceAll(url.PathEscape(opts.Title), "+", "%2B"),
strings.ReplaceAll(url.PathEscape(description), "+", "%2B"),
opts.SourceBranch,
opts.TargetBranch,
opts.SourceProject.ID,
opts.TargetProject.ID)
q := u.Query()
q.Set("merge_request[title]", opts.Title)
q.Add("merge_request[description]", description)
q.Add("merge_request[source_branch]", opts.SourceBranch)
q.Add("merge_request[target_branch]", opts.TargetBranch)
q.Add("merge_request[source_project_id]", strconv.Itoa(opts.SourceProject.ID))
q.Add("merge_request[target_project_id]", strconv.Itoa(opts.TargetProject.ID))
u.RawQuery = q.Encode()
return u.String(), nil
}

27
commands/mr/create/mr_create_test.go
View File

@ -313,3 +313,30 @@ closes 1234
`, opts.Description)
})
}
func TestGenerateMRCompareURL(t *testing.T) {
opts := &CreateOpts{
Labels: []string{"backend", "frontend"},
Assignees: []string{"johndoe", "janedoe"},
Reviewers: []string{"user", "person"},
MileStone: 15,
TargetProject: &gitlab.Project{ID: 100},
SourceProject: &gitlab.Project{
ID: 101,
WebURL: "https://gitlab.example.com/gitlab-org/gitlab",
},
Title: "Autofill tests | for this @project",
SourceBranch: "@|calc",
TargetBranch: "project/my-branch",
}
u, err := generateMRCompareURL(opts)
expectedUrl := "https://gitlab.example.com/gitlab-org/gitlab/-/merge_requests/new?" +
"merge_request%5Bdescription%5D=%0A%2Flabel+~%22backend%22~%22frontend%22%0A%2Fassign+johndoe%2C+janedoe%0A%2Freviewer+user%2C+person%0A%2Fmilestone+%2515&" +
"merge_request%5Bsource_branch%5D=%40%7Ccalc&merge_request%5Bsource_project_id%5D=101&merge_request%5Btarget_branch%5D=project%2Fmy-branch&merge_request%5Btarget_project_id%5D=100&" +
"merge_request%5Btitle%5D=Autofill+tests+%7C+for+this+%40project"
assert.NoError(t, err)
assert.Equal(t, expectedUrl, u)
}

27
commands/release/download/download.go
View File

@ -8,6 +8,7 @@ import (
"os"
"path"
"path/filepath"
"strings"
"github.com/MakeNowJust/heredoc"
"github.com/spf13/cobra"
@ -190,7 +191,22 @@ func downloadAssets(httpClient *api.Client, io *iostreams.IOStreams, toDownload
color.ProgressIcon(),
color.Blue("name"), *asset.Name,
color.Blue("url"), *asset.URL)
err := downloadAsset(httpClient, *asset.URL, filepath.Join(destDir, *asset.Name))
var sanitizedAssetName string
if asset.Name != nil {
sanitizedAssetName = sanitizeAssetName(*asset.Name)
}
destDir, err := filepath.Abs(destDir)
if err != nil {
return fmt.Errorf("resolving absolute download directory path: %v", err)
}
destPath := filepath.Join(destDir, sanitizedAssetName)
if !strings.HasPrefix(destPath, destDir) {
return fmt.Errorf("invalid file path name")
}
err = downloadAsset(httpClient, *asset.URL, destPath)
if err != nil {
return err
}
@ -199,6 +215,15 @@ func downloadAssets(httpClient *api.Client, io *iostreams.IOStreams, toDownload
return nil
}
func sanitizeAssetName(asset string) string {
if !strings.HasPrefix(asset, "/") {
// Prefix the asset with "/" ensures that filepath.Clean removes all `/..`
// See rule 4 of filepath.Clean for more information: https://pkg.go.dev/path/filepath#Clean
asset = "/" + asset
}
return filepath.Clean(asset)
}
func downloadAsset(client *api.Client, assetURL, destinationPath string) error {
var body io.Reader
// color := streams.Color()

83
commands/release/download/download_test.go Normal file
View File

@ -0,0 +1,83 @@
package download
import (
"net/http"
"os"
"path/filepath"
"testing"
"github.com/alecthomas/assert"
"github.com/stretchr/testify/require"
"gitlab.com/gitlab-org/cli/api"
"gitlab.com/gitlab-org/cli/commands/release/releaseutils/upload"
"gitlab.com/gitlab-org/cli/pkg/httpmock"
"gitlab.com/gitlab-org/cli/pkg/iostreams"
)
func doesFileExist(fileName string) bool {
_, err := os.Stat(fileName)
return err == nil
}
func Test_downloadAssets(t *testing.T) {
assetUrl := "https://gitlab.com/gitlab-org/cli/-/archive/"
fakeHTTP := &httpmock.Mocker{
MatchURL: httpmock.HostAndPath,
}
client, _ := api.TestClient(&http.Client{Transport: fakeHTTP}, "", "", false)
tests := []struct {
name string
filename string
want string
wantErr bool
}{
{
name: "A regular filename",
filename: "cli-v1.22.0.tar",
want: "cli-v1.22.0.tar",
},
{
name: "A filename with directory traversal",
filename: "cli-v1.../../22.0.tar",
want: "22.0.tar",
},
{
name: "A particularly nasty filename",
filename: "..././..././..././etc/password_file",
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
fullUrl := assetUrl + tt.filename
fakeHTTP.RegisterResponder("GET", fullUrl, httpmock.NewStringResponse(200, `test_data`))
io, _, _, _ := iostreams.Test()
release := &upload.ReleaseAsset{
Name: &tt.filename,
URL: &fullUrl,
}
releases := []*upload.ReleaseAsset{release}
tempPath, tempPathErr := os.MkdirTemp("/tmp", "temp_tester")
require.NoError(t, tempPathErr)
filePathWanted := filepath.Join(tempPath, tt.want)
err := downloadAssets(client, io, releases, tempPath)
if tt.wantErr {
assert.Error(t, err, "Should error out if a path doesn't exist")
return
}
assert.NoError(t, err, "Should not have errors")
assert.True(t, doesFileExist(filePathWanted), "File should exist")
})
}
}

1
gl-code-quality-report.json Normal file
View File

@ -0,0 +1 @@
[]

15
pkg/browser/browser.go
View File

@ -4,7 +4,6 @@ import (
"os"
"os/exec"
"runtime"
"strings"
"github.com/google/shlex"
)
@ -19,22 +18,16 @@ func Command(url, launcher string) (*exec.Cmd, error) {
// ForOS produces an exec.Cmd to open the web browser for different OS
func ForOS(goos, url string) *exec.Cmd {
exe := "open"
var args []string
cmd := exec.Command("xdg-open", url)
switch goos {
case "darwin":
args = append(args, url)
cmd = exec.Command("open", url)
case "windows":
exe = "cmd"
r := strings.NewReplacer("&", "^&")
args = append(args, "/c", "start", r.Replace(url))
default:
exe = "xdg-open"
args = append(args, url)
cmd = exec.Command("rundll32", "url.dll,FileProtocolHandler", url)
}
cmd := exec.Command(exe, args...)
cmd.Stderr = os.Stderr
return cmd
}

2
pkg/browser/browser_test.go
View File

@ -37,7 +37,7 @@ func TestForOS(t *testing.T) {
goos: "windows",
url: "https://example.com/path?a=1&b=2&c=3",
},
want: []string{"cmd", "/c", "start", "https://example.com/path?a=1^&b=2^&c=3"},
want: []string{"rundll32", "url.dll,FileProtocolHandler", "https://example.com/path?a=1&b=2&c=3"},
},
}
for _, tt := range tests {

35
pkg/iostreams/iostreams.go
View File

@ -1,11 +1,13 @@
package iostreams
import (
"bufio"
"bytes"
"fmt"
"io"
"os"
"os/exec"
"regexp"
"strings"
"time"
@ -36,6 +38,8 @@ type IOStreams struct {
displayHyperlinks string
}
var controlCharRegEx = regexp.MustCompile(`\[(\d*(;\d+)*[^m])`)
func Init() *IOStreams {
stdoutIsTTY := IsTerminal(os.Stdout)
stderrIsTTY := IsTerminal(os.Stderr)
@ -67,6 +71,10 @@ func Init() *IOStreams {
return ioStream
}
func stripControlCharacters(input string) string {
return controlCharRegEx.ReplaceAllString(input, "^[[${1}")
}
func (s *IOStreams) PromptEnabled() bool {
if s.promptDisabled {
return false
@ -111,6 +119,8 @@ func (s *IOStreams) StartPager() error {
}
}
pagerEnv = append(pagerEnv, "LESSSECURE=1")
if s.shouldDisplayHyperlinks() {
pagerEnv = append(pagerEnv, "LESS=FrX")
} else if _, ok := os.LookupEnv("LESS"); !ok {
@ -128,7 +138,28 @@ func (s *IOStreams) StartPager() error {
if err != nil {
return err
}
s.StdOut = pagedOut
pipeReader, pipeWriter := io.Pipe()
s.StdOut = pipeWriter
// TODO: Unfortunately, trying to add an error channel introduces a wait that locks up the code.
// We should eventually add some error reporting for the go function
go func() {
defer pagedOut.Close()
scanner := bufio.NewScanner(pipeReader)
for scanner.Scan() {
newData := stripControlCharacters(scanner.Text())
_, err = fmt.Fprintln(pagedOut, newData)
if err != nil {
return
}
}
}()
err = pagerCmd.Start()
if err != nil {
return err
@ -142,7 +173,7 @@ func (s *IOStreams) StopPager() {
return
}
_ = s.StdOut.(io.ReadCloser).Close()
_ = s.StdOut.(io.WriteCloser).Close()
_, _ = s.pagerProcess.Wait()
s.pagerProcess = nil
}

60
pkg/iostreams/iostreams_test.go
View File

@ -205,3 +205,63 @@ func Test_HelperFunctions(t *testing.T) {
assert.Equal(t, err, &bytes.Buffer{})
})
}
func Test_stripControlCharacters(t *testing.T) {
type args struct {
badString string
}
tests := []struct {
name string
args args
want string
}{
{
name: "With moving 2 lines up",
args: args{
badString: "echo evil!" + //
"exit 0" + //
"echo Hello World!",
},
want: "echo evil!exit 0^[[2Aecho Hello World!",
},
{
name: "With obfuscating characters",
args: args{
badString: "echo evil!" + //
"exit 0" + //
"echo Hello World!",
},
want: "echo evil!exit 0^[[2;0;0;5;3;2;Aecho Hello World!",
},
{
name: "With clearing the screen",
args: args{
badString: "echo evil!" + //
"exit 0" + //
"echo Hello World!",
},
want: "echo evil!exit 0^[[2Lecho Hello World!",
},
{
name: "control character with empty parameters",
args: args{
badString: "echo Hello World!",
},
want: "^[[2;;;Aecho Hello World!",
},
{
name: "With colors",
args: args{
badString: "\033[0;30mSome text",
},
want: "^[[0;30mSome text",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := stripControlCharacters(tt.args.badString)
assert.Equal(t, got, tt.want)
})
}
}