fix: correct overstated claims, constrain outbound requests, drop dead cache

[drmayu7]

Security and performance work carried out on a fork, offered back upstream. Rebased onto current main, and updated to address the review.

What changed since the review

All three findings were real. Two are fixed; the third is deferred with the claim narrowed, per the fallback suggested in the review.

1. Timeout — claim narrowed, real fix deferred

I could not confirm core's behaviour independently: I have no read access to the REDCap source on my server, so I took the reading of Config/init_functions.php on trust. Assuming it holds, fhir_timeout bounds only connection establishment, not a server that accepts and then stalls.

Rather than ship a fix built on a guess, the claim is now scoped to what is true — in the changelog, in the settings reference, in the getFhirTimeout() docblock, and in the module configuration screen the administrator actually reads while setting the value. The circuit breaker's coverage is also stated honestly: it helps when a slow or erroring server eventually returns, since those calls are recorded, but it does not help against a true indefinite hang, because the breaker is only informed after the call returns.

The open question, which is the one thing blocking the real fix: what TLS options does core set on its curl handles? If core disables peer verification and I enable it, I break every site whose terminology server presents an internal CA — the common deployment for this module. If I disable it when core verifies, I have quietly weakened transport security inside a security release. Once that is answered, the module can issue its own curl requests with an explicit CURLOPT_TIMEOUT.

2. Circuit breaker — claim corrected, no locking added

The read-then-write races are real. The defect was the docblock asserting "exactly one concurrent request", not the behaviour: the breaker exists to stop a stampede, and a handful of probes slipping through at a window boundary is harmless. A real guarantee would cost a GET_LOCK() round trip on every autocomplete keystroke, or SQL against redcap_external_module_settings. Neither seemed worth it, so the documentation now describes what the code does — normally one probe, occasionally a few, and a counter that can undercount under concurrency so the breaker may open after slightly more than three failures.

Happy to implement the guarantee instead if you would rather have it.

3. Dead cache — removed

Gone, along with the comment claiming a per-keystroke benefit it could not deliver. global $Proj and the metadata fast path — the part that actually mattered — are retained.

Also in this update

• Outbound FHIR requests are now constrained to the configured server. FhirRequestPolicy::isWithinBase() checks scheme, host and port, requires the path to sit at or below the configured base, rejects mismatched embedded credentials, and rejects dot-segment traversal in literal, percent-encoded, double-encoded, backslash-separated and ;-parameter forms. Rejections are logged (with credentials stripped) rather than failing silently. The docblock is explicit that this is not a general SSRF guard: it validates the URL this module constructs, and does not cover redirect-following by core's helpers or DNS-based rebinding.
• A test suite. You noted there was no test framework, so there is now a dependency-free runner — no composer, no PHPUnit, php tests/run.php, 62 assertions. The timeout resolution, every circuit-breaker state transition, and the URL containment rules are covered. tests/run.php refuses to run under a web SAPI.
• findValueSet() no longer returns an empty list on failure. It now returns the existing ['error' => ...] shape and the service replies 502, matching getValueSetInfo(), so an outage is not rendered as "no matches".
• Changelog no longer asserts version numbers, since release-please now derives them. Site-specific deployment prose has been removed. The credential non-migration warning is kept and now names both the Basic Auth password and the OAuth2 client secret — changing a setting's type does not migrate the stored value, and the failure is silent.

What is verified, and what is not

php -l is clean on all files, config.json parses, php tests/run.php passes, and Psalm's taint analysis reports no findings. php-version-min stays 5.4.0 and framework-version stays 1.

The tests cover pure decision logic only. They do not prove that anything times out against a genuinely stalled server, and they cannot — that needs a live environment and I have no staging instance. Runtime behaviour was exercised by hand against an Ontoserver-backed endpoint. An independent look is still worth having before merging.

Framework 16

Interested, and I would suggest keeping it separate from this PR. The changes here are framework-independent, and bundling a REDCap floor change from 8.8.1 to 14.6.4 into a security diff makes both harder to review. If you push your branch I will work from it — my REDCap is on 16.0.37, so I can exercise framework 16 directly.

[dconlan]

@drmayu7

Thanks for this PR.

I pointed Claude at it and this is what the adversarial review gave me.

--
Adversarial review of this PR (static/code review — not run against a live FHIR server)

Thanks for the thorough writeup and the tranching — the no-auth removal and the XSS fix in particular are solid, verified findings. A few things worth resolving before merge:

1. (High) The timeout fix doesn't bound the failure mode it's meant to fix, on any host with curl installed

httpGet/httpPost delegate entirely to REDCap core's http_get()/http_post() (Config/init_functions.php). I checked those directly: when curl is available (the default on essentially every production install), the only timeout curl ever gets is CURLOPT_CONNECTTIMEOUT — core never sets
CURLOPT_TIMEOUT. CONNECTTIMEOUT bounds just the TCP handshake.

The scenario in the PR description — "a slow or restarting terminology server could hold PHP workers open" — is a server that accepts the connection and then stalls before sending a response. That's not covered: curl_exec() can still block indefinitely once connected, parking the worker exactly
as before this change. Only the file_get_contents fallback (used when curl is absent) gets a true end-to-end timeout, since the stream context timeout option is total-time, not connect-time.

So fhir_timeout genuinely helps against an unreachable host (refused/dropped connection), but not against a slow one — which is the more likely real failure mode for a terminology server under load, and the case actually named in the PR. Suggest either fixing this at the
http_get/http_post call site (there's no way to pass CURLOPT_TIMEOUT through core's helpers as written, so this may need the module to make its own curl calls for the FHIR paths), or scoping the README/PR claim down to "protects against unreachable hosts" until it's addressed.

2. (Medium) Circuit breaker's atomicity is weaker than the commit messages describe

isCircuitOpen() and recordFhirFailure() do read-then-write over getSystemSetting/setSystemSetting, which round-trip the DB with no locking. Two consequences:

• The "exactly one trial request is allowed through" property (the re-arm in isCircuitOpen()) isn't guaranteed — concurrent requests arriving at the window boundary can each read the window as expired before either writes the re-arm, so more than one trial request can go out.
• Consecutive-failure counting can lose increments under concurrent failures (classic read-N/write-N+1 race), so the breaker can take longer than 3 actual failures to open.

Neither is a security issue, and both are still a clear improvement over no breaker — but I'd soften the "exactly one" language in the docs/commit history, or add a lock (e.g. a short-lived unique key insert) if the single-prober property actually matters operationally.

3. (Low) The new getHideChoice() cache doesn't do anything

The fix keeps the important part — restoring the global $Proj fast path is a real, verified perf win. But the added static $cache keyed pid|field is commented as caching "per keystroke"; OntologyManager::searchOntology() makes one call per HTTP request, and each keystroke is its own PHP
process, so the static cache resets every time and is only ever read once per request. Harmless, but it can be dropped — it's dead weight.

Confirmed correct, no notes:

• No-auth removal on FindValueSetService.php — verified the Online Designer builds its URL with getUrl(..., $noAuth=false, ...), so it's the only caller and is unaffected.
• XSS fix — grepped the file for every .append(/.html(/innerHTML call; the two patched spots were the only insertion points, both now go through .text()/createTextNode.
• expires_in seconds-vs-ms fix, the PHP 8 array_key_exists(null, …) guards, the unknown-action 400, and getAuthHeader()'s false-propagation all match the real failure contracts of the functions they touch.
• No PHP >5.4 syntax introduced — the php-version-min: 5.4.0 claim holds against this diff.

--

I do have some work locally on the project to move to framework 16. If you want to address the issues raised above I can merge the PR and try and upgrade the EM in the official registry. If you are interested I can also commit my changes to a branch and we can work on a new release.

[drmayu7]

Thanks for taking the time on this, and for running it through a proper adversarial pass. All three findings are real — I've checked each against the code. Here's what I plan to change.

1. Timeout — agreed, and it's the one that matters

I can't independently confirm the core behaviour: I don't have read access to the REDCap source on my server, so I'm taking your reading of Config/init_functions.php on trust. Assuming it holds, delegating to core can't be salvaged — there's no way to pass CURLOPT_TIMEOUT through those helpers as written.

Rather than try to match behaviour I can't read, I'd like to stop calling core for the module's outbound requests entirely: one module-owned curl path setting both CURLOPT_CONNECTTIMEOUT and CURLOPT_TIMEOUT, keeping the existing file_get_contents fallback for hosts without curl — that path already gets a true end-to-end timeout, since the stream context option is total-time. Proxy handling mirrors what the module's own fallback already does with sameHostUrl() / PROXY_HOSTNAME / PROXY_USERNAME_PASSWORD, so it doesn't need core either. It also lets the Content-type workaround in httpPost go away.

I'd apply it to the OAuth2 token endpoint as well — a stalled token endpoint parks a worker just as effectively as a stalled FHIR server. The breaker stays wrapped around the three FHIR entry points rather than the HTTP helpers, so OAuth negotiation against a different host still isn't trapped by it.

One question, and it's the thing actually blocking me: what TLS options does core set on those curl handles? If core disables peer verification and I enable it, I break every site whose terminology server presents an internal CA or self-signed certificate — and since these usually sit on internal networks, that could be a lot of them. If I disable it, I've quietly weakened transport security inside a security release. I'd rather not guess, and you can read core where I can't.

2. Breaker atomicity — you're right, and I'd like to fix the claim rather than the code

The read-then-write races are real. But I think the defect here is the docblock asserting "exactly one concurrent request", not the behaviour itself. The breaker exists to stop a stampede of hundreds of workers dialling a dead server; a handful of probes slipping through at a window boundary is harmless. A lost increment only means it opens on the fourth failure instead of the third, and self-corrects.

The options for a real guarantee are a GET_LOCK() round-trip on the hot path of every autocomplete keystroke, or SQL written directly against redcap_external_module_settings, which is framework-internal. Neither looks worth it for a property that doesn't seem to matter operationally. So I'll rewrite the docblock and the PR description to describe what the code actually does — normally one probe, occasionally a few, and failure counting that can undercount under concurrency.

Say the word if you'd rather have the hard guarantee and I'll implement it properly instead.

3. Dead cache — agreed, removing it

Keeping global $Proj and the $Proj->metadata[$field] gating, which is the part that actually mattered, and dropping the comment that claims a per-keystroke benefit it can't deliver.

On testing

Fair hit. I've since built a dependency-free PHP harness on a later branch — no composer, no PHPUnit, just php tests/run.php — and I'll port it here to cover the breaker decisions, timeout resolution and URL validation. That means extracting the pure logic out of the settings I/O so it's reachable without REDCap.

It won't prove curl actually times out against a genuinely stalled server, and it won't prove TLS verification doesn't break real deployments. Both need a live environment and I have no staging instance, so I'll be explicit in the PR about where the tests stop rather than implying they cover more than they do.

Housekeeping since the PR opened

I'll rebase onto current main — it's conflicting now — retitle with a conventional-commit prefix for the title lint, and drop the "v0.5.1 / v0.5.2" framing, since release-please owns version numbers now and 0.5.1 is already spoken for. The credential-masking caveat survives as a deployment note rather than a version claim: the stored password doesn't migrate when the setting type changes, so it has to be re-entered immediately after upgrading or lookups fail silently with an empty dropdown.

One heads-up: making the curl call visible to Psalm will most likely trip TaintedSSRF, since at the moment the sink is hidden inside core where the scanner can't see it. I'd rather answer that with a helper constraining the URL to the configured FHIR base than with a taint-escape annotation — a real constraint rather than a suppression.

Framework 16

Yes, I'm interested. I'd suggest keeping it separate from this PR though. The three fixes above are framework-independent — there's no HTTP helper at any framework version, so the timeout work is identical either way — and bundling a REDCap floor change from 8.8.1 to 14.6.4 into a security diff makes both harder to review.

If you push your branch I'll work from it. My REDCap is on 16.0.37, so I can exercise framework 16 directly, though not 17 since that needs 17.0.1.