From a6dd698114821e13b5426ece8e66e01e7c9561fb Mon Sep 17 00:00:00 2001 From: mesh-tools Date: Mon, 19 Oct 2026 21:31:09 +0000 Subject: [PATCH] Escape group and repository names in every delivery key so a dotted branch keys (issue 1323) --- go/delivery/delivery.go | 44 +++++++++++++++++++ go/delivery/gate.go | 32 +++++++------- go/delivery/gate_test.go | 86 ++++++++++++++++++++++++++++++++++++-- go/delivery/source.go | 56 ++++++++++++++++++++++--- go/delivery/source_test.go | 6 +-- 5 files changed, 198 insertions(+), 26 deletions(-) diff --git a/go/delivery/delivery.go b/go/delivery/delivery.go index fe99528..ff6625e 100644 --- a/go/delivery/delivery.go +++ b/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: `.`. A module named `node` is // refused, since `.node` is the node's own facts. func HaveKey(node, module string) (string, error) { diff --git a/go/delivery/gate.go b/go/delivery/gate.go index cd85b3f..fe9cf56 100644 --- a/go/delivery/gate.go +++ b/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: ``. 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..`. 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..", 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 // `...`, the act being `status=` (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=` 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") diff --git a/go/delivery/gate_test.go b/go/delivery/gate_test.go index d8306a1..437d27b 100644 --- a/go/delivery/gate_test.go +++ b/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) + } +} diff --git a/go/delivery/source.go b/go/delivery/source.go index 0ec2568..25bbe5e 100644 --- a/go/delivery/source.go +++ b/go/delivery/source.go @@ -67,18 +67,62 @@ const SourcePrefix = "src1:" // CandidatesKey is the key, in CandidatesBucket, of a 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 `/` 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 /, 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 /, 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: ``. diff --git a/go/delivery/source_test.go b/go/delivery/source_test.go index 8969545..c269277 100644 --- a/go/delivery/source_test.go +++ b/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)