Reject internal-address authority values on followers/sync at the route layer#3232
Merged
Reject internal-address authority values on followers/sync at the route layer#3232
Conversation
resolve_public_host() previously only looked up A records via gethostbynamel(), so any host with AAAA-only records was rejected even if it was a perfectly public IPv6 address. The current callers (CIMD discovery, SSE proxy) all gate on the false return and stay safe, but the helper documents itself as an SSRF guard for general use, and a future caller forgetting the false-check would silently lose protection on IPv6 deployments. Add a dns_get_record(DNS_AAAA) lookup alongside gethostbynamel(), validate every returned address against private/reserved ranges, and prefer IPv4 when both exist (mirrors wp_safe_remote_get's default). Extract the IPv4-mapped IPv6 byte-pattern check into is_ipv4_mapped_ipv6() so the literal path and the AAAA path can share it, and add unit tests for that helper.
Introduce an `activitypub_pre_resolve_public_host` filter so tests can inject pre-resolved address sets without making real DNS queries. Add tests covering the new code paths the previous coverage missed: IPv4 preference when both record types exist, fall-through to IPv6 when only AAAA resolves, split-horizon rejection (one private address fails the whole answer) for both IPv4 and IPv6, IPv4-mapped IPv6 rejection in the AAAA path, and the empty-resolution case.
There was a problem hiding this comment.
Pull request overview
Adds route-layer validation to reject obviously internal-network authority values on the FEP-8fcf /followers/sync endpoint, failing fast with rest_invalid_param.
Changes:
- Added
validate_callbackfor theauthorityroute arg to reject private/reserved IPs and localhost-style hostnames. - Added PHPUnit coverage asserting internal authorities are rejected during route validation (before signature verification / handler logic).
- Added a security patch changelog entry.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
includes/rest/class-followers-controller.php |
Adds route arg validation for /followers/sync authority to reject internal-address shapes early. |
tests/phpunit/tests/includes/rest/class-test-followers-controller.php |
Adds data provider + test ensuring internal authority values fail with rest_invalid_param (400). |
.github/changelog/harden-followers-sync-authority |
Records the security patch changelog entry for the change. |
pfefferle
force-pushed
the
harden/followers-sync-authority
branch
from
f82459c to
e430639
Compare
pfefferle
changed the base branch from
trunk
to
harden/resolve-public-host-ipv6
…tests The previous stub helper registered a filter without removing it, so the injected addresses persisted across test boundaries. Track every registered callback and remove them all in tear_down() so the suite stays order-independent.
pfefferle
force-pushed
the
harden/followers-sync-authority
branch
from
e430639 to
cfb2880
Compare
- is_ipv4_mapped_ipv6() short-circuits on non-IPv6 input before inet_pton(), so feeding it 'not-an-ip' or '' (which the unit tests do) no longer triggers a warning. - The activitypub_pre_resolve_public_host filter return value now has its ipv4 / ipv6 keys verified to be arrays before iteration; a misbehaving filter callback can't crash the foreach with a type error.
pfefferle
force-pushed
the
harden/followers-sync-authority
branch
2 times, most recently
from
747639e to
df7a234
Compare
The closure has no parameters, so registering it with `accepted_args = 2` would have WordPress invoke it with two arguments the callable doesn't expect. Falling back to the add_filter() default keeps the call-site argument count aligned with the closure shape.
…te layer The /followers/sync (FEP-8fcf) authority parameter is the verified peer's host, used downstream as a SQL filter against stored follower GUIDs. There's no outbound fetch, and the existing signer-host check already requires the asked authority to match the verified peer's keyId host — so a crafted authority is practically rejected today. Add a route-level validate_callback that refuses obvious internal-address shapes (RFC1918, loopback, link-local metadata, unspecified IPv4, IPv6 loopback, localhost, *.localhost, *.local) so those requests fail fast with a clean rest_invalid_param error instead of getting deeper into the handler. Defense-in-depth and cleaner request validation, no security posture change in the single-attacker model.
- Strip a single FQDN trailing dot before checking against the localhost / *.localhost / *.local exclusions. Without this, "localhost." or "printer.local." would slip past the validator. - Reject IPv4-mapped IPv6 literals at the IP-literal branch explicitly, matching the rejection done elsewhere in the codebase for the same reason: FILTER_FLAG_NO_RES_RANGE catches the ::ffff:0:0/96 range on some PHP builds but not others, so SSRF guards shouldn't depend on it. - Cover the two new edge-case classes in the test data provider.
Extract the lowercase + bracket-strip + trailing-dot-trim normalization into a private normalize_host() helper and run both $signer_host and $asked_host through it in get_partial_followers(). Without this, semantically equivalent authorities (e.g. "https://example.com." vs "https://example.com" or "[::1]" vs "::1") could be parsed into strings that don't match each other and produce a spurious 403, even when the signing peer and the asked authority are the same. The validate_callback for the route arg now uses the same helper so the two code paths share one canonical form.
pfefferle
force-pushed
the
harden/followers-sync-authority
branch
from
df7a234 to
29f5edc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Proposed changes:
/followers/syncauthorityparameter is the verified peer's host, used downstream as a SQL filter against stored follower GUIDs. There's no outbound fetch, and the existing signer-host check already requires the asked authority to match the verified peer's keyId host — so a crafted authority is practically rejected today.validate_callbackthat refuses obvious internal-address shapes (RFC1918, loopback, link-local metadata, unspecified IPv4, IPv6 loopback,localhost,*.localhost,*.local) so those requests fail fast with a cleanrest_invalid_paramerror instead of getting deeper into the handler.Other information:
Testing instructions:
/followers/syncrequest with a valid peer keyId pointing to your realmastodon.social(or similar) authority. The request should round-trip as before.authority=https://127.0.0.1,https://10.0.0.1,https://169.254.169.254,https://[::1],https://localhost,https://api.localhost, orhttps://printer.local. Each should return a400withrest_invalid_parambefore signature verification even runs.Changelog entry
Changelog Entry Details
Significance
Type
Message
Reject follower sync requests targeted at internal-network hosts at the route layer.