Skip to content

Commit 44d768b

Browse files
authored
Bug 2000514
1 parent 14ab224 commit 44d768b

3 files changed

Lines changed: 67 additions & 8 deletions

File tree

src/applications/harbormaster/step/HarbormasterHTTPRequestBuildStepImplementation.php

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -60,22 +60,27 @@ public function execute(
6060
$method = nonempty(idx($settings, 'method'), 'POST');
6161

6262
try {
63-
PhabricatorEnv::requireValidRemoteURIForFetch(
64-
$uri,
65-
array(
66-
'http',
67-
'https',
68-
));
63+
list($fetch_uri, $fetch_host) =
64+
PhabricatorEnv::requireSafeRemoteURIForFetch(
65+
$uri,
66+
array(
67+
'http',
68+
'https',
69+
));
6970
} catch (Exception $ex) {
7071
$this->logSilencedCall($build, $build_target, pht('HTTP Request: Invalid URI'));
7172
throw new HarbormasterBuildFailureException();
7273
}
7374

74-
$future = id(new HTTPSFuture($uri))
75+
$future = id(new HTTPSFuture($fetch_uri))
7576
->setMethod($method)
7677
->setFollowLocation(false)
7778
->setTimeout(60);
7879

80+
if ($fetch_host !== null) {
81+
$future->addHeader('Host', $fetch_host);
82+
}
83+
7984
$content_type = $settings['content_type'];
8085
if ($content_type) {
8186
$future->addHeader('Content-Type', $content_type);

src/applications/herald/worker/HeraldWebhookWorker.php

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -185,13 +185,34 @@ private function callWebhookWithLock(
185185
$signature = PhabricatorHash::digestHMACSHA256($payload, $key);
186186
$uri = $hook->getWebhookURI();
187187

188-
$future = id(new HTTPSFuture($uri))
188+
try {
189+
list($fetch_uri, $fetch_host) =
190+
PhabricatorEnv::requireSafeRemoteURIForFetch(
191+
$uri,
192+
array(
193+
'http',
194+
'https',
195+
));
196+
} catch (Exception $ex) {
197+
$this->failRequest(
198+
$request,
199+
HeraldWebhookRequest::ERRORTYPE_HOOK,
200+
HeraldWebhookRequest::ERROR_URI);
201+
throw new PhabricatorWorkerPermanentFailureException(
202+
$ex->getMessage());
203+
}
204+
205+
$future = id(new HTTPSFuture($fetch_uri))
189206
->setMethod('POST')
190207
->addHeader('Content-Type', 'application/json')
191208
->addHeader('X-Phabricator-Webhook-Signature', $signature)
192209
->setTimeout(15)
193210
->setData($payload);
194211

212+
if ($fetch_host !== null) {
213+
$future->addHeader('Host', $fetch_host);
214+
}
215+
195216
list($status) = $future->resolve();
196217

197218
if ($status->isTimeout()) {

src/infrastructure/env/PhabricatorEnv.php

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -850,6 +850,39 @@ public static function requireValidRemoteURIForFetch(
850850
}
851851

852852

853+
/**
854+
* Validate a remote URI and return a rebinding-safe ($fetch_uri, $fetch_host)
855+
* pair for use with HTTPSFuture.
856+
*
857+
* For HTTP, returns the pre-resolved IP URI plus the original domain as the
858+
* Host header value, eliminating DNS re-resolution at request time. For
859+
* HTTPS, returns the original URI and null (cURL cannot verify SSL certs
860+
* against bare IPs, but rebinding is not practical without a valid cert).
861+
*
862+
* @param string URI to fetch.
863+
* @param list<string> Allowed protocols.
864+
* @return pair<wild, string|null> Fetch URI and optional Host header value.
865+
* @task uri
866+
*/
867+
public static function requireSafeRemoteURIForFetch(
868+
$raw_uri,
869+
array $protocols) {
870+
871+
list($resolved_uri, $domain) = self::requireValidRemoteURIForFetch(
872+
$raw_uri,
873+
$protocols);
874+
875+
$uri = new PhutilURI($raw_uri);
876+
if ($uri->getProtocol() === 'http') {
877+
$port = $uri->getPort();
878+
$host = $port ? $domain.':'.$port : $domain;
879+
return array($resolved_uri, $host);
880+
}
881+
882+
return array($raw_uri, null);
883+
}
884+
885+
853886
/**
854887
* Determine if an IP address is in the outbound address blacklist.
855888
*

0 commit comments

Comments
 (0)