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

Abuse Protection for webhooks #491

Merged
merged 8 commits into from
May 19, 2020
Merged

Conversation

n3wscott
Copy link
Member

@n3wscott n3wscott commented May 4, 2020

No description provided.

@slinkydeveloper
Copy link
Member

looks like you pulled in the changes to the website 😄

@n3wscott
Copy link
Member Author

n3wscott commented May 6, 2020

I bet I just have to rebase

@n3wscott n3wscott marked this pull request as ready for review May 8, 2020 15:04
@slinkydeveloper slinkydeveloper self-requested a review May 8, 2020 15:09
v2/cmd/samples/http/raw/main.go Outdated Show resolved Hide resolved
AutoACKCallback: true,
AllowedOrigins: []string{"http://localhost:8181"},
}
p.OptionsHandlerFn = p.OptionsHandler
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member Author

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

Copy link
Member Author

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

Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

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

v2/cmd/samples/http/sender/main.go Outdated Show resolved Hide resolved
v2/protocol/http/protocol.go Show resolved Hide resolved
Scott Nichols added 5 commits May 11, 2020 14:45
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>
Signed-off-by: Scott Nichols <snichols@vmware.com>
@n3wscott n3wscott changed the title WIP of Abuse Protection for webhooks Abuse Protection for webhooks May 11, 2020
Scott Nichols added 2 commits May 19, 2020 09:16
Signed-off-by: Scott Nichols <snichols@vmware.com>
Signed-off-by: Scott Nichols <snichols@vmware.com>
Copy link
Member

@slinkydeveloper slinkydeveloper left a 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) {
Copy link
Member

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": {
Copy link
Member

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

@n3wscott
Copy link
Member Author

Issues added.

@n3wscott n3wscott merged commit 97abfeb into cloudevents:master May 19, 2020
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.

None yet

2 participants