Implement wings security fixes #156
Conversation
📝 WalkthroughWalkthroughThis PR introduces a deauthorize-user endpoint that revokes all connections and tokens for specified users across selected or all managed servers. It also implements websocket event rate limiting with throttling signals, server suspension handling with connection cancellation, per-server-per-user token denial, SFTP connection lifecycle management, and a new ContextBag utility for managing per-key cancelable contexts. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Router as Router<br/>/api/deauthorize-user
participant Server as Server
participant WebSocket as WebSocket Manager
participant Tokens as Token Denier
participant SFTP as SFTP Manager
Client->>Router: POST deauthorize-user<br/>{user, servers?}
rect rgb(200, 220, 255)
note over Router,SFTP: For each targeted server
Router->>Server: server.Get(id)
Router->>WebSocket: server.Websockets().CancelAll()
WebSocket->>WebSocket: Send close signal to all<br/>connected clients
Router->>SFTP: server.Sftp().CancelAll()
SFTP->>SFTP: Cancel all per-user<br/>contexts
Router->>Tokens: tokens.DenyForServer(server, user)
Tokens->>Tokens: Record denial timestamp<br/>in userDenylist
end
Router->>Client: HTTP 204 No Content
rect rgb(240, 255, 240)
note over Client,Router: Subsequent WS/SFTP attempts
Client->>Router: WebSocket handshake or<br/>SFTP connection
Router->>Tokens: Denylisted() check
Tokens->>Router: true (user denied for server)
Router->>Client: Connection rejected
end
sequenceDiagram
actor Client
participant Handler as WebSocket<br/>Handler
participant Limiter as Rate Limiter
participant Queue as Message Queue
Client->>Handler: Send rapid messages
loop For each message
Handler->>Limiter: IsThrottled(event)
alt Event allowed
Limiter->>Limiter: Clear throttling flag
Limiter->>Handler: return false
Handler->>Queue: Process message
else Event throttled
Limiter->>Limiter: Mark as throttled
Limiter->>Handler: return true
rect rgb(255, 240, 240)
Handler->>Handler: Send ThrottledEvent signal
Handler->>Client: {event: "throttled", args: [...]}
end
Handler->>Handler: Drop message (log)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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 |
![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
Fix all issues with AI Agents 🤖
In @router/router_system.go:
- Around line 281-286: When deauthorizing from all servers the code currently
cancels websockets and SFTP but omits revoking JWTs; add a call to
tokens.DenyForServer for each server in the m.All() loop so tokens are denied
just like in the per-server path. Specifically, inside the else branch iterating
over m.All(), after or before s.Websockets().CancelAll() and
s.Sftp().Cancel(data.User), invoke tokens.DenyForServer(ctx, s.Id(), data.User)
(matching the signature used in the per-server branch) and handle/log any
returned error consistently with the existing per-server error handling.
- Around line 262-269: The handler currently binds JSON into the local struct
via c.BindJSON(&data) but does not validate data.User; add a check after
BindJSON to return a 400 Bad Request when data.User == "" (use the Gin context
like c.JSON or c.AbortWithStatusJSON with an error message) to stop processing
malformed requests before calling the connection cancel/deny-token logic that
relies on data.User.
In @router/websocket/limiter.go:
- Around line 50-67: The For method uses a double-checked locking pattern that
can race between RUnlock() and Lock(); after acquiring the write lock in For,
re-check l.limits[name] before creating and assigning a new limiter to avoid
overwriting a limiter another goroutine may have created. Concretely, inside For
after l.mu.Lock() (and before creating rate.NewLimiter), check if v, ok :=
l.limits[name]; if ok return v, otherwise create, assign l.limits[name] =
rate.NewLimiter(limit, burst) and return it; keep the existing RLock/Unlock and
defer l.mu.Unlock() usage.
- Around line 16-38: IsThrottled holds h.limiter.mu while calling
h.unsafeSendJson which acquires the Handler RWMutex and can deadlock; fix by
updating h.limiter.throttles inside the locked section but record locally (e.g.
needSend := true) whether to send the ThrottledEvent, then unlock (remove defer
lock/unlock pattern) and only call h.unsafeSendJson when not holding
h.limiter.mu; keep calls to l.Allow() and setting throttles[e]=true/false inside
the limiter lock so state remains consistent.
In @server/server.go:
- Around line 203-210: The comment about websocket and SFTP disconnection logic
is misplaced inside parseInvocation; remove or move that block to the
appropriate suspension/Sync() area. Locate the parseInvocation function and
delete the irrelevant comment lines referencing websocket/SFTP/JWT disconnects
(around the code that replaces ${SERVER_PORT}, ${SERVER_MEMORY}, ${SERVER_IP} on
the invocation string), or relocate that comment to the Sync() suspension
handler where websocket/SFTP disconnect logic lives to avoid confusion.
In @sftp/server.go:
- Around line 170-176: The goroutine waiting on ctx.Done() can leak if
rs.Serve() returns first; create a done channel (e.g., done := make(chan
struct{})) and have the goroutine select on both <-ctx.Done() and <-done so it
exits on normal session completion; after rs.Serve() returns, close(done) (or
send a value) to signal normal termination, keeping the existing log call to
srv.Log().WithField("user", conn.User()).Warn("sftp: terminating active
session") and still calling _ = rs.Close() when ctx.Done() triggers.
🧹 Nitpick comments (5)
server/connections.go (1)
10-19: Consider using RLock for read path optimization.The method uses a write lock for lazy initialization, which is correct but could be optimized since subsequent calls only need read access after initialization.
🔎 Optional optimization using double-checked locking
func (s *Server) Sftp() *system.ContextBag { - s.Lock() - defer s.Unlock() + s.RLock() + if s.sftpBag != nil { + defer s.RUnlock() + return s.sftpBag + } + s.RUnlock() + s.Lock() + defer s.Unlock() + // Double-check after acquiring write lock if s.sftpBag == nil { s.sftpBag = system.NewContextBag(s.Context()) } return s.sftpBag }This optimization reduces lock contention for read operations after initialization, though the current implementation is correct.
router/router_server_ws.go (1)
71-82: Minor: Redundantbreakstatement.The
breakon line 80 is unnecessary since there's only one case in the select. While not a bug, it can be removed for cleaner code.🔎 Proposed fix
go func() { select { // When the main context is canceled (through disconnect, server deletion, or server // suspension) close the connection itself. case <-ctx.Done(): handler.Logger().Debug("closing connection to server websocket") if err := handler.Connection.Close(); err != nil { handler.Logger().WithError(err).Error("failed to close websocket connection") } - break } }()system/context_bag.go (1)
19-21: Consider: Nil context handling.If
nilis passed toNewContextBag, callingContext()will panic whencontext.WithCancel(nil)is invoked. Consider either documenting this requirement or adding a nil check.🔎 Proposed fix (option: add nil check)
func NewContextBag(ctx context.Context) *ContextBag { + if ctx == nil { + ctx = context.Background() + } return &ContextBag{ctx: ctx, items: make(map[string]ctxHolder)} }router/websocket/websocket.go (1)
285-305: Clarify the rationale for the duplicate suspension check.There are two
IsSuspended()checks: one at lines 285-287 (before authentication validation) and another at lines 303-305 (after authentication). While this may be intentional to handle a race where suspension occurs during auth processing, consider adding a brief comment explaining why both checks are necessary. This helps future maintainers understand the security rationale.The throttling check at lines 289-291 correctly returns
nilto silently drop throttled events without surfacing an error.sftp/server.go (1)
173-173: Minor: Consider usinghandler.User()for consistency.The log message uses
conn.User()which returns the SSH username, whilehandler.User()returns the user UUID extracted from permissions. Usinghandler.User()would be consistent with the context key used at line 167.🔎 Proposed fix
- srv.Log().WithField("user", conn.User()).Warn("sftp: terminating active session") + srv.Log().WithField("user", handler.User()).Warn("sftp: terminating active session")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
router/router.gorouter/router_server.gorouter/router_server_ws.gorouter/router_system.gorouter/tokens/websocket.gorouter/websocket/limiter.gorouter/websocket/listeners.gorouter/websocket/message.gorouter/websocket/websocket.goserver/connections.goserver/server.goserver/websockets.gosftp/handler.gosftp/server.gosystem/context_bag.go
🧰 Additional context used
🧬 Code graph analysis (6)
server/connections.go (2)
server/server.go (1)
Server(30-82)system/context_bag.go (2)
ContextBag(13-17)NewContextBag(19-21)
router/websocket/listeners.go (3)
router/websocket/message.go (2)
Message(19-26)Event(3-3)events/events.go (1)
Event(13-16)internal/models/activity.go (1)
Event(11-11)
sftp/server.go (3)
config/config.go (1)
Get(429-437)server/server.go (1)
Server(30-82)sftp/handler.go (1)
NewHandler(40-60)
router/router_server_ws.go (3)
router/websocket/limiter.go (1)
NewLimiter(40-45)router/websocket/message.go (3)
Message(19-26)Event(3-3)ThrottledEvent(16-16)server/errors.go (1)
ErrSuspended(9-9)
router/websocket/limiter.go (2)
router/websocket/message.go (6)
Event(3-3)Message(19-26)ThrottledEvent(16-16)AuthenticationEvent(9-9)SendServerLogsEvent(11-11)SendCommandEvent(12-12)router/websocket/websocket.go (1)
Handler(41-49)
server/server.go (1)
system/context_bag.go (1)
ContextBag(13-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Test (ubuntu-22.04, 1.25.1, linux, amd64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.24.7, linux, amd64)
- GitHub Check: Analyze (go)
🔇 Additional comments (27)
server/websockets.go (1)
28-33: LGTM!The
Len()method is correctly implemented with proper mutex locking, consistent with other methods inWebsocketBag. The thread-safe accessor provides a clean way to check connection count.router/router_server.go (1)
297-298: LGTM!The deprecation notice correctly points users to the new
/api/deauthorize-userendpoint, facilitating migration to the improved API.router/router.go (1)
69-69: Verify authorization checks in the deauthorizeUser handler.The handler is properly defined in
router/router_system.goat line 261. However, the implementation lacks explicit authorization validation. The handler does not verify whether the authenticated user has permission to deauthorize the specified user or to target specific servers. Currently, any request that passes theprotectedmiddleware can deauthorize any user across any server without additional permission checks. Ensure the handler validates that the authenticated user is authorized to perform this action (e.g., system admin verification or server ownership check).server/connections.go (1)
10-19: ContextBag implementation is properly thread-safe.The
system.ContextBagis defined insystem/context_bag.goand correctly implements thread-safe, per-key context management usingsync.Mutex. TheNewContextBag()factory function and associated methods are available in the codebase. TheSftp()method's usage of lazy initialization paired with the Server's mutex is correct.router/websocket/message.go (1)
3-3: Type safety improvement for websocket events.The introduction of the
Eventtype alias strengthens type safety by preventing arbitrary strings from being used as event identifiers. The addition ofThrottledEventaligns with the rate limiting features mentioned in the PR objectives. All Message construction sites in the codebase correctly use the Event type, either through predefined constants or explicit type casting.router/tokens/websocket.go (2)
27-48: LGTM! Per-server-per-user token denial mechanism.The new
DenyForServerfunction correctly implements per-server-per-user token denial using a composite key. The thread-safesync.Mapis appropriate here.Consider: Memory growth over time. The
userDenylistentries are never cleaned up. For long-running instances with many deauthorization events, this could lead to unbounded memory growth. A periodic cleanup of entries older than the maximum JWT lifetime would prevent this.
104-108: LGTM!The
userDenylistcheck correctly uses the same composite key format asDenyForServerand properly compares token issuance time against the denial timestamp.router/router_server_ws.go (3)
31-47: LGTM! Connection limiting with appropriate feedback.The 30-connection limit per server is a reasonable safeguard against resource exhaustion. The TODO comment acknowledges the limitation of not being per-user, which is acceptable for now.
108-134: LGTM! Rate limiting with throttle notification.The rate limiter correctly allows a burst of 10 messages then enforces ~5 messages/second sustained rate. The throttle notification is sent once per throttle period, avoiding notification spam.
98-106: LGTM! Suspended server handling.Properly checks suspension status after connection upgrade and sends a descriptive close message with custom code 4409, allowing clients to handle this case appropriately.
system/context_bag.go (1)
27-57: LGTM! Thread-safe context management.The
ContextBagimplementation correctly handles:
- Lazy context creation with proper mutex protection
- Safe cancellation of individual or all contexts
- Proper cleanup by calling cancel functions before removing entries
server/server.go (2)
270-274: LGTM! Immediate connection termination on suspension.When the server is suspended, all websocket and SFTP connections are immediately canceled. This ensures users cannot continue interacting with a suspended server.
72-72: LGTM!The
sftpBagfield is correctly added to support per-user SFTP context management. Based on the AI summary, it's lazily initialized via aSftp()accessor method.sftp/handler.go (1)
315-318: LGTM!Simple accessor method exposing the user UUID for per-user context management in SFTP connection lifecycle. Aligns with the broader PR changes for user-specific connection handling.
router/websocket/listeners.go (1)
132-150: LGTM! Type-safe event handling.The changes correctly adapt to the
Eventtype introduced inmessage.go:
- Line 132: Converts
e.Topic(string) toEventtype for theMessagestruct- Line 150: Converts back to string for the error handler which expects a string parameter
router/websocket/websocket.go (5)
24-24: LGTM!Import addition for
internal/modelsis correctly placed to support the activity logging functionality.
48-48: LGTM!Adding the
limiterfield to theHandlerstruct properly integrates the per-connection rate limiting mechanism.
87-87: LGTM - Compression and read limits configured.Enabling compression with level 5 provides a good balance between CPU usage and bandwidth savings. The 4096-byte read limit is a reasonable security measure to prevent oversized message attacks.
Also applies to: 114-115
123-123: LGTM!Initializing the limiter via
NewLimiter()ensures each handler has its own rate limiting bucket.
158-158: LGTM - Type conversion for prefix check.Casting
v.Eventtostringbefore callingHasPrefixaligns with the newEventtype alias.sftp/server.go (3)
129-134: LGTM!Ignoring the error from
ch.Rejectis acceptable since the connection is being rejected anyway and there's no meaningful recovery action.
146-146: LGTM!Ignoring the
req.Replyerror is appropriate; logging or handling errors here would not change the outcome.
150-157: LGTM - Clean extraction to Handle method.Delegating SFTP session handling to
c.Handleimproves code organization and enables per-user context management.router/websocket/limiter.go (4)
10-14: LGTM!The
LimiterBucketstruct is well-designed with appropriate synchronization primitives and maps for tracking rate limiters and throttle state per event type.
40-45: LGTM!The
NewLimiter()function correctly initializes the maps with reasonable initial capacity.
69-83: LGTM - Rate limits are reasonable for security.The rate limits are well-chosen:
- Authentication/logs: 2 per 5 seconds (prevents brute force)
- Commands: 10 per second (allows reasonable interaction)
- Default: 4 per second (balanced protection)
85-91: LGTM!The
limiterNamefunction correctly groups events to share rate limiters where appropriate while isolating specific high-risk events.
1 participant
Implements the following security fixes.
pterodactyl/wings#287
pterodactyl/wings#288
pterodactyl/wings#289
pterodactyl/wings#290
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.