diff --git a/.gitattributes b/.gitattributes index 5f07965..f97bc28 100644 --- a/.gitattributes +++ b/.gitattributes @@ -6,6 +6,7 @@ /.php-cs-fixer.dist.php export-ignore /examples export-ignore /phpmd.xml export-ignore +/phpstan-examples.neon export-ignore /phpstan.neon export-ignore /phpunit.xml export-ignore /tests export-ignore diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 75868f4..052a933 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2,7 +2,8 @@ name: CI on: push: - branches: [ master ] + # Maintenance branches follow the 2.x naming Composer understands as 2.x-dev. + branches: [ master, '[0-9]+.x' ] pull_request: workflow_dispatch: @@ -20,7 +21,7 @@ jobs: strategy: fail-fast: false matrix: - php: ['7.4', '8.0', '8.1', '8.2', '8.3', '8.4'] + php: ['7.4', '8.0', '8.1', '8.2', '8.3', '8.4', '8.5'] steps: - uses: actions/checkout@v4 @@ -28,7 +29,7 @@ jobs: uses: shivammathur/setup-php@v2 with: php-version: ${{ matrix.php }} - extensions: json, curl + extensions: json, ctype, curl coverage: none - name: Install dependencies @@ -53,7 +54,7 @@ jobs: uses: shivammathur/setup-php@v2 with: php-version: '8.3' - extensions: json, curl + extensions: json, ctype, curl coverage: none - name: Install dependencies @@ -68,9 +69,12 @@ jobs: - name: Static analysis (PHPStan) run: composer stan + # examples/ sits outside phpstan.neon, and composer lint only syntax-checks it. + - name: Static analysis (examples) + run: composer stan-examples + - name: Mess detection (PHPMD) run: composer md - name: Security audit (composer) run: composer audit - continue-on-error: true diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml new file mode 100644 index 0000000..2bc02ac --- /dev/null +++ b/.github/workflows/release.yml @@ -0,0 +1,127 @@ +name: Release + +# Tag push -> GitHub release with the CHANGELOG section + a Packagist re-crawl. +# The Packagist GitHub hook already publishes new tags on its own; this ping is +# the fallback for when it lags or breaks (e.g. after a repository rename). +# Run via workflow_dispatch to ping Packagist without cutting a release. + +on: + push: + tags: + # Historic tags use both spellings: v1.1.2 and 3.0.0. + - 'v[0-9]+.[0-9]+.[0-9]+*' + - '[0-9]+.[0-9]+.[0-9]+*' + workflow_dispatch: + +permissions: + contents: write + +concurrency: + group: release-${{ github.ref }} + cancel-in-progress: false + +jobs: + release: + name: GitHub release & Packagist + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + # Full history: the release step compares this tag against every other + # one to decide whether it is really the newest version. + fetch-depth: 0 + + - name: Set up PHP + if: startsWith(github.ref, 'refs/tags/') + uses: shivammathur/setup-php@v2 + with: + php-version: '8.3' + coverage: none + + - name: Verify Unitpay::VERSION matches the tag + if: startsWith(github.ref, 'refs/tags/') + run: | + set -euo pipefail + declared=$(php -r 'require "src/Unitpay.php"; echo Unitpay\Unitpay::VERSION;') + expected="${GITHUB_REF_NAME#v}" + if [ "$declared" != "$expected" ]; then + echo "::error::Tag ${GITHUB_REF_NAME} declares version '${expected}' but Unitpay::VERSION is '${declared}'. Bump the constant and retag." + exit 1 + fi + echo "Unitpay::VERSION=${declared} matches tag ${GITHUB_REF_NAME}" + + - name: Extract the CHANGELOG section for this tag + if: startsWith(github.ref, 'refs/tags/') + run: | + set -euo pipefail + # CHANGELOG headings are always "### v3.0.0 — 2026-07-25"; tags are not + # always prefixed, so normalise 3.0.0 and v3.0.0 to the same lookup. + heading="### v${GITHUB_REF_NAME#v} " + awk -v h="$heading" 'index($0, h) == 1 { found = 1; next } found && /^### / { exit } found' \ + CHANGELOG.md > release-notes.md + if [ ! -s release-notes.md ]; then + echo "::warning::No CHANGELOG section matching '${heading}' — falling back to generated notes" + fi + + - name: Create the GitHub release + if: startsWith(github.ref, 'refs/tags/') + env: + GH_TOKEN: ${{ github.token }} + run: | + set -euo pipefail + if gh release view "$GITHUB_REF_NAME" >/dev/null 2>&1; then + echo "Release $GITHUB_REF_NAME already exists, leaving it untouched" + exit 0 + fi + # GitHub picks "Latest" by tag date, so a 2.x backport cut after 3.0.0 + # would steal the badge. Strip the optional v prefix before comparing, + # otherwise git sorts v1.1.2 above 3.0.0. + highest=$(git tag | sed 's/^v//' | sort -V | tail -1) + if [ "${GITHUB_REF_NAME#v}" = "$highest" ]; then latest=true; else latest=false; fi + echo "Highest tag in the repository: $highest (this release --latest=$latest)" + if [ -s release-notes.md ]; then + gh release create "$GITHUB_REF_NAME" --title "$GITHUB_REF_NAME" --latest="$latest" --notes-file release-notes.md + else + gh release create "$GITHUB_REF_NAME" --title "$GITHUB_REF_NAME" --latest="$latest" --generate-notes + fi + + - name: Ask Packagist to re-crawl the repository + env: + PACKAGIST_USERNAME: ${{ vars.PACKAGIST_USERNAME }} + PACKAGIST_TOKEN: ${{ secrets.PACKAGIST_TOKEN }} + run: | + set -euo pipefail + if [ -z "${PACKAGIST_USERNAME:-}" ] || [ -z "${PACKAGIST_TOKEN:-}" ]; then + echo "PACKAGIST_USERNAME (variable) or PACKAGIST_TOKEN (secret) is missing" >&2 + exit 1 + fi + # The safe token is enough here: update-package is the one write endpoint + # it may call. Bearer auth keeps the token out of the request URL. + response=$(curl -sS --fail-with-body \ + --retry 3 --retry-delay 5 --retry-all-errors \ + -X POST \ + -H 'Content-Type: application/json' \ + -H "Authorization: Bearer ${PACKAGIST_USERNAME}:${PACKAGIST_TOKEN}" \ + -A "unitpay-php-release (+https://github.com/${GITHUB_REPOSITORY})" \ + -d "{\"repository\":\"https://github.com/${GITHUB_REPOSITORY}\"}" \ + https://packagist.org/api/update-package) + echo "$response" + echo "$response" | jq -e '.status == "success"' >/dev/null + + - name: Wait for the version to appear on Packagist + if: startsWith(github.ref, 'refs/tags/') + run: | + set -euo pipefail + package=$(jq -r .name composer.json) + version="${GITHUB_REF_NAME#v}" + # Packagist queues the crawl, so this is a poll, not an immediate check. + for _ in $(seq 1 12); do + if curl -sS "https://repo.packagist.org/p2/${package}.json" \ + | jq -e --arg p "$package" --arg v "$version" \ + '[.packages[$p][].version] | any(. == $v or . == "v" + $v)' >/dev/null; then + echo "$package $version is live on Packagist" + exit 0 + fi + sleep 10 + done + echo "::warning::$package $version is not on Packagist yet — check https://packagist.org/packages/$package" diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f1189f..7f93263 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,41 @@ # Changelog +### v4.0.0 — 2026-07-30 + +**Breaking release.** The transport contract changed: `Unitpay\Http\TransportInterface` now returns a `Response` object instead of `string|false`, and inbound webhooks older than the tolerance window are rejected. If you implement your own transport, see [docs/migration-v4.md](docs/migration-v4.md); if you only use the SDK's own transport, the upgrade is a version bump. + +> **3.1.0 was never published.** The BC-safe tech-debt batch prepared under that number ships here instead, so this entry covers both it and the 4.0 work. Nothing was dropped. + +**Breaking changes.** + +* **`TransportInterface::send(): string|false` became `request(): Response`.** The old contract threw away the HTTP status, the response headers and the cURL error, so a connect failure, a read timeout, a 404, a 500 with an HTML body, a disabled `allow_url_fopen` and malformed JSON all reached the caller as the same `UnitpayTransportException('Temporary server error. Please try again later.')` — including the two that are permanent rather than temporary. `Unitpay\Http\Response` carries the status, body, headers, errno and a `wasRequestSent()` flag. Only custom transport implementations are affected; the before/after is in [docs/migration-v4.md](docs/migration-v4.md) +* **Transport failures are now typed.** `UnitpayTransportException` stays the base and still extends `InvalidArgumentException`, so existing catch blocks keep working, but it now has three subclasses that say what actually happened: `UnitpayNetworkException` (no response, with `getErrno()` / `getTransportError()`), `UnitpayHttpException` (non-2xx, with `getStatusCode()` / `getResponseBody()`) and `UnitpayResponseException` (2xx whose body is not a JSON object). The blanket "Temporary server error" message is gone — if you matched on it, match on the class +* **The client fingerprint header was renamed `X-Unitpay-Client` → `Unitpay-Client`.** [RFC 6648](https://www.rfc-editor.org/rfc/rfc6648) retired the `X-` convention in 2012, and four more SDKs are planned against this same header — fixing the name after five implementations ship would leave a permanently two-named protocol. The fixed fields inside it are unchanged; the new `module` and `stack` fields carry name and version as separate keys rather than a joined `Name/Version` string, because a product name may itself contain a `/` and the most natural module name to pass is the Composer package name (`setModule('unitpay/woocommerce', '2.1')` would otherwise produce `unitpay/woocommerce/2.1`: three segments, one delimiter). The `User-Agent` keeps the readable joined token because that field cannot carry structure. This affects you only if something on your side reads the SDK's own outgoing headers. The SDK sends one name, not both — but 2.x and 3.x installs keep sending the old one, so a consumer has to accept both for as long as those are in the field +* **Inbound webhooks older than 300 seconds are rejected** with `UnitpayReplayException`. A signature proves a webhook was genuine once, not that it is genuine now: until this release a captured request replayed indefinitely. `params[date]` is parsed against a fixed **UTC+3** offset rather than with `strtotime()`, which would resolve it against the ambient `date.timezone` and make the same webhook pass on a Moscow server and fail on a UTC one. `UnitpayReplayException` extends `UnitpaySignatureException`, so a handler that already rejects a bad signature needs no change. Widen or disable the window with `webhook()->setWebhookTolerance()`. A webhook carrying no `date` is accepted: the field is inside the signed payload, so an attacker cannot strip it to skip the check +* **Five deprecated `PaymentObject` values were removed:** `EXCISE`, `GAMBLING_BET`, `GAMBLING_PRIZE`, `LOTTERY_PRIZE` and `COMPOSITE`. The public API rejects all five, so no receipt has ever fiscalized with one; they were announced for removal in 2.1.0 (for 3.0), kept once to avoid a second migration alongside the namespace break, and re-slated for 4.0 — this is that removal. `CashItem` does not validate its `$type` against a whitelist, so if you pass the raw string `'excise'` nothing in the SDK changes for you: the backend rejects it exactly as before. Only code naming the constant breaks, and it breaks loudly at load time (`Error: Undefined constant`) rather than silently at fiscalization. Pick a supported value from `Unitpay\Model\Enum\PaymentObject`; the grep and the reasoning are in [docs/migration-v4.md](docs/migration-v4.md) + +**New.** + +* **Network retries, enabled by default.** `RetryingTransport` repeats an attempt only when it provably never reached Unitpay — DNS failure, refused connection, connect-phase timeout — up to twice with capped exponential backoff and jitter. It does **not** retry a read timeout, a 5xx, a 409 or a 429. That is narrower than what other payment SDKs do, and deliberately so: they can attach an idempotency key that makes a repeat harmless, and the Unitpay API accepts none, so repeating a delivered `initPayment` can create a second payment. Disable with `DefaultTransport::withoutRetries()`. The `file_get_contents` fallback is never retried — it cannot report which phase failed +* **Configurable transport timeouts:** `new CurlTransport($connectTimeout, $timeout)`, defaulting to the previous 5s and 10s. A non-positive value is rejected at construction, since cURL reads 0 as "wait forever" +* **Integration identity in telemetry:** `setModule('unitpay-woocommerce', '2.1')` names what you wrote, and `setStack(['WordPress' => '6.5', 'WooCommerce' => '8.2'])` names what it runs on, outermost host first. Both ride in `User-Agent` and `Unitpay-Client`; the fixed fingerprint could not tell a shipped module from a bare script. The division deliberately asks nothing of you taxonomically — a stack entry is a known product a server can categorise from its name, and a module name is arbitrary, which is why it keeps a field of its own. That also makes stacks expressible that a fixed CMS/framework/module triple could not: WordPress + WooCommerce + plugin, Bitrix + an Aspro solution + module, OpenCart + ocStore. The runtime is not part of the stack — `lang_version` and `platform` are still filled in by the SDK. `setStack()` replaces rather than appends, so it is idempotent; an associative array rules out duplicate names and preserves order, which is presentation only. `disableTelemetry()` drops `Unitpay-Client` entirely while the `User-Agent` keeps naming the SDK. Values are product names and versions supplied by the integrator — still no PII and still no extra request +* **Telemetry can never break a request.** These setters run in an integration's bootstrap, so none of them raises on the values you pass: a blank name or version is ignored rather than rejected (a CMS that stops exposing its version string costs a header field, not a checkout — and in a stack only that entry is dropped, not its neighbours), control characters are stripped and each half is capped at 64/32 bytes, at most eight stack entries are sent, and a value that is not valid UTF-8 no longer blanks the whole payload — `json_encode` used to return `false` there and the cast turned that into an empty header, so one legacy windows-1251 CMS name cost `sdk_version` and `lang_version` too. A non-ASCII name rides in the JSON header only, since a `User-Agent` cannot carry raw UTF-8. `CurlTransport::sanitizeHeaders()` drops any header line carrying a CR or LF as a second line of defence: the stream path joins header lines with `\r\n`, so a newline inside a slot value used to add a header line of the caller's choosing +* **Documented how to log a failure without logging your secret key.** The API takes `secretKey` as a query parameter, so it sits in the arguments of the stack frames: `$e->getTrace()` returns it in full when `zend.exception_ignore_args` is switched off (PHP defaults it to `1`, where every channel is clean, and `getMessage()` is clean either way). This is not new — it is as old as the API contract — but 4.0 is the release that gives you typed accessors worth logging instead. See [docs/api-methods.md](docs/api-methods.md) + +**Behavior changes carried over from the unreleased 3.1.0.** Three settings that used to fail silently now either bind correctly or say what is wrong. Nothing changed on the wire. + +* **Fluent setter params bind to the calls that accept them, not to whichever call runs first.** `AbstractService::request()` used to drain the accumulated params on every method, so `setCashItems()` followed by any lookup sent the 54-FZ receipt with that lookup — where it is meaningless — and the payment created right after it carried no receipt at all. Nothing reported the loss. The params are now read and cleared only by `form()`, `payments()->initPayment()` and `payments()->offsetAdvance()`; every other service call leaves them untouched, so a receipt survives an intervening lookup and still reaches its payment. A consuming call still clears them even when the request fails, so a retry must re-apply the setters. If your code relied on a receipt reaching a non-payment method, it was already being discarded before the payment — the fix makes it arrive instead +* **`setAllowedIps()` and `addAllowedIps()` reject malformed entries** with `UnitpayValidationException` instead of accepting them. A typo such as `31.186.100.4 9` or a `/33` prefix produced an allowlist that matched nothing, so every webhook was refused with a bare `IP address Error` and no hint at the cause. The whole call is rejected rather than the offending entry alone, so the allowlist is never left half-configured. `refreshAllowedIps()` is unchanged and still fail-safe: it drops bad feed entries and never throws +* **The constructor `$domain` must be a bare host** (optionally with a port) and is validated at construction time, throwing `UnitpayValidationException` otherwise. It feeds three URLs — the API endpoint, the hosted form and the webhook IP feed — so a value carrying a scheme or a path (`https://unitpay.ru`, `unitpay.ru/api`) used to produce malformed URLs whose failure surfaced much later as a transport error or an allowlist that quietly failed to refresh +* QA: PHPStan raised from level 6 to **level 8**, with no baseline and no suppressions. The three findings in `src/` were real type smells — the transport result reached `json_decode` as `string|false`, `curl_exec()` was returned as `bool|string` against a `string|false` signature, and `makeService()` returned a four-way union that each service getter narrowed without proof. Behavior is unchanged in all three +* QA: the PHPMD ruleset was still the one written for the pre-3.0 single-file class, lifting `ExcessiveClassComplexity` to 60 and `NPathComplexity` to 300 and excluding five further rules. Measured against the current `src/`, none of those five fires and the class-complexity default is never reached, so they were dropped. What remains is only what actually fires, each named in a comment; the two thresholds `checkHandlerRequest()` needs now sit just above its measured values +* **`ext-ctype` is now declared** in `composer.json`. `IpAllowlist` has always called `ctype_digit()` unconditionally, and unlike `ext-json` — compiled in unconditionally since PHP 8.0 — `ctype` is a bundled extension a build can still drop with `--disable-ctype`, so the dependency was real and undeclared. This is a stricter install constraint, which is why it lands in a major rather than a patch. Still no Composer packages at runtime, and `ext-curl` stays in `suggest`: `CurlTransport` keeps its `file_get_contents()` fallback +* QA: a new `ComposerRequirementsTest` holds the manifest to the code. It scans `src/` with the tokenizer for calls into extensions a build can disable and asserts each one is declared in `require` (or in `suggest`, which is `ext-curl` and only `ext-curl`), and it asserts `require` contains nothing but `php` and `ext-*`. The zero-Composer-dependency stance and the extension list are now regressions a test catches, not conventions a reviewer has to remember +* QA: `examples/` is analysed by `composer check` and by CI, through the new `composer stan-examples` script and `phpstan-examples.neon`. `examples/` is the source of truth for usage samples but sat outside `phpstan.neon`, and `composer lint` only syntax-checks it, so a renamed method or a changed argument count there stayed invisible while the whole gate was green. It runs at level 2 — method existence is only checked on expressions from that level up, and the examples call everything through a variable, never through `$this`, so level 0 misses precisely the regressions this guard exists for. The single `variable.undefined` identifier is suppressed, because every example pulls its variables in through `require` of `config.php` / `order.php`, which PHPStan does not follow; nothing else is +* QA: both PHPStan scripts pin `memory_limit=512M`. PHP's 128M default aborts PHPStan's parallel worker, which used to make a bare `composer check` fail for an environment reason that looked like a code finding +* CI: added PHP **8.5** to the test matrix, which now covers the whole declared `>=7.4` range, and `composer audit` no longer swallows failures +* CI: a tagged release verifies that `Unitpay::VERSION` matches the tag before publishing anything — the telemetry tests assert against the constant itself, so a forgotten bump used to stay green and ship the wrong version in `User-Agent`, `Unitpay-Client` and the form's `sdk` parameter. Release tags are unprefixed (`3.1.0`), the form used since 2.0.0 + ### v3.0.0 — 2026-07-25 **Breaking release.** The SDK is no longer a single file in the global namespace: it is now a PSR-4 package (`Unitpay\` → `src/`) split into layers — Http, Api services, Signature, Webhook, Model/Enum, Exception — behind a thin `Unitpay\Unitpay` facade. Nothing changed on the wire; only the PHP surface you call. Step-by-step renames are in [docs/migration-v3.md](docs/migration-v3.md). diff --git a/README.md b/README.md index 57b9261..633d8c5 100644 --- a/README.md +++ b/README.md @@ -14,16 +14,22 @@ Everything hangs off one entry point, `Unitpay\Unitpay`, which hands out service Official Unitpay documentation: [help.unitpay.ru](https://help.unitpay.ru) -> **Upgrading from 2.x?** 3.0 moves every class into the `Unitpay\` namespace and replaces +> **Upgrading from 3.x?** 4.0 changes the `TransportInterface` contract and starts +> rejecting webhooks older than 5 minutes. If you use the SDK's own transport and your +> server clock is synchronised, it is a version bump — see the +> [v4 Migration Guide](docs/migration-v4.md). +> +> **Upgrading from 2.x?** 3.0 moved every class into the `Unitpay\` namespace and replaced > `api('method', [...])` with typed service methods. There is no compatibility shim — -> see the [v3 Migration Guide](docs/migration-v3.md). +> see the [v3 Migration Guide](docs/migration-v3.md), then the v4 guide. ## Requirements * PHP >= 7.4 * ext-json +* ext-ctype -No runtime dependencies. `ext-curl` is optional: the default transport uses it when +No Composer dependencies. `ext-curl` is optional: the default transport uses it when present and falls back to `file_get_contents()` otherwise. ## Installation @@ -78,7 +84,7 @@ Prefer a server-to-server call? Use `$unitpay->payments()->initPayment(...)` — * **Swappable transport** — inject any `Unitpay\Http\TransportInterface` to plug in your own HTTP stack or to test without the network. * **Typed exceptions** — all implement `UnitpayExceptionInterface`. -* **Zero dependencies** — `ext-json` only (`ext-curl` optional). +* **Zero dependencies** — no Composer packages; `ext-json` + `ext-ctype` (`ext-curl` optional). ## Documentation @@ -88,7 +94,8 @@ Prefer a server-to-server call? Use `$unitpay->payments()->initPayment(...)` — | [Fiscal Receipts](docs/receipts.md) | 54-FZ receipt line items via `CashItem` | | [API Methods](docs/api-methods.md) | Full service reference and account-level calls | | [Webhooks](docs/webhooks.md) | Payment handler + keeping the IP allowlist fresh | -| [Telemetry](docs/telemetry.md) | Anonymous SDK version fingerprint | +| [Telemetry](docs/telemetry.md) | Anonymous SDK fingerprint and naming your integration | +| [v4 Migration Guide](docs/migration-v4.md) | Upgrading from 3.x to 4.0 | | [v3 Migration Guide](docs/migration-v3.md) | Upgrading from 2.x to 3.0 | Runnable samples for every method group live in [`examples/`](examples). diff --git a/composer.json b/composer.json index 17b8a89..a1f8b24 100644 --- a/composer.json +++ b/composer.json @@ -13,7 +13,8 @@ ], "require":{ "php": ">=7.4", - "ext-json": "*" + "ext-json": "*", + "ext-ctype": "*" }, "require-dev": { "phpunit/phpunit": "^9.6", @@ -38,7 +39,8 @@ "scripts": { "test": "phpunit", "lint": "parallel-lint src examples tests", - "stan": "phpstan analyse --no-progress", + "stan": "@php -d memory_limit=512M vendor/bin/phpstan analyse --no-progress", + "stan-examples": "@php -d memory_limit=512M vendor/bin/phpstan analyse --no-progress --configuration=phpstan-examples.neon", "cs-check": "php-cs-fixer fix --dry-run --diff", "cs-fix": "php-cs-fixer fix", "md": "@php -d error_reporting=\"E_ALL & ~E_DEPRECATED\" vendor/bin/phpmd src text phpmd.xml", @@ -46,6 +48,7 @@ "@lint", "@cs-check", "@stan", + "@stan-examples", "@md", "@test" ] @@ -53,10 +56,11 @@ "scripts-descriptions": { "test": "Run the PHPUnit test suite", "lint": "Syntax-lint every PHP file in parallel", - "stan": "Run PHPStan static analysis", + "stan": "Run PHPStan static analysis on src/ and tests/ (level 8)", + "stan-examples": "Run PHPStan over examples/ (level 2) to catch a usage sample broken by a public-surface change", "cs-check": "Report code-style violations without changing files", "cs-fix": "Apply code-style fixes in place", "md": "Run PHPMD mess detection on src/", - "check": "Run lint, cs-check, stan, md and test in sequence" + "check": "Run lint, cs-check, stan, stan-examples, md and test in sequence" } } diff --git a/docs/api-methods.md b/docs/api-methods.md index bf3f0be..2bfcaad 100644 --- a/docs/api-methods.md +++ b/docs/api-methods.md @@ -7,8 +7,55 @@ takes its required parameters as arguments and everything else in a trailing opt array. `secretKey` is added automatically from the constructor. Full parameters and response formats are in the [official API documentation](https://help.unitpay.ru). -Every method returns the decoded JSON envelope as `object`, and throws a -`UnitpayTransportException` when no usable response comes back. +Every method returns the decoded JSON envelope as `object`. When no usable response comes +back it throws one of three exceptions, all extending `UnitpayTransportException` — so a +single `catch (UnitpayTransportException $e)` still covers everything, while the concrete +class tells you what to do next: + +| Exception | Raised when | Carries | Safe to repeat? | +| --- | --- | --- | --- | +| `UnitpayNetworkException` | no response arrived — DNS, refused connection, timeout, or `allow_url_fopen` disabled with no ext-curl | `getErrno()`, `getTransportError()` | only if the message says the request was not sent | +| `UnitpayHttpException` | Unitpay answered with a non-2xx status | `getStatusCode()`, `getResponseBody()` | no — it was processed far enough to produce a status | +| `UnitpayResponseException` | a 2xx arrived whose body is not a JSON object | `getStatusCode()`, `getResponseBody()` | no — the call was delivered and accepted | + +`getResponseBody()` keeps whatever came back, including the HTML error page a gateway +returns on a 502. That is what Unitpay support will ask you to quote — put it in your log, +not on the page, since an upstream error body can name internal hosts. + +A missing or empty secret key still throws `UnitpayValidationException` before any request +is made. + +## Logging these exceptions without logging your key + +The Unitpay API takes `secretKey` as a query parameter, so the key travels through the SDK +as an ordinary argument. It never reaches `getMessage()`, and it is never written to the +error log — but it does sit in the *arguments* of the stack frames, and PHP can be +configured to keep those: + +| | `zend.exception_ignore_args=1` (PHP's default since 7.4) | `zend.exception_ignore_args=0` | +| --- | --- | --- | +| `getMessage()`, the typed accessors | safe | safe | +| `getTraceAsString()`, `(string) $e` | safe | leaks the first 15 characters when the key was a direct string argument — `new Unitpay(...)`, `SignatureBuilder::build()` | +| `getTrace()` | safe | **leaks the whole key**, which travels inside the `$params` array | + +So: + +```php +// ✅ Safe on any configuration. +myLogger()->error($e->getMessage(), ['status' => $e->getStatusCode()]); + +// ❌ Dumps the secret key verbatim when zend.exception_ignore_args=0. +myLogger()->error('Unitpay failed', ['trace' => $e->getTrace()]); +``` + +Check the setting before you trust a handler you did not write: error trackers such as +Sentry and Bugsnag, and Monolog's `IntrospectionProcessor`, serialize frame arguments when +PHP gives them any. Either leave `zend.exception_ignore_args` at its default of `1`, or add +`secretKey` to the tracker's scrubbing list. + +This is a property of the API contract rather than of the SDK: the key is a request +parameter, so anything that records the request records it. The same applies to your +access logs if you ever proxy these calls. ## `payments()` @@ -88,10 +135,33 @@ Note: `confirmPayment` and `cancelPayment` return a top-level `message` ## Fluent parameters Parameters accumulated by `setCashItems()`, `setCustomerEmail()`, `setCustomerPhone()` and -`setBackUrl()` are merged into the next call — `form()` or any service method — and cleared -afterwards, so a reused instance never carries one order's receipt into the next. Explicit -options take precedence over accumulated ones. The clearing happens even when the request -fails, so a retry must re-apply the setters. +`setBackUrl()` are merged into the calls that accept them, and cleared afterwards — so a +reused instance never carries one order's receipt into the next. Explicit options take +precedence over accumulated ones. The clearing happens even when the request fails, so a +retry must re-apply the setters. + +Only three calls consume them: + +| Call | Consumes | +| --- | --- | +| `form()` | `backUrl`, `customerEmail`, `customerPhone`, `cashItems` | +| `payments()->initPayment()` | `backUrl`, `customerEmail`, `customerPhone`, `cashItems` | +| `payments()->offsetAdvance()` | `cashItems` | + +Every other service method — `getPayment`, `refundPayment`, `confirmPayment`, +`cancelPayment` and everything on `subscriptions()`, `payouts()` and `reference()` — neither +receives nor clears them. A lookup issued between the setters and the payment therefore +leaves the receipt alone: + +```php +$unitpay->setCashItems([$item]); + +$unitpay->reference()->getPartner($login); // no receipt attached, nothing consumed +$unitpay->payments()->initPayment(...); // the receipt arrives here +``` + +> Before 3.1 every service call drained these params, so the lookup above sent the receipt +> with `getPartner` and the payment was created without one. ## See Also diff --git a/docs/getting-started.md b/docs/getting-started.md index 56e0bfa..0d72424 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -6,8 +6,9 @@ * PHP >= 7.4 * ext-json +* ext-ctype -No runtime dependencies. The SDK is a PSR-4 package: namespace `Unitpay\` maps to `src/`, +No Composer dependencies. The SDK is a PSR-4 package: namespace `Unitpay\` maps to `src/`, with `Unitpay\Unitpay` as the single entry point. `ext-curl` is optional — the default transport uses it when present and falls back to `file_get_contents()` otherwise. @@ -56,11 +57,44 @@ a `TransportInterface`, the inbound request array, and the client IP: new Unitpay(string $domain, ?string $secretKey = null, ?TransportInterface $transport = null, ?array $request = null, ?string $clientIp = null) ``` +`$domain` is a **bare host** — `unitpay.ru`, or the address Unitpay support gave you — with +an optional `:port`. No scheme, path or query: it is interpolated into the API endpoint, the +hosted-form URL and the webhook IP feed, so `https://unitpay.ru` or `unitpay.ru/api` would +produce broken URLs. Anything else throws `UnitpayValidationException` from the constructor. + +### Transport, timeouts and retries + +Leave `$transport` null and you get `DefaultTransport::create()`: cURL (falling back to +`file_get_contents` when `ext-curl` is missing) behind a retry policy. Override it to tune +either half: + +```php +use Unitpay\Http\CurlTransport; +use Unitpay\Http\DefaultTransport; +use Unitpay\Http\RetryingTransport; + +// Longer read timeout, still retried. +new Unitpay('unitpay.ru', $secretKey, new RetryingTransport(new CurlTransport(5, 30))); + +// No retries — fail on the first attempt. +new Unitpay('unitpay.ru', $secretKey, DefaultTransport::withoutRetries()); +``` + +Both timeouts are in seconds and must be positive; cURL treats 0 as "wait forever", so it +is rejected at construction rather than at the first request. + +**What gets retried is narrow on purpose.** Only a failure that provably never left the +client — DNS failure, refused connection, connect-phase timeout — is repeated. A read +timeout, a 5xx, a 409 and a 429 are not, because Unitpay may already have acted on the +request and the API accepts no idempotency key to make a repeat harmless. The +`file_get_contents` fallback is never retried: it cannot tell the phases apart. + ## Create a payment (Unitpay hosted form) `form()` builds a signed redirect URL to Unitpay's hosted payment page. Fluent setters (`setBackUrl`, `setCustomerEmail`, `setCustomerPhone`, `setCashItems`) are optional and -apply to both `form()` and the service calls. +apply to `form()`, `payments()->initPayment()` and `payments()->offsetAdvance()` — see +[Fluent parameters](api-methods.md#fluent-parameters). ```php result->type) ## Testing without the network -The transport sits behind `Unitpay\Http\TransportInterface`, so you can inject a fake and +The transport sits behind `Unitpay\Http\TransportInterface` — one method, +`request(string $url, array $headers = []): Response` — so you can inject a fake and exercise the SDK without HTTP. The webhook verifier likewise accepts the inbound request array and the client IP, instead of reading `$_GET` / `$_SERVER['REMOTE_ADDR']`: diff --git a/docs/migration-v4.md b/docs/migration-v4.md new file mode 100644 index 0000000..cad8626 --- /dev/null +++ b/docs/migration-v4.md @@ -0,0 +1,248 @@ +# Migrating to v4.0 + +[← Getting Started](getting-started.md) · [Back to README](../README.md) · [Migrating to v3 →](migration-v3.md) + +Four things break in 4.0, and only one of them affects most integrations. + +| Change | Affects you if… | Effort | +| --- | --- | --- | +| `TransportInterface` returns a `Response` | you wrote your own transport | ~10 lines | +| Stale webhooks are rejected | your handler runs more than 5 minutes behind, or your clock drifts | one call, or nothing | +| `X-Unitpay-Client` renamed to `Unitpay-Client` | something on your side reads the SDK's own outgoing headers | one rename | +| Five deprecated `PaymentObject` values removed | you name `EXCISE`, `GAMBLING_BET`, `GAMBLING_PRIZE`, `LOTTERY_PRIZE` or `COMPOSITE` | one constant, or nothing | + +Everything else is additive: configurable timeouts, retries for requests that never left +the client, and the telemetry identity calls. If you use the SDK's own transport and your clock +is synchronised, upgrading is a version bump. + +Coming from 2.x? Do [the v3 migration](migration-v3.md) first — that one moved every class +into the `Unitpay\` namespace. + +## 1. Custom transports: `send()` → `request()` + +The old contract returned `string|false`, which discarded the HTTP status, the response +headers and the cURL error. That is why every failure used to arrive as the same +"Temporary server error", and it is why the SDK could not tell a safe retry from a +dangerous one. + +```diff + final class MyTransport implements Unitpay\Http\TransportInterface + { +- public function send(string $url, array $headers = []) ++ public function request(string $url, array $headers = []): Unitpay\Http\Response + { +- $body = my_http_get($url, $headers); +- +- return $body === null ? false : $body; ++ $result = my_http_get($url, $headers); ++ ++ if ($result === null) { ++ // No response. The third argument is the important one — see below. ++ return Unitpay\Http\Response::failed(0, 'my transport failed', false); ++ } ++ ++ return Unitpay\Http\Response::received($result->status, $result->body, $result->headers); + } + } +``` + +`Response` is built through two named constructors, so a "failed" result cannot +accidentally carry an HTTP 200: + +* `Response::received(int $status, string $body, array $headers = [])` — a response + arrived, whatever its status. +* `Response::failed(int $errno, string $error, bool $requestSent)` — no response arrived. + +### The failure arguments are a safety decision, not a detail + +The retry policy reads them, and the Unitpay API accepts no idempotency key: repeating a +delivered `initPayment` can create a second payment. An attempt is repeated only on +positive knowledge that it never left the client. + +`$requestSent` states whether the request had already gone out. Pass `true` when you know +the bytes went out — a read timeout, for example. Do not infer it from an error code: cURL +reports the same `CURLE_OPERATION_TIMEDOUT` for a connect timeout and a read timeout, and +`CurlTransport` has to consult `CURLINFO_CONNECT_TIME` to tell them apart. + +**If you cannot tell, say so with the errno rather than the boolean:** + +```php +// "Something failed and I cannot classify it." Never retried. +return Response::failed(Response::ERRNO_LOCAL, 'my transport cannot report why', false); + +// "Nothing left this machine." Safe to retry. +return Response::failed(CURLE_COULDNT_CONNECT, 'connection refused', false); +``` + +`$requestSent = false` alone means *provably* not sent, which is a claim strong enough to +license a retry — it is not the place to express uncertainty. `ERRNO_LOCAL` is: it marks a +failure the transport could not classify, and the SDK never retries one. The SDK's own +`file_get_contents` fallback uses it for exactly this reason, since it sees no +connect/read phase. + +Your transport should not throw. Describe the outcome in a `Response`; the SDK decides +which exception it deserves. + +## 2. Failures are now typed + +`UnitpayTransportException` is still thrown and still extends `InvalidArgumentException`, +so an existing `catch` keeps working. What changed is that it is now a base class with +three concrete cases underneath and accessors that carry the detail: + +| Class | Raised when | Useful accessors | +| --- | --- | --- | +| `UnitpayNetworkException` | no response arrived — DNS, refused, timeout, or a local misconfiguration | `getErrno()`, `getTransportError()` | +| `UnitpayHttpException` | a response arrived with a non-2xx status | `getStatusCode()`, `getResponseBody()` | +| `UnitpayResponseException` | a 2xx arrived whose body is not a JSON object | `getStatusCode()`, `getResponseBody()` | + +```php +use Unitpay\Exception\UnitpayHttpException; +use Unitpay\Exception\UnitpayNetworkException; +use Unitpay\Exception\UnitpayTransportException; + +try { + $response = $unitpay->payments()->getPayment($paymentId); +} catch (UnitpayHttpException $e) { + // Unitpay answered and refused. The body is what support will ask you for. + myLogger()->error('Unitpay HTTP ' . $e->getStatusCode(), ['body' => $e->getResponseBody()]); +} catch (UnitpayNetworkException $e) { + // Nothing came back. The message says whether the request was actually sent. + myLogger()->error('Unitpay unreachable: ' . $e->getMessage()); +} catch (UnitpayTransportException $e) { + // Still catches everything, including the malformed-payload case. +} +``` + +One message is gone deliberately: **"Temporary server error. Please try again later."** It +was also used for a disabled `allow_url_fopen`, which is permanent. If you match on that +string, match on the exception class instead. + +Note what the examples above log: `getMessage()` and the typed accessors, never +`$e->getTrace()`. The secret key is a query parameter of this API, so it sits in the +arguments of the stack frames — harmless under PHP's default `zend.exception_ignore_args=1`, +and dumped verbatim by `getTrace()` when that is switched off. This is not new in 4.0, but +4.0 is the release that gives you something better to log; see +[Logging these exceptions without logging your key](api-methods.md#logging-these-exceptions-without-logging-your-key). + +## 3. Webhooks: the replay window + +`checkHandlerRequest()` now also checks that `params[date]` is within **300 seconds** of +your server clock, and throws `UnitpayReplayException` when it is not. Without it a +captured webhook stays replayable for as long as the secret key lives — the signature says +a request was genuine once, not that it is genuine now. + +`UnitpayReplayException` extends `UnitpaySignatureException`, so a handler that already +rejects on a bad signature rejects a replay too, with no code change. + +**Two things can break here, both worth fixing rather than tolerating:** + +* **An unsynchronised server clock.** `date` is Moscow time (UTC+3) and the SDK parses it + against that fixed offset, so your server's timezone does not matter — but its clock + does. Run NTP. +* **A handler behind a slow queue.** If webhooks can sit for minutes before + `checkHandlerRequest()` runs, verify on receipt rather than on processing. + +If neither is fixable right now: + +```php +$unitpay->webhook()->setWebhookTolerance(900); // 15 minutes +$unitpay->webhook()->setWebhookTolerance(0); // off — replayable forever +``` + +A webhook that carries no `date` at all is accepted. It is inside the signed payload, so +an attacker cannot strip it to skip the check — removing any parameter breaks the +signature, which is verified first. + +## 4. The client header was renamed + +`X-Unitpay-Client` is now `Unitpay-Client`. The fixed fields inside it are unchanged: + +```diff +-X-Unitpay-Client: {"sdk_version":"3.0.0","api_version":"v1","lang":"php", ...} ++Unitpay-Client: {"sdk_version":"4.0.0","api_version":"v1","lang":"php", ...} +``` + +The `X-` prefix was retired by [RFC 6648](https://www.rfc-editor.org/rfc/rfc6648) in 2012. + +Two optional fields are new in 4.0, and both carry name and version as separate keys rather +than a joined `Name/Version` string — a product name may itself contain a `/`, and the most +natural module name to pass is the Composer package name: + +```json +{"sdk_version":"4.0.0", ..., + "module":{"name":"unitpay-woocommerce","version":"2.1"}, + "stack":[{"name":"WordPress","version":"6.5"},{"name":"WooCommerce","version":"8.2"}]} +``` + +Neither key is sent unless you fill it in with `setModule()` / `setStack()` — see section 6. +The `User-Agent` keeps the readable joined token, because that field cannot carry structure. + +This affects you only if something on your side reads the SDK's own outgoing headers: a +custom transport that logs them, or a test that asserts on them. The SDK sends one name, +not both — but note that 2.x and 3.x installs keep sending `X-Unitpay-Client`, so anything +parsing this header has to accept both names for as long as those versions are in the field. + +See [Telemetry](telemetry.md) for the full payload and the input-handling rules. + +## 5. The deprecated `PaymentObject` values are gone + +Five constants were removed from `Unitpay\Model\Enum\PaymentObject`: + +```text +EXCISE GAMBLING_BET GAMBLING_PRIZE LOTTERY_PRIZE COMPOSITE +``` + +The public API rejects all five and always has — that is why they were deprecated back in +2.1.0. They were kept through 3.0 only to avoid stacking a dictionary change on top of the +namespace break, with the removal re-slated for 4.0. This is that removal. + +Whether this touches you depends on how you name the value, not on which value you send: + +* **You pass the constant** — `new CashItem('Ticket', 1, 100, Nds::NONE, PaymentObject::LOTTERY_PRIZE)`. + This breaks at load time with `Error: Undefined constant`. Pick a supported value from + `PaymentObject`; the receipt line was never going to fiscalize with the old one anyway. +* **You pass the raw string** — `new CashItem('Ticket', 1, 100, Nds::NONE, 'lottery_prize')`. + Nothing changes for you. `CashItem` does not validate `$type` against a whitelist, so the + string travels to the backend and is rejected there, exactly as before this release. + +To find the first case, grep your integration for the five names: + +```sh +grep -rn 'PaymentObject::\(EXCISE\|GAMBLING_BET\|GAMBLING_PRIZE\|LOTTERY_PRIZE\|COMPOSITE\)' . +``` + +See [Receipts](receipts.md) for the supported dictionary. + +## 6. What you get without doing anything + +* **Retries.** The default transport now repeats a request that provably never reached + Unitpay — DNS failure, refused connection, connect timeout — up to twice, with capped + exponential backoff. It never repeats a read timeout, a 5xx, a 409 or a 429, because the + server may already have acted on those. Turn it off with + `DefaultTransport::withoutRetries()`. +* **Configurable timeouts.** `new CurlTransport($connectTimeout, $timeout)`, defaulting to + the previous 5s/10s. +* **Integration identity in telemetry.** `setModule('unitpay-woocommerce', '2.1')` names what + you shipped, `setStack(['WordPress' => '6.5', 'WooCommerce' => '8.2'])` names what it runs + on — see [Telemetry](telemetry.md). Worth filling in if you ship a module, a plugin or a + template. + +## Checklist + +1. Replace `send(): string|false` with `request(): Response` in any custom transport, and + decide `$requestSent` deliberately — `false` when unsure. +2. Drop any string matching on `"Temporary server error"`. +3. Confirm your webhook host runs NTP, or call `setWebhookTolerance()`. +4. If anything on your side reads the SDK's outgoing headers, follow the + `X-Unitpay-Client` → `Unitpay-Client` rename. +5. Grep for `PaymentObject::EXCISE`, `GAMBLING_BET`, `GAMBLING_PRIZE`, `LOTTERY_PRIZE` and + `COMPOSITE` — those five constants are gone. +6. Optionally: name your integration with `setModule()` / `setStack()`, tune the timeouts, + decide whether you want retries. + +## See Also + +* [Getting Started](getting-started.md) — construction, transports, error handling +* [Webhooks](webhooks.md) — the full inbound verification model +* [API Methods](api-methods.md) — the service surface and what each call throws +* [CHANGELOG](../CHANGELOG.md) — everything in 4.0.0, including the folded-in 3.1.0 batch diff --git a/docs/receipts.md b/docs/receipts.md index b186f6f..e552440 100644 --- a/docs/receipts.md +++ b/docs/receipts.md @@ -26,6 +26,19 @@ $item->setMeasure(Measure::ITEM); $unitpay->setCashItems([$item]); ``` +The receipt waits on the instance until one of the three calls that accept it runs — +`form()`, `payments()->initPayment()` or `payments()->offsetAdvance()` — and is cleared +there. Other service calls ignore it, so a lookup in between is harmless: + +```php +$unitpay->setCashItems([$item]); +$unitpay->payments()->getPayment($someOtherId); // receipt untouched +$unitpay->payments()->initPayment(...); // receipt arrives here +``` + +A consuming call clears the receipt even if the request fails, so a retry must call +`setCashItems()` again. See [Fluent parameters](api-methods.md#fluent-parameters). + The dictionaries live in `Unitpay\Model\Enum` as const-classes: `Nds`, `PaymentObject`, `PaymentMethod`, `Measure` (and `PaymentType` for payment-method codes). They are plain classes with `public const`, not native enums, because the SDK supports PHP 7.4. @@ -34,10 +47,6 @@ classes with `public const`, not native enums, because the SDK supports PHP 7.4. > separate path for "real" 20%. Pick the rate that matches the actual receipt (see > [CHANGELOG.md](../CHANGELOG.md)). -Some payment-object values are kept only for backward compatibility and are rejected by -the public API: `EXCISE`, `GAMBLING_BET`, `GAMBLING_PRIZE`, `LOTTERY_PRIZE`, `COMPOSITE`. -Do not use them in new code; they are slated for removal in 4.0. - ## See Also * [Getting Started](getting-started.md) — create a payment with `form()` or the API diff --git a/docs/telemetry.md b/docs/telemetry.md index b7e3fdb..ddd5c86 100644 --- a/docs/telemetry.md +++ b/docs/telemetry.md @@ -8,7 +8,7 @@ self-identification, like any `User-Agent` — it makes **no extra network calls sends secrets, amounts, or customer data: * Service calls (`payments()`, `subscriptions()`, `payouts()`, `reference()`) carry a - `User-Agent: unitpay-php-sdk/ api/` header and an `X-Unitpay-Client` JSON header + `User-Agent: unitpay-php-sdk/ api/` header and a `Unitpay-Client` JSON header with `sdk_version`, `api_version` (the Unitpay API surface targeted), `lang`, `lang_version`, `platform` (coarse OS family only), `publisher`. * `form()` URLs carry an `sdk=php__` query parameter (outside the @@ -16,8 +16,133 @@ sends secrets, amounts, or customer data: The webhook IP-feed fetch is a plain GET and carries no fingerprint headers. -That is the whole of it — there is no separate telemetry endpoint, no opt-in beacon, and -nothing to configure. +There is no separate telemetry endpoint and no beacon. + +## Naming your integration + +The fields above cannot tell a shipped module from a bare script. Two optional calls fix +that, and between them you never have to classify anything: + +* **`setModule()`** — what *you* wrote: the module, plugin, template or application. +* **`setStack()`** — what it *runs on*, outermost host first. + +```php +$unitpay->setModule('unitpay-woocommerce', '2.1') + ->setStack(['WordPress' => '6.5', 'WooCommerce' => '8.2']); +``` + +Both appear in both headers: + +```text +User-Agent: unitpay-php-sdk/4.0.0 api/v1 WordPress/6.5 WooCommerce/8.2 unitpay-woocommerce/2.1 +Unitpay-Client: {"sdk_version":"4.0.0", ..., + "module":{"name":"unitpay-woocommerce","version":"2.1"}, + "stack":[{"name":"WordPress","version":"6.5"},{"name":"WooCommerce","version":"8.2"}]} +``` + +In `Unitpay-Client` each entry is an object, so a consumer never has to guess where a name +ends and a version begins — a product name may legitimately contain a slash, and the most +natural module name to pass is the Composer package name (`unitpay/woocommerce`). The +`User-Agent` keeps the joined `Name/Version` form because that field cannot carry structure; +treat the JSON header as the machine-readable one. + +Set both once, right after construction — they apply to every later service call, including +through service objects that were already created. + +### The runtime is not part of the stack + +PHP and the operating system are reported automatically in `lang_version` and `platform`. The +rule: **if the SDK can find it out itself, it is not stack.** So PHP never appears in a PHP +stack, but WooCommerce does. + +### Worked examples + +```php +// CMS plugin +->setModule('unitpay-bitrix', '3.1') +->setStack(['Bitrix' => SM_VERSION]); + +// four layers — a solution on top of a CMS, your module on top of that +->setModule('unitpay-bitrix', '3.1') +->setStack(['Bitrix' => SM_VERSION, 'Aspro Optimus' => ASPRO_OPTIMUS_VERSION]); + +// a fork of a CMS: report both, nothing is lost +->setModule('unitpay-opencart', '1.4') +->setStack(['OpenCart' => '3.0.3.8', 'ocStore' => '3.0.3.2']); + +// no CMS +->setModule('acme-shop', config('app.release')) +->setStack(['Laravel' => app()->version()]); + +// nothing underneath — a bare script, a bot, a webhook receiver +->setModule('acme-tilda-bridge', '1.4'); +``` + +The last case is ordinary, not a gap: leave the stack unset rather than inventing a value to +fill it in. A version you made up is indistinguishable from a real one. + +`setStack()` **replaces** the whole stack, so calling it twice is safe in a bootstrap that may +run more than once. + +### Nothing here can break a request + +These setters usually run in an integration's bootstrap, so none of them raises on the values +you pass: + +* An unset module, or an empty stack, is omitted rather than sent empty. +* A blank name or version is ignored — the call leaves the value as it was. If nothing was set + before, nothing is sent; if a value was already there, it stays. A CMS that stops exposing + its version string costs you a field in a header, not a checkout. In a stack, only that one + entry is dropped; its neighbours survive. +* Control characters are stripped, and each half is capped (64 bytes for a name, 32 for a + version) so a stray value cannot bloat the header. +* At most **eight** stack entries are sent. Far above any real stack, and it keeps a runaway + loop from building a header an intermediary may reject. +* A non-ASCII name (`1С-Битрикс`, `Аспро: Магазин`) rides in the JSON header only. A + `User-Agent` is an ASCII field, and an absent token there is better than a mangled one; the + JSON header carries the name losslessly. +* An associative array cannot hold a duplicate product name, and a list passed by mistake + (`setStack(['WordPress'])`) is dropped rather than reported as a product named `0`. + +**These are product names and versions you supply.** Nothing is collected from the +environment beyond what is already listed above, and nothing identifies a merchant, a +payment or a customer. + +## If you ship a template + +A template — a repository someone clones and edits, rather than a package they install — +behaves differently from a module, in three ways worth knowing: + +* **The version freezes.** Whoever cloned at `1.0` reports `1.0` for as long as the code runs, + because nobody re-clones a template. So a stale version here is not someone who failed to + update; there is no update channel. It still tells you which template generation is live in + production, which is worth knowing when you plan a breaking change. +* **The code is forked immediately.** After a week it is not your template any more. +* **The identification line is the one most likely to be renamed**, because it sits in code the + merchant edits. + +So write it to survive: + +```php +// Template identity. Please do not rename — this is how we know which template +// generations are live and what to warn about before a breaking change. +// Shipping something of your own on top? Add it to the stack: +// ->setStack(['acme-bot' => '2.3']) +$unitpay->setModule('unitpay-telegram-bot-template', '1.0'); +``` + +That also gives whoever forked it a better option than overwriting your name: keep it, and add +their own line to the stack. You then see both the origin and what grew out of it. + +## Turning it off + +```php +$unitpay->disableTelemetry(); +``` + +`Unitpay-Client` is then not sent at all. The `User-Agent` keeps `unitpay-php-sdk/` +— a request that identifies no library at all is materially harder for Unitpay support to +help with, so that much stays. ## See Also diff --git a/docs/webhooks.md b/docs/webhooks.md index a8fccb7..51b8747 100644 --- a/docs/webhooks.md +++ b/docs/webhooks.md @@ -89,12 +89,20 @@ webhook verifier: error it keeps the built-in list and never throws. It makes a blocking HTTP request, so **don't call it on every webhook**: run it on a schedule (e.g. a daily cron), cache `getAllowedIps()`, and feed the cached list back with `setAllowedIps($cached)` in the - handler. + handler. Since 4.0 the default transport also retries a connect failure twice, so the + worst case is roughly `(1 + 2) × timeout` plus backoff — about 32 seconds with the + default 10s timeout. Fine on a cron, unacceptable in a handler. * `$webhook->addAllowedIps(['1.2.3.4', ...])` adds your own IPs (e.g. a proxy or relay) on top of the Unitpay list; they persist across `refreshAllowedIps()`. * `$webhook->setAllowedIps([...])` replaces the Unitpay list outright. Passing an empty array is fail-closed, not a no-op: with no `addAllowedIps()` entries it rejects every webhook. +* Both setters accept exact IPv4/IPv6 addresses and CIDR ranges, and reject anything else + with `UnitpayValidationException`. A malformed entry (`'31.186.100.4 9'`, + `'31.186.100.0/33'`) would otherwise match nothing and every webhook would fail with a + bare `IP address Error`. The whole call is rejected, so the allowlist is never left + half-configured. `refreshAllowedIps()` keeps its fail-safe contract instead: it drops bad + entries from the feed and never throws. * Override `getIp()` if you run behind a proxy (the check uses `REMOTE_ADDR`, not the spoofable `X-Forwarded-For`). `getIp()` and `isAllowedIp()` are `protected`, so extend `WebhookVerifier` to change them. @@ -111,6 +119,35 @@ cache_set('unitpay_ips', $ips); ->checkHandlerRequest(); ``` +## Replay protection + +A signature proves a webhook was genuine when it was sent, not that it is genuine now: +without a freshness check, anyone who captures one request can resend it forever. + +`checkHandlerRequest()` therefore also verifies that `params[date]` is within **300 +seconds** of your server clock, and throws `UnitpayReplayException` when it is not. That +exception extends `UnitpaySignatureException`, so a handler that already rejects on a bad +signature rejects a replay unchanged. + +The order of checks is signature → freshness → source IP. Freshness runs after the +signature deliberately: an unsigned payload must not be usable to probe how far your clock +is off. + +**Timezone.** Unitpay sends `date` as Moscow time (`Y-m-d H:i:s`, UTC+3). The SDK parses it +against that fixed offset, never with `strtotime()` — which would resolve it against the +server's `date.timezone` and reject every webhook on a UTC host, three hours being ten +times the window. Your server timezone does not matter; your server **clock** does, so run +NTP. + +```php +$unitpay->webhook()->setWebhookTolerance(900); // 15 minutes, e.g. behind a slow queue +$unitpay->webhook()->setWebhookTolerance(0); // off — a captured webhook replays forever +``` + +A webhook that carries no `date` is accepted rather than refused. The field is part of the +signed payload, so an attacker cannot strip it to skip the check: removing any parameter +changes the hash and the signature check, which runs first, rejects the request. + ## See Also * [API Methods](api-methods.md) — the service calls that trigger these callbacks diff --git a/examples/config.php b/examples/config.php index 2ca0416..522ad57 100644 --- a/examples/config.php +++ b/examples/config.php @@ -17,3 +17,9 @@ // overrides the project key from the constructor. $login = getenv('UNITPAY_LOGIN') ?: 'partner@example.com'; $accountSecretKey = getenv('UNITPAY_ACCOUNT_SECRET_KEY') ?: 'set-account-key-in-env'; + +// Integration identity: setModule() names what you wrote, setStack() what it runs on. These +// ride along in User-Agent / Unitpay-Client on every service call and are product names and +// versions only — no PII. Skip them for a bare script; fill them in if you ship a module. +// $unitpay->setModule('unitpay-bitrix', '3.1')->setStack(['Bitrix' => '22.0']); +// Opt out entirely with $unitpay->disableTelemetry(). diff --git a/examples/paymentInfo.php b/examples/paymentInfo.php index ae12fc4..bfb7686 100644 --- a/examples/paymentInfo.php +++ b/examples/paymentInfo.php @@ -3,12 +3,20 @@ header('Content-Type: text/html; charset=UTF-8'); /** - * Payment info + * Payment info, with the 4.0 error surface spelled out. + * + * Every transport failure still extends UnitpayTransportException, so one catch is enough + * when all you need is "it failed". Catch the subclasses when the answer changes what you + * do next — the difference between "Unitpay refused this" and "the request never left the + * server" is the difference between a support ticket and a retry. * * @link https://help.unitpay.ru/payments/payment-info */ use Unitpay\Exception\UnitpayExceptionInterface; +use Unitpay\Exception\UnitpayHttpException; +use Unitpay\Exception\UnitpayNetworkException; +use Unitpay\Exception\UnitpayResponseException; use Unitpay\Unitpay; require_once __DIR__ . '/../vendor/autoload.php'; @@ -24,6 +32,23 @@ } elseif (isset($response->error->message)) { print 'Error: ' . $response->error->message; } +} catch (UnitpayHttpException $exception) { + // Unitpay answered and refused. getResponseBody() holds what it actually said — a JSON + // error envelope, or the HTML page a gateway serves on a 502. Quote that in a support + // ticket, not just the status. + printf( + 'Unitpay returned HTTP %d: %s', + (int) $exception->getStatusCode(), + (string) $exception->getResponseBody() + ); +} catch (UnitpayResponseException $exception) { + // A 2xx whose body is not JSON — usually a captive portal or a proxy in the way. + print 'Unexpected response body: ' . (string) $exception->getResponseBody(); +} catch (UnitpayNetworkException $exception) { + // No response at all. The message states whether the request was actually sent, which + // is what decides whether repeating it is safe. + print 'Could not reach Unitpay: ' . $exception->getMessage(); } catch (UnitpayExceptionInterface $exception) { + // Everything else the SDK raises: a missing secret key, a bad argument. print 'SDK error: ' . $exception->getMessage(); } diff --git a/examples/receipt.php b/examples/receipt.php index 2c18225..ebb8bd2 100644 --- a/examples/receipt.php +++ b/examples/receipt.php @@ -4,9 +4,11 @@ /** * 54-FZ fiscal receipt: line items are described by CashItem objects and attached to - * the payment via setCashItems(). The receipt goes out with the next form()/service call - * and is cleared after a successful call. For the customer to receive the receipt, set - * their contact (email and/or phone) via setCustomerEmail()/setCustomerPhone(). + * the payment via setCashItems(). The receipt goes out with the next call that accepts + * one — form(), payments()->initPayment() or payments()->offsetAdvance() — and is cleared + * there; other service calls leave it alone, so a lookup in between is harmless. A retry + * after a failed call must re-apply the setters. For the customer to receive the receipt, + * set their contact (email and/or phone) via setCustomerEmail()/setCustomerPhone(). * * The dictionaries of VAT rates, payment objects, payment methods and units of measure * are const-classes under Unitpay\Model\Enum: Nds, PaymentObject, PaymentMethod, Measure. diff --git a/phpmd.xml b/phpmd.xml index d6f0ad7..b9226a4 100644 --- a/phpmd.xml +++ b/phpmd.xml @@ -6,46 +6,40 @@ xsi:noNamespaceSchemaLocation="http://pmd.sf.net/ruleset_xml_schema.xsd"> - Curated PHPMD ruleset for the single-file, global-namespace Unitpay SDK. - A few default rules are excluded or retuned because they conflict with the - SDK's deliberate design rather than pointing at real problems. + PHPMD ruleset for the layered Unitpay SDK (PSR-4, src/). + + Stock rulesets with the smallest possible set of deviations: every exclusion and + every retuned threshold below corresponds to a violation that actually fires + against the current src/ and is named explicitly. The generous thresholds this + file used to carry were granted to the pre-3.0 single-file god class; that class + is gone, so they were dropped rather than inherited — an over-tuned detector + would silently accept a slide back towards it. - + - - - - - - - + - - - - - - - - @@ -53,29 +47,55 @@ - - + - + - + + + + + + + + + + + - + - diff --git a/phpstan-examples.neon b/phpstan-examples.neon new file mode 100644 index 0000000..3bf6463 --- /dev/null +++ b/phpstan-examples.neon @@ -0,0 +1,30 @@ +# Static analysis for examples/, which phpstan.neon deliberately leaves out. +# +# examples/ is the source of truth for usage samples, so a renamed class, a renamed method +# or a changed argument count there must fail the build. `composer lint` only syntax-checks +# the directory, which leaves all three invisible. +# +# Why level 2 and one suppression rather than a lower level: catching a renamed method +# needs method existence checked on *expressions*, and the examples call everything through +# a variable (`$unitpay->initPayment(...)`), never through `$this`. That check arrives at +# level 2 — at level 0 the whole point of this config is missed, verified by probing it with +# a call to a method that does not exist. +# +# The one thing level 2 cannot do here is resolve the variables: every example pulls +# $unitpay and the order data in through `require` of config.php / order.php, which PHPStan +# does not follow, so all 56 findings above level 0 are `variable.undefined` on exactly +# those variables. Suppressing that single identifier keeps the real checks — unknown +# classes, unknown methods, wrong arity, PHPDoc validity — and drops nothing else. +# reportUnmatchedIgnoredErrors is off so that restructuring an example cannot fail the +# build for having *fewer* false positives than before. +parameters: + level: 2 + paths: + - examples + bootstrapFiles: + - vendor/autoload.php + reportUnmatchedIgnoredErrors: false + ignoreErrors: + - + identifier: variable.undefined + path: examples/* diff --git a/phpstan.neon b/phpstan.neon index 63aadba..1cd333b 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -1,5 +1,5 @@ parameters: - level: 6 + level: 8 paths: - src - tests diff --git a/src/Api/AbstractService.php b/src/Api/AbstractService.php index ea4cd8a..abad6d2 100644 --- a/src/Api/AbstractService.php +++ b/src/Api/AbstractService.php @@ -2,10 +2,15 @@ namespace Unitpay\Api; +use Unitpay\Exception\UnitpayHttpException; +use Unitpay\Exception\UnitpayNetworkException; +use Unitpay\Exception\UnitpayResponseException; use Unitpay\Exception\UnitpayTransportException; use Unitpay\Exception\UnitpayValidationException; +use Unitpay\Http\Response; use Unitpay\Http\TransportInterface; use Unitpay\Signature\SignatureBuilder; +use Unitpay\Telemetry\ClientInfo; /** * Shared request pipeline for the API services: merges accumulated fluent params, @@ -18,31 +23,44 @@ abstract class AbstractService private string $apiUrl; private ?string $secretKey; private PendingParams $pending; - private string $sdkVersion; - private string $apiVersion; + private ClientInfo $clientInfo; public function __construct( TransportInterface $transport, string $apiUrl, ?string $secretKey, PendingParams $pending, - string $sdkVersion, - string $apiVersion + ClientInfo $clientInfo ) { $this->transport = $transport; $this->apiUrl = $apiUrl; $this->secretKey = $secretKey; $this->pending = $pending; - $this->sdkVersion = $sdkVersion; - $this->apiVersion = $apiVersion; + $this->clientInfo = $clientInfo; } /** - * Runs a server-to-server call. Accumulated fluent params (cashItems, backUrl, ...) - * are drained and merged, with the explicit call params taking precedence. An - * explicit non-empty secretKey overrides the instance key so account-level methods - * (getPartner, payouts, ...) can use the account key. The params are cleared as part - * of drain(), symmetric with form(). + * Merges the accumulated fluent params (cashItems, backUrl, customerEmail, + * customerPhone) into a call's own params and clears them; explicit call params take + * precedence. Only initPayment() and offsetAdvance() accept them. + * + * Clearing happens before the request, so a failed call consumes them too and a retry + * must re-apply the setters — symmetric with form(). + * + * @param array $params + * @return array + */ + protected function withPending(array $params): array + { + return array_merge($this->pending->drain(), $params); + } + + /** + * Runs a server-to-server call. An explicit non-empty secretKey overrides the instance + * key so account-level methods (getPartner, payouts, ...) can use the account key. + * + * This does NOT touch the accumulated fluent params: a consuming method opts in + * explicitly by wrapping its params in withPending(). * * @param array $params * @throws UnitpayValidationException when the secret key is unset/empty @@ -50,8 +68,6 @@ public function __construct( */ protected function request(string $method, array $params): object { - $params = array_merge($this->pending->drain(), $params); - if (empty($params['secretKey'])) { $params['secretKey'] = $this->secretKey; } @@ -68,33 +84,69 @@ protected function request(string $method, array $params): object PHP_QUERY_RFC3986 ); - $response = json_decode($this->transport->send($requestUrl, $this->fingerprintHeaders())); - if (!is_object($response)) { - throw new UnitpayTransportException('Temporary server error. Please try again later.'); + return $this->decode($this->transport->request($requestUrl, $this->clientInfo->headers())); + } + + /** + * Turns a transport result into the decoded JSON envelope, or into the exception that + * describes what actually went wrong. Before 4.0 every branch below collapsed into one + * "Temporary server error" — including a disabled allow_url_fopen, which is permanent. + * + * @throws UnitpayNetworkException no response arrived + * @throws UnitpayHttpException a response arrived with a non-2xx status + * @throws UnitpayResponseException a 2xx arrived whose body is not a JSON object + */ + private function decode(Response $response): object + { + if ($response->getStatusCode() === 0) { + throw new UnitpayNetworkException( + $this->networkMessage($response), + $response->getErrno(), + $response->getTransportError() + ); + } + + if (!$response->isSuccessful()) { + throw new UnitpayHttpException( + sprintf('Unitpay API returned HTTP %d.', $response->getStatusCode()), + $response->getStatusCode(), + $response->getBody() + ); } - return $response; + $decoded = json_decode($response->getBody()); + if (!is_object($decoded)) { + throw new UnitpayResponseException( + sprintf( + 'Unitpay API returned HTTP %d with a body that is not a JSON object (%s).', + $response->getStatusCode(), + json_last_error_msg() + ), + $response->getStatusCode(), + $response->getBody() + ); + } + + return $decoded; } /** - * SDK self-identification headers (anonymous, no PII): a short User-Agent plus an - * X-Unitpay-Client JSON object. api_version is the Unitpay API surface targeted; - * platform is the coarse OS family only. - * @return string[] + * Whether the request reached Unitpay decides what the caller may safely do next, so + * the message says it outright. The API accepts no idempotency key, so a blind repeat + * of a delivered initPayment can create a second payment. */ - private function fingerprintHeaders(): array + private function networkMessage(Response $response): string { - $client = (string) json_encode([ - 'sdk_version' => $this->sdkVersion, - 'api_version' => $this->apiVersion, - 'lang' => 'php', - 'lang_version' => PHP_VERSION, - 'platform' => PHP_OS_FAMILY, - 'publisher' => 'unitpay', - ]); - return [ - 'User-Agent: unitpay-php-sdk/' . $this->sdkVersion . ' api/' . $this->apiVersion, - 'X-Unitpay-Client: ' . $client, - ]; + $detail = sprintf('%s (error %d)', $response->getTransportError(), $response->getErrno()); + + if ($response->wasRequestSent()) { + return 'No response from the Unitpay API: ' . $detail + . '. The request was sent, so it may already have been processed — check the' + . ' payment state before repeating it.'; + } + + return 'Could not reach the Unitpay API: ' . $detail + . '. The request was not sent, so nothing was processed.'; } + } diff --git a/src/Api/PaymentService.php b/src/Api/PaymentService.php index 5554fd6..ca05cc9 100644 --- a/src/Api/PaymentService.php +++ b/src/Api/PaymentService.php @@ -9,18 +9,22 @@ final class PaymentService extends AbstractService { /** + * Creates a payment. Consuming call: the params accumulated by setCashItems(), + * setCustomerEmail(), setCustomerPhone() and setBackUrl() are folded in here and + * cleared. + * * @param int|float|string $sum * @param int|string $projectId * @param array $options extra params (e.g. desc, currency, account) */ public function initPayment(string $account, $sum, $projectId, string $paymentType, array $options = []): object { - return $this->request('initPayment', array_merge([ + return $this->request('initPayment', $this->withPending(array_merge([ 'account' => $account, 'sum' => $sum, 'projectId' => $projectId, 'paymentType' => $paymentType, - ], $options)); + ], $options))); } /** @@ -64,15 +68,16 @@ public function cancelPayment($paymentId, array $options = []): object /** * Advance-offset fiscal receipt. Account-level: pass the account key in - * $options['secretKey'] and optionally cashItems. + * $options['secretKey'] and optionally cashItems. Consuming call: a receipt set via + * setCashItems() is folded in here and cleared. * @param int|string $paymentId * @param array $options */ public function offsetAdvance(string $login, $paymentId, array $options = []): object { - return $this->request('offsetAdvance', array_merge([ + return $this->request('offsetAdvance', $this->withPending(array_merge([ 'login' => $login, 'paymentId' => $paymentId, - ], $options)); + ], $options))); } } diff --git a/src/Api/PendingParams.php b/src/Api/PendingParams.php index 24fc18e..1b76fea 100644 --- a/src/Api/PendingParams.php +++ b/src/Api/PendingParams.php @@ -4,9 +4,11 @@ /** * Mutable holder for the fluent-setter params (cashItems, customerEmail, backUrl, - * customerPhone) accumulated on the facade. Shared between the facade and its - * services so a setter chain reaches the next form()/service call, then drained so a - * reused instance never carries this call's params into the following one. + * customerPhone) accumulated on the facade. Shared between the facade and its services. + * + * Only form(), initPayment() and offsetAdvance() read and drain them; every other call + * leaves them untouched. A consuming call clears them, so a reused instance never carries + * one order's receipt into the next. */ final class PendingParams { diff --git a/src/Exception/UnitpayHttpException.php b/src/Exception/UnitpayHttpException.php new file mode 100644 index 0000000..f30e4c9 --- /dev/null +++ b/src/Exception/UnitpayHttpException.php @@ -0,0 +1,21 @@ +statusCode = $statusCode; + $this->responseBody = $responseBody; + } +} diff --git a/src/Exception/UnitpayNetworkException.php b/src/Exception/UnitpayNetworkException.php new file mode 100644 index 0000000..344ef08 --- /dev/null +++ b/src/Exception/UnitpayNetworkException.php @@ -0,0 +1,21 @@ +errno = $errno; + $this->transportError = $transportError; + } +} diff --git a/src/Exception/UnitpayReplayException.php b/src/Exception/UnitpayReplayException.php new file mode 100644 index 0000000..8b5e8ae --- /dev/null +++ b/src/Exception/UnitpayReplayException.php @@ -0,0 +1,15 @@ +statusCode = $statusCode; + $this->responseBody = $responseBody; + } +} diff --git a/src/Exception/UnitpayTransportException.php b/src/Exception/UnitpayTransportException.php index 34f6919..10f3e94 100644 --- a/src/Exception/UnitpayTransportException.php +++ b/src/Exception/UnitpayTransportException.php @@ -2,7 +2,46 @@ namespace Unitpay\Exception; -/** api() could not obtain a usable response from Unitpay (network or response parsing). */ +/** + * A service call could not obtain a usable response from Unitpay. + * + * Base of the three concrete cases — UnitpayNetworkException (no response), + * UnitpayHttpException (non-2xx) and UnitpayResponseException (unusable payload) — so a + * caller that only wants "the request failed" still gets away with one catch. Every + * accessor is nullable because each subclass fills in only what its case actually knows. + * + * This class deliberately takes plain scalars rather than an Http\Response: Exception is + * a leaf layer and must not depend on Http (see .ai-factory/ARCHITECTURE.md). Turning a + * Response into one of these lives in the Api layer. + */ class UnitpayTransportException extends \InvalidArgumentException implements UnitpayExceptionInterface { + protected ?int $statusCode = null; + protected ?int $errno = null; + protected ?string $transportError = null; + protected ?string $responseBody = null; + + /** HTTP status, or null when no response arrived. */ + public function getStatusCode(): ?int + { + return $this->statusCode; + } + + /** cURL errno (or Http\Response::ERRNO_LOCAL), or null when a response did arrive. */ + public function getErrno(): ?int + { + return $this->errno; + } + + /** Transport-level error text, or null when a response did arrive. */ + public function getTransportError(): ?string + { + return $this->transportError; + } + + /** Raw response body, or null when no response arrived. */ + public function getResponseBody(): ?string + { + return $this->responseBody; + } } diff --git a/src/Http/CurlTransport.php b/src/Http/CurlTransport.php index 7e8a84e..f841a43 100644 --- a/src/Http/CurlTransport.php +++ b/src/Http/CurlTransport.php @@ -2,6 +2,8 @@ namespace Unitpay\Http; +use Unitpay\Exception\UnitpayValidationException; + /** * Default transport: cURL (with connect/read timeouts and no dependency on * allow_url_fopen) when ext-curl is present, otherwise a file_get_contents @@ -10,45 +12,256 @@ * The file_get_contents fallback suppresses its transport warning via * set_error_handler (not the '@' operator, which QA rules forbid) — otherwise that * warning would log the URL together with the secret. + * + * Neither path throws: the outcome is described in a Response and classified by the + * calling layer. */ final class CurlTransport implements TransportInterface { + private int $connectTimeout; + private int $timeout; + + /** + * @param int $connectTimeout seconds allowed for establishing the connection. cURL + * only; the file_get_contents fallback has no separate + * connect timeout and is bounded by $timeout alone. + * @param int $timeout seconds allowed for the whole request + * + * @throws UnitpayValidationException when either timeout is not positive + */ + public function __construct(int $connectTimeout = 5, int $timeout = 10) + { + // Rejected here rather than at the first request, so a misconfiguration surfaces + // where it was made. cURL reads 0 as "wait forever", which in a payment flow means + // a request that never returns. + self::assertPositive($connectTimeout, 'Connect timeout'); + self::assertPositive($timeout, 'Timeout'); + + $this->connectTimeout = $connectTimeout; + $this->timeout = $timeout; + } + + public function getConnectTimeout(): int + { + return $this->connectTimeout; + } + + public function getTimeout(): int + { + return $this->timeout; + } + /** * @param string[] $headers - * @return string|false */ - public function send(string $url, array $headers = []) + public function request(string $url, array $headers = []): Response { + $headers = self::sanitizeHeaders($headers); + if (function_exists('curl_init')) { - $ch = curl_init($url); - $opts = [ - CURLOPT_RETURNTRANSFER => true, - CURLOPT_CONNECTTIMEOUT => 5, - CURLOPT_TIMEOUT => 10, - ]; - if ($headers !== []) { - $opts[CURLOPT_HTTPHEADER] = $headers; - } - curl_setopt_array($ch, $opts); - $body = curl_exec($ch); - if (\PHP_VERSION_ID < 80000) { - curl_close($ch); - } - return $body; + return $this->requestViaCurl($url, $headers); + } + + return $this->requestViaStream($url, $headers); + } + + /** + * Drops any header line carrying a CR or an LF. + * + * The stream path joins header lines with "\r\n", so a newline inside a value adds a + * header line of the caller's choosing. Since 4.0.0 nothing the SDK itself builds can + * contain one — `Telemetry\ClientInfo` strips control characters where the value comes + * in — which is exactly why this is the second line of defence and not the first. + * + * The offending line is dropped rather than raised on: a transport that aborts a + * payment over a malformed diagnostic header would repeat the mistake this release + * removed from the telemetry setters. + * + * Public and static so it can be exercised without a socket — the request paths + * themselves talk to the network and are not covered by the suite. + * + * @param string[] $headers + * @return string[] + */ + public static function sanitizeHeaders(array $headers): array + { + return array_values(array_filter($headers, static function (string $header): bool { + return strpbrk($header, "\r\n") === false; + })); + } + + /** + * @param string[] $headers + */ + private function requestViaCurl(string $url, array $headers): Response + { + $ch = curl_init($url); + $opts = [ + CURLOPT_RETURNTRANSFER => true, + CURLOPT_CONNECTTIMEOUT => $this->connectTimeout, + CURLOPT_TIMEOUT => $this->timeout, + // Prepends the header block to the body rather than taking a + // CURLOPT_HEADERFUNCTION callback, whose mandatory handle argument would go + // unused. Redirects are not followed, so there is exactly one block. + CURLOPT_HEADER => true, + ]; + if ($headers !== []) { + $opts[CURLOPT_HTTPHEADER] = $headers; + } + curl_setopt_array($ch, $opts); + + $raw = curl_exec($ch); + $errno = curl_errno($ch); + $error = curl_error($ch); + $status = (int) curl_getinfo($ch, CURLINFO_RESPONSE_CODE); + $headerSize = (int) curl_getinfo($ch, CURLINFO_HEADER_SIZE); + $connectTime = (float) curl_getinfo($ch, CURLINFO_CONNECT_TIME); + + if (\PHP_VERSION_ID < 80000) { + curl_close($ch); + } + + // curl_exec() returns true only when RETURNTRANSFER is off; it is always on here. + if ($errno !== 0 || !is_string($raw)) { + return Response::failed($errno, $error, self::wasRequestSent($connectTime)); } - $http = ['timeout' => 10]; + return Response::received( + $status, + substr($raw, $headerSize), + self::splitHeaderBlock(substr($raw, 0, $headerSize)) + ); + } + + /** + * @param string[] $headers + */ + private function requestViaStream(string $url, array $headers): Response + { + if (!filter_var(ini_get('allow_url_fopen'), FILTER_VALIDATE_BOOLEAN)) { + return Response::failed( + Response::ERRNO_LOCAL, + 'ext-curl is not installed and allow_url_fopen is disabled in php.ini, so the SDK ' + . 'cannot make outbound requests. This is a permanent local configuration problem, ' + . 'not a temporary one: install ext-curl or enable allow_url_fopen.', + false + ); + } + + $http = [ + 'timeout' => $this->timeout, + // Without this a 4xx/5xx yields false and the error body is lost — exactly the + // diagnostic this release exists to preserve. + 'ignore_errors' => true, + ]; if ($headers !== []) { $http['header'] = implode("\r\n", $headers); } $context = stream_context_create(['http' => $http]); + set_error_handler(static function () { return true; }); try { - return file_get_contents($url, false, $context); + // The stream wrapper writes $http_response_header into this scope, but only + // once a response actually arrives. Seeding it keeps the no-response case from + // reading an undefined variable. + $http_response_header = []; + $body = file_get_contents($url, false, $context); + $responseHeaders = $http_response_header; } finally { restore_error_handler(); } + + if (!is_string($body)) { + return Response::failed( + // ERRNO_LOCAL is what keeps this attempt from being retried: this path sees + // no connect/read phase, so the failure may be a request Unitpay already + // processed. The requestSent flag below cannot carry that meaning — false + // there asserts "provably not sent", which would license a retry. + Response::ERRNO_LOCAL, + 'The request failed and the file_get_contents fallback cannot report why. ' + . 'Install ext-curl for a diagnosable transport.', + false + ); + } + + /** @var string[] $responseHeaders */ + return Response::received(self::parseStatus($responseHeaders), $body, $responseHeaders); + } + + /** + * @throws UnitpayValidationException + */ + private static function assertPositive(int $seconds, string $label): void + { + if ($seconds <= 0) { + throw new UnitpayValidationException( + sprintf('%s must be a positive number of seconds, got %d.', $label, $seconds) + ); + } + } + + /** + * Whether the request had already been handed to the wire when the attempt failed — + * the flag the retry policy is built on, so getting it wrong risks a duplicate + * payment rather than a duplicate log line. + * + * cURL reports CURLE_OPERATION_TIMEDOUT (28) for both a connect timeout and a read + * timeout, so the errno cannot separate them. CURLINFO_CONNECT_TIME can: it is + * exactly 0.0 until a connection is established. Measured on cURL 8.21.0: + * + * case errno connect_time pretransfer_time + * HTTP 200 0 0.003443 0.144292 + * HTTP 404 0 0.002099 0.169619 + * DNS failure 6 0.000000 0.001893 + * connection refused 7 0.000000 0.000316 + * connect timeout 28 0.000000 3.003365 + * + * Note the third column: CURLINFO_PRETRANSFER_TIME is NOT zero on connect-phase + * failures on modern cURL, so it cannot be used for this — do not "simplify" back to + * it. + * + * A TLS handshake that fails after the TCP connect reports connect_time > 0 with the + * request still unsent, so this answers "sent" there. That is the deliberate + * direction of the error: claiming "sent" costs a missed retry, claiming "not sent" + * wrongly costs a second payment. + */ + private static function wasRequestSent(float $connectTime): bool + { + return $connectTime > 0.0; + } + + /** + * Splits the raw cURL header block into the same shape the stream wrapper produces, + * so both transports hand Response identical header lines. + * + * @return string[] + */ + private static function splitHeaderBlock(string $block): array + { + $lines = preg_split('/\r\n|\n/', trim($block)); + + return $lines === false ? [] : array_values(array_filter($lines, static function (string $line): bool { + return $line !== ''; + })); + } + + /** + * Reads the status out of the response header lines. Defaults to 200 when no status + * line is present: file_get_contents returning a string means the fetch itself + * succeeded, and the pre-4.0 contract treated any body as a success. + * + * @param string[] $headers + */ + private static function parseStatus(array $headers): int + { + foreach ($headers as $header) { + if (preg_match('#^HTTP/\d(?:\.\d)?\s+(\d{3})#', $header, $matches) === 1) { + return (int) $matches[1]; + } + } + + return 200; } } diff --git a/src/Http/DefaultTransport.php b/src/Http/DefaultTransport.php new file mode 100644 index 0000000..4635bfc --- /dev/null +++ b/src/Http/DefaultTransport.php @@ -0,0 +1,29 @@ +statusCode = $statusCode; + $this->body = $body; + $this->headers = $headers; + $this->errno = $errno; + $this->error = $error; + $this->requestSent = $requestSent; + } + + /** + * An HTTP response came back, whatever its status. Receiving one proves the request + * reached the server, so requestSent is always true here. + * + * @param string[] $headers raw response header lines, as received + */ + public static function received(int $statusCode, string $body, array $headers = []): self + { + return new self($statusCode, $body, $headers, 0, '', true); + } + + /** + * No HTTP response came back. + * + * @param int $errno cURL errno, or ERRNO_LOCAL for a local failure + * @param bool $requestSent whether the request had already been put on the wire — see + * wasRequestSent(); pass false when it cannot be determined + */ + public static function failed(int $errno, string $error, bool $requestSent): self + { + return new self(0, '', [], $errno, $error, $requestSent); + } + + /** HTTP status, or 0 when no response arrived. */ + public function getStatusCode(): int + { + return $this->statusCode; + } + + public function getBody(): string + { + return $this->body; + } + + /** @return string[] raw response header lines */ + public function getHeaders(): array + { + return $this->headers; + } + + /** cURL errno, ERRNO_LOCAL for a local failure, or 0 when the transport did not fail. */ + public function getErrno(): int + { + return $this->errno; + } + + /** Transport-level error text, or '' when the transport did not fail. */ + public function getTransportError(): string + { + return $this->error; + } + + /** + * Whether the request was already on the wire when the attempt failed. + * + * This is the retry-safety signal, and the only thing a retry decision may be based + * on. cURL reports CURLE_OPERATION_TIMEDOUT (28) for both a connect timeout and a read + * timeout, so the errno cannot tell the two apart — but the consequence differs + * completely: after a read timeout the server may already have created the payment, + * and the Unitpay API accepts no idempotency key to make a repeat harmless. + */ + public function wasRequestSent(): bool + { + return $this->requestSent; + } + + /** Whether an HTTP response arrived with a 2xx status. */ + public function isSuccessful(): bool + { + return $this->statusCode >= 200 && $this->statusCode < 300; + } +} diff --git a/src/Http/RetryingTransport.php b/src/Http/RetryingTransport.php new file mode 100644 index 0000000..0c28198 --- /dev/null +++ b/src/Http/RetryingTransport.php @@ -0,0 +1,151 @@ +inner = $inner; + $this->maxRetries = $maxRetries; + $this->baseDelay = $baseDelay; + $this->maxDelay = $maxDelay; + } + + /** The wrapped transport, for callers that need to reach it without the retry policy. */ + public function getInner(): TransportInterface + { + return $this->inner; + } + + /** + * @param string[] $headers + */ + public function request(string $url, array $headers = []): Response + { + $attempts = 0; + + do { + if ($attempts > 0) { + $this->sleep($this->backoff($attempts)); + } + $response = $this->inner->request($url, $headers); + ++$attempts; + } while ($this->shouldRetry($response) && $attempts <= $this->maxRetries); + + return $this->describeAttempts($response, $attempts); + } + + /** + * Pauses between attempts. Overridden in tests so the suite never actually waits. + */ + protected function sleep(float $seconds): void + { + usleep((int) round($seconds * 1000000)); + } + + /** + * Retry only on positive knowledge that the request never left this client. + * + * Three conditions, each excluding a way the server may already have acted: + * - a status means it answered; + * - wasRequestSent() means it saw the request even though no status came back; + * - ERRNO_LOCAL means the transport could not classify the failure at all, and + * "unknown" is not "not sent". The file_get_contents fallback reports exactly this: + * it sees no connect/read phase, so a failure there may be a delivered request. + * + * The last condition is the one that is easy to lose. Dropping it makes a fallback + * install repeat a delivered initPayment, and the API accepts no idempotency key to + * make that harmless. Do not widen this to cover 5xx or 429 either, for the same reason. + */ + private function shouldRetry(Response $response): bool + { + return $response->getStatusCode() === 0 + && !$response->wasRequestSent() + && $response->getErrno() !== Response::ERRNO_LOCAL; + } + + /** + * Exponential backoff, capped, with jitter over the upper half of the interval so a + * fleet of clients failing at the same moment does not retry in lockstep. + */ + private function backoff(int $retry): float + { + $delay = min($this->baseDelay * (2 ** ($retry - 1)), $this->maxDelay); + $jittered = $delay * 0.5 * (1 + (mt_rand() / mt_getrandmax())); + + return max($this->baseDelay * 0.5, $jittered); + } + + /** + * Records the attempt count in the failure the caller receives. Without it a retried + * failure is indistinguishable from a single one, and the seconds spent retrying look + * like an unexplained stall. + */ + private function describeAttempts(Response $response, int $attempts): Response + { + if ($attempts < 2 || $response->getStatusCode() !== 0) { + return $response; + } + + return Response::failed( + $response->getErrno(), + sprintf('%s (after %d attempts)', $response->getTransportError(), $attempts), + $response->wasRequestSent() + ); + } +} diff --git a/src/Http/TransportInterface.php b/src/Http/TransportInterface.php index 4824466..1371087 100644 --- a/src/Http/TransportInterface.php +++ b/src/Http/TransportInterface.php @@ -4,15 +4,15 @@ /** * Outbound HTTP transport used by the API services and the webhook IP-feed fetch. - * Implementations perform an HTTP GET and return the raw response body, or false - * on a transport error. Inject a fake in tests to exercise the SDK without the - * network. + * Implementations perform an HTTP GET and describe the outcome in a Response — they + * report failures through it rather than throwing, so the calling layer owns the + * decision of which exception a given outcome deserves. Inject a fake in tests to + * exercise the SDK without the network. */ interface TransportInterface { /** * @param string[] $headers HTTP headers of the form "Name: value" - * @return string|false raw response body, or false on a transport error */ - public function send(string $url, array $headers = []); + public function request(string $url, array $headers = []): Response; } diff --git a/src/Model/Enum/PaymentObject.php b/src/Model/Enum/PaymentObject.php index e5f01cc..a3ac9ea 100644 --- a/src/Model/Enum/PaymentObject.php +++ b/src/Model/Enum/PaymentObject.php @@ -58,15 +58,4 @@ final class PaymentObject public const COMMODITY_WITHOUT_MARK = 'commodity_without_mark'; /** Commodity subject to marking, with a mark code */ public const COMMODITY_MARK = 'commodity_mark'; - - /** @deprecated Rejected by the public API; will be removed in 4.0. */ - public const EXCISE = 'excise'; - /** @deprecated Rejected by the public API; will be removed in 4.0. */ - public const GAMBLING_BET = 'gambling_bet'; - /** @deprecated Rejected by the public API; will be removed in 4.0. */ - public const GAMBLING_PRIZE = 'gambling_prize'; - /** @deprecated Rejected by the public API; will be removed in 4.0. */ - public const LOTTERY_PRIZE = 'lottery_prize'; - /** @deprecated Rejected by the public API; will be removed in 4.0. */ - public const COMPOSITE = 'composite'; } diff --git a/src/Telemetry/ClientInfo.php b/src/Telemetry/ClientInfo.php new file mode 100644 index 0000000..1376ae7 --- /dev/null +++ b/src/Telemetry/ClientInfo.php @@ -0,0 +1,260 @@ + + */ + private array $stack = []; + + public function __construct(string $sdkVersion, string $apiVersion) + { + $this->sdkVersion = $sdkVersion; + $this->apiVersion = $apiVersion; + } + + /** + * Names the integration itself — the module, plugin, template or application that wraps + * this SDK. + * + * A value that cannot produce a meaningful token is dropped, not rejected. This runs in + * an integration's bootstrap, so raising here would let a cosmetic concern abort a + * checkout — and a CMS that stops exposing its version string is an ordinary outcome of a + * CMS update, not an exceptional one. + */ + public function setModule(string $name, string $version): void + { + $entry = self::entry($name, $version); + if ($entry === null) { + // Returning leaves any earlier value in place, so a setter that starts coming back + // empty costs the update rather than the identity already reported. + return; + } + + $this->module = $entry; + } + + /** + * Replaces the whole stack: the products this integration runs on, outermost host first. + * + * Replacement rather than appending, so the call is idempotent and safe in a bootstrap + * that may execute twice. Someone who forked a template adds a line to the same array, + * which is the case an append method would have existed for — and append can be + * introduced later, while removing it could not. + * + * Order is presentation only: it makes the User-Agent read like the stack it describes, + * and grouping happens by name, which is order-independent. + * + * @param array $stack product name => version. A non-string key + * means a list was passed by mistake and is + * dropped rather than turned into a product + * literally named "0". + */ + public function setStack(array $stack): void + { + $this->stack = []; + foreach ($stack as $name => $version) { + if (count($this->stack) >= self::MAX_STACK_ENTRIES) { + break; + } + if (!is_string($name) || !is_scalar($version)) { + continue; + } + + $entry = self::entry($name, (string) $version); + if ($entry !== null) { + $this->stack[] = $entry; + } + } + } + + /** + * Stops sending Unitpay-Client entirely. The User-Agent keeps the SDK version alone, + * because a request with no product identification at all is harder to support than one + * that says which library sent it. + */ + public function disable(): void + { + $this->enabled = false; + } + + /** + * @return string[] header lines ready for the transport + */ + public function headers(): array + { + if (!$this->enabled) { + return ['User-Agent: unitpay-php-sdk/' . $this->sdkVersion]; + } + + $headers = ['User-Agent: ' . $this->userAgent()]; + $client = $this->clientJson(); + if ($client !== null) { + $headers[] = 'Unitpay-Client: ' . $client; + } + + return $headers; + } + + /** + * Cleans both halves and refuses the pair when either is unusable. + * + * @return array{name: string, version: string}|null + */ + private static function entry(string $name, string $version): ?array + { + $name = self::clean($name, self::MAX_NAME_BYTES); + $version = self::clean($version, self::MAX_VERSION_BYTES); + if ($name === '' || $version === '') { + // A blank half would emit a meaningless "/1.0" or "Bitrix/" token. + return null; + } + + return ['name' => $name, 'version' => $version]; + } + + /** + * Strips control characters, trims, and bounds the length. + * + * Removing C0 and DEL is what closes header injection at its source: trim() only cleans + * the edges, so a CR/LF in the middle of a value used to travel all the way to the + * transport, which joins header lines with "\r\n" on the stream path. + * + * The cap counts bytes because ext-mbstring is not a declared dependency. Cutting a + * multi-byte character in half is then undone by dropping the trailing bytes of the + * incomplete sequence — at most three — so valid UTF-8 in stays valid UTF-8 out. + */ + private static function clean(string $value, int $maxBytes): string + { + $value = trim((string) preg_replace('/[\x00-\x1F\x7F]/', '', $value)); + if (strlen($value) <= $maxBytes) { + return $value; + } + + $value = substr($value, 0, $maxBytes); + for ($i = 0; $i < 3 && $value !== '' && preg_match('//u', $value) !== 1; $i++) { + $value = substr($value, 0, -1); + } + + return $value; + } + + /** + * The only place the two halves are joined. A User-Agent cannot carry structure, so its + * tokens stay the human-readable "Name/Version" form and the JSON header is what a parser + * should read. The module goes last, being the narrowest context of all. + */ + private function userAgent(): string + { + $parts = ['unitpay-php-sdk/' . $this->sdkVersion, 'api/' . $this->apiVersion]; + + $entries = $this->stack; + if ($this->module !== null) { + $entries[] = $this->module; + } + + foreach ($entries as $entry) { + $token = $entry['name'] . '/' . $entry['version']; + // A User-Agent is an ASCII field, and a Cyrillic product name would otherwise put + // raw UTF-8 bytes into it. The JSON header carries the entry losslessly either + // way, so an absent token beats a mangled one. + if (preg_match('/^[\x20-\x7E]+$/', $token) === 1) { + $parts[] = $token; + } + } + + return implode(' ', $parts); + } + + /** + * api_version is the Unitpay API surface targeted; platform is the coarse OS family only, + * never the full uname. The module is an object and the stack an ordered array of them, so + * a consumer never has to guess where a name ends and a version begins. An empty one is + * omitted rather than sent empty. + * + * @return string|null null when the payload could not be encoded at all — see below + */ + private function clientJson(): ?string + { + $client = [ + 'sdk_version' => $this->sdkVersion, + 'api_version' => $this->apiVersion, + 'lang' => 'php', + 'lang_version' => PHP_VERSION, + 'platform' => PHP_OS_FAMILY, + 'publisher' => 'unitpay', + ]; + if ($this->module !== null) { + $client['module'] = $this->module; + } + if ($this->stack !== []) { + $client['stack'] = $this->stack; + } + + // JSON_UNESCAPED_SLASHES only so a name carrying a slash reads normally. Deliberately + // no JSON_UNESCAPED_UNICODE: the \uXXXX escaping is what keeps this header value pure + // ASCII, which an HTTP header has to be. + // + // JSON_INVALID_UTF8_SUBSTITUTE is what keeps one bad byte from costing the whole + // payload: a legacy windows-1251 CMS name used to make json_encode return false, + // which the old `(string)` cast turned into an empty header — losing sdk_version and + // the PHP version along with the entry that caused it. + $json = json_encode($client, JSON_UNESCAPED_SLASHES | JSON_INVALID_UTF8_SUBSTITUTE); + + // Belt and braces: with the substitute flag a false return is all but unreachable, but + // if it happens the header is omitted rather than sent blank. + return $json === false ? null : $json; + } +} diff --git a/src/Unitpay.php b/src/Unitpay.php index 71db1a6..64e0198 100644 --- a/src/Unitpay.php +++ b/src/Unitpay.php @@ -2,16 +2,18 @@ namespace Unitpay; +use Unitpay\Api\AbstractService; use Unitpay\Api\PaymentService; use Unitpay\Api\PayoutService; use Unitpay\Api\PendingParams; use Unitpay\Api\ReferenceService; use Unitpay\Api\SubscriptionService; use Unitpay\Exception\UnitpayValidationException; -use Unitpay\Http\CurlTransport; +use Unitpay\Http\DefaultTransport; use Unitpay\Http\TransportInterface; use Unitpay\Model\CashItem; use Unitpay\Signature\SignatureBuilder; +use Unitpay\Telemetry\ClientInfo; use Unitpay\Webhook\WebhookVerifier; /** @@ -25,7 +27,7 @@ final class Unitpay { /** SDK version; sent in the telemetry fingerprint. Keep in sync with the release git tag. */ - public const VERSION = '3.0.0'; + public const VERSION = '4.0.0'; /** Unitpay API surface this SDK targets; sent in the telemetry fingerprint. */ public const API_VERSION = 'v1'; @@ -36,6 +38,7 @@ final class Unitpay private TransportInterface $transport; private SignatureBuilder $signature; private PendingParams $pending; + private ClientInfo $clientInfo; private WebhookVerifier $webhookVerifier; private ?PaymentService $payments = null; private ?SubscriptionService $subscriptions = null; @@ -43,9 +46,14 @@ final class Unitpay private ?ReferenceService $reference = null; /** - * @param string $domain host only, e.g. "unitpay.ru" — without scheme or path. - * @param TransportInterface|null $transport outbound HTTP transport for api()/feed fetch. - * Defaults to CurlTransport. Inject a fake to test without the network. + * @param string $domain host only, e.g. "unitpay.ru" — without scheme or path, + * optionally with a :port. + * @param TransportInterface|null $transport outbound HTTP transport for the service calls + * and the IP-feed fetch. Defaults to a CurlTransport wrapped + * in a RetryingTransport, which repeats an attempt only when + * it provably never reached Unitpay. Pass + * DefaultTransport::withoutRetries() to switch retries off, + * or a fake to test without the network. * @param array|null $request inbound webhook array read by the webhook * verifier. Defaults to $_GET. * @param string|null $clientIp sender IP used by the webhook verifier. Defaults to @@ -58,13 +66,15 @@ public function __construct( ?array $request = null, ?string $clientIp = null ) { + $this->assertValidDomain($domain); $this->secretKey = $secretKey; $this->apiUrl = "https://$domain/api"; $this->formUrl = "https://$domain/pay/"; $ipsUrl = "https://$domain/ips/ips_webhooks.json"; - $this->transport = $transport ?? new CurlTransport(); + $this->transport = $transport ?? DefaultTransport::create(); $this->signature = new SignatureBuilder(); $this->pending = new PendingParams(); + $this->clientInfo = new ClientInfo(self::VERSION, self::API_VERSION); $this->webhookVerifier = new WebhookVerifier( $secretKey, $this->signature, @@ -105,6 +115,51 @@ public function webhook(): WebhookVerifier return $this->webhookVerifier; } + /** + * Names the integration itself — the module, plugin, template or application wrapping + * this SDK, e.g. setModule('unitpay-bitrix', '3.1'). + * + * The value rides along in User-Agent and Unitpay-Client on every service call. It is a + * product name and version only — no PII, no identifiers. Without it the fingerprint + * cannot tell a shipped module apart from a bare script. + * + * A blank name or version is ignored rather than rejected: telemetry never throws into a + * payment flow. + */ + public function setModule(string $name, string $version): self + { + $this->clientInfo->setModule($name, $version); + return $this; + } + + /** + * Names the products this integration runs on, outermost host first: + * setStack(['WordPress' => '6.5', 'WooCommerce' => '8.2']). + * + * The runtime does not belong here — PHP and the OS are reported automatically. The rule + * is: if the SDK can find it out itself, it is not stack. + * + * Replaces the whole stack, so the call is idempotent. Nothing is rejected: an entry with + * a blank half is dropped without taking its neighbours, and at most eight are sent. + * + * @param array $stack product name => version + */ + public function setStack(array $stack): self + { + $this->clientInfo->setStack($stack); + return $this; + } + + /** + * Stops sending the Unitpay-Client header. The User-Agent keeps naming the SDK and + * its version, which is what makes a request supportable at all. + */ + public function disableTelemetry(): self + { + $this->clientInfo->disable(); + return $this; + } + /** * Sets the URL Unitpay will return the payer to after payment. */ @@ -203,21 +258,41 @@ public function form(string $publicKey, $sum, string $account, string $desc, str } /** - * @param class-string $class - * @return PaymentService|SubscriptionService|PayoutService|ReferenceService + * @template T of AbstractService + * @param class-string $class + * @return T */ - private function makeService(string $class) + private function makeService(string $class): AbstractService { return new $class( $this->transport, $this->apiUrl, $this->secretKey, $this->pending, - self::VERSION, - self::API_VERSION + $this->clientInfo ); } + /** + * Rejects anything that is not a bare host (optionally with a port). The domain is + * interpolated into the API, form and IP-feed URLs, so a scheme or path would produce + * malformed URLs. + * + * @throws UnitpayValidationException when $domain is not a bare host + */ + private function assertValidDomain(string $domain): void + { + // Labels of 1-63 alphanumerics/hyphens (not hyphen-edged), 253 chars total, optional :port. + $label = '[A-Za-z0-9](?:[A-Za-z0-9-]{0,61}[A-Za-z0-9])?'; + $host = '(?:' . $label . '\.)*' . $label; + if (preg_match('~^(?=.{1,253}(?::|$))' . $host . '(?::\d{1,5})?$~', $domain) !== 1) { + throw new UnitpayValidationException( + 'Domain must be a bare host such as "unitpay.ru", without scheme, path or query, got ' + . var_export($domain, true) + ); + } + } + /** * Machine-readable fingerprint token for the form() URL: php__. * major.minor so the exact PHP patch is not exposed in the buyer-visible payment form URL. diff --git a/src/Webhook/WebhookVerifier.php b/src/Webhook/WebhookVerifier.php index 36bc191..f972b6f 100644 --- a/src/Webhook/WebhookVerifier.php +++ b/src/Webhook/WebhookVerifier.php @@ -2,7 +2,10 @@ namespace Unitpay\Webhook; +use DateTimeImmutable; +use DateTimeZone; use Unitpay\Exception\UnitpayIpException; +use Unitpay\Exception\UnitpayReplayException; use Unitpay\Exception\UnitpaySignatureException; use Unitpay\Exception\UnitpayUnsupportedMethodException; use Unitpay\Exception\UnitpayValidationException; @@ -11,9 +14,14 @@ /** * Verifies inbound Unitpay webhooks and builds the handler responses. A webhook is - * trusted only when BOTH the SHA-256 signature (constant-time) and the source-IP - * allowlist pass. Also owns the keep-fresh IP allowlist (refresh from the published - * feed, merchant additions, effective union). + * trusted only when ALL of the SHA-256 signature (constant-time), the freshness of its + * `date` and the source-IP allowlist pass, checked in that order. Also owns the + * keep-fresh IP allowlist (refresh from the published feed, merchant additions, + * effective union). + * + * The freshness check is what stops a captured webhook from being replayed indefinitely: + * a signature proves a request was genuine once, not that it is genuine now. It is on by + * default with a 300s window — see setWebhookTolerance(). */ class WebhookVerifier { @@ -24,6 +32,17 @@ class WebhookVerifier */ private const SUPPORTED_PARTNER_METHODS = ['check', 'pay', 'preauth', 'error']; + /** + * Timezone of the `date` param. Unitpay documents the format (`Y-m-d H:i:s`) but not + * the zone; it is Moscow time, fixed offset. Parsing against it explicitly — rather + * than with strtotime(), which honours the ambient date.timezone — is what keeps the + * verdict identical on a UTC server and a Moscow one. + */ + private const WEBHOOK_TIMEZONE = '+03:00'; + + /** Default replay window, in seconds. Matches what Stripe uses. */ + private const DEFAULT_TOLERANCE = 300; + private ?string $secretKey; private SignatureBuilder $signature; private TransportInterface $transport; @@ -48,6 +67,7 @@ class WebhookVerifier */ private array $customIps = []; private ?IpAllowlist $ipAllowlist = null; + private int $tolerance = self::DEFAULT_TOLERANCE; private ?string $handlerMethod = null; /** @var array|null */ private ?array $handlerParams = null; @@ -110,6 +130,10 @@ public function checkHandlerRequest(): bool throw new UnitpaySignatureException('Wrong signature'); } + // After the signature, never before it: an unsigned payload must not be usable to + // probe how far this server's clock is off. + $this->assertFresh($params); + if (!$this->isAllowedIp($ip)) { throw new UnitpayIpException('IP address Error'); } @@ -139,6 +163,28 @@ public function getHandlerParams(): ?array return $this->handlerParams; } + /** + * Sets how far the webhook's `date` may be from this server's clock, in seconds. + * Defaults to 300. Pass 0 to disable the check — a webhook then stays replayable for + * as long as its signature is valid, which is forever. + * + * Raise it if your handler sits behind a queue that can hold a webhook for minutes, + * or if the server clock is not synchronised. Both are better fixed than tolerated. + * + * @throws UnitpayValidationException on a negative value + */ + public function setWebhookTolerance(int $seconds): self + { + if ($seconds < 0) { + throw new UnitpayValidationException( + sprintf('Webhook tolerance must not be negative, got %d.', $seconds) + ); + } + $this->tolerance = $seconds; + + return $this; + } + /** * Overrides the list of Unitpay IPs allowed to call the handler. Fully replaces the * built-in default (or previously fetched) list, but does NOT touch the merchant IPs @@ -146,10 +192,13 @@ public function getHandlerParams(): ?array * * Passing an empty array with no addAllowedIps() entries leaves the allowlist empty, * so every webhook is rejected (fail-closed, not a no-op) — pass at least one IP/CIDR. - * @param string[] $ips + * + * @param string[] $ips exact IPs and/or CIDR ranges + * @throws UnitpayValidationException on a malformed entry */ public function setAllowedIps(array $ips): self { + $this->assertValidEntries($ips); $this->supportedUnitpayIp = $ips; $this->ipAllowlist = null; return $this; @@ -159,15 +208,40 @@ public function setAllowedIps(array $ips): self * Adds the merchant's own IP/CIDR ranges (e.g. your proxy/relay) on top of the * Unitpay list. Preserved across refreshAllowedIps()/setAllowedIps(). Duplicates * are removed. + * * @param string[] $ips exact IPs and/or CIDR ranges + * @throws UnitpayValidationException on a malformed entry */ public function addAllowedIps(array $ips): self { + $this->assertValidEntries($ips); $this->customIps = array_values(array_unique(array_merge($this->customIps, $ips))); $this->ipAllowlist = null; return $this; } + /** + * Rejects malformed allowlist entries up front. The whole call fails, so the allowlist + * is never left half-configured. + * + * The feed path does NOT go through here: refreshAllowedIps() stays fail-safe and drops + * bad entries instead of throwing. Only the two manual setters throw. + * + * @param string[] $ips + * @throws UnitpayValidationException + */ + private function assertValidEntries(array $ips): void + { + foreach ($ips as $entry) { + if (!IpAllowlist::isValidEntry($entry)) { + throw new UnitpayValidationException( + 'Invalid IP allowlist entry: ' . var_export($entry, true) + . '. Expected an IPv4/IPv6 address or a CIDR range like "77.75.153.0/25".' + ); + } + } + } + /** * Fetches Unitpay's current published webhook IPs and makes them the allowlist. * @@ -214,6 +288,84 @@ public function getErrorHandlerResponse(string $message): string return (string) json_encode(['error' => ['message' => $message]]); } + /** + * Current unix time. A seam so tests can move the clock without sleeping — the same + * role getIp() and isAllowedIp() play for the request. + */ + protected function now(): int + { + return time(); + } + + /** + * Rejects a webhook whose payment timestamp is too far from this server's clock. + * + * Safe to trust because `date` is part of the signed payload — SignatureBuilder hashes + * every param except the signature itself — so it cannot be back-dated, and it cannot + * be stripped either: removing it changes the hash and the signature check above fails + * first. That last point is why an absent `date` is allowed through rather than + * refused. It is documented for all four inbound methods, but if Unitpay ever omits it + * the absence is theirs, not an attacker's, and rejecting those webhooks would cost + * the merchant real notifications while preventing nothing. + * + * @param array $params + * @throws UnitpayReplayException when the timestamp is unreadable or outside the window + */ + private function assertFresh(array $params): void + { + if ($this->tolerance === 0 || !isset($params['date'])) { + return; + } + + $timestamp = self::parseWebhookDate($params['date']); + if ($timestamp === null) { + throw new UnitpayReplayException(sprintf( + 'Webhook timestamp "%s" is not a valid Y-m-d H:i:s date.', + is_scalar($params['date']) ? (string) $params['date'] : gettype($params['date']) + )); + } + + $drift = abs($this->now() - $timestamp); + if ($drift > $this->tolerance) { + throw new UnitpayReplayException(sprintf( + 'Webhook timestamp %s is outside the %ds tolerance window (off by %ds).', + (string) $params['date'], + $this->tolerance, + $drift + )); + } + } + + /** + * @param mixed $value + * @return int|null unix timestamp, or null when the value is not a well-formed date + */ + private static function parseWebhookDate($value): ?int + { + if (!is_string($value)) { + return null; + } + + $date = DateTimeImmutable::createFromFormat( + 'Y-m-d H:i:s', + $value, + new DateTimeZone(self::WEBHOOK_TIMEZONE) + ); + if ($date === false) { + return null; + } + + // createFromFormat accepts overflowing components ("2026-13-45") and reports them + // as warnings instead of failing, so the strict answer needs this second look. + // PHP >= 8.2 returns false here when there is nothing to report. + $errors = DateTimeImmutable::getLastErrors(); + if (is_array($errors) && ($errors['warning_count'] > 0 || $errors['error_count'] > 0)) { + return null; + } + + return $date->getTimestamp(); + } + /** * Sender IP of the inbound request (the overridden clientIp or $_SERVER['REMOTE_ADDR']). * Override for proxy-aware logic. @@ -239,11 +391,18 @@ protected function isAllowedIp(string $ip): bool /** * Fetches and validates the published webhook IP feed. + * + * Deliberately throws nothing, unlike the API services which turn the same transport + * results into typed exceptions: a webhook handler must not start failing because a + * best-effort feed refresh could not reach the server. Any non-2xx, any transport + * error and any malformed payload yield null, and the caller keeps the current list. + * * @return string[]|null validated non-empty list, or null on any error */ private function fetchUnitpayIps(): ?array { - $body = $this->transport->send($this->ipsUrl); - return is_string($body) ? IpAllowlist::parseWebhooksFeed($body) : null; + $response = $this->transport->request($this->ipsUrl); + + return $response->isSuccessful() ? IpAllowlist::parseWebhooksFeed($response->getBody()) : null; } } diff --git a/tests/Api/AbstractServiceTest.php b/tests/Api/AbstractServiceTest.php index fde5a34..e54b287 100644 --- a/tests/Api/AbstractServiceTest.php +++ b/tests/Api/AbstractServiceTest.php @@ -5,7 +5,11 @@ use InvalidArgumentException; use PHPUnit\Framework\TestCase; use Tests\Support\FakeTransport; +use Unitpay\Exception\UnitpayHttpException; +use Unitpay\Exception\UnitpayNetworkException; +use Unitpay\Exception\UnitpayResponseException; use Unitpay\Exception\UnitpayTransportException; +use Unitpay\Http\Response; use Unitpay\Model\CashItem; use Unitpay\Unitpay; @@ -63,8 +67,9 @@ public function testCashItemsFromSetterAreSentByService(): void $this->assertStringContainsString('cashItems=', $url); $this->assertStringContainsString('customerEmail=', $url); - $query = $transport->query(); - $items = json_decode(base64_decode((string) $query['cashItems']), true); + // parse_str() types query values as array|string; cashItems is always scalar here. + $encoded = $transport->query()['cashItems'] ?? ''; + $items = json_decode(base64_decode(is_string($encoded) ? $encoded : ''), true); $this->assertSame('Coffee', $items[0]['name']); } @@ -105,11 +110,12 @@ public function testFluentSetterParamsDoNotBleedIntoNextCall(): void } /** - * Fluent-setter params are cleared once the request has been attempted — on a transport - * failure too, not only on success — so a stale receipt cannot leak into an unrelated - * later call on a reused instance. A retry must re-apply the setters (symmetric with form()). + * A CONSUMING call clears the fluent-setter params once the request has been attempted — + * on a transport failure too, not only on success — so a stale receipt cannot leak into + * an unrelated later order on a reused instance. A retry must re-apply the setters + * (symmetric with form()). */ - public function testFluentSetterParamsAreClearedAfterFailedCall(): void + public function testFluentSetterParamsAreClearedAfterFailedConsumingCall(): void { // The first call simulates a transport failure (false), later ones succeed. $transport = new FakeTransport(false, '{"result":{}}'); @@ -117,26 +123,59 @@ public function testFluentSetterParamsAreClearedAfterFailedCall(): void $unitpay->setCashItems([new CashItem('Coffee', 1, 100.0)]); try { - $unitpay->payments()->getPayment(1); + $unitpay->payments()->initPayment('order-1', 100, 7, 'card'); $this->fail('expected a transport exception on the first call'); } catch (UnitpayTransportException $e) { // expected: the transport returned false } - $unitpay->payments()->getPayment(2); + $unitpay->payments()->initPayment('order-2', 100, 7, 'card'); - // The receipt was consumed by the failed call and did NOT leak into the next one. $this->assertStringContainsString('cashItems=', $transport->url(0)); $this->assertStringNotContainsString('cashItems=', $transport->url(1)); } - public function testNonObjectResponseIsReportedAsTemporaryServerError(): void + /** + * A lookup issued between setCashItems() and initPayment() must not swallow the + * receipt: only the calls that accept the fluent-setter params may drain them. + */ + public function testNonConsumingCallNeitherReceivesNorConsumesPendingParams(): void { - $unitpay = new Unitpay('unitpay.test', 'secret', new FakeTransport('this is not json')); + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'my-secret', $transport); + $unitpay->setCashItems([new CashItem('Coffee', 1, 100.0)]) + ->setCustomerEmail('buyer@example.com'); - $this->expectException(InvalidArgumentException::class); - $this->expectExceptionMessage('Temporary server error'); - $unitpay->payments()->getPayment(1); + $unitpay->reference()->getPartner('partner@example.com'); + $unitpay->payments()->initPayment('order-1', 100, 7, 'card'); + + $this->assertStringNotContainsString('cashItems=', $transport->url(0)); + $this->assertStringNotContainsString('customerEmail=', $transport->url(0)); + $this->assertStringContainsString('cashItems=', $transport->url(1)); + $this->assertStringContainsString('customerEmail=', $transport->url(1)); + } + + /** + * A failing non-consuming call must not eat the pending params either — the params + * belong to the payment that has not been sent yet. + */ + public function testFailedNonConsumingCallLeavesPendingParamsIntact(): void + { + $transport = new FakeTransport(false, '{"result":{}}'); + $unitpay = new Unitpay('unitpay.test', 'my-secret', $transport); + $unitpay->setCashItems([new CashItem('Coffee', 1, 100.0)]); + + try { + $unitpay->payments()->getPayment(1); + $this->fail('expected a transport exception on the first call'); + } catch (UnitpayTransportException $e) { + // expected: the transport returned false + } + + $unitpay->payments()->initPayment('order-1', 100, 7, 'card'); + + $this->assertStringNotContainsString('cashItems=', $transport->url(0)); + $this->assertStringContainsString('cashItems=', $transport->url(1)); } public function testMissingSecretThrows(): void @@ -148,20 +187,128 @@ public function testMissingSecretThrows(): void $unitpay->payments()->getPayment(1); } - /** A transport failure is a typed exception, still catchable as InvalidArgumentException. */ - public function testTransportFailureThrowsTypedTransportException(): void + /** + * A 2xx whose body is not a JSON object is a protocol problem, not a network one: + * the server answered, it just did not answer with what the API promises. + */ + public function testNonJsonBodyThrowsResponseException(): void { - $unitpay = new Unitpay('unitpay.test', 'secret', new FakeTransport(false)); + $unitpay = new Unitpay('unitpay.test', 'secret', new FakeTransport('this is not json')); + + try { + $unitpay->payments()->getPayment(1); + $this->fail('expected a response exception'); + } catch (UnitpayResponseException $e) { + $this->assertSame('this is not json', $e->getResponseBody()); + $this->assertInstanceOf(UnitpayTransportException::class, $e); + $this->assertInstanceOf(InvalidArgumentException::class, $e); + } + } + + /** A connect-phase failure never reached the server; the message must say so. */ + public function testConnectFailureThrowsNetworkException(): void + { + $transport = new FakeTransport(Response::failed(7, 'Failed to connect to unitpay.test port 443', false)); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + + try { + $unitpay->payments()->getPayment(1); + $this->fail('expected a network exception'); + } catch (UnitpayNetworkException $e) { + $this->assertSame(7, $e->getErrno()); + $this->assertNotNull($e->getTransportError()); + $this->assertStringContainsString('Failed to connect', (string) $e->getTransportError()); + $this->assertStringContainsString('was not sent', $e->getMessage()); + } + } + + /** + * A read timeout carries the same cURL errno as a connect timeout but a completely + * different consequence: the server saw the request and may already have created the + * payment. The message has to say that out loud — it is the difference between "retry + * safely" and "check before retrying". + */ + public function testReadTimeoutSaysTheRequestMayHaveBeenProcessed(): void + { + $transport = new FakeTransport(Response::failed(28, 'Operation timed out after 10001 milliseconds', true)); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + + try { + $unitpay->payments()->initPayment('order-1', 100, 7, 'card'); + $this->fail('expected a network exception'); + } catch (UnitpayNetworkException $e) { + $this->assertSame(28, $e->getErrno()); + $this->assertStringContainsString('may already have been processed', $e->getMessage()); + $this->assertStringNotContainsString('was not sent', $e->getMessage()); + } + } + + public function testHttp404ThrowsHttpExceptionCarryingTheStatus(): void + { + $transport = new FakeTransport(Response::received(404, '{"error":{"message":"Not found"}}')); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + + try { + $unitpay->payments()->getPayment(1); + $this->fail('expected an http exception'); + } catch (UnitpayHttpException $e) { + $this->assertSame(404, $e->getStatusCode()); + $this->assertStringContainsString('404', $e->getMessage()); + } + } + + /** + * A 500 with an HTML error page used to be indistinguishable from a timeout. The body + * is what an integrator needs to quote in a support ticket, so it must survive. + */ + public function testHttp500CarriesTheResponseBody(): void + { + $html = '

502 Bad Gateway

'; + $transport = new FakeTransport(Response::received(500, $html)); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + + try { + $unitpay->payments()->getPayment(1); + $this->fail('expected an http exception'); + } catch (UnitpayHttpException $e) { + $this->assertSame(500, $e->getStatusCode()); + $this->assertSame($html, $e->getResponseBody()); + } + } + + /** + * The five cases above are distinct classes now, but a caller that only wants "the + * request failed" must still get away with a single catch — and with the pre-4.0 + * InvalidArgumentException catch it may already have. + * + * @dataProvider transportFailures + * @param string|false|Response $result + */ + public function testEveryTransportFailureIsStillOneCatch($result): void + { + $unitpay = new Unitpay('unitpay.test', 'secret', new FakeTransport($result)); try { $unitpay->payments()->getPayment(1); $this->fail('expected a transport exception'); } catch (UnitpayTransportException $e) { $this->assertInstanceOf(InvalidArgumentException::class, $e); - $this->assertStringContainsString('Temporary server error', $e->getMessage()); + $this->assertStringNotContainsString('Temporary server error', $e->getMessage()); } } + /** @return array */ + public function transportFailures(): array + { + return [ + 'connect failure' => [Response::failed(7, 'Failed to connect', false)], + 'read timeout' => [Response::failed(28, 'Operation timed out', true)], + 'http 404' => [Response::received(404, '')], + 'http 500' => [Response::received(500, 'oops')], + 'non-json 200' => ['this is not json'], + ]; + } + /** Account-level methods can override the project key with the account key (secretKey). */ public function testExplicitSecretKeyOverridesInstanceKey(): void { diff --git a/tests/Api/PaymentServiceTest.php b/tests/Api/PaymentServiceTest.php index bd44725..562b007 100644 --- a/tests/Api/PaymentServiceTest.php +++ b/tests/Api/PaymentServiceTest.php @@ -4,6 +4,7 @@ use PHPUnit\Framework\TestCase; use Tests\Support\FakeTransport; +use Unitpay\Model\CashItem; use Unitpay\Unitpay; final class PaymentServiceTest extends TestCase @@ -19,7 +20,10 @@ public function testInitPaymentReturnsDecodedResponseViaInjectedTransport(): voi $response = $unitpay->payments()->initPayment('1', 100, 7, 'card'); - $this->assertSame(42, $response->result->receiptId); + $this->assertSame( + ['result' => ['receiptId' => 42]], + json_decode((string) json_encode($response), true) + ); } public function testInitPaymentSendsItsRequiredParams(): void @@ -105,4 +109,33 @@ public function testOffsetAdvanceSendsLoginAndPaymentId(): void $this->assertSame('partner@example.com', $query['login']); $this->assertSame('555', $query['paymentId']); } + + /** + * offsetAdvance is a consuming call: the advance-offset receipt is optional, but when + * it is set via setCashItems() it must reach the request and be cleared afterwards. + */ + public function testOffsetAdvanceCarriesTheAccumulatedReceipt(): void + { + $transport = new FakeTransport(); + $unitpay = $this->unitpay($transport); + $unitpay->setCashItems([new CashItem('Coffee', 1, 100.0)]); + + $unitpay->payments()->offsetAdvance('partner@example.com', 555); + $unitpay->payments()->offsetAdvance('partner@example.com', 556); + + $this->assertStringContainsString('cashItems=', $transport->url(0)); + $this->assertStringNotContainsString('cashItems=', $transport->url(1)); + } + + /** getPayment does not accept a receipt, so it must never carry one. */ + public function testGetPaymentNeverCarriesTheAccumulatedReceipt(): void + { + $transport = new FakeTransport(); + $unitpay = $this->unitpay($transport); + $unitpay->setCashItems([new CashItem('Coffee', 1, 100.0)]); + + $unitpay->payments()->getPayment(555); + + $this->assertStringNotContainsString('cashItems=', $transport->url(0)); + } } diff --git a/tests/Api/TelemetryTest.php b/tests/Api/TelemetryTest.php index 232288d..8c8382a 100644 --- a/tests/Api/TelemetryTest.php +++ b/tests/Api/TelemetryTest.php @@ -20,7 +20,7 @@ public function testServiceCallSendsFingerprintHeaders(): void $unitpay->payments()->getPayment(1); $ua = $transport->header('User-Agent'); - $client = $transport->header('X-Unitpay-Client'); + $client = $transport->header('Unitpay-Client'); $this->assertSame('unitpay-php-sdk/' . Unitpay::VERSION . ' api/' . Unitpay::API_VERSION, $ua); @@ -33,6 +33,446 @@ public function testServiceCallSendsFingerprintHeaders(): void $this->assertSame('unitpay', $decoded['publisher']); } + public function testModuleAndStackReachBothHeaders(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + $unitpay->setModule('unitpay-woocommerce', '2.1') + ->setStack(['WordPress' => '6.5', 'WooCommerce' => '8.2']); + + $unitpay->payments()->getPayment(1); + + // Outermost host first, the module last as the narrowest context. + $this->assertSame( + 'unitpay-php-sdk/' . Unitpay::VERSION . ' api/' . Unitpay::API_VERSION + . ' WordPress/6.5 WooCommerce/8.2 unitpay-woocommerce/2.1', + $transport->header('User-Agent') + ); + + $decoded = json_decode((string) $transport->header('Unitpay-Client'), true); + $this->assertSame(['name' => 'unitpay-woocommerce', 'version' => '2.1'], $decoded['module']); + $this->assertSame( + [ + ['name' => 'WordPress', 'version' => '6.5'], + ['name' => 'WooCommerce', 'version' => '8.2'], + ], + $decoded['stack'] + ); + } + + /** + * The four-layer stack the three fixed slots could not express: a solution on top of a + * CMS, with the payment module on top of that. + */ + public function testAFourLayerStackIsExpressible(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + $unitpay->setModule('unitpay-bitrix', '3.1') + ->setStack(['Bitrix' => '22.0', 'Aspro Optimus' => '1.8.2']); + + $unitpay->payments()->getPayment(1); + + $decoded = json_decode((string) $transport->header('Unitpay-Client'), true); + $this->assertSame( + ['Bitrix', 'Aspro Optimus'], + array_column($decoded['stack'], 'name') + ); + $this->assertSame('unitpay-bitrix', $decoded['module']['name']); + } + + /** Idempotent by design, so a bootstrap that runs twice cannot double the stack. */ + public function testSetStackReplacesRatherThanAppends(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + $unitpay->setStack(['WordPress' => '6.5']); + $unitpay->setStack(['Bitrix' => '22.0']); + + $unitpay->payments()->getPayment(1); + + $decoded = json_decode((string) $transport->header('Unitpay-Client'), true); + $this->assertSame([['name' => 'Bitrix', 'version' => '22.0']], $decoded['stack']); + } + + /** A list was passed where a map was meant; "0" is not a product name. */ + public function testANumericKeyIsDropped(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + $unitpay->setStack(['WordPress', 'Bitrix' => '22.0']); + + $unitpay->payments()->getPayment(1); + + $decoded = json_decode((string) $transport->header('Unitpay-Client'), true); + $this->assertSame([['name' => 'Bitrix', 'version' => '22.0']], $decoded['stack']); + } + + /** A list bounds the header where three fixed slots used to bound it for free. */ + public function testTheStackIsCappedAtEightEntries(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + + $stack = []; + for ($i = 1; $i <= 12; $i++) { + $stack['Product' . $i] = '1.0'; + } + $unitpay->setStack($stack); + + $unitpay->payments()->getPayment(1); + + $decoded = json_decode((string) $transport->header('Unitpay-Client'), true); + $this->assertCount(8, $decoded['stack']); + $this->assertSame('Product8', $decoded['stack'][7]['name']); + } + + /** + * The reason the JSON header carries an object rather than a joined string: a Composer + * package name contains the separator, so "unitpay/woocommerce/2.1" gives a consumer + * three segments and one delimiter to guess between. + */ + public function testANameContainingASlashStaysSeparable(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + $unitpay->setModule('unitpay/woocommerce', '2.1'); + + $unitpay->payments()->getPayment(1); + + $client = (string) $transport->header('Unitpay-Client'); + $this->assertStringContainsString('"module":{"name":"unitpay/woocommerce"', $client); + + $decoded = json_decode($client, true); + $this->assertSame(['name' => 'unitpay/woocommerce', 'version' => '2.1'], $decoded['module']); + } + + /** + * An empty stack must not appear at all, rather than appear as `[]` — an integration with + * nothing under it, such as a bare script or a Telegram bot, is an ordinary case. + */ + public function testAnEmptyStackIsOmitted(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + $unitpay->setModule('unitpay-telegram-bot-template', '1.0'); + + $unitpay->payments()->getPayment(1); + + $decoded = json_decode((string) $transport->header('Unitpay-Client'), true); + $this->assertArrayHasKey('module', $decoded); + $this->assertArrayNotHasKey('stack', $decoded); + } + + /** And the same the other way round: a stack with no module named. */ + public function testAnUnsetModuleIsOmitted(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + $unitpay->setStack(['Laravel' => '11.0']); + + $unitpay->payments()->getPayment(1); + + $decoded = json_decode((string) $transport->header('Unitpay-Client'), true); + $this->assertArrayHasKey('stack', $decoded); + $this->assertArrayNotHasKey('module', $decoded); + } + + /** + * The facade caches service objects on first use, so a value set afterwards has to + * reach the service that already exists — which only works because ClientInfo is + * shared by reference rather than copied into the service at construction. + */ + public function testAValueSetAfterAServiceWasBuiltStillTakesEffect(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + + $unitpay->payments()->getPayment(1); + $unitpay->setStack(['Bitrix' => '22.0']); + $unitpay->payments()->getPayment(2); + + $this->assertStringNotContainsString('Bitrix/22.0', (string) $transport->header('User-Agent', 0)); + $this->assertStringContainsString('Bitrix/22.0', (string) $transport->header('User-Agent', 1)); + } + + public function testDisableTelemetryDropsTheClientHeaderButKeepsTheSdkVersion(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + $unitpay->setStack(['Bitrix' => '22.0'])->disableTelemetry(); + + $unitpay->payments()->getPayment(1); + + $this->assertNull($transport->header('Unitpay-Client')); + $this->assertSame('unitpay-php-sdk/' . Unitpay::VERSION, $transport->header('User-Agent')); + } + + /** + * A blank half would emit a meaningless "Bitrix/" or "/22.0" token, so the value is + * dropped — but dropped silently. These setters run in an integration's bootstrap, and + * a CMS that stops exposing its version string must cost a field in a header, not a + * checkout. + * + * @dataProvider incompleteValues + */ + public function testAnIncompleteModuleIsIgnoredRatherThanRejected(string $name, string $version): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + + $unitpay->setModule($name, $version); + $unitpay->payments()->getPayment(1); + + $decoded = json_decode((string) $transport->header('Unitpay-Client'), true); + $this->assertArrayNotHasKey('module', $decoded); + $this->assertSame( + 'unitpay-php-sdk/' . Unitpay::VERSION . ' api/' . Unitpay::API_VERSION, + $transport->header('User-Agent') + ); + } + + /** @return array */ + public function incompleteValues(): array + { + return [ + 'empty name' => ['', '22.0'], + 'empty version' => ['Bitrix', ''], + 'blank name' => [' ', '22.0'], + ]; + } + + /** + * trim() only cleans the edges, so a CR/LF in the middle of a value used to reach the + * transport — which joins header lines with "\r\n" — and add a header line of the + * caller's choosing. Slot values often come from a module's settings screen. + */ + public function testControlCharactersCannotAddAHeaderLine(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + $unitpay->setModule("evil\r\nX-Injected: 1", '1.0'); + + $unitpay->payments()->getPayment(1); + + $ua = (string) $transport->header('User-Agent'); + $this->assertStringNotContainsString("\r", $ua); + $this->assertStringNotContainsString("\n", $ua); + + $decoded = json_decode((string) $transport->header('Unitpay-Client'), true); + $this->assertSame('evilX-Injected: 1', $decoded['module']['name']); + } + + /** + * Ignoring a blank value means leaving it alone, not clearing it: a setter that starts + * coming back empty costs the update, not the value already reported. + */ + public function testABlankOverwriteLeavesTheEarlierValueInPlace(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + $unitpay->setModule('unitpay-bitrix', '3.1'); + $unitpay->setModule('unitpay-bitrix', ''); + + $unitpay->payments()->getPayment(1); + + $decoded = json_decode((string) $transport->header('Unitpay-Client'), true); + $this->assertSame(['name' => 'unitpay-bitrix', 'version' => '3.1'], $decoded['module']); + } + + /** A value that is nothing but control characters has no usable half left. */ + public function testAValueOfOnlyControlCharactersIsDropped(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + $unitpay->setModule("\r\n\t", '3.1'); + + $unitpay->payments()->getPayment(1); + + $decoded = json_decode((string) $transport->header('Unitpay-Client'), true); + $this->assertArrayNotHasKey('module', $decoded); + } + + /** + * The cap counts bytes, so it can land inside a multi-byte character. 30 three-byte + * characters are 90 bytes; cutting at 64 leaves one stray byte, which must be dropped + * rather than shipped as a broken sequence. + */ + public function testAnOverlongValueIsTruncatedOnACharacterBoundary(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + $unitpay->setModule(str_repeat('中', 30), '1.0'); + + $unitpay->payments()->getPayment(1); + + $decoded = json_decode((string) $transport->header('Unitpay-Client'), true); + $this->assertSame(str_repeat('中', 21), $decoded['module']['name']); + $this->assertSame(1, preg_match('//u', $decoded['module']['name'])); + } + + /** + * json_encode returns false on invalid UTF-8, and the old `(string)` cast turned that + * into an empty header — so one legacy windows-1251 CMS name cost the entire payload, + * sdk_version and lang_version included. + */ + public function testAnInvalidlyEncodedNameCannotBlankTheHeader(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + // "Битрикс" as a legacy windows-1251 install would hand it over: not valid UTF-8. + $unitpay->setModule("\xC1\xE8\xF2\xF0\xE8\xEA\xF1", '22.0'); + + $unitpay->payments()->getPayment(1); + + $client = (string) $transport->header('Unitpay-Client'); + $this->assertNotSame('', $client); + + $decoded = json_decode($client, true); + $this->assertSame(Unitpay::VERSION, $decoded['sdk_version']); + $this->assertSame(PHP_VERSION, $decoded['lang_version']); + $this->assertArrayHasKey('module', $decoded); + + $this->assertSame( + 'unitpay-php-sdk/' . Unitpay::VERSION . ' api/' . Unitpay::API_VERSION, + $transport->header('User-Agent') + ); + } + + /** + * A User-Agent is an ASCII field. The JSON header keeps the name losslessly through + * \uXXXX escaping, which is also what keeps that header value itself ASCII. + */ + public function testANonAsciiNameRidesInTheJsonHeaderOnly(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + $unitpay->setModule('1С-Битрикс', '22.0'); + + $unitpay->payments()->getPayment(1); + + $client = (string) $transport->header('Unitpay-Client'); + $this->assertSame(1, preg_match('/^[\x20-\x7E]+$/', $client)); + + $decoded = json_decode($client, true); + $this->assertSame(['name' => '1С-Битрикс', 'version' => '22.0'], $decoded['module']); + + $this->assertSame( + 'unitpay-php-sdk/' . Unitpay::VERSION . ' api/' . Unitpay::API_VERSION, + $transport->header('User-Agent') + ); + } + + /** + * Everything above is the module path. The stack is a second, unbounded input surface, so + * each guarantee has to hold per entry — and one bad entry must not take its neighbours + * with it. + */ + public function testControlCharactersInAStackEntryCannotAddAHeaderLine(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + $unitpay->setStack(["evil\r\nX-Injected: 1" => '1.0']); + + $unitpay->payments()->getPayment(1); + + $ua = (string) $transport->header('User-Agent'); + $this->assertStringNotContainsString("\r", $ua); + $this->assertStringNotContainsString("\n", $ua); + + $decoded = json_decode((string) $transport->header('Unitpay-Client'), true); + $this->assertSame('evilX-Injected: 1', $decoded['stack'][0]['name']); + } + + public function testAStackEntryOfOnlyControlCharactersIsDroppedWithoutItsNeighbours(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + $unitpay->setStack([ + 'WordPress' => '6.5', + "\r\n\t" => '1.0', + 'WooCommerce' => '8.2', + ]); + + $unitpay->payments()->getPayment(1); + + $decoded = json_decode((string) $transport->header('Unitpay-Client'), true); + $this->assertSame(['WordPress', 'WooCommerce'], array_column($decoded['stack'], 'name')); + } + + public function testABlankVersionDropsOnlyItsOwnStackEntry(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + // Exactly what a CMS that stops exposing its version hands over. + $unitpay->setStack(['WordPress' => '6.5', 'WooCommerce' => '', 'Bitrix' => '22.0']); + + $unitpay->payments()->getPayment(1); + + $decoded = json_decode((string) $transport->header('Unitpay-Client'), true); + $this->assertSame(['WordPress', 'Bitrix'], array_column($decoded['stack'], 'name')); + } + + public function testAnOverlongStackEntryIsTruncatedOnACharacterBoundary(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + $unitpay->setStack([str_repeat('中', 30) => '1.0']); + + $unitpay->payments()->getPayment(1); + + $decoded = json_decode((string) $transport->header('Unitpay-Client'), true); + $this->assertSame(str_repeat('中', 21), $decoded['stack'][0]['name']); + $this->assertSame(1, preg_match('//u', $decoded['stack'][0]['name'])); + } + + public function testAnInvalidlyEncodedStackEntryCannotBlankTheHeader(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + // "Битрикс" in windows-1251: not valid UTF-8. + $unitpay->setStack(["\xC1\xE8\xF2\xF0\xE8\xEA\xF1" => '22.0']); + + $unitpay->payments()->getPayment(1); + + $client = (string) $transport->header('Unitpay-Client'); + $this->assertNotSame('', $client); + + $decoded = json_decode($client, true); + $this->assertSame(Unitpay::VERSION, $decoded['sdk_version']); + $this->assertSame(PHP_VERSION, $decoded['lang_version']); + $this->assertCount(1, $decoded['stack']); + + $this->assertSame( + 'unitpay-php-sdk/' . Unitpay::VERSION . ' api/' . Unitpay::API_VERSION, + $transport->header('User-Agent') + ); + } + + public function testANonAsciiStackEntryRidesInTheJsonHeaderOnly(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.test', 'secret', $transport); + $unitpay->setStack(['Bitrix' => '22.0', 'Аспро: Магазин' => '1.8.2']); + + $unitpay->payments()->getPayment(1); + + $client = (string) $transport->header('Unitpay-Client'); + $this->assertSame(1, preg_match('/^[\x20-\x7E]+$/', $client)); + + $decoded = json_decode($client, true); + $this->assertSame( + ['name' => 'Аспро: Магазин', 'version' => '1.8.2'], + $decoded['stack'][1] + ); + + // The ASCII entry still makes the User-Agent; the other one is absent, not mangled. + $this->assertSame( + 'unitpay-php-sdk/' . Unitpay::VERSION . ' api/' . Unitpay::API_VERSION . ' Bitrix/22.0', + $transport->header('User-Agent') + ); + } + /** The IP-feed fetch is a plain GET: no fingerprint headers ride along with it. */ public function testIpFeedFetchDoesNotCarryFingerprintHeaders(): void { @@ -42,6 +482,6 @@ public function testIpFeedFetchDoesNotCarryFingerprintHeaders(): void $unitpay->webhook()->refreshAllowedIps(); $this->assertNull($transport->header('User-Agent')); - $this->assertNull($transport->header('X-Unitpay-Client')); + $this->assertNull($transport->header('Unitpay-Client')); } } diff --git a/tests/ComposerRequirementsTest.php b/tests/ComposerRequirementsTest.php new file mode 100644 index 0000000..1cb8098 --- /dev/null +++ b/tests/ComposerRequirementsTest.php @@ -0,0 +1,236 @@ + the extension that provides it. + * + * Only extensions a build can actually drop are listed. `pcre`, `filter`, `hash`, + * `json` on PHP 8 and the standard library cannot be disabled on the supported range + * (`>=7.4`), so requiring a declaration for `preg_match()`, `filter_var()`, + * `hash_equals()` or `inet_pton()` would be noise rather than a guard. + * + * @var array + */ + private const DISABLEABLE_EXTENSIONS = [ + 'bcadd' => 'bcmath', + 'ctype_' => 'ctype', + 'curl_' => 'curl', + 'finfo_' => 'fileinfo', + 'gmp_' => 'gmp', + 'iconv' => 'iconv', + 'json_' => 'json', + 'mb_' => 'mbstring', + 'mime_content_type' => 'fileinfo', + 'openssl_' => 'openssl', + 'simplexml_' => 'simplexml', + ]; + + /** + * Every extension whose functions appear in `src/` is named in the manifest: in + * `require` when the SDK cannot work without it, or in `suggest` when the call sits + * behind a `function_exists()` guard — which is exactly and only `ext-curl`, because + * `CurlTransport` falls back to `file_get_contents()`. + */ + public function testEveryExtensionUsedInSrcIsDeclared(): void + { + $required = $this->extensionNames($this->manifestSection('require')); + $suggested = $this->extensionNames($this->manifestSection('suggest')); + $declared = array_merge($required, $suggested); + + $usages = $this->extensionUsagesInSrc(); + $this->assertNotSame([], $usages, 'Scanning src/ found no extension calls at all — the scanner is broken, not the manifest.'); + + foreach ($usages as $extension => $evidence) { + $this->assertContains( + $extension, + $declared, + sprintf( + 'src/ uses %s, so composer.json must declare "ext-%s" in require ' + . '(or in suggest when every call is guarded by function_exists()).', + $evidence, + $extension + ) + ); + } + + $this->assertContains('curl', $suggested, 'ext-curl belongs in suggest: the cURL transport is optional.'); + $this->assertNotContains( + 'curl', + $required, + 'ext-curl must not become a hard requirement — CurlTransport keeps a file_get_contents() fallback so the SDK installs where cURL is absent.' + ); + } + + /** + * `require` holds only `php` and `ext-*`. Zero Composer packages at runtime is the + * stance that keeps the PHP floor at 7.4 and the install friction at nothing; the + * benchmark in .ai-factory/references/ shows what the alternative costs. + */ + public function testRequireContainsNoComposerPackages(): void + { + foreach (array_keys($this->manifestSection('require')) as $requirement) { + $requirement = (string) $requirement; + + $this->assertTrue( + $requirement === 'php' || strpos($requirement, 'ext-') === 0, + sprintf('composer.json require must hold only "php" and "ext-*" entries, found "%s".', $requirement) + ); + } + } + + /** + * @return array the named top-level object from composer.json, or [] when absent + */ + private function manifestSection(string $section): array + { + $raw = file_get_contents(dirname(__DIR__) . '/composer.json'); + $this->assertIsString($raw, 'composer.json must be readable.'); + + $decoded = json_decode((string) $raw, true); + $manifest = is_array($decoded) ? $decoded : []; + $this->assertNotSame([], $manifest, 'composer.json must decode to a non-empty JSON object.'); + + return isset($manifest[$section]) && is_array($manifest[$section]) ? $manifest[$section] : []; + } + + /** + * Extension names, `ext-` prefix stripped, from one manifest section. + * + * @param array $section + * + * @return string[] + */ + private function extensionNames(array $section): array + { + $names = []; + + foreach (array_keys($section) as $key) { + $key = (string) $key; + + if (strpos($key, 'ext-') === 0) { + $names[] = substr($key, 4); + } + } + + return $names; + } + + /** + * Extensions the code in `src/` actually reaches for, with the call that proves it. + * + * @return array extension name => "function() in src/Path/File.php" + */ + private function extensionUsagesInSrc(): array + { + $root = dirname(__DIR__); + $usages = []; + + foreach ($this->phpFilesIn($root . '/src') as $path) { + $code = file_get_contents($path); + + if (!is_string($code)) { + continue; + } + + foreach ($this->functionCallsIn($code) as $function) { + $extension = $this->extensionProviding($function); + + if ($extension === null || isset($usages[$extension])) { + continue; + } + + $usages[$extension] = sprintf('%s() in %s', $function, str_replace($root . '/', '', $path)); + } + } + + ksort($usages); + + return $usages; + } + + /** + * @return string[] absolute paths, sorted so the reported evidence is stable + */ + private function phpFilesIn(string $directory): array + { + $paths = []; + $files = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator($directory, \FilesystemIterator::SKIP_DOTS) + ); + + foreach ($files as $file) { + if ($file instanceof \SplFileInfo && $file->getExtension() === 'php') { + $paths[] = $file->getPathname(); + } + } + + sort($paths); + + return $paths; + } + + /** + * Bare function names referenced by the source, via the tokenizer rather than a regex + * so that comments and string literals cannot fake a dependency — `function_exists( + * 'curl_init')` must not count on its own, and a docblock naming `mb_strlen()` must + * not demand ext-mbstring. Method calls and declarations are skipped for the same + * reason. + * + * @return string[] + */ + private function functionCallsIn(string $code): array + { + $calls = []; + $tokens = token_get_all($code); + $skipAfter = [T_OBJECT_OPERATOR, T_DOUBLE_COLON, T_FUNCTION, T_NEW, T_CONST]; + $previous = null; + + foreach ($tokens as $token) { + if (!is_array($token)) { + if (trim($token) !== '') { + $previous = null; + } + + continue; + } + + if (in_array($token[0], [T_WHITESPACE, T_COMMENT, T_DOC_COMMENT], true)) { + continue; + } + + if ($token[0] === T_STRING && !in_array($previous, $skipAfter, true)) { + $calls[] = strtolower($token[1]); + } + + $previous = $token[0]; + } + + return $calls; + } + + private function extensionProviding(string $function): ?string + { + foreach (self::DISABLEABLE_EXTENSIONS as $prefix => $extension) { + if (strpos($function, $prefix) === 0) { + return $extension; + } + } + + return null; + } +} diff --git a/tests/FloatHandlingTest.php b/tests/FloatHandlingTest.php index f57d9fd..0360689 100644 --- a/tests/FloatHandlingTest.php +++ b/tests/FloatHandlingTest.php @@ -35,7 +35,9 @@ private function sign(array $params): string } /** - * @return array + * parse_str() yields string values plus arrays for bracketed keys, hence the union. + * + * @return array|string> */ private function queryOf(string $url): array { diff --git a/tests/Http/CurlTransportTest.php b/tests/Http/CurlTransportTest.php new file mode 100644 index 0000000..c65e676 --- /dev/null +++ b/tests/Http/CurlTransportTest.php @@ -0,0 +1,103 @@ +assertSame(5, $transport->getConnectTimeout()); + $this->assertSame(10, $transport->getTimeout()); + } + + public function testTimeoutsAreConfigurable(): void + { + $transport = new CurlTransport(2, 30); + + $this->assertSame(2, $transport->getConnectTimeout()); + $this->assertSame(30, $transport->getTimeout()); + } + + /** + * cURL reads 0 as "wait forever". In a payment flow that is a request which never + * comes back, so it is rejected rather than passed through. + * + * @dataProvider nonPositiveTimeouts + */ + public function testNonPositiveConnectTimeoutIsRejected(int $seconds): void + { + $this->expectException(UnitpayValidationException::class); + $this->expectExceptionMessage('Connect timeout must be a positive number of seconds'); + + new CurlTransport($seconds, 10); + } + + /** + * @dataProvider nonPositiveTimeouts + */ + public function testNonPositiveTimeoutIsRejected(int $seconds): void + { + $this->expectException(UnitpayValidationException::class); + $this->expectExceptionMessage('Timeout must be a positive number of seconds'); + + new CurlTransport(5, $seconds); + } + + /** @return array */ + public function nonPositiveTimeouts(): array + { + return [ + 'zero' => [0], + 'negative' => [-1], + ]; + } + + /** The message has to name the offending value, or the caller has to go looking. */ + public function testRejectionMessageNamesTheValue(): void + { + try { + new CurlTransport(5, -3); + $this->fail('expected a validation exception'); + } catch (UnitpayValidationException $e) { + $this->assertStringContainsString('-3', $e->getMessage()); + } + } + + /** + * The stream path joins header lines with "\r\n", so a newline inside a value would add + * a header line of the caller's choosing. Nothing the SDK builds can carry one — this + * is the second line of defence, and it drops the line rather than raising, because a + * transport must not abort a payment over a diagnostic header. + */ + public function testHeaderLinesCarryingANewlineAreDropped(): void + { + $sanitized = CurlTransport::sanitizeHeaders([ + 'User-Agent: unitpay-php-sdk/4.0.0', + "Unitpay-Client: {\"lang\":\"php\"}\r\nX-Injected: 1", + 'Accept: application/json', + ]); + + $this->assertSame( + ['User-Agent: unitpay-php-sdk/4.0.0', 'Accept: application/json'], + $sanitized + ); + } + + public function testOrdinaryHeaderLinesPassThroughUnchanged(): void + { + $headers = ['User-Agent: unitpay-php-sdk/4.0.0', 'Unitpay-Client: {"lang":"php"}']; + + $this->assertSame($headers, CurlTransport::sanitizeHeaders($headers)); + } +} diff --git a/tests/Http/DefaultTransportTest.php b/tests/Http/DefaultTransportTest.php new file mode 100644 index 0000000..336d2ab --- /dev/null +++ b/tests/Http/DefaultTransportTest.php @@ -0,0 +1,47 @@ +assertInstanceOf(RetryingTransport::class, $transport); + + // assertInstanceOf does not narrow for PHPStan without phpstan/phpstan-phpunit, + // and the declared return type is the interface — so state the type once here. + /** @var RetryingTransport $transport */ + $inner = $transport->getInner(); + $this->assertInstanceOf(CurlTransport::class, $inner); + + /** @var CurlTransport $inner */ + $this->assertSame(5, $inner->getConnectTimeout()); + $this->assertSame(10, $inner->getTimeout()); + } + + /** The documented off switch. */ + public function testWithoutRetriesReturnsBareCurl(): void + { + $this->assertInstanceOf(CurlTransport::class, DefaultTransport::withoutRetries()); + } + + /** Each call builds its own stack: two clients must not share one transport instance. */ + public function testEachCallReturnsAFreshStack(): void + { + $this->assertNotSame(DefaultTransport::create(), DefaultTransport::create()); + } +} diff --git a/tests/Http/ResponseTest.php b/tests/Http/ResponseTest.php new file mode 100644 index 0000000..fde975c --- /dev/null +++ b/tests/Http/ResponseTest.php @@ -0,0 +1,103 @@ +assertSame(200, $response->getStatusCode()); + $this->assertSame('{"result":{}}', $response->getBody()); + $this->assertSame(['Content-Type: application/json'], $response->getHeaders()); + $this->assertSame(0, $response->getErrno()); + $this->assertSame('', $response->getTransportError()); + } + + /** A response that came back proves the request reached the server, whatever its status. */ + public function testReceivedAlwaysCountsAsRequestSent(): void + { + $this->assertTrue(Response::received(200, 'ok')->wasRequestSent()); + $this->assertTrue(Response::received(500, 'boom')->wasRequestSent()); + } + + public function testFailedHasNoStatusAndCarriesTheTransportError(): void + { + $response = Response::failed(7, 'Failed to connect to unitpay.ru port 443', false); + + $this->assertSame(0, $response->getStatusCode()); + $this->assertSame('', $response->getBody()); + $this->assertSame(7, $response->getErrno()); + $this->assertSame('Failed to connect to unitpay.ru port 443', $response->getTransportError()); + } + + /** + * The retry-safety signal. cURL reports CURLE_OPERATION_TIMEDOUT (28) for both a + * connect timeout and a read timeout, so the errno alone cannot tell whether the + * server saw the request. Only wasRequestSent() may drive a retry decision — a + * read timeout means the payment may already have been created. + */ + public function testWasRequestSentDistinguishesConnectFromReadTimeout(): void + { + $connectTimeout = Response::failed(28, 'Connection timed out after 5001 milliseconds', false); + $readTimeout = Response::failed(28, 'Operation timed out after 10001 milliseconds', true); + + $this->assertSame($connectTimeout->getErrno(), $readTimeout->getErrno()); + $this->assertFalse($connectTimeout->wasRequestSent()); + $this->assertTrue($readTimeout->wasRequestSent()); + } + + /** + * @dataProvider successfulStatuses + */ + public function testIsSuccessfulAcceptsTheWhole2xxRange(int $status): void + { + $this->assertTrue(Response::received($status, '')->isSuccessful()); + } + + /** @return array */ + public function successfulStatuses(): array + { + return [ + 'status 200' => [200], + 'status 201' => [201], + 'status 204' => [204], + 'status 299' => [299], + ]; + } + + /** + * @dataProvider unsuccessfulStatuses + */ + public function testIsSuccessfulRejectsEverythingOutside2xx(int $status): void + { + $this->assertFalse(Response::received($status, '')->isSuccessful()); + } + + /** @return array */ + public function unsuccessfulStatuses(): array + { + return [ + 'status 199' => [199], + 'status 301' => [301], + 'status 404' => [404], + 'status 429' => [429], + 'status 500' => [500], + ]; + } + + public function testFailedIsNeverSuccessful(): void + { + $this->assertFalse(Response::failed(6, 'Could not resolve host', false)->isSuccessful()); + } +} diff --git a/tests/Http/RetryingTransportTest.php b/tests/Http/RetryingTransportTest.php new file mode 100644 index 0000000..053aef0 --- /dev/null +++ b/tests/Http/RetryingTransportTest.php @@ -0,0 +1,252 @@ +request('https://unitpay.test/api?method=getPayment'); + + $this->assertSame(3, $inner->callCount()); + $this->assertSame(200, $response->getStatusCode()); + } + + /** + * The single most important case in this file. A read timeout carries the same cURL + * errno as a connect timeout, but the server saw the request — retrying it is how you + * charge a customer twice. + */ + public function testNeverRetriesAfterTheRequestReachedTheServer(): void + { + $inner = new FakeTransport( + Response::failed(28, 'Operation timed out after 10001 milliseconds', true), + Response::received(200, '{"result":{}}') + ); + $transport = new SleeplessRetryingTransport($inner, 3); + + $response = $transport->request('https://unitpay.test/api?method=initPayment'); + + $this->assertSame(1, $inner->callCount()); + $this->assertSame(0, $response->getStatusCode()); + $this->assertTrue($response->wasRequestSent()); + $this->assertSame([], $transport->sleeps()); + } + + /** + * A status means the server answered, so it processed the request far enough to + * decide. None of these may be repeated blindly — not even a 429, which other SDKs + * retry only because they can send an idempotency key with it. + * + * @dataProvider serverAnsweredStatuses + */ + public function testNeverRetriesAnyStatusTheServerReturned(int $status): void + { + $inner = new FakeTransport( + Response::received($status, 'body'), + Response::received(200, '{"result":{}}') + ); + $transport = new SleeplessRetryingTransport($inner, 3); + + $response = $transport->request('https://unitpay.test/api?method=initPayment'); + + $this->assertSame(1, $inner->callCount()); + $this->assertSame($status, $response->getStatusCode()); + } + + /** @return array */ + public function serverAnsweredStatuses(): array + { + return [ + 'status 409' => [409], + 'status 429' => [429], + 'status 500' => [500], + 'status 502' => [502], + 'status 503' => [503], + ]; + } + + /** + * A transport that cannot classify its own failure must never be retried. + * + * The file_get_contents fallback is the case that matters: it sees no connect/read + * phase, so a failure there may be a request Unitpay already processed. Retrying it + * is how a fallback install charges a customer twice, and docs/getting-started.md + * states outright that this path is never retried. `false` for "unknown" is not the + * same claim as `false` for "provably never sent" — only the latter may be retried. + * + * @dataProvider unclassifiableFailures + */ + public function testNeverRetriesAFailureTheTransportCouldNotClassify(Response $failure): void + { + $inner = new FakeTransport($failure, Response::received(200, '{"result":{}}')); + $transport = new SleeplessRetryingTransport($inner, 3); + + $transport->request('https://unitpay.test/api?method=initPayment'); + + $this->assertSame(1, $inner->callCount()); + $this->assertSame([], $transport->sleeps()); + } + + /** @return array */ + public function unclassifiableFailures(): array + { + return [ + 'stream fallback failure' => [ + Response::failed(Response::ERRNO_LOCAL, 'the fallback cannot report why', false), + ], + 'allow_url_fopen disabled' => [ + Response::failed(Response::ERRNO_LOCAL, 'ext-curl missing and allow_url_fopen off', false), + ], + ]; + } + + public function testDoesNotRetryASuccess(): void + { + $inner = new FakeTransport('{"result":{}}'); + $transport = new SleeplessRetryingTransport($inner, 3); + + $transport->request('https://unitpay.test/api?method=getPayment'); + + $this->assertSame(1, $inner->callCount()); + } + + public function testStopsAtTheConfiguredLimitAndReturnsTheLastFailure(): void + { + $inner = new FakeTransport(Response::failed(6, 'Could not resolve host', false)); + $transport = new SleeplessRetryingTransport($inner, 2); + + $response = $transport->request('https://unitpay.test/api?method=getPayment'); + + // 1 initial attempt + 2 retries. + $this->assertSame(3, $inner->callCount()); + $this->assertSame(6, $response->getErrno()); + $this->assertCount(2, $transport->sleeps()); + } + + public function testZeroRetriesPerformsExactlyOneCall(): void + { + $inner = new FakeTransport(Response::failed(7, 'Connection refused', false)); + $transport = new SleeplessRetryingTransport($inner, 0); + + $transport->request('https://unitpay.test/api?method=getPayment'); + + $this->assertSame(1, $inner->callCount()); + $this->assertSame([], $transport->sleeps()); + } + + /** + * A retry must re-send the same bytes. If the URL or the headers were rebuilt between + * attempts, a signed request could be re-signed differently — or the fluent-setter + * params could be dropped, which is why retries live below the service layer at all. + */ + public function testEveryAttemptSendsIdenticalUrlAndHeaders(): void + { + $inner = new FakeTransport(Response::failed(7, 'Connection refused', false)); + $transport = new SleeplessRetryingTransport($inner, 2); + $url = 'https://unitpay.test/api?method=initPayment&sum=100&signature=abc'; + $headers = ['User-Agent: unitpay-php-sdk/4.0.0', 'Unitpay-Client: {"lang":"php"}']; + + $transport->request($url, $headers); + + $this->assertSame(3, $inner->callCount()); + $this->assertSame($url, $inner->url(0)); + $this->assertSame($url, $inner->url(1)); + $this->assertSame($url, $inner->url(2)); + foreach ([0, 1, 2] as $attempt) { + $this->assertSame('unitpay-php-sdk/4.0.0', $inner->header('User-Agent', $attempt)); + $this->assertSame('{"lang":"php"}', $inner->header('Unitpay-Client', $attempt)); + } + } + + /** Backoff grows and stays under the cap, so a retry storm cannot stall a request thread. */ + public function testBackoffIsExponentialAndCapped(): void + { + $inner = new FakeTransport(Response::failed(7, 'Connection refused', false)); + $transport = new SleeplessRetryingTransport($inner, 4, 0.5, 2.0); + + $transport->request('https://unitpay.test/api?method=getPayment'); + + $sleeps = $transport->sleeps(); + $this->assertCount(4, $sleeps); + foreach ($sleeps as $delay) { + $this->assertGreaterThanOrEqual(0.25, $delay); + $this->assertLessThanOrEqual(2.0, $delay); + } + } + + /** + * Retries must not be invisible: a caller staring at a network error needs to know the + * SDK already tried three times before giving up. + */ + public function testTheReturnedFailureReportsHowManyAttemptsWereMade(): void + { + $inner = new FakeTransport(Response::failed(7, 'Connection refused', false)); + $transport = new SleeplessRetryingTransport($inner, 2); + + $response = $transport->request('https://unitpay.test/api?method=getPayment'); + + $this->assertStringContainsString('Connection refused', $response->getTransportError()); + $this->assertStringContainsString('3 attempts', $response->getTransportError()); + } + + /** A single attempt is not "1 attempts", and it is not a retry story worth telling. */ + public function testASingleAttemptDoesNotClaimToHaveRetried(): void + { + $inner = new FakeTransport(Response::failed(7, 'Connection refused', false)); + $transport = new SleeplessRetryingTransport($inner, 0); + + $response = $transport->request('https://unitpay.test/api?method=getPayment'); + + $this->assertSame('Connection refused', $response->getTransportError()); + } + + /** + * @dataProvider invalidConfigurations + */ + public function testInvalidConfigurationIsRejectedAtConstruction( + int $maxRetries, + float $baseDelay, + float $maxDelay, + string $expectedMessage + ): void { + $this->expectException(UnitpayValidationException::class); + $this->expectExceptionMessage($expectedMessage); + + new RetryingTransport(new FakeTransport(), $maxRetries, $baseDelay, $maxDelay); + } + + /** @return array */ + public function invalidConfigurations(): array + { + return [ + 'negative retries' => [-1, 0.5, 2.0, 'Max retries must not be negative'], + 'negative base delay' => [2, -0.5, 2.0, 'Base delay must not be negative'], + 'max delay below base' => [2, 2.0, 0.5, 'Max delay must not be smaller than the base delay'], + ]; + } +} diff --git a/tests/Model/Enum/PaymentObjectTest.php b/tests/Model/Enum/PaymentObjectTest.php new file mode 100644 index 0000000..8ed94b5 --- /dev/null +++ b/tests/Model/Enum/PaymentObjectTest.php @@ -0,0 +1,104 @@ +constants(); + + foreach (self::REMOVED_IN_4_0 as $name) { + $this->assertArrayNotHasKey( + $name, + $constants, + sprintf( + 'PaymentObject::%s was removed in 4.0.0 because the public API rejects it. ' + . 'Re-adding it is a regression, not a restoration.', + $name + ) + ); + } + } + + /** + * Pins the surviving dictionary whole: a value typo introduced while editing neighbouring + * lines fails here, and so does an undocumented backend addition — which CLAUDE.md requires + * to be recorded in CHANGELOG.md before it lands. + */ + public function testSupportedValuesMatchTheBackendDictionary(): void + { + $expected = [ + 'COMMODITY' => 'commodity', + 'JOB' => 'job', + 'SERVICE' => 'service', + 'LOTTERY' => 'lottery', + 'INTELLECTUAL_ACTIVITY' => 'intellectual_activity', + 'PAYMENT' => 'payment', + 'AGENT_COMMISSION' => 'agent_commission', + 'PAYMENT_2' => 'payment_2', + 'ANOTHER' => 'another', + 'PROPERTY_RIGHT' => 'property_right', + 'NON_OPERATING_GAIN' => 'non-operating_gain', + 'INSURANCE_PREMIUM' => 'insurance_premium', + 'SALES_TAX' => 'sales_tax', + 'RESORT_FEE' => 'resort_fee', + 'DEPOSIT' => 'deposit', + 'EXPENSE' => 'expense', + 'PENSION_INSURANCE_IP' => 'pension_insurance_ip', + 'PENSION_INSURANCE' => 'pension_insurance', + 'MEDICAL_INSURANCE_IP' => 'medical_insurance_ip', + 'MEDICAL_INSURANCE' => 'medical_insurance', + 'SOCIAL_INSURANCE' => 'social_insurance', + 'CASINO_PAYMENT' => 'casino_payment', + 'ISSUANCE_BANK' => 'issuance_bank', + 'COMMODITY_WITHOUT_MARK' => 'commodity_without_mark', + 'COMMODITY_MARK' => 'commodity_mark', + ]; + $actual = $this->constants(); + + // Sorted on both sides so declaration order stays free to change; content does not. + ksort($expected); + ksort($actual); + + $this->assertSame($expected, $actual); + } + + /** + * @return array + */ + private function constants(): array + { + /** @var array $constants */ + $constants = (new ReflectionClass(PaymentObject::class))->getConstants(); + + return $constants; + } +} diff --git a/tests/Support/FakeTransport.php b/tests/Support/FakeTransport.php index 63baea1..f6331ea 100644 --- a/tests/Support/FakeTransport.php +++ b/tests/Support/FakeTransport.php @@ -2,37 +2,46 @@ namespace Tests\Support; +use Unitpay\Http\Response; use Unitpay\Http\TransportInterface; /** * Test double for the outbound HTTP transport: records every call (URL + headers) and - * replays a queue of canned bodies, so the SDK can be exercised without the network. - * Replaces the constructor callable the pre-3.0 suite injected. + * replays a queue of canned results, so the SDK can be exercised without the network. + * + * A queued item may be: + * - a string → shorthand for HTTP 200 with that body (the common case); + * - false → shorthand for a connect-phase failure (no response, request not sent); + * - a Response → the full result, needed for status codes, headers, and the read-timeout + * case where the request DID reach the server. */ final class FakeTransport implements TransportInterface { /** @var array */ private array $calls = []; - /** @var array */ + /** @var array */ private array $responses; /** - * @param string|false ...$responses bodies returned by successive send() calls; the - * last one is reused once the queue runs out. Pass - * false to simulate a transport failure. + * @param string|false|Response ...$responses results returned by successive request() + * calls; the last one is reused once the + * queue runs out. */ public function __construct(...$responses) { - /** @var array $responses */ - $this->responses = $responses === [] ? ['{"result":{}}'] : $responses; + if ($responses === []) { + $responses = ['{"result":{}}']; + } + + /** @var array $responses */ + $this->responses = array_map([self::class, 'normalize'], $responses); } /** * @param string[] $headers - * @return string|false */ - public function send(string $url, array $headers = []) + public function request(string $url, array $headers = []): Response { $this->calls[] = ['url' => $url, 'headers' => $headers]; @@ -59,8 +68,10 @@ public function lastUrl(): string } /** - * Query string of the n-th call, parsed into an array. - * @return array + * Query string of the n-th call, parsed into an array. parse_str() yields string + * values plus arrays for bracketed keys, hence the union. + * + * @return array|string> */ public function query(int $index = 0): array { @@ -80,4 +91,21 @@ public function header(string $name, int $index = 0): ?string return null; } + + /** + * @param string|false|Response $response + */ + private static function normalize($response): Response + { + if ($response instanceof Response) { + return $response; + } + + if ($response === false) { + // errno 7 = CURLE_COULDNT_CONNECT: the historical meaning of a bare `false`. + return Response::failed(7, 'Simulated transport failure', false); + } + + return Response::received(200, $response); + } } diff --git a/tests/Support/FrozenClockWebhookVerifier.php b/tests/Support/FrozenClockWebhookVerifier.php new file mode 100644 index 0000000..d67123b --- /dev/null +++ b/tests/Support/FrozenClockWebhookVerifier.php @@ -0,0 +1,27 @@ +now = $timestamp; + + return $this; + } + + protected function now(): int + { + return $this->now; + } +} diff --git a/tests/Support/SleeplessRetryingTransport.php b/tests/Support/SleeplessRetryingTransport.php new file mode 100644 index 0000000..d134ee7 --- /dev/null +++ b/tests/Support/SleeplessRetryingTransport.php @@ -0,0 +1,26 @@ +sleeps; + } + + protected function sleep(float $seconds): void + { + $this->sleeps[] = $seconds; + } +} diff --git a/tests/UnitpayCashItemsTest.php b/tests/UnitpayCashItemsTest.php index 95a41f6..8678e04 100644 --- a/tests/UnitpayCashItemsTest.php +++ b/tests/UnitpayCashItemsTest.php @@ -23,8 +23,10 @@ private function serializedItems(Unitpay $unitpay): array { $url = $unitpay->form('pk', 1, 'acc', 'desc'); parse_str((string) parse_url($url, PHP_URL_QUERY), $query); + // parse_str() types query values as array|string; cashItems is always scalar here. + $encoded = $query['cashItems'] ?? ''; - return json_decode(base64_decode((string) $query['cashItems']), true); + return json_decode(base64_decode(is_string($encoded) ? $encoded : ''), true); } public function testRequiredFieldsAreAlwaysSerialized(): void diff --git a/tests/UnitpayFacadeTest.php b/tests/UnitpayFacadeTest.php index c35e923..7d0fee4 100644 --- a/tests/UnitpayFacadeTest.php +++ b/tests/UnitpayFacadeTest.php @@ -8,6 +8,9 @@ use Unitpay\Api\PayoutService; use Unitpay\Api\ReferenceService; use Unitpay\Api\SubscriptionService; +use Unitpay\Exception\UnitpayValidationException; +use Unitpay\Http\CurlTransport; +use Unitpay\Http\RetryingTransport; use Unitpay\Model\CashItem; use Unitpay\Unitpay; use Unitpay\Webhook\WebhookVerifier; @@ -72,18 +75,77 @@ public function testAllServicesShareTheInjectedTransport(): void } /** - * The fluent setters live on the facade but their params belong to whichever service - * is called next — the pending-params holder is shared, not per-service. + * The pending-params holder is shared across services rather than per-service, so a + * receipt set on the facade reaches PaymentService — but only the calls that accept it: + * a payout lookup in between neither receives nor consumes it. */ - public function testAccumulatedParamsReachAnyService(): void + public function testAccumulatedParamsReachTheConsumingServiceOnly(): void { $transport = new FakeTransport(); $unitpay = new Unitpay('unitpay.test', 'secret', $transport); $unitpay->setCashItems([new CashItem('Coffee', 1, 100.0)]); $unitpay->payouts()->massPaymentCommissions('partner@example.com'); + $unitpay->payments()->initPayment('order-1', 100, 7, 'card'); - $this->assertArrayHasKey('cashItems', $transport->query()); + $this->assertArrayNotHasKey('cashItems', $transport->query(0)); + $this->assertArrayHasKey('cashItems', $transport->query(1)); + } + + /** + * @return array + */ + public function validDomainProvider(): array + { + return [ + 'production' => ['unitpay.ru'], + 'test tld' => ['unitpay.test'], + 'subdomain' => ['sandbox.unitpay.ru'], + 'single label' => ['localhost'], + 'host with port' => ['localhost:8080'], + 'hyphenated' => ['my-shop.example.com'], + ]; + } + + /** + * @dataProvider validDomainProvider + */ + public function testConstructorAcceptsBareHost(string $domain): void + { + $this->assertInstanceOf(Unitpay::class, new Unitpay($domain, 'secret')); + } + + /** + * @return array + */ + public function invalidDomainProvider(): array + { + return [ + 'with scheme' => ['https://unitpay.ru'], + 'scheme only' => ['http://'], + 'with path' => ['unitpay.ru/api'], + 'with query' => ['unitpay.ru?x=1'], + 'with fragment' => ['unitpay.ru#frag'], + 'with userinfo' => ['user@unitpay.ru'], + 'leading space' => [' unitpay.ru'], + 'trailing space' => ['unitpay.ru '], + 'empty' => [''], + 'hyphen edged' => ['-unitpay.ru'], + 'double dot' => ['unitpay..ru'], + ]; + } + + /** + * A scheme or path used to sail through and surface later as a transport error or an + * allowlist that silently failed to refresh. + * + * @dataProvider invalidDomainProvider + */ + public function testConstructorRejectsAnythingButABareHost(string $domain): void + { + $this->expectException(UnitpayValidationException::class); + $this->expectExceptionMessage('Domain must be a bare host'); + new Unitpay($domain, 'secret'); } /** The domain given to the constructor drives every endpoint the facade builds. */ @@ -99,7 +161,7 @@ public function testDomainDrivesFormAndApiEndpoints(): void $this->assertStringStartsWith('https://unitpay.test/api?', $transport->lastUrl()); } - /** Without an injected transport the facade still builds — it falls back to CurlTransport. */ + /** Without an injected transport the facade still builds — it falls back to the default stack. */ public function testFacadeIsUsableWithoutAnInjectedTransport(): void { $unitpay = new Unitpay('unitpay.test', 'secret'); @@ -107,4 +169,28 @@ public function testFacadeIsUsableWithoutAnInjectedTransport(): void $this->assertInstanceOf(PaymentService::class, $unitpay->payments()); $this->assertStringStartsWith('https://unitpay.test/pay/pk?', $unitpay->form('pk', 100, 'acc', 'desc')); } + + /** + * Retries being on by default is a promise made in the CHANGELOG and in + * docs/getting-started.md, and it lives in exactly one line of wiring. Every other test + * injects a FakeTransport, so replacing DefaultTransport::create() with a bare + * `new CurlTransport()` would leave the whole suite green while the promise quietly + * stopped being true. This is the assertion that notices. + * + * Reflection rather than a public getter: which transport the facade composed is an + * implementation detail, and exposing it just to test it would put it in the API. + */ + public function testFacadeDefaultsToTheRetryingStack(): void + { + $property = new \ReflectionProperty(Unitpay::class, 'transport'); + if (\PHP_VERSION_ID < 80100) { + // Required before 8.1, and deprecated from 8.5 — call it only where it does something. + $property->setAccessible(true); + } + + $transport = $property->getValue(new Unitpay('unitpay.test', 'secret')); + + $this->assertInstanceOf(RetryingTransport::class, $transport); + $this->assertInstanceOf(CurlTransport::class, $transport->getInner()); + } } diff --git a/tests/UnitpayFormTest.php b/tests/UnitpayFormTest.php index 08da885..eb031e0 100644 --- a/tests/UnitpayFormTest.php +++ b/tests/UnitpayFormTest.php @@ -3,6 +3,7 @@ namespace Tests; use PHPUnit\Framework\TestCase; +use Tests\Support\FakeTransport; use Unitpay\Exception\UnitpayValidationException; use Unitpay\Model\CashItem; use Unitpay\Signature\SignatureBuilder; @@ -13,7 +14,9 @@ final class UnitpayFormTest extends TestCase private const SECRET = 'secret'; /** - * @return array + * parse_str() yields string values plus arrays for bracketed keys, hence the union. + * + * @return array|string> */ private function queryOf(string $url): array { @@ -116,6 +119,25 @@ public function testFormClearsAccumulatedParamsAfterCall(): void $this->assertArrayNotHasKey('customerEmail', $second); } + /** + * A service call issued between the setters and form() must not swallow them: the + * receipt and customer belong to the form, not to the lookup that ran first. + */ + public function testFormKeepsSettersAcrossAnInterveningServiceCall(): void + { + $transport = new FakeTransport(); + $unitpay = new Unitpay('unitpay.ru', self::SECRET, $transport); + $unitpay->setCustomerEmail('customer@example.com') + ->setCashItems([new CashItem('X', 1, 100.0)]); + + $unitpay->payments()->getPayment(555); + $query = $this->queryOf($unitpay->form('pk', 100, 'acc', 'desc')); + + $this->assertStringNotContainsString('cashItems=', $transport->url(0)); + $this->assertSame('customer@example.com', $query['customerEmail']); + $this->assertArrayHasKey('cashItems', $query); + } + /** The form signature must cover ONLY the four vital params, not the setter params. */ public function testFormSignatureExcludesSetterParams(): void { diff --git a/tests/Webhook/AllowedIpsTest.php b/tests/Webhook/AllowedIpsTest.php index 51129eb..6a1f768 100644 --- a/tests/Webhook/AllowedIpsTest.php +++ b/tests/Webhook/AllowedIpsTest.php @@ -5,6 +5,7 @@ use PHPUnit\Framework\TestCase; use Tests\Support\FakeTransport; use Unitpay\Exception\UnitpayIpException; +use Unitpay\Exception\UnitpayValidationException; use Unitpay\Signature\SignatureBuilder; use Unitpay\Unitpay; use Unitpay\Webhook\WebhookVerifier; @@ -192,6 +193,84 @@ public function testEmptyAllowlistRejectsEveryWebhook(): void $webhook->checkHandlerRequest(); } + // --- manual entry validation ------------------------------------------ + + /** + * @return array + */ + public function malformedEntryProvider(): array + { + return [ + 'trailing junk' => ['31.186.100.4 9'], + 'prefix out of range' => ['31.186.100.0/33'], + 'not an address' => ['garbage'], + 'octet out of range' => ['999.999.999.999'], + 'empty string' => [''], + ]; + } + + /** + * A typo used to be silent: the entry matched nothing and every webhook was refused + * with a bare "IP address Error". + * + * @dataProvider malformedEntryProvider + */ + public function testSetAllowedIpsRejectsMalformedEntry(string $entry): void + { + $webhook = (new Unitpay('unitpay.ru', self::SECRET))->webhook(); + + $this->expectException(UnitpayValidationException::class); + $this->expectExceptionMessage('Invalid IP allowlist entry'); + $webhook->setAllowedIps([$entry]); + } + + /** + * @dataProvider malformedEntryProvider + */ + public function testAddAllowedIpsRejectsMalformedEntry(string $entry): void + { + $webhook = (new Unitpay('unitpay.ru', self::SECRET))->webhook(); + + $this->expectException(UnitpayValidationException::class); + $webhook->addAllowedIps([$entry]); + } + + /** The whole call is rejected, so the allowlist is never left half-configured. */ + public function testRejectedCallLeavesThePreviousAllowlistIntact(): void + { + $webhook = (new Unitpay('unitpay.ru', self::SECRET))->webhook(); + $webhook->setAllowedIps(['203.0.113.7']); + + try { + $webhook->setAllowedIps(['198.51.100.5', 'garbage']); + $this->fail('expected the malformed entry to be rejected'); + } catch (UnitpayValidationException $e) { + // expected + } + + $this->assertSame(['203.0.113.7'], $webhook->getAllowedIps()); + } + + public function testValidIpv4Ipv6AndCidrEntriesAreAccepted(): void + { + $webhook = (new Unitpay('unitpay.ru', self::SECRET))->webhook(); + + $webhook->setAllowedIps(['203.0.113.7', '203.0.113.0/24', '2001:db8::1', '2001:db8::/32']); + + $this->assertSame( + ['203.0.113.7', '203.0.113.0/24', '2001:db8::1', '2001:db8::/32'], + $webhook->getAllowedIps() + ); + } + + /** The feed path stays fail-safe: it drops bad entries instead of throwing. */ + public function testFeedRefreshStillDoesNotThrowOnMalformedEntries(): void + { + $webhook = $this->handler($this->feed(['garbage', '31.186.100.0/33']), self::DEFAULT_IP); + + $this->assertTrue($webhook->refreshAllowedIps()->checkHandlerRequest()); + } + // --- matcher cache reset --------------------------------------------- public function testAddAllowedIpsInvalidatesTheMatcherCache(): void diff --git a/tests/Webhook/WebhookReplayTest.php b/tests/Webhook/WebhookReplayTest.php new file mode 100644 index 0000000..ad801c1 --- /dev/null +++ b/tests/Webhook/WebhookReplayTest.php @@ -0,0 +1,255 @@ + $overrides + * @return array{method: string, params: array} + */ + private function requestAt(int $timestamp, string $method = 'pay', array $overrides = []): array + { + $params = array_merge([ + 'account' => '42', + 'orderSum' => '100.00', + 'unitpayId' => '999', + 'date' => WebhookVerifierTest::dateAt($timestamp), + ], $overrides); + + $params['signature'] = (new SignatureBuilder())->build($params, self::SECRET, $method); + + return ['method' => $method, 'params' => $params]; + } + + /** + * @param array{method: string, params: array} $request + */ + private function verifier(array $request): FrozenClockWebhookVerifier + { + $verifier = new FrozenClockWebhookVerifier( + self::SECRET, + new SignatureBuilder(), + new FakeTransport(), + 'https://unitpay.ru/ips/ips_webhooks.json', + $request, + self::ALLOWED_IP + ); + + return $verifier->freezeAt(self::NOW); + } + + public function testAFreshWebhookPasses(): void + { + $this->assertTrue($this->verifier($this->requestAt(self::NOW))->checkHandlerRequest()); + } + + public function testAWebhookFromTenMinutesAgoIsRejected(): void + { + $this->expectException(UnitpayReplayException::class); + $this->expectExceptionMessage('outside the 300s tolerance window'); + + $this->verifier($this->requestAt(self::NOW - 600))->checkHandlerRequest(); + } + + /** Clock skew cuts both ways: a timestamp from the future is just as untrustworthy. */ + public function testAWebhookFromTenMinutesInTheFutureIsRejected(): void + { + $this->expectException(UnitpayReplayException::class); + + $this->verifier($this->requestAt(self::NOW + 600))->checkHandlerRequest(); + } + + /** + * @dataProvider boundaryOffsets + */ + public function testTheBoundaryIsInclusive(int $offset): void + { + $this->assertTrue($this->verifier($this->requestAt(self::NOW + $offset))->checkHandlerRequest()); + } + + /** @return array */ + public function boundaryOffsets(): array + { + return [ + 'exactly 300s old' => [-300], + 'exactly 300s ahead' => [300], + ]; + } + + /** + * @dataProvider justOutsideOffsets + */ + public function testOneSecondPastTheBoundaryIsRejected(int $offset): void + { + $this->expectException(UnitpayReplayException::class); + + $this->verifier($this->requestAt(self::NOW + $offset))->checkHandlerRequest(); + } + + /** @return array */ + public function justOutsideOffsets(): array + { + return [ + '301s old' => [-301], + '301s ahead' => [301], + ]; + } + + public function testToleranceZeroDisablesTheCheck(): void + { + $verifier = $this->verifier($this->requestAt(self::NOW - 86400))->setWebhookTolerance(0); + + $this->assertTrue($verifier->checkHandlerRequest()); + } + + public function testToleranceIsConfigurable(): void + { + $verifier = $this->verifier($this->requestAt(self::NOW - 600))->setWebhookTolerance(900); + + $this->assertTrue($verifier->checkHandlerRequest()); + } + + public function testNegativeToleranceIsRejected(): void + { + $this->expectException(UnitpayValidationException::class); + $this->expectExceptionMessage('Webhook tolerance must not be negative'); + + $this->verifier($this->requestAt(self::NOW))->setWebhookTolerance(-1); + } + + /** + * A date the SDK cannot parse is a failed freshness check, not something to wave + * through — otherwise garbage in the field would silently disable the window. + * + * @dataProvider malformedDates + */ + public function testAMalformedDateIsRejected(string $date): void + { + $this->expectException(UnitpayReplayException::class); + + $this->verifier($this->requestAt(self::NOW, 'pay', ['date' => $date]))->checkHandlerRequest(); + } + + /** @return array */ + public function malformedDates(): array + { + return [ + 'not a date' => ['not-a-date'], + 'impossible components' => ['2026-13-45 99:99:99'], + 'wrong format' => ['25/07/2026 12:00'], + 'empty' => [''], + ]; + } + + /** + * The regression this whole design exists to prevent. `strtotime()` would resolve the + * same string against the ambient date.timezone, so a webhook accepted on a Moscow + * server would be rejected on a UTC one — three hours is ten times the window. + * + * @dataProvider serverTimezones + */ + public function testTheVerdictDoesNotDependOnTheServerTimezone(string $timezone): void + { + $original = date_default_timezone_get(); + date_default_timezone_set($timezone); + + try { + $this->assertTrue($this->verifier($this->requestAt(self::NOW))->checkHandlerRequest()); + } finally { + date_default_timezone_set($original); + } + } + + /** @return array */ + public function serverTimezones(): array + { + return [ + 'UTC' => ['UTC'], + 'Europe/Moscow' => ['Europe/Moscow'], + 'America/New_York' => ['America/New_York'], + 'Asia/Tokyo' => ['Asia/Tokyo'], + ]; + } + + /** + * Ordering matters: the signature gate stays first, so an attacker cannot use an + * unsigned payload to probe how far the server clock is off. + */ + public function testABadSignatureIsReportedBeforeAStaleDate(): void + { + $request = $this->requestAt(self::NOW - 86400); + $request['params']['orderSum'] = '0.01'; // invalidates the signature + + $this->expectException(UnitpaySignatureException::class); + $this->expectExceptionMessage('Wrong signature'); + + $this->verifier($request)->checkHandlerRequest(); + } + + /** + * `date` is documented for all four inbound methods, but it is also part of the signed + * payload — an attacker cannot strip it, because removing any param changes the hash + * and the signature check fails first. So if it is ever genuinely absent, that is + * Unitpay's choice, not an attack, and rejecting every such webhook would cost the + * merchant real notifications for no security gain. + */ + public function testAWebhookWithoutADateIsAcceptedRatherThanBlocked(): void + { + $params = ['account' => '42', 'orderSum' => '100.00', 'unitpayId' => '999']; + $params['signature'] = (new SignatureBuilder())->build($params, self::SECRET, 'pay'); + + $this->assertTrue($this->verifier(['method' => 'pay', 'params' => $params])->checkHandlerRequest()); + } + + /** The window applies to every inbound method, not only to `pay`. */ + public function testTheWindowAppliesToEveryInboundMethod(): void + { + foreach (['check', 'pay', 'preauth', 'error'] as $method) { + $fresh = $this->verifier($this->requestAt(self::NOW, $method)); + $this->assertTrue($fresh->checkHandlerRequest(), $method . ' should pass when fresh'); + + try { + $this->verifier($this->requestAt(self::NOW - 3600, $method))->checkHandlerRequest(); + $this->fail($method . ' should be rejected when stale'); + } catch (UnitpayReplayException $e) { + $this->addToAssertionCount(1); + } + } + } + + /** Still catchable by handlers written against the pre-4.0 exception surface. */ + public function testReplayExceptionIsCaughtBySignatureExceptionHandlers(): void + { + try { + $this->verifier($this->requestAt(self::NOW - 600))->checkHandlerRequest(); + $this->fail('expected a replay exception'); + } catch (UnitpaySignatureException $e) { + $this->assertInstanceOf(UnitpayReplayException::class, $e); + $this->assertInstanceOf(WebhookVerifier::class, $this->verifier($this->requestAt(self::NOW))); + } + } +} diff --git a/tests/Webhook/WebhookVerifierTest.php b/tests/Webhook/WebhookVerifierTest.php index ba952fe..6f9e483 100644 --- a/tests/Webhook/WebhookVerifierTest.php +++ b/tests/Webhook/WebhookVerifierTest.php @@ -30,7 +30,9 @@ private function validRequest(string $method = 'pay', array $overrides = []): ar 'account' => '42', 'orderSum' => '100.00', 'orderCurrency' => 'RUB', - 'date' => '2026-07-20 12:00:00', + // Fresh by default: the replay window is on out of the box, so a fixed date + // would make every webhook test start failing the moment it went stale. + 'date' => self::dateAt(time()), 'payerSum' => '100.00', 'unitpayId' => '999', ], $overrides); @@ -40,6 +42,16 @@ private function validRequest(string $method = 'pay', array $overrides = []): ar return ['method' => $method, 'params' => $params]; } + /** + * Formats a unix timestamp the way Unitpay sends it: `Y-m-d H:i:s` wall clock in + * UTC+3. gmdate() rather than date() so the fixture does not shift with the ambient + * date.timezone — which is exactly what the timezone regression test below checks. + */ + public static function dateAt(int $timestamp): string + { + return gmdate('Y-m-d H:i:s', $timestamp + 3 * 3600); + } + /** * @param array $params */