amadan
Refuse a version component that does not fit an int, and more redirect rules than the edge accepts
strictVersion accepted any run of digits and the three Atoi errors were discarded, so on a 64-bit build 1.9223372036854775808.0 and 1.9223372036854775809.0 both read as the same saturated minor, and the first directory kept the /1/ rewrite. A component that does not fit an int is now not a version, and the directory is refused as not a release. The boundary test uses the machine's own math.MaxInt, so it holds on a 32-bit build as well. CARLOS's edge takes 2,000 rules and 256 KiB in a _redirects file (internal/edge/redirects.go), and Build wrote one rule per major with no limit, so a site past them assembled here and failed in the packer. Both limits are refused here first, naming the figure. 2,000 majors build and 2,001 do not.
Human
Prompt
The final review's Minors 4 and 5: overflowing version components breaking the newest-release rewrite, and a site builder that could emit more rules than the edge accepts.
From this commit's own message. The prompt ledger has no entries for it — which is what a squashed, rebased or amended commit looks like, and what every commit looks like in a repo without a ledger.
Summary
internal/site: parseVersion returns false on an Atoi error, so an overflowing component is not a release rather than a saturated number; Build refuses more than 2,000 _redirects rules or 256 KiB, CARLOS's limits, naming the figures. Boundary tests at math.MaxInt and one over, and at 2,000 and 2,001 majors.
Agent notes
The two refusals are new user-facing error strings, outside the copy batch Paul reviewed; they are listed in the final-fix report for the next review. The byte limit cannot be reached with 2,000 rules of versions that fit an int, but it is the edge's second limit and costs a line.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2 files changed,
+93
−3
internal/site/site.go+29 −3internal/site/site_test.go+64 −0
diff --git a/internal/site/site.go b/internal/site/site.go| index d5c7f5b..29889e3 100644 |
| --- a/internal/site/site.go |
| +++ b/internal/site/site.go |
| @@ -69,12 +69,32 @@ func parseVersion(name string) (version, bool) { |
| return version{}, false |
| } |
| v := version{name: name} |
| - v.major, _ = strconv.Atoi(m[1]) |
| - v.minor, _ = strconv.Atoi(m[2]) |
| - v.patch, _ = strconv.Atoi(m[3]) |
| + // A component that does not fit an int is not a version either: Atoi |
| + // saturates on overflow, so two such versions would read as one |
| + // number, and the rewrite could land on the older of them. |
| + var err error |
| + if v.major, err = strconv.Atoi(m[1]); err != nil { |
| + return version{}, false |
| + } |
| + if v.minor, err = strconv.Atoi(m[2]); err != nil { |
| + return version{}, false |
| + } |
| + if v.patch, err = strconv.Atoi(m[3]); err != nil { |
| + return version{}, false |
| + } |
| return v, true |
| } |
| +// The edge's limits on one _redirects file, from CARLOS's |
| +// internal/edge/redirects.go: 2,000 rules and 256 KiB (and 2,048 bytes |
| +// a line, which one rule per major cannot reach once every component |
| +// fits an int). A site past either assembles here and is refused by the |
| +// packer, so it is refused here first, naming the figure. |
| +const ( |
| + redirectsMaxRules = 2000 |
| + redirectsMaxBytes = 256 << 10 |
| +) |
| + |
| // less orders versions numerically, so 1.10.0 is newer than 1.9.3. |
| func (v version) less(w version) bool { |
| if v.major != w.major { |
| @@ -143,10 +163,16 @@ func Build(releases fs.FS, pages fs.FS) (map[string][]byte, error) { |
| majors = append(majors, m) |
| } |
| sort.Sort(sort.Reverse(sort.IntSlice(majors))) |
| + if len(majors) > redirectsMaxRules { |
| + return nil, fmt.Errorf("%s would hold %d rules, one per major, and the edge accepts %d", redirectsName, len(majors), redirectsMaxRules) |
| + } |
| var rules bytes.Buffer |
| for _, m := range majors { |
| fmt.Fprintf(&rules, "/%d/* /%s/:splat 200\n", m, newest[m].name) |
| } |
| + if rules.Len() > redirectsMaxBytes { |
| + return nil, fmt.Errorf("%s would be %d bytes, and the edge accepts %d", redirectsName, rules.Len(), redirectsMaxBytes) |
| + } |
| out[redirectsName] = rules.Bytes() |
| err = fs.WalkDir(pages, ".", func(p string, d fs.DirEntry, err error) error { |
diff --git a/internal/site/site_test.go b/internal/site/site_test.go| index 14352c8..07bf9f0 100644 |
| --- a/internal/site/site_test.go |
| +++ b/internal/site/site_test.go |
| @@ -5,9 +5,12 @@ import ( |
| "crypto/sha256" |
| "encoding/hex" |
| "io/fs" |
| + "math" |
| + "math/big" |
| "os" |
| "path/filepath" |
| "sort" |
| + "strconv" |
| "strings" |
| "testing" |
| "testing/fstest" |
| @@ -404,3 +407,64 @@ func TestALinkedSumsFileIsRefused(t *testing.T) { |
| t.Errorf("the error does not refuse SHA256SUMS as not a regular file: %v", err) |
| } |
| } |
| + |
| +// TestAVersionComponentMustFit: a component as large as an int is a |
| +// version, ordered by its number; one larger is not a release at all. |
| +// strconv.Atoi saturates on overflow, so before this two overflowing |
| +// minors read as the same number and /1/ stayed on whichever came |
| +// first. The boundary is the machine's int, so the test holds on a |
| +// 32-bit build as well. |
| +func TestAVersionComponentMustFit(t *testing.T) { |
| + most := strconv.Itoa(math.MaxInt) |
| + out, err := site.Build(releasesOf("1.2.0", "1."+most+".0"), pagesOf(nil)) |
| + if err != nil { |
| + t.Fatalf("Build with a minor of %s: %v", most, err) |
| + } |
| + if want := "/1/* /1." + most + ".0/:splat 200\n"; string(out["_redirects"]) != want { |
| + t.Errorf("_redirects is %q, want %q", out["_redirects"], want) |
| + } |
| + over := new(big.Int).Add(big.NewInt(math.MaxInt), big.NewInt(1)).String() |
| + for _, name := range []string{"1." + over + ".0", over + ".0.0", "1.0." + over} { |
| + _, err := site.Build(releasesOf("1.2.0", name), pagesOf(nil)) |
| + if err == nil { |
| + t.Errorf("Build accepted releases/%s, whose component does not fit an int", name) |
| + continue |
| + } |
| + if !strings.Contains(err.Error(), "releases/"+name+" is not a release") { |
| + t.Errorf("Build refused %s for the wrong reason: %v", name, err) |
| + } |
| + } |
| + // Two overflowing minors: refused, not read as one number. |
| + over2 := new(big.Int).Add(big.NewInt(math.MaxInt), big.NewInt(2)).String() |
| + if _, err := site.Build(releasesOf("1."+over+".0", "1."+over2+".0"), pagesOf(nil)); err == nil { |
| + t.Error("Build accepted two releases whose minors both overflow, which Atoi reads as one number") |
| + } |
| +} |
| + |
| +// TestTheRedirectsStayUnderTheEdgesLimits: one rule per major, and the |
| +// edge accepts 2,000 rules in a _redirects file (CARLOS's |
| +// redirectsMaxRules). 2,000 majors build; 2,001 are refused here, by |
| +// the figure, rather than assembling a site the packer rejects. |
| +func TestTheRedirectsStayUnderTheEdgesLimits(t *testing.T) { |
| + majors := func(n int) []string { |
| + names := make([]string, 0, n) |
| + for i := 1; i <= n; i++ { |
| + names = append(names, strconv.Itoa(i)+".0.0") |
| + } |
| + return names |
| + } |
| + out, err := site.Build(releasesOf(majors(2000)...), pagesOf(nil)) |
| + if err != nil { |
| + t.Fatalf("Build with 2000 majors: %v", err) |
| + } |
| + if n := strings.Count(string(out["_redirects"]), "\n"); n != 2000 { |
| + t.Errorf("_redirects holds %d rules, want 2000", n) |
| + } |
| + _, err = site.Build(releasesOf(majors(2001)...), pagesOf(nil)) |
| + if err == nil { |
| + t.Fatal("Build accepted 2001 majors: 2001 rules, past the edge's 2000") |
| + } |
| + if !strings.Contains(err.Error(), "2001 rules") || !strings.Contains(err.Error(), "accepts 2000") { |
| + t.Errorf("the refusal does not give both figures: %v", err) |
| + } |
| +} |