fix(leads): authenticate desktop Turnstile bypass#3491
Conversation
|
@lspassos1 is attempting to deploy a commit to the World Monitor Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR closes the unauthenticated desktop Turnstile bypass by adding HMAC-SHA256 signed request authentication: the sidecar signs outgoing
Confidence Score: 3/5Merge-safe after resolving the P1 warning gap; deploying as-is risks operators misreading the security posture during rollout. One P1 security logic issue (legacy bypass silently overrides configured HMAC key with no warning) plus two P2 style findings. The P1 doesn't allow bypassing the rate limit but it does completely nullify the HMAC during the rollout window, which is the primary security goal of this PR. server/worldmonitor/leads/v1/register-interest.ts — specifically the legacy bypass ordering and warning in
|
| Filename | Overview |
|---|---|
| server/worldmonitor/leads/v1/register-interest.ts | Core security change: adds HMAC-SHA256 verification for the desktop Turnstile bypass; contains a P1 issue where the legacy bypass fires before the signing-key check, allowing unsigned requests through even when the key is configured |
| src-tauri/sidecar/local-api-server.mjs | Adds proxyRegisterInterestToCloud to sign outgoing requests with HMAC and strip stale headers; upgrades fetchWithTimeout HTTPS path to return a real Response; missing explicit User-Agent header per AGENTS.md convention |
| tests/register-interest-desktop-auth.test.mjs | New server-side regression tests covering rejection, stale timestamps, tampering, canonicalization, legacy mode, and valid-signature happy path — good breadth |
| src-tauri/sidecar/local-api-server.test.mjs | Adds sidecar integration test verifying that stale client headers are stripped, source is overridden to desktop-settings, and a fresh valid HMAC signature is produced before forwarding |
| api/_cors.js | Adds the two new desktop auth headers to Access-Control-Allow-Headers in both CORS helper functions |
| server/cors.ts | Mirrors the CORS header update for the TypeScript server-side CORS helper |
| docs/api-platform.mdx | Documents the new desktop signature headers, HMAC input format, and rollout behavior for register-interest |
| docs/usage-rate-limits.mdx | Updates rate-limit table to reflect that the desktop bypass now requires a signed HMAC rather than being an unchecked bypass |
Sequence Diagram
sequenceDiagram
participant DA as Desktop App (Tauri)
participant SC as Local Sidecar
participant CA as Cloud API
DA->>SC: POST /api/register-interest
Note over SC: No CONVEX_URL → proxy to cloud
SC->>SC: Parse body, force source=desktop-settings
SC->>SC: Read shared signing key
alt Key configured
SC->>SC: timestamp = Date.now()
SC->>SC: HMAC-SHA256(key, timestamp + newline + canonicalJSON)
SC->>SC: Set Desktop-Timestamp + Desktop-Signature headers
end
SC->>SC: Strip stale/framing headers
SC->>CA: POST with signed headers + rewritten body
CA->>CA: isDesktopSource = true → verifyDesktopAuth()
alt No headers + ALLOW_LEGACY=true
CA-->>SC: Legacy bypass accepted (rate-limited 2/hr)
else Key missing and no legacy
CA-->>SC: 403 Desktop authentication failed
else Headers present + key configured
CA->>CA: Validate timestamp within 5-min window
CA->>CA: Validate signature format
CA->>CA: Compute expected HMAC, timingSafeEqual()
alt HMAC matches
CA->>CA: Check scoped rate limit (2/hr per IP)
CA-->>SC: 200 registered
else HMAC mismatch or stale
CA-->>SC: 403 Desktop authentication failed
end
end
SC-->>DA: Forward cloud response
Reviews (1): Last reviewed commit: "fix(leads): fail closed for unsigned des..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!timestamp && !signature && process.env[DESKTOP_AUTH_ALLOW_LEGACY_ENV] === 'true') { | ||
| console.warn(`[register-interest] ${DESKTOP_AUTH_ALLOW_LEGACY_ENV}=true; accepting unsigned legacy desktop bypass`); | ||
| return; | ||
| } | ||
|
|
||
| if (!secret) { | ||
| console.warn(`[register-interest] ${DESKTOP_AUTH_SECRET_ENV} not set; rejecting desktop bypass`); | ||
| throw new ApiError(403, 'Desktop authentication failed', ''); |
There was a problem hiding this comment.
The legacy-mode check fires before the HMAC signing key is read. When the ALLOW_LEGACY env var and the shared signing key are both set simultaneously, any unsigned desktop request (omitting both auth headers entirely) is accepted through the legacy path — the HMAC provides zero protection for that traffic class. An attacker who knows the legacy flag is active can always bypass authentication by simply not sending the two desktop auth headers.
The console.warn only says "accepting unsigned legacy desktop bypass" and gives no indication that a configured signing key is being skipped, making this operationally invisible. The warning should surface whether a key is present, so operators can confirm when the rollout safety valve is still open despite a key having been deployed.
| const headers = toHeaders(req.headers, { stripOrigin: true }); | ||
| headers.delete('Authorization'); | ||
| headers.delete('If-None-Match'); | ||
| headers.delete('If-Modified-Since'); | ||
| headers.delete('Transfer-Encoding'); | ||
| headers.delete('Content-Encoding'); | ||
| headers.delete('Connection'); | ||
| headers.delete('Expect'); | ||
| headers.delete(DESKTOP_AUTH_TIMESTAMP_HEADER); | ||
| headers.delete(DESKTOP_AUTH_SIGNATURE_HEADER); | ||
| headers.set('Origin', 'https://worldmonitor.app'); | ||
| headers.set('Content-Type', 'application/json'); | ||
| headers.set('Content-Length', String(Buffer.byteLength(body))); | ||
|
|
||
| const secret = process.env[DESKTOP_AUTH_SECRET_ENV]; | ||
| if (secret) { | ||
| const timestamp = String(Date.now()); | ||
| headers.set(DESKTOP_AUTH_TIMESTAMP_HEADER, timestamp); | ||
| headers.set(DESKTOP_AUTH_SIGNATURE_HEADER, signDesktopAuthPayload(secret, timestamp, normalizedPayload)); | ||
| } |
There was a problem hiding this comment.
User-Agent header in cloud forwarding request
Per the project's AGENTS.md convention, server-side fetch calls must always include a User-Agent header. proxyRegisterInterestToCloud builds its outgoing headers from the desktop client's request via toHeaders, which may or may not carry a User-Agent. If the desktop Tauri shell doesn't populate one, the cloud receives an identityless request.
headers.set('Origin', 'https://worldmonitor.app');
headers.set('Content-Type', 'application/json');
headers.set('Content-Length', String(Buffer.byteLength(body)));
headers.set('User-Agent', 'WorldMonitor-Desktop-Sidecar/1.0'); // add thisContext Used: AGENTS.md (source)
| function timingSafeStringEqual(a: string, b: string): boolean { | ||
| if (a.length !== b.length) return false; | ||
| let diff = 0; | ||
| for (let i = 0; i < a.length; i += 1) { | ||
| diff |= a.charCodeAt(i) ^ b.charCodeAt(i); | ||
| } | ||
| return diff === 0; | ||
| } |
There was a problem hiding this comment.
timingSafeStringEqual leaks length via early return
The guard if (a.length !== b.length) return false short-circuits before the constant-time loop, technically making length a timing oracle. In practice this isn't exploitable here — the regex ^sha256=[a-f0-9]{64}$ on supplied and HMAC's fixed output guarantee both strings are always 71 chars — but the implementation can mislead future callers into assuming it is fully timing-safe.
Since the environment uses crypto.subtle (edge runtime), consider an unconditional length-equalization approach:
function timingSafeStringEqual(a: string, b: string): boolean {
const maxLen = Math.max(a.length, b.length);
let diff = a.length !== b.length ? 1 : 0;
for (let i = 0; i < maxLen; i += 1) {
diff |= (a.charCodeAt(i) || 0) ^ (b.charCodeAt(i) || 0);
}
return diff === 0;
}![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0c37e7932
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const secret = process.env[DESKTOP_AUTH_SECRET_ENV]; | ||
| if (secret) { | ||
| const timestamp = String(Date.now()); | ||
| headers.set(DESKTOP_AUTH_TIMESTAMP_HEADER, timestamp); | ||
| headers.set(DESKTOP_AUTH_SIGNATURE_HEADER, signDesktopAuthPayload(secret, timestamp, normalizedPayload)); |
There was a problem hiding this comment.
proxyRegisterInterestToCloud rewrites every /api/register-interest request to source: "desktop-settings" but only attaches HMAC headers when WM_DESKTOP_SHARED_SECRET is present. In the packaged desktop flow, that secret is not currently part of the sidecar secret plumbing (SUPPORTED_SECRET_KEYS in src-tauri/src/main.rs and the sidecar env allowlists do not include it), so this branch forwards unsigned desktop-bypass requests and they are rejected with 403 once cloud enables WM_DESKTOP_SHARED_SECRET without legacy mode, effectively breaking desktop signup. This path should either guarantee the secret is injected or return a local configuration error instead of forwarding unsigned bypass traffic.
Useful? React with 👍 / 👎.
|
Follow-up pushed in Changes:
Validation:
|
1 participant
Summary
Authenticates the desktop lead-capture Turnstile bypass with an HMAC-SHA256 shared-secret signature. Desktop requests keep their no-Turnstile path, but only when the sidecar signs the canonical payload and the server verifies a fresh timestamped signature.
cc @koala73
Closes #3252
Type of change
Affected areas
/api/*)Root cause
Desktop signup requests used
source: desktop-settingsto bypass Turnstile because the desktop shell has no captcha surface. Without request authentication, a caller could spoof that source and reach the bypass path.Changes
registerInterest.WM_DESKTOP_SHARED_SECRETis missing unlessWM_DESKTOP_AUTH_ALLOW_LEGACY=trueis explicitly set.source: desktop-settings, sign the cloud fallback request, and strip stale/framing headers before forwarding the rewritten JSON body.Responsefrom the sidecar HTTPS fallback path so the outer server can safely readarrayBuffer().Validation
npx tsx --test tests/register-interest-desktop-auth.test.mjsnode --test --test-name-pattern "signs desktop register-interest" src-tauri/sidecar/local-api-server.test.mjsnpm run typecheck:apinpx biome lint server/worldmonitor/leads/v1/register-interest.ts src-tauri/sidecar/local-api-server.mjs tests/register-interest-desktop-auth.test.mjs src-tauri/sidecar/local-api-server.test.mjsgit diff --checkRisk
Moderate. Deployments must set
WM_DESKTOP_SHARED_SECRETbefore unsigned desktop requests are rejected.WM_DESKTOP_AUTH_ALLOW_LEGACY=trueremains available as an explicit temporary rollout fallback for older builds.