From 4876b59cf0766a9556d2c2b8fdbd6613f503d598 Mon Sep 17 00:00:00 2001 From: sdarbinyan Date: Thu, 27 Aug 2026 04:23:56 +0400 Subject: [PATCH] fix(cart): validate bank redirect URL, add checkout idempotency key 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. --- docs/FORK-HARVEST-TODO.md | 32 +++++++++++++++------------- src/app/pages/cart/cart.component.ts | 29 ++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 16 deletions(-) diff --git a/docs/FORK-HARVEST-TODO.md b/docs/FORK-HARVEST-TODO.md index cfd8a89..74c1d05 100644 --- a/docs/FORK-HARVEST-TODO.md +++ b/docs/FORK-HARVEST-TODO.md @@ -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** diff --git a/src/app/pages/cart/cart.component.ts b/src/app/pages/cart/cart.component.ts index 6340b47..e4822c2 100644 --- a/src/app/pages/cart/cart.component.ts +++ b/src/app/pages/cart/cart.component.ts @@ -65,6 +65,11 @@ export class CartComponent implements OnDestroy { selectedPaymentMethod = signal('qr'); paymentId = signal(''); linkCopied = signal(false); + // FH-1.4: one key per checkout attempt, generated when the popup opens and + // reused across retryPayment() - a double-click can't reach here twice + // (checkoutInFlight guards the entry point above), and a retry after a + // failed attempt must not read as a second payment for the same cart. + private checkoutIdempotencyKey = ''; // Email collection after successful payment userEmail = signal(''); @@ -242,6 +247,7 @@ export class CartComponent implements OnDestroy { } openPaymentPopup(paymentMethod: PaymentMethod): void { + this.checkoutIdempotencyKey = crypto.randomUUID(); this.analytics.track('payment_started', { paymentMethod }); this.showPaymentPopup.set(true); this.selectedPaymentMethod.set(paymentMethod); @@ -347,7 +353,10 @@ export class CartComponent implements OnDestroy { ): void { this.paymentGateway.create(paymentMethod as PackagePaymentMethod, { checkoutSessionId: session.checkoutSessionId, - metadata: { merchantReference }, + // PaymentRequest has no dedicated idempotency field (package contract, + // node_modules/@marketplaces/payment) - metadata is the only + // extensibility point, same as merchantReference already uses. + metadata: { merchantReference, idempotencyKey: this.checkoutIdempotencyKey }, }).subscribe({ next: (attempt) => this.handlePaymentAttempt(attempt, paymentMethod), error: (err) => { @@ -482,10 +491,28 @@ export class CartComponent implements OnDestroy { return; } + // FH-1.2: bankUrl comes from the backend's own payment-attempt response + // (attempt.action.url), so this isn't validating an arbitrary client + // input - it's refusing to trust-bypass-render anything the backend + // didn't actually hand back as a real https redirect target. + if (!this.isHttpsUrl(bankUrl)) { + console.error('Refusing to open bank payment popup: non-https redirect URL', bankUrl); + this.setPaymentError(); + return; + } + this.bankPaymentFrameUrl.set(this.sanitizer.bypassSecurityTrustResourceUrl(bankUrl)); this.showBankPaymentPopup.set(true); } + private isHttpsUrl(url: string): boolean { + try { + return new URL(url).protocol === 'https:'; + } catch { + return false; + } + } + closeBankPaymentPopup(): void { this.showBankPaymentPopup.set(false); this.bankPaymentFrameUrl.set(null);