feat: Treat rate limit header value as comma-separated list

This commit updates performRateLimiting to treat the rate limit header
value as a comma-separated list and enforce rate limiting based on the
first value in that list.

Certain HTTP headers, such as X-Forwarded-For and other headers that
are combined according to RFC 7230, can be represented as a
comma-separated list of values. Intermediate proxies may add their own
values to these headers, modifying the resulting value. For example,
an end user with a single IP address proxied through a fleet of load
balancers using the X-Forwarded-For header may be associated with
multiple X-Forwarded-For header values, e.g.,
"2.2.2.2,100.100.100.100" and "2.2.2.2,300.300.300.300". The current
implementation of performRateLimiting treats each of these as separate
rate limiting keys.

To address this issue, this commit splits the rate limit header by
commas and takes the first value (with whitespace removed) to use as
the rate limiting key.

Note that this logic is superficially similar to the
utilities.GetIPAddress function with two key differences. In
performRateLimiting, there is no set format for a given rate limiting
key, nor is there a fallback value after the first value in the list
that the API should use.

Author: jnschaeffer

internal/api/middleware.go

@@ -64,19 +64,43 @@ var emailRateLimitCounter = observability.ObtainMetricCounter("gotrue_email_rate
 func (a *API) performRateLimiting(lmt *limiter.Limiter, req *http.Request) error {
 	limitHeader := a.config.RateLimitHeader
 
+	// If no rate limit header was set, ignore rate limiting
 	if limitHeader == "" {
 		return nil
 	}
 
-	key := req.Header.Get(limitHeader)
+	valuesStr := req.Header.Get(limitHeader)
 
-	if key == "" {
+	// If a rate limit header was set, but has no value, ignore rate limiting but warn with an error
+	if valuesStr == "" {
 		log := observability.GetLogEntry(req).Entry
 		log.WithField("header", limitHeader).Warn("request does not have a value for the rate limiting header, rate limiting is not applied")
 
 		return nil
 	}
 
+	// According to RFC 7230 section 3.2.2, multiple headers with the same name are equivalent
+	// to a single header with that name where each value is separated by a comma and whitespace.
+	//
+	// Note that there is some ambiguity in RFC 7230 where section 3.2.4 states that
+	// header field values (which can contain commas) are processed independently of the header
+	// field name, and thus it is not always clear if a comma is a list delimiter or simply par
+	// of a single value.
+	//
+	// Given that this function is primarily for use with headers like X-Forwarded-For which
+	// vendors generally combine into comma-separated lists, we opt for the simpler approach
+	// here and split the header value by commas before taking the first value.
+	values := strings.SplitN(valuesStr, ",", 2)
+
+	// We will always get at least one value back, so this operation is safe
+	key := strings.TrimSpace(values[0])
+
+	// If the rate limit header has at least one value, but the first value is all whitespace, return an error
+	if key == "" {
+		return apierrors.NewBadRequestError(apierrors.ErrorCodeValidationFailed, "Invalid rate limit header value")
+	}
+
+	// Otherwise, apply rate limiting based on the first rate limit header value
 	if err := tollbooth.LimitByKeys(lmt, []string{key}); err != nil {
 		return apierrors.NewTooManyRequestsError(apierrors.ErrorCodeOverRequestRateLimit, "Request rate limit reached")
 	}

internal/api/middleware_test.go

@@ -415,6 +415,117 @@ func TestTimeoutResponseWriter(t *testing.T) {
 	require.Equal(t, w1.Result(), w2.Result())
 }
 
+func (ts *MiddlewareTestSuite) TestPerformRateLimiting() {
+	ts.Config.RateLimitHeader = "X-Test-Perform-Rate-Limiting"
+
+	tests := []struct {
+		name         string
+		headerValues []string
+		expError     error
+	}{
+		{
+			name: "no value",
+			headerValues: []string{
+				"",
+				"",
+			},
+			expError: nil,
+		},
+		{
+			name: "single end user value",
+			headerValues: []string{
+				"192.168.1.100",
+				"192.168.1.100",
+			},
+			expError: apierrors.NewTooManyRequestsError(
+				apierrors.ErrorCodeOverRequestRateLimit,
+				"Request rate limit reached",
+			),
+		},
+		{
+			name: "same end user value, multiple proxies",
+			headerValues: []string{
+				"2600:cafe:beef::1,192.168.1.100",
+				"2600:cafe:beef::1,192.168.1.200",
+			},
+			expError: apierrors.NewTooManyRequestsError(
+				apierrors.ErrorCodeOverRequestRateLimit,
+				"Request rate limit reached",
+			),
+		},
+		{
+			name: "multiple end user values, single proxy",
+			headerValues: []string{
+				"2600:cafe:beef::1,192.168.1.100",
+				"3700:dead:abcd::2,192.168.1.100",
+			},
+			expError: nil,
+		},
+		{
+			name: "same end user value, multiple proxies, with whitespace",
+			headerValues: []string{
+				"2600:cafe:beef::1     ,192.168.1.100",
+				"2600:cafe:beef::1 ,     192.168.1.200",
+			},
+			expError: apierrors.NewTooManyRequestsError(
+				apierrors.ErrorCodeOverRequestRateLimit,
+				"Request rate limit reached",
+			),
+		},
+		{
+			name: "malformed header, all whitespace",
+			headerValues: []string{
+				" ",
+			},
+			expError: apierrors.NewBadRequestError(
+				apierrors.ErrorCodeOverRequestRateLimit,
+				"Invalid rate limit header value",
+			),
+		},
+		{
+			name: "malformed header, no whitespace",
+			headerValues: []string{
+				",192.168.1.100",
+			},
+			expError: apierrors.NewBadRequestError(
+				apierrors.ErrorCodeOverRequestRateLimit,
+				"Invalid rate limit header value",
+			),
+		},
+		{
+			name: "malformed header, with whitespace",
+			headerValues: []string{
+				"     ,192.168.1.100",
+			},
+			expError: apierrors.NewBadRequestError(
+				apierrors.ErrorCodeOverRequestRateLimit,
+				"Invalid rate limit header value",
+			),
+		},
+	}
+
+	for _, tt := range tests {
+		// Trigger a rate limiting error if we see the same end-user key twice in the same
+		// test case
+		lmt := tollbooth.NewLimiter(
+			1,
+			&limiter.ExpirableOptions{
+				DefaultExpirationTTL: time.Hour,
+			},
+		)
+
+		var obsError error
+
+		for _, h := range tt.headerValues {
+			req := httptest.NewRequest(http.MethodGet, "http://localhost", nil)
+			req.Header.Add(ts.Config.RateLimitHeader, h)
+			obsError = ts.API.performRateLimiting(lmt, req)
+		}
+
+		require.ErrorIs(ts.T(), obsError, tt.expError, "error for test '%s'", tt.name)
+	}
+}
+
 func (ts *MiddlewareTestSuite) TestLimitHandler() {
 	ts.Config.RateLimitHeader = "X-Rate-Limit"
 	lmt := tollbooth.NewLimiter(5, &limiter.ExpirableOptions{