Escape group and repository names in every delivery key so a dotted branch keys (issue 1323)

go/delivery/delivery.go

@@ -35,6 +35,7 @@ package delivery
 
 import (
 	"bytes"
+	"encoding/hex"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -105,6 +106,49 @@ func checkName(kind, name string) error {
 	return nil
 }
 
+// EscapeName is name as a key part: a character outside letters, digits, `-`, `/` and `_` is written `=` and its
+// two lowercase hex digits per byte, and `=` itself is escaped, so any non-empty name keys and a name of allowed
+// characters only keys as it is. UnescapeName is its inverse.
+func EscapeName(name string) string {
+	var b strings.Builder
+	for i := 0; i < len(name); i++ {
+		c := name[i]
+		switch {
+		case c >= 'a' && c <= 'z', c >= 'A' && c <= 'Z', c >= '0' && c <= '9', c == '-', c == '/', c == '_':
+			b.WriteByte(c)
+		default:
+			fmt.Fprintf(&b, "=%02x", c)
+		}
+	}
+	return b.String()
+}
+
+// UnescapeName is the inverse of EscapeName; it refuses a part with a bare `=` or any character EscapeName leaves
+// escaped.
+func UnescapeName(part string) (string, error) {
+	var out []byte
+	for i := 0; i < len(part); i++ {
+		c := part[i]
+		if c != '=' {
+			if !nameOK.MatchString(string(c)) {
+				return "", fmt.Errorf("delivery: %q holds %q, which a key part escapes", part, c)
+			}
+			out = append(out, c)
+			continue
+		}
+		if i+3 > len(part) {
+			return "", fmt.Errorf("delivery: %q ends in an incomplete escape", part)
+		}
+		v, err := hex.DecodeString(part[i+1 : i+3])
+		if err != nil || part[i+1:i+3] != strings.ToLower(part[i+1:i+3]) {
+			return "", fmt.Errorf("delivery: %q holds a malformed escape %q", part, part[i:i+3])
+		}
+		out = append(out, v[0])
+		i += 2
+	}
+	return string(out), nil
+}
+
 // HaveKey is the key, in HaveBucket, of a module's have on a node: `<node>.<module>`. A module named `node` is
 // refused, since `<node>.node` is the node's own facts.
 func HaveKey(node, module string) (string, error) {

go/delivery/gate.go

@@ -56,23 +56,21 @@ var hashOK = regexp.MustCompile(`^[0-9a-f]{12}$`)
 
 // MergesKey is the key, in MergesBucket, of a group: `<group>`.
 func MergesKey(group string) (string, error) {
-	if err := checkName("group", group); err != nil {
-		return "", err
-	}
-	return group, nil
+	return groupKey(group)
 }
 
 // MergeAskKey is the key, in AsksBucket, of the ask for merging a group at these heads: `merge.<group>.<heads
 // hash>`. The teller's answer has the same key in AnswersBucket (AnswerKey).
 func MergeAskKey(group string, heads []Head) (string, error) {
-	if err := checkName("group", group); err != nil {
+	part, err := groupKey(group)
+	if err != nil {
 		return "", err
 	}
 	hash, err := HeadsHash(heads)
 	if err != nil {
 		return "", err
 	}
-	return AskKindMerge + "." + group + "." + hash, nil
+	return AskKindMerge + "." + part + "." + hash, nil
 }
 
 // ParseAskKey is the inverse of MergeAskKey: the kind, the group and the heads hash.
@@ -81,13 +79,14 @@ func ParseAskKey(key string) (kind, group, hash string, err error) {
 	if len(parts) != 3 || parts[0] != AskKindMerge {
 		return "", "", "", fmt.Errorf("delivery: %q is not an ask key, merge.<group>.<heads hash>", key)
 	}
-	if err := checkName("group", parts[1]); err != nil {
+	group, err = parseGroupKey(parts[1])
+	if err != nil {
 		return "", "", "", fmt.Errorf("delivery: %q is not an ask key: %w", key, err)
 	}
 	if !hashOK.MatchString(parts[2]) {
 		return "", "", "", fmt.Errorf("delivery: %q is not an ask key: %q is not 12 hex digits", key, parts[2])
 	}
-	return parts[0], parts[1], parts[2], nil
+	return parts[0], group, parts[2], nil
 }
 
 // AnswerKey is the key, in AnswersBucket, of the teller's answer to the ask at askKey: the same string.
@@ -147,7 +146,8 @@ func ParseOwedAct(act string) (kind, context string, err error) { return checkOw
 // `<repository>.<number>.<head>.<act>`, the act being `status=<context>` (mesh/merge-gate), `view`, `note` or
 // `comment`.
 func OwedKey(repository string, number int, head, act string) (string, error) {
-	if err := checkRepository(repository); err != nil {
+	repo, err := escapeRepository(repository)
+	if err != nil {
 		return "", err
 	}
 	if number <= 0 {
@@ -159,7 +159,7 @@ func OwedKey(repository string, number int, head, act string) (string, error) {
 	if _, _, err := checkOwedAct(act); err != nil {
 		return "", err
 	}
-	return repository + "." + strconv.Itoa(number) + "." + head + "." + act, nil
+	return repo + "." + strconv.Itoa(number) + "." + head + "." + act, nil
 }
 
 // ParseOwedKey is the inverse of OwedKey; act is whole, as `status=<context>` for a status.
@@ -172,10 +172,14 @@ func ParseOwedKey(key string) (repository string, number int, head, act string,
 	if convErr != nil {
 		return "", 0, "", "", fmt.Errorf("delivery: %q is not an owed key: %q is not a pull request number", key, parts[1])
 	}
-	if _, err := OwedKey(parts[0], number, parts[2], parts[3]); err != nil {
+	repository, err = unescapeRepository(parts[0])
+	if err != nil {
 		return "", 0, "", "", fmt.Errorf("delivery: %q is not an owed key: %w", key, err)
 	}
-	return parts[0], number, parts[2], parts[3], nil
+	if _, err := OwedKey(repository, number, parts[2], parts[3]); err != nil {
+		return "", 0, "", "", fmt.Errorf("delivery: %q is not an owed key: %w", key, err)
+	}
+	return repository, number, parts[2], parts[3], nil
 }
 
 // Merge is the value of a merges key: the ask approved and, for each pull request, the head the approval expects
@@ -197,8 +201,8 @@ type MergePull struct {
 }
 
 func (m Merge) check() error {
-	if err := checkName("group", m.Group); err != nil {
-		return err
+	if m.Group == "" {
+		return fmt.Errorf("delivery: a merge names no group")
 	}
 	if m.Ask == "" {
 		return fmt.Errorf("delivery: a merge names no ask")

go/delivery/gate_test.go

@@ -21,8 +21,8 @@ func TestMergeKeys(t *testing.T) { // 1201/AC1
 	if k, err := MergesKey("feat/x"); err != nil || k != "feat/x" {
 		t.Fatalf("merges key = %q, %v", k, err)
 	}
-	if _, err := MergesKey("a.b"); err == nil {
-		t.Error("a group with a dot accepted")
+	if _, err := MergesKey(""); err == nil {
+		t.Error("an empty group accepted")
 	}
 	hh, _ := HeadsHash(gateHeads())
 	key, err := MergeAskKey("feat/x", gateHeads())
@@ -42,7 +42,7 @@ func TestMergeKeys(t *testing.T) { // 1201/AC1
 	if _, err := MergeAskKey("", gateHeads()); err == nil {
 		t.Error("an ask key without a group accepted")
 	}
-	for _, bad := range []string{"", "merge", "merge.g", "merge.g.h.x", "other.g.0123456789ab", "merge..0123456789ab", "merge.g.nothex"} {
+	for _, bad := range []string{"", "merge", "merge.g", "merge.g.h.x", "other.g.0123456789ab", "merge..0123456789ab", "merge.g.nothex", "merge.a=zz.0123456789ab", "merge.a=4.0123456789ab", "merge.a=4A.0123456789ab", "merge.a+b.0123456789ab"} {
 		if _, _, _, err := ParseAskKey(bad); err == nil {
 			t.Errorf("parse ask key %q accepted", bad)
 		}
@@ -299,3 +299,83 @@ func TestGateBuckets(t *testing.T) {
 		t.Error("a bucket or history differs from the decided layout")
 	}
 }
+
+// today's keys: names of letters, digits, - / and _ key exactly as before the escape.
+func TestKeysOfPlainNamesAreUnchanged(t *testing.T) { // 1323/AC2
+	hh, _ := HeadsHash(gateHeads())
+	for _, name := range []string{"feat/x", "fix/1323-escape", "feat/delivery_source-keys", "main", "A/B_c-9"} {
+		if k, err := MergesKey(name); err != nil || k != name {
+			t.Errorf("merges key %q = %q, %v", name, k, err)
+		}
+		if k, err := CandidatesKey(name); err != nil || k != name {
+			t.Errorf("candidates key %q = %q, %v", name, k, err)
+		}
+		if k, err := MergeAskKey(name, gateHeads()); err != nil || k != "merge."+name+"."+hh {
+			t.Errorf("ask key %q = %q, %v", name, k, err)
+		}
+	}
+	if k, err := OwedKey("novox/mesh-sdk", 36, "aaaa", "status=mesh/merge-gate"); err != nil || k != "novox/mesh-sdk.36.aaaa.status=mesh/merge-gate" {
+		t.Errorf("owed key = %q, %v", k, err)
+	}
+}
+
+func TestKeysEscapeAndRoundTrip(t *testing.T) { // 1323/AC1
+	hh, _ := HeadsHash(gateHeads())
+	for _, name := range []string{"fix/1.2", "feat/a+b", "feat/a=b", "a.b+c=d", "fix/ü", "x y"} {
+		esc := EscapeName(name)
+		if strings.ContainsAny(esc, ".+ ") {
+			t.Errorf("escape of %q = %q holds a dot, plus or space", name, esc)
+		}
+		if back, err := UnescapeName(esc); err != nil || back != name {
+			t.Errorf("unescape(%q) = %q, %v; want %q", esc, back, err, name)
+		}
+		if k, err := MergesKey(name); err != nil || k != esc {
+			t.Errorf("merges key %q = %q, %v", name, k, err)
+		}
+		ck, err := CandidatesKey(name)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if g, err := ParseCandidatesKey(ck); err != nil || g != name {
+			t.Errorf("candidates round trip %q = %q, %v", name, g, err)
+		}
+		ak, err := MergeAskKey(name, gateHeads())
+		if err != nil {
+			t.Fatal(err)
+		}
+		if _, g, h, err := ParseAskKey(ak); err != nil || g != name || h != hh {
+			t.Errorf("ask round trip %q = %q %q %v", name, g, h, err)
+		}
+		if AnswerKey(ak) != ak {
+			t.Error("answer key differs from the ask key")
+		}
+		// the ask and the answer of such a group encode and decode
+		ask := Ask{Group: name, Heads: gateHeads(), Ask: asks.Ask{ID: AskID(ak), Options: []asks.Option{{ID: "ok"}}}}
+		if data, err := EncodeAsk(ask); err != nil {
+			t.Errorf("encode ask for %q: %v", name, err)
+		} else if _, err := DecodeAsk(data); err != nil {
+			t.Errorf("decode ask for %q: %v", name, err)
+		}
+		repo := "novox/" + name[strings.Index(name, "/")+1:]
+		if !strings.Contains(name, "/") {
+			repo = "novox/" + name
+		}
+		ok, err := OwedKey(repo, 3, "aaaa", "view")
+		if err != nil {
+			t.Errorf("owed key for repository %q: %v", repo, err)
+			continue
+		}
+		if r, n, h, a, err := ParseOwedKey(ok); err != nil || r != repo || n != 3 || h != "aaaa" || a != "view" {
+			t.Errorf("owed round trip %q = %q %d %q %q %v", repo, r, n, h, a, err)
+		}
+	}
+	if EscapeName("a.b") != "a=2eb" || EscapeName("a=b") != "a=3db" || EscapeName("a+b") != "a=2bb" {
+		t.Errorf("escapes: %q %q %q", EscapeName("a.b"), EscapeName("a=b"), EscapeName("a+b"))
+	}
+	m := Merge{Group: "fix/1.2+x=y", Ask: "merge.x.0123456789ab", Pulls: []MergePull{{Head: gateHeads()[0], State: MergeMerging}}}
+	if data, err := EncodeMerge(m); err != nil {
+		t.Errorf("encode merge: %v", err)
+	} else if back, err := DecodeMerge(data); err != nil || back.Group != m.Group {
+		t.Errorf("decode merge: %v %v", back.Group, err)
+	}
+}

go/delivery/source.go

@@ -67,18 +67,62 @@ const SourcePrefix = "src1:"
 // CandidatesKey is the key, in CandidatesBucket, of a group: `<group>`, the branch name the group's pull requests
 // share (a slash is allowed); a lone pull request is a group of one under its branch name.
 func CandidatesKey(group string) (string, error) {
-	if err := checkName("group", group); err != nil {
+	return groupKey(group)
+}
+
+// ParseCandidatesKey is the inverse of CandidatesKey.
+func ParseCandidatesKey(key string) (group string, err error) {
+	if group, err = parseGroupKey(key); err != nil {
+		return "", fmt.Errorf("delivery: %q is not a candidates key: %w", key, err)
+	}
+	return group, nil
+}
+
+// groupKey is a group name as a key part: escaped (EscapeName), so a branch such as `fix/1.2` keys.
+func groupKey(group string) (string, error) {
+	if group == "" {
+		return "", fmt.Errorf("delivery: the group name is empty")
+	}
+	return EscapeName(group), nil
+}
+
+// parseGroupKey is the inverse of groupKey.
+func parseGroupKey(part string) (string, error) {
+	if part == "" {
+		return "", fmt.Errorf("delivery: the group name is empty")
+	}
+	group, err := UnescapeName(part)
+	if err != nil {
 		return "", err
 	}
 	return group, nil
 }
 
-// ParseCandidatesKey is the inverse of CandidatesKey.
-func ParseCandidatesKey(key string) (group string, err error) {
-	if group, err = CandidatesKey(key); err != nil {
-		return "", fmt.Errorf("delivery: %q is not a candidates key: %w", key, err)
+// escapeRepository is `<owner>/<name>` as a key part, each part escaped; the repository must have exactly one slash
+// and neither part empty.
+func escapeRepository(repository string) (string, error) {
+	owner, name, ok := strings.Cut(repository, "/")
+	if !ok || owner == "" || name == "" || strings.Contains(name, "/") {
+		return "", fmt.Errorf("delivery: the repository %q is not <owner>/<name>, with exactly one slash", repository)
 	}
-	return group, nil
+	return EscapeName(owner) + "/" + EscapeName(name), nil
+}
+
+// unescapeRepository is the inverse of escapeRepository.
+func unescapeRepository(part string) (string, error) {
+	owner, name, ok := strings.Cut(part, "/")
+	if !ok || owner == "" || name == "" || strings.Contains(name, "/") {
+		return "", fmt.Errorf("delivery: the repository %q is not <owner>/<name>, with exactly one slash", part)
+	}
+	o, err := UnescapeName(owner)
+	if err != nil {
+		return "", err
+	}
+	n, err := UnescapeName(name)
+	if err != nil {
+		return "", err
+	}
+	return o + "/" + n, nil
 }
 
 // ModulesKey is the key, in ModulesBucket, of a module: `<module>`.

go/delivery/source_test.go

@@ -22,9 +22,6 @@ func TestSourceKeys(t *testing.T) { // 1200/AC1
 		t.Fatalf("parse modules = %q, %v", m, err)
 	}
 	for _, bad := range []string{"", "a.b", "a*b", "a>b", "a b", "a\tb"} {
-		if _, err := CandidatesKey(bad); err == nil {
-			t.Errorf("candidates key %q accepted", bad)
-		}
 		if _, err := ModulesKey(bad); err == nil {
 			t.Errorf("modules key %q accepted", bad)
 		}
@@ -35,6 +32,9 @@ func TestSourceKeys(t *testing.T) { // 1200/AC1
 			t.Errorf("parse modules %q accepted", bad)
 		}
 	}
+	if _, err := CandidatesKey(""); err == nil {
+		t.Error("an empty candidates key accepted")
+	}
 	ev, err := ForgeEvent("novox/mesh-sdk", ForgeMerged)
 	if err != nil || ev != "forge.novox.mesh-sdk.merged" {
 		t.Fatalf("forge event = %q, %v", ev, err)