Skip to content

fix: stop TypeError on non-object responses (public key, error bodies, pagination) - #19

Merged
wzul merged 6 commits into
mainfrom
fix/public-key-string-response
Oct 23, 2026
Merged

wzul merged 6 commits into
mainfrom
fix/public-key-string-response

Conversation

@wzul

@wzul wzul commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

$chip->publicKey->get() throws:

TypeError: Chip\Http\GuzzleClient::request(): Return value must be of type stdClass|array, string returned

so the public key can never be fetched and callback signature verification through the SDK is impossible (closes #18).

Same bug class elsewhere. An audit of every path where a decoded response reaches a typed parameter or return found three more TypeError shapes, all fixed here too:

  1. Error bodies — detail/message as a list or object was passed straight into ChipApiException::__construct(string $message), and a JSON scalar body into its ?array $responseBody. A 4xx/5xx therefore raised TypeError instead of a ChipApiException, so catch (ChipApiException $e) never ran.
  2. Pagination — iterate() passed the raw next value to request(string $endpoint): next: false (some deployments emit it) raised TypeError: Argument #2 ($endpoint) must be of type string, false given, and next: "" looped forever. Affects webhooks, company_statements, billing_templates, billing templates clients, clients.

Root cause

GET /public_key/ does not return an object. It returns a JSON-encoded PEM string ("-----BEGIN PUBLIC KEY-----\n…", quotes included) — this is documented in the API reference (/public_key/ → "The response body is a JSON-encoded PEM string … not a bare PEM body and not a {"key": ...} object") and matched by the other SDKs (chip-go-sdk does json.Unmarshal(body, &result) into a string; the Node example does JSON.parse(data); the WooCommerce plugin treats the decoded body as a string).

GuzzleClient::request() returns \stdClass|array, and its body handling was json_decode($body, true) ?? new \stdClass(). A JSON string body decodes to a PHP string, which violates the declared return type → TypeError. Even without the type error, PublicKeyResource::get() did (array) $response and $arr['public_key'] ?? '', which would have returned '' for a string body.

Changes

  • lib/Http/ClientInterface.php, GuzzleClient.php, RetryClient.php: request() return type is now \stdClass|array<string, mixed>|string.
    • a decoded string body is returned as-is;
    • a plain-text (non-JSON) body is returned instead of being silently replaced with an empty stdClass;
    • an empty body still returns stdClass (unchanged 204/{} behaviour).
  • lib/Resource/PublicKeyResource.php: get() returns the bare PEM (trimmed, so it is accepted by openssl_verify() / ChipApi::verify()), and still accepts the legacy {"public_key": "…"} object body.
  • lib/Resource/BillingResource.php: create() return type follows the client (it returns the raw response).
  • CHANGELOG.md (Unreleased) and README.md (public-key example now shows the verify() round trip).
  • lib/Http/GuzzleClient.php: the two duplicated error-mapping blocks are now one describeError() that returns [status, body, message] with both values coerced (is_array($decoded) ? $decoded : [], and a list/object detail JSON-encoded into the message; otherwise the transport message is kept).
  • lib/Resource/{Webhooks,Statements,Billing,Clients}Resource.php: iterate() continues only when next is a non-empty string.
  • tests/ResponseShapeTest.php (new, 14 cases): array/object detail, empty-array and empty-string detail, message as a list, JSON scalar body, plain-text body, missing detail, next: false / next: "" on all five iterate() sites, and a normal string next page.
  • tests/ApiTest.php: two pre-existing dead locals ($body in the payment-methods test, $result in the turnover test) that PHPStan's current bleeding edge flags as never read are replaced with real assertions (GET payment_methods sends no body; turnover maps the response array). Without this the Static Analysis job fails on the PR for reasons unrelated to the fix.

Verification

  • composer test — 127 tests, 375 assertions, OK. New tests:
    • testGetPublicKey — JSON-encoded PEM body → exact PEM returned;
    • testPublicKeyFromApiVerifiesCallbackSignature — fetch the key, then ChipApi::verify() on the existing signed-payload fixture returns true;
    • testGetPublicKeyAcceptsPlainTextPemBody — bare PEM body;
    • testGetPublicKeyAcceptsObjectBody — legacy {"public_key": …} body.
  • The new tests bite: with the lib/ changes reverted, testGetPublicKey and testPublicKeyFromApiVerifiesCallbackSignature fail with exactly the reported TypeError, and the plain-text case returns ''. Same check for ResponseShapeTest: 5 of its 8 cases fail on the previous code with ChipApiException::__construct(): Argument #1 ($message) must be of type string, array given, Argument #3 ($responseBody) must be of type ?array, string given, RetryClient::request(): Argument #2 ($endpoint) must be of type string, false given, and an exhausted mock queue for next: "" (the infinite loop).
  • composer phpstan — no errors (level 8, PHPStan 2.3.1 with bleeding edge, same config CI uses).
  • composer cs-check — clean.
  • GitHub Actions on this PR: all 7 checks pass (Static Analysis, Code Style, PHP 8.1–8.5).
  • Artifact install check (what a consumer actually gets, not the working tree): a throwaway project resolves the branch as a Composer VCS dependency, installs chip/chip-sdk-php dev-fix/... 4fdb64d, and the installed package returns the PEM from publicKey->get() and raises a catchable ClientException for {"detail": ["a","b"]}.
  • Live check against the real gateway with a test-mode API key (read-only GET /public_key/):
    • raw response: HTTP/2 200, body = "-----BEGIN PUBLIC KEY-----\n…\n-----END PUBLIC KEY-----\n" — a JSON-encoded string, 638 bytes, exactly as documented;
    • unpatched SDK (main): publicKey->get() throws TypeError: Chip\Http\GuzzleClient::request(): Return value must be of type stdClass|array, string returned — the reported bug reproduces against the live API, not just in a mock;
    • this branch: get() returns a 624-byte PEM string; openssl_pkey_get_public() parses it (RSA, 3072-bit).
  • End-to-end callback signature verification against the live gateway (test mode): created a purchase with a success_callback pointing at a temporary receiver, marked it paid, and verified the delivered callback with this branch:
    • callback delivered, body 3777 bytes, X-Signature = 512-char base64;
    • payload id matches the purchase, status: paid, event_type: purchase.paid, is_test: true;
    • ChipApi::verify($rawBody, $signature, $chip->publicKey->get()) → true;
    • same signature against a tampered body → false (no false positive).

Potential Risks & Review Items

  • Return type widening (\stdClass|array → \stdClass|array|string) on ClientInterface::request() is additive and BC-safe for third-party implementations (narrower covariant return types stay valid). Callers that assumed only object/array responses and cast with (array) keep working; only PublicKeyResource actually had to change.
  • Behaviour change for non-JSON bodies: previously a plain-text 200 response was silently converted to an empty stdClass; it is now returned as a string. This is what makes the public key work, but it means a mis-behaving endpoint that returns text/HTML with a 200 could now surface a raw string to a resource that does (array) $response (it degrades to an empty/odd array rather than an exception). Empty bodies are unchanged.
  • PublicKeyResource::get() still returns '' when the body has neither a string nor a public_key field — callers should treat an empty result as a failure, as before. Worth deciding whether that should throw instead (out of scope for this fix).
  • Error-message text changes for the new shapes only: when detail/message is a list or object the message becomes its JSON encoding (e.g. ["first","second"]); when the body is a JSON scalar or plain text the exception keeps the transport message and getResponseBody() is [] rather than a type error. Nothing that previously worked changes text.
  • Pagination is now stricter: a next that is not a non-empty string ends the iteration instead of raising. A paginated response whose next is a bare relative path without a query string still works (any non-empty string is followed verbatim, as before); only false/null/"" changed behaviour.
  • No release/tag: version bump is left to the maintainers after review.

Release

The 2.1.1 section heading is already in CHANGELOG.md in this PR, so releasing needs no follow-up PR on main (which requires a reviewed PR): after merge it is a tag push only.

git checkout main && git pull
git tag -a v2.1.1 -m "v2.1.1" <merge-commit>
git push origin v2.1.1

Version facts: composer.json carries no version key (version = git tag + changelog heading) and .github/workflows/ci.yml has no changelog/release check, so nothing else needs editing. Semver: 2.1.1 (patch) — if a different number is wanted, only that one changelog heading changes before merge.

Scope / siblings

Only the PHP SDK is affected. The OpenAPI schema declares PublicKey: type: string, so the generated C#/Java SDKs already deserialize it as a string, and chip-go-sdk (json.Unmarshal into a string) and the Node example (JSON.parse) handle it. No sibling SDK change is needed.

wzul added 2 commits October 23, 2026 13:38
GET /public_key/ answers with a JSON-encoded PEM string. GuzzleClient
declared stdClass|array as its return type, so decoding that body threw
a TypeError and the public key could never be retrieved for callback
signature verification.

- widen ClientInterface/GuzzleClient/RetryClient request() to
  stdClass|array<string, mixed>|string (BC-safe: implementers may keep
  the narrower type via covariance)
- return decoded string bodies as-is; return plain-text bodies instead of
  discarding them; keep stdClass for empty bodies
- PublicKeyResource::get() returns the bare PEM (and still accepts the
  legacy {public_key: ...} object shape)
- BillingResource::create() return type follows the client
- tests: cover JSON-encoded PEM, plain-text PEM, legacy object body, and a
  full fetch-then-verify round trip

Fixes #18
PHPStan's bleeding edge flags $body (payment methods) and $result
(turnover) as never read. Replace them with real assertions so static
analysis passes: GET payment_methods sends no body, and turnover maps the
response array.
Folds the release heading into this PR so the release needs no follow-up
commit on main (main requires a reviewed PR): after merge it is a tag
push only -- `git tag v2.1.1 && git push origin v2.1.1`.

No version field exists in composer.json and ci.yml has no changelog
check, so the tag plus this heading are the whole version bump.
wzul added 2 commits October 23, 2026 17:53
…onses

Audited every path where a decoded response reaches a typed parameter or
return; three more TypeError shapes turned up beside the public-key one:

- error bodies: Detail/message as a list or object was passed straight to
  ChipApiException::__construct(string $message), and a JSON scalar body to
  its ?array $responseBody -- so a 4xx/5xx raised TypeError instead of a
  ChipApiException the caller can catch. Normalise both in describeError().
- pagination: iterate() passed the raw next value to request(string
  $endpoint); next:false (seen from some deployments) raised TypeError and
  next:"" looped forever. Only a non-empty string continues now, across
  webhooks, company_statements, billing_templates, billing clients, clients.

Regression tests in tests/ResponseShapeTest.php cover each shape; all five
fail with the reported TypeError (or an exhausted mock loop) on the
previous code.

Also folds the 2.1.1 changelog entries.
The previous commit referenced these tests but did not stage the new file.
Eight cases: array/object detail, JSON scalar body, plain-text body, missing
detail, next:false, next:"", and a normal string next page.
@wzul wzul changed the title fix: return public key PEM instead of throwing on string body fix: stop TypeError on non-object responses (public key, error bodies, pagination) Oct 23, 2026
@wzul
wzul requested a review from LiyanaAmil October 23, 2026 15:55
QA gap: the pagination fix touched five iterate() sites but only two had a
regression test. Adds the missing three (company_statements, billing
template clients, clients) and pins the describeError() edge cases:
detail as [], detail as "", and a message list when detail is absent.
An empty detail list no longer becomes the useless message "[]" -- it
falls back to the transport message.
@wzul
wzul merged commit fc04174 into main Oct 23, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Public key retrieval throws TypeError because HTTP client excludes string responses

2 participants