Skip to content

Commit 931c8d8

Browse files
authored
fix: never render the cloud sign-up email form before region detection settles (Comfy-Org#14945)
## Summary The cloud sign-up email form was live and submittable for up to two seconds before the region check finished, so a user the China rule is meant to block could create the account inside that window. ## Changes - **What**: `userIsInChina` was a `ref(false)` resolved in a mount hook, so `false` meant both "still checking" and "not in China" and the form rendered immediately. Region state is now a three-way `pending | blocked | allowed` behind `useRegionGate()`, with a skeleton holding the space until detection settles. The form cannot render early because it sits behind `v-else`. `isInChina()` now prefers Cloudflare's edge geo-IP over the reachability heuristic. The heuristic was unsound in both directions — a VPN user in China reaches Google and is waved through, a `zh-CN` user anywhere is blocked whenever Google is briefly unreachable — and it pinged google.com and baidu.com with the user's IP from the page whose purpose is minimizing data exposure. It stays as the fallback when the edge cannot answer, so the desktop mirror picker keeps its current tolerance. Every probe leg is now bounded; two previously had no deadline at all and could hang until the browser's own network timeout. Detection always settles, so callers need no timeout of their own. `SignInContent.vue` had the identical construction and now consumes the same composable, so the race is closed on both sign-up surfaces rather than one. - **Breaking**: None. Email sign-up stays blocked in China, Google and GitHub stay allowed, login stays unguarded, and the user-facing copy is byte-identical. ## Review Focus The policy shape is unchanged; only the timing of the gate is. Previously the form was interactive during detection, which under a data-controllership rule is a real hole rather than a cosmetic flash. Two constants carry over from the old code rather than being retuned: `PROBE_TIMEOUT_MS` (2000, previously the Google leg's race) and `CHINA_LATENCY_MS` (150). No new timeout was introduced, and the composable and both views contain no timers — a caller-side deadline would decide "allowed" while a real "blocked" answer was still in flight. `getClientCountry()` reads `loc=` from `/cdn-cgi/trace`. Cloudflare documents that path only as a troubleshooting aid, not a supported API, so the durable form is a first-party endpoint echoing `CF-IPCountry`. Failure returns `undefined` and falls through to the heuristic. A client-side gate is bypassable by design. See the comment below on where enforcement has to live. ## Screenshots Simulated China conditions, same moments on each branch, after choosing **Use email instead**. **Before** — the form is live throughout detection. 300 ms <img src="https://github.com/user-attachments/assets/a829cf19-663f-4d32-9058-2f1d92177df6" width="640"> 1200 ms <img src="https://github.com/user-attachments/assets/cb2de729-3d10-4df2-82d8-c46d00af21d8" width="640"> 2800 ms — the notice finally replaces it <img src="https://github.com/user-attachments/assets/9f87673a-4934-423d-8fe7-91ca73399236" width="640"> **After** — a skeleton holds the space; no email or password field exists in the DOM. 300 ms <img src="https://github.com/user-attachments/assets/b0b6d743-d08d-4a56-91de-c8717e6fb257" width="640"> 1200 ms <img src="https://github.com/user-attachments/assets/ae73b761-f360-4f1f-81a2-e25040b6e5b2" width="640"> 2800 ms — settled to the notice <img src="https://github.com/user-attachments/assets/26ed1bf0-9753-4a50-bc49-a2cd82003e05" width="640"> ## Testing 44 tests across five files, every one mutation-checked. **43 killed, 1 survivor** (recorded below rather than papered over). `isInChina` / `getClientCountry` — 17 cases, 100% branch coverage: | Case | Mutation applied | Result | | --- | --- | --- | | Uppercases the `loc` value | drop `.toUpperCase()` | fails | | `undefined` for non-200 | delete the `response.ok` guard | fails | | `undefined` for a body with no `loc` | default the missing line to a country | fails | | `undefined` for an empty `loc` | `\|\| undefined` → `?? undefined` | fails | | `undefined` for `XX` / `T1` sentinels | return the country unfiltered; trim the sentinel set | 2 fail | | `undefined` for a network error | rethrow instead of catching | fails | | Edge never answers → gives up | bare `fetch`; deadline never rejects; timeout 2000→4000 | fails | | Prefers the edge over the heuristic | run the probe eagerly alongside the edge | fails | | Edge `CN` → blocked / other → allowed | `country === 'CN'` → `!==` | 2 fail | | Falls back to the heuristic on a sentinel | remove the sentinel filter | 2 fail | | Edge down + Google reachable → allowed | invert the google-success return | fails | | Edge down + Baidu fast → blocked | latency compare → `false`; threshold 150→0 | fails | | Neither probe answers → locale decides | inner catch → `true`; force the locale check | fails | | Every probe hangs → still settles | unbound a probe leg | fails | Region gate and both sign-up surfaces — 27 cases: | Case | Mutation applied | Result | | --- | --- | --- | | Starts pending | `ref('pending')` → `ref('allowed')` | fails | | Pending → blocked / → allowed | invert the two status branches | 2 fail | | Rejection fails open to allowed | remove `.catch`; `.catch(() => true)` | fails | | A slow real answer is never pre-empted | add a caller-side 3s deadline | fails | | Form withheld while pending (both surfaces) | pending branch → `v-if="false"` | 2 fail | | Notice replaces form in China (both) | blocked branch → `v-else-if="false"` | 2 fail | | Form renders outside China (both) | invert the blocked branch | 2 fail | | Form released when detection fails (both) | invert the blocked branch | 2 fail | | Never renders the form in China, pending or settled | disable both branches | fails | | Social sign-up unaffected in every state (4 rows) | gate the social buttons on `regionStatus === 'allowed'` | 4 fail | | Login view has no region gate | `CloudSignInForm` → `v-if="false"` | fails | | Query forwarding, free-runs copy, email toggle, webview notice | drop `query: route.query`; alter the count; force the branch | 4 fail | **One survivor, left in place.** `SignInContent > leaves sign-in ungated by region` is a real assertion — deleting `SignInForm` kills it, and it catches a gate that withholds sign-in *while the probe is pending*. What it cannot catch is a gate that revokes sign-in once the probe settles to `blocked`, which is what its name promises. Cause is timing: the assertion resolves on the first tick, before `onMounted`'s gate flips. Its `CloudLoginView` twin is genuinely killed, because that view has no gate wired at all. Also worth recording: renaming the locale key `signupBlocked` breaks no test, because `t`/`st` are mocked to echo the key. The two `signup_blocked` tests still pin real branch routing (killed three ways), but locale-key existence is not covered on this branch. `auth/popup-blocked` is covered, via the locale-derived table in Comfy-Org#14944. The `SignInContent` dialog is covered by unit assertions rather than a screenshot — driving it needs a local ComfyUI backend.
1 parent f51e309 commit 931c8d8

9 files changed

Lines changed: 755 additions & 50 deletions

File tree

Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,169 @@
1+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
2+
3+
import { getClientCountry, isInChina } from './networkUtil'
4+
5+
const traceResponse = (body: string, ok = true) =>
6+
({ ok, text: () => Promise.resolve(body) }) as Response
7+
8+
const TRACE_BODY = (loc: string) =>
9+
`fl=1187f27\nh=cloud.comfy.org\nloc=${loc}\ntls=TLSv1.3\n`
10+
11+
/** Blackholes: never settles and ignores the abort signal. */
12+
const neverSettles = () => new Promise<Response>(() => {})
13+
14+
/** Resolves to the settled value, or to `PENDING` if the promise has not. */
15+
const settlementOf = <T>(promise: Promise<T>) =>
16+
Promise.race([promise, Promise.resolve().then(() => 'PENDING' as const)])
17+
18+
const fetchMock = vi.fn()
19+
20+
beforeEach(() => {
21+
vi.stubGlobal('fetch', fetchMock)
22+
vi.stubGlobal('navigator', { language: 'en-US' })
23+
fetchMock.mockReset()
24+
})
25+
26+
afterEach(() => {
27+
vi.unstubAllGlobals()
28+
vi.useRealTimers()
29+
})
30+
31+
describe('getClientCountry', () => {
32+
it('returns the uppercased loc value from the edge trace', async () => {
33+
fetchMock.mockResolvedValue(traceResponse(TRACE_BODY('cn')))
34+
35+
await expect(getClientCountry()).resolves.toBe('CN')
36+
})
37+
38+
it.for([
39+
[
40+
'a non-200 response',
41+
() => Promise.resolve(traceResponse('loc=CN', false))
42+
],
43+
[
44+
'a body with no loc line',
45+
() => Promise.resolve(traceResponse('h=x\nts=1'))
46+
],
47+
['an empty loc value', () => Promise.resolve(traceResponse('loc=\n'))],
48+
[
49+
'an unknown-country sentinel',
50+
() => Promise.resolve(traceResponse(TRACE_BODY('XX')))
51+
],
52+
['a Tor sentinel', () => Promise.resolve(traceResponse(TRACE_BODY('T1')))],
53+
['a network error', () => Promise.reject(new Error('offline'))]
54+
] as const)('returns undefined for %s', async ([, respond]) => {
55+
fetchMock.mockImplementation(respond)
56+
57+
await expect(getClientCountry()).resolves.toBeUndefined()
58+
})
59+
60+
it('gives up rather than hanging when the edge never answers', async () => {
61+
vi.useFakeTimers()
62+
fetchMock.mockImplementation(neverSettles)
63+
64+
const country = getClientCountry()
65+
expect(await settlementOf(country)).toBe('PENDING')
66+
67+
await vi.advanceTimersByTimeAsync(2000)
68+
69+
expect(await settlementOf(country)).toBeUndefined()
70+
})
71+
72+
it('gives up when the edge sends headers and then stalls the body', async () => {
73+
vi.useFakeTimers()
74+
fetchMock.mockResolvedValue({
75+
ok: true,
76+
text: () => new Promise<string>(() => {})
77+
} as Response)
78+
79+
const country = getClientCountry()
80+
expect(await settlementOf(country)).toBe('PENDING')
81+
82+
await vi.advanceTimersByTimeAsync(2000)
83+
84+
expect(await settlementOf(country)).toBeUndefined()
85+
})
86+
})
87+
88+
describe('isInChina', () => {
89+
it('trusts the edge answer over the reachability heuristic', async () => {
90+
fetchMock.mockResolvedValue(traceResponse(TRACE_BODY('CN')))
91+
92+
await expect(isInChina()).resolves.toBe(true)
93+
expect(fetchMock).toHaveBeenCalledTimes(1)
94+
})
95+
96+
it('reports outside China when the edge names another country', async () => {
97+
fetchMock.mockResolvedValue(traceResponse(TRACE_BODY('IN')))
98+
99+
await expect(isInChina()).resolves.toBe(false)
100+
})
101+
102+
it('reports inside China when only Baidu answers, and fast', async () => {
103+
fetchMock.mockImplementation((url: string) =>
104+
url.includes('cdn-cgi') || url.includes('google')
105+
? Promise.reject(new Error('blocked'))
106+
: Promise.resolve({ ok: true } as Response)
107+
)
108+
109+
await expect(
110+
isInChina(),
111+
'a China-routed client on a non-zh locale is only detectable by Baidu latency'
112+
).resolves.toBe(true)
113+
})
114+
115+
it.for(['XX', 'T1'] as const)(
116+
'falls back to the heuristic when the edge answers %s',
117+
async (sentinel) => {
118+
vi.stubGlobal('navigator', { language: 'zh-CN' })
119+
fetchMock.mockImplementation((url: string) =>
120+
url.includes('cdn-cgi')
121+
? Promise.resolve(traceResponse(TRACE_BODY(sentinel)))
122+
: Promise.reject(new Error('blocked'))
123+
)
124+
125+
await expect(
126+
isInChina(),
127+
'a sentinel names no country, so treating it as "not China" would wave through Tor and unknown-IP clients'
128+
).resolves.toBe(true)
129+
}
130+
)
131+
132+
it('reports outside China when the edge is down but Google answers', async () => {
133+
fetchMock.mockImplementation((url: string) =>
134+
url.includes('cdn-cgi')
135+
? Promise.reject(new Error('edge down'))
136+
: Promise.resolve({ ok: true } as Response)
137+
)
138+
139+
await expect(isInChina()).resolves.toBe(false)
140+
})
141+
142+
it('falls back to the locale when neither probe answers', async () => {
143+
fetchMock.mockRejectedValue(new Error('blocked'))
144+
145+
await expect(isInChina()).resolves.toBe(false)
146+
})
147+
148+
it('falls back to the reachability heuristic when the edge cannot answer', async () => {
149+
vi.stubGlobal('navigator', { language: 'zh-CN' })
150+
fetchMock.mockImplementation((url: string) =>
151+
url.includes('cdn-cgi')
152+
? Promise.reject(new Error('edge down'))
153+
: Promise.reject(new Error('blocked'))
154+
)
155+
156+
await expect(isInChina()).resolves.toBe(true)
157+
})
158+
159+
it('settles even when every probe hangs forever', async () => {
160+
vi.useFakeTimers()
161+
fetchMock.mockImplementation(neverSettles)
162+
163+
const verdict = isInChina()
164+
for (const _ of [0, 1, 2]) await vi.advanceTimersByTimeAsync(2000)
165+
166+
expect(await settlementOf(verdict)).toBe(false)
167+
expect(fetchMock).toHaveBeenCalledTimes(3)
168+
})
169+
})

packages/shared-frontend-utils/src/networkUtil.ts

Lines changed: 94 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -4,59 +4,119 @@ const VALID_STATUS_CODES = [200, 201, 301, 302, 307, 308]
44
export const checkUrlReachable = async (url: string): Promise<boolean> => {
55
try {
66
const response = await axios.head(url)
7-
// Additional check for successful response
87
return VALID_STATUS_CODES.includes(response.status)
98
} catch {
109
return false
1110
}
1211
}
1312

1413
/**
15-
* Checks if the user is likely in mainland China by:
16-
* 1. Checking navigator.language
17-
* 2. Testing connectivity to commonly blocked services
18-
* 3. Testing latency to China-specific domains
14+
* A CDN implementation detail, not a contract we own. The durable form is a
15+
* first-party endpoint echoing `CF-IPCountry`.
1916
*/
20-
export async function isInChina(): Promise<boolean> {
21-
// Quick check based on language/locale
22-
const isChineseLocale = navigator.language.toLowerCase().startsWith('zh-cn')
17+
const CLIENT_COUNTRY_URL = 'https://cloud.comfy.org/cdn-cgi/trace'
18+
19+
/** Bounds every probe leg; two previously had none. */
20+
const PROBE_TIMEOUT_MS = 2000
21+
22+
/** Baidu answering this fast implies a China route. */
23+
const CHINA_LATENCY_MS = 150
24+
25+
/** `XX` is an unknown country and `T1` is Tor: neither names where the client is. */
26+
const UNRESOLVED_COUNTRIES = new Set(['XX', 'T1'])
27+
28+
const parseTraceCountry = (body: string): string | undefined => {
29+
const country =
30+
body
31+
.split('\n')
32+
.find((line) => line.startsWith('loc='))
33+
?.slice('loc='.length)
34+
.trim()
35+
.toUpperCase() || undefined
36+
37+
return country && UNRESOLVED_COUNTRIES.has(country) ? undefined : country
38+
}
39+
40+
/**
41+
* The abort signal alone is not enough: a request the browser never resolves
42+
* nor rejects would hang forever, so the deadline rejects independently.
43+
*
44+
* `read` runs inside the deadline so a response that sends headers and then
45+
* stalls its body is bounded too.
46+
*/
47+
async function fetchWithin<T>(
48+
url: string,
49+
init: RequestInit,
50+
read: (response: Response) => Promise<T>
51+
): Promise<T> {
52+
const controller = new AbortController()
53+
let expire: ReturnType<typeof setTimeout> | undefined
54+
55+
const deadline = new Promise<never>((_, reject) => {
56+
expire = setTimeout(() => {
57+
controller.abort()
58+
reject(new Error(`Timed out after ${PROBE_TIMEOUT_MS}ms: ${url}`))
59+
}, PROBE_TIMEOUT_MS)
60+
})
2361

2462
try {
25-
// Test connectivity to Google - commonly blocked in China
26-
const googleTest = await Promise.race([
27-
fetch('https://www.google.com', {
28-
mode: 'no-cors',
29-
cache: 'no-cache'
30-
}),
31-
new Promise((_, reject) => setTimeout(() => reject(), 2000))
63+
return await Promise.race([
64+
fetch(url, { ...init, signal: controller.signal }).then(read),
65+
deadline
3266
])
67+
} finally {
68+
clearTimeout(expire)
69+
}
70+
}
3371

34-
// If Google is accessible, user is likely not in China
35-
if (googleTest) {
36-
return false
37-
}
72+
/** ISO country from the CDN edge, or `undefined` when it cannot answer. */
73+
export async function getClientCountry(): Promise<string | undefined> {
74+
try {
75+
const body = await fetchWithin(
76+
CLIENT_COUNTRY_URL,
77+
{ cache: 'no-store' },
78+
async (response) => (response.ok ? response.text() : undefined)
79+
)
80+
return body === undefined ? undefined : parseTraceCountry(body)
3881
} catch {
39-
// Google is not accessible - potential indicator of being in China
40-
if (isChineseLocale) {
41-
return true
42-
}
82+
return undefined
83+
}
84+
}
85+
86+
const probe = (url: string) =>
87+
fetchWithin(url, { mode: 'no-cors', cache: 'no-cache' }, async () => {})
88+
89+
/**
90+
* Fallback for when the edge cannot answer. Unsound both ways: a VPN user in
91+
* China reaches Google, and a `zh-CN` user anywhere is blocked whenever Google
92+
* is briefly unreachable.
93+
*/
94+
async function isInChinaByProbe(): Promise<boolean> {
95+
const isChineseLocale = navigator.language.toLowerCase().startsWith('zh-cn')
96+
97+
try {
98+
await probe('https://www.google.com')
99+
return false
100+
} catch {
101+
if (isChineseLocale) return true
43102

44-
// Additional check - test latency to a reliable Chinese domain
45103
try {
46104
const start = performance.now()
47-
await fetch('https://www.baidu.com', {
48-
mode: 'no-cors',
49-
cache: 'no-cache'
50-
})
51-
const latency = performance.now() - start
52-
53-
// If Baidu responds quickly (<150ms), user is likely in China
54-
return latency < 150
105+
await probe('https://www.baidu.com')
106+
return performance.now() - start < CHINA_LATENCY_MS
55107
} catch {
56-
// If both tests fail, default to locale check
57108
return isChineseLocale
58109
}
59110
}
111+
}
112+
113+
/**
114+
* Prefers the edge's geo-IP, falling back to the heuristic. Always settles, so
115+
* callers must not add a timeout that could pre-empt a slow but real answer.
116+
*/
117+
export async function isInChina(): Promise<boolean> {
118+
const country = await getClientCountry()
119+
if (country !== undefined) return country === 'CN'
60120

61-
return false
121+
return isInChinaByProbe()
62122
}

0 commit comments

Comments
 (0)