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

Update webhook handler example to use http.MaxBytesReader #914

Merged
merged 1 commit into from
Aug 12, 2019

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Aug 12, 2019

Updates the webhook handler example to use http.MaxBytesReader to
protect against a malicious client streaming an endless request body.

We're making a similar change in our server side documentation examples,
so I'm updating this spot as well for consistency.

r? @rattrayalex-stripe

Updates the webhook handler example to use `http.MaxBytesReader` to
protect against a malicious client streaming an endless request body.

We're making a similar change in our server side documentation examples,
so I'm updating this spot as well for consistency.
Copy link

@rattrayalex-stripe rattrayalex-stripe left a comment

Choose a reason for hiding this comment

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

Still not the biggest fan (verbose, distracting, low likelihood of causing problems, 64k could possibly be too low and be a footgun) but +1 on consistency and I defer to you on all of these questions anyway.

@brandur-stripe
Copy link
Contributor

Still not the biggest fan (verbose, distracting, low likelihood of causing problems, 64k could possibly be too low and be a footgun) but +1 on consistency and I defer to you on all of these questions anyway.

Yeah, definitely fair enough. I thought about adding another if conditional as well where we check whether the number of bytes read equals max size and throws a specialized error in that case, because the trouble is that if it was, then the current error will be some JSON decoding thing like missing } or something terribly confusing like that. But of course that adds even more code to the sample which makes it even harder to parse.

Anyway, going to merge this for now to keep it consistent with the other docs, but definitely up for revisiting if we find it was the wrong decision longer term.

Thanks!

@brandur-stripe brandur-stripe merged commit b7fac25 into master Aug 12, 2019
@remi-stripe remi-stripe deleted the brandur-update-webhook-example branch April 29, 2020 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants