From df4a3618c4c671faacb659f5e666b7b7293ae4e7 Mon Sep 17 00:00:00 2001 From: Den <2119348+dzianisv@users.noreply.github.com> Date: Sat, 18 Jul 2026 16:04:33 -0700 Subject: [PATCH] fix(connection): fix 6 correctness bugs in auth/connect/diagnostics flow (#131) 1. buildRequestHeaders: UTF-8-encode Basic-auth credentials before btoa() so non-ASCII usernames/passwords don't throw (Hermes' btoa is Latin1-only and the throw was an unhandled rejection that hung the connect spinner). 2. diagnostics classify(): check root.ok (server reachable) before !internet.ok, so a reachable-but-failing server (e.g. wrong auth) is no longer misdiagnosed as "no internet" just because the public-internet probe also failed (captive portal, Tailscale-only network, etc). 3. sdk.ts createClient: strip trailing slashes from baseUrl once, so a trailing-slash URL from Advanced mode / Edit screen doesn't produce a double slash on every request path. 4. add.tsx / [id].tsx: wrap addConnection/updateConnection in try/catch so a SecureStore failure after a successful test resets the spinner and shows an alert instead of hanging forever. Adds connection.shared.alerts.saveFailedTitle/saveFailedMessage (en + zh-Hans). 5. add.tsx / [id].tsx: build the diagnostics probe's auth with buildAuth() instead of a hand-rolled expression, so the probe reproduces the real request's credentials (previously Quick Connect's password-only case sent no auth to the probe at all). 6. add.tsx handleQuickConnect: stop sending the shared `username` state, which could carry a stray value typed earlier in Advanced mode and silently override the "opencode" default after "Back to Quick". Claude-Session: https://claude.ai/code/session_01T12AhSnQVrSxNnvwfCx2z6 Co-authored-by: engineer Co-authored-by: Claude Opus 4.8 --- app/connection/[id].tsx | 56 +++++++++++--------- app/connection/add.tsx | 79 ++++++++++++++++------------ src/lib/diagnostics-classify.test.ts | 13 +++++ src/lib/diagnostics-classify.ts | 10 ++-- src/lib/headers.test.ts | 28 ++++++++++ src/lib/headers.ts | 13 ++++- src/lib/i18n/en.json | 4 +- src/lib/i18n/zh-Hans.json | 4 +- src/lib/sdk.ts | 7 +++ 9 files changed, 150 insertions(+), 64 deletions(-) diff --git a/app/connection/[id].tsx b/app/connection/[id].tsx index 92f9268..49701d2 100644 --- a/app/connection/[id].tsx +++ b/app/connection/[id].tsx @@ -19,6 +19,7 @@ import type { ConnectionType } from "../../src/lib/types" import { probeConnection, shareReport } from "../../src/lib/diagnostics" import { captureDiagnostic } from "../../src/lib/sentry" import { parseUrl } from "../../src/lib/diagnostics-classify" +import { buildAuth } from "../../src/lib/auth" // labelKey (not literal text): this is a module-level constant evaluated // before i18next is guaranteed ready, so the label is resolved with t() at @@ -101,10 +102,7 @@ export default function EditConnectionScreen() { } // Failed: run active diagnostics, capture to Sentry, offer a shareable report. - const report = await probeConnection( - url.trim(), - username.trim() && password ? { username: username.trim(), password } : undefined, - ) + const report = await probeConnection(url.trim(), buildAuth(username, password)) captureDiagnostic(report) setIsTesting(false) @@ -136,28 +134,36 @@ export default function EditConnectionScreen() { } setIsSaving(true) - await updateConnection( - connection.id, - { - name: name.trim(), - type, - url: url.trim(), - directory: directory.trim() || undefined, - username: username.trim() || undefined, - }, - // Empty = keep existing password (the field loads blank); a typed value - // rotates it in SecureStore. - password || undefined, - ) - // If this was the active connection, the SSE loop may have stopped - // retrying after a prior 401 (see events.ts) — reconnect now with the - // freshly saved credentials instead of leaving the user stuck until - // they relaunch the app. - if (useConnections.getState().activeConnection?.id === connection.id) { - useEvents.getState().connect() + try { + await updateConnection( + connection.id, + { + name: name.trim(), + type, + url: url.trim(), + directory: directory.trim() || undefined, + username: username.trim() || undefined, + }, + // Empty = keep existing password (the field loads blank); a typed value + // rotates it in SecureStore. + password || undefined, + ) + // If this was the active connection, the SSE loop may have stopped + // retrying after a prior 401 (see events.ts) — reconnect now with the + // freshly saved credentials instead of leaving the user stuck until + // they relaunch the app. + if (useConnections.getState().activeConnection?.id === connection.id) { + useEvents.getState().connect() + } + setIsSaving(false) + router.back() + } catch { + setIsSaving(false) + Alert.alert( + t("connection.shared.alerts.saveFailedTitle"), + t("connection.shared.alerts.saveFailedMessage"), + ) } - setIsSaving(false) - router.back() } const handleDelete = () => { diff --git a/app/connection/add.tsx b/app/connection/add.tsx index 30c0036..f35bbd9 100644 --- a/app/connection/add.tsx +++ b/app/connection/add.tsx @@ -19,6 +19,7 @@ import type { ConnectionType } from "../../src/lib/types" import { probeConnection, shareReport } from "../../src/lib/diagnostics" import { captureDiagnostic } from "../../src/lib/sentry" import { parseUrl } from "../../src/lib/diagnostics-classify" +import { buildAuth } from "../../src/lib/auth" import { AnalyticsEvent, track } from "../../src/lib/analytics" import { submitWaitlistSignup, buildWaitlistMailtoUrl } from "../../src/lib/waitlist" @@ -75,14 +76,17 @@ export default function AddConnectionScreen() { track(AnalyticsEvent.ConnectionFormSubmitted, { mode: "quick" }) setIsConnecting(true) - // Test connection first + // Test connection first. Quick Connect has no username field, so the + // connection is intentionally saved without one — buildAuth() defaults + // it to "opencode" wherever auth is built. Sending the `username` state + // here would leak a value typed earlier in Advanced mode (issue: Back to + // Quick silently overriding the default). const result = await testConnection( { id: "", name: name || t("connection.shared.namePlaceholder"), type: "local", url: serverUrl, - username: username.trim() || undefined, }, "onboarding", password || undefined, @@ -90,23 +94,27 @@ export default function AddConnectionScreen() { if (result.ok) { // Save and go back - await addConnection( - { - name: name.trim() || t("connection.shared.namePlaceholder"), - type: "local", - url: serverUrl, - username: username.trim() || undefined, - }, - password || undefined, - ) - setIsConnecting(false) - router.back() + try { + await addConnection( + { + name: name.trim() || t("connection.shared.namePlaceholder"), + type: "local", + url: serverUrl, + }, + password || undefined, + ) + setIsConnecting(false) + router.back() + } catch { + setIsConnecting(false) + Alert.alert( + t("connection.shared.alerts.saveFailedTitle"), + t("connection.shared.alerts.saveFailedMessage"), + ) + } } else { // Failed: run active diagnostics, capture to Sentry, offer a shareable report. - const report = await probeConnection( - serverUrl, - username.trim() && password ? { username: username.trim(), password } : undefined, - ) + const report = await probeConnection(serverUrl, buildAuth(undefined, password)) captureDiagnostic(report) setIsConnecting(false) Alert.alert( @@ -160,28 +168,33 @@ export default function AddConnectionScreen() { ) if (result.ok) { - await addConnection( - { - name: name.trim(), - type, - url: url.trim(), - directory: directory.trim() || undefined, - username: username.trim() || undefined, - }, - password || undefined, - ) - setIsConnecting(false) - router.back() + try { + await addConnection( + { + name: name.trim(), + type, + url: url.trim(), + directory: directory.trim() || undefined, + username: username.trim() || undefined, + }, + password || undefined, + ) + setIsConnecting(false) + router.back() + } catch { + setIsConnecting(false) + Alert.alert( + t("connection.shared.alerts.saveFailedTitle"), + t("connection.shared.alerts.saveFailedMessage"), + ) + } return } // Failed: same "Connection Failed" alert as Quick Connect — run active // diagnostics, capture to Sentry, and offer a shareable report instead of // silently persisting an unreachable/unauthorized connection. - const report = await probeConnection( - url.trim(), - username.trim() && password ? { username: username.trim(), password } : undefined, - ) + const report = await probeConnection(url.trim(), buildAuth(username, password)) captureDiagnostic(report) setIsConnecting(false) Alert.alert( diff --git a/src/lib/diagnostics-classify.test.ts b/src/lib/diagnostics-classify.test.ts index b47770a..7f60fc3 100644 --- a/src/lib/diagnostics-classify.test.ts +++ b/src/lib/diagnostics-classify.test.ts @@ -110,6 +110,19 @@ test("internet up, root unreachable, no timeout -> server-unreachable", () => { assert.equal(r.classification, "server-unreachable") }) +test("root reachable but internet probe down still classifies as health-failed, not no-internet", () => { + // Regression: the user's own server responded (proving the path to it + // works — e.g. captive portal or no WAN but Tailscale LAN still up), so + // this must not be misreported as "no internet". + const r = classify( + okUrl, + probe({ ok: false, error: "HTTP 401", status: 401 }), // health + probe({ ok: false }), // internet (down) + probe({ ok: true }), // root (reachable) + ) + assert.equal(r.classification, "health-failed") +}) + test("server-unreachable adds MagicDNS hint only for hostnames, not IPs", () => { const fail = { internet: probe({ ok: true }), root: probe({ ok: false }) } const hostR = classify(parseUrl("http://box.ts.net:8080"), probe({ error: "refused" }), fail.internet, fail.root) diff --git a/src/lib/diagnostics-classify.ts b/src/lib/diagnostics-classify.ts index d0cd1b4..cdd8082 100644 --- a/src/lib/diagnostics-classify.ts +++ b/src/lib/diagnostics-classify.ts @@ -62,13 +62,17 @@ export function classify( if (isTls) { return { classification: "tls-error", summary: "TLS/certificate problem. Try http:// instead of https://, or fix the server certificate." } } + // The user's own server responded to *something* (even a 401/403/404) — + // that proves the path to the server works, so a failed public-internet + // probe (captive portal, no WAN but Tailscale LAN still up, etc.) must not + // override it and misreport a reachable server as "no internet". + if (root.ok) { + return { classification: "health-failed", summary: `Server is reachable but /global/health failed (HTTP ${health.status ?? "error"}). Likely wrong path, auth, or an old server version.` } + } if (!internet.ok) { return { classification: "no-internet", summary: "The device has no working internet/network at all (public check also failed). Check Wi-Fi/data and Tailscale (VPN) status." } } // Internet works, server does not. - if (root.ok) { - return { classification: "health-failed", summary: `Server is reachable but /global/health failed (HTTP ${health.status ?? "error"}). Likely wrong path, auth, or an old server version.` } - } if (isTimeout) { return { classification: "timeout", summary: "Connection to the server timed out (dropped, not refused). Likely a firewall, wrong port, or Tailscale ACL blocking the device." } } diff --git a/src/lib/headers.test.ts b/src/lib/headers.test.ts index ea01f59..b1d9543 100644 --- a/src/lib/headers.test.ts +++ b/src/lib/headers.test.ts @@ -57,3 +57,31 @@ test("empty-string directory is treated as absent (falsy)", () => { const h = buildRequestHeaders({ directory: "" }) assert.equal("x-opencode-directory" in h, false) }) + +// Hermes' `btoa` is Latin1-only and throws a RangeError on non-ASCII input. +// The auth header must be built with a UTF-8-safe base64 helper so a +// non-ASCII username/password never throws (which otherwise surfaces as an +// unhandled rejection and a stuck connect spinner). + +test("ASCII credentials produce the exact same header as plain btoa (no behavior change)", () => { + const h = buildRequestHeaders({ auth: { username: "alice", password: "s3cret" } }) + assert.equal(h["Authorization"], `Basic ${btoa("alice:s3cret")}`) +}) + +test("non-ASCII username does not throw and round-trips back to the credentials", () => { + const auth = { username: "usér", password: "s3cret" } + assert.doesNotThrow(() => buildRequestHeaders({ auth })) + const h = buildRequestHeaders({ auth }) + const b64 = h["Authorization"].replace("Basic ", "") + const decoded = decodeURIComponent(escape(atob(b64))) + assert.equal(decoded, `${auth.username}:${auth.password}`) +}) + +test("non-ASCII password does not throw and round-trips back to the credentials", () => { + const auth = { username: "admin", password: "pässwörd123" } + assert.doesNotThrow(() => buildRequestHeaders({ auth })) + const h = buildRequestHeaders({ auth }) + const b64 = h["Authorization"].replace("Basic ", "") + const decoded = decodeURIComponent(escape(atob(b64))) + assert.equal(decoded, `${auth.username}:${auth.password}`) +}) diff --git a/src/lib/headers.ts b/src/lib/headers.ts index 3ffddb1..f5c4fcd 100644 --- a/src/lib/headers.ts +++ b/src/lib/headers.ts @@ -9,6 +9,17 @@ export interface HeaderConfig { auth?: { username: string; password: string } } +// `btoa` is Latin1-only and throws a range error on any character outside +// the Latin1 byte range (e.g. a non-ASCII username/password). UTF-8-encode +// first so arbitrary Unicode credentials survive - this is the standard +// browser idiom for UTF-8-safe base64, and matches RFC 7617 (Basic auth +// credentials are UTF-8 before being base64-encoded). ASCII input is +// byte-identical to plain `btoa` since encodeURIComponent/unescape +// round-trip it unchanged. +function toBase64Utf8(str: string): string { + return btoa(unescape(encodeURIComponent(str))) +} + export function buildRequestHeaders(config: HeaderConfig): Record { const headers: Record = { "Content-Type": "application/json", @@ -25,7 +36,7 @@ export function buildRequestHeaders(config: HeaderConfig): Record