From 1a240952c739dd2e6c3b1eae47aea2f9938fb6ec Mon Sep 17 00:00:00 2001 From: CarterPerez-dev Date: Tue, 12 May 2026 02:56:29 -0400 Subject: [PATCH] fix(canary-phase2): address audit findings before rollup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two audit agents (code-reviewer + spec-adherence) both returned PASS but flagged substantive findings. Per fix-in-phase / no-backlog-rot, clearing every MEDIUM + LOW in-phase. Findings addressed: MEDIUM — realIP RemoteAddr fallback returned "IP:port" verbatim, where the spec wants the bare IP (geoip + downstream parsing assume host-only). Now uses net.SplitHostPort with raw-string fallback if not host:port. Adds 4 RemoteAddr cases: IPv4 strips port, IPv6 bracket form strips brackets+port, loopback IPv6, and the port-less raw fallback. MEDIUM — realIP XFF branch accepted whatever rightmost-comma-split produced, so headers like "198.51.100.1, " (trailing comma) yielded "" and skipped XRI/RemoteAddr entirely. Extracted into lastNonEmptyXFF which walks right-to-left and falls through cleanly. Adds two cases: trailing-comma falls through, all-empty entries fall through. MEDIUM — pixel.TransparentGIF was an exported mutable []byte; any caller could clobber it process-wide (mutating one response's body would affect every subsequent webbug trigger). Renamed to unexported transparentGIF + exposed pixel.Clone() and pixel.Len(). Webbug now calls pixel.Clone() per trigger; new test TestTrigger_ResponseBodyIsIndependentCopyPerCall verifies two triggers return independent slices. LOW (reviewer-escalated to HIGH-for-Phase-3) — Trigger on nil token returned a non-nil event with empty TokenID. Since events.token_id is a NOT NULL FK to tokens.id, the Phase 3 handler could not have persisted that event anyway, and the contract was implicit (would have required an inline comment, which the no-comments rule forbids). Now: nil token in → nil event, non-nil response out. Contract is explicit in the return shape. Test asserts evt is nil for nil-token path. LOW — no IPv6 coverage in IP-precedence tests. Added IPv6 cases for XFF rightmost and three RemoteAddr forms (bracketed, loopback, no-port). NIT — gracefulShutdown swallowed every shutdown error, returning nil unconditionally. Now collects with errors.Join so callers can detect partial-shutdown for telemetry/exit-code purposes. Audits accepted as-is (NIT, not blocking): - Artifact discriminated-union shape (Phase 3+ concern when other Kinds are produced) - Registry returning bare map (read-only post-Build; concurrent reads are safe by Go's memory model) - Build(_ Config) ignoring its arg (signature reserved for future stateful generators) Quality verified post-fix: build/vet/lint clean (0 issues), 14 webbug tests + 6 pixel tests + 4 registry tests PASS under -race, integration tests unchanged. --- .../backend/cmd/canary/main.go | 8 +- .../internal/token/generators/pixel/pixel.go | 12 +- .../token/generators/pixel/pixel_test.go | 41 +++- .../token/generators/webbug/generator.go | 45 ++-- .../token/generators/webbug/generator_test.go | 203 +++++++++++------- 5 files changed, 210 insertions(+), 99 deletions(-) diff --git a/PROJECTS/beginner/canary-token-generator/backend/cmd/canary/main.go b/PROJECTS/beginner/canary-token-generator/backend/cmd/canary/main.go index c0f88ec9..dea17cac 100644 --- a/PROJECTS/beginner/canary-token-generator/backend/cmd/canary/main.go +++ b/PROJECTS/beginner/canary-token-generator/backend/cmd/canary/main.go @@ -5,6 +5,7 @@ package main import ( "context" + "errors" "flag" "fmt" "log/slog" @@ -159,23 +160,28 @@ func gracefulShutdown( ) defer cancel() + var errs []error if err := srv.Shutdown(shutdownCtx, drainDelay); err != nil { logger.Error("server shutdown error", "error", err) + errs = append(errs, fmt.Errorf("server shutdown: %w", err)) } if telemetry != nil { if err := telemetry.Shutdown(shutdownCtx); err != nil { logger.Error("telemetry shutdown error", "error", err) + errs = append(errs, fmt.Errorf("telemetry shutdown: %w", err)) } } if err := rdb.Close(); err != nil { logger.Error("redis close error", "error", err) + errs = append(errs, fmt.Errorf("redis close: %w", err)) } if err := db.Close(); err != nil { logger.Error("database close error", "error", err) + errs = append(errs, fmt.Errorf("database close: %w", err)) } logger.Info("application stopped") - return nil + return errors.Join(errs...) } func setupLogger(cfg config.LogConfig) *slog.Logger { diff --git a/PROJECTS/beginner/canary-token-generator/backend/internal/token/generators/pixel/pixel.go b/PROJECTS/beginner/canary-token-generator/backend/internal/token/generators/pixel/pixel.go index 5b2159a7..dec64e3e 100644 --- a/PROJECTS/beginner/canary-token-generator/backend/internal/token/generators/pixel/pixel.go +++ b/PROJECTS/beginner/canary-token-generator/backend/internal/token/generators/pixel/pixel.go @@ -3,7 +3,9 @@ package pixel -var TransparentGIF = []byte{ +import "slices" + +var transparentGIF = []byte{ 0x47, 0x49, 0x46, 0x38, 0x39, 0x61, 0x01, 0x00, 0x01, 0x00, 0x80, 0x00, 0x00, 0x00, 0x00, 0x00, 0xff, 0xff, 0xff, 0x21, 0xf9, 0x04, 0x01, 0x00, 0x00, 0x00, 0x00, 0x2c, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x01, 0x00, @@ -11,3 +13,11 @@ var TransparentGIF = []byte{ } const ContentType = "image/gif" + +func Clone() []byte { + return slices.Clone(transparentGIF) +} + +func Len() int { + return len(transparentGIF) +} diff --git a/PROJECTS/beginner/canary-token-generator/backend/internal/token/generators/pixel/pixel_test.go b/PROJECTS/beginner/canary-token-generator/backend/internal/token/generators/pixel/pixel_test.go index a5347cb3..7edc0839 100644 --- a/PROJECTS/beginner/canary-token-generator/backend/internal/token/generators/pixel/pixel_test.go +++ b/PROJECTS/beginner/canary-token-generator/backend/internal/token/generators/pixel/pixel_test.go @@ -24,27 +24,32 @@ var ( gifTrailer = byte(0x3b) ) -func TestTransparentGIF_Length(t *testing.T) { - require.Len(t, pixel.TransparentGIF, expectedLength) +func TestLen_Is43(t *testing.T) { + require.Equal(t, expectedLength, pixel.Len()) } -func TestTransparentGIF_MagicBytes(t *testing.T) { +func TestClone_Length(t *testing.T) { + require.Len(t, pixel.Clone(), expectedLength) +} + +func TestClone_MagicBytes(t *testing.T) { + g := pixel.Clone() require.True( t, - bytes.HasPrefix(pixel.TransparentGIF, gif89aMagic), + bytes.HasPrefix(g, gif89aMagic), "expected GIF89a magic prefix, got % x", - pixel.TransparentGIF[:len(gif89aMagic)], + g[:len(gif89aMagic)], ) require.Equal( t, gifTrailer, - pixel.TransparentGIF[len(pixel.TransparentGIF)-1], + g[len(g)-1], "expected trailing GIF terminator 0x3B", ) } -func TestTransparentGIF_DecodesAsImageGIF(t *testing.T) { - img, err := gif.Decode(bytes.NewReader(pixel.TransparentGIF)) +func TestClone_DecodesAsImageGIF(t *testing.T) { + img, err := gif.Decode(bytes.NewReader(pixel.Clone())) require.NoError(t, err) require.NotNil(t, img) @@ -53,6 +58,26 @@ func TestTransparentGIF_DecodesAsImageGIF(t *testing.T) { require.Equal(t, expectedHeight, bounds.Dy()) } +func TestClone_ReturnsIndependentCopy(t *testing.T) { + a := pixel.Clone() + b := pixel.Clone() + require.Equal(t, a, b, "two clones must be byte-equal") + + a[0] = 0x00 + require.Equal( + t, + byte(0x47), + b[0], + "mutating one clone must not affect another", + ) + require.Equal( + t, + byte(0x47), + pixel.Clone()[0], + "mutating one clone must not affect the package-internal source", + ) +} + func TestContentType_IsImageGIF(t *testing.T) { require.Equal(t, "image/gif", pixel.ContentType) } diff --git a/PROJECTS/beginner/canary-token-generator/backend/internal/token/generators/webbug/generator.go b/PROJECTS/beginner/canary-token-generator/backend/internal/token/generators/webbug/generator.go index 47e17612..c0492b91 100644 --- a/PROJECTS/beginner/canary-token-generator/backend/internal/token/generators/webbug/generator.go +++ b/PROJECTS/beginner/canary-token-generator/backend/internal/token/generators/webbug/generator.go @@ -5,6 +5,7 @@ package webbug import ( "context" + "net" "net/http" "strings" @@ -50,25 +51,26 @@ func (g *Generator) Trigger( t *token.Token, r *http.Request, ) (*event.Event, *generators.TriggerResponse, error) { - tokenID := "" - if t != nil { - tokenID = t.ID - } - evt := &event.Event{ - TokenID: tokenID, - SourceIP: realIP(r), - UserAgent: optionalHeader(r.UserAgent()), - Referer: optionalHeader(r.Header.Get(headerReferer)), - } resp := &generators.TriggerResponse{ StatusCode: http.StatusOK, ContentType: pixel.ContentType, - Body: pixel.TransparentGIF, + Body: pixel.Clone(), ExtraHeaders: map[string]string{ headerCacheControl: cacheControlNoStore, headerPragma: pragmaNoCache, }, } + + if t == nil { + return nil, resp, nil + } + + evt := &event.Event{ + TokenID: t.ID, + SourceIP: realIP(r), + UserAgent: optionalHeader(r.UserAgent()), + Referer: optionalHeader(r.Header.Get(headerReferer)), + } return evt, resp, nil } @@ -84,12 +86,27 @@ func realIP(r *http.Request) string { if v := strings.TrimSpace(r.Header.Get(headerCFConnectingIP)); v != "" { return v } - if v := r.Header.Get(headerXForwardedFor); v != "" { - parts := strings.Split(v, ",") - return strings.TrimSpace(parts[len(parts)-1]) + if v := lastNonEmptyXFF(r.Header.Get(headerXForwardedFor)); v != "" { + return v } if v := strings.TrimSpace(r.Header.Get(headerXRealIP)); v != "" { return v } + if host, _, err := net.SplitHostPort(r.RemoteAddr); err == nil { + return host + } return r.RemoteAddr } + +func lastNonEmptyXFF(header string) string { + if header == "" { + return "" + } + parts := strings.Split(header, ",") + for i := len(parts) - 1; i >= 0; i-- { + if v := strings.TrimSpace(parts[i]); v != "" { + return v + } + } + return "" +} diff --git a/PROJECTS/beginner/canary-token-generator/backend/internal/token/generators/webbug/generator_test.go b/PROJECTS/beginner/canary-token-generator/backend/internal/token/generators/webbug/generator_test.go index fc2c2d9a..c5018981 100644 --- a/PROJECTS/beginner/canary-token-generator/backend/internal/token/generators/webbug/generator_test.go +++ b/PROJECTS/beginner/canary-token-generator/backend/internal/token/generators/webbug/generator_test.go @@ -106,72 +106,114 @@ func TestTrigger_RecordsEventWithRequestMetadata(t *testing.T) { }, ) - t.Run( - "source ip precedence CF > XFF (last) > XRI > RemoteAddr", - func(t *testing.T) { - cases := []struct { - name string - headers map[string]string - remote string - wantIP string - }{ - { - name: "CF wins over XFF and XRI", - headers: map[string]string{ - "CF-Connecting-IP": "203.0.113.10", - "X-Forwarded-For": "198.51.100.1, 198.51.100.2", - "X-Real-IP": "192.0.2.99", - }, - remote: "127.0.0.1:9999", - wantIP: "203.0.113.10", + t.Run("source ip precedence", func(t *testing.T) { + cases := []struct { + name string + headers map[string]string + remote string + wantIP string + }{ + { + name: "CF wins over XFF and XRI", + headers: map[string]string{ + "CF-Connecting-IP": "203.0.113.10", + "X-Forwarded-For": "198.51.100.1, 198.51.100.2", + "X-Real-IP": "192.0.2.99", }, - { - name: "XFF rightmost wins over XRI when no CF", - headers: map[string]string{ - "X-Forwarded-For": "198.51.100.1, 198.51.100.7", - "X-Real-IP": "192.0.2.99", - }, - remote: "127.0.0.1:9999", - wantIP: "198.51.100.7", + remote: "127.0.0.1:9999", + wantIP: "203.0.113.10", + }, + { + name: "XFF rightmost wins over XRI when no CF", + headers: map[string]string{ + "X-Forwarded-For": "198.51.100.1, 198.51.100.7", + "X-Real-IP": "192.0.2.99", }, - { - name: "XRI when no CF or XFF", - headers: map[string]string{ - "X-Real-IP": "192.0.2.99", - }, - remote: "127.0.0.1:9999", - wantIP: "192.0.2.99", + remote: "127.0.0.1:9999", + wantIP: "198.51.100.7", + }, + { + name: "XFF trailing-comma falls through to XRI", + headers: map[string]string{ + "X-Forwarded-For": "198.51.100.1, ", + "X-Real-IP": "192.0.2.99", }, - { - name: "RemoteAddr when no proxy headers", - headers: nil, - remote: "127.0.0.1:9999", - wantIP: "127.0.0.1:9999", + remote: "127.0.0.1:9999", + wantIP: "198.51.100.1", + }, + { + name: "XFF entirely empty entries fall through to XRI", + headers: map[string]string{ + "X-Forwarded-For": ", ,", + "X-Real-IP": "192.0.2.99", }, - { - name: "CF value is trimmed of whitespace", - headers: map[string]string{ - "CF-Connecting-IP": " 203.0.113.10 ", - }, - remote: "127.0.0.1:9999", - wantIP: "203.0.113.10", + remote: "127.0.0.1:9999", + wantIP: "192.0.2.99", + }, + { + name: "XRI when no CF or XFF", + headers: map[string]string{ + "X-Real-IP": "192.0.2.99", }, - } - for _, tc := range cases { - tc := tc - t.Run(tc.name, func(t *testing.T) { - r := httptest.NewRequest(http.MethodGet, "/c/token1", nil) - for k, v := range tc.headers { - r.Header.Set(k, v) - } - r.RemoteAddr = tc.remote - evt, _, err := g.Trigger(context.Background(), tok, r) - require.NoError(t, err) - require.Equal(t, tc.wantIP, evt.SourceIP) - }) - } - }, - ) + remote: "127.0.0.1:9999", + wantIP: "192.0.2.99", + }, + { + name: "RemoteAddr IPv4 strips port", + headers: nil, + remote: "127.0.0.1:9999", + wantIP: "127.0.0.1", + }, + { + name: "RemoteAddr IPv6 strips brackets and port", + headers: nil, + remote: "[2001:db8::1]:54321", + wantIP: "2001:db8::1", + }, + { + name: "RemoteAddr loopback IPv6 strips brackets and port", + headers: nil, + remote: "[::1]:9999", + wantIP: "::1", + }, + { + name: "RemoteAddr without port falls back to raw value", + headers: nil, + remote: "127.0.0.1", + wantIP: "127.0.0.1", + }, + { + name: "XFF IPv6 rightmost", + headers: map[string]string{ + "X-Forwarded-For": "198.51.100.1, 2001:db8::dead", + }, + remote: "127.0.0.1:9999", + wantIP: "2001:db8::dead", + }, + { + name: "CF value is trimmed of whitespace", + headers: map[string]string{ + "CF-Connecting-IP": " 203.0.113.10 ", + }, + remote: "127.0.0.1:9999", + wantIP: "203.0.113.10", + }, + } + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, "/c/token1", nil) + for k, v := range tc.headers { + r.Header.Set(k, v) + } + r.RemoteAddr = tc.remote + evt, _, err := g.Trigger(context.Background(), tok, r) + require.NoError(t, err) + require.NotNil(t, evt) + require.Equal(t, tc.wantIP, evt.SourceIP) + }) + } + }) t.Run( "missing user agent and referer record as nil pointers", @@ -183,6 +225,7 @@ func TestTrigger_RecordsEventWithRequestMetadata(t *testing.T) { evt, _, err := g.Trigger(context.Background(), tok, r) require.NoError(t, err) + require.NotNil(t, evt) require.Nil( t, evt.UserAgent, @@ -208,11 +251,30 @@ func TestTrigger_ResponseIs43ByteGIF(t *testing.T) { require.Equal(t, http.StatusOK, resp.StatusCode) require.Equal(t, pixel.ContentType, resp.ContentType) require.Len(t, resp.Body, gifByteLength) - require.Equal(t, pixel.TransparentGIF, resp.Body) + require.Equal(t, pixel.Clone(), resp.Body) require.Equal(t, cacheControlNoStore, resp.ExtraHeaders["Cache-Control"]) require.Equal(t, pragmaNoCache, resp.ExtraHeaders["Pragma"]) } +func TestTrigger_ResponseBodyIsIndependentCopyPerCall(t *testing.T) { + g := webbug.New() + tok := newWebbugToken("abc") + r := httptest.NewRequest(http.MethodGet, "/c/abc", nil) + + _, resp1, err := g.Trigger(context.Background(), tok, r) + require.NoError(t, err) + _, resp2, err := g.Trigger(context.Background(), tok, r) + require.NoError(t, err) + + resp1.Body[0] = 0x00 + require.Equal( + t, + byte(0x47), + resp2.Body[0], + "each Trigger call must produce an independent body slice", + ) +} + func TestTrigger_TokenNotFound_StillReturnsGIF(t *testing.T) { g := webbug.New() r := httptest.NewRequest(http.MethodGet, "/c/does-not-exist", nil) @@ -228,19 +290,10 @@ func TestTrigger_TokenNotFound_StillReturnsGIF(t *testing.T) { require.NotNil(t, resp, "nil-token path must still return GIF response") require.Equal(t, http.StatusOK, resp.StatusCode) require.Equal(t, pixel.ContentType, resp.ContentType) - require.Equal(t, pixel.TransparentGIF, resp.Body) - require.NotNil(t, evt, "event value still produced for forensic continuity") - require.Empty( + require.Equal(t, pixel.Clone(), resp.Body) + require.Nil( t, - evt.TokenID, - "empty TokenID signals nil token; persistence layer decides what to do", + evt, + "nil-token path returns nil event so the handler cannot accidentally persist a row with empty TokenID (FK violation)", ) - require.Equal( - t, - "203.0.113.100", - evt.SourceIP, - "source IP still captured for forensics", - ) - require.NotNil(t, evt.UserAgent) - require.Equal(t, "curl/8.0.0", *evt.UserAgent) }