From 5ef510eac4ceb82cb44e3d435b372c154faaffc8 Mon Sep 17 00:00:00 2001 From: zak Date: Mon, 15 May 2017 08:43:21 +0100 Subject: [PATCH 1/5] Warn on use of abbreviated sha1 commit hash Update manifest validation to warn when an abbreviated version of a sha1 commit hash is used. Add test for warn. --- manifest.go | 7 ++++++- manifest_test.go | 8 ++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/manifest.go b/manifest.go index 557659bdc4..ce8317f281 100644 --- a/manifest.go +++ b/manifest.go @@ -67,8 +67,13 @@ func validateManifest(s string) ([]error, error) { for key, value := range v.(map[string]interface{}) { // Check if the key is valid switch key { - case "name", "branch", "revision", "version", "source": + case "name", "branch", "version", "source": // valid key + case "revision": + // Check if sha1 hash is abbreviated + if valueStr, ok := value.(string); ok && len(valueStr) < 40 { + errs = append(errs, fmt.Errorf("sha1 hash %q should not be in abbreviated form", valueStr)) + } case "metadata": // Check if metadata is of Map type if reflect.TypeOf(value).Kind() != reflect.Map { diff --git a/manifest_test.go b/manifest_test.go index 3eb87df105..7ba311fa10 100644 --- a/manifest_test.go +++ b/manifest_test.go @@ -206,6 +206,14 @@ func TestValidateManifest(t *testing.T) { `, want: []error{}, }, + { + tomlString: ` + [[dependencies]] + name = "github.com/foo/bar" + revision = "b86ad16" + `, + want: []error{errors.New("sha1 hash \"b86ad16\" should not be in abbreviated form")}, + }, } // constains for error From 57a79c5a4170c581b37d036c2ee7d2edd7069e2a Mon Sep 17 00:00:00 2001 From: zak Date: Mon, 15 May 2017 20:08:27 +0100 Subject: [PATCH 2/5] Update to accept valid bzr revision-id Update manifest validation to accept a valid bzr revision id. Update git abbreviated hash check to use a regex instead of len check. Add tests for passing and failing bzr revision. --- manifest.go | 14 +++++++++++--- manifest_test.go | 18 +++++++++++++++++- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/manifest.go b/manifest.go index ce8317f281..afe729e5a0 100644 --- a/manifest.go +++ b/manifest.go @@ -9,6 +9,7 @@ import ( "fmt" "io" "reflect" + "regexp" "sort" "github.com/golang/dep/internal/gps" @@ -70,9 +71,16 @@ func validateManifest(s string) ([]error, error) { case "name", "branch", "version", "source": // valid key case "revision": - // Check if sha1 hash is abbreviated - if valueStr, ok := value.(string); ok && len(valueStr) < 40 { - errs = append(errs, fmt.Errorf("sha1 hash %q should not be in abbreviated form", valueStr)) + if valueStr, ok := value.(string); ok { + // Check if sha1 hash is abbreviated + validGit := regexp.MustCompile("[a-f0-9]{40}").MatchString(valueStr) + if !validGit { + // Check for valid bzr revision-id + validBzr := regexp.MustCompile(".*-[0-9]{14}(-[a-zA-Z0-9]+)?").MatchString(valueStr) + if !validBzr { + errs = append(errs, fmt.Errorf("revision %q should not be in abbreviated form", valueStr)) + } + } } case "metadata": // Check if metadata is of Map type diff --git a/manifest_test.go b/manifest_test.go index 7ba311fa10..43cdf68d26 100644 --- a/manifest_test.go +++ b/manifest_test.go @@ -212,7 +212,23 @@ func TestValidateManifest(t *testing.T) { name = "github.com/foo/bar" revision = "b86ad16" `, - want: []error{errors.New("sha1 hash \"b86ad16\" should not be in abbreviated form")}, + want: []error{errors.New("revision \"b86ad16\" should not be in abbreviated form")}, + }, + { + tomlString: ` + [[dependencies]] + name = "bazaar.foobar.com/~bzr/trunk" + revision = "foo@bar.com-12345-wiuilyamo9ian0m7" + `, + want: []error{errors.New("revision \"foo@bar.com-12345-wiuilyamo9ian0m7\" should not be in abbreviated form")}, + }, + { + tomlString: ` + [[dependencies]] + name = "bazaar.foobar.com/~bzr/trunk" + revision = "foo@bar.com-20161116211307-wiuilyamo9ian0m7" + `, + want: []error{}, }, } From a26329bd15f6270cbf3c662a70cf560079f4df23 Mon Sep 17 00:00:00 2001 From: zak Date: Tue, 16 May 2017 15:10:20 +0100 Subject: [PATCH 3/5] Update variable naming and fix typo in comment --- manifest.go | 4 ++-- manifest_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/manifest.go b/manifest.go index afe729e5a0..2c82fecb1b 100644 --- a/manifest.go +++ b/manifest.go @@ -73,8 +73,8 @@ func validateManifest(s string) ([]error, error) { case "revision": if valueStr, ok := value.(string); ok { // Check if sha1 hash is abbreviated - validGit := regexp.MustCompile("[a-f0-9]{40}").MatchString(valueStr) - if !validGit { + validHash := regexp.MustCompile("[a-f0-9]{40}").MatchString(valueStr) + if !validHash { // Check for valid bzr revision-id validBzr := regexp.MustCompile(".*-[0-9]{14}(-[a-zA-Z0-9]+)?").MatchString(valueStr) if !validBzr { diff --git a/manifest_test.go b/manifest_test.go index 43cdf68d26..bc47cec353 100644 --- a/manifest_test.go +++ b/manifest_test.go @@ -232,7 +232,7 @@ func TestValidateManifest(t *testing.T) { }, } - // constains for error + // contains for error contains := func(s []error, e error) bool { for _, a := range s { if a.Error() == e.Error() { From aff937071c584d84f82c4c910b23d1c177eb30e5 Mon Sep 17 00:00:00 2001 From: zak Date: Thu, 18 May 2017 08:23:51 +0100 Subject: [PATCH 4/5] Regex improvements and reuse Update the regex to match that from the bzr revision-id generation tests and reuse it's compiled form. Update to match sha1 revision hash on hex string and length rather than regex. Add a test for an even length, abbreviated, sha1 revision hash. --- manifest.go | 9 +++++---- manifest_test.go | 8 ++++++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/manifest.go b/manifest.go index 2c82fecb1b..7eac7cb1bd 100644 --- a/manifest.go +++ b/manifest.go @@ -6,6 +6,7 @@ package dep import ( "bytes" + "encoding/hex" "fmt" "io" "reflect" @@ -51,6 +52,7 @@ func validateManifest(s string) ([]error, error) { // Convert tree to a map manifest := tree.ToMap() + bzrRevID := regexp.MustCompile(".*-\\d{14}-[a-z0-9]{16}") // Look for unknown fields and collect errors for prop, val := range manifest { switch prop { @@ -73,11 +75,10 @@ func validateManifest(s string) ([]error, error) { case "revision": if valueStr, ok := value.(string); ok { // Check if sha1 hash is abbreviated - validHash := regexp.MustCompile("[a-f0-9]{40}").MatchString(valueStr) - if !validHash { + _, err = hex.DecodeString(valueStr) + if err != nil || len(valueStr) != 40 { // Check for valid bzr revision-id - validBzr := regexp.MustCompile(".*-[0-9]{14}(-[a-zA-Z0-9]+)?").MatchString(valueStr) - if !validBzr { + if !bzrRevID.MatchString(valueStr) { errs = append(errs, fmt.Errorf("revision %q should not be in abbreviated form", valueStr)) } } diff --git a/manifest_test.go b/manifest_test.go index bc47cec353..479c9c310b 100644 --- a/manifest_test.go +++ b/manifest_test.go @@ -214,6 +214,14 @@ func TestValidateManifest(t *testing.T) { `, want: []error{errors.New("revision \"b86ad16\" should not be in abbreviated form")}, }, + { + tomlString: ` + [[dependencies]] + name = "github.com/foo/bar" + revision = "867f832e" + `, + want: []error{errors.New("revision \"867f832e\" should not be in abbreviated form")}, + }, { tomlString: ` [[dependencies]] From 6099d485b586f7a8be20308de9fc5cf9b1f847ff Mon Sep 17 00:00:00 2001 From: zak Date: Thu, 18 May 2017 09:04:50 +0100 Subject: [PATCH 5/5] Update to use raw string in regex Use raw string with backticks to avoid having to double escape inside a regex. --- manifest.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/manifest.go b/manifest.go index 7eac7cb1bd..39ebfdf412 100644 --- a/manifest.go +++ b/manifest.go @@ -52,7 +52,7 @@ func validateManifest(s string) ([]error, error) { // Convert tree to a map manifest := tree.ToMap() - bzrRevID := regexp.MustCompile(".*-\\d{14}-[a-z0-9]{16}") + bzrRevID := regexp.MustCompile(`.*-\d{14}-[a-z0-9]{16}`) // Look for unknown fields and collect errors for prop, val := range manifest { switch prop {