Webhook configurability - #1445
Conversation
* blocked_networks - ranges of IPs to which webhooks must not deliver * deliver_exception_hosts - host names to which webhooks must deliver
* TILED_WEBHOOKS_ALLOW_DELIVERY_HOSTS is a more clear name than the previous one
* rename the current defn as "standard" because they are not site- specific * combine the user-defined and standard networks
* even if a host is within a prohibited range of hosts, it must be able to receive messages from us
|
closes #1380 |
|
For logging config, consider updating this too. tiled/tiled/server/logging_config.py Lines 70 to 77 in 1853c2a |
* local_blocked_networks should be network ranges * allow_delivery_hosts should be hostnames of valid hosts * fixes for handling these types
* fix tests for local_blocked_networks and allow_delivery_hosts * update tests to add patches for socket.getfqdn as we are checking hostnames now * add tests for incorrect network range and hostname
* remove necessity for local blocked networks list
fixed in ce4ba27 |
* empty list is sufficient
* instantiating validator runs checks, but the validator is not needed - add noqa
|
This comment wasn't addressed. The file isn't changed at all by this PR. (Maybe a change got lost somehow?) |
There was a problem hiding this comment.
Pull request overview
This PR adds configuration-driven webhook SSRF controls so deployments can extend the blocked network list and explicitly allow deliveries to specific hostnames.
Changes:
- Add
blocked_networksandallow_delivery_hoststoWebhooksConfig, and plumb them into webhook URL validation. - Extend SSRF safety checks to merge standard blocked ranges with locally configured ranges, and introduce an allow-list override.
- Add documentation/examples (config templates, compose env vars) and tests covering the new behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tiled/server/webhooks.py | Adds support for merging standard + configured blocked networks and allow-listing specific hosts during SSRF validation. |
| tiled/server/webhook_router.py | Validates new webhook config options and passes them into SSRF checks during webhook registration. |
| tiled/config.py | Introduces new WebhooksConfig settings (blocked_networks, allow_delivery_hosts). |
| tests/test_webhooks.py | Adds unit tests for custom blocked networks and allow-list behavior, plus validator config validation. |
| example_log_config.yml | Adds an example logger configuration for tiled. |
| config.example.yml | Documents new webhook SSRF configuration options in the example config. |
| compose.dev.yml | Adds env var wiring for the new webhook SSRF configuration options. |
| CHANGELOG.md | Adds an entry describing the new webhook SSRF configurability. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ALL_BLOCKED_NETWORKS = get_combined_blocked_networks(local_blocked_networks) | ||
| for _family, _type, _proto, _canonname, sockaddr in infos: | ||
| ip_str = sockaddr[0] | ||
| try: | ||
| addr = ipaddress.ip_address(ip_str) | ||
| host = socket.getfqdn(ip_str) |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (9)
tiled/server/webhooks.py:143
- The
ValueErrorsection in this docstring has malformed backticks and refers to the old_BLOCKED_NETWORKSname, which can break rendered docs and confuse readers.
ValueError
If the URL hostname resolves to any address in the union of ``_BLOCKED_NETWORKS` and blocked_networks`.
tiled/server/webhooks.py:169
- The allow-list override currently compares
socket.getfqdn(ip_str)(reverse DNS of the resolved address) againstallow_delivery_hosts. This is not tied to the URL’s requested hostname and can be unreliable (missing/incorrect PTR records). Compare against the parsed URL hostname instead, and avoid the reverse lookup.
ALL_BLOCKED_NETWORKS = get_combined_blocked_networks(local_blocked_networks)
for _family, _type, _proto, _canonname, sockaddr in infos:
ip_str = sockaddr[0]
try:
addr = ipaddress.ip_address(ip_str)
tests/test_webhooks.py:1154
- This test relies on real DNS resolution of
notmyrealhost, which can be non-deterministic across environments (and makes the test depend on network/DNS). Patch the socket lookup to force agaierrorinstead.
def test_check_not_available_hostname(self) -> None:
"""Invalid hostname must throw exception."""
with pytest.raises(HTTPException) as exc_info:
validator = _build_url_validator( # noqa: F841
WebhooksConfig(
allow_private_addresses=False,
allow_delivery_hosts=["notmyrealhost"],
)
)
tiled/server/webhooks.py:138
- Docstring references
_BLOCKED_NETWORKS, but the constant was renamed to_STANDARD_BLOCKED_NETWORKS. This makes the parameter docs misleading for the new configurability.
This issue also appears on line 142 of the same file.
local_blocked_networks:
List of networks to combine with _BLOCKED_NETWORKS.
allow_delivery_hosts:
List of hosts to always allow, overriding any blocked_networks
tiled/server/webhooks.py:155
allow_delivery_hostsvalidation usessocket.getfqdn(host)equality, which is not a reliable hostname validity check and can reject valid hostnames depending on DNS/canonicalization. If the intent is to reject IP literals, check withipaddress.ip_addressinstead.
if allow_delivery_hosts:
for host in allow_delivery_hosts:
fqdn = socket.getfqdn(host)
if fqdn != host:
raise ValueError(f"Allow delivery host {host} must be a valid hostname")
tiled/server/webhook_router.py:88
allow_delivery_hostsvalidation usessocket.gethostbyname, which is IPv4-only and also permits IP literals (they "resolve" to themselves). This can make config validation disagree withcheck_url_ssrf_safetyand breaks IPv6 hostnames; usegetaddrinfoand explicitly reject IP addresses.
for hostname in config.allow_delivery_hosts:
try:
host_ip = socket.gethostbyname(hostname)
ipaddress.ip_address(host_ip)
except socket.gaierror as exc:
tests/test_webhooks.py:692
- This unit test helper prints to stdout, which adds noise and can cause flaky test output checks. Remove the
printcall.
This issue also appears on line 1146 of the same file.
def _fake2(hostname):
print(f"hostname: {hostname}")
if hostname in ("example.com", "93.184.216.34"):
tiled/config.py:258
- The docstring refers to
blocked_network list(singular) and uses single backticks, which is inconsistent with the rest of this docstring’s ReST formatting. This should refer to theblocked_networksfield.
allow_delivery_hosts: list of str
List of host names to which webhooks must be delivered, regardless of
whether in the `blocked_network list` or not.
compose.dev.yml:16
- In docker-compose,
${VAR}expands to an empty string when unset. Setting these list-typed settings to an empty string can cause settings parsing/validation errors at startup. Use a default like:-[](JSON list) or omit these env vars when unset.
- TILED_WEBHOOKS_SECRET_KEYS=${TILED_WEBHOOKS_SECRET_KEYS:-}
- TILED_WEBHOOKS_BLOCKED_NETWORKS=${TILED_WEBHOOKS_BLOCKED_NETWORKS}
- TILED_WEBHOOKS_ALLOW_DELIVERY_HOSTS=${TILED_WEBHOOKS_ALLOW_DELIVERY_HOSTS}
- TILED_WEBHOOKS_ALLOW_HTTP=${TILED_WEBHOOKS_ALLOW_HTTP:-false}
- TILED_WEBHOOKS_ALLOW_PRIVATE_ADDRESSES=${TILED_WEBHOOKS_ALLOW_PRIVATE_ADDRESSES:-false}
* ensure this is a list of hostnames, not IP addresses
* without this line, the next line has no `network` to work on
|
As discussed on Zoom, the comment about logging still needs to be addressed. The hard-coded defaults in |
Checklist