-
Notifications
You must be signed in to change notification settings - Fork 218
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
Abuse Protection for webhooks #491
Conversation
looks like you pulled in the changes to the website 😄 |
I bet I just have to rebase |
cb32692
to
b3a2bcf
Compare
AutoACKCallback: true, | ||
AllowedOrigins: []string{"http://localhost:8181"}, | ||
} | ||
p.OptionsHandlerFn = p.OptionsHandler |
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.
?
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.
Can this be a default?
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.
yeah a future PR can add this to the default client to respond to OPTIONS
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.
I think it should be opt-in, because you need to know the rate limit and such
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.
plus the default allowed origin would have to be "*" which seems like a bad default
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.
Do you think it should be an opt-in or an opt-out feature? I don't think this interfere with the average usage of sdk-go
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.
because OPTIONS is optional by the spec, it should be OPT-IN so that integrators have to make choices about rate limits and origin filters up front.
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.
Okok i feel we should document it and make a "better entrypoint", like an option for the protocol EnableWebHookSupport()
or something like that that does this assignment
Signed-off-by: Scott Nichols <snichols@vmware.com>
Signed-off-by: Scott Nichols <snichols@vmware.com>
Signed-off-by: Scott Nichols <snichols@vmware.com>
Signed-off-by: Scott Nichols <snichols@vmware.com>
Signed-off-by: Scott Nichols <snichols@vmware.com>
6e7ea65
to
18e0cf8
Compare
Signed-off-by: Scott Nichols <snichols@vmware.com>
Signed-off-by: Scott Nichols <snichols@vmware.com>
Signed-off-by: Scott Nichols <snichols@vmware.com>
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, can you open issues for "followup" comments?
} | ||
// TODO: it is not clear what the rules for allowed hosts are. | ||
// Need to find docs for this. For now, test for prefix. | ||
if strings.HasPrefix(ro, ao) { |
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.
Maybe we could use this https://github.com/rs/cors/blob/master/cors.go or something similar to validate the origins, let's do it in a follow up.
} | ||
}, | ||
}, | ||
"405 if the receiver is not expecting a GET request": { |
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.
Maybe in a followup, but we need a test for the use case:
I want to set a receiver that doesn't accept events, accepts method
GET
, and the user replies with events
Issues added. |
No description provided.