-
Notifications
You must be signed in to change notification settings - Fork 293
Fix HTTPS connections with untrusted certs incorrectly shown as unreachable #1189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 4 commits
7fd5e25
618005e
e068ad4
9f767c0
1e54339
20311b6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,10 +26,12 @@ import { | |
| Bluetooth, | ||
| Cable, | ||
| CheckCircle2, | ||
| ExternalLink, | ||
| Globe, | ||
| Loader2, | ||
| type LucideIcon, | ||
| MousePointerClick, | ||
| ShieldAlert, | ||
| XCircle, | ||
| } from "lucide-react"; | ||
| import { useCallback, useEffect, useMemo, useReducer } from "react"; | ||
|
|
@@ -38,7 +40,7 @@ import { DialogWrapper } from "../DialogWrapper.tsx"; | |
| import { urlOrIpv4Schema } from "./validation.ts"; | ||
|
|
||
| type TabKey = ConnectionType; | ||
| type TestingStatus = "idle" | "testing" | "success" | "failure"; | ||
| type TestingStatus = "idle" | "testing" | "success" | "failure" | "cert-error"; | ||
| type DialogState = { | ||
| tab: TabKey; | ||
| name: string; | ||
|
|
@@ -390,9 +392,11 @@ export default function AddConnectionDialog({ | |
| return; | ||
| } | ||
| dispatch({ type: "SET_TEST_STATUS", payload: "testing" }); | ||
| const reachable = await testHttpReachable(validatedURL.data); | ||
| const { reachable, certError } = await testHttpReachable(validatedURL.data); | ||
| if (reachable) { | ||
| dispatch({ type: "SET_TEST_STATUS", payload: "success" }); | ||
| } else if (certError) { | ||
| dispatch({ type: "SET_TEST_STATUS", payload: "cert-error" }); | ||
| } else { | ||
| dispatch({ type: "SET_TEST_STATUS", payload: "failure" }); | ||
| toast({ | ||
|
|
@@ -482,6 +486,24 @@ export default function AddConnectionDialog({ | |
| )} | ||
| </div> | ||
| )} | ||
| {state.testStatus === "cert-error" && ( | ||
| <div className="flex items-center gap-1 text-sm text-amber-600 dark:text-amber-400"> | ||
| <ShieldAlert className="h-4 w-4 shrink-0" /> | ||
| <span> | ||
| {"Certificate not trusted. "} | ||
| <a | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we have a component which includes target and rel |
||
| href={`${state.protocol}://${state.url}`} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| className="inline-flex items-center gap-0.5 underline hover:no-underline" | ||
| > | ||
| Open in new tab | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same thing here. |
||
| <ExternalLink className="h-3 w-3" /> | ||
| </a> | ||
| {" and accept the warning, then test again."} | ||
| </span> | ||
| </div> | ||
| )} | ||
| </div> | ||
| <p className="text-xs text-slate-500 dark:text-slate-400"> | ||
| {t("addConnection.httpConnection.connectionTest.description")} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import type { Connection } from "@core/stores/deviceStore/types"; | ||
| import { testHttpReachable } from "@pages/Connections/utils"; | ||
| import { httpErrorMessage, testHttpReachable } from "@pages/Connections/utils"; | ||
| import { createLogger } from "@meshtastic/sdk"; | ||
| import { TransportHTTP } from "@meshtastic/transport-http"; | ||
| import { | ||
|
|
@@ -62,15 +62,9 @@ export async function openTransport( | |
| async function openHttp( | ||
| conn: Connection & { type: "http"; url: string }, | ||
| ): Promise<OpenTransportResult> { | ||
| const ok = await testHttpReachable(conn.url); | ||
| if (!ok) { | ||
| const url = new URL(conn.url); | ||
| const isHTTPS = url.protocol === "https:"; | ||
| throw new Error( | ||
| isHTTPS | ||
| ? `Cannot reach HTTPS endpoint. If using a self-signed certificate, open ${conn.url} in a new tab, accept the certificate warning, then try connecting again.` | ||
| : "HTTP endpoint not reachable (may be blocked by CORS)", | ||
| ); | ||
| const { reachable, certError } = await testHttpReachable(conn.url); | ||
| if (!reachable) { | ||
| throw new Error(httpErrorMessage(conn.url, certError)); | ||
|
Comment on lines
+65
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Preserve
Proposed fix const { reachable, certError } = await testHttpReachable(conn.url);
if (!reachable) {
- throw new Error(httpErrorMessage(conn.url, certError));
+ const error = new Error(httpErrorMessage(conn.url, certError)) as Error & {
+ errorKind?: Connection["errorKind"];
+ };
+ error.errorKind = certError ? "cert" : "network";
+ throw error;
}Also thread it through the const message = err instanceof Error ? err.message : String(err);
-updateStatus(id, "error", message);
+const errorKind = (err as { errorKind?: Connection["errorKind"] }).errorKind;
+updateStatus(id, "error", message, errorKind);🤖 Prompt for AI Agents |
||
| } | ||
| const url = new URL(conn.url); | ||
| const isTLS = url.protocol === "https:"; | ||
|
|
@@ -203,20 +197,29 @@ async function openSerial( | |
| return { transport: result.value, serialPort: port }; | ||
| } | ||
|
|
||
| export type ProbeResult = { | ||
| status: "online" | "configured" | "disconnected" | "error"; | ||
| error?: string; | ||
| errorKind?: Connection["errorKind"]; | ||
| }; | ||
|
|
||
| /** | ||
| * Probes a saved connection for reachability/permission without opening it. | ||
| * Used by refreshStatuses to update the saved-connection status badges. | ||
| */ | ||
| export async function probeConnection( | ||
| conn: Connection, | ||
| ): Promise<"online" | "configured" | "disconnected" | "error"> { | ||
| export async function probeConnection(conn: Connection): Promise<ProbeResult> { | ||
| switch (conn.type) { | ||
| case "http": { | ||
| const ok = await testHttpReachable(conn.url); | ||
| return ok ? "online" : "error"; | ||
| const { reachable, certError } = await testHttpReachable(conn.url); | ||
| if (reachable) return { status: "online" }; | ||
| return { | ||
| status: "error", | ||
| error: httpErrorMessage(conn.url, certError), | ||
| errorKind: certError ? "cert" : "network", | ||
| }; | ||
| } | ||
| case "bluetooth": { | ||
| if (!("bluetooth" in navigator)) return "disconnected"; | ||
| if (!("bluetooth" in navigator)) return { status: "disconnected" }; | ||
| try { | ||
| const known = await ( | ||
| navigator.bluetooth as Navigator["bluetooth"] & { | ||
|
|
@@ -226,17 +229,13 @@ export async function probeConnection( | |
| const hasPermission = known?.some( | ||
| (d: BluetoothDevice) => d.id === conn.deviceId, | ||
| ); | ||
| // Permission granted ≠ device configured. The card surfaces "online" | ||
| // (i.e. "available, click to connect") so the user explicitly opts | ||
| // into the configure handshake. "configured" is reserved for when | ||
| // the firmware has actually replied with config-complete. | ||
| return hasPermission ? "online" : "disconnected"; | ||
| return { status: hasPermission ? "online" : "disconnected" }; | ||
| } catch { | ||
| return "disconnected"; | ||
| return { status: "disconnected" }; | ||
| } | ||
| } | ||
| case "serial": { | ||
| if (!("serial" in navigator)) return "disconnected"; | ||
| if (!("serial" in navigator)) return { status: "disconnected" }; | ||
| try { | ||
| const ports: SerialPort[] = await ( | ||
| navigator as Navigator & { | ||
|
|
@@ -255,9 +254,9 @@ export async function probeConnection( | |
| info.usbProductId === conn.usbProductId | ||
| ); | ||
| }); | ||
| return hasPermission ? "online" : "disconnected"; | ||
| return { status: hasPermission ? "online" : "disconnected" }; | ||
| } catch { | ||
| return "disconnected"; | ||
| return { status: "disconnected" }; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,7 @@ import { useToast } from "@core/hooks/useToast.ts"; | |
| import { useNavigate } from "@tanstack/react-router"; | ||
| import { | ||
| ArrowLeft, | ||
| ExternalLink, | ||
| LinkIcon, | ||
| MoreHorizontal, | ||
| RotateCw, | ||
|
|
@@ -372,9 +373,20 @@ function ConnectionCard({ | |
| </CardHeader> | ||
| <CardContent className="pt-0"> | ||
| {connection.error ? ( | ||
| <p className="text-sm text-red-600 dark:text-red-400"> | ||
| {connection.error} | ||
| </p> | ||
| <div className="space-y-1"> | ||
| <p className="text-sm text-red-600 dark:text-red-400">{connection.error}</p> | ||
| {connection.errorKind === "cert" && connection.type === "http" && ( | ||
| <a | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
component
|
||
| href={connection.url} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| className="inline-flex items-center gap-1 text-sm text-red-600 dark:text-red-400 underline hover:no-underline" | ||
| > | ||
| <ExternalLink className="size-3" /> | ||
| Open in new tab → | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. connection.json |
||
| </a> | ||
| )} | ||
| </div> | ||
| ) : connection.lastConnectedAt ? ( | ||
| <p className="text-sm text-slate-500 dark:text-slate-400"> | ||
| {t("lastConnectedAt", { date: "" })}{" "} | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -40,27 +40,55 @@ export function createConnectionFromInput(input: NewConnection): Connection { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Cert rejection requires a TCP connection + TLS handshake before the browser rejects, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // so it takes at least ~20ms on a LAN. Failures faster than that are port-closed or | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // ICMP-unreachable (no TLS ever attempted). ARP timeouts for phantom IPs on the same | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // subnet take ~1-3s, so failures slower than 2000ms (but before the AbortController | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // fires) are that — not a cert rejection. True timeouts come through as AbortError. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const CERT_MIN_MS = 20; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const CERT_MAX_MS = 2000; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export async function testHttpReachable( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| url: string, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| timeoutMs = 2500, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): Promise<boolean> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| timeoutMs = 10000, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): Promise<{ reachable: boolean; certError: boolean }> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const start = performance.now(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const controller = new AbortController(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const timer = setTimeout(() => controller.abort(), timeoutMs); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Use no-cors to avoid CORS failure; opaque responses resolve but status is 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // no-cors: opaque responses resolve fine; CORS failures and cert rejections both throw TypeError | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await fetch(url, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| method: "GET", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mode: "no-cors", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cache: "no-store", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| signal: controller.signal, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clearTimeout(timer); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return { reachable: true, certError: false }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (err) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const elapsed = performance.now() - start; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const wasAborted = err instanceof DOMException && err.name === "AbortError"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const certError = | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| !wasAborted && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| elapsed >= CERT_MIN_MS && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| elapsed < CERT_MAX_MS && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| url.startsWith("https:"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return { reachable: false, certError }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
58
to
+77
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win Clear the abort timer on every exit path. When Proposed fix export async function testHttpReachable(
url: string,
timeoutMs = 10000,
): Promise<{ reachable: boolean; certError: boolean }> {
const start = performance.now();
+ const controller = new AbortController();
+ const timer = setTimeout(() => controller.abort(), timeoutMs);
try {
- const controller = new AbortController();
- const timer = setTimeout(() => controller.abort(), timeoutMs);
// no-cors: opaque responses resolve fine; CORS failures and cert rejections both throw TypeError
await fetch(url, {
method: "GET",
mode: "no-cors",
cache: "no-store",
signal: controller.signal,
});
- clearTimeout(timer);
return { reachable: true, certError: false };
} catch (err) {
const elapsed = performance.now() - start;
const wasAborted = err instanceof DOMException && err.name === "AbortError";
@@
url.startsWith("https:");
return { reachable: false, certError };
+ } finally {
+ clearTimeout(timer);
}
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function httpErrorMessage(url: string, certError: boolean): string { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (certError) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return `Self-signed certificate not trusted. Open ${url} in a new tab, accept the security warning, then return here and try again.`; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pls add to connections.json |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (new URL(url).protocol === "https:") { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "HTTPS endpoint not reachable. Check that the device is online."; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pls add to connections.json |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "HTTP endpoint not reachable (may be blocked by CORS or a network issue)"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pls add to connections.json |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function connectionTypeIcon(type: ConnectionType): LucideIcon { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (type === "http") { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return Globe; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll have to make sure these strings are added to connections.json and referenced using the i18next
tfunction. You can find examples around the code base.