fix(monitors): bound certificate probe and gate client on scheme
Addresses review feedback on #3848. Timeout: the probe now dials a user-defined port, so it can reach a service that accepts the connection but never completes the TLS handshake, hanging the request indefinitely. ssl-checker already wires a "timeout" listener that rejects with "Timed Out", but its defaults set no timeout, so it never fired. Passing one activates it. The bundled type declaration is a hand-maintained subset of https.RequestOptions and omitted "timeout"; it is added there so the call typechecks against an option the library already supports. Client gating: "http" is a single monitor type covering both schemes, so an http:// monitor passed the existing type check and requested a certificate anyway. With the server now rejecting those, the details page would surface a 400 on every visit. The scheme is gated client side to match, and the stat box falls back to "N/A" as before. Test comment removed as requested; the timeout is covered by a new assertion in the same file.
This commit is contained in:
1 parent
25a7ac4877
commit
a672f7604d
4 files changed
+31
-8
No files matched your search
@@ -34,6 +34,17 @@ import { Typography } from "@mui/material";
|
||||
|
||||
const certificateDateFormat = "MMM D, YYYY h A";
|
||||
|
||||
const isHttpsUrl = (url?: string): boolean => {
|
||||
if (!url) {
|
||||
return false;
|
||||
}
|
||||
try {
|
||||
return new URL(url).protocol === "https:";
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
};
|
||||
|
||||
interface CertificateResponse {
|
||||
certificateDate: string;
|
||||
}
|
||||
@@ -80,13 +91,15 @@ const UptimeDetailsPage = () => {
|
||||
const monitor = monitorData?.monitor;
|
||||
const monitorStats = monitorDetailsData?.monitorStats ?? null;
|
||||
|
||||
// Certificate fetch - only for HTTP monitors
|
||||
// Certificate fetch - only for monitors served over HTTPS. A "http" type
|
||||
// monitor may still have an http:// URL, which serves no certificate, so the
|
||||
// scheme is gated here to match the server rather than requesting a 400.
|
||||
const certificateUrl = useMemo(() => {
|
||||
if (!monitorId || monitor?.type !== "http") {
|
||||
if (!monitorId || monitor?.type !== "http" || !isHttpsUrl(monitor?.url)) {
|
||||
return null;
|
||||
}
|
||||
return `/monitors/certificate/${monitorId}`;
|
||||
}, [monitorId, monitor?.type]);
|
||||
}, [monitorId, monitor?.type, monitor?.url]);
|
||||
|
||||
const { data: certificateData } = useGet<CertificateResponse>(
|
||||
certificateUrl,
|
||||
|
||||
@@ -8,6 +8,9 @@ type SSLCheckerType = typeof sslChecker;
|
||||
type WhoisModule = typeof whoiser;
|
||||
|
||||
const DEFAULT_HTTPS_PORT = 443;
|
||||
// Ports are user-defined, so a probe can dial something that accepts the
|
||||
// connection and never completes the handshake. Bound the wait explicitly.
|
||||
const CERTIFICATE_TIMEOUT_MS = 10_000;
|
||||
|
||||
export const fetchMonitorCertificate = async (checker: SSLCheckerType, monitor: Monitor): Promise<SSLDetails> => {
|
||||
const monitorUrl = new URL(monitor.url);
|
||||
@@ -20,7 +23,7 @@ export const fetchMonitorCertificate = async (checker: SSLCheckerType, monitor:
|
||||
// URL.port is "" for the scheme's default port, so fall back to 443; without
|
||||
// this the certificate of a monitor on a non-standard port was probed on 443.
|
||||
const port = monitorUrl.port === "" ? DEFAULT_HTTPS_PORT : Number(monitorUrl.port);
|
||||
const cert = await checker(hostname, { port });
|
||||
const cert = await checker(hostname, { port, timeout: CERTIFICATE_TIMEOUT_MS });
|
||||
if (cert?.validTo === null || cert?.validTo === undefined) {
|
||||
throw new Error("Certificate not found");
|
||||
}
|
||||
|
||||
Vendored
+3
@@ -12,6 +12,9 @@ declare module "ssl-checker" {
|
||||
export interface SSLOptions {
|
||||
method?: "GET" | "HEAD";
|
||||
port?: number;
|
||||
// Inherited from https.RequestOptions; ssl-checker rejects with "Timed Out"
|
||||
// when the socket exceeds it.
|
||||
timeout?: number;
|
||||
agent?: Agent;
|
||||
rejectUnauthorized?: boolean;
|
||||
validateSubjectAltName?: boolean;
|
||||
|
||||
@@ -3,10 +3,6 @@ import { fetchMonitorCertificate } from "../../../src/api/controllers/controller
|
||||
import type { Monitor } from "../../../src/domain/monitors/monitor.type.ts";
|
||||
import type { SSLDetails, SSLOptions } from "ssl-checker";
|
||||
|
||||
// Regression cover for #3646: the certificate probe parsed the monitor URL but
|
||||
// dropped its port, so a monitor on a non-standard port had its certificate
|
||||
// checked on 443 and surfaced as "N/A" in the UI.
|
||||
|
||||
const VALID_CERT: SSLDetails = {
|
||||
daysRemaining: 30,
|
||||
valid: true,
|
||||
@@ -54,6 +50,14 @@ describe("fetchMonitorCertificate", () => {
|
||||
expect(calls[0].options?.port).toBe(443);
|
||||
});
|
||||
|
||||
it("bounds the probe with a timeout so a user-defined port cannot hang it", async () => {
|
||||
const { checker, calls } = spyChecker();
|
||||
|
||||
await fetchMonitorCertificate(checker, monitorWithUrl("https://example.com:54321"));
|
||||
|
||||
expect(calls[0].options?.timeout).toBe(10_000);
|
||||
});
|
||||
|
||||
it("never passes NaN as the port", async () => {
|
||||
const { checker, calls } = spyChecker();
|
||||
|
||||
|
||||
Reference in new issue
Block a user