Open
Conversation
Implements 6 features from the gateway improvement roadmap: - Upstream timeouts (DialTimeout, ResponseHeaderTimeout, IdleConnTimeout) - TLS configuration for backends (TLSSkipVerify, RequireTLS) - W3C TraceContext header propagation (traceparent, tracestate, X-Request-ID) - IP allowlist/denylist support (AllowedCIDRs, BlockedCIDRs) - Request size limits (MaxBodySize) - Per-route rate limiting infrastructure Domain changes: - Added 8 new fields to GatewayRoute struct - Extended CreateRouteParams with all new fields - Updated GetProxy interface to return route reference
- injectTraceHeaders: adds X-Request-ID and W3C TraceContext headers - checkCIDR: validates client IP against route allowlist/blocklist - limitedReader: enforces MaxBodySize on incoming requests - Updated Proxy to apply all Phase 1 middleware checks - Updated CreateRouteRequest with all new JSON fields - Fixed mock GetProxy signature to match interface change
- Added routes map for per-route limiter tracking - Added GetRouteLimiter method for route-specific rate limiting - Updated cleanupLoop to also clear routes map
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughExtended gateway routing to support CIDR-based IP access control, backend connection/timeout tuning (dial, idle, response timeouts), TLS verification options, request body size limits, and W3C-compliant request tracing. Updated data models, service interfaces, handler implementation, and rate limiter to enable per-route configuration and automatic trace header injection. ChangesGateway Routing Configuration & Control Flow
Sequence DiagramsequenceDiagram
participant Client
participant Handler as GatewayHandler
participant Service as GatewayService
participant Transport as http.Transport
participant Backend as Backend Server
Client->>Handler: HTTP Request
Handler->>Handler: injectTraceHeaders<br/>(set X-Request-ID,<br/>traceparent/tracestate)
Handler->>Service: GetProxy(method, path)
Service->>Service: Find matching route
Service->>Service: buildTLSConfig(route)
Service->>Service: Create Transport with<br/>dial/idle/response timeouts,<br/>TLS settings
Service-->>Handler: (*ReverseProxy,<br/>*GatewayRoute,<br/>params, found)
Handler->>Handler: checkCIDR(route)<br/>(check blocked/allowed<br/>CIDR lists)
alt Access Denied
Handler-->>Client: 403 Forbidden
else Access Allowed
Handler->>Handler: limitedReader<br/>(apply MaxBodySize)
Handler->>Handler: Inject path params<br/>into context
Handler->>Transport: Forward request
Transport->>Backend: Dial with configured<br/>timeout, TLS settings
Backend-->>Transport: Response
Transport-->>Handler: Response
Handler-->>Client: Response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 21 minutes and 19 seconds.Comment |
poyrazK
added a commit
that referenced
this pull request
May 2, 2026
- Add Close() to limitedReader to properly forward to underlying ReadCloser - Handle error from crypto/rand.Read in generateTraceID/generateSpanID - Add *slog.Logger to GatewayHandler and log malformed CIDR strings - Validate contradictory TLS settings (require_tls + tls_skip_verify) - Add unit tests for trace ID generation and CIDR check - Update NewGatewayHandler calls to pass logger
- Add nil check before h.logger.Warn() to prevent panic - Add comment explaining error shadowing in limitedReader.Read - Fix missing trailing newlines in test files
poyrazK
force-pushed
the
release/gateway-phase1
branch
from
bb8c7e4 to
17e301f
Compare
Add OpenAPI definitions for new gateway route fields: - Timeout settings: dial_timeout, response_header_timeout, idle_conn_timeout - TLS settings: tls_skip_verify, require_tls - Security: allowed_cidrs, blocked_cidrs - Request limits: max_body_size
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
internal/core/services/gateway.go (3)
111-111: 💤 Low valueSilent failure after route creation.
The error from
RefreshRoutesis discarded. If refresh fails, the newly created route won't be active in the proxy until the next successful refresh, potentially confusing the user.As per coding guidelines: "Do not use silent failures - avoid blank identifier assignment like
_ = someFunc()"🔧 Suggested fix
- _ = s.RefreshRoutes(ctx) + if err := s.RefreshRoutes(ctx); err != nil { + s.logger.Warn("failed to refresh routes after create", "route_id", route.ID, "error", err) + } return route, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/gateway.go` at line 111, The call to s.RefreshRoutes(ctx) currently discards errors via the blank identifier; change it to capture and handle the error from RefreshRoutes (e.g., err := s.RefreshRoutes(ctx)) and then handle it appropriately — log the error using the component logger or return/propagate the error from the surrounding function so route creation failures are visible and prevent silent failures; update any surrounding function signature if needed to return the error so callers can respond.
49-49: 💤 Low valueSilent failure on initial route load.
Discarding the error from
RefreshRouteshides startup failures. Consider logging the error or returning it so callers can decide how to handle initialization issues.As per coding guidelines: "Do not use silent failures - avoid blank identifier assignment like
_ = someFunc()"🔧 Suggested fix
// Initial load - _ = s.RefreshRoutes(context.Background()) + if err := s.RefreshRoutes(context.Background()); err != nil && logger != nil { + logger.Warn("failed to load initial routes", "error", err) + } return s🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/gateway.go` at line 49, The call to s.RefreshRoutes(context.Background()) currently discards errors using the blank identifier; capture the returned error (err := s.RefreshRoutes(context.Background())) and handle it instead of dropping it—either return the error to the caller (propagate initialization failure) or log it with the service logger (e.g., s.logger.Errorf / s.logger.Error) including the error value and context ("failed to refresh routes"). Replace the `_ =` usage with this explicit error capture and handling in the surrounding function.
160-164: ⚡ Quick winSilently skipping routes with invalid proxy configuration.
When
createReverseProxyfails, the error is discarded and the loop continues. This can hide configuration issues (e.g., invalidTargetURL) making debugging difficult.🔧 Suggested fix
for _, r := range routes { proxy, err := s.createReverseProxy(r) if err != nil { + if s.logger != nil { + s.logger.Warn("failed to create proxy for route", "route_id", r.ID, "name", r.Name, "error", err) + } continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/gateway.go` around lines 160 - 164, The loop that calls s.createReverseProxy(r) currently swallows errors (just continue) which hides bad route configs; update the loop that iterates over routes so that when s.createReverseProxy(r) returns an error you log or surface it with context (include route identifier like r.Name or r.TargetURL and the error message) instead of silently continuing — ideally collect such errors and return a combined error from the surrounding function or at minimum call the service logger (e.g., s.logger.Errorf) with a clear message indicating which route failed and why so misconfigurations in createReverseProxy are visible.docs/swagger/swagger.json (1)
11069-11122: ⚡ Quick winAdd bounds and docs to new
CreateRouteRequesttunables.
integertunables currently accept negative values in the schema contract. Addminimumconstraints (and brief descriptions) to prevent invalid client input.Proposed schema tightening
"dial_timeout": { - "type": "integer" + "description": "TCP dial timeout in milliseconds", + "type": "integer", + "minimum": 0 }, "idle_conn_timeout": { - "type": "integer" + "description": "Idle connection timeout in milliseconds", + "type": "integer", + "minimum": 0 }, "max_body_size": { - "type": "integer" + "description": "Max request body size in bytes", + "type": "integer", + "minimum": 0 }, @@ "response_header_timeout": { - "type": "integer" + "description": "Time to receive headers in milliseconds", + "type": "integer", + "minimum": 0 },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/swagger/swagger.json` around lines 11069 - 11122, The integer tunables in the CreateRouteRequest schema currently allow negatives; update each integer property (dial_timeout, idle_conn_timeout, max_body_size, priority, rate_limit, response_header_timeout) to include a minimum: 0 constraint and add a brief description indicating the unit and that the value must be non-negative (e.g., "Dial timeout in milliseconds; must be >= 0"). Ensure you modify the corresponding properties in the swagger JSON for CreateRouteRequest so clients cannot submit negative values.internal/handlers/gateway_handler_cidr_test.go (1)
12-58: ⚡ Quick winPlease fold the CIDR cases into a table-driven test.
The setup is identical across these three scenarios, so table-driving them will make it easier to extend coverage as the access-control rules evolve.
As per coding guidelines,
**/*_test.go: "Use table-driven tests in test files".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handlers/gateway_handler_cidr_test.go` around lines 12 - 58, The three nearly identical tests for checkCIDR should be consolidated into one table-driven test to reduce duplication: create a TestCheckCIDR_Table (or rename one of the existing tests) that iterates over a slice of cases (each case including name, RemoteAddr value, route.BlockedCIDRs, route.AllowedCIDRs, and expected bool), set up the shared test harness (gin.TestMode, GatewayHandler{logger:nil}, gin.CreateTestContext + request) once per subtest, call handler.checkCIDR(c, &domain.GatewayRoute{BlockedCIDRs:..., AllowedCIDRs:...}) and assert the expected result using t.Run for each case; reference the existing symbols handler.checkCIDR, GatewayHandler, and domain.GatewayRoute when locating code to change.internal/handlers/gateway_handler_trace_test.go (1)
9-38: ⚡ Quick winPrefer a table-driven test for the trace/span ID cases.
These four checks use the same harness and only vary by generator and expected length/uniqueness assertion, so a table would cut duplication and match the repo’s test convention.
As per coding guidelines,
**/*_test.go: "Use table-driven tests in test files".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handlers/gateway_handler_trace_test.go` around lines 9 - 38, Combine the four separate tests into a single table-driven test that iterates over cases for generateTraceID and generateSpanID; for each case include the generator function (reference generateTraceID and generateSpanID), the expected length (32 for trace, 16 for span) and whether to check uniqueness, then run each case in a subtest (t.Run) calling the generator twice to assert length of the first result and inequality between the two results when uniqueness is required; keep t.Parallel() usage per subtest and preserve the same assertion messages.internal/handlers/gateway_handler.go (1)
159-161: ⚡ Quick winUse
httputil.Errorfor the new gateway error branches.These paths bypass the handler-layer error contract and return ad-hoc JSON instead of the standard mapped response shape.
As per coding guidelines,
internal/handlers/*_handler.go: "Use handler-layer error handling withhttputil.Error(c, err)to automatically map errors to HTTP status codes".Also applies to: 238-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handlers/gateway_handler.go` around lines 159 - 161, Replace the ad-hoc JSON responses in gateway_handler.go with handler-layer error handling: where you currently do c.JSON(http.StatusNotFound, gin.H{"error":"No route found for "+path}) (the !ok branch using variables c, ok, path), create an error value (e.g., errors.New or fmt.Errorf with the same message) and call httputil.Error(c, err) so the standard error-to-status mapping is used; apply the same change to the other ad-hoc branches around the later section (the block you noted at 238-260) so all gateway error responses use httputil.Error(c, err) instead of c.JSON.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/domain/gateway.go`:
- Around line 24-31: The JSON tags for the optional Gateway config fields must
include omitempty; update the struct tags for DialTimeout,
ResponseHeaderTimeout, IdleConnTimeout, TLSSkipVerify, RequireTLS, AllowedCIDRs,
BlockedCIDRs, and MaxBodySize to add `omitempty` so zero/nil values are omitted
during serialization (e.g., change `json:"dial_timeout"` to
`json:"dial_timeout,omitempty"` etc.); if you anticipate needing to distinguish
an explicit false from omitted in future PATCH semantics, consider changing
TLSSkipVerify/RequireTLS to `*bool` before adding omitempty, otherwise keep them
as plain bools with omitempty.
In `@internal/core/services/gateway.go`:
- Around line 238-246: The buildTLSConfig function currently only sets
cfg.MinVersion when route.RequireTLS is true, leaving clients to default to
older TLS versions; update buildTLSConfig to always set a baseline
cfg.MinVersion = tls.VersionTLS12 and then, if route.RequireTLS requires a
stricter level, override it (e.g., set tls.VersionTLS13) based on
route.RequireTLS to ensure at least TLS 1.2 for all connections while still
allowing higher minima when required; modify the function referenced as
buildTLSConfig and use route.RequireTLS to conditionally raise the MinVersion.
In `@internal/handlers/gateway_handler.go`:
- Around line 190-203: The injectTraceHeaders function currently overwrites
inbound trace headers; change it to preserve incoming W3C TraceContext: check
c.GetHeader("traceparent") and c.GetHeader("tracestate") first and, if present,
copy them through to both c.Request.Header and c.Header instead of replacing
them; only call generateTraceID()/generateSpanID() and set a new traceparent
when no incoming traceparent exists, and do not clear tracestate when an inbound
value is present. Keep the existing X-Request-ID logic unchanged.
- Around line 223-262: The current logic silently skips parse errors and a nil
client IP which effectively fails open; update the enforcement so it fails
closed: when parsing c.ClientIP() into clientIP yields nil, call
c.AbortWithStatusJSON(http.StatusForbidden, ...) and return false instead of
returning true; similarly, if net.ParseCIDR(cidrStr) returns an error for any
entry in route.BlockedCIDRs or route.AllowedCIDRs, log the error via h.logger
(as you already do) and then abort the request (c.AbortWithStatusJSON with an
appropriate error) and return false rather than continuing; additionally, add a
note to validate/parse route.BlockedCIDRs and route.AllowedCIDRs at route
initialization time so misconfigured CIDRs fail fast before runtime.
- Around line 277-289: The current limitedReader.Read returning io.EOF lets
ReverseProxy forward silently truncated bodies; instead enforce
route.MaxBodySize in the request handling layer: check req.ContentLength against
route.MaxBodySize and immediately respond with HTTP 413 if ContentLength >
MaxBodySize, and for requests without Content-Length (chunked) pre-read up to
MaxBodySize+1 bytes (e.g., using io.LimitedReader or io.ReadFull) before handing
req.Body to the proxy and return 413 if more bytes exist; remove/stop relying on
limitedReader.Read to signal truncation (or change it to set a truncation flag
only) so that oversized requests are rejected by the handler (reference symbols:
limitedReader.Read, route.MaxBodySize and the handler function that wraps
req.Body before calling ReverseProxy).
In `@pkg/ratelimit/limiter.go`:
- Around line 57-72: GetRouteLimiter currently always uses the global i.rate and
i.burst so per-route GatewayRoute.RateLimit is ignored; change
GetRouteLimiter(routeID uuid.UUID, key string) to accept explicit per-route
limits (e.g. r rate.Limit, burst int) and use rate.NewLimiter(r, burst) when
creating a new limiter, and if a limiter already exists call limiter.SetLimit(r)
(and adjust burst by replacing the limiter or recreating if burst differs) so
route-specific RateLimit and burst are enforced; update callers to pass
rate.Limit(route.RateLimit) and an appropriate burst (e.g.
int(route.RateLimit)).
---
Nitpick comments:
In `@docs/swagger/swagger.json`:
- Around line 11069-11122: The integer tunables in the CreateRouteRequest schema
currently allow negatives; update each integer property (dial_timeout,
idle_conn_timeout, max_body_size, priority, rate_limit, response_header_timeout)
to include a minimum: 0 constraint and add a brief description indicating the
unit and that the value must be non-negative (e.g., "Dial timeout in
milliseconds; must be >= 0"). Ensure you modify the corresponding properties in
the swagger JSON for CreateRouteRequest so clients cannot submit negative
values.
In `@internal/core/services/gateway.go`:
- Line 111: The call to s.RefreshRoutes(ctx) currently discards errors via the
blank identifier; change it to capture and handle the error from RefreshRoutes
(e.g., err := s.RefreshRoutes(ctx)) and then handle it appropriately — log the
error using the component logger or return/propagate the error from the
surrounding function so route creation failures are visible and prevent silent
failures; update any surrounding function signature if needed to return the
error so callers can respond.
- Line 49: The call to s.RefreshRoutes(context.Background()) currently discards
errors using the blank identifier; capture the returned error (err :=
s.RefreshRoutes(context.Background())) and handle it instead of dropping
it—either return the error to the caller (propagate initialization failure) or
log it with the service logger (e.g., s.logger.Errorf / s.logger.Error)
including the error value and context ("failed to refresh routes"). Replace the
`_ =` usage with this explicit error capture and handling in the surrounding
function.
- Around line 160-164: The loop that calls s.createReverseProxy(r) currently
swallows errors (just continue) which hides bad route configs; update the loop
that iterates over routes so that when s.createReverseProxy(r) returns an error
you log or surface it with context (include route identifier like r.Name or
r.TargetURL and the error message) instead of silently continuing — ideally
collect such errors and return a combined error from the surrounding function or
at minimum call the service logger (e.g., s.logger.Errorf) with a clear message
indicating which route failed and why so misconfigurations in createReverseProxy
are visible.
In `@internal/handlers/gateway_handler_cidr_test.go`:
- Around line 12-58: The three nearly identical tests for checkCIDR should be
consolidated into one table-driven test to reduce duplication: create a
TestCheckCIDR_Table (or rename one of the existing tests) that iterates over a
slice of cases (each case including name, RemoteAddr value, route.BlockedCIDRs,
route.AllowedCIDRs, and expected bool), set up the shared test harness
(gin.TestMode, GatewayHandler{logger:nil}, gin.CreateTestContext + request) once
per subtest, call handler.checkCIDR(c, &domain.GatewayRoute{BlockedCIDRs:...,
AllowedCIDRs:...}) and assert the expected result using t.Run for each case;
reference the existing symbols handler.checkCIDR, GatewayHandler, and
domain.GatewayRoute when locating code to change.
In `@internal/handlers/gateway_handler_trace_test.go`:
- Around line 9-38: Combine the four separate tests into a single table-driven
test that iterates over cases for generateTraceID and generateSpanID; for each
case include the generator function (reference generateTraceID and
generateSpanID), the expected length (32 for trace, 16 for span) and whether to
check uniqueness, then run each case in a subtest (t.Run) calling the generator
twice to assert length of the first result and inequality between the two
results when uniqueness is required; keep t.Parallel() usage per subtest and
preserve the same assertion messages.
In `@internal/handlers/gateway_handler.go`:
- Around line 159-161: Replace the ad-hoc JSON responses in gateway_handler.go
with handler-layer error handling: where you currently do
c.JSON(http.StatusNotFound, gin.H{"error":"No route found for "+path}) (the !ok
branch using variables c, ok, path), create an error value (e.g., errors.New or
fmt.Errorf with the same message) and call httputil.Error(c, err) so the
standard error-to-status mapping is used; apply the same change to the other
ad-hoc branches around the later section (the block you noted at 238-260) so all
gateway error responses use httputil.Error(c, err) instead of c.JSON.
🪄 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: c001f51f-ce85-47f5-b1f5-88c7d699a2d5
📒 Files selected for processing (12)
docs/swagger/docs.godocs/swagger/swagger.jsondocs/swagger/swagger.yamlinternal/api/setup/router.gointernal/core/domain/gateway.gointernal/core/ports/gateway.gointernal/core/services/gateway.gointernal/handlers/gateway_handler.gointernal/handlers/gateway_handler_cidr_test.gointernal/handlers/gateway_handler_test.gointernal/handlers/gateway_handler_trace_test.gopkg/ratelimit/limiter.go
Comment on lines
+24
to
+31
| DialTimeout int64 `json:"dial_timeout"` // TCP dial timeout in milliseconds | ||
| ResponseHeaderTimeout int64 `json:"response_header_timeout"` // Time to receive headers in milliseconds | ||
| IdleConnTimeout int64 `json:"idle_conn_timeout"` // Idle connection timeout in milliseconds | ||
| TLSSkipVerify bool `json:"tls_skip_verify"` // Skip TLS verification for backend | ||
| RequireTLS bool `json:"require_tls"` // Force HTTPS for backend | ||
| AllowedCIDRs []string `json:"allowed_cidrs"` // IPs allowed to access (empty = all) | ||
| BlockedCIDRs []string `json:"blocked_cidrs"` // IPs blocked from access | ||
| MaxBodySize int64 `json:"max_body_size"` // Max request body size in bytes |
There was a problem hiding this comment.
New optional fields are missing omitempty in their JSON tags — coding guidelines violation.
All eight new fields are optional (zero/nil means "disabled" or "no restriction") but none carry omitempty:
| Field | Zero-value meaning |
|---|---|
DialTimeout / ResponseHeaderTimeout / IdleConnTimeout |
use transport default |
MaxBodySize |
no limit |
AllowedCIDRs / BlockedCIDRs |
no CIDR restriction |
TLSSkipVerify / RequireTLS |
safe defaults (false) |
For the bool fields, note that omitempty on a plain bool will omit false, which is fine for GET/serialization semantics here since false is the safe default. If a future update path (e.g. PATCH) needs to explicitly reset a flag to false and detect that in a JSON body, the fields would need to be *bool instead.
🔧 Proposed fix
- DialTimeout int64 `json:"dial_timeout"` // TCP dial timeout in milliseconds
- ResponseHeaderTimeout int64 `json:"response_header_timeout"` // Time to receive headers in milliseconds
- IdleConnTimeout int64 `json:"idle_conn_timeout"` // Idle connection timeout in milliseconds
- TLSSkipVerify bool `json:"tls_skip_verify"` // Skip TLS verification for backend
- RequireTLS bool `json:"require_tls"` // Force HTTPS for backend
- AllowedCIDRs []string `json:"allowed_cidrs"` // IPs allowed to access (empty = all)
- BlockedCIDRs []string `json:"blocked_cidrs"` // IPs blocked from access
- MaxBodySize int64 `json:"max_body_size"` // Max request body size in bytes
+ DialTimeout int64 `json:"dial_timeout,omitempty"` // TCP dial timeout in milliseconds
+ ResponseHeaderTimeout int64 `json:"response_header_timeout,omitempty"` // Time to receive headers in milliseconds
+ IdleConnTimeout int64 `json:"idle_conn_timeout,omitempty"` // Idle connection timeout in milliseconds
+ TLSSkipVerify bool `json:"tls_skip_verify,omitempty"` // Skip TLS verification for backend
+ RequireTLS bool `json:"require_tls,omitempty"` // Force HTTPS for backend
+ AllowedCIDRs []string `json:"allowed_cidrs,omitempty"` // IPs allowed to access (empty = all)
+ BlockedCIDRs []string `json:"blocked_cidrs,omitempty"` // IPs blocked from access
+ MaxBodySize int64 `json:"max_body_size,omitempty"` // Max request body size in bytesAs per coding guidelines: "Include json tags on struct fields with omitempty for optional fields."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| DialTimeout int64 `json:"dial_timeout"` // TCP dial timeout in milliseconds | |
| ResponseHeaderTimeout int64 `json:"response_header_timeout"` // Time to receive headers in milliseconds | |
| IdleConnTimeout int64 `json:"idle_conn_timeout"` // Idle connection timeout in milliseconds | |
| TLSSkipVerify bool `json:"tls_skip_verify"` // Skip TLS verification for backend | |
| RequireTLS bool `json:"require_tls"` // Force HTTPS for backend | |
| AllowedCIDRs []string `json:"allowed_cidrs"` // IPs allowed to access (empty = all) | |
| BlockedCIDRs []string `json:"blocked_cidrs"` // IPs blocked from access | |
| MaxBodySize int64 `json:"max_body_size"` // Max request body size in bytes | |
| DialTimeout int64 `json:"dial_timeout,omitempty"` // TCP dial timeout in milliseconds | |
| ResponseHeaderTimeout int64 `json:"response_header_timeout,omitempty"` // Time to receive headers in milliseconds | |
| IdleConnTimeout int64 `json:"idle_conn_timeout,omitempty"` // Idle connection timeout in milliseconds | |
| TLSSkipVerify bool `json:"tls_skip_verify,omitempty"` // Skip TLS verification for backend | |
| RequireTLS bool `json:"require_tls,omitempty"` // Force HTTPS for backend | |
| AllowedCIDRs []string `json:"allowed_cidrs,omitempty"` // IPs allowed to access (empty = all) | |
| BlockedCIDRs []string `json:"blocked_cidrs,omitempty"` // IPs blocked from access | |
| MaxBodySize int64 `json:"max_body_size,omitempty"` // Max request body size in bytes |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/domain/gateway.go` around lines 24 - 31, The JSON tags for the
optional Gateway config fields must include omitempty; update the struct tags
for DialTimeout, ResponseHeaderTimeout, IdleConnTimeout, TLSSkipVerify,
RequireTLS, AllowedCIDRs, BlockedCIDRs, and MaxBodySize to add `omitempty` so
zero/nil values are omitted during serialization (e.g., change
`json:"dial_timeout"` to `json:"dial_timeout,omitempty"` etc.); if you
anticipate needing to distinguish an explicit false from omitted in future PATCH
semantics, consider changing TLSSkipVerify/RequireTLS to `*bool` before adding
omitempty, otherwise keep them as plain bools with omitempty.
Comment on lines
+238
to
+246
| func (s *GatewayService) buildTLSConfig(route *domain.GatewayRoute) *tls.Config { | ||
| cfg := &tls.Config{ | ||
| InsecureSkipVerify: route.TLSSkipVerify, | ||
| } | ||
| if route.RequireTLS { | ||
| cfg.MinVersion = tls.VersionTLS12 | ||
| } | ||
| return cfg | ||
| } |
There was a problem hiding this comment.
TLS MinVersion not set when RequireTLS is false.
The static analysis warning is valid. When RequireTLS is false, no MinVersion is configured, defaulting to TLS 1.0 for client connections. Setting a baseline MinVersion of TLS 1.2 regardless of RequireTLS improves the default security posture for all backend connections.
🔒 Suggested fix
func (s *GatewayService) buildTLSConfig(route *domain.GatewayRoute) *tls.Config {
cfg := &tls.Config{
InsecureSkipVerify: route.TLSSkipVerify,
+ MinVersion: tls.VersionTLS12,
}
- if route.RequireTLS {
- cfg.MinVersion = tls.VersionTLS12
- }
return cfg
}Note: If RequireTLS is intended to enforce a higher minimum (e.g., TLS 1.3), adjust accordingly:
cfg.MinVersion = tls.VersionTLS12
if route.RequireTLS {
cfg.MinVersion = tls.VersionTLS13
}🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 238-240: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: route.TLSSkipVerify,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/services/gateway.go` around lines 238 - 246, The buildTLSConfig
function currently only sets cfg.MinVersion when route.RequireTLS is true,
leaving clients to default to older TLS versions; update buildTLSConfig to
always set a baseline cfg.MinVersion = tls.VersionTLS12 and then, if
route.RequireTLS requires a stricter level, override it (e.g., set
tls.VersionTLS13) based on route.RequireTLS to ensure at least TLS 1.2 for all
connections while still allowing higher minima when required; modify the
function referenced as buildTLSConfig and use route.RequireTLS to conditionally
raise the MinVersion.
Comment on lines
+190
to
+203
| func (h *GatewayHandler) injectTraceHeaders(c *gin.Context) { | ||
| requestID := c.GetHeader("X-Request-ID") | ||
| if requestID == "" { | ||
| requestID = uuid.New().String() | ||
| } | ||
| c.Request.Header.Set("X-Request-ID", requestID) | ||
| c.Header("X-Request-ID", requestID) | ||
|
|
||
| // W3C TraceContext | ||
| traceID := generateTraceID() | ||
| spanID := generateSpanID() | ||
| c.Request.Header.Set("traceparent", fmt.Sprintf("00-%s-%s-01", traceID, spanID)) | ||
| c.Request.Header.Set("tracestate", "") | ||
| } |
There was a problem hiding this comment.
Don't reset the trace context on every proxied request.
This helper always replaces traceparent and clears tracestate, so end-to-end tracing stops at the gateway hop instead of propagating through it. Preserve the inbound trace context when present and only mint a new one for requests that arrive without tracing headers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/handlers/gateway_handler.go` around lines 190 - 203, The
injectTraceHeaders function currently overwrites inbound trace headers; change
it to preserve incoming W3C TraceContext: check c.GetHeader("traceparent") and
c.GetHeader("tracestate") first and, if present, copy them through to both
c.Request.Header and c.Header instead of replacing them; only call
generateTraceID()/generateSpanID() and set a new traceparent when no incoming
traceparent exists, and do not clear tracestate when an inbound value is
present. Keep the existing X-Request-ID logic unchanged.
Comment on lines
+223
to
+262
| clientIP := net.ParseIP(c.ClientIP()) | ||
| if clientIP == nil { | ||
| return true // Allow if we can't parse IP | ||
| } | ||
|
|
||
| // Check blocked CIDRs first (takes precedence) | ||
| for _, cidrStr := range route.BlockedCIDRs { | ||
| _, ipNet, err := net.ParseCIDR(cidrStr) | ||
| if err != nil { | ||
| if h.logger != nil { | ||
| h.logger.Warn("invalid blocked CIDR", "cidr", cidrStr, "error", err) | ||
| } | ||
| continue | ||
| } | ||
| if ipNet.Contains(clientIP) { | ||
| c.AbortWithStatusJSON(http.StatusForbidden, gin.H{"error": "access denied"}) | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| // If allowlist is non-empty, only allow matched IPs | ||
| if len(route.AllowedCIDRs) > 0 { | ||
| allowed := false | ||
| for _, cidrStr := range route.AllowedCIDRs { | ||
| _, ipNet, err := net.ParseCIDR(cidrStr) | ||
| if err != nil { | ||
| if h.logger != nil { | ||
| h.logger.Warn("invalid allowed CIDR", "cidr", cidrStr, "error", err) | ||
| } | ||
| continue | ||
| } | ||
| if ipNet.Contains(clientIP) { | ||
| allowed = true | ||
| break | ||
| } | ||
| } | ||
| if !allowed { | ||
| c.AbortWithStatusJSON(http.StatusForbidden, gin.H{"error": "access denied"}) | ||
| return false | ||
| } |
There was a problem hiding this comment.
CIDR enforcement currently fails open on parse/config errors.
If the client IP cannot be parsed, this returns true, and malformed CIDRs are just skipped. That means a typo in allowed_cidrs/blocked_cidrs or an unparseable caller address silently disables the restriction you just added. Reject invalid CIDR config up front, and fail closed here whenever route-level CIDR rules cannot be evaluated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/handlers/gateway_handler.go` around lines 223 - 262, The current
logic silently skips parse errors and a nil client IP which effectively fails
open; update the enforcement so it fails closed: when parsing c.ClientIP() into
clientIP yields nil, call c.AbortWithStatusJSON(http.StatusForbidden, ...) and
return false instead of returning true; similarly, if net.ParseCIDR(cidrStr)
returns an error for any entry in route.BlockedCIDRs or route.AllowedCIDRs, log
the error via h.logger (as you already do) and then abort the request
(c.AbortWithStatusJSON with an appropriate error) and return false rather than
continuing; additionally, add a note to validate/parse route.BlockedCIDRs and
route.AllowedCIDRs at route initialization time so misconfigured CIDRs fail fast
before runtime.
Comment on lines
+277
to
+289
| func (l *limitedReader) Read(p []byte) (n int, err error) { | ||
| if l.read >= l.limit { | ||
| return 0, io.EOF | ||
| } | ||
| toRead := l.limit - l.read | ||
| if int64(len(p)) > toRead { | ||
| p = p[:toRead] | ||
| } | ||
| n, err = l.ReadCloser.Read(p) | ||
| l.read += int64(n) | ||
| if l.read >= l.limit && err == nil { | ||
| err = io.EOF | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the gateway_handler.go file
cat -n internal/handlers/gateway_handler.go | head -300Repository: poyrazK/thecloud
Length of output: 10487
🏁 Script executed:
# Search for where proxy comes from and what it is
rg "GetProxy" internal/handlers/gateway_handler.go -A 2 -B 2
# Search for proxy implementation
rg "type.*Proxy" internal/ --type go | head -20
# Search for MaxBodySize usage
rg "MaxBodySize" internal/ --type go -B 2 -A 2Repository: poyrazK/thecloud
Length of output: 3024
🏁 Script executed:
# Find GetProxy implementation in the service
rg "func.*GetProxy" internal/ --type go -A 10
# Search for the service that handles GetProxy
rg "GatewayService" internal/core/ports/ --type go -B 2 -A 15Repository: poyrazK/thecloud
Length of output: 7707
🏁 Script executed:
# Find httputil.ReverseProxy definition
find . -name "*.go" -type f -exec grep -l "ReverseProxy" {} \; | grep -E "(pkg|internal)" | head -10
# Check what httputil.ReverseProxy is
rg "type ReverseProxy" --type go -B 2 -A 5Repository: poyrazK/thecloud
Length of output: 173
🏁 Script executed:
# Search in pkg directory for ReverseProxy
rg "type ReverseProxy" pkg/ --type go -B 2 -A 15
# Also check the httputil import in gateway_handler.go
rg "httputil" internal/handlers/gateway_handler.goRepository: poyrazK/thecloud
Length of output: 854
🏁 Script executed:
# Check the httputil package
ls -la pkg/httputil/
# Find ReverseProxy definition
rg "ReverseProxy" . --type go -B 1 -A 8 | head -50Repository: poyrazK/thecloud
Length of output: 4427
🏁 Script executed:
# Check if httputil has ReverseProxy definition
cat pkg/httputil/*.go | grep -A 20 "ReverseProxy"
# Or check if it's imported from net/http/httputil
rg "NewSingleHostReverseProxy" . --type go -B 2 -A 2
# Check the actual gateway.go service to see how proxy is created
cat -n internal/core/services/gateway.go | grep -A 30 "func.*GetProxy"Repository: poyrazK/thecloud
Length of output: 3472
🏁 Script executed:
# Check if httputil is a custom package or import from standard library
cat pkg/httputil/response.go | head -20
# Search for Content-Length checks or pre-validation
rg "Content-Length" internal/handlers/gateway_handler.go
# Check if there's any check of request size before wrapping
rg -B 5 -A 5 "limitedReader" internal/handlers/gateway_handler.goRepository: poyrazK/thecloud
Length of output: 1317
🏁 Script executed:
# Check if httputil has a ReverseProxy type or if it's just re-exporting net/http/httputil
rg "ReverseProxy" pkg/httputil/ --type go -B 2 -A 10
# Check imports in httputil
cat pkg/httputil/*.go | grep -E "^import|ReverseProxy" | head -30Repository: poyrazK/thecloud
Length of output: 139
🏁 Script executed:
# Check if httputil is just re-exporting standard library's ReverseProxy
grep -r "ReverseProxy" pkg/httputil/
# Check the actual import statement and type usage in gateway.go
head -50 internal/core/services/gateway.go | grep -E "import|httputil"
# Get more context around where proxy is created
sed -n '240,280p' internal/core/services/gateway.goRepository: poyrazK/thecloud
Length of output: 1266
🏁 Script executed:
# Check for any tests around MaxBodySize and body truncation
rg "MaxBodySize" internal/handlers/gateway_handler_test.go -B 3 -A 10
# Check if there's Content-Length validation anywhere
rg "Content-Length" internal/ --type go -B 2 -A 2
# Verify the issue: when io.EOF is returned, the proxy sees complete body
cat -n pkg/httputil/*.go | grep -i "reverseproxy\|director"Repository: poyrazK/thecloud
Length of output: 1694
🏁 Script executed:
# Check the complete gateway service to see if there's any Content-Length validation
cat -n internal/core/services/gateway.go | head -150
# Check how gin/http handles io.EOF from request body
rg "Request\.Body" internal/handlers/gateway_handler.go -B 2 -A 2
# Verify that there's no 413 rejection logic
rg "413\|PayloadTooLarge\|RequestEntityTooLarge" internal/ --type goRepository: poyrazK/thecloud
Length of output: 6042
🏁 Script executed:
# Check if there's any middleware or earlier validation for request size
rg "MaxBodySize\|Request\.Body" internal/core/services/gateway.go -B 3 -A 3
# Look for any tests that verify behavior with oversized bodies
find . -name "*gateway*test.go" -type f -exec grep -l "MaxBodySize\|limit" {} \;
# Check the test file for insights on body size handling
cat internal/core/services/gateway_test.go | grep -A 20 "MaxBodySize"Repository: poyrazK/thecloud
Length of output: 190
🏁 Script executed:
# Check gateway_handler_test.go for MaxBodySize tests
cat internal/handlers/gateway_handler_test.go | grep -A 30 "MaxBodySize"
# Check gateway_e2e_test.go for body size tests
cat tests/gateway_e2e_test.go | grep -A 30 "MaxBodySize"Repository: poyrazK/thecloud
Length of output: 42
🏁 Script executed:
# Search for any tests in gateway files mentioning body or size
rg "body\|size\|limit" tests/gateway_e2e_test.go --type go -i | head -20
# Check if there are any tests for the Proxy handler
rg "func Test.*Proxy" internal/handlers/gateway_handler_test.go -A 30Repository: poyrazK/thecloud
Length of output: 4631
🏁 Script executed:
# Check how http.Request.ContentLength is used
rg "ContentLength" internal/handlers/ --type go -B 2 -A 2
# Check if there's any validation in the Proxy handler before calling proxy.ServeHTTP
cat -n internal/handlers/gateway_handler.go | sed -n '152,189p'Repository: poyrazK/thecloud
Length of output: 1240
Request size limit silently truncates bodies instead of rejecting them with HTTP 413.
When route.MaxBodySize > 0, the handler wraps the request body in a limitedReader that returns io.EOF upon reaching the limit. The standard library's httputil.ReverseProxy interprets io.EOF as a normal end-of-stream, not a truncation error, and forwards the incomplete payload to the backend. This causes:
- Corrupted file uploads and form submissions
- Invalid signatures (AWS, JWT, etc.) on truncated payloads
- Silent failures without error feedback to the client
The handler should reject oversized requests with HTTP 413 before passing them to the proxy, or check Content-Length headers upfront.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/handlers/gateway_handler.go` around lines 277 - 289, The current
limitedReader.Read returning io.EOF lets ReverseProxy forward silently truncated
bodies; instead enforce route.MaxBodySize in the request handling layer: check
req.ContentLength against route.MaxBodySize and immediately respond with HTTP
413 if ContentLength > MaxBodySize, and for requests without Content-Length
(chunked) pre-read up to MaxBodySize+1 bytes (e.g., using io.LimitedReader or
io.ReadFull) before handing req.Body to the proxy and return 413 if more bytes
exist; remove/stop relying on limitedReader.Read to signal truncation (or change
it to set a truncation flag only) so that oversized requests are rejected by the
handler (reference symbols: limitedReader.Read, route.MaxBodySize and the
handler function that wraps req.Body before calling ReverseProxy).
Comment on lines
+57
to
+72
| func (i *IPRateLimiter) GetRouteLimiter(routeID uuid.UUID, key string) *rate.Limiter { | ||
| i.mu.Lock() | ||
| defer i.mu.Unlock() | ||
|
|
||
| if i.routes[routeID] == nil { | ||
| i.routes[routeID] = make(map[string]*rate.Limiter) | ||
| } | ||
|
|
||
| limiter, exists := i.routes[routeID][key] | ||
| if !exists { | ||
| limiter = rate.NewLimiter(i.rate, i.burst) | ||
| i.routes[routeID][key] = limiter | ||
| } | ||
|
|
||
| return limiter | ||
| } |
There was a problem hiding this comment.
GetRouteLimiter ignores GatewayRoute.RateLimit — all routes get the same global rate.
rate.NewLimiter takes a rate.Limit (events per second) and burst size, but GetRouteLimiter hard-wires both to the global i.rate / i.burst initialized at construction. Since GatewayRoute.RateLimit (the per-route requests-per-second value) is never threaded through here, every route enforces an identical limit regardless of its individual configuration. The name "per-route rate limiting" doesn't hold.
The rate.Limiter also exposes SetLimit / SetLimitAt for live updates, so you could additionally update existing limiters when a route is reconfigured without needing a full eviction.
The minimum fix is to accept explicit r rate.Limit and burst int parameters from the caller, which already has access to the matched route.RateLimit:
🔧 Proposed fix
-// GetRouteLimiter returns a rate limiter for a specific route and client key.
-// This enables per-route rate limiting while maintaining per-client tracking.
-func (i *IPRateLimiter) GetRouteLimiter(routeID uuid.UUID, key string) *rate.Limiter {
+// GetRouteLimiter returns a rate limiter for a specific route and client key.
+// r and burst override the global defaults, enabling true per-route limits.
+func (i *IPRateLimiter) GetRouteLimiter(routeID uuid.UUID, key string, r rate.Limit, burst int) *rate.Limiter {
i.mu.Lock()
defer i.mu.Unlock()
if i.routes[routeID] == nil {
i.routes[routeID] = make(map[string]*rate.Limiter)
}
limiter, exists := i.routes[routeID][key]
if !exists {
- limiter = rate.NewLimiter(i.rate, i.burst)
+ limiter = rate.NewLimiter(r, burst)
i.routes[routeID][key] = limiter
}
return limiter
}The handler already holds the matched *domain.GatewayRoute and can pass rate.Limit(route.RateLimit) and a suitable burst (e.g. route.RateLimit) to the call site.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (i *IPRateLimiter) GetRouteLimiter(routeID uuid.UUID, key string) *rate.Limiter { | |
| i.mu.Lock() | |
| defer i.mu.Unlock() | |
| if i.routes[routeID] == nil { | |
| i.routes[routeID] = make(map[string]*rate.Limiter) | |
| } | |
| limiter, exists := i.routes[routeID][key] | |
| if !exists { | |
| limiter = rate.NewLimiter(i.rate, i.burst) | |
| i.routes[routeID][key] = limiter | |
| } | |
| return limiter | |
| } | |
| // GetRouteLimiter returns a rate limiter for a specific route and client key. | |
| // r and burst override the global defaults, enabling true per-route limits. | |
| func (i *IPRateLimiter) GetRouteLimiter(routeID uuid.UUID, key string, r rate.Limit, burst int) *rate.Limiter { | |
| i.mu.Lock() | |
| defer i.mu.Unlock() | |
| if i.routes[routeID] == nil { | |
| i.routes[routeID] = make(map[string]*rate.Limiter) | |
| } | |
| limiter, exists := i.routes[routeID][key] | |
| if !exists { | |
| limiter = rate.NewLimiter(r, burst) | |
| i.routes[routeID][key] = limiter | |
| } | |
| return limiter | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/ratelimit/limiter.go` around lines 57 - 72, GetRouteLimiter currently
always uses the global i.rate and i.burst so per-route GatewayRoute.RateLimit is
ignored; change GetRouteLimiter(routeID uuid.UUID, key string) to accept
explicit per-route limits (e.g. r rate.Limit, burst int) and use
rate.NewLimiter(r, burst) when creating a new limiter, and if a limiter already
exists call limiter.SetLimit(r) (and adjust burst by replacing the limiter or
recreating if burst differs) so route-specific RateLimit and burst are enforced;
update callers to pass rate.Limit(route.RateLimit) and an appropriate burst
(e.g. int(route.RateLimit)).
The tls_skip_verify feature intentionally allows users to disable TLS verification for development/testing backends. gosec G402 flags this pattern as a security risk, but it is the intended behavior for this feature, not a vulnerability.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements Phase 1 of the API Gateway improvement roadmap with 6 features:
dial_timeout,response_header_timeout,idle_conn_timeoutfields (in ms) toGatewayRoutetls_skip_verifyandrequire_tlsfor backend HTTPS communicationtraceparent,tracestate) andX-Request-IDto upstream requestsallowed_cidrsandblocked_cidrsfields with CIDR matchingmax_body_sizefield withlimitedReaderenforcementGetRouteLimitertoIPRateLimiterfor route-specific rate limitsChanges
Commits
Test plan
go test ./internal/handlers/ -run Gateway- 10/10 tests passgo test ./internal/core/services/ -run Gateway- tests passgo test ./pkg/ratelimit/...- tests passgo build ./...- builds successfullyFiles changed
internal/core/domain/gateway.gointernal/core/ports/gateway.gointernal/core/services/gateway.gointernal/handlers/gateway_handler.gointernal/handlers/gateway_handler_test.gopkg/ratelimit/limiter.goSummary by CodeRabbit
New Features
Documentation