From 98dd09847ae59d699418a57b0e62e81074a22eb8 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Sat, 19 Sep 2026 21:08:04 +0200 Subject: [PATCH] test(06-05): close framework coverage gaps in bouncer, surf, wire, fetchguard - Registry neither-interface, nil-registry, and authenticate default - MemoryStore sweep actually drops expired entries - RegisterHouseMiddlewareFactory duplicate-name failure - pathScopedCORS unmatched path plus empty-raw-group introspection - Time UnmarshalJSON +00:00/Z and PublicOnlyMode host/IP cases --- bouncer/registry_coverage_test.go | 85 +++++++++++++++++++++++++++ fetchguard/fetch_coverage_test.go | 98 +++++++++++++++++++++++++++++++ surf/cors_coverage_test.go | 92 +++++++++++++++++++++++++++++ surf/limiter_coverage_test.go | 90 ++++++++++++++++++++++++++++ surf/routetable_coverage_test.go | 83 ++++++++++++++++++++++++++ wire/response_coverage_test.go | 72 +++++++++++++++++++++++ 6 files changed, 520 insertions(+) create mode 100644 bouncer/registry_coverage_test.go create mode 100644 fetchguard/fetch_coverage_test.go create mode 100644 surf/cors_coverage_test.go create mode 100644 surf/limiter_coverage_test.go create mode 100644 surf/routetable_coverage_test.go create mode 100644 wire/response_coverage_test.go diff --git a/bouncer/registry_coverage_test.go b/bouncer/registry_coverage_test.go new file mode 100644 index 0000000..431f382 --- /dev/null +++ b/bouncer/registry_coverage_test.go @@ -0,0 +1,85 @@ +package bouncer + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// Gap (a): Registry.Register of a type implementing neither Guard nor +// CredentialGuard is already asserted by TestRegisterNeitherInterfaceNamesPluginAndName. +// This file covers the remaining Register fail-loud branches and the +// authenticate() default that Register itself makes unreachable through +// the public API. + +func TestRegisterNilRegistryAndEmptyGuard(t *testing.T) { + var nilReg *Registry + if err := nilReg.Register("golem15.demo", "jwt", writerGuard{}); err == nil || !strings.Contains(err.Error(), "nil") { + t.Fatalf("nil registry: %v", err) + } + reg := NewRegistry() + if err := reg.Register("golem15.demo", "", writerGuard{}); err == nil || !strings.Contains(err.Error(), "golem15.demo") { + t.Fatalf("empty name: %v", err) + } + if err := reg.Register("golem15.demo", "jwt", nil); err == nil || !strings.Contains(err.Error(), "jwt") { + t.Fatalf("nil guard: %v", err) + } +} + +func TestAuthenticateDefaultNeitherInterface(t *testing.T) { + // Register rejects this type; authenticate's default is only reachable + // by calling it directly (same-package coverage of the fail-closed branch). + p, cred, err := authenticate(notAGuard{}, httptest.NewRequest(http.MethodGet, "/", nil)) + if p != nil || cred != nil { + t.Fatalf("principal=%v cred=%v", p, cred) + } + if err == nil || !strings.Contains(err.Error(), "neither Guard nor CredentialGuard") { + t.Fatalf("want neither-interface error, got %v", err) + } +} + +func TestNilRegistryMiddleware(t *testing.T) { + var nilReg *Registry + if _, err := nilReg.Middleware("jwt"); err == nil || !strings.Contains(err.Error(), "nil") { + t.Fatalf("nil registry Middleware: %v", err) + } +} + +func TestGuardAuthenticateSuccessAttachesUser(t *testing.T) { + reg := NewRegistry() + principal := &Principal{ID: 7} + if err := reg.Register("golem15.user", "jwt", writerGuard{principal: principal}); err != nil { + t.Fatal(err) + } + mw, err := reg.Middleware("jwt") + if err != nil { + t.Fatal(err) + } + h := mw(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + got, ok := User(r.Context()) + if !ok || got != principal { + t.Fatalf("user = %+v ok=%t", got, ok) + } + if _, ok := Credential(r.Context()); ok { + t.Fatal("plain Guard must not attach a credential") + } + w.WriteHeader(http.StatusNoContent) + })) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/", nil)) + if rec.Code != http.StatusNoContent { + t.Fatalf("status = %d", rec.Code) + } +} + +func TestWithUserNilContext(t *testing.T) { + p := &Principal{ID: 1} + got, ok := User(WithUser(nil, p)) + if !ok || got != p { + t.Fatalf("got %+v ok=%t", got, ok) + } + if _, ok := User(nil); ok { + t.Fatal("nil context must have no user") + } +} diff --git a/fetchguard/fetch_coverage_test.go b/fetchguard/fetch_coverage_test.go new file mode 100644 index 0000000..bc23eaf --- /dev/null +++ b/fetchguard/fetch_coverage_test.go @@ -0,0 +1,98 @@ +package fetchguard + +import ( + "errors" + "io" + "net" + "net/http" + "net/http/httptest" + "testing" + "time" +) + +// Gap (f): PublicOnlyMode private-IP rejection is already asserted by +// TestFetchPrivateIPBlockedInBothModes/PublicOnlyMode. This file adds +// PublicOnlyMode accepting any host when the dial-time IP check is +// satisfied (loopback httptest with skipReservedCheck — a live public +// IP dial would require outbound network and is not asserted here). + +func TestFetchPublicOnlyModeAcceptsAnyHostWhenPublic(t *testing.T) { + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/plain") + io.WriteString(w, "ok") + })) + t.Cleanup(srv.Close) + + res, err := Fetch(t.Context(), srv.URL, withTestLoopback(srv, Policy{ + Mode: PublicOnlyMode, + MaxBytes: 1024, + Timeout: 2 * time.Second, + }), nil) + if err != nil { + t.Fatalf("PublicOnlyMode must accept the httptest host: %v", err) + } + if string(res.Body) != "ok" { + t.Fatalf("body = %q", res.Body) + } +} + +func TestFetchPublicOnlyModePrivateIPRejected(t *testing.T) { + // Explicit restatement of the private-IP case under PublicOnlyMode so + // this coverage file names both halves of gap (f). + srv := httptest.NewTLSServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + t.Error("handler must not run for a private dial") + })) + t.Cleanup(srv.Close) + + _, err := Fetch(t.Context(), srv.URL, Policy{ + Mode: PublicOnlyMode, + Timeout: 2 * time.Second, + MaxBytes: 1024, + }, nil) + if reasonFrom(t, err) != ReasonPrivateIP { + t.Fatalf("reason = %q, want %s", reasonFrom(t, err), ReasonPrivateIP) + } +} + +func TestHostAllowedExactAndDottedSuffix(t *testing.T) { + allowed := []string{"discogs.com", ""} + if !hostAllowed("discogs.com", allowed) { + t.Fatal("exact match") + } + if !hostAllowed("api.discogs.com", allowed) { + t.Fatal("dotted-suffix match") + } + if hostAllowed("evil-discogs.com", allowed) { + t.Fatal("raw suffix must not match") + } + if hostAllowed("example.test", allowed) { + t.Fatal("unrelated host") + } +} + +func TestMapTransportErrorReasons(t *testing.T) { + if got := mapTransportError(errPrivateIP); got.Reason != ReasonPrivateIP { + t.Fatalf("private_ip = %s", got.Reason) + } + dns := &net.DNSError{Err: "no such host", Name: "nope.test", IsNotFound: true} + if got := mapTransportError(dns); got.Reason != ReasonUnresolvable { + t.Fatalf("dns = %s", got.Reason) + } + if got := mapTransportError(errors.New("connection reset")); got.Reason != ReasonNetworkError { + t.Fatalf("other = %s", got.Reason) + } +} + +func TestErrorStringWithoutInner(t *testing.T) { + e := &Error{Reason: ReasonScheme} + if e.Error() != "fetchguard: scheme" { + t.Fatalf("Error() = %q", e.Error()) + } + var nilE *Error + if nilE.Error() != "fetchguard: error" { + t.Fatalf("nil Error() = %q", nilE.Error()) + } + if nilE.Unwrap() != nil { + t.Fatal("nil Unwrap") + } +} diff --git a/surf/cors_coverage_test.go b/surf/cors_coverage_test.go new file mode 100644 index 0000000..215adc8 --- /dev/null +++ b/surf/cors_coverage_test.go @@ -0,0 +1,92 @@ +package surf + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +// Gap (d): pathScopedCORS with a path matching NONE of the configured +// globs. TestCORSPathScopedHeaders already covers the two named fonoteka +// groups; this fixture is framework-only (/healthz vs api/*). + +func TestCORSPathScopedNoMatchIndependentOfFonoteka(t *testing.T) { + cfg := CORSConfig{ + Paths: []string{"api/*", "oauth/mcp/*"}, + AllowedMethods: []string{"*"}, + AllowedOrigins: []string{"*"}, + AllowedHeaders: []string{"*"}, + } + inner := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + }) + h := pathScopedCORS(cfg, inner) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/healthz", nil)) + if rec.Code != http.StatusNoContent { + t.Fatalf("status = %d", rec.Code) + } + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Fatalf("unmatched path must not set ACAO, got %q", got) + } +} + +func TestCORSAllowOriginExactAndPattern(t *testing.T) { + cfg := CORSConfig{ + Paths: []string{"api/*"}, + AllowedMethods: []string{"GET", "POST"}, + AllowedOrigins: []string{"https://app.example.test"}, + AllowedOriginsPatterns: []string{`^https://.*\.example\.test$`}, + AllowedHeaders: []string{"Authorization", "Content-Type"}, + ExposedHeaders: []string{"X-RateLimit-Limit"}, + MaxAge: 600, + SupportsCredentials: true, + } + inner := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + h := pathScopedCORS(cfg, inner) + + t.Run("exact origin", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/api/v1/items", nil) + req.Header.Set("Origin", "https://app.example.test") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Header().Get("Access-Control-Allow-Origin") != "https://app.example.test" { + t.Fatalf("ACAO = %q", rec.Header().Get("Access-Control-Allow-Origin")) + } + if rec.Header().Get("Vary") != "Origin" { + t.Fatalf("Vary = %q", rec.Header().Get("Vary")) + } + if rec.Header().Get("Access-Control-Allow-Credentials") != "true" { + t.Fatal("missing credentials header") + } + if rec.Header().Get("Access-Control-Max-Age") != "600" { + t.Fatalf("Max-Age = %q", rec.Header().Get("Access-Control-Max-Age")) + } + }) + + t.Run("pattern origin", func(t *testing.T) { + req := httptest.NewRequest(http.MethodOptions, "/api/v1/items", nil) + req.Header.Set("Origin", "https://admin.example.test") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != http.StatusNoContent { + t.Fatalf("preflight status = %d", rec.Code) + } + if rec.Header().Get("Access-Control-Allow-Origin") != "https://admin.example.test" { + t.Fatalf("ACAO = %q", rec.Header().Get("Access-Control-Allow-Origin")) + } + }) + + t.Run("disallowed origin", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/api/v1/items", nil) + req.Header.Set("Origin", "https://evil.test") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Fatalf("disallowed origin ACAO = %q", got) + } + }) +} diff --git a/surf/limiter_coverage_test.go b/surf/limiter_coverage_test.go new file mode 100644 index 0000000..dc0dfcd --- /dev/null +++ b/surf/limiter_coverage_test.go @@ -0,0 +1,90 @@ +package surf + +import ( + "net/http" + "net/http/httptest" + "net/netip" + "os" + "path/filepath" + "testing" + "time" + + "git.golem15.com/golem15/summercms/compass" +) + +// Gap (b): MemoryStore's sweep goroutine (purge), not TooManyAttempts' lazy +// expiry. Construct with a short sweep and assert the internal map drops +// an expired entry without calling TooManyAttempts. + +func TestMemoryStoreSweepRemovesExpiredEntry(t *testing.T) { + s := NewMemoryStore(15 * time.Millisecond) + t.Cleanup(func() { close(s.stop) }) + + s.Hit("k", 25*time.Millisecond) + s.mu.Lock() + n := len(s.entries) + s.mu.Unlock() + if n != 1 { + t.Fatalf("after Hit, entries = %d", n) + } + + deadline := time.Now().Add(200 * time.Millisecond) + for time.Now().Before(deadline) { + s.mu.Lock() + n = len(s.entries) + s.mu.Unlock() + if n == 0 { + return + } + time.Sleep(10 * time.Millisecond) + } + t.Fatalf("sweep did not drop expired entry, count=%d", n) +} + +func TestMemoryStoreAvailableInExpiredAndMissing(t *testing.T) { + s := NewMemoryStore(0) + if d := s.AvailableIn("missing"); d != 0 { + t.Fatalf("missing AvailableIn = %s", d) + } + s.Hit("k", 20*time.Millisecond) + if d := s.AvailableIn("k"); d <= 0 { + t.Fatalf("live AvailableIn = %s", d) + } + time.Sleep(30 * time.Millisecond) + if d := s.AvailableIn("k"); d != 0 { + t.Fatalf("expired AvailableIn = %s", d) + } +} + +func TestTrustedProxiesParsesConfig(t *testing.T) { + if TrustedProxies(nil) != nil { + t.Fatal("nil cfg must return nil") + } + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "app.yaml"), []byte("name: t\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "http.yaml"), []byte("trusted_proxies:\n - 10.0.0.0/8\n - not-a-cidr\n - 192.168.0.0/16\n"), 0o644); err != nil { + t.Fatal(err) + } + cfg, err := compass.Open(compass.Options{Dir: dir, Environ: []string{}}) + if err != nil { + t.Fatal(err) + } + got := TrustedProxies(cfg) + if len(got) != 2 || got[0].String() != "10.0.0.0/8" || got[1].String() != "192.168.0.0/16" { + t.Fatalf("TrustedProxies = %v", got) + } +} + +func TestClientIPNilRequestAndEmptyXFF(t *testing.T) { + if got := ClientIP(nil, nil); got != "" { + t.Fatalf("nil request = %q", got) + } + trusted := []netip.Prefix{mustPrefix("10.0.0.0/8")} + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.RemoteAddr = "10.0.0.1:443" + if got := ClientIP(req, trusted); got != "10.0.0.1" { + t.Fatalf("trusted RemoteAddr, empty XFF = %q", got) + } +} diff --git a/surf/routetable_coverage_test.go b/surf/routetable_coverage_test.go new file mode 100644 index 0000000..5b8da19 --- /dev/null +++ b/surf/routetable_coverage_test.go @@ -0,0 +1,83 @@ +package surf + +import ( + "net/http" + "strings" + "testing" + + "git.golem15.com/golem15/summercms/pact" +) + +// Gap (c): RegisterHouseMiddlewareFactory duplicate-name failure. The +// non-house RegisterMiddlewareFactory duplicate path is already covered +// by TestDuplicateMiddlewareFactoryNamesPluginAndName. + +func TestDuplicateHouseMiddlewareFactoryNamesPluginAndName(t *testing.T) { + r := New(nil) + fn := func(string) pact.Middleware { + return func(next http.Handler) http.Handler { return next } + } + if err := r.RegisterHouseMiddlewareFactory("golem15.demo", "house.param", fn); err != nil { + t.Fatal(err) + } + err := r.RegisterHouseMiddlewareFactory("golem15.other", "house.param", fn) + if err == nil || !strings.Contains(err.Error(), "golem15.demo") || !strings.Contains(err.Error(), "house.param") { + t.Fatalf("want plugin and factory name in error, got %v", err) + } +} + +func TestRegisterHouseMiddlewareFactoryEmptyName(t *testing.T) { + r := New(nil) + err := r.RegisterHouseMiddlewareFactory("golem15.demo", "", func(string) pact.Middleware { + return func(next http.Handler) http.Handler { return next } + }) + if err == nil || !strings.Contains(err.Error(), "golem15.demo") { + t.Fatalf("empty factory name: %v", err) + } +} + +// Gap (g): Router.Routes() on an empty router, and an empty raw group +// (zero routes). Raw:true is inspectable on the Group itself; Routes() +// only lists registered handlers, so an empty raw group produces no +// RouteInfo — that is structural, not a missing assertion. + +func TestRoutesEmptyRouter(t *testing.T) { + r := New(nil) + got := r.Routes() + if got == nil { + t.Fatal("empty router Routes() must return a non-nil empty slice") + } + if len(got) != 0 { + t.Fatalf("empty router Routes() = %#v", got) + } + if (*Router)(nil).Routes() != nil { + t.Fatal("nil router Routes() must return nil") + } +} + +func TestEmptyRawGroupRawFlagOnGroupNotRouteInfo(t *testing.T) { + r := New(nil) + var inner *Group + r.GroupRaw("/oauth", nil, func(g pact.Router) { + inner = g.(*Group) + }) + if inner == nil || !inner.raw { + t.Fatal("empty GroupRaw must keep raw=true on Group") + } + if len(r.Routes()) != 0 { + t.Fatalf("empty raw group leaked RouteInfo: %v", r.Routes()) + } + + // Group.GroupRaw (nested) is a distinct method from Router.GroupRaw. + r.Group("/wrap", nil, func(g pact.Router) { + g.GroupRaw("/inner", nil, func(c pact.Router) { + inner = c.(*Group) + }) + }) + if inner == nil || !inner.raw { + t.Fatal("nested GroupRaw must be raw") + } + if len(r.Routes()) != 0 { + t.Fatalf("nested empty raw group leaked RouteInfo: %v", r.Routes()) + } +} diff --git a/wire/response_coverage_test.go b/wire/response_coverage_test.go new file mode 100644 index 0000000..ef772a1 --- /dev/null +++ b/wire/response_coverage_test.go @@ -0,0 +1,72 @@ +package wire + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" +) + +// Gap (e): Time.UnmarshalJSON round-trips both +00:00 and Z. The happy +// path already lives in TestTimeUnmarshalJSONAcceptsOffsetAndZ; this +// file covers null, invalid JSON, and a non-nil empty slice. + +func TestTimeUnmarshalJSONNullAndInvalid(t *testing.T) { + var tm Time + if err := json.Unmarshal([]byte("null"), &tm); err != nil { + t.Fatal(err) + } + if !tm.IsZero() { + t.Fatalf("null must decode to zero Time, got %v", tm.Time) + } + if err := json.Unmarshal([]byte(`"not-a-time"`), &tm); err == nil { + t.Fatal("invalid timestamp must fail") + } + if err := json.Unmarshal([]byte(`42`), &tm); err == nil { + t.Fatal("non-string must fail") + } + var nilT *Time + if err := nilT.UnmarshalJSON([]byte(`"2024-01-02T03:04:05Z"`)); err != nil { + t.Fatalf("nil receiver: %v", err) + } +} + +func TestTimeUnmarshalJSONOffsetAndZRoundTrip(t *testing.T) { + want := time.Date(2024, 1, 2, 3, 4, 5, 0, time.UTC) + for _, raw := range []string{`"2024-01-02T03:04:05+00:00"`, `"2024-01-02T03:04:05Z"`} { + var tm Time + if err := json.Unmarshal([]byte(raw), &tm); err != nil { + t.Fatalf("%s: %v", raw, err) + } + if !tm.Equal(want) { + t.Fatalf("%s = %v", raw, tm.Time) + } + b, err := json.Marshal(tm) + if err != nil { + t.Fatal(err) + } + if string(b) != `"2024-01-02T03:04:05+00:00"` { + t.Fatalf("marshal %s -> %s", raw, b) + } + } +} + +func TestWriteJSONEncodeErrorWritesOpaque500(t *testing.T) { + rec := httptest.NewRecorder() + WriteJSON(rec, http.StatusOK, make(chan int)) + if rec.Code != http.StatusInternalServerError { + t.Fatalf("status = %d", rec.Code) + } + if rec.Body.String() != `{"error":true,"message":"Internal server error"}` { + t.Fatalf("body = %q", rec.Body.String()) + } +} + +func TestSliceNonNilUnchanged(t *testing.T) { + in := []int{1, 2} + got := Slice(in) + if len(got) != 2 || got[0] != 1 || got[1] != 2 { + t.Fatalf("got %v", got) + } +}