Repository: osbuild/osbuild-composer
Pull request #1367 (2021-04-19 19:12:44)

[rhel-8.4.0] ostree url parsing
Cherry picked #1329 for RHEL 8.4.

Planned for z-stream. [rhbz#1951192](https://bugzilla.redhat.com/show_bug.cgi?id=1951192)



Patch:
diff --git a/docs/news/unreleased/ostree-ref-handling.md b/docs/news/unreleased/ostree-ref-handling.md
new file mode 100644
index 000000000..84ca55d23
--- /dev/null
+++ b/docs/news/unreleased/ostree-ref-handling.md
@@ -0,0 +1,3 @@
+# Improve OSTree Repository URL and Ref parsing
+
+If the OSTree Repository URL did not end in a `/` the parsing would fail with a less-than-useful error message.  This has been fixed. Error messages for different failure cases have also been improved.
diff --git a/internal/weldr/api.go b/internal/weldr/api.go
index 4ff555f47..3572e3b5e 100644
--- a/internal/weldr/api.go
+++ b/internal/weldr/api.go
@@ -89,6 +89,7 @@ func (api *API) systemRepoNames() (names []string) {
 }
 
 var ValidBlueprintName = regexp.MustCompile(`^[a-zA-Z0-9._-]+$`)
+var ValidOSTreeRef = regexp.MustCompile(`^(?:[\w\d][-._\w\d]*\/)*[\w\d][-._\w\d]*$`)
 
 func New(rpmmd rpmmd.RPMMD, arch distro.Arch, distro distro.Distro, repos []rpmmd.RepoConfig, logger *log.Logger, store *store.Store, workers *worker.Server, compatOutputDir string) *API {
 	api := &API{
@@ -372,6 +373,18 @@ func verifyStringsWithRegex(writer http.ResponseWriter, strings []string, re *re
 	return true
 }
 
+func verifyOSTreeRef(writer http.ResponseWriter, ref string, re *regexp.Regexp) bool {
+	if len(ref) > 0 && re.MatchString(ref) {
+		return true
+	}
+	errors := responseError{
+		ID:  "InvalidChars",
+		Msg: "Invalid ostree ref",
+	}
+	statusResponseError(writer, http.StatusBadRequest, errors)
+	return false
+}
+
 func statusResponseError(writer http.ResponseWriter, code int, errors ...responseError) {
 	type reply struct {
 		Status bool            `json:"status"`
@@ -1805,18 +1818,14 @@ func ostreeResolveRef(location, ref string) (string, error) {
 	if err != nil {
 		return "", err
 	}
-	u, err = u.Parse("refs/heads/")
-	if err != nil {
-		return "", err
-	}
-	u, err = u.Parse(ref)
-	if err != nil {
-		return "", err
-	}
+	u.Path = path.Join(u.Path, "refs/heads/", ref)
 	resp, err := http.Get(u.String())
 	if err != nil {
 		return "", err
 	}
+	if resp.StatusCode != http.StatusOK {
+		return "", fmt.Errorf("ostree repository %q returned status: %s", u.String(), resp.Status)
+	}
 	body, err := ioutil.ReadAll(resp.Body)
 	if err != nil {
 		return "", err
@@ -1825,7 +1834,7 @@ func ostreeResolveRef(location, ref string) (string, error) {
 	// Check that this is at least a hex string.
 	_, err = hex.DecodeString(parent)
 	if err != nil {
-		return "", err
+		return "", fmt.Errorf("ostree repository %q returned invalid reference", u.String())
 	}
 	return parent, nil
 }
@@ -1905,6 +1914,8 @@ func (api *API) composeHandler(writer http.ResponseWriter, request *http.Request
 	// set default ostree ref, if one not provided
 	if cr.OSTree.Ref == "" {
 		cr.OSTree.Ref = imageType.OSTreeRef()
+	} else if !verifyOSTreeRef(writer, cr.OSTree.Ref, ValidOSTreeRef) {
+		return
 	}
 
 	if !verifyStringsWithRegex(writer, []string{cr.BlueprintName}, ValidBlueprintName) {


Test patch:
diff --git a/internal/weldr/api_test.go b/internal/weldr/api_test.go
index 535122659..201cc7c0a 100644
--- a/internal/weldr/api_test.go
+++ b/internal/weldr/api_test.go
@@ -4,6 +4,7 @@ import (
 	"archive/tar"
 	"bytes"
 	"encoding/json"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"math/rand"
@@ -26,6 +27,7 @@ import (
 
 	"github.com/BurntSushi/toml"
 	"github.com/google/go-cmp/cmp"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -671,7 +673,8 @@ func TestCompose(t *testing.T) {
 		{false, "POST", "/api/v1/compose", `{"blueprint_name": "test","compose_type":"qcow2","branch":"master","upload":{"image_name":"test_upload","provider":"aws","settings":{"region":"frankfurt","accessKeyID":"accesskey","secretAccessKey":"secretkey","bucket":"clay","key":"imagekey"}}}`, http.StatusOK, `{"status": true}`, expectedComposeLocalAndAws, []string{"build_id"}},
 		{false, "POST", "/api/v1/compose", `{"blueprint_name": "test","compose_type":"qcow2","branch":"master","ostree":{"ref":"refid","parent":"parentid","url":""}}`, http.StatusOK, `{"status": true}`, expectedComposeOSTreeRef, []string{"build_id"}},
 		{false, "POST", "/api/v1/compose?test=2", `{"blueprint_name": "test","compose_type":"qcow2","branch":"master","ostree":{"ref":"refid","parent":"","url":"http://ostree/"}}`, http.StatusOK, `{"status": true}`, expectedComposeOSTreeURL, []string{"build_id"}},
-		{false, "POST", "/api/v1/compose", `{"blueprint_name": "test","compose_type":"qcow2","branch":"master","ostree":{"ref":"refid","parent":"","url":"invalid-url"}}`, http.StatusBadRequest, `{"status":false,"errors":[{"id":"OSTreeCommitError","msg":"Get \"/refs/heads/refid\": unsupported protocol scheme \"\""}]}`, nil, []string{"build_id"}},
+		{false, "POST", "/api/v1/compose", `{"blueprint_name": "test","compose_type":"qcow2","branch":"master","ostree":{"ref":"refid","parent":"","url":"invalid-url"}}`, http.StatusBadRequest, `{"status":false,"errors":[{"id":"OSTreeCommitError","msg":"Get \"invalid-url/refs/heads/refid\": unsupported protocol scheme \"\""}]}`, nil, []string{"build_id"}},
+		{false, "POST", "/api/v1/compose", `{"blueprint_name": "test","compose_type":"qcow2","branch":"master","ostree":{"ref":"/bad/ref","parent":"","url":"http://ostree/"}}`, http.StatusBadRequest, `{"status":false,"errors":[{"id":"InvalidChars","msg":"Invalid ostree ref"}]}`, expectedComposeOSTreeURL, []string{"build_id"}},
 	}
 
 	tempdir, err := ioutil.TempDir("", "weldr-tests-")
@@ -1339,3 +1342,55 @@ func TestModulesList(t *testing.T) {
 		test.TestRoute(t, api, true, "GET", c.Path, ``, c.ExpectedStatus, c.ExpectedJSON)
 	}
 }
+
+func TestOstreeResolveRef(t *testing.T) {
+	goodRef := "5330bb1b8820944567f519de66ad6354c729b6b490dea1c5a7ba320c9f147c58"
+	badRef := "<html>not a ref</html>"
+
+	handler := http.NewServeMux()
+	handler.HandleFunc("/refs/heads/rhel/8/x86_64/edge", func(w http.ResponseWriter, r *http.Request) {
+		http.NotFound(w, r)
+	})
+	handler.HandleFunc("/refs/heads/test_forbidden", func(w http.ResponseWriter, r *http.Request) {
+		http.Error(w, "", http.StatusForbidden)
+	})
+	handler.HandleFunc("/refs/heads/get_bad_ref", func(w http.ResponseWriter, r *http.Request) {
+		fmt.Fprint(w, badRef)
+	})
+
+	handler.HandleFunc("/refs/heads/test_redir", func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, "/refs/heads/valid/ostree/ref", http.StatusFound)
+	})
+	handler.HandleFunc("/refs/heads/valid/ostree/ref", func(w http.ResponseWriter, r *http.Request) {
+		fmt.Fprint(w, goodRef)
+	})
+
+	srv := httptest.NewServer(handler)
+	defer srv.Close()
+
+	type input struct {
+		location string
+		ref      string
+	}
+	validCases := map[input]string{
+		{srv.URL, "test_redir"}:       goodRef,
+		{srv.URL, "valid/ostree/ref"}: goodRef,
+	}
+	for in, expOut := range validCases {
+		out, err := ostreeResolveRef(in.location, in.ref)
+		assert.NoError(t, err)
+		assert.Equal(t, expOut, out)
+	}
+
+	errCases := map[input]string{
+		{"not-a-url", "a-bad-ref"}:             "Get \"not-a-url/refs/heads/a-bad-ref\": unsupported protocol scheme \"\"",
+		{"http://0.0.0.0:10/repo", "whatever"}: "Get \"http://0.0.0.0:10/repo/refs/heads/whatever\": dial tcp 0.0.0.0:10: connect: connection refused",
+		{srv.URL, "rhel/8/x86_64/edge"}:        fmt.Sprintf("ostree repository \"%s/refs/heads/rhel/8/x86_64/edge\" returned status: 404 Not Found", srv.URL),
+		{srv.URL, "test_forbidden"}:            fmt.Sprintf("ostree repository \"%s/refs/heads/test_forbidden\" returned status: 403 Forbidden", srv.URL),
+		{srv.URL, "get_bad_ref"}:               fmt.Sprintf("ostree repository \"%s/refs/heads/get_bad_ref\" returned invalid reference", srv.URL),
+	}
+	for in, expMsg := range errCases {
+		_, err := ostreeResolveRef(in.location, in.ref)
+		assert.EqualError(t, err, expMsg)
+	}
+}
diff --git a/test/cases/ostree-ng.sh b/test/cases/ostree-ng.sh
index 418be05a0..0b30e58b3 100755
--- a/test/cases/ostree-ng.sh
+++ b/test/cases/ostree-ng.sh
@@ -65,7 +65,7 @@ OS_VARIANT="rhel8-unknown"
 TEST_UUID=$(uuidgen)
 IMAGE_KEY="osbuild-composer-ostree-test-${TEST_UUID}"
 GUEST_ADDRESS=192.168.100.50
-URL=http://192.168.100.1/repo/
+URL=http://192.168.100.1/repo
 
 # Set up temporary files.
 TEMPDIR=$(mktemp -d)
@@ -127,6 +127,7 @@ build_image() {
             }
         }" | tee "$COMPOSE_START"
     else
+        # Test ref begining with /
         sudo curl --silent --header "Content-Type: application/json" --unix-socket /run/weldr/api.socket http://localhost/api/v1/compose --data "{
             \"blueprint_name\": \"$blueprint_name\",
             \"compose_type\": \"$image_type\",
@@ -308,7 +309,8 @@ sudo composer-cli blueprints push "$BLUEPRINT_FILE"
 sudo composer-cli blueprints depsolve installer
 
 # Build installer image.
-build_image installer rhel-edge-installer "$URL"
+# Test --url arg following by URL with tailling slash for bz#1942029
+build_image installer rhel-edge-installer "${URL}/"
 
 # Download the image
 greenprint "📥 Downloading the installer image"
@@ -459,6 +461,7 @@ sudo composer-cli blueprints push "$BLUEPRINT_FILE"
 sudo composer-cli blueprints depsolve upgrade
 
 # Build upgrade image.
+# Test --url arg following by URL without tailling slash for bz#1942029
 build_image upgrade rhel-edge-container "$URL"
 
 # Download the image
@@ -491,7 +494,7 @@ done;
 
 # Get ostree commit value.
 greenprint "🕹 Get ostree upgrade commit value"
-UPGRADE_HASH=$(curl ${URL}refs/heads/"${OSTREE_REF}")
+UPGRADE_HASH=$(curl ${URL}/refs/heads/"${OSTREE_REF}")
 
 # Clean compose and blueprints.
 greenprint "🧽 Clean up upgrade blueprint and compose"