fix(gateway/webhook): return 404 for unregistered source instead of blocking on nil channel (#6999) - #7161
Open
kotwal-itpro wants to merge 1 commit into
Conversation
…locking on nil channel (rudderlabs#6999) RequestHandler used to fetch the per-source request channel from webhook.requestQ and immediately send on it. When the source type had not been registered — a condition that can occur when webhookV2 is disabled and the lazy registration in processBackendConfig has not yet run for a given source, or when a request arrives for a source type this instance does not handle — the map lookup returns a nil channel and the send blocks forever. The HTTP handler goroutine never returns, the client hangs, and a goroutine is leaked per unregistered request. That is a straightforward DoS. Comma-ok the map lookup, release the read lock before writing the response, and fail fast with the existing response.InvalidWebhookSource / 404 mapping. Adds a regression test that omits Register() and asserts the handler returns StatusNotFound within 2s (rather than blocking indefinitely). Runs cleanly under -race. Fixes rudderlabs#6999
|
Thank you @kotwal-itpro 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. |
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
1 participant
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.
Fixes #6999.
Problem
RequestHandlerlooked up the per-source request channel fromwebhook.requestQand immediately sent on it. When the source type had not beenRegister()ed — a condition that can occur whenwebhookV2is disabled and the lazy registration inprocessBackendConfighas not yet run for a given source, or when a request arrives for a source type this instance does not handle — the map lookup returned a nil channel and the send blocked forever. The HTTP handler goroutine never returned, the client hung, and a goroutine leaked per unregistered request. This is the DoS described in #6999.Fix
gateway/webhook/webhook.go:228.response.InvalidWebhookSource→ HTTP 404 mapping (already defined ingateway/response).Test
Adds
TestWebhookRequestHandlerReturns404WhenSourceNotRegisteredwhich:webhookV2HandlerEnabled(the pre-condition under which the bug reproduces on current HEAD).Register().time.Afterso the test fails hard (rather than hanging) if the handler ever regresses back to blocking on nil channel.http.StatusNotFoundand body containsresponse.InvalidWebhookSource.Passes under
go test -race ./gateway/webhook/.... Full webhook test suite still passes.