Conversation
The option looked the api url host up and rejected private IPs, but the request that followed handed the host name to curl, which looked it up again. When the answer changed in between, the image, the options and the api key could still end up at a private address. The IPs from the check are now kept and passed on to curl in CURLOPT_RESOLVE, so the request connects to the addresses that were checked. The url itself is passed on unchanged, so the Host header and the TLS SNI are unaffected.
22c14df to
e3188fc
Compare
CURLOPT_RESOLVE was added in curl 7.21.3, and ext/curl only registers the constant when it is built against that version or newer. The option is now checked in checkOperationality(), so the converter reports itself as not operational instead of raising an Error mid-convert on PHP 8, or silently skipping the pin on older PHP. Also correct the documentation: it claimed the first IP was pinned on every libcurl below 7.59.0, which is not true below 7.21.3, where pinning is not available at all.
A proxy resolves the host itself and connects on our behalf, so with a proxy in between, the request never reaches one of the IPs that ::checkOperationality() checked, and CURLOPT_RESOLVE has no effect at all. libcurl takes a proxy from the environment unless it is told not to, so on any setup with http_proxy (or https_proxy / all_proxy) set, the option silently stopped delivering what it promises. Setting CURLOPT_PROXY to an empty string disables proxy use for the handle, also when the environment holds one. It is only done when the option is enabled, so nothing changes for anyone who does not use it. Also noted in ::createResolveOption() why the IPs are not written within [brackets]: libcurl only accepts those from 7.57.0, and an entry it cannot parse is skipped, which would leave the request unpinned on the older versions that this feature supports.
|
Added one more commit: While re-checking this branch I found that the pin can be discarded without any error. libcurl ignores Measured on libcurl 8.21.0 with the same option value this code builds:
The commit clears One more thing worth recording, in case it looks like an oversight: the IPv6 addresses are deliberately passed without square brackets. Brackets are accepted from libcurl 7.57.0 onwards, but versions below that reject a bracketed entry and skip it silently, which would reintroduce the unpinned request on exactly the old versions this option still supports. There is a comment in the code so it does not get "corrected" later. |
This branch has not been deployed
1 participant
What this changes
Wpc::checkOperationality()resolves the api url host withdns_get_record()and refuses the conversion when it points at a private address. The request that follows then hands the original host name to curl, which resolves it a second time. The two lookups are independent, so the address that was checked is not necessarily the address that is connected to.If the second answer differs from the first, the multipart POST - the source image, the conversion options and, depending on the setup, the api key - can still reach an address the check was meant to exclude.
The option matters exactly where the api url is not fully trusted: the docs recommend it for integrations that let a user choose
wpc-api-url, and it is off by default.The fix
checkIfUrlIsPublicIp()becomesgetPublicIpsOfUrl()and returns the addresses it validated instead of a bool.doActualConvert()passes them to curl throughCURLOPT_RESOLVE, so the request connects to the addresses that were checked.The url itself is passed on unchanged, so the
Hostheader and the TLS SNI are unaffected. When the url already contains an IP there is no lookup to pin and nothing is added. All validated addresses are pinned, so an A + AAAA host keeps its fallback.CURLOPT_PROTOCOLSandCURLOPT_REDIR_PROTOCOLSare already handled inCurlTrait::initCurl()and were left alone.Tests
Two tests in
tests/Convert/Converters/WPCTest.phpcover the new behaviour, with a smallWpcExposerin the style of the existing exposers.phpcs(PSR2) andphpstanat level 4 are clean.Sponsor company: OpenServis.cz
Fixes #362