From 04272ae67350a843861cca22934f72616dab8b74 Mon Sep 17 00:00:00 2001 From: sdarbinyan Date: Fri, 21 Aug 2026 10:30:27 +0400 Subject: [PATCH] fix(geo): stop calling ip-api.com from the browser (FH-1.1) detectLocation() fetched http://ip-api.com over plaintext from an HTTPS storefront. Browsers block mixed active content, so the request never completed and auto-detect only ever took its error branch - region detection has been dead in production, not merely insecure. The attempt also handed every visitor's IP to a third party from the page itself. Geo now resolves through the tenant API at {baseUrl}/geo/resolve, the same base /regions already uses. The server reads the client IP; the browser sends nothing and receives no third-party payload. The endpoint is specified in BACKEND-API-REFERENCE.md and is not built yet. Until it ships the client falls back to the manual region picker - identical to the behaviour production already had. Adds location.service.spec.ts: geo goes to the tenant API, no request leaves that origin or uses http://, failure degrades to the manual picker, and detection is not retried once attempted. Co-Authored-By: Claude Opus 5 --- BACKEND-API-REFERENCE.md | 12 +++ docs/FORK-HARVEST-TODO.md | 6 +- src/app/services/location.service.spec.ts | 89 +++++++++++++++++++++++ src/app/services/location.service.ts | 9 ++- 4 files changed, 112 insertions(+), 4 deletions(-) create mode 100644 src/app/services/location.service.spec.ts diff --git a/BACKEND-API-REFERENCE.md b/BACKEND-API-REFERENCE.md index 7bfda30..ac31d13 100644 --- a/BACKEND-API-REFERENCE.md +++ b/BACKEND-API-REFERENCE.md @@ -307,6 +307,18 @@ Base: `ApiConfigService.getBaseUrl()`. Headers on every call (`apiHeadersInterce | `/items/{id}/questiion` | POST | `{ question, sessionID, timestamp }` | `{ message }` — **literal typo `questiion`, preserve it, matches the client** | | `/purchase-email` | POST | `{ email, phone?, telegramUserId, items[] }` | `{ message }` | | `/regions` | GET | — | `Region[]` — client falls back **silently** to 6 hardcoded regions on any error | +| `/geo/resolve` | GET | — | `GeoIpResponse` — **not built yet**, see below | + +**`/geo/resolve` — new, required.** Resolves the *caller's* IP to a coarse location so the storefront can pre-select a region. The server reads the client IP (behind the proxy, so honour `X-Forwarded-For` with `trustProxy`); the browser sends nothing and receives no third-party payload. + +Response is the existing `GeoIpResponse` shape (`src/app/models/location.model.ts`): `{ city, country, countryCode, region?, timezone?, lat?, lon? }`. + +Rules: +- City-level precision only. Do not return coordinates finer than the city centroid, and do not persist the lookup against a customer record — this runs for anonymous visitors. +- Any failure returns a non-2xx. The client already treats every error as "stay on the manual picker", so a degraded geo provider must never block the storefront. +- Rate-limit per IP; it is an unauthenticated endpoint. + +This replaces a direct browser call to `http://ip-api.com`, which leaked every visitor's IP to a third party and — being plaintext on an HTTPS origin — was blocked as mixed content, so region auto-detect never actually worked in production. Until this endpoint ships the client silently falls back to the manual region picker, which is the same behaviour production has had all along. ### 6.1 Products — the tolerance contract diff --git a/docs/FORK-HARVEST-TODO.md b/docs/FORK-HARVEST-TODO.md index 1e28334..7cb2a57 100644 --- a/docs/FORK-HARVEST-TODO.md +++ b/docs/FORK-HARVEST-TODO.md @@ -27,7 +27,8 @@ Improvements only. Nothing here regresses our Angular version, test count, or ar ## Wave 1 — Live defects with a security benefit (Lane A, this sprint) -- [ ] **FH-1.1 — Kill the plaintext third-party geo call** · S · Lane A +- [x] **FH-1.1 — Kill the plaintext third-party geo call** · S · Lane A · **done 2026-08-21** + 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). `src/app/services/location.service.ts:75` calls `http://ip-api.com/json/?fields=…` from an HTTPS origin. Browsers block mixed active content, so `detectLocation()` has been silently taking its error branch in production — region auto-detect is dead, not just insecure. It is also a third-party geo leak on every session. **Do:** remove the direct call. Resolve region server-side (`GET /api/v1/geo/resolve`, backend reads the client IP) or drop auto-detect and keep the manual region picker. **Done when:** zero `http://` literals in `src/`; a unit test asserts `detectLocation()` issues no cross-origin request to a non-allowlisted host. @@ -37,7 +38,8 @@ Improvements only. Nothing here regresses our Angular version, test count, or ar **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.3 — Remove provider credentials from the browser** · M · Lane A · *depends on FH-0.2* +- [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. `src/app/services/api.service.ts:675` sets `authorization-key` and `userid-value` headers client-side. `api.service.ts:143` ships a partner ID literal (`'web-97ec-9c57-4dde-9037-3a68f7f83750'`) in the bundle. Their audit's most serious finding, and it is correct. **Do:** delete both header paths and the literal; the browser gets a checkout URL or a status endpoint, never a credential. **Done when:** `grep -ri "authorization-key\|userid-value" src/` returns nothing; no partner ID literal in `dist/`; a CI check greps the built bundle for both patterns. diff --git a/src/app/services/location.service.spec.ts b/src/app/services/location.service.spec.ts new file mode 100644 index 0000000..1f8e72d --- /dev/null +++ b/src/app/services/location.service.spec.ts @@ -0,0 +1,89 @@ +import { provideHttpClient } from '@angular/common/http'; +import { HttpTestingController, provideHttpClientTesting } from '@angular/common/http/testing'; +import { TestBed } from '@angular/core/testing'; +import { ApiConfigService } from '../core/config/api-config.service'; +import { LocalStorageService } from '../core/storage/local-storage.service'; +import { LocationService } from './location.service'; + +/** + * FH-1.1. detectLocation() used to call http://ip-api.com directly. On an + * HTTPS storefront the browser blocks mixed active content, so the request + * never completed and region auto-detect silently did nothing in production + * - and the attempt still exposed the visitor's IP to a third party. + * + * These tests pin both halves of the fix: geo resolution goes to our own + * tenant API, and nothing in this service reaches a foreign origin. + */ +describe('LocationService', () => { + const baseUrl = 'https://api.gorbushka.market'; + let service: LocationService; + let httpTesting: HttpTestingController; + + beforeEach(() => { + TestBed.configureTestingModule({ + providers: [ + LocationService, + provideHttpClient(), + provideHttpClientTesting(), + { provide: ApiConfigService, useValue: { getBaseUrl: () => baseUrl } }, + { + provide: LocalStorageService, + useValue: { getJSON: () => null, setJSON: () => {}, removeItem: () => {} }, + }, + ], + }); + + service = TestBed.inject(LocationService); + httpTesting = TestBed.inject(HttpTestingController); + // The constructor loads regions; flush it so each test starts clean. + httpTesting.expectOne(`${baseUrl}/regions`).flush([]); + }); + + afterEach(() => httpTesting.verify()); + + it('resolves geo through the tenant API, not a third-party host', () => { + service.detectLocation(); + + const request = httpTesting.expectOne(`${baseUrl}/geo/resolve`); + expect(request.request.method).toBe('GET'); + request.flush({ city: 'Москва', country: 'Россия', countryCode: 'RU' }); + }); + + it('issues no request to a foreign or plaintext origin', () => { + service.detectLocation(); + + for (const request of httpTesting.match(() => true)) { + expect(request.request.url.startsWith(baseUrl)) + .withContext(`unexpected off-origin request: ${request.request.url}`) + .toBe(true); + expect(request.request.url.startsWith('http://')) + .withContext(`plaintext request: ${request.request.url}`) + .toBe(false); + request.flush({}); + } + }); + + it('degrades to the manual picker when geo resolution fails', () => { + service.detectLocation(); + httpTesting + .expectOne(`${baseUrl}/geo/resolve`) + .flush(null, { status: 503, statusText: 'Service Unavailable' }); + + expect(service.region()).toBeNull(); + expect(service.detecting()).toBe(false); + expect(service.autoDetected()).toBe(true); + }); + + it('does not re-request geo once detection has been attempted', () => { + service.detectLocation(); + httpTesting.expectOne(`${baseUrl}/geo/resolve`).flush({ + city: 'Ереван', + country: 'Армения', + countryCode: 'AM', + }); + + service.detectLocation(); + + httpTesting.expectNone(`${baseUrl}/geo/resolve`); + }); +}); diff --git a/src/app/services/location.service.ts b/src/app/services/location.service.ts index b272a56..f549b37 100644 --- a/src/app/services/location.service.ts +++ b/src/app/services/location.service.ts @@ -71,8 +71,13 @@ export class LocationService { if (this.detectedSignal()) return; // already tried this.loadingSignal.set(true); - // Using free ip-api.com — no key required, 45 req/min - this.http.get('http://ip-api.com/json/?fields=city,country,countryCode,region,timezone,lat,lon') + // Was a direct plaintext call to ip-api.com. Two problems, one of them + // fatal: browsers block mixed active content, so on an HTTPS storefront + // this request never completed and auto-detect only ever took the error + // branch below. It also handed every visitor's IP to a third party from + // the page itself. The client IP is the server's to read - same tenant + // API base as /regions, same-origin, nothing leaves our infrastructure. + this.http.get(`${this.apiConfig.getBaseUrl()}/geo/resolve`) .subscribe({ next: (geo) => { this.detectedSignal.set(true);