From 0ca9ab8ee7029e0957121954db900b677e2157bd Mon Sep 17 00:00:00 2001 From: per1234 Date: Tue, 19 Jan 2021 14:26:52 -0800 Subject: [PATCH] Add check for submission URL being a Git repository With the current submission system, it's common for people to provide the tag/release URL rather than the repository URL. I think it likely the same sort of thing will occur with the new system. Previously, a very naive check was done for the path component of the URL to have two levels. It turns out that the Gitlab repository URLs have more levels than that. The superior approach is to use `git ls-remote` to verify that the URL is a Git repository. --- manager/main.go | 36 ++++++++++++++++++------------------ manager/main_test.go | 20 +++++--------------- 2 files changed, 23 insertions(+), 33 deletions(-) diff --git a/manager/main.go b/manager/main.go index bf67498e..b268a58b 100644 --- a/manager/main.go +++ b/manager/main.go @@ -227,11 +227,7 @@ func populateSubmission(submissionURL string, listPath *paths.Path) (submissionT } // Resolve redirects and normalize. - normalizedURLObject, err := normalizeURL(httpResponse.Request.URL) - if err != nil { - submission.Error = err.Error() - return submission, "" - } + normalizedURLObject := normalizeURL(httpResponse.Request.URL) submission.NormalizedURL = normalizedURLObject.String() @@ -241,6 +237,17 @@ func populateSubmission(submissionURL string, listPath *paths.Path) (submissionT return submission, "" } + // Check if URL is a Git repository + err = exec.Command("git", "ls-remote", normalizedURLObject.String()).Run() + if err != nil { + if _, ok := err.(*exec.ExitError); ok { + submission.Error = "Submission URL is not a Git clone URL (e.g., https://github.com/arduino-libraries/Servo)." + return submission, "" + } + + panic(err) + } + // Check if the URL is already in the index. listLines, err := listPath.ReadFileAsLines() occurrences := 0 @@ -249,11 +256,8 @@ func populateSubmission(submissionURL string, listPath *paths.Path) (submissionT if err != nil { panic(err) // All list items have already passed parsing so something is broken if this happens. } - normalizedListURLObject, err := normalizeURL(listURLObject) - if err != nil { - panic(fmt.Errorf("When processing list item %s: %s", listURL, err)) // All list items have already been through normalization without error so something is broken if this happens. - } + normalizedListURLObject := normalizeURL(listURLObject) if normalizedListURLObject.String() == normalizedURLObject.String() { occurrences++ if occurrences > 1 { @@ -350,21 +354,17 @@ func populateSubmission(submissionURL string, listPath *paths.Path) (submissionT } // normalizeURL converts the URL into the standardized format used in the index. -func normalizeURL(rawURL *url.URL) (url.URL, error) { +func normalizeURL(rawURL *url.URL) url.URL { normalizedPath := strings.TrimRight(rawURL.Path, "/") if !strings.HasSuffix(normalizedPath, ".git") { normalizedPath += ".git" } - if len(strings.Split(normalizedPath, "/")) != 3 { - return url.URL{}, fmt.Errorf("Submission URL must point to the root of the repository") - } return url.URL{ - Scheme: "https", - Host: rawURL.Host, - Path: normalizedPath, - }, - nil + Scheme: "https", + Host: rawURL.Host, + Path: normalizedPath, + } } func uRLIsUnder(childURL url.URL, parentCandidates []string) bool { diff --git a/manager/main_test.go b/manager/main_test.go index 5e2b68a8..14228996 100644 --- a/manager/main_test.go +++ b/manager/main_test.go @@ -13,7 +13,6 @@ package main import ( - "fmt" "net/url" "testing" @@ -154,14 +153,11 @@ func Test_normalizeURL(t *testing.T) { testName string rawURL string expectedNormalizedURL string - expectedError string }{ - {"Trailing slash", "https://github.com/foo/bar/", "https://github.com/foo/bar.git", ""}, - {".git suffix", "https://github.com/foo/bar.git", "https://github.com/foo/bar.git", ""}, - {"Too few path elements", "https://github.com/foo", "", "Submission URL must point to the root of the repository"}, - {"Too many path elements", "https://github.com/foo/bar/baz", "", "Submission URL must point to the root of the repository"}, - {"http://", "http://github.com/foo/bar", "https://github.com/foo/bar.git", ""}, - {"git://", "git://github.com/foo/bar", "https://github.com/foo/bar.git", ""}, + {"Trailing slash", "https://github.com/foo/bar/", "https://github.com/foo/bar.git"}, + {".git suffix", "https://github.com/foo/bar.git", "https://github.com/foo/bar.git"}, + {"http://", "http://github.com/foo/bar", "https://github.com/foo/bar.git"}, + {"git://", "git://github.com/foo/bar", "https://github.com/foo/bar.git"}, } for _, testTable := range testTables { @@ -170,13 +166,7 @@ func Test_normalizeURL(t *testing.T) { expectedNormalizedURL, err := url.Parse(testTable.expectedNormalizedURL) require.Nil(t, err) - normalizedURL, err := normalizeURL(rawURL) - assert.Equal(t, *expectedNormalizedURL, normalizedURL, testTable.testName) - if testTable.expectedError == "" { - assert.Nil(t, err, testTable.testName) - } else { - assert.Equal(t, fmt.Errorf(testTable.expectedError), err, testTable.testName) - } + assert.Equal(t, *expectedNormalizedURL, normalizeURL(rawURL), testTable.testName) } }