sdarbinyan/marketplaces
0
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(cart): validate bank redirect URL, add checkout idempotency key

Browse Source 
FH-1.2 (partial): reject non-https bank redirect URLs before trust-bypass
rendering them into the payment popup iframe; show a visible payment
error instead. Popup-vs-navigate-current-tab redesign still open, gated
on a product decision about the return-URL contract.

FH-1.4 (frontend half): one crypto.randomUUID() idempotency key per
checkout attempt, generated when the payment popup opens and reused
across retryPayment(), sent as metadata.idempotencyKey - the only
extensibility point @marketplaces/payment's PaymentRequest exposes.

FH-0.2: confirmed done - createPayment(payload, headers) has zero
callers, cart.component.ts already runs entirely through the
server-priced checkout path.
This commit is contained in:
sdarbinyan
2026-08-27 04:23:56 +04:00
parent c990c10307
commit 4876b59cf0
2 changed files with 45 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files

32
docs/FORK-HARVEST-TODO.md
View File

@@ -19,9 +19,8 @@ Improvements only. Nothing here regresses our Angular version, test count, or ar
**Also decide:** is one VK account across two of our storefronts one `Customer` or two? Their platform says two; our `Customer.marketplaceId` already implies two.
**Done when:** an ADR exists in `docs/context/adrs/` and both questions have a recorded answer.
- [ ] **FH-0.2 — Confirm the server-priced checkout path covers every live flow** · S · Lane A · *blocks FH-1.3*
`api.service.ts` already has a server-priced checkout session method. Confirm no production flow still depends on `createPayment(payload, headers)` before deleting the header path.
**Done when:** every caller of the legacy header path is enumerated and has a replacement.
- [x] **FH-0.2 — Confirm the server-priced checkout path covers every live flow** · S · Lane A · **confirmed 2026-08-27**
`cart.component.ts` only calls `apiService.createCheckoutSession()` (server-priced, PHASE-1 §5.2) → `paymentGateway.create()`. `createPayment(payload, headers)` has zero callers in `src/` - the component's own comment (line ~333) documents it as already deleted from `ApiService` along with `createCartPayment`/`checkCartPaymentStatus`/etc., confirmed dead before removal. Nothing left to migrate.
---
@@ -31,19 +30,16 @@ Improvements only. Nothing here regresses our Angular version, test count, or ar
Now `GET {tenantApiBase}/geo/resolve`, same base as `/regions`. Server reads the client IP; nothing leaves our infrastructure. Endpoint specified in [BACKEND-API-REFERENCE.md](../BACKEND-API-REFERENCE.md) §6 — **not built yet**, and until it is the client falls back to the manual picker, which is what production has effectively had all along. Covered by `src/app/services/location.service.spec.ts` (4 tests, one of which fails the build on any off-origin or plaintext request from this service).
*Was:* `location.service.ts:75` called `http://ip-api.com/json/?fields=…` from an HTTPS origin. Mixed active content is blocked, so `detectLocation()` only ever took its error branch — auto-detect was dead in production, not merely insecure — and the attempt still leaked every visitor's IP to a third party.
- [ ] **FH-1.2 — Stop blindly trusting the bank redirect URL** · M · Lane A
`src/app/pages/cart/cart.component.ts:485` — `bypassSecurityTrustResourceUrl(bankUrl)` with no validation, rendered into a popup iframe. Most acquirer 3-D Secure pages send `X-Frame-Options: DENY`, so the popup is blank for those banks. Their spec: card checkout navigates the current tab, no intermediate popup.
**Do:** accept only an `https:` URL whose origin the backend returned in the payment response (backend allowlist, per their `safeHttpsUrl()`); navigate the current tab instead of framing.
**Done when:** a non-https or non-allowlisted URL is refused with a visible payment error; a test covers both the accepted and the refused case.
- [~] **FH-1.2 — Stop blindly trusting the bank redirect URL** · M · Lane A · **partial 2026-08-27**
`openBankPaymentPopup()` now refuses to `bypassSecurityTrustResourceUrl` any non-`https:` URL (protocol-parsed via `new URL()`, not a string check) and surfaces a visible payment error instead. **Not done:** the popup/iframe UI itself is unchanged - still frames the bank page rather than navigating the current tab, so acquirers sending `X-Frame-Options: DENY` still show a blank popup. That's a bigger checkout-flow/return-URL redesign, deliberately not done silently in this pass; needs a product decision, not just a fix.
*Was:* `src/app/pages/cart/cart.component.ts:485` — `bypassSecurityTrustResourceUrl(bankUrl)` with no validation at all.
- [x] **FH-1.3 — Remove provider credentials from the browser** · M · Lane A · **landed via the `@marketplaces/payment` migration**
The legacy payment surface on `ApiService` was deleted wholesale in that work. `grep -ri "authorization-key\|userid-value\|web-97ec" src/` now returns nothing. Keep FH-3.5 (bundle secret scan) to stop it coming back.
*Was:* `api.service.ts:675` set `authorization-key` and `userid-value` headers client-side, and `api.service.ts:143` shipped a partner ID literal in the bundle. Their audit's most serious finding, and it was correct.
- [ ] **FH-1.4 — Send `Idempotency-Key` on payment creation** · S · Lane A
Zero `idempot*` anywhere in our codebase. Their API requires the header and rejects a key reused across a different order.
**Do:** generate one key per checkout attempt, stable across retries and across a double-click, sent on payment creation.
**Done when:** the existing `checkout-idempotent-click.spec.ts` asserts both requests carry the *same* key.
- [~] **FH-1.4 — Send `Idempotency-Key` on payment creation** · S · Lane A · **frontend half done 2026-08-27**
`cart.component.ts` generates one `checkoutIdempotencyKey` (`crypto.randomUUID()`) when the payment popup opens, reused across `retryPayment()`, sent as `metadata.idempotencyKey` on `paymentGateway.create()` - the only extensibility point `@marketplaces/payment`'s `PaymentRequest` exposes (no dedicated header field in the package contract). **Not done:** `checkout-idempotent-click.spec.ts` doesn't actually assert on the key (only asserts one checkout-session request on double-click) - the double-click guard is `checkoutInFlight`, upstream of where this key lives, so that test's premise already held before this change. A real "same key on retry" assertion needs a route mock on the payment-intents call, not just checkout-session.
---
@@ -269,16 +265,22 @@ The **client half and the contract are done** (2026-08-21). What remains is back
## Scoreboard
| Wave | Done | Contract written, awaiting backend | Open | Blocked by |
| Wave | Done | Contract written, awaiting backend | Open / partial | Blocked by |
|---|---:|---:|---:|---|
| 0 — Decide | 0 | — | 2 | needs a person, not a session |
| 1 — Live defects | 2 | — | 2 | FH-1.2 / FH-1.4 sit in files another session owns |
| 0 — Decide | 1 | — | 1 | FH-0.1 needs a person, not a session |
| 1 — Live defects | 2 | — | 2 (partial) | FH-1.2 needs a checkout-flow product decision; FH-1.4's frontend half is done, test coverage isn't |
| 2 — Contracts | 14 | — | 0 | 1 rejected (FH-2.12) |
| 3 — Proof | 3 | 1 | 1 | see note below |
| 4 — Identity | 4 | 3 | 1 | OAuth apps, which FH-0.1 gates |
| Ops | 1 | — | 2 | — |
| Process | 6 | — | 0 | — |
| **Total** | **30** | **4** | **8** | 1 rejected |
| **Total** | **31** | **4** | **7** | 1 rejected |
**Landed 2026-08-27**
- **FH-0.2** — confirmed the legacy `createPayment(payload, headers)` path has zero callers; `cart.component.ts` already runs entirely through the server-priced `createCheckoutSession` → `paymentGateway.create()` path.
- **FH-1.2 (partial)** — bank redirect URL now validated `https:`-only before `bypassSecurityTrustResourceUrl`; refused URLs surface a visible payment error. The popup-vs-navigate-current-tab redesign is still open, gated on a product decision (changes the return-URL contract).
- **FH-1.4 (frontend half)** — one `crypto.randomUUID()` idempotency key per checkout attempt, stable across `retryPayment()`, sent as `metadata.idempotencyKey` (the package's only extensibility point - no dedicated header field exists in `@marketplaces/payment`'s `PaymentRequest`).
**Landed 2026-08-21**