fix: stop URL-containment rejections failing silently, tighten checks

Closes the remaining gaps found in the final review of the SSRF/robustness
fixes:

- httpGet()/httpPost() now error_log() the rejected URL and configured base
  (with any embedded credentials stripped) when isWithinBase() refuses a
  request, instead of just returning false with no trace - previously this
  was the exact silent-failure mode (empty dropdown, no error anywhere) this
  PR set out to fix.
- isWithinBase() now rejects non-http(s) schemes, so the $baseOverride
  self-check path used by validateSettings() (isWithinBase($x, $x)) can no
  longer treat gopher:// or ftp:// as well-formed. Added 3 assertions
  (59 -> 62).
- findValueSet() now returns ['error' => ...] instead of a bare [] on circuit-
  breaker-open and transport failure, reusing the shape it already used for
  an unknown search type, and FindValueSetService.php sets HTTP 502 on that
  shape - mirroring how getValueSetInfo()'s false return already becomes a
  502. Verified this can't affect the online designer: its ajax call defines
  no `error` handler, so a non-2xx response simply means the autocomplete
  list doesn't update for that keystroke, rather than a 200 that looks like
  a genuine empty result.
- Removed the always-false isset($http_response_header) diagnostic in
  validateSettings() (that variable belongs to httpPost()'s scope, not this
  one) and simplified the message it was decorating.
- tests/run.php now exits immediately when not run under the CLI SAPI, since
  it ships inside the module directory under the REDCap web root.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NVZekhAz2pittp3iGgsKnK
(cherry picked from commit 6084ecfcd21347898cbef5d53a3bc3e1b0390280)

Author: drmayu7

FhirOntologyAutocompleteExternalModule.php

@@ -180,8 +180,10 @@ public function validateSettings($settings)
                 // boundary check the way the other call sites use $baseOverride.
                 $response = $this->httpPost($authEndpoint, $params, 'application/x-www-form-urlencoded', $headers, $authEndpoint);
                 if ($response === false) {
-                    $r = isset($http_response_header) ? implode("", $http_response_header) : '';
-                    $errors .= "Failed to get Authentication Token for fhir server at '" . $authEndpoint . "' response = false, r='" . $r . "'\n";
+                    // $http_response_header is populated inside httpPost()'s own scope, not
+                    // here, so it is never available at this call site - do not promise a
+                    // response body this diagnostic can never show.
+                    $errors .= "Failed to get Authentication Token for fhir server at '" . $authEndpoint . "' - request failed or was refused\n";
                 } else {
                     // a false or unparseable response decodes to null, and array_key_exists(null)
                     // is a fatal TypeError on PHP 8
@@ -797,8 +799,13 @@ public function findValueSet($type, $query)
             return ['error' => "Unknown search type $type"];
         }
         if ($this->isCircuitOpen()) {
-            // Server has failed repeatedly - fail fast.
-            return [];
+            // Server has failed repeatedly - fail fast. Use the same ['error' => ...]
+            // shape as the "unknown search type" case above, rather than an empty
+            // array, so FindValueSetService.php can tell a genuine failure apart
+            // from "no matches" instead of returning a misleadingly successful
+            // empty result (see getValueSetInfo(), which does the equivalent with
+            // a false return).
+            return ['error' => 'The terminology server is not responding. Please try again shortly.'];
         }
         $headers = ['User-Agent: Redcap'];
         $authHeader = $this->getAuthHeader();
@@ -815,7 +822,7 @@ public function findValueSet($type, $query)
         }
         if ($result_json === false) {
             $this->recordFhirFailureIfSlow(microtime(true) - $startedAt);
-            return [];
+            return ['error' => 'The terminology server is not responding. Please try again shortly.'];
         }
         $this->recordFhirSuccess();
         return $processFunction(json_decode($result_json, true));
@@ -942,6 +949,39 @@ public function recordFhirSuccess()
     }
 
 
+    /**
+     * Returns $url with any embedded userinfo (user:pass@) stripped, for safe
+     * inclusion in log messages. Falls back to the original value if it cannot
+     * be parsed as a URL.
+     */
+    private function urlForLogging($url)
+    {
+        if (!is_string($url) || '' === $url) {
+            return (string)$url;
+        }
+        $parts = parse_url($url);
+        if (!is_array($parts) || (!isset($parts['user']) && !isset($parts['pass']))) {
+            return $url;
+        }
+        $result = '';
+        if (isset($parts['scheme'])) {
+            $result .= $parts['scheme'] . '://';
+        }
+        if (isset($parts['host'])) {
+            $result .= $parts['host'];
+        }
+        if (isset($parts['port'])) {
+            $result .= ':' . $parts['port'];
+        }
+        if (isset($parts['path'])) {
+            $result .= $parts['path'];
+        }
+        if (isset($parts['query'])) {
+            $result .= '?' . $parts['query'];
+        }
+        return $result;
+    }
+
     public function httpGet($fullUrl, $headers, $baseOverride = null)
     {
         // getFhirServerUri() strips any trailing slash from the configured setting.
@@ -954,6 +994,8 @@ public function httpGet($fullUrl, $headers, $baseOverride = null)
         // trivially true.
         $base = (null === $baseOverride) ? $this->getFhirServerUri() : $baseOverride;
         if (!FhirRequestPolicy::isWithinBase($fullUrl, $base)) {
+            error_log('FhirOntologyAutocompleteExternalModule: httpGet refused URL outside configured FHIR server - url='
+                . $this->urlForLogging($fullUrl) . ' base=' . $this->urlForLogging($base));
             return false;
         }
         $timeout = $this->getFhirTimeout();
@@ -1003,12 +1045,16 @@ public function httpPost($fullUrl, $postData, $contentType, $headers, $baseOverr
         // trivially true.
         if (null !== $baseOverride) {
             $allowed = FhirRequestPolicy::isWithinBase($fullUrl, $baseOverride);
+            $baseForLog = $baseOverride;
         } else {
             $tokenEndpoint = $this->getSystemSetting('cc_token_endpoint');
+            $baseForLog = $this->getFhirServerUri();
             $allowed = ($tokenEndpoint && $fullUrl === $tokenEndpoint)
-                || FhirRequestPolicy::isWithinBase($fullUrl, $this->getFhirServerUri());
+                || FhirRequestPolicy::isWithinBase($fullUrl, $baseForLog);
         }
         if (!$allowed) {
+            error_log('FhirOntologyAutocompleteExternalModule: httpPost refused URL outside configured FHIR server - url='
+                . $this->urlForLogging($fullUrl) . ' base=' . $this->urlForLogging($baseForLog));
             return false;
         }
         $timeout = $this->getFhirTimeout();

FhirRequestPolicy.php

@@ -86,10 +86,10 @@ public static function opensBreaker($failureCount, $threshold = self::BREAKER_FA
     /**
      * True when $url addresses the same origin as $baseUri, sits at or below its
      * path, carries no embedded credentials other than a copy of the base's own,
-     * and contains no dot-segment traversal. Every outbound request is checked
-     * against the configured FHIR server so that a malformed or hostile setting
-     * cannot turn the module into a proxy for arbitrary hosts on the REDCap
-     * server's network.
+     * and contains no dot-segment traversal. Every URL this module constructs
+     * before handing it to the HTTP helper is checked against this, so a
+     * malformed or hostile setting cannot make the module request a path or
+     * host outside the configured FHIR server *at the point this check runs*.
      *
      * Specifically: scheme, host and port must match; the path must be the base
      * or a descendant; and dot-segment traversal (., .., etc.) is rejected in
@@ -103,6 +103,15 @@ public static function opensBreaker($failureCount, $threshold = self::BREAKER_FA
      * reject per the Unicode spec (e.g. overlong UTF-8 sequences) are not blocked
      * — the module constructs every outbound path from fixed components so such
      * sequences never appear in practice.
+     *
+     * This is NOT a general SSRF guard. It performs no DNS resolution, so a
+     * configured host that resolves (now or later) to a link-local or private
+     * address passes unchanged. It also does not see what happens after the
+     * request leaves this check: REDCap core's http_get()/http_post() follow
+     * HTTP redirects, so a terminology server that replies
+     * 302 -> http://169.254.169.254/latest/meta-data/ is followed there
+     * regardless of what this method decided. Redirect-following and DNS
+     * rebinding are both outside what this check covers.
      */
     public static function isWithinBase($url, $baseUri)
     {
@@ -137,6 +146,13 @@ public static function isWithinBase($url, $baseUri)
                 return false;
             }
         }
+        // Scheme is already confirmed equal above, but that alone does not make it
+        // a valid HTTP(S) URL - without this, isWithinBase($baseOverride, $baseOverride)
+        // (used by validateSettings() as a well-formedness check) would accept
+        // schemes such as gopher:// or ftp:// as well-formed.
+        if (!in_array(strtolower($u['scheme']), array('http', 'https'))) {
+            return false;
+        }
         if (self::port($u) !== self::port($b)) {
             return false;
         }

FindValueSetService.php

@@ -77,6 +77,14 @@
 
 if ('find' === $action){
     $result = $module->findValueSet($type, $query);
+    if (is_array($result) && array_key_exists('error', $result)) {
+        // Breaker open, transport failure, or (in practice unreachable, since
+        // $type is validated above) an unknown search type. Signal it with a
+        // non-2xx status, the same way getValueSetInfo()'s false return becomes
+        // a 502 below, instead of a 200 the autocomplete widget would read as a
+        // genuine "no matches".
+        http_response_code(502);
+    }
     echo json_encode($result, JSON_PRETTY_PRINT|JSON_UNESCAPED_SLASHES);
 }
 else {

tests/run.php

@@ -1,8 +1,13 @@
 <?php
+if (PHP_SAPI !== 'cli') { exit; }
 /**
  * Plain-PHP test runner. No composer, no PHPUnit - the module must stay
  * installable by copying a directory, so tests carry no dependencies.
  *
+ * This file lives inside the module directory, which sits under the REDCap
+ * web root, so it must refuse to execute as an unauthenticated web request -
+ * hence the SAPI guard above, as the first executable statement.
+ *
  * Usage: php tests/run.php
  */
 
@@ -165,6 +170,16 @@ function assertFalse($actual, $label)
 assertTrue(FhirRequestPolicy::isWithinBase('https://ts.example.org/fhir/x?q=../../etc', $base),
     'isWithinBase: traversal in query string allowed');
 
+// --- scheme must be http/https (validateSettings()'s self-check path uses ----
+// --- isWithinBase($x, $x), where scheme equality alone lets anything through) --
+
+assertFalse(FhirRequestPolicy::isWithinBase('gopher://internal:70/', 'gopher://internal:70/'),
+    'isWithinBase: gopher scheme rejected even when url equals base');
+assertFalse(FhirRequestPolicy::isWithinBase('ftp://internal/fhir', 'ftp://internal/fhir'),
+    'isWithinBase: ftp scheme rejected even when url equals base');
+assertTrue(FhirRequestPolicy::isWithinBase('http://ts.example.org/fhir', 'http://ts.example.org/fhir'),
+    'isWithinBase: http scheme still allowed when url equals base');
+
 // --- summary --------------------------------------------------------------
 
 $passed = $GLOBALS['tests_passed'];