From af95ef4a43874f67ae50bfa1bfea7f1001680743 Mon Sep 17 00:00:00 2001 From: Pascal Minder Date: Wed, 12 Aug 2026 11:42:36 +0200 Subject: [PATCH 1/3] Limit the number of forwarded IPs evaluated per request Theoretically an attacker could forge the X-Forwared-For or X-Real-IP header content with a long number of IP addresses. This would lead to a sequential look-up via the API. Each request can so take up to len(chain) * apiTimeoutMs time to resolve. Requests with more than 10 IPs across both headers are now denied with the same log-and-403 handling as unparsable addresses. --- geoblock.go | 8 ++++++ geoblock_test.go | 75 ++++++++++++++++++++++++++++++++++++++++++++++++ readme.md | 2 ++ 3 files changed, 85 insertions(+) diff --git a/geoblock.go b/geoblock.go index e9800ae..97550ac 100755 --- a/geoblock.go +++ b/geoblock.go @@ -26,6 +26,9 @@ const ( countryCodeLength = 2 defaultDeniedRequestHTTPStatusCode = 403 defaultCacheWriteCycle = 15 + // maxRequestIPCount caps how many IPs from X-Forwarded-For and X-Real-IP + // are evaluated per request. + maxRequestIPCount = 10 ) // Config the plugin configuration. @@ -514,6 +517,11 @@ func (a *GeoBlock) collectRemoteIP(req *http.Request) ([]*net.IP, error) { xRealIPValue := req.Header.Get(xRealIP) xRealIPList := strings.FieldsFunc(xRealIPValue, splitFn) + // Deny connection if the number of IP entries is larger than the allowed. + if ipCount := len(xForwardedForIPs) + len(xRealIPList); ipCount > maxRequestIPCount { + return nil, fmt.Errorf("too many IP addresses in request headers (%d, limit %d)", ipCount, maxRequestIPCount) + } + for _, value := range xForwardedForIPs { value = strings.Trim(value, " ") ipAddress, err := parseIP(value) diff --git a/geoblock_test.go b/geoblock_test.go index 6bcde5c..555432a 100755 --- a/geoblock_test.go +++ b/geoblock_test.go @@ -2065,6 +2065,81 @@ func TestTimeoutOnCacheRefresh_AllowWhenIgnoreTimeoutTrue(t *testing.T) { assertStatusCode(t, rec.Result(), http.StatusOK) } +func TestTooManyForwardedIPsDenied(t *testing.T) { + // Raise an error in the API, since the request must be rejected before any + // IP to country lookup is done. + apiStub := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + t.Error("API was called for a request exceeding the IP count limit") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("CH")) + })) + defer apiStub.Close() + + cfg := createTesterConfig() + cfg.API = apiStub.URL + "/{ip}" + cfg.Countries = append(cfg.Countries, "CH") + + ctx := context.Background() + next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}) + + handler, err := geoblock.New(ctx, next, cfg, t.Name()) + if err != nil { + t.Fatal(err) + } + + // 11 X-Forwarded-For entries + elevenIPs := make([]string, 11) + for i := range elevenIPs { + elevenIPs[i] = fmt.Sprintf("10.1.1.%d", i+1) + } + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "http://localhost", nil) + req.Header.Add(xForwardedFor, strings.Join(elevenIPs, ", ")) + handler.ServeHTTP(rec, req) + assertStatusCode(t, rec.Result(), http.StatusForbidden) + + // X-Real-IP entries count toward the same limit + rec = httptest.NewRecorder() + req = httptest.NewRequest(http.MethodGet, "http://localhost", nil) + req.Header.Add(xForwardedFor, strings.Join(elevenIPs[:10], ", ")) + req.Header.Add("X-Real-IP", "10.2.2.1") + handler.ServeHTTP(rec, req) + assertStatusCode(t, rec.Result(), http.StatusForbidden) +} + +func TestForwardedIPCountAtLimitAllowed(t *testing.T) { + apiStub := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("CH")) + })) + defer apiStub.Close() + + cfg := createTesterConfig() + cfg.API = apiStub.URL + "/{ip}" + cfg.Countries = append(cfg.Countries, "CH") + + ctx := context.Background() + next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}) + + handler, err := geoblock.New(ctx, next, cfg, t.Name()) + if err != nil { + t.Fatal(err) + } + + // Exactly 10 entries (the limit) must still be evaluated and allowed. + tenIPs := make([]string, 10) + for i := range tenIPs { + tenIPs[i] = chExampleIP + } + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "http://localhost", nil) + req.Header.Add(xForwardedFor, strings.Join(tenIPs, ", ")) + handler.ServeHTTP(rec, req) + assertStatusCode(t, rec.Result(), http.StatusOK) +} + func createTesterConfig() *geoblock.Config { cfg := geoblock.CreateConfig() diff --git a/readme.md b/readme.md index 4a21553..c0c5973 100755 --- a/readme.md +++ b/readme.md @@ -546,6 +546,8 @@ Allows to define a target for the logs of the middleware. The path must look lik Basically tells GeoBlock to only allow/deny a request based on the first IP address in the X-ForwardedFor HTTP header. This is useful for servers behind e.g. a Cloudflare proxy. +Note: requests carrying more than 10 IP addresses in total across the `X-Forwarded-For` and `X-Real-IP` headers are denied outright. Every collected address is looked up sequentially, so this limit prevents a single request with a fabricated many-hop header chain from stalling the middleware. Legitimate proxy chains should be far shorter. + ### Redirect denied requests `redirectUrlIfDenied` Allows redirecting denied requests with a HTTP 302 (Found) status code to the configured URL. So instead of "blocking" the client, the client will be redirected to the configured URL. From 364816affd1190648fd9dc84af885bad7cb2d51e Mon Sep 17 00:00:00 2001 From: Pascal Minder Date: Wed, 12 Aug 2026 13:38:50 +0200 Subject: [PATCH 2/3] Negatively cache failed country lookups A failed lookup cached nothing, so every subsequent request from that IP re-attempted the API and waited out up to apiTimeoutMs again. Failed lookups are now also cached as ipEntries for a short time (30s). Depending on the configuration they are allowed or denied. ipEntry gained a String method so cache log lines stay readable and distinguish negative entries. --- geoblock.go | 73 ++++++++++++++++++++++++++++++- geoblock_test.go | 109 +++++++++++++++++++++++++++++++++++++++++++++++ readme.md | 2 + 3 files changed, 182 insertions(+), 2 deletions(-) diff --git a/geoblock.go b/geoblock.go index 97550ac..ea6f348 100755 --- a/geoblock.go +++ b/geoblock.go @@ -29,6 +29,17 @@ const ( // maxRequestIPCount caps how many IPs from X-Forwarded-For and X-Real-IP // are evaluated per request. maxRequestIPCount = 10 + // failureCacheTTL is how long a failed lookup is remembered. Within this + // window requests from the same IP replay the configured allow/deny policy. + failureCacheTTL = 30 * time.Second +) + +// Kinds of lookup failure recorded in negatively cached entries. The timeout +// kind is kept separate because ignoreApiTimeout applies only to timeouts. +const ( + failureNone = iota + failureGeneric + failureTimeout ) // Config the plugin configuration. @@ -63,6 +74,19 @@ type Config struct { type ipEntry struct { Country string Timestamp time.Time + // Failure marks a negatively cached lookup. Zero means a successful lookup. + Failure int +} + +func (e ipEntry) String() string { + switch e.Failure { + case failureTimeout: + return fmt.Sprintf("{lookup timeout %s}", e.Timestamp) + case failureGeneric: + return fmt.Sprintf("{lookup failure %s}", e.Timestamp) + default: + return fmt.Sprintf("{%s %s}", e.Country, e.Timestamp) + } } // CreateConfig creates the default plugin configuration. @@ -370,10 +394,16 @@ func (a *GeoBlock) allowDenyIPAddress(requestIPAddr *net.IP, req *http.Request) // shouldRefreshEntry reports whether a cached entry has outlived its TTL and // must be re-fetched from the API. // -// An explicit cacheTtlSeconds takes effect on its own. When it is unset (<= 0), +// Negatively cached failures always expire after failureCacheTTL so the API is +// retried soon after an outage. For successful lookups an explicit +// cacheTtlSeconds takes effect on its own. When it is unset (<= 0), // forceMonthlyUpdate preserves the legacy behavior of refreshing entries once // they are ~30 days old otherwise entries never expire by age. func (a *GeoBlock) shouldRefreshEntry(entry ipEntry) bool { + if entry.Failure != failureNone { + return time.Since(entry.Timestamp) >= failureCacheTTL + } + ttl := a.cacheTTL if ttl <= 0 { if !a.forceMonthlyUpdate { @@ -414,6 +444,12 @@ func (a *GeoBlock) allowDenyCachedRequestIP(requestIPAddr *net.IP, req *http.Req } } + // a cached failure that has not expired yet. Replay the + // allow/deny policy without a new API round trip. + if entry.Failure != failureNone { + return a.handleCachedLookupFailure(entry, requestIPAddr), "" + } + // check if we are in black/white-list mode and allow/deny based on country code. // Note: allowUnknownCountries only has an effect in whitelist mode. In blacklist // mode an unknown country is, by definition, not on the blocklist, so @@ -454,7 +490,7 @@ func (a *GeoBlock) allowDenyCachedRequestIP(requestIPAddr *net.IP, req *http.Req return true, entry.Country } -// handleLookupError applies the configured fail-open policy to a failed +// handleLookupError applies the configured allow/deny policy to a failed // country lookup: any API failure allows the request when ignoreApiFailures is // set, a timeout allows it when ignoreApiTimeout is set, anything else denies. func (a *GeoBlock) handleLookupError(err error, requestIPAddr *net.IP) bool { @@ -472,6 +508,24 @@ func (a *GeoBlock) handleLookupError(err error, requestIPAddr *net.IP) bool { return false } +// handleCachedLookupFailure applies the same allow/deny policy as +// handleLookupError, but from a negatively cached entry instead of a live +// error, so no time is spent waiting on the API. +func (a *GeoBlock) handleCachedLookupFailure(entry ipEntry, requestIPAddr *net.IP) bool { + if a.ignoreAPIFailures { + a.infoLogger.Printf("%s: request allowed [%s] due to cached API failure", a.name, requestIPAddr) + return true + } + + if entry.Failure == failureTimeout && a.ignoreAPITimeout { + a.infoLogger.Printf("%s: request allowed [%s] due to cached API timeout", a.name, requestIPAddr) + return true + } + + a.infoLogger.Printf("%s: request denied [%s] due to cached API failure", a.name, requestIPAddr) + return false +} + func (a *GeoBlock) cachedRequestIP(requestIPAddr *net.IP, req *http.Request) (bool, string) { ipAddressString := requestIPAddr.String() cacheEntry, ok := a.database.Get(ipAddressString) @@ -501,6 +555,11 @@ func (a *GeoBlock) cachedRequestIP(requestIPAddr *net.IP, req *http.Request) (bo } } + // a negatively cached failure carries no country to report + if entry.Failure != failureNone { + return false, "" + } + return true, entry.Country } @@ -550,6 +609,16 @@ func (a *GeoBlock) createNewIPEntry(req *http.Request, ipAddressString string) ( country, err := a.getCountryCode(req, ipAddressString) if err != nil { + // Negatively cache the failure so requests within failureCacheTTL + // replay the allow/deny policy without another API round trip. + failure := failureGeneric + if os.IsTimeout(err) { + failure = failureTimeout + } + entry = ipEntry{Failure: failure, Timestamp: time.Now()} + a.database.Add(ipAddressString, entry) + a.ipDatabasePersistence.MarkDirty() + return entry, err } diff --git a/geoblock_test.go b/geoblock_test.go index 555432a..dbed767 100755 --- a/geoblock_test.go +++ b/geoblock_test.go @@ -2140,6 +2140,115 @@ func TestForwardedIPCountAtLimitAllowed(t *testing.T) { assertStatusCode(t, rec.Result(), http.StatusOK) } +func TestApiFailureNegativelyCached_Denied(t *testing.T) { + var apiCalls atomic.Int32 + apiStub := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + apiCalls.Add(1) + w.WriteHeader(http.StatusInternalServerError) + })) + defer apiStub.Close() + + cfg := createTesterConfig() + cfg.API = apiStub.URL + "/{ip}" + cfg.Countries = append(cfg.Countries, "CH") + // fail-closed defaults: lookup failures deny + + ctx := context.Background() + next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}) + + handler, err := geoblock.New(ctx, next, cfg, t.Name()) + if err != nil { + t.Fatal(err) + } + + // Both requests are denied, but only the first one may hit the API. The + // second must be answered from the negative cache. + for i := 0; i < 2; i++ { + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "http://localhost", nil) + req.Header.Add(xForwardedFor, chExampleIP) + handler.ServeHTTP(rec, req) + assertStatusCode(t, rec.Result(), http.StatusForbidden) + } + + if calls := apiCalls.Load(); calls != 1 { + t.Errorf("expected exactly 1 API call, got %d", calls) + } +} + +func TestApiFailureNegativelyCached_AllowedWhenIgnoreFailuresTrue(t *testing.T) { + var apiCalls atomic.Int32 + apiStub := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + apiCalls.Add(1) + w.WriteHeader(http.StatusInternalServerError) + })) + defer apiStub.Close() + + cfg := createTesterConfig() + cfg.API = apiStub.URL + "/{ip}" + cfg.Countries = append(cfg.Countries, "CH") + cfg.IgnoreAPIFailures = true // API failures should ALLOW + + ctx := context.Background() + next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}) + + handler, err := geoblock.New(ctx, next, cfg, t.Name()) + if err != nil { + t.Fatal(err) + } + + for i := 0; i < 2; i++ { + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "http://localhost", nil) + req.Header.Add(xForwardedFor, chExampleIP) + handler.ServeHTTP(rec, req) + assertStatusCode(t, rec.Result(), http.StatusOK) + } + + if calls := apiCalls.Load(); calls != 1 { + t.Errorf("expected exactly 1 API call, got %d", calls) + } +} + +func TestApiTimeoutNegativelyCached_AllowedWhenIgnoreTimeoutTrue(t *testing.T) { + var apiCalls atomic.Int32 + apiStub := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + apiCalls.Add(1) + time.Sleep(50 * time.Millisecond) // > APITimeoutMs below + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("CH")) + })) + defer apiStub.Close() + + cfg := createTesterConfig() + cfg.API = apiStub.URL + "/{ip}" + cfg.Countries = append(cfg.Countries, "CH") + cfg.APITimeoutMs = 5 // 5ms client timeout + cfg.IgnoreAPITimeout = true // timeouts should ALLOW + + ctx := context.Background() + next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}) + + handler, err := geoblock.New(ctx, next, cfg, t.Name()) + if err != nil { + t.Fatal(err) + } + + // The first request waits out the client timeout. The second one must be + // allowed from the negative cache without contacting the API again. + for i := 0; i < 2; i++ { + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "http://localhost", nil) + req.Header.Add(xForwardedFor, chExampleIP) + handler.ServeHTTP(rec, req) + assertStatusCode(t, rec.Result(), http.StatusOK) + } + + if calls := apiCalls.Load(); calls != 1 { + t.Errorf("expected exactly 1 API call, got %d", calls) + } +} + func createTesterConfig() *geoblock.Config { cfg := geoblock.CreateConfig() diff --git a/readme.md b/readme.md index c0c5973..a8f5905 100755 --- a/readme.md +++ b/readme.md @@ -482,6 +482,8 @@ If the `ignoreAPITimeout` option is set to `true`, a request is allowed even if If the `ignoreAPIFailures` option is set to `true`, a request is allowed even if the API returns a non-200 status code or an error occurs during the API request. +Note: failed lookups are remembered for 30 seconds. Within that window further requests from the same IP are allowed or denied according to the `ignoreAPIFailures`/`ignoreAPITimeout` policy directly from the cache, without waiting on the API again. After the window the API is retried. This keeps an API outage from adding up to `apiTimeoutMs` of latency to every request. + ### Set custom HTTP header field to retrieve the country code from `ipGeolocationHttpHeaderField` Allow setting the name of a custom HTTP header field to retrieve the country code from. E.g. `cf-ipcountry` for Cloudflare. From e1454bdd55b8926071476294fd269862a289b1c3 Mon Sep 17 00:00:00 2001 From: Pascal Minder Date: Wed, 12 Aug 2026 14:09:29 +0200 Subject: [PATCH 3/3] Harden the geolocation API client plumbing - Use on shared client per middleware, and the requests use NewRequestWithContext with the incoming request's context. - Fix issue with non-200 early return requests. Defer Body.Close(). - Bound request body read to a fixed size. - The shared transport raises MaxIdleConnsPerHost from the default 2 to 16. --- geoblock.go | 60 ++++++++++++++++++++++++++++----------- geoblock_test.go | 74 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 16 deletions(-) diff --git a/geoblock.go b/geoblock.go index ea6f348..a23f441 100755 --- a/geoblock.go +++ b/geoblock.go @@ -4,6 +4,7 @@ package geoblock import ( "context" "encoding/gob" + "errors" "fmt" "io" "log" @@ -32,6 +33,14 @@ const ( // failureCacheTTL is how long a failed lookup is remembered. Within this // window requests from the same IP replay the configured allow/deny policy. failureCacheTTL = 30 * time.Second + // maxAPIResponseBytes bounds how much of an API response is read. A valid + // response is a two-letter country code (or a short unknown sentinel), so + // anything larger is invalid and must not be buffered. + maxAPIResponseBytes = 64 + // apiMaxIdleConnsPerHost sizes the keep-alive connection pool towards the + // geolocation API. This allows to avoid a fresh TLS handshake for most + // calls. + apiMaxIdleConnsPerHost = 16 ) // Kinds of lookup failure recorded in negatively cached entries. The timeout @@ -103,7 +112,7 @@ type GeoBlock struct { logAllowedRequests bool logAPIRequests bool apiURI string - apiTimeoutMs int + apiClient *http.Client cacheTTL time.Duration ignoreAPITimeout bool ignoreAPIFailures bool @@ -250,7 +259,7 @@ func buildGeoBlock( logAllowedRequests: config.LogAllowedRequests, logAPIRequests: config.LogAPIRequests, apiURI: config.API, - apiTimeoutMs: config.APITimeoutMs, + apiClient: newAPIClient(config.APITimeoutMs), cacheTTL: time.Duration(config.CacheTTLSeconds) * time.Second, ignoreAPITimeout: config.IgnoreAPITimeout, ignoreAPIFailures: config.IgnoreAPIFailures, @@ -609,6 +618,12 @@ func (a *GeoBlock) createNewIPEntry(req *http.Request, ipAddressString string) ( country, err := a.getCountryCode(req, ipAddressString) if err != nil { + // If the request was canceled, do not cache the result, since it does + // not say anything about the health of the API. + if errors.Is(err, context.Canceled) { + return entry, err + } + // Negatively cache the failure so requests within failureCacheTTL // replay the allow/deny policy without another API round trip. failure := failureGeneric @@ -647,7 +662,7 @@ func (a *GeoBlock) getCountryCode(req *http.Request, ipAddressString string) (st ) } - country, err := a.callGeoJS(ipAddressString) + country, err := a.callGeoJS(req.Context(), ipAddressString) if err != nil { if !os.IsTimeout(err) && !a.ignoreAPITimeout { a.infoLogger.Printf("%s: %s", a.name, err) @@ -658,34 +673,48 @@ func (a *GeoBlock) getCountryCode(req *http.Request, ipAddressString string) (st return country, nil } -func (a *GeoBlock) callGeoJS(ipAddress string) (string, error) { - geoJsClient := http.Client{ - Timeout: time.Millisecond * time.Duration(a.apiTimeoutMs), +// newAPIClient builds the shared HTTP client for country lookups. All calls go +// to the single configured API host, so the transport keeps a larger idle +// connection pool than the default to avoid TLS handshakes on bursts. +func newAPIClient(timeoutMs int) *http.Client { + transport := http.DefaultTransport.(*http.Transport).Clone() + transport.MaxIdleConnsPerHost = apiMaxIdleConnsPerHost + + return &http.Client{ + Timeout: time.Millisecond * time.Duration(timeoutMs), + Transport: transport, } +} +func (a *GeoBlock) callGeoJS(ctx context.Context, ipAddress string) (string, error) { apiURI := strings.Replace(a.apiURI, "{ip}", ipAddress, 1) if a.logAPIRequests { a.infoLogger.Printf("%s: Sending request to %s", a.name, apiURI) } - req, err := http.NewRequest(http.MethodGet, apiURI, nil) + // The lookup runs on behalf of the incoming request: when its client goes + // away, the context cancels the API call instead of letting it run on. + req, err := http.NewRequestWithContext(ctx, http.MethodGet, apiURI, nil) if err != nil { return "", err } - res, err := geoJsClient.Do(req) + res, err := a.apiClient.Do(req) if err != nil { return "", err } + defer func() { + // Drain (bounded) so the keep-alive connection can be reused. + _, _ = io.Copy(io.Discard, io.LimitReader(res.Body, maxAPIResponseBytes)) + _ = res.Body.Close() + }() if res.StatusCode != http.StatusOK { return "", fmt.Errorf("API response status code: %d", res.StatusCode) } - if res.Body != nil { - defer res.Body.Close() - } - - body, err := io.ReadAll(res.Body) + // Never buffer more than a plausible response. An oversized body fails the + // length check below instead of exhausting memory. + body, err := io.ReadAll(io.LimitReader(res.Body, maxAPIResponseBytes)) if err != nil { return "", err } @@ -694,12 +723,11 @@ func (a *GeoBlock) callGeoJS(ipAddress string) (string, error) { countryCode := strings.TrimSuffix(sb, "\n") // api response for unknown country - if len([]rune(countryCode)) == len(a.unknownCountryCode) && countryCode == a.unknownCountryCode { + if countryCode == a.unknownCountryCode { return unknownCountryCode, nil } - // this could possible cause a DoS attack - if len([]rune(countryCode)) != countryCodeLength { + if len(countryCode) != countryCodeLength { return "", fmt.Errorf("API response has more or less than 2 characters") } diff --git a/geoblock_test.go b/geoblock_test.go index dbed767..845268b 100755 --- a/geoblock_test.go +++ b/geoblock_test.go @@ -2249,6 +2249,80 @@ func TestApiTimeoutNegativelyCached_AllowedWhenIgnoreTimeoutTrue(t *testing.T) { } } +func TestOversizedApiResponseDenied(t *testing.T) { + // A response far larger than any valid country code must be rejected + // without being buffered whole. + apiStub := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write(bytes.Repeat([]byte("X"), 1<<20)) + })) + defer apiStub.Close() + + cfg := createTesterConfig() + cfg.API = apiStub.URL + "/{ip}" + cfg.Countries = append(cfg.Countries, "CH") + + ctx := context.Background() + next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}) + + handler, err := geoblock.New(ctx, next, cfg, t.Name()) + if err != nil { + t.Fatal(err) + } + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "http://localhost", nil) + req.Header.Add(xForwardedFor, chExampleIP) + handler.ServeHTTP(rec, req) + + assertStatusCode(t, rec.Result(), http.StatusForbidden) +} + +func TestCanceledRequestNotNegativelyCached(t *testing.T) { + var apiCalls atomic.Int32 + apiStub := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + apiCalls.Add(1) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("CH")) + })) + defer apiStub.Close() + + cfg := createTesterConfig() + cfg.API = apiStub.URL + "/{ip}" + cfg.Countries = append(cfg.Countries, "CH") + + ctx := context.Background() + next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}) + + handler, err := geoblock.New(ctx, next, cfg, t.Name()) + if err != nil { + t.Fatal(err) + } + + // A request whose client already went away. The lookup is aborted and the + // request denied, but the abort must not be cached as an API failure. + canceledCtx, cancel := context.WithCancel(context.Background()) + cancel() + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "http://localhost", nil).WithContext(canceledCtx) + req.Header.Add(xForwardedFor, chExampleIP) + handler.ServeHTTP(rec, req) + assertStatusCode(t, rec.Result(), http.StatusForbidden) + + // The next request from the same IP must reach the API and succeed + // instead of replaying a bogus negative cache entry. + rec = httptest.NewRecorder() + req = httptest.NewRequest(http.MethodGet, "http://localhost", nil) + req.Header.Add(xForwardedFor, chExampleIP) + handler.ServeHTTP(rec, req) + assertStatusCode(t, rec.Result(), http.StatusOK) + + if calls := apiCalls.Load(); calls != 1 { + t.Errorf("expected exactly 1 API call, got %d", calls) + } +} + func createTesterConfig() *geoblock.Config { cfg := geoblock.CreateConfig()