feat: add IP blocklist settings#1771
Conversation
…stream When an upstream provider (observed with PackyCode proxy) returns a text content block before the thinking block (text → thinking order instead of the typical thinking → text), the inbound stream transformer failed to close the text block before opening the thinking block. This caused both blocks to share the same index, resulting in Claude Code reporting "Content block not found" errors. The fix adds a check for `hasTextContentStarted` in the reasoning content handler, mirroring the existing `hasToolContentStarted` check, ensuring the text block is properly closed (with citations flushed) before the thinking block begins. Closes looplj#1655
…block-before-thinking
Greptile SummaryThis PR adds a system-wide IP blocklist feature: admins can configure blocked IP addresses and CIDR ranges via a new Security tab, and the middleware rejects matching external API requests with 403 before authentication runs. A table shortcut also lets admins block/unblock IPs directly from request records.
Confidence Score: 3/5The PR should not merge until the missing authorization check on the security settings mutation is added; the DB-per-request pattern in the middleware is also worth addressing before the feature ships at scale. The
|
| Filename | Overview |
|---|---|
| internal/server/middleware/ip_blocklist.go | New middleware blocking requests by IP/CIDR; issues with uncached DB reads per request, context.Background() in log calls, and unconditional reading of untrusted proxy headers. |
| internal/server/gql/system.resolvers.go | New SecuritySettings query and UpdateSecuritySettings mutation added without the scopes.UserHasScope scope guard that other sensitive resolvers (TriggerGcCleanup, PreviewGcCleanup) enforce. |
| internal/server/routes.go | WithIPBlocklist applied before auth middleware on all four external API route groups; SystemService added to Services struct; ordering looks correct. |
| internal/server/biz/system.go | SecuritySettings and SetSecuritySettings added with proper normalization (trim, deduplicate); SecuritySettingsOrDefault for safe fallback; clean implementation. |
| internal/server/middleware/ip_blocklist_test.go | Good unit test coverage: exact match, CIDR, IPv6, trimming, invalid entries, multiple candidates, deduplication; tests set TrustedProxies(nil) which is the expected test-mode config. |
| frontend/src/features/requests/components/requests-columns.tsx | Block/unblock buttons added to client IP column; isBlocked check uses exact string match so CIDR-blocked IPs display incorrect UI state. |
| frontend/src/features/system/components/security-settings.tsx | New Security settings panel with TagsInput for blocked IPs; normalization and hasChanges logic are correct; save is gated on actual changes. |
| internal/server/gql/system.graphql | SecuritySettings type and UpdateSecuritySettingsInput added correctly; non-null list return type matches the always-initialized default. |
Sequence Diagram
sequenceDiagram
participant Client
participant IPBlocklist as WithIPBlocklist Middleware
participant DB as SystemService (DB)
participant Auth as Auth Middleware
participant Handler as API Handler
Client->>IPBlocklist: External API request
IPBlocklist->>IPBlocklist: Collect candidate IPs (ClientIP, X-Forwarded-For, X-Real-IP)
IPBlocklist->>DB: SecuritySettingsOrDefault() [DB read per request]
DB-->>IPBlocklist: BlockedIPs list
alt Any candidate IP is blocked
IPBlocklist-->>Client: 403 Forbidden
else Not blocked
IPBlocklist->>Auth: c.Next()
Auth->>Handler: Authenticated request
Handler-->>Client: API response
end
Comments Outside Diff (1)
-
frontend/src/features/requests/components/requests-columns.tsx, line 85 (link)CIDR-blocked IPs appear as unblocked in the UI
blockedIPs.includes(normalizedIP)does a plain string equality check, so an IP that is blocked because it falls inside a stored CIDR range (e.g.192.168.1.0/24) will show the "block" button instead of the "unblock" button in the request table. Clicking the block button then adds a redundant exact-IP entry alongside the existing CIDR rule. Resolving the discrepancy would require a client-side CIDR lookup or a server-sideisBlockedflag per record.
Reviews (1): Last reviewed commit: "feat: add IP blocklist settings" | Re-trigger Greptile
| // UpdateSecuritySettings is the resolver for the updateSecuritySettings field. | ||
| func (r *mutationResolver) UpdateSecuritySettings(ctx context.Context, input UpdateSecuritySettingsInput) (bool, error) { | ||
| current, err := r.systemService.SecuritySettings(ctx) | ||
| if err != nil { | ||
| return false, fmt.Errorf("failed to read current security settings: %w", err) | ||
| } | ||
|
|
||
| newSettings := biz.SecuritySettings{ | ||
| BlockedIPs: current.BlockedIPs, | ||
| } | ||
|
|
||
| if input.BlockedIPs != nil { | ||
| newSettings.BlockedIPs = input.BlockedIPs | ||
| } | ||
|
|
||
| err = r.systemService.SetSecuritySettings(ctx, newSettings) | ||
| if err != nil { | ||
| return false, fmt.Errorf("failed to update security settings: %w", err) | ||
| } | ||
|
|
||
| return true, nil | ||
| } |
There was a problem hiding this comment.
UpdateSecuritySettings and SecuritySettings have no scopes.UserHasScope guard, unlike TriggerGcCleanup (line 245) and PreviewGcCleanup (line 334) in the same file, which both explicitly check ScopeWriteSettings/ScopeReadSettings. Any authenticated admin JWT holder — regardless of their assigned scopes — can read and overwrite the IP blocklist. The frontend correctly gates the UI behind hasScope('write_settings'), but the backend does not enforce it, so the restriction is trivially bypassed by calling the GraphQL mutation directly.
| func isBlockedIP(clientIPs []string, blockedIPs []string) bool { | ||
| for _, clientIP := range clientIPs { | ||
| clientAddr, err := netip.ParseAddr(clientIP) | ||
| if err != nil { | ||
| log.Warn(context.Background(), "failed to parse client IP", log.String("client_ip", clientIP), log.Cause(err)) | ||
| continue | ||
| } |
There was a problem hiding this comment.
isBlockedIP and isBlockedAddr pass context.Background() to log.Warn, discarding any request-scoped tracing or correlation IDs. The request context should be threaded through from the gin handler so warning log lines about bad client IPs can be linked back to the originating request.
| func isBlockedIP(clientIPs []string, blockedIPs []string) bool { | |
| for _, clientIP := range clientIPs { | |
| clientAddr, err := netip.ParseAddr(clientIP) | |
| if err != nil { | |
| log.Warn(context.Background(), "failed to parse client IP", log.String("client_ip", clientIP), log.Cause(err)) | |
| continue | |
| } | |
| func isBlockedIP(ctx context.Context, clientIPs []string, blockedIPs []string) bool { | |
| for _, clientIP := range clientIPs { | |
| clientAddr, err := netip.ParseAddr(clientIP) | |
| if err != nil { | |
| log.Warn(ctx, "failed to parse client IP", log.String("client_ip", clientIP), log.Cause(err)) | |
| continue | |
| } |
| if xff := c.Request.Header.Get("X-Forwarded-For"); xff != "" { | ||
| before, _, _ := strings.Cut(xff, ",") | ||
| add(before) | ||
| } | ||
|
|
||
| add(c.Request.Header.Get("X-Real-IP")) |
There was a problem hiding this comment.
X-Forwarded-For and X-Real-IP are read from the raw request headers regardless of whether the request came from a trusted proxy. When the server is directly exposed or the reverse proxy does not strip client-supplied headers, a client can forge these headers with any IP value. Because isBlockedIP blocks a request when any candidate is in the blocklist, the false-positive direction is the concern: if the forged first-XFF entry happens to match a blocked IP, the legitimate client (whose actual IP is not blocked) receives a 403. The code should validate the headers are from a trusted proxy before including them in the candidate list.
| } | ||
|
|
||
| ctx := authz.WithSystemBypass(c.Request.Context(), "ip-blocklist-middleware") | ||
| settings := systemService.SecuritySettingsOrDefault(ctx) |
There was a problem hiding this comment.
SecuritySettingsOrDefault issues a synchronous database read for each call to WithIPBlocklist, and the middleware is applied to four separate route groups. For high-traffic deployments this adds a blocking DB round-trip to every incoming external API request. The IP blocklist is a rarely-changing setting; a short-lived in-memory cache (e.g. 10–30 seconds TTL) would eliminate the per-request overhead without meaningfully delaying blocklist updates.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
* fix(llm): close text block before starting thinking block in inbound stream When an upstream provider (observed with PackyCode proxy) returns a text content block before the thinking block (text → thinking order instead of the typical thinking → text), the inbound stream transformer failed to close the text block before opening the thinking block. This caused both blocks to share the same index, resulting in Claude Code reporting "Content block not found" errors. The fix adds a check for `hasTextContentStarted` in the reasoning content handler, mirroring the existing `hasToolContentStarted` check, ensuring the text block is properly closed (with citations flushed) before the thinking block begins. Closes looplj#1655 * feat: add IP blocklist settings
Summary
This PR adds system-wide IP blocklist settings for external API requests.
Admins can now configure blocked IP addresses and CIDR ranges from the System settings page. Requests from blocked IPs are rejected with
403 Forbiddenbefore normal external API processing begins.It also adds request-table shortcuts to block or unblock a client IP directly from request records.
Changes
Backend
Add
SecuritySettingssystem settings.security_settingsblocked_ipsAdd GraphQL support for security settings.
securitySettingsupdateSecuritySettings(input: UpdateSecuritySettingsInput!)Add IP blocklist middleware.
403 Forbidden.Apply the middleware to external API routes
//openapi/v1betaEnforce IP blocklist before API authenticat
Client IP handling
The middleware checks multiple IP sources:
c.ClientIP()X-Forwarded-ForX-Real-IPThis helps keep the backend enforcement behav in request records, especially when AxonHub is
deployed behind a reverse proxy.
Frontend
Tests
X-Forwarded-ForandX-Real-IPExpected behavior