amadan
geoip: the strip treats underscores as hyphens, so X_Carlos_Geo_* aliases (one key to WSGI/CGI) never reach an app; tests allow only the five exact names through (Astra code review R12 P1)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3 files changed,
+38
−8
internal/edge/geoipheaders_test.go+11 −4internal/geoip/answer.go+13 −4internal/geoip/answer_test.go+14 −0
CI passed — run details
diff --git a/internal/edge/geoipheaders_test.go b/internal/edge/geoipheaders_test.go| index 7833229ba..8ccd150ad 100644 |
| --- a/internal/edge/geoipheaders_test.go |
| +++ b/internal/edge/geoipheaders_test.go |
| @@ -363,7 +363,7 @@ func TestForgeryMatrixAcrossLocalProxyTypes(t *testing.T) { |
| echo := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| got := map[string]string{} |
| for k := range r.Header { |
| - if strings.HasPrefix(strings.ToLower(k), "x-carlos-geo-") || strings.EqualFold(k, geoip.HeaderHop) { |
| + if n := strings.ToLower(strings.ReplaceAll(k, "_", "-")); strings.HasPrefix(n, "x-carlos-geo-") || n == strings.ToLower(geoip.HeaderHop) { |
| got[k] = r.Header.Get(k) |
| } |
| } |
| @@ -403,6 +403,8 @@ func TestForgeryMatrixAcrossLocalProxyTypes(t *testing.T) { |
| r.Header.Set("X-Carlos-Geo-City", "Atlantis") |
| r.Header.Set(geoip.HeaderHop, "hit;src=forged") |
| r.Header.Set("Connection", "X-Carlos-Geo-Country, X-Carlos-Geo-Source") |
| + r.Header["X_Carlos_Geo_Country"] = []string{"KP"} // WSGI reads this as HTTP_X_CARLOS_GEO_COUNTRY |
| + r.Header["X_Carlos_Hop_Geo"] = []string{"hit;src=forged"} |
| code, h := serveGeo(t, e, r) |
| if code != 200 { |
| t.Errorf("%s: status %d", name, code) |
| @@ -411,9 +413,14 @@ func TestForgeryMatrixAcrossLocalProxyTypes(t *testing.T) { |
| if h[geoip.HeaderCountry] != "GB" || h[geoip.HeaderSource] != "TestDB-GB" { |
| t.Errorf("%s: app saw %v, want the edge's GB answer", name, h) |
| } |
| - for _, bad := range []string{"X-Carlos-Geo-City", geoip.HeaderHop} { |
| - if _, ok := h[bad]; ok { |
| - t.Errorf("%s: %s reached the app", name, bad) |
| + // Only the edge's own five, under their exact names, may arrive: |
| + // any other spelling of the namespace (a city, the hop header, an |
| + // underscore alias WSGI would read as the real one) is a forgery. |
| + allowed := map[string]bool{geoip.HeaderCountry: true, geoip.HeaderSubdivision: true, |
| + geoip.HeaderLat: true, geoip.HeaderLon: true, geoip.HeaderSource: true} |
| + for k := range h { |
| + if !allowed[k] { |
| + t.Errorf("%s: %s reached the app", name, k) |
| } |
| } |
| } |
diff --git a/internal/geoip/answer.go b/internal/geoip/answer.go| index 7350d2fc3..2f35bee16 100644 |
| --- a/internal/geoip/answer.go |
| +++ b/internal/geoip/answer.go |
| @@ -70,17 +70,26 @@ func FromSnapshot(s geo.Snapshot) Answer { |
| // an X-Carlos-Geo-City (or any later name) that looks platform-set. |
| const geoPrefix = "X-Carlos-Geo-" |
| -// StripAll deletes every X-Carlos-Geo-* header and X-Carlos-Hop-Geo. |
| -// Header keys are canonicalised by net/http, but a map built by hand may |
| -// not be, so the match is case-insensitive. |
| +// StripAll deletes every X-Carlos-Geo-* header and X-Carlos-Hop-Geo, in |
| +// any case and with underscores in place of hyphens: CGI and WSGI turn |
| +// both into the same HTTP_X_CARLOS_GEO_* key, so X_Carlos_Geo_Country |
| +// would otherwise reach a Python app as the platform's own header (code |
| +// review R12-1). |
| func StripAll(h http.Header) { |
| for k := range h { |
| - if len(k) >= len(geoPrefix) && strings.EqualFold(k[:len(geoPrefix)], geoPrefix) || strings.EqualFold(k, HeaderHop) { |
| + if reserved(k) { |
| delete(h, k) |
| } |
| } |
| } |
| +// reserved reports whether a header name falls in the platform's geo |
| +// namespace once underscores are read as hyphens. |
| +func reserved(k string) bool { |
| + n := strings.ReplaceAll(k, "_", "-") |
| + return len(n) >= len(geoPrefix) && strings.EqualFold(n[:len(geoPrefix)], geoPrefix) || strings.EqualFold(n, HeaderHop) |
| +} |
| + |
| // WriteApp deletes the app headers and writes this answer's. A miss and |
| // off write nothing at all; Source is written only beside another field. |
| func (a Answer) WriteApp(h http.Header) { |
diff --git a/internal/geoip/answer_test.go b/internal/geoip/answer_test.go| index 9ff42c005..a709b670d 100644 |
| --- a/internal/geoip/answer_test.go |
| +++ b/internal/geoip/answer_test.go |
| @@ -100,3 +100,17 @@ func TestStripAllRemovesTheWholeNamespace(t *testing.T) { |
| t.Fatalf("Capture must strip the namespace too: %v", h2) |
| } |
| } |
| + |
| +// Code review R12-1: underscore spellings are the same header to CGI/WSGI. |
| +func TestStripAllCatchesUnderscoreSpellings(t *testing.T) { |
| + h := http.Header{} |
| + for _, k := range []string{"X_Carlos_Geo_Country", "X_CARLOS_GEO_SOURCE", "X-Carlos_Geo-Lat", "X_Carlos_Hop_Geo", "x_carlos_hop_geo"} { |
| + h[k] = []string{"forged"} |
| + } |
| + h.Set("X-Carlos-Version", "keep") |
| + h["X_Forwarded_For"] = []string{"keep"} |
| + StripAll(h) |
| + if len(h) != 2 { |
| + t.Fatalf("after strip = %v", h) |
| + } |
| +} |