feat(webhook): add request/response size limits #4837
Conversation
via new config property webhook.maxRequestBytes (default 0). When greater than 0, only requests whose header + body size is less than or equal to maxRequestBytes are allowed.
via new config property webhook.maxResponseBytes (default 0). When greater than 0, only responses whose header + body size is less than or equal to maxResponseBytes are allowed.
…onse size instead of cancelling the call. The resulting exceptions from canceling were different locally and in CI, and I suspect throwing our exceptions makes it easier to provide better error messages to the user.
| try { | ||
| contentLength = requestBody.contentLength(); | ||
| if (contentLength == -1) { | ||
| // Given that OkHttp3ClientHttpRequestFactory.buildRequest (called by |
There was a problem hiding this comment.
I believe this is possible. I think this occurs for streaming, e.g. trying to send a request with a massive file returns content length == -1. See chunked encoding.
There was a problem hiding this comment.
Now, if webhooks can't do chunked encoding, then we can assume -1 wont be the value.
There was a problem hiding this comment.
I'll admit I haven't tried that hard to send a streamed request via a webhook stage. I don't think it's possible though since the request is defined in the pipeline config. Of course with SpEL expressions, etc. it can be dynamic and huge, but I still think it ends up being a String with a known length by the time we get here.
| long headerSize = request.headers().byteCount(); | ||
|
|
||
| // If the headers are already too big, no need to deal with the body size | ||
| if (headerSize > maxRequestBytes) { |
There was a problem hiding this comment.
I do like max payload size, but having a limitation on headers seems like that would already be handled by MOST http servers. Albeit they all default to some non-consistent value.
Is this more from a security stand point?
There was a problem hiding this comment.
Yes, security, It's more from a quality of service / denial of service standpoint. Like, we didn't want pipeline authors to be able to generate massive requests and bog down Spinnaker.
via new config properties webhook.maxRequestBytes and webhook.maxResponseBytes. The default value for both is 0.
When greater than 0, only requests/responses whose header + body size is less than or equal to the max are allowed.