From 68546badb3b29cd9d064a09b6445e70c238f7172 Mon Sep 17 00:00:00 2001 From: claude Date: Mon, 7 Sep 2026 14:19:30 +0100 Subject: [PATCH] reject non-3xx status codes; close the CSV file; add tests A row with a status code outside 3xx (e.g. "0" from a typo) was accepted at load and then panicked net/http's WriteHeader on the first matching request. ReadCsvRedirects now rejects anything that isn't 300-399, with a line number in the message, and reports a non-numeric code clearly. Also defer reader.Close() (the file was left open for the process lifetime) and drop the unreachable "row == nil" check. Adds internal/{csv,redirect,http}_test.go covering CSV parsing (valid, reordered headings, header-only, missing heading, non-numeric and non-3xx status, ragged row, missing file), Redirects.Find / Redirect.Match, and ServeHTTP (redirect, 404, query string dropped). Co-Authored-By: Claude Sonnet 5 --- internal/csv.go | 18 +++---- internal/csv_test.go | 102 ++++++++++++++++++++++++++++++++++++++ internal/http_test.go | 56 +++++++++++++++++++++ internal/redirect_test.go | 31 ++++++++++++ 4 files changed, 198 insertions(+), 9 deletions(-) create mode 100644 internal/csv_test.go create mode 100644 internal/http_test.go create mode 100644 internal/redirect_test.go diff --git a/internal/csv.go b/internal/csv.go index d2b5877..68fec58 100644 --- a/internal/csv.go +++ b/internal/csv.go @@ -3,6 +3,7 @@ package internal import ( "encoding/csv" "errors" + "fmt" "io" "os" "strconv" @@ -25,6 +26,7 @@ func ReadCsvRedirects(file string) (Redirects, error) { if err != nil { return nil, err } + defer reader.Close() csvReader := csv.NewReader(reader) @@ -44,7 +46,7 @@ func ReadCsvRedirects(file string) (Redirects, error) { // Read data redirects := Redirects{} - for { + for line := 2; ; line++ { row, err := csvReader.Read() if err != nil { if errors.Is(err, io.EOF) { @@ -52,22 +54,20 @@ func ReadCsvRedirects(file string) (Redirects, error) { } return nil, err } - if row == nil { - break - } statusCode, err := strconv.Atoi(row[statusCol]) if err != nil { - return nil, err + return nil, fmt.Errorf("line %d: status code %q is not a number", line, row[statusCol]) + } + if statusCode < 300 || statusCode > 399 { + return nil, fmt.Errorf("line %d: status code %d is not a redirect (must be 3xx)", line, statusCode) } - redirect := &Redirect{ + redirects = append(redirects, &Redirect{ From: row[fromCol], To: row[toCol], StatusCode: statusCode, - } - - redirects = append(redirects, redirect) + }) } return redirects, nil diff --git a/internal/csv_test.go b/internal/csv_test.go new file mode 100644 index 0000000..92b46fb --- /dev/null +++ b/internal/csv_test.go @@ -0,0 +1,102 @@ +package internal + +import ( + "os" + "path/filepath" + "testing" +) + +func writeCSV(t *testing.T, content string) string { + t.Helper() + p := filepath.Join(t.TempDir(), "redirects.csv") + if err := os.WriteFile(p, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + return p +} + +func TestReadCsvRedirects(t *testing.T) { + t.Run("valid file", func(t *testing.T) { + p := writeCSV(t, "From,To,Status Code\n/a,https://example.com/a,301\n/b,https://example.com/b,302\n") + + got, err := ReadCsvRedirects(p) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(got) != 2 { + t.Fatalf("got %d redirects, want 2", len(got)) + } + if got[0].From != "/a" || got[0].To != "https://example.com/a" || got[0].StatusCode != 301 { + t.Errorf("row 0 = %+v", got[0]) + } + }) + + t.Run("headings in any order", func(t *testing.T) { + p := writeCSV(t, "Status Code,To,From\n301,https://example.com,/x\n") + + got, err := ReadCsvRedirects(p) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got[0].From != "/x" || got[0].To != "https://example.com" || got[0].StatusCode != 301 { + t.Errorf("got %+v", got[0]) + } + }) + + t.Run("header only yields no redirects", func(t *testing.T) { + got, err := ReadCsvRedirects(writeCSV(t, "From,To,Status Code\n")) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(got) != 0 { + t.Fatalf("got %d redirects, want 0", len(got)) + } + }) + + t.Run("missing heading is an error", func(t *testing.T) { + if _, err := ReadCsvRedirects(writeCSV(t, "From,To\n/a,https://example.com\n")); err == nil { + t.Fatal("want error for missing Status Code heading") + } + }) + + t.Run("non-numeric status is an error", func(t *testing.T) { + if _, err := ReadCsvRedirects(writeCSV(t, "From,To,Status Code\n/a,https://example.com,oops\n")); err == nil { + t.Fatal("want error for non-numeric status code") + } + }) + + t.Run("non-3xx status is an error", func(t *testing.T) { + for _, code := range []string{"0", "-1", "200", "404", "500", "1000"} { + _, err := ReadCsvRedirects(writeCSV(t, "From,To,Status Code\n/a,https://example.com,"+code+"\n")) + if err == nil { + t.Errorf("status %s: want error, got nil", code) + } + } + }) + + t.Run("ragged row is an error", func(t *testing.T) { + if _, err := ReadCsvRedirects(writeCSV(t, "From,To,Status Code\n/a,https://example.com\n")); err == nil { + t.Fatal("want error for a row with too few fields") + } + }) + + t.Run("missing file is an error", func(t *testing.T) { + if _, err := ReadCsvRedirects(filepath.Join(t.TempDir(), "nope.csv")); err == nil { + t.Fatal("want error for a file that does not exist") + } + }) +} + +func TestHeadingsIndexOf(t *testing.T) { + h := Headings{"From", "To", "Status Code"} + + if got := h.IndexOf("To"); got != 1 { + t.Errorf("IndexOf(To) = %d, want 1", got) + } + if got := h.IndexOf("Status Code"); got != 2 { + t.Errorf("IndexOf(Status Code) = %d, want 2", got) + } + if got := h.IndexOf("Nope"); got != -1 { + t.Errorf("IndexOf(Nope) = %d, want -1", got) + } +} diff --git a/internal/http_test.go b/internal/http_test.go new file mode 100644 index 0000000..42210f2 --- /dev/null +++ b/internal/http_test.go @@ -0,0 +1,56 @@ +package internal + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/rs/zerolog" +) + +func newTestServer(rs Redirects) *HttpServer { + return &HttpServer{ + CLI: &CLI{}, + Log: zerolog.Nop(), + Redirects: rs, + } +} + +func TestServeHTTP(t *testing.T) { + srv := newTestServer(Redirects{ + {From: "/old", To: "https://example.com/new", StatusCode: 301}, + }) + + t.Run("known path redirects with Location", func(t *testing.T) { + rec := httptest.NewRecorder() + srv.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/old", nil)) + + if rec.Code != 301 { + t.Errorf("status = %d, want 301", rec.Code) + } + if loc := rec.Header().Get("Location"); loc != "https://example.com/new" { + t.Errorf("Location = %q", loc) + } + }) + + t.Run("unknown path is 404 with no Location", func(t *testing.T) { + rec := httptest.NewRecorder() + srv.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/nope", nil)) + + if rec.Code != http.StatusNotFound { + t.Errorf("status = %d, want 404", rec.Code) + } + if loc := rec.Header().Get("Location"); loc != "" { + t.Errorf("Location = %q, want empty", loc) + } + }) + + t.Run("query string is dropped (current behaviour)", func(t *testing.T) { + rec := httptest.NewRecorder() + srv.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/old?ref=x", nil)) + + if loc := rec.Header().Get("Location"); loc != "https://example.com/new" { + t.Errorf("Location = %q", loc) + } + }) +} diff --git a/internal/redirect_test.go b/internal/redirect_test.go new file mode 100644 index 0000000..0568139 --- /dev/null +++ b/internal/redirect_test.go @@ -0,0 +1,31 @@ +package internal + +import "testing" + +func TestRedirectsFind(t *testing.T) { + rs := Redirects{ + {From: "/a", To: "https://example.com/a", StatusCode: 301}, + {From: "/b", To: "https://example.com/b", StatusCode: 302}, + } + + if got := rs.Find("/b"); got == nil || got.To != "https://example.com/b" { + t.Errorf("Find(/b) = %+v", got) + } + if got := rs.Find("/missing"); got != nil { + t.Errorf("Find(/missing) = %+v, want nil", got) + } + if got := rs.Find("/A"); got != nil { + t.Errorf("Find is case-sensitive; Find(/A) = %+v, want nil", got) + } +} + +func TestRedirectMatch(t *testing.T) { + r := &Redirect{From: "/x"} + + if !r.Match("/x") { + t.Error("Match(/x) = false, want true") + } + if r.Match("/x/") { + t.Error("Match(/x/) = true, want false (exact match only)") + } +}