Skip to content
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: discontinue using ioutil #1966

Merged
merged 1 commit into from May 13, 2022
Merged

chore: discontinue using ioutil #1966

merged 1 commit into from May 13, 2022

Conversation

whynowy
Copy link
Member

@whynowy whynowy commented May 13, 2022

Signed-off-by: Derek Wang whynowy@gmail.com

  1. Stop using ioutil;
  2. use limit reader to prevent large content caused crash.

Fixes: #1946

Checklist:

Signed-off-by: Derek Wang <whynowy@gmail.com>
Copy link
Member

@sarabala1979 sarabala1979 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@whynowy whynowy merged commit 143f8be into argoproj:master May 13, 2022
7 checks passed
@whynowy whynowy deleted the looog branch May 13, 2022 15:29
whynowy added a commit that referenced this pull request Jun 9, 2022
Signed-off-by: Derek Wang <whynowy@gmail.com>
@@ -86,8 +86,8 @@ func (router *Router) HandleRoute(writer http.ResponseWriter, request *http.Requ
return
}
}

body, err := ioutil.ReadAll(request.Body)
request.Body = http.MaxBytesReader(writer, request.Body, 65536)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@whynowy sorry for the delay, I am just now seeing this - Does that mean we are no longer supporting gitlab payloads above 65KB? (same question goes for bitbucket, bitbucket server and webhook eventsources)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@whynowy and btw do we have a different behavior atm in github? I wasn't able to find the same change in github eventsource

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daniel-codefresh - 65536 is a very arbitrary number I put in order to close the security issue - I thought as a webhook notification it might not be over 65K (obviously I could be wrong). Do you have a recommended number since you are more familiar with them?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my experience, webhook payloads can grow up to 1MB and even more in some cases, especially with monorepos. I think the best solution would be to allow the users configure the max payload size by themselves (and set some default value in case they don't). I suggest to add an optional MaxPayloadSize field under WebhookContext which is used by all of the git/webhook related eventsources, wdyt @whynowy ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good idea @daniel-codefresh .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created a new issue on the subject and assigned to myself: #2088

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 Uses of deprecated API can be used to cause DoS in user-facing endpoints
3 participants