fix(gateway/webhook): prevent nil-channel send DoS in RequestHandler#7069
Open
kuntal1461 wants to merge 1 commit into
Open
fix(gateway/webhook): prevent nil-channel send DoS in RequestHandler#7069kuntal1461 wants to merge 1 commit into
kuntal1461 wants to merge 1 commit into
Conversation
When webhookV2HandlerEnabled is false and a source has not been registered, requestQ lookup returns nil. Sending on a nil channel blocks forever, permanently consuming the HTTP worker goroutine. - Guard the channel send with a nil check; return HTTP 503 immediately for unregistered sources instead of blocking - Wrap the channel send in a select with r.Context().Done() and the shutdown context (webhook.ctx) so the handler exits promptly when the client disconnects or the server shuts down - Buffer the done channel (size 1) so the batcher goroutine can always complete its send even if RequestHandler returned early, preventing goroutine leaks - Also guard resp := <-done with r.Context().Done() for the same reason - Store the background context in HandleT.ctx (set in Setup) to enable the shutdown-aware select in RequestHandler Fixes rudderlabs#6999 Authored-By: Kuntal Maity <kuntal.1461@gmail.com>
|
Thank you @kuntal1461 for contributing this PR. |
Contributor
|
This PR is considered to be stale. It has been open 20 days with no further activity thus it is going to be closed in 7 days. To avoid such a case please consider removing the stale label manually or add a comment to the PR. |
Author
|
hi |
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
Fixes #6999 — sending on a nil channel in
RequestHandlerblocks the HTTP worker goroutine forever, enabling a denial-of-service condition.Root cause:
webhook.requestQ[sourceDefName]returnsnilwhen the source has not been registered (e.g.webhookV2HandlerEnabled=falseand no priorRegistercall). Go's nil channel send blocks indefinitely.Changes:
requestQ == nilafter the map lookup and return HTTP 503service unavailableimmediately instead of blocking.selectwithr.Context().Done()(client disconnect / deadline) andwebhook.ctx.Done()(server shutdown). The RLock is held across the select to prevent a concurrentShutdownfrom closing the channel mid-send.donechannel (size 1) — ifRequestHandlerexits early due to context cancellation, the batcher goroutine can still complete its send without leaking.resp := <-done— also wraps the response-wait in aselectwithr.Context().Done()so a slow transformer doesn't permanently hold a worker after the client has gone.HandleT.ctxadded and populated inSetupsoRequestHandlercan reference it.Test plan
TestWebhookRequestHandlerSourceNotRegistered— V1 mode, noRegistercall → asserts HTTP 503 returned immediately, no blockTestWebhookRequestHandlerRequestContextCancellation— blocking adapter + 100 ms request deadline → asserts handler returns promptlygo test ./gateway/webhook/... -race -count=1)webhookV2HandlerEnabled=falseand send a request for an unregistered source — expect immediate 503