-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: reduce default max payload size in webhooks to 50MB #21101
chore: reduce default max payload size in webhooks to 50MB #21101
Conversation
Signed-off-by: pashakostohrys <pavel@codefresh.io>
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pasha-codefresh. Suggesting few additional changes.
Doc update:
argo-cd/docs/operator-manual/argocd-cm.yaml
Lines 434 to 435 in 96d0226
# The maximum size of the payload that can be sent to the webhook server. webhook.maxPayloadSizeMB: "1024" argo-cd/docs/operator-manual/webhook.md
Line 22 in 96d0226
To prevent DDoS attacks with unauthenticated webhook events (the `/api/webhook` endpoint currently lacks rate limiting protection), it is recommended to limit the payload size. You can achieve this by configuring the `argocd-cm` ConfigMap with the `webhook.maxPayloadSizeMB` attribute. The default value is 1GB.
and a nit for consistency
argo-cd/util/webhook/webhook_test.go
Line 63 in 96d0226
defaultMaxPayloadSize := int64(1) * 1024 * 1024 * 1024
Oh, thank you! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #21101 +/- ##
=========================================
Coverage ? 53.47%
=========================================
Files ? 324
Lines ? 55581
Branches ? 0
=========================================
Hits ? 29721
Misses ? 23255
Partials ? 2605 ☔ View full report in Codecov by Sentry. |
Signed-off-by: pashakostohrys <pavel@codefresh.io>
@svghadi fixed |
Signed-off-by: pashakostohrys <pavel@codefresh.io>
Signed-off-by: pashakostohrys <pavel@codefresh.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
…21101) * chore: reduce default max payload size in webhooks to 50MB Signed-off-by: pashakostohrys <pavel@codefresh.io> * chore: reduce default max payload size in webhooks to 50MB Signed-off-by: pashakostohrys <pavel@codefresh.io> * chore: reduce default max payload size in webhooks to 50MB Signed-off-by: pashakostohrys <pavel@codefresh.io> * chore: reduce default max payload size in webhooks to 50MB Signed-off-by: pashakostohrys <pavel@codefresh.io> --------- Signed-off-by: pashakostohrys <pavel@codefresh.io>
During the implementation of GHSA-jmvp-698c-4x3w, we initially set the payload size limit to 1GB by default. However, after further discussion with the SIG-Security group, we decided to reduce this limit to 50MB, as 1GB was still considered too large for a payload size.