Disallow cross-origin requests for webhooks#65928
Disallow cross-origin requests for webhooks#65928esev wants to merge 2 commits intohome-assistant:devfrom
Conversation
probot-home-assistant
bot
added
core
has-tests
integration: webhook
new-feature
small-pr
|
Why does the WebhookView set It doesn't matter for this PR, as a normal |
| _LOGGER.warning("Received remote request for local webhook %s", webhook_id) | ||
| return Response(status=HTTPStatus.OK) | ||
|
|
||
| # The presence of NON_LOCAL_HEADERS could indicate a CSRF request from a |
There was a problem hiding this comment.
I think that this misses the use case in which a user hosts a local website to provide guests with a basic webhook powered UI.
There was a problem hiding this comment.
(cross-origin is required for that too)
There was a problem hiding this comment.
Nice catch! WDYT about checking the referer/origin URLs against the cors_allowed_origins configuration option from the http component? If the URLs match, then consider the request as allowed for a local_only webhook. That would permit this use case without allowing all of the internet to access the webhook. I think that effectively makes local_only webhooks behave as-if cors_allowed = False.
There was a problem hiding this comment.
Yeah I can't remember why CORS was allowed to True, but I agree that since it can opt-in origins via the http config, it's fine to disable it.
There was a problem hiding this comment.
Sounds good. Thank you. I've done that, and also added a referer header check for HEAD/POST requests. I also updated the description to consider this PR a breaking change, just in the off chance that someone was relying on this behavior.
Can you add the breaking change label to this PR so that it'll show-up in the release notes?
esev
changed the title
| _LOGGER.debug("%s", content) | ||
| return Response(status=HTTPStatus.OK) | ||
|
|
||
| # POST & HEAD are considered simple requests. They do not trigger a CORS preflight |
There was a problem hiding this comment.
Meh, I forgot about the simple requests. I don't feel like rolling our own CROS is a good way to go.
Do we really need it given the frontend change enforcing that we push for long random strings.
There was a problem hiding this comment.
I actually feel this PR isn't needed if everyone used long random strings from the start. But there are tutorials online that recommend short names, and those names are likely going to be the same for everyone that followed the tutorial.
If you have a few moments, could you review the comments in #56446? That's how I got started down this path. GET is somewhat more dangerous because it's trivial to avoid sending the referer header too. IMHO, PUT (with CORS disabled) is the safest method for webhooks, but I definitely see the utility in allowing more methods.
My day job focuses on keeping users safe online. So it's easy for me to fall into the trap of preferring security over usability. If you could review the comments in #56446, and provide a bit of direction, that would be helpful for me. I'm happy to make the necessary changes to keep users safe. But I'd also rather not go down a path that makes things harder for users or that tries to fix a problem that isn't very wide-spread.
You have a much better perspective on this. I appreciate any feedback you can provide.
There was a problem hiding this comment.
I suggest we close this PR and instead go for the method picker that you showed a screenshot off. Although I would even move that into the overflow menu so that we don't have to confuse the user always with it. (and GET disabled by default)
And for local network, that too can be put in overflow menu but we limit to local network by default.
There was a problem hiding this comment.
Closing this. Thank you for taking a look and the feedback. I appreciate it!
I'll work on polishing-up the method/local options and try to have something ready early next week.
3 participants
Breaking change
Webhooks have been adjusted to not allow access from web browsers by default. Trusted web pages can still be allowed by adjusting the cors_allowed_origins configuration for the http integration. If you see a log message similar to
Received likely CSRF request from non-trusted origin for webhook, then add the origin to the cors_allowed_origins configuration.Proposed change
Webhooks can be triggered by devices and services outside of Home Assistant. But they can also be triggered by cross-origin requests in the browser. A web browser could accidentally allow a non-local website to activate a webhook.
This PR aims to make it harder for a non-local website to activate/trigger a webhook via a browser. It does so by disabling the
cors_allowedsetting, preventing a non-trusted website from sending non-simple requests. HEAD & POST methods are considered simple requests and do not trigger a CORS preflight check. This PR adds a "Referer" header check to verify these requests come from a trusted site.Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: