test(gateway): add auth/rate-limit middleware integration tests (Closes #2)#35
Conversation
📝 WalkthroughWalkthroughThe PR rewrites gateway middleware token validation and rate-limit key derivation in ChangesGateway Middleware Auth and Rate Limiting
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthMiddleware
participant RateLimitMiddleware
participant Handler
Client->>AuthMiddleware: Request with Authorization: Bearer token
AuthMiddleware->>AuthMiddleware: validateToken(trim, prefix checks)
alt token missing or invalid
AuthMiddleware-->>Client: 401 unauthorized JSON
else token valid
AuthMiddleware->>AuthMiddleware: derive user_id/session_id via tokenSlug
AuthMiddleware->>RateLimitMiddleware: forward with context set
RateLimitMiddleware->>RateLimitMiddleware: rateLimitClientKey (user:<id> or api_key or ip)
alt bucket exhausted
RateLimitMiddleware-->>Client: 429 rate_limit_exceeded
else allowed
RateLimitMiddleware->>Handler: forward request
Handler-->>Client: response
end
end
Related issues: Suggested labels: go, gateway, tests Suggested reviewers: jackjin1997 🐰 A bunny hops through tokens tight, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
market/gateway/middleware_test.go (1)
81-98: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTest only proves bucket isolation, not bucket sharing, despite its name.
TestAuthBeforeRateLimitSharesPerUserBucketscalls each token exactly once, so it verifies that two different authenticated users don't collide into the same bucket, but never confirms that repeated requests from the same token share (and deplete) the same bucket. Consider adding a third call reusing"alpha-token-1"and asserting it now gets rate-limited (burst=1), which would more directly validate per-user bucket keying/sharing as the test name implies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@market/gateway/middleware_test.go` around lines 81 - 98, The test named TestAuthBeforeRateLimitSharesPerUserBuckets only proves that different authenticated users get separate rate-limit buckets, but it does not verify that the same token reuses the same bucket. Update this test to make an additional request with the same token (for example, reuse alpha-token-1 after its first successful call) and assert that it is rate-limited with burst=1. Keep the existing AuthMiddleware and RateLimitMiddleware setup, and use the same bearerRequest helper and handler/called assertions to validate per-user bucket sharing more directly.market/gateway/middleware.go (1)
439-450: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
tokenSlugsilently drops unrecognized characters, risking user-id collisions.Any rune outside
[a-z0-9]or-_.:is dropped entirely rather than rejected or preserved, so distinct raw tokens (e.g."abc!"and"abc") collapse to the same slug and thus the same deriveduser_id/session_id. SincevalidateTokenis the only implementation wired intoAuthMiddleware, this is presently the de-facto identity derivation, not just a test fixture.Consider rejecting tokens containing characters outside the allowed set instead of silently stripping them, to avoid identity collisions until real token validation is implemented.
♻️ Proposed fix: reject disallowed characters instead of dropping them
func tokenSlug(token string) string { var b strings.Builder for _, r := range strings.ToLower(token) { switch { case r >= 'a' && r <= 'z', r >= '0' && r <= '9': b.WriteRune(r) case r == '-', r == '_', r == '.', r == ':': b.WriteByte('_') + default: + return "" } } return strings.Trim(b.String(), "_") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@market/gateway/middleware.go` around lines 439 - 450, The tokenSlug helper is currently stripping any rune outside the allowed slug characters, which can collapse different tokens into the same derived identity. Update tokenSlug in middleware.go to validate the full token input and reject any disallowed characters instead of silently dropping them, so validateToken/AuthMiddleware cannot produce colliding user_id or session_id values. Keep the existing normalization for allowed characters, but ensure tokenSlug returns an error or otherwise signals invalid input when it encounters unsupported runes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@market/gateway/middleware_test.go`:
- Around line 100-139: The 429 response assertion in
TestAnonymousRequestsRateLimitedByIP is using expectJSONFields with
map[string]string, which cannot decode the numeric retry_after field from the
rate-limit payload. Update expectJSONFields to unmarshal into a generic map or a
typed struct that accepts mixed value types, then keep the existing error field
check so the RateLimitMiddleware response can be verified without JSON decoding
failing first.
---
Nitpick comments:
In `@market/gateway/middleware_test.go`:
- Around line 81-98: The test named TestAuthBeforeRateLimitSharesPerUserBuckets
only proves that different authenticated users get separate rate-limit buckets,
but it does not verify that the same token reuses the same bucket. Update this
test to make an additional request with the same token (for example, reuse
alpha-token-1 after its first successful call) and assert that it is
rate-limited with burst=1. Keep the existing AuthMiddleware and
RateLimitMiddleware setup, and use the same bearerRequest helper and
handler/called assertions to validate per-user bucket sharing more directly.
In `@market/gateway/middleware.go`:
- Around line 439-450: The tokenSlug helper is currently stripping any rune
outside the allowed slug characters, which can collapse different tokens into
the same derived identity. Update tokenSlug in middleware.go to validate the
full token input and reject any disallowed characters instead of silently
dropping them, so validateToken/AuthMiddleware cannot produce colliding user_id
or session_id values. Keep the existing normalization for allowed characters,
but ensure tokenSlug returns an error or otherwise signals invalid input when it
encounters unsupported runes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 741df287-9514-4f02-a50f-c50dd98d91f9
📒 Files selected for processing (4)
diagnostic/build-00000000.jsondiagnostic/build-00000000.logdmarket/gateway/middleware.gomarket/gateway/middleware_test.go
👮 Files not reviewed due to content moderation or server errors (2)
- diagnostic/build-00000000.json
- diagnostic/build-00000000.logd
| func TestAnonymousRequestsRateLimitedByIP(t *testing.T) { | ||
| handler := RateLimitMiddleware(1, 1)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.WriteHeader(http.StatusNoContent) | ||
| })) | ||
|
|
||
| first := httptest.NewRecorder() | ||
| req := httptest.NewRequest(http.MethodGet, "/health", nil) | ||
| req.RemoteAddr = "203.0.113.44:8080" | ||
| handler.ServeHTTP(first, req) | ||
| if first.Code != http.StatusNoContent { | ||
| t.Fatalf("first request status = %d", first.Code) | ||
| } | ||
|
|
||
| second := httptest.NewRecorder() | ||
| handler.ServeHTTP(second, req) | ||
| if second.Code != http.StatusTooManyRequests { | ||
| t.Fatalf("second request status = %d, want 429", second.Code) | ||
| } | ||
| expectJSONFields(t, second, map[string]string{"error": "rate_limit_exceeded"}) | ||
| } | ||
|
|
||
| func bearerRequest(token, remoteAddr string) *http.Request { | ||
| req := httptest.NewRequest(http.MethodGet, "/orders", nil) | ||
| req.RemoteAddr = remoteAddr | ||
| req.Header.Set("Authorization", "Bearer "+token) | ||
| return req | ||
| } | ||
|
|
||
| func expectJSONFields(t *testing.T, rec *httptest.ResponseRecorder, want map[string]string) { | ||
| t.Helper() | ||
| var got map[string]string | ||
| if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { | ||
| t.Fatalf("response is not JSON: %v; body=%s", err, rec.Body.String()) | ||
| } | ||
| for key, value := range want { | ||
| if got[key] != value { | ||
| t.Fatalf("field %q = %q, want %q; body=%s", key, got[key], value, rec.Body.String()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'FILE: market/gateway/middleware_test.go\n'
wc -l market/gateway/middleware_test.go
printf '\n--- relevant lines ---\n'
sed -n '1,220p' market/gateway/middleware_test.go
printf '\nFILE: market/gateway/middleware.go\n'
wc -l market/gateway/middleware.go
printf '\n--- relevant lines ---\n'
sed -n '1,240p' market/gateway/middleware.go
printf '\n--- json.Unmarshal probe ---\n'
python3 - <<'PY'
import json
samples = [
b'{"error":"rate_limit_exceeded","retry_after":123}',
b'{"error":"rate_limit_exceeded"}',
]
for s in samples:
print("sample:", s.decode())
try:
obj = json.loads(s)
print("loads ok:", obj)
except Exception as e:
print("loads err:", type(e).__name__, e)
PYRepository: jackjin1997/TentOfTrials
Length of output: 12978
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- RateLimitMiddleware excerpt ---\n'
sed -n '240,380p' market/gateway/middleware.go
printf '\n--- Go encoding/json probe ---\n'
cat >/tmp/json_probe.go <<'GO'
package main
import (
"encoding/json"
"fmt"
)
func main() {
var got map[string]string
err := json.Unmarshal([]byte(`{"error":"rate_limit_exceeded","retry_after":123}`), &got)
fmt.Printf("err=%T %v\n", err, err)
fmt.Printf("got=%#v\n", got)
}
GO
go run /tmp/json_probe.goRepository: jackjin1997/TentOfTrials
Length of output: 253
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- matches for rate limit response fields ---'
rg -n 'rate_limit_exceeded|retry_after|middlewareJSON\(' market/gateway/middleware.go
echo
echo '--- rate limit middleware excerpt ---'
python3 - <<'PY'
from pathlib import Path
text = Path('market/gateway/middleware.go').read_text().splitlines()
for i, line in enumerate(text, 1):
if 250 <= i <= 360:
print(f"{i:4d}: {line}")
PY
echo
echo '--- Go encoding/json probe ---'
cat >/tmp/json_probe.go <<'GO'
package main
import (
"encoding/json"
"fmt"
)
func main() {
var got map[string]string
err := json.Unmarshal([]byte(`{"error":"rate_limit_exceeded","retry_after":123}`), &got)
fmt.Printf("err=%T %v\n", err, err)
fmt.Printf("got=%#v\n", got)
}
GO
go run /tmp/json_probe.goRepository: jackjin1997/TentOfTrials
Length of output: 4822
Decode the rate-limit body into a generic map or typed struct. expectJSONFields uses map[string]string, so the 429 payload’s numeric retry_after field makes json.Unmarshal return cannot unmarshal number into Go value of type string before the "error" assertion runs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@market/gateway/middleware_test.go` around lines 100 - 139, The 429 response
assertion in TestAnonymousRequestsRateLimitedByIP is using expectJSONFields with
map[string]string, which cannot decode the numeric retry_after field from the
rate-limit payload. Update expectJSONFields to unmarshal into a generic map or a
typed struct that accepts mixed value types, then keep the existing error field
check so the RateLimitMiddleware response can be verified without JSON decoding
failing first.
1 participant
Summary
Adds focused Go integration tests for gateway auth + rate-limit middleware and minimal testability fixes in
middleware.go.Changes
market/gateway/middleware_test.go— missing/invalid token 401 JSON shape, valid token context propagation, auth-before-rate-limit ordering, per-user vs per-IP bucketsmarket/gateway/middleware.go— deterministic token stub, authenticated rate-limit keys, rename middleware helpers to avoid duplicate symbols withapi.goValidation
go test ./market/gateway -run Middleware -count=1 python build.pyCloses #2
Wallet:
Do4v7foHJvRJLpRRoGaVPWX6DDEjX3yTK7J91gpwUQpESummary by CodeRabbit
Bug Fixes
Tests