Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 32 additions & 4 deletions src/selfhost/health.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,12 +165,13 @@ export function codexAuthReadinessProbe(
*/
export function browserEndpointReadinessProbe(
env: Record<string, string | undefined>,
fetchImpl: (url: string) => Promise<{ ok: boolean }>,
fetchImpl: (url: string, init?: { headers: Record<string, string> }) => Promise<{ ok: boolean }>,
cacheMs = 30_000,
): ReadinessProbe | null {
const endpoint = (env.BROWSER_WS_ENDPOINT ?? "").trim();
if (!endpoint) return null;
const versionUrl = browserVersionUrl(endpoint);
const auth = browserAuthHeaders(endpoint);
// A configured-but-unparseable endpoint is a real misconfiguration, and one the ordinary capture path would
// only surface as a per-shot render failure. Fail readiness closed rather than skipping the probe.
if (versionUrl === null) return { name: "browser_endpoint", check: () => Promise.resolve(false) };
Expand All @@ -183,7 +184,7 @@ export function browserEndpointReadinessProbe(
const now = Date.now();
if (cached !== undefined && now < cachedUntil) return Promise.resolve(cached);
if (inFlight) return inFlight;
inFlight = fetchImpl(versionUrl)
inFlight = fetchImpl(versionUrl, auth)
.then((response) => response.ok)
.catch(() => false)
.then((ok) => {
Expand All @@ -200,8 +201,9 @@ export function browserEndpointReadinessProbe(
}

/** The `http(s)://<host>/json/version` sibling of a `ws(s)://` browserless endpoint, or null when the
* configured value is not a parseable ws/wss URL. Query strings (browserless carries `?token=`) are dropped:
* the version endpoint needs no auth and the token must never end up in a probe URL that could be logged. */
* configured value is not a parseable ws/wss URL. The query string (browserless carries `?token=`) is
* dropped so the token can never end up in a probe URL that might be logged -- it travels as an
* Authorization header instead, see browserAuthHeaders. */
function browserVersionUrl(endpoint: string): string | null {
try {
const url = new URL(endpoint);
Expand All @@ -212,6 +214,32 @@ function browserVersionUrl(endpoint: string): string | null {
}
}

/**
* `Authorization: Bearer <token>` for a browserless endpoint that carries `?token=`, or no headers when it
* does not.
*
* REGRESSION THIS FIXES: the probe originally dropped the query string and sent nothing, on the stated
* assumption that `/json/version` needs no auth. It does. A browserless started with a TOKEN answers 401 to
* an unauthenticated `/json/version` (verified against browserless v2 / Chrome 149 on the live ORB), so
* `browser_endpoint` reported false forever on every deployment that runs it the recommended way -- readiness
* permanently not-ready, container permanently unhealthy, while screenshot capture worked perfectly. A probe
* that cannot pass when the thing it probes is healthy is worse than no probe: it trains operators to ignore
* readiness.
*
* The header, not the query param, keeps the original privacy intent intact: the token still never appears in
* the URL, so it cannot leak through a logged request line, a proxy access log, or an error message that
* echoes the URL.
*/
export function browserAuthHeaders(endpoint: string): { headers: Record<string, string> } | undefined {
try {
const token = new URL(endpoint).searchParams.get("token");
return token ? { headers: { Authorization: `Bearer ${token}` } } : undefined;
} catch {
// An unparseable endpoint already fails the probe closed via browserVersionUrl; nothing to add here.
return undefined;
}
}

/** Boot-time DATA-SAFETY advisory. A single SQLite file with no acknowledged backup is a data-loss SPOF — yet
* `/ready` would still answer 200, so an operator can run with zero durability believing they're healthy. Returns
* the warning to log at boot (or null on Postgres, or once the operator sets `BACKUP_ACKNOWLEDGED=true` after
Expand Down
6 changes: 4 additions & 2 deletions src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -941,8 +941,10 @@ async function main(): Promise<void> {
// #9487/#9464: browserless readiness. A visual-capture outage used to be invisible in /ready and in
// Prometheus while it silently affected gate outcomes; this makes it observable at the same place every
// other optional backend is. No-op unless BROWSER_WS_ENDPOINT is configured.
const browserProbe = browserEndpointReadinessProbe(process.env, async (url) => {
const response = await fetch(url, { signal: AbortSignal.timeout(1500) });
const browserProbe = browserEndpointReadinessProbe(process.env, async (url, init) => {
// `init` carries the Authorization header when BROWSER_WS_ENDPOINT has a `?token=`. Forwarding it is
// what makes the probe work at all against a tokened browserless, which 401s /json/version otherwise.
const response = await fetch(url, { ...init, signal: AbortSignal.timeout(1500) });
return { ok: response.ok };
});
if (browserProbe) {
Expand Down
52 changes: 52 additions & 0 deletions test/unit/selfhost-health.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { describe, expect, it, vi } from "vitest";
import { createD1Adapter, nodeSqliteDriver } from "../../src/selfhost/d1-adapter";
import {
backupAcknowledgedGaugeValue,
browserAuthHeaders,
browserEndpointReadinessProbe,
buildHealthBody,
codexAuthReadinessProbe,
Expand Down Expand Up @@ -543,3 +544,54 @@ describe("browserEndpointReadinessProbe (#9487)", () => {
expect(calls).toBe(2);
});
});

describe("browser_endpoint authenticates (#9487 follow-up)", () => {
const TOKENED = "ws://browserless:3000?token=secret-value";

it("REGRESSION: a tokened browserless answers 401 unauthenticated — the probe must send credentials", async () => {
// The live failure: browserless v2 started with a TOKEN (the documented way) 401s an unauthenticated
// /json/version, so browser_endpoint reported false forever while capture worked fine. Readiness that
// cannot pass on a healthy backend trains operators to ignore readiness.
const seen: Array<{ url: string; headers: Record<string, string> | undefined }> = [];
const probe = browserEndpointReadinessProbe({ BROWSER_WS_ENDPOINT: TOKENED }, (url, init) => {
seen.push({ url, headers: init?.headers });
// Model the real server: 401 unless credentials are presented.
return Promise.resolve({ ok: init?.headers?.Authorization === "Bearer secret-value" });
});
expect(await probe?.check()).toBe(true);
expect(seen[0]?.headers).toEqual({ Authorization: "Bearer secret-value" });
});

it("INVARIANT: the token never appears in the probe URL, only in the header", () => {
// The original code dropped the query string for a good reason -- a token in a URL leaks through access
// logs and error messages that echo it. The fix must keep that property while still authenticating.
const headers = browserAuthHeaders(TOKENED);
expect(headers).toEqual({ headers: { Authorization: "Bearer secret-value" } });
const seen: string[] = [];
browserEndpointReadinessProbe({ BROWSER_WS_ENDPOINT: TOKENED }, (url) => {
seen.push(url);
return Promise.resolve({ ok: true });
})?.check();
expect(seen[0]).toBe("http://browserless:3000/json/version");
expect(seen[0]).not.toContain("secret-value");
expect(seen[0]).not.toContain("token");
});

it("an endpoint with NO token sends no Authorization header", () => {
expect(browserAuthHeaders("ws://browserless:3000")).toBeUndefined();
});

it("an unparseable endpoint yields no headers rather than throwing", () => {
expect(browserAuthHeaders("not a url")).toBeUndefined();
});

it("wss maps to https and still carries the header", () => {
const seen: Array<{ url: string; headers: Record<string, string> | undefined }> = [];
browserEndpointReadinessProbe({ BROWSER_WS_ENDPOINT: "wss://remote.example:443?token=t" }, (url, init) => {
seen.push({ url, headers: init?.headers });
return Promise.resolve({ ok: true });
})?.check();
expect(seen[0]?.url).toBe("https://remote.example/json/version");
expect(seen[0]?.headers).toEqual({ Authorization: "Bearer t" });
});
});