Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MSC2970: Remove pusher path requirement #2970
base: old_master
Are you sure you want to change the base?
MSC2970: Remove pusher path requirement #2970
Changes from all commits
c5f8479
0c051a2
0ac32dc
96bee63
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
8000 was picked fairly randomly based on https://tools.ietf.org/html/rfc7230#section-3.1.1.
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.
Thinking about this, I think it would be quite a beneficial addition. However, it would be a backwards-incompatible change to the pusher API (currently, there is no way to do a "test notification" other than by making up an event of some form), and generally needs a bit more time to write into an MSC than I have time for right now.
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 was going to suggest something like this, though I thought about requiring either that the pusher URL ends with
/_matrix/push/v1/notify
or that it responds toGET
requests with a predetermined response that proves it's expecting pushes.Agreed this should be out-of-scope for this MSC.
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.
As discussed above this could be done pretty easily by adding an
httpv2
pusher type? I guess this would require clients to know the types that a server supports though, which they might not currently.Also -- would this be compatible with the proposed projects above (matrix-gotify, etc.)?
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.
@dkasak Another way to verify the pusher URL is how JMAP does it like a phone number verification code.
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.
In addition to @clokep's comment (here) there's an added concern of being able to register pushers which point at random web services. This not only can point at internal infrastructure, but also a random webserver out in the wild. A handshake process may be in order.
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.
Is the same as @clokep's comment, or somehow different?
@clokep wrote:
@turt2live wrote
in any case, IMHO: this is no different from any other service that provides webhooks: there is no requirement on the target path for a Github event notifications, for example.
Pivoting into internal infrastructure should be handled with IP blacklists etc (an effective IP blacklist is already required for URL previews.)
Other services that provide webhooks don't require a handshake, and I'm unconvinced that we need one. It sounds like an an annoyance for people to set up.
Concerns over traffic amplification should be handled with rate-limiting (and making sure that pushers that don't get the correct response get disabled), which is orthogonal to the URL path.
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.
My intention from that blurb is pretty much the same as what @turt2live wrote.
Yes, that's true. We should at least document that this does have security implications to it.
(P.S. Sorry for forgetting to use a thread!)
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 agree. I think the security implications of sending less restricted HTTP requests from the homeserver are important to note but worth the minor risk.
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 hope I've now described the security considerations in question.
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.
Seems reasonable to me! 👍