devpkg: better handling of escapes in flakeref paths (#1598)

Split flakeref paths before unescaping to properly handle encoded
slashes (%2F). Use `URL.String()` to ensure that unescaped characters
get re-escaped for `FlakeRef.URL`.

Author: gcurtis

internal/devpkg/flakeref.go

@@ -107,17 +107,10 @@ func parseFlakeURLRef(ref string) (parsed FlakeRef, fragment string, err error)
 		// [flake:]<flake-id>(/<rev-or-ref>(/rev)?)?
 
 		parsed.Type = "indirect"
-
-		// "indirect" is parsed as a path, "flake:indirect" is parsed as
-		// opaque because it has a scheme.
-		path := refURL.Path
-		if path == "" {
-			path, err = url.PathUnescape(refURL.Opaque)
-			if err != nil {
-				path = refURL.Opaque
-			}
+		split, err := splitPathOrOpaque(refURL)
+		if err != nil {
+			return FlakeRef{}, "", redact.Errorf("parse flake reference URL path: %v", err)
 		}
-		split := strings.SplitN(path, "/", 3)
 		parsed.ID = split[0]
 		if len(split) > 1 {
 			if isGitHash(split[1]) {
@@ -136,7 +129,7 @@ func parseFlakeURLRef(ref string) (parsed FlakeRef, fragment string, err error)
 		if refURL.Path == "" {
 			parsed.Path, err = url.PathUnescape(refURL.Opaque)
 			if err != nil {
-				parsed.Path = refURL.Opaque
+				return FlakeRef{}, "", err
 			}
 		} else {
 			parsed.Path = refURL.Path
@@ -147,16 +140,20 @@ func parseFlakeURLRef(ref string) (parsed FlakeRef, fragment string, err error)
 		} else {
 			parsed.Type = "file"
 		}
-		parsed.URL = ref
 		parsed.Dir = refURL.Query().Get("dir")
+		parsed.URL = refURL.String()
 	case "tarball+http", "tarball+https", "tarball+file":
 		parsed.Type = "tarball"
 		parsed.Dir = refURL.Query().Get("dir")
-		parsed.URL = ref[8:] // remove tarball+
+
+		refURL.Scheme = refURL.Scheme[8:] // remove tarball+
+		parsed.URL = refURL.String()
 	case "file+http", "file+https", "file+file":
 		parsed.Type = "file"
 		parsed.Dir = refURL.Query().Get("dir")
-		parsed.URL = ref[5:] // remove file+
+
+		refURL.Scheme = refURL.Scheme[5:] // remove file+
+		parsed.URL = refURL.String()
 	case "git", "git+http", "git+https", "git+ssh", "git+git", "git+file":
 		parsed.Type = "git"
 		q := refURL.Query()
@@ -185,17 +182,10 @@ func parseGitHubFlakeRef(refURL *url.URL, parsed *FlakeRef) error {
 	// github:<owner>/<repo>(/<rev-or-ref>)?(\?<params>)?
 
 	parsed.Type = "github"
-	path := refURL.Path
-	if path == "" {
-		var err error
-		path, err = url.PathUnescape(refURL.Opaque)
-		if err != nil {
-			path = refURL.Opaque
-		}
+	split, err := splitPathOrOpaque(refURL)
+	if err != nil {
+		return err
 	}
-	path = strings.TrimPrefix(path, "/")
-
-	split := strings.SplitN(path, "/", 3)
 	parsed.Owner = split[0]
 	parsed.Repo = split[1]
 	if len(split) > 2 {
@@ -280,7 +270,7 @@ func (f FlakeRef) String() string {
 		}
 		url := &url.URL{
 			Scheme:   "github",
-			Opaque:   buildOpaquePath(f.Owner, f.Repo, f.Rev, f.Ref),
+			Opaque:   buildEscapedPath(f.Owner, f.Repo, f.Rev, f.Ref),
 			RawQuery: buildQueryString("host", f.Host, "dir", f.Dir),
 		}
 		return url.String()
@@ -290,7 +280,7 @@ func (f FlakeRef) String() string {
 		}
 		url := &url.URL{
 			Scheme:   "flake",
-			Opaque:   buildOpaquePath(f.ID, f.Ref, f.Rev),
+			Opaque:   buildEscapedPath(f.ID, f.Ref, f.Rev),
 			RawQuery: buildQueryString("dir", f.Dir),
 		}
 		return url.String()
@@ -301,7 +291,7 @@ func (f FlakeRef) String() string {
 		f.Path = path.Clean(f.Path)
 		url := &url.URL{
 			Scheme: "path",
-			Opaque: buildOpaquePath(strings.Split(f.Path, "/")...),
+			Opaque: buildEscapedPath(strings.Split(f.Path, "/")...),
 		}
 
 		// Add the / prefix back if strings.Split removed it.
@@ -359,9 +349,40 @@ func isArchive(path string) bool {
 		strings.HasSuffix(path, ".zip")
 }
 
-// buildOpaquePath escapes and joins path elements for a URL flakeref. The
+// splitPathOrOpaque splits a URL path by '/'. If the path is empty, it splits
+// the opaque instead. Splitting happens before unescaping the path or opaque,
+// ensuring that path elements with an encoded '/' (%2F) are not split.
+// For example, "/dir/file%2Fname" becomes the elements "dir" and "file/name".
+func splitPathOrOpaque(u *url.URL) ([]string, error) {
+	upath := u.EscapedPath()
+	if upath == "" {
+		upath = u.Opaque
+	}
+	upath = strings.TrimSpace(upath)
+	if upath == "" {
+		return nil, nil
+	}
+
+	// We don't want an empty element if the path is rooted.
+	if upath[0] == '/' {
+		upath = upath[1:]
+	}
+	upath = path.Clean(upath)
+
+	var err error
+	split := strings.Split(upath, "/")
+	for i := range split {
+		split[i], err = url.PathUnescape(split[i])
+		if err != nil {
+			return nil, err
+		}
+	}
+	return split, nil
+}
+
+// buildEscapedPath escapes and joins path elements for a URL flakeref. The
 // resulting path is cleaned according to url.JoinPath.
-func buildOpaquePath(elem ...string) string {
+func buildEscapedPath(elem ...string) string {
 	for i := range elem {
 		elem[i] = url.PathEscape(elem[i])
 	}

internal/devpkg/flakeref_test.go

@@ -17,6 +17,9 @@ func TestParseFlakeRef(t *testing.T) {
 		// Not a path and not a valid URL.
 		"://bad/url": {},
 
+		// Invalid escape.
+		"path:./relative/my%flake": {},
+
 		// Path-like references start with a '.' or '/'.
 		// This distinguishes them from indirect references
 		// (./nixpkgs is a directory; nixpkgs is an indirect).
@@ -60,7 +63,7 @@ func TestParseFlakeRef(t *testing.T) {
 		// Indirect references.
 		"flake:indirect":          {Type: "indirect", ID: "indirect"},
 		"flake:indirect/ref":      {Type: "indirect", ID: "indirect", Ref: "ref"},
-		"flake:indirect/my%20ref": {Type: "indirect", ID: "indirect", Ref: "my ref"},
+		"flake:indirect/my%2Fref": {Type: "indirect", ID: "indirect", Ref: "my/ref"},
 		"flake:indirect/5233fd2ba76a3accb5aaa999c00509a11fd0793c":     {Type: "indirect", ID: "indirect", Rev: "5233fd2ba76a3accb5aaa999c00509a11fd0793c"},
 		"flake:indirect/ref/5233fd2ba76a3accb5aaa999c00509a11fd0793c": {Type: "indirect", ID: "indirect", Ref: "ref", Rev: "5233fd2ba76a3accb5aaa999c00509a11fd0793c"},
 
@@ -139,11 +142,6 @@ func TestParseFlakeRef(t *testing.T) {
 		"http://example.com/flake":            {Type: "file", URL: "http://example.com/flake"},
 		"http://example.com/flake.git":        {Type: "file", URL: "http://example.com/flake.git"},
 		"http://example.com/flake?dir=subdir": {Type: "file", URL: "http://example.com/flake?dir=subdir", Dir: "subdir"},
-
-		// Interpret opaques with invalid escapes literally.
-		"path:./relative/my%flake": {Type: "path", Path: "./relative/my%flake"},
-		"flake:indirect/my%ref":    {Type: "indirect", ID: "indirect", Ref: "my%ref"},
-		"github:NixOS/nix/my%ref":  {Type: "github", Owner: "NixOS", Repo: "nix", Ref: "my%ref"},
 	}
 
 	for ref, want := range cases {