refactor(client): use IClientService for Etherpad HTTP (#103) - #123
Merged
Conversation
EtherpadClient talked to Etherpad via file_get_contents + stream contexts, bypassing Nextcloud's HTTP client (instance proxy / central TLS + timeout config). Inject OCP\Http\Client\IClientService and route both the API call path (sendRequest) and the version-detection GET (sendPublicGetRequest) through it. - Redirects stay disabled (allow_redirects => ['max' => 0]) so a redirect can never leak the apikey to a foreign host; timeout stays 15s; Accept: json. - NC's client throws on >= 400, so doRequest() recovers the real response via getResponseFromThrowable() (NC 29+; we target 31+) to keep status handling at the call site, and treats a response-less throwable as a transport error. - The injected client is finally mockable: new unit tests cover success, apikey-from-AdminSettingsRepository (the #105 regression guard, previously only catchable via e2e), non-zero API code, invalid JSON, HTTP-error cause chain, and transport-failure wrapping. - Add OCP Http\Client stubs (IClientService/IClient/IResponse) for PHPUnit; Psalm resolves the real types via nextcloud/ocp (baseline shrinks by 1). PHPUnit 402 green; Psalm green.
Nextcloud's HTTP client blocks loopback/LAN targets by default (SSRF protection for user-supplied URLs). The Etherpad API host is admin-configured and is very commonly a local address (here http://localhost:9001), so the IClientService switch made every API call fail with a LocalServerException -> 'Etherpad API request failed: createAuthorIfNotExistsFor' (HTTP 400 on open-by-id) -- the same surface symptom as the #105 regression. file_get_contents had no such guard, hence this only appeared after the IClientService refactor. Pass nextcloud.allow_local_address=true for these trusted admin-only calls. User-reachable external pad fetching is unaffected (ExternalPadExportFetcher keeps its own public-IP / DNS-rebinding guards). Verified on NC 33: pad open + admin connection test work again.
There was a problem hiding this comment.
Pull request overview
This PR refactors EtherpadClient to use Nextcloud’s HTTP client (OCP\Http\Client\IClientService) instead of file_get_contents/stream contexts, improving proxy/TLS/timeout integration and enabling proper unit testing of request/response handling.
Changes:
- Inject
IClientServiceintoEtherpadClientand route both API calls and public GETs throughrequest(), with shared request options (timeout, JSONAccept, redirects disabled, and local-address allowance). - Add unit tests covering common success/error paths using a mocked HTTP client.
- Add PHPUnit stubs for
OCP\Http\Clientinterfaces and wire them into the PHPUnit bootstrap; shrink the Psalm baseline accordingly.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/Service/EtherpadClient.php | Replaces file_get_contents HTTP with Nextcloud IClientService, centralizing request options and error recovery. |
| tests/phpunit/unit/EtherpadClientTest.php | Adds unit coverage for API success, API-level errors, invalid JSON, and transport/HTTP failure wrapping. |
| tests/phpunit/stubs/OCP/Http/Client/IClient.php | Adds PHPUnit stub for OCP\Http\Client\IClient to allow mocking without a full Nextcloud runtime. |
| tests/phpunit/stubs/OCP/Http/Client/IClientService.php | Adds PHPUnit stub for IClientService used for dependency injection in tests. |
| tests/phpunit/stubs/OCP/Http/Client/IResponse.php | Adds PHPUnit stub for IResponse used by the new client-based request path. |
| tests/phpunit/bootstrap.php | Registers the new OCP HTTP client stubs for the unit test environment. |
| psalm-baseline.xml | Removes a baseline entry made obsolete by the refactor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| return $client->getResponseFromThrowable($e); | ||
| } catch (\Throwable) { | ||
| throw new EtherpadClientException('Etherpad transport error: ' . $e->getMessage()); |
Comment on lines
+106
to
+112
| public function testApiCallSurfacesHttpErrorStatusAsCause(): void { | ||
| // e.g. a wrong apikey makes Etherpad answer 401 — the failure mode the | ||
| // #105 regression produced. apiCall() wraps every sendRequest failure | ||
| // as "request failed: <method>" and preserves the HTTP error as the | ||
| // cause (exactly what the server log showed during #105). | ||
| $client = $this->clientWithResponse($this->response(401, 'Unauthorized')); | ||
|
|
…eck comment Review follow-ups on #103 (both non-blocking): - Add a unit test asserting detectApiVersion() issues a plain GET against <host>/api with no request body and the shared base options (redirects disabled, allow_local_address). - The HealthCheck transport-error branch still mentioned file_get_contents/curl; it now goes through IClientService, so update the comment. Test/comment only, no runtime behaviour change. PHPUnit 403 + Psalm green.
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.
What
EtherpadClienttalked to Etherpad viafile_get_contents+ stream contexts, bypassing Nextcloud's HTTP client (instance proxy / central TLS + timeout config) and leaving the class untestable. Route all HTTP throughOCP\Http\Client\IClientService.Changes
IClientService; replacesendRequest(API calls) andsendPublicGetRequest(version detection). Removedfile_get_contents,stream_context_create,$http_response_header,buildHttpContext,extractStatusCode.allow_redirects => ['max' => 0]) so a redirect can't leak the apikey; timeout stays 15s;Accept: application/json.doRequest()recovers the real response viagetResponseFromThrowable()(NC 29+, we target 31+) to keep status handling at the call site; a response-less throwable is a transport failure.nextcloud.allow_local_address => truefor these calls — the Etherpad API host is admin-configured and commonly a loopback/LAN address (e.g.http://localhost:9001). NC's client blocks local addresses by default (SSRF protection for user URLs); without this every API call failed withLocalServerException. User-reachable external pad fetching is untouched (ExternalPadExportFetcherkeeps its own public-IP / DNS-rebinding guards).AdminSettingsRepository::getApiKey()(the Store the Etherpad API key as a sensitive app config value #105 regression guard, previously only catchable via e2e), non-zero API code, invalid JSON, the HTTP-error cause chain, and transport-failure wrapping. AddedOCP\Http\Clientstubs (IClientService/IClient/IResponse) for PHPUnit.Acceptance
file_get_contentsfor HTTP inEtherpadClientEtherpadClientTestconstructor calls updatedVerification
RedundantCast).Note: caught a real regression mid-PR (local-address blocking) that PHPUnit could not see — exactly the failure mode the full-e2e-before-merge rule exists for.
Closes #103.