From 98516d35c8f5700d2fb9b70ccaa9d3ce238cee51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Wed, 20 Feb 2019 10:09:48 +0100 Subject: [PATCH 1/3] Add sha validations to raw operations --- internal/helper/security.go | 10 +++++++++ internal/helper/security_test.go | 21 ++++++++++++++++++ internal/service/diff/commit.go | 10 +++++++++ internal/service/diff/raw_test.go | 36 +++++++++++++++++++++++++++++++ 4 files changed, 77 insertions(+) diff --git a/internal/helper/security.go b/internal/helper/security.go index afbcced9d50..5af5bb9c580 100644 --- a/internal/helper/security.go +++ b/internal/helper/security.go @@ -1,6 +1,7 @@ package helper import ( + "fmt" "os" "regexp" "strings" @@ -24,3 +25,12 @@ var hostPattern = regexp.MustCompile(`(?i)([a-z][a-z0-9+\-.]*://)([a-z0-9\-._~%! func SanitizeString(str string) string { return hostPattern.ReplaceAllString(str, "$1[FILTERED]@$3$4") } + +// ValidSha checks if SHA is valid +func ValidSha(sha string) error { + if match, err := regexp.MatchString(`\A[0-9a-f]{40}\z`, sha); !match || err != nil { + return fmt.Errorf("Invalid Sha") + } + + return nil +} diff --git a/internal/helper/security_test.go b/internal/helper/security_test.go index 9a8125dac61..7121f0afc88 100644 --- a/internal/helper/security_test.go +++ b/internal/helper/security_test.go @@ -40,3 +40,24 @@ func TestSanitizeString(t *testing.T) { assert.Equal(t, tc.output, SanitizeString(tc.input)) } } + +func TestValidSha(t *testing.T) { + testCases := []struct { + sha string + error bool + }{ + {"", true}, + {"invalid-sha", true}, + {"878d0d962673697c1d038d47e8070f8e7a807028", false}, + {"878d0d962673697c1d038d47e8070f8e7a80702", true}, + {"878d0d962673697c1d038d47e8070f8e7a807028a", true}, + } + + for _, tc := range testCases { + if tc.error { + assert.Error(t, ValidSha(tc.sha), "Invalid Sha") + } else { + assert.NoError(t, ValidSha(tc.sha)) + } + } +} diff --git a/internal/service/diff/commit.go b/internal/service/diff/commit.go index be42f6c5be2..ff07ad50664 100644 --- a/internal/service/diff/commit.go +++ b/internal/service/diff/commit.go @@ -9,6 +9,7 @@ import ( "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/internal/diff" "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/helper" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) @@ -196,10 +197,19 @@ func validateRequest(in requestWithLeftRightCommitIds) error { if in.GetLeftCommitId() == "" { return fmt.Errorf("empty LeftCommitId") } + + if err := helper.ValidSha(in.GetLeftCommitId()); err != nil { + return fmt.Errorf("invalid LeftCommitId SHA") + } + if in.GetRightCommitId() == "" { return fmt.Errorf("empty RightCommitId") } + if err := helper.ValidSha(in.GetLeftCommitId()); err != nil { + return fmt.Errorf("invalid RightCommitId SHA") + } + return nil } diff --git a/internal/service/diff/raw_test.go b/internal/service/diff/raw_test.go index e775d48dd59..80c7a1b3def 100644 --- a/internal/service/diff/raw_test.go +++ b/internal/service/diff/raw_test.go @@ -97,6 +97,24 @@ func TestFailedRawDiffRequestDueToValidations(t *testing.T) { }, code: codes.InvalidArgument, }, + { + desc: "invalid left commit", + request: &gitalypb.RawDiffRequest{ + Repository: testRepo, + RightCommitId: "invalid-sha", + LeftCommitId: "e395f646b1499e8e0279445fc99a0596a65fab7e", + }, + code: codes.InvalidArgument, + }, + { + desc: "invalid right commit", + request: &gitalypb.RawDiffRequest{ + Repository: testRepo, + RightCommitId: "8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab", + LeftCommitId: "invalid-sha", + }, + code: codes.InvalidArgument, + }, } for _, testCase := range testCases { @@ -189,6 +207,24 @@ func TestFailedRawPatchRequestDueToValidations(t *testing.T) { }, code: codes.InvalidArgument, }, + { + desc: "invalid left commit", + request: &gitalypb.RawPatchRequest{ + Repository: testRepo, + RightCommitId: "invalid-sha", + LeftCommitId: "e395f646b1499e8e0279445fc99a0596a65fab7e", + }, + code: codes.InvalidArgument, + }, + { + desc: "invalid right commit", + request: &gitalypb.RawPatchRequest{ + Repository: testRepo, + RightCommitId: "8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab", + LeftCommitId: "invalid-sha", + }, + code: codes.InvalidArgument, + }, } for _, testCase := range testCases { -- GitLab From eb2f68c830696b37f2574675cf313e91b1ab1f27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Wed, 20 Feb 2019 11:52:02 +0100 Subject: [PATCH 2/3] Fixed some bugs and tests --- internal/helper/security.go | 4 ++-- internal/helper/security_test.go | 2 +- internal/service/diff/commit.go | 2 +- internal/service/diff/numstat_test.go | 8 ++++---- internal/service/diff/raw_test.go | 8 ++++---- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/internal/helper/security.go b/internal/helper/security.go index 5af5bb9c580..54bc4ce4b67 100644 --- a/internal/helper/security.go +++ b/internal/helper/security.go @@ -28,8 +28,8 @@ func SanitizeString(str string) string { // ValidSha checks if SHA is valid func ValidSha(sha string) error { - if match, err := regexp.MatchString(`\A[0-9a-f]{40}\z`, sha); !match || err != nil { - return fmt.Errorf("Invalid Sha") + if match, err := regexp.MatchString(`[0-9a-f]{40}`, sha); !match || err != nil { + return fmt.Errorf("invalid Sha") } return nil diff --git a/internal/helper/security_test.go b/internal/helper/security_test.go index 7121f0afc88..fa6f5940929 100644 --- a/internal/helper/security_test.go +++ b/internal/helper/security_test.go @@ -50,7 +50,7 @@ func TestValidSha(t *testing.T) { {"invalid-sha", true}, {"878d0d962673697c1d038d47e8070f8e7a807028", false}, {"878d0d962673697c1d038d47e8070f8e7a80702", true}, - {"878d0d962673697c1d038d47e8070f8e7a807028a", true}, + {"878d0d962673697c1d038d47e8070f8e7a807028~", false}, } for _, tc := range testCases { diff --git a/internal/service/diff/commit.go b/internal/service/diff/commit.go index ff07ad50664..db185475b6f 100644 --- a/internal/service/diff/commit.go +++ b/internal/service/diff/commit.go @@ -206,7 +206,7 @@ func validateRequest(in requestWithLeftRightCommitIds) error { return fmt.Errorf("empty RightCommitId") } - if err := helper.ValidSha(in.GetLeftCommitId()); err != nil { + if err := helper.ValidSha(in.GetRightCommitId()); err != nil { return fmt.Errorf("invalid RightCommitId SHA") } diff --git a/internal/service/diff/numstat_test.go b/internal/service/diff/numstat_test.go index ddebbf7c153..fe39f2de029 100644 --- a/internal/service/diff/numstat_test.go +++ b/internal/service/diff/numstat_test.go @@ -180,19 +180,19 @@ func TestFailedDiffStatsRequest(t *testing.T) { repo: testRepo, leftCommitID: "invalidinvalidinvalid", rightCommitID: "8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab", - err: codes.Unavailable, + err: codes.InvalidArgument, }, { desc: "invalid right commit", repo: testRepo, leftCommitID: "e4003da16c1c2c3fc4567700121b17bf8e591c6c", rightCommitID: "invalidinvalidinvalid", - err: codes.Unavailable, + err: codes.InvalidArgument, }, { desc: "left commit not found", repo: testRepo, - leftCommitID: "z4003da16c1c2c3fc4567700121b17bf8e591c6c", + leftCommitID: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", rightCommitID: "8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab", err: codes.Unavailable, }, @@ -200,7 +200,7 @@ func TestFailedDiffStatsRequest(t *testing.T) { desc: "right commit not found", repo: testRepo, leftCommitID: "e4003da16c1c2c3fc4567700121b17bf8e591c6c", - rightCommitID: "z4003da16c1c2c3fc4567700121b17bf8e591c6c", + rightCommitID: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", err: codes.Unavailable, }, } diff --git a/internal/service/diff/raw_test.go b/internal/service/diff/raw_test.go index 80c7a1b3def..67d2b92034e 100644 --- a/internal/service/diff/raw_test.go +++ b/internal/service/diff/raw_test.go @@ -98,7 +98,7 @@ func TestFailedRawDiffRequestDueToValidations(t *testing.T) { code: codes.InvalidArgument, }, { - desc: "invalid left commit", + desc: "invalid right commit", request: &gitalypb.RawDiffRequest{ Repository: testRepo, RightCommitId: "invalid-sha", @@ -107,7 +107,7 @@ func TestFailedRawDiffRequestDueToValidations(t *testing.T) { code: codes.InvalidArgument, }, { - desc: "invalid right commit", + desc: "invalid left commit", request: &gitalypb.RawDiffRequest{ Repository: testRepo, RightCommitId: "8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab", @@ -208,7 +208,7 @@ func TestFailedRawPatchRequestDueToValidations(t *testing.T) { code: codes.InvalidArgument, }, { - desc: "invalid left commit", + desc: "invalid right commit", request: &gitalypb.RawPatchRequest{ Repository: testRepo, RightCommitId: "invalid-sha", @@ -217,7 +217,7 @@ func TestFailedRawPatchRequestDueToValidations(t *testing.T) { code: codes.InvalidArgument, }, { - desc: "invalid right commit", + desc: "invalid left commit", request: &gitalypb.RawPatchRequest{ Repository: testRepo, RightCommitId: "8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab", -- GitLab From 5bf9dd2e7d42f9e90b1cedcf32542e187b3aa43c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Wed, 20 Feb 2019 11:55:38 +0100 Subject: [PATCH 3/3] Added changelog --- .../unreleased/fj-add-sha-validations-to-raw-operations.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/fj-add-sha-validations-to-raw-operations.yml diff --git a/changelogs/unreleased/fj-add-sha-validations-to-raw-operations.yml b/changelogs/unreleased/fj-add-sha-validations-to-raw-operations.yml new file mode 100644 index 00000000000..41e935dc21d --- /dev/null +++ b/changelogs/unreleased/fj-add-sha-validations-to-raw-operations.yml @@ -0,0 +1,5 @@ +--- +title: Added sha format validations for Raw diff operations +merge_request: +author: +type: security -- GitLab