relmeta,internal/task: make version semantics explicit For std/cmd, we plumb through the canonical go1.m.n; downstreams that join golang.org/x/* and std/cmd need to handle the version semantics internally. Change-Id: I5e095251fcb103ea9d723831ebc37ee8d00973b7 Reviewed-on: https://go-review.googlesource.com/c/build/+/820468 Reviewed-by: Neal Patel <nealpatel@google.com> Reviewed-by: Nicholas Husin <husin@google.com> LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
diff --git a/internal/relui/buildrelease_test.go b/internal/relui/buildrelease_test.go index f86574a..b9ffa47 100644 --- a/internal/relui/buildrelease_test.go +++ b/internal/relui/buildrelease_test.go
@@ -2223,7 +2223,7 @@ Track: relmeta.Private, Package: "crypto/tls", Changelists: []string{"https://go-internal-review.git.corp.google.com/c/go/+/1234"}, - TargetReleases: []string{"1.25.1", "1.26.1"}, + TargetReleases: []string{"go1.25.1", "go1.26.1"}, ReleaseNote: "crypto/tls: bad handshake causes panic.\n\nA specially crafted ClientHello triggers a nil pointer dereference.", GitHubIssueID: 99999, VulnReportID: "GO-2026-9001", @@ -2235,7 +2235,7 @@ Track: relmeta.Private, Package: "cmd/go", Changelists: []string{"https://go-internal-review.git.corp.google.com/c/go/+/5678"}, - TargetReleases: []string{"1.26.1"}, + TargetReleases: []string{"go1.26.1"}, ReleaseNote: "cmd/go: module download executes arbitrary code.\n\nA crafted go.sum allows execution of untrusted binaries.", GitHubIssueID: 99998, VulnReportID: "GO-2026-9002",
diff --git a/internal/relui/workflows.go b/internal/relui/workflows.go index c6a477e..e670171 100644 --- a/internal/relui/workflows.go +++ b/internal/relui/workflows.go
@@ -629,7 +629,7 @@ } var reports []*report.Report for _, p := range rm.Patches { - mod, err := task.DeriveVulnModuleInfo(p) + mod, err := task.StdVulnModuleInfo(p) if err != nil { return "", err }
diff --git a/internal/task/privx.go b/internal/task/privx.go index a636c66..ce12fd9 100644 --- a/internal/task/privx.go +++ b/internal/task/privx.go
@@ -448,28 +448,26 @@ // // These will generate the cve5/osv files that // must be included in the diff below. - return MailVulnReports(ctx, x.PublicGerrit, reports, reviewers) } -// vulnModuleInfo derives the [VulnModuleInfo] for a single patch. For -// golang.org/x patches it validates that the package belongs to the -// tagged repo and uses the network-resolved vulnerableAt (the -// historical x-repo behavior). std/cmd patches do not flow through a -// TagRepo, so for them the module value and vulnerable_at are derived -// locally from the patch via [DeriveVulnModuleInfo]. +// vulnModuleInfo derives the [VulnModuleInfo] for a single patch. func (x *PrivXPatch) vulnModuleInfo(p *relmeta.SecurityPatch, tagged TagRepo, vulnerableAt *report.Version) (VulnModuleInfo, error) { - if strings.HasPrefix(p.Package, "golang.org/x/") { - repo, err := repoName(p.Package) - if err != nil { - return VulnModuleInfo{}, err - } - if got, want := repo, tagged.Name; got != want { - return VulnModuleInfo{}, fmt.Errorf("package mismatch: %q vs %q", got, want) - } - return VulnModuleInfo{Module: tagged.ModPath, VulnerableAt: vulnerableAt}, nil + repo, err := repoName(p.Package) + if err != nil { + return VulnModuleInfo{}, err } - return DeriveVulnModuleInfo(p) + if got, want := repo, tagged.Name; got != want { + return VulnModuleInfo{}, fmt.Errorf("package mismatch: %q vs %q", got, want) + } + if tagged.NewerVersion == "" { + return VulnModuleInfo{}, fmt.Errorf("repo %q was not tagged", tagged.Name) + } + return VulnModuleInfo{ + Module: tagged.ModPath, + Versions: report.Versions{report.Fixed(strings.TrimPrefix(tagged.NewerVersion, "v"))}, + VulnerableAt: vulnerableAt, + }, nil } func (x *PrivXPatch) UpdateGitHubIssues(ctx *wf.TaskContext, rm *relmeta.ReleaseMilestone) error {
diff --git a/internal/task/privx_test.go b/internal/task/privx_test.go index c263f33..b67411b 100644 --- a/internal/task/privx_test.go +++ b/internal/task/privx_test.go
@@ -142,8 +142,6 @@ Thanks to a very levitated gopher for reporting this issue. This is CVE-1970-0001 and Go issue https://go.dev/issue/4294967296. - target_releases: - - 1.1.0 cve: CVE-1970-0001 github_issue_id: 4294967296 vuln_report_id: GO-1970-0001 @@ -164,8 +162,6 @@ Thanks to a confused poet for reporting this issue. This is CVE-1970-0002 and Go issue https://go.dev/issue/4294967297. - target_releases: - - 1.1.0 cve: CVE-1970-0002 github_issue_id: 4294967297 vuln_report_id: GO-1970-0002 @@ -187,8 +183,6 @@ Thanks to a very levitated gopher for reporting this issue. This is CVE-1970-0003 and Go issue https://go.dev/issue/4294967298. - target_releases: - - 1.1.0 cve: CVE-1970-0003 github_issue_id: 4294967298 vuln_report_id: GO-1970-0003 @@ -511,6 +505,12 @@ if mod.Packages[0].Package != p.Package { t.Errorf("patch %d: package = %q, want %q", p.ID, mod.Packages[0].Package, p.Package) } + if want := (report.Versions{report.Fixed("1.1.0")}); !reflect.DeepEqual(mod.Versions, want) { + t.Errorf("patch %d: versions = %v, want %v", p.ID, mod.Versions, want) + } + if mod.VulnerableAt == nil || mod.VulnerableAt.Version != "1.0.0" { + t.Errorf("patch %d: vulnerable_at = %v, want 1.0.0", p.ID, mod.VulnerableAt) + } // CVE metadata. if vr.CVEMetadata == nil {
diff --git a/internal/task/vulndb.go b/internal/task/vulndb.go index cf8664a..cf69403 100644 --- a/internal/task/vulndb.go +++ b/internal/task/vulndb.go
@@ -51,9 +51,8 @@ URL: announceURL, }) - versions, err := VulnReportVersions(p.TargetReleases) - if err != nil { - return nil, err + if len(mod.Versions) == 0 { + return nil, errors.New("missing versions") } // Derive the vuln report's summary and description @@ -90,7 +89,7 @@ Modules: []*report.Module{ { Module: mod.Module, - Versions: versions, + Versions: mod.Versions, VulnerableAt: mod.VulnerableAt, // TODO(nealpatel): for completeness, we should // support the edge case of vendored x/ repo fix @@ -114,26 +113,24 @@ type VulnModuleInfo struct { Module string + Versions report.Versions VulnerableAt *report.Version } -// VulnReportVersions constructs the expected -// list of [report.Versions] that flags which -// major.minor.patch semvers are affected. -// -// targetReleases are expected to conform to -// the [report.Version] semantics which notably -// do not include the prefixed 'v'. -func VulnReportVersions(targetReleases []string) (report.Versions, error) { +// stdVulnReportVersions converts the go1.m.n version string into a format +// that [report.Version] expects and returns a sorted [report.Versions] +// from oldest to newest, adding the necessary introduced versions. +func stdVulnReportVersions(targetReleases []string) (report.Versions, error) { if len(targetReleases) == 0 { return nil, errors.New("missing target releases") } semVers := make([]string, len(targetReleases)) for i := range targetReleases { - semVers[i] = "v" + targetReleases[i] - if !semver.IsValid(semVers[i]) { - return nil, fmt.Errorf("invalid semver %q (target: %q)", semVers[i], targetReleases[i]) + bare, err := goTagToBareSemver(targetReleases[i]) + if err != nil { + return nil, err } + semVers[i] = "v" + bare } semver.Sort(semVers) var versions report.Versions @@ -146,27 +143,41 @@ // standard convention in golang/vulndb. if i > 0 { mm := semver.MajorMinor(v) + ".0-0" - versions = append(versions, report.Introduced(mm)) + versions = append(versions, report.Introduced(mm[1:])) } - versions = append(versions, report.Fixed(v)) + versions = append(versions, report.Fixed(v[1:])) } return versions, nil } -// DeriveVulnModuleInfo derives the [VulnModuleInfo] for a std/cmd -// security patch without a network call. The module value is taken -// from [VulnModule] and the VulnerableAt version is derived from -// its [VulnerableAtFromTargetReleases]. -// -// The x-repo path uses [ResolveVulnerableVersion] instead, which -// resolves over the network; see [PrivXPatch.CreateVulnReports]. -func DeriveVulnModuleInfo(p *relmeta.SecurityPatch) (VulnModuleInfo, error) { - vulnerableAt, err := VulnerableAtFromTargetReleases(p.TargetReleases) +func goTagToBareSemver(goTag string) (string, error) { + bare, ok := strings.CutPrefix(goTag, "go") + if !ok || bare == "" { + return "", fmt.Errorf("target release %q is not a go1.X.Y tag", goTag) + } + if strings.Count(bare, ".") != 2 { + return "", fmt.Errorf("target release %q must have three components (go1.X.Y)", goTag) + } + v := "v" + bare + if !semver.IsValid(v) { + return "", fmt.Errorf("invalid semver %q (target: %q)", v, goTag) + } + return bare, nil +} + +// StdVulnModuleInfo derives the [VulnModuleInfo] for a std/cmd security patch. +func StdVulnModuleInfo(p *relmeta.SecurityPatch) (VulnModuleInfo, error) { + versions, err := stdVulnReportVersions(p.TargetReleases) + if err != nil { + return VulnModuleInfo{}, err + } + vulnerableAt, err := stdVulnerableAt(p.TargetReleases) if err != nil { return VulnModuleInfo{}, err } return VulnModuleInfo{ Module: VulnModule(p.Package), + Versions: versions, VulnerableAt: vulnerableAt, }, nil } @@ -183,16 +194,17 @@ return "std" } -func VulnerableAtFromTargetReleases(targetReleases []string) (*report.Version, error) { +func stdVulnerableAt(targetReleases []string) (*report.Version, error) { if len(targetReleases) == 0 { return nil, errors.New("missing target releases") } semVers := make([]string, len(targetReleases)) for i := range targetReleases { - semVers[i] = "v" + targetReleases[i] - if !semver.IsValid(semVers[i]) { - return nil, fmt.Errorf("invalid semver %q (target: %q)", semVers[i], targetReleases[i]) + bare, err := goTagToBareSemver(targetReleases[i]) + if err != nil { + return nil, err } + semVers[i] = "v" + bare } semver.Sort(semVers) highest := semVers[len(semVers)-1] // e.g. "v1.26.3"
diff --git a/internal/task/vulndb_test.go b/internal/task/vulndb_test.go index 27f90aa..2fbbfb1 100644 --- a/internal/task/vulndb_test.go +++ b/internal/task/vulndb_test.go
@@ -61,7 +61,7 @@ } } -func TestVulnReportVersions(t *testing.T) { +func TestStdVulnReportVersions(t *testing.T) { tests := []struct { name string targets []string @@ -70,16 +70,16 @@ }{ { name: "single", - targets: []string{"1.1.0"}, - want: report.Versions{report.Fixed("v1.1.0")}, + targets: []string{"go1.1.0"}, + want: report.Versions{report.Fixed("1.1.0")}, }, { name: "two versions", - targets: []string{"1.24.1", "1.23.5"}, + targets: []string{"go1.24.1", "go1.23.5"}, want: report.Versions{ - report.Fixed("v1.23.5"), - report.Introduced("v1.24.0-0"), - report.Fixed("v1.24.1"), + report.Fixed("1.23.5"), + report.Introduced("1.24.0-0"), + report.Fixed("1.24.1"), }, }, { @@ -92,38 +92,52 @@ targets: []string{"not-a-version"}, wantErr: true, }, + { + name: "bare semver rejected", + targets: []string{"1.24.1"}, + wantErr: true, + }, + { + name: "two component rejected", + targets: []string{"go1.26"}, + wantErr: true, + }, + { + name: "one component rejected", + targets: []string{"go1"}, + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := VulnReportVersions(tt.targets) + got, err := stdVulnReportVersions(tt.targets) if (err != nil) != tt.wantErr { - t.Fatalf("VulnReportVersions(%v): err = %v, wantErr = %v", tt.targets, err, tt.wantErr) + t.Fatalf("stdVulnReportVersions(%v): err = %v, wantErr = %v", tt.targets, err, tt.wantErr) } if err != nil { return } if !reflect.DeepEqual(got, tt.want) { - t.Errorf("VulnReportVersions(%v):\ngot %v\nwant %v", tt.targets, got, tt.want) + t.Errorf("stdVulnReportVersions(%v):\ngot %v\nwant %v", tt.targets, got, tt.want) } }) } } func TestVulnReport(t *testing.T) { - mod := VulnModuleInfo{Module: "golang.org/x/net", VulnerableAt: report.VulnerableAt("1.0.0")} + mod := VulnModuleInfo{Module: "golang.org/x/net", Versions: report.Versions{report.Fixed("1.1.0")}, VulnerableAt: report.VulnerableAt("1.0.0")} const announceURL = "https://groups.google.com/g/golang-announce/c/test" t.Run("valid", func(t *testing.T) { p := &relmeta.SecurityPatch{ - Package: "golang.org/x/net/http2", - GitHubIssueID: 12345, - Changelists: []string{"https://go.dev/cl/111"}, - TargetReleases: []string{"1.1.0"}, - ReleaseNote: "net/http2: bad things happen.\n\nDetails about the bad things.", - CVE: "CVE-2026-0001", - CWE: "CWE-400", - Credits: []string{"Alice"}, - VulnReportID: "GO-2026-0001", + Package: "golang.org/x/net/http2", + GitHubIssueID: 12345, + Changelists: []string{"https://go.dev/cl/111"}, + ReleaseNote: "net/http2: bad things happen.\n\nDetails about the bad things.", + CVE: "CVE-2026-0001", + CWE: "CWE-400", + Credits: []string{"Alice"}, + VulnReportID: "GO-2026-0001", } r, err := VulnReport(p, mod, announceURL) if err != nil { @@ -157,17 +171,16 @@ t.Run("dotted identifier preserves interior periods", func(t *testing.T) { p := &relmeta.SecurityPatch{ - Package: "net/http", - GitHubIssueID: 12345, - Changelists: []string{"https://go.dev/cl/111"}, - TargetReleases: []string{"1.1.0"}, - ReleaseNote: "net/http: TLS 1.3 handshake panics.\n\nDetails about the panic.", - CVE: "CVE-2026-0002", - CWE: "CWE-400", - Credits: []string{"Bob"}, - VulnReportID: "GO-2026-0002", + Package: "net/http", + GitHubIssueID: 12345, + Changelists: []string{"https://go.dev/cl/111"}, + ReleaseNote: "net/http: TLS 1.3 handshake panics.\n\nDetails about the panic.", + CVE: "CVE-2026-0002", + CWE: "CWE-400", + Credits: []string{"Bob"}, + VulnReportID: "GO-2026-0002", } - stdMod := VulnModuleInfo{Module: "std", VulnerableAt: report.VulnerableAt("1.0.0")} + stdMod := VulnModuleInfo{Module: "std", Versions: report.Versions{report.Fixed("1.1.0")}, VulnerableAt: report.VulnerableAt("1.0.0")} r, err := VulnReport(p, stdMod, announceURL) if err != nil { t.Fatal(err) @@ -182,7 +195,6 @@ Package: "golang.org/x/net/http2", GitHubIssueID: 12345, Changelists: []string{"https://go.dev/cl/111"}, - TargetReleases: []string{"1.1.0"}, ReleaseNote: "net/http2: bad things happen.\n\nOriginal description.", VulnReportDesc: "Overridden description.", VulnReportID: "GO-2026-0001", @@ -197,12 +209,23 @@ } }) + t.Run("missing versions", func(t *testing.T) { + p := &relmeta.SecurityPatch{ + Package: "golang.org/x/net/http2", + GitHubIssueID: 12345, + Changelists: []string{"https://go.dev/cl/111"}, + ReleaseNote: "net/http2: bad.\n\nDetails.", + } + if _, err := VulnReport(p, VulnModuleInfo{Module: "golang.org/x/net"}, announceURL); err == nil { + t.Fatal("expected error for missing versions") + } + }) + t.Run("missing github issue", func(t *testing.T) { p := &relmeta.SecurityPatch{ - Package: "golang.org/x/net/http2", - Changelists: []string{"https://go.dev/cl/111"}, - TargetReleases: []string{"1.1.0"}, - ReleaseNote: "net/http2: bad.\n\nDetails.", + Package: "golang.org/x/net/http2", + Changelists: []string{"https://go.dev/cl/111"}, + ReleaseNote: "net/http2: bad.\n\nDetails.", } if _, err := VulnReport(p, mod, announceURL); err == nil { t.Fatal("expected error for missing github issue") @@ -211,10 +234,9 @@ t.Run("missing changelists", func(t *testing.T) { p := &relmeta.SecurityPatch{ - Package: "golang.org/x/net/http2", - GitHubIssueID: 12345, - TargetReleases: []string{"1.1.0"}, - ReleaseNote: "net/http2: bad.\n\nDetails.", + Package: "golang.org/x/net/http2", + GitHubIssueID: 12345, + ReleaseNote: "net/http2: bad.\n\nDetails.", } if _, err := VulnReport(p, mod, announceURL); err == nil { t.Fatal("expected error for missing changelists") @@ -223,11 +245,10 @@ t.Run("missing announce URL", func(t *testing.T) { p := &relmeta.SecurityPatch{ - Package: "golang.org/x/net/http2", - GitHubIssueID: 12345, - Changelists: []string{"https://go.dev/cl/111"}, - TargetReleases: []string{"1.1.0"}, - ReleaseNote: "net/http2: bad.\n\nDetails.", + Package: "golang.org/x/net/http2", + GitHubIssueID: 12345, + Changelists: []string{"https://go.dev/cl/111"}, + ReleaseNote: "net/http2: bad.\n\nDetails.", } if _, err := VulnReport(p, mod, ""); err == nil { t.Fatal("expected error for missing announce URL") @@ -236,11 +257,10 @@ t.Run("malformed release note no newline", func(t *testing.T) { p := &relmeta.SecurityPatch{ - Package: "golang.org/x/net/http2", - GitHubIssueID: 12345, - Changelists: []string{"https://go.dev/cl/111"}, - TargetReleases: []string{"1.1.0"}, - ReleaseNote: "no newline here", + Package: "golang.org/x/net/http2", + GitHubIssueID: 12345, + Changelists: []string{"https://go.dev/cl/111"}, + ReleaseNote: "no newline here", } if _, err := VulnReport(p, mod, announceURL); err == nil { t.Fatal("expected error for malformed release note") @@ -249,11 +269,10 @@ t.Run("malformed release note no colon", func(t *testing.T) { p := &relmeta.SecurityPatch{ - Package: "golang.org/x/net/http2", - GitHubIssueID: 12345, - Changelists: []string{"https://go.dev/cl/111"}, - TargetReleases: []string{"1.1.0"}, - ReleaseNote: "no colon in subject\n\nDetails.", + Package: "golang.org/x/net/http2", + GitHubIssueID: 12345, + Changelists: []string{"https://go.dev/cl/111"}, + ReleaseNote: "no colon in subject\n\nDetails.", } if _, err := VulnReport(p, mod, announceURL); err == nil { t.Fatal("expected error for malformed subject") @@ -262,11 +281,10 @@ t.Run("non-ascii summary", func(t *testing.T) { p := &relmeta.SecurityPatch{ - Package: "golang.org/x/net/http2", - GitHubIssueID: 12345, - Changelists: []string{"https://go.dev/cl/111"}, - TargetReleases: []string{"1.1.0"}, - ReleaseNote: "net/http2: 日本語\n\nDetails.", + Package: "golang.org/x/net/http2", + GitHubIssueID: 12345, + Changelists: []string{"https://go.dev/cl/111"}, + ReleaseNote: "net/http2: 日本語\n\nDetails.", } if _, err := VulnReport(p, mod, announceURL); err == nil { t.Fatal("expected error for non-ascii summary") @@ -298,7 +316,7 @@ } } -func TestVulnerableAtFromTargetReleases(t *testing.T) { +func TestStdVulnerableAt(t *testing.T) { tests := []struct { name string targets []string @@ -307,17 +325,17 @@ }{ { name: "two release lines", - targets: []string{"1.25.10", "1.26.3"}, + targets: []string{"go1.25.10", "go1.26.3"}, wantVer: "1.26.2", }, { name: "single release", - targets: []string{"1.26.3"}, + targets: []string{"go1.26.3"}, wantVer: "1.26.2", }, { name: "result patch zero", - targets: []string{"1.24.1", "1.25.1"}, + targets: []string{"go1.24.1", "go1.25.1"}, wantVer: "1.25.0", }, { @@ -327,7 +345,7 @@ }, { name: "patch zero", - targets: []string{"1.26.0"}, + targets: []string{"go1.26.0"}, wantErr: true, }, { @@ -337,13 +355,23 @@ }, { name: "two component version", - targets: []string{"1.26"}, + targets: []string{"go1.26"}, + wantErr: true, + }, + { + name: "bare semver rejected", + targets: []string{"1.26.3"}, + wantErr: true, + }, + { + name: "one component rejected", + targets: []string{"go1"}, wantErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := VulnerableAtFromTargetReleases(tt.targets) + got, err := stdVulnerableAt(tt.targets) if (err != nil) != tt.wantErr { t.Fatalf("got %v, want %v", err, tt.wantErr) } @@ -357,10 +385,10 @@ } } -func TestVulnerableAtFromTargetReleasesPrerelease(t *testing.T) { +func TestStdVulnerableAtPrerelease(t *testing.T) { // A pre-release suffix like "1.26.3-rc1" passes semver.IsValid // but makes the patch extraction fail (strconv.Atoi on "3-rc1"). - _, err := VulnerableAtFromTargetReleases([]string{"1.26.3-rc1"}) + _, err := stdVulnerableAt([]string{"go1.26.3-rc1"}) if err == nil { t.Fatal("expected error for pre-release target, got nil") } @@ -369,12 +397,12 @@ } } -func TestDeriveVulnModuleInfo(t *testing.T) { +func TestStdVulnModuleInfo(t *testing.T) { p := &relmeta.SecurityPatch{ Package: "net/http", - TargetReleases: []string{"1.25.10", "1.26.3"}, + TargetReleases: []string{"go1.25.10", "go1.26.3"}, } - mod, err := DeriveVulnModuleInfo(p) + mod, err := StdVulnModuleInfo(p) if err != nil { t.Fatal(err) } @@ -384,6 +412,10 @@ if mod.VulnerableAt == nil || mod.VulnerableAt.Version != "1.26.2" { t.Errorf("got %v, want 1.26.2", mod.VulnerableAt) } + want := report.Versions{report.Fixed("1.25.10"), report.Introduced("1.26.0-0"), report.Fixed("1.26.3")} + if !reflect.DeepEqual(mod.Versions, want) { + t.Errorf("got %v, want %v", mod.Versions, want) + } } type fakeVulnGerrit struct {
diff --git a/relmeta/relmeta.go b/relmeta/relmeta.go index e66c54e..3310359 100644 --- a/relmeta/relmeta.go +++ b/relmeta/relmeta.go
@@ -31,7 +31,9 @@ Package string `yaml:"package"` Changelists []string `yaml:"changelists"` ReleaseNote string `yaml:"release_note"` - // TODO(nealpatel): do not omitempty; this is required. + // TODO(nealpatel): do not omitempty; this is required + // only for std/cmd (not golang.org/x/) which means it + // needs to re-scoped for clarity. TargetReleases []string `yaml:"target_releases,omitempty"` GitHubIssueID int64 `yaml:"github_issue_id"` VulnReportID string `yaml:"vuln_report_id"` // for example, GO-20YY-NNNN