Public: anyone can read this; the crew can change it.
A screenshot never shows a browser error: looks of private apps show a certificate warning #7833
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/7830-a-screenshot-never-shows-a-browser-error → main 5 commits fork 3 threads · 3 unresolved
5 files
changed +209
−9
src/core/looks.ts +11
| @@ -284,6 +284,17 @@ export function deleteLooks( | ||
| 284 | 284 | return gone.map(toLook); |
| 285 | 285 | } |
| 286 | 286 | |
| 287 | /** Delete every thumb of an app that was taken as a stranger; the ones deleted. */ | |
| 288 | export function deleteStrangerThumbs(db: Database, appId: string): Look[] { | |
| 289 | return db | |
| 290 | .query< | |
| 291 | Row, | |
| 292 | [string] | |
| 293 | >(`DELETE FROM look WHERE app_id = ?1 AND kind = 'thumb' AND taken_as = 'nobody' AND attached_by IS NULL RETURNING *`) | |
| 294 | .all(appId) | |
| 295 | .map(toLook); | |
| 296 | } | |
| 297 | ||
| 287 | 298 | /** |
| 288 | 299 | * Which taking of a picture this is: a picture retaken at the same commit |
| 289 | 300 | * (a thumb from before thumbs were the page, or one that failed) is a new |
src/looks/capture.ts +41 −1
| @@ -181,6 +181,32 @@ export function parseRecorded(raw: unknown): Recorded { | ||
| 181 | 181 | }; |
| 182 | 182 | } |
| 183 | 183 | |
| 184 | /** | |
| 185 | * Whether the page the browser ended on is the one asked for: why it is | |
| 186 | * not, or undefined when it is. A browser's own error page (a certificate | |
| 187 | * warning, a refused connection) or a page on another host is never a | |
| 188 | * picture of the app, so the look fails and the last good one stays. | |
| 189 | */ | |
| 190 | export function refusal( | |
| 191 | host: string, | |
| 192 | navigationError: string | undefined, | |
| 193 | href: string, | |
| 194 | ): string | undefined { | |
| 195 | if (navigationError !== undefined && navigationError !== "") | |
| 196 | return `the browser showed an error page instead of the app (${navigationError.slice(0, 100)})`; | |
| 197 | let url: URL; | |
| 198 | try { | |
| 199 | url = new URL(href); | |
| 200 | } catch { | |
| 201 | return "the browser ended on a page with no address"; | |
| 202 | } | |
| 203 | if (url.protocol === "chrome-error:") | |
| 204 | return "the browser showed an error page instead of the app"; | |
| 205 | if (url.hostname !== host) | |
| 206 | return `the page left the app's host for ${url.hostname === "" ? url.protocol : url.hostname}`; | |
| 207 | return undefined; | |
| 208 | } | |
| 209 | ||
| 184 | 210 | /** The DevTools endpoint Chromium writes into its profile once it listens. */ |
| 185 | 211 | async function endpoint( |
| 186 | 212 | profile: string, |
| @@ -421,10 +447,24 @@ async function shoot( | ||
| 421 | 447 | sessionId, |
| 422 | 448 | ); |
| 423 | 449 | const loaded = dt.heard("Page.loadEventFired", sessionId); |
| 424 | await dt.send("Page.navigate", { url: input.url }, sessionId); | |
| 450 | const nav = await dt.send("Page.navigate", { url: input.url }, sessionId); | |
| 425 | 451 | // A page that never finishes loading is pictured as it stands. |
| 426 | 452 | await Promise.race([loaded, Bun.sleep(LOAD_MS)]); |
| 427 | 453 | await Bun.sleep(SETTLE_MS); |
| 454 | const where = await dt.send( | |
| 455 | "Runtime.evaluate", | |
| 456 | { expression: "location.href", returnByValue: true }, | |
| 457 | sessionId, | |
| 458 | ); | |
| 459 | const href = String( | |
| 460 | (where["result"] as { value?: unknown } | undefined)?.value ?? "", | |
| 461 | ); | |
| 462 | const refused = refusal( | |
| 463 | host, | |
| 464 | typeof nav["errorText"] === "string" ? nav["errorText"] : undefined, | |
| 465 | href, | |
| 466 | ); | |
| 467 | if (refused !== undefined) throw new Error(refused); | |
| 428 | 468 | let recorded = parseRecorded(undefined); |
| 429 | 469 | try { |
| 430 | 470 | const read = await dt.send( |
src/looks/service.ts +24 −1
| @@ -42,6 +42,7 @@ import { | ||
| 42 | 42 | import type { ForgeEvent } from "../core/events.ts"; |
| 43 | 43 | import { |
| 44 | 44 | deleteLooks, |
| 45 | deleteStrangerThumbs, | |
| 45 | 46 | DEFAULT_PAGE, |
| 46 | 47 | LEGACY_THUMB_WIDTH, |
| 47 | 48 | DARK_KINDS, |
| @@ -368,8 +369,11 @@ export function startLooks(app: App, options: LooksOptions): LooksHandle { | ||
| 368 | 369 | const sealed = sealedPreview(app.db, repo.id, item.env); |
| 369 | 370 | const everyone = |
| 370 | 371 | !sealed && repo.visibility === "public" && record.access === "public"; |
| 372 | // A private app turns a signed-out visitor away to sign in, on another | |
| 373 | // host: its thumb is taken through the grant door as the owner, like | |
| 374 | // its other pictures, and never leaves the app's host (#7830). | |
| 371 | 375 | const asNobody = (kind: LookKind): boolean => |
| 372 | !sealed && (everyone || kind === "thumb"); | |
| 376 | !sealed && (everyone || (kind === "thumb" && record.access === "public")); | |
| 373 | 377 | const owner = getUserById(app.db, repo.ownerId); |
| 374 | 378 | const asOwner: GrantWho | undefined = |
| 375 | 379 | owner === undefined |
| @@ -592,6 +596,24 @@ export function startLooks(app: App, options: LooksOptions): LooksHandle { | ||
| 592 | 596 | if (look.path !== "") rmSync(join(dataDir, look.path), { force: true }); |
| 593 | 597 | } |
| 594 | 598 | |
| 599 | /** | |
| 600 | * Before #7830 a private app's thumb was taken as a signed-out visitor, | |
| 601 | * who is sent to sign in on another host: its picture is a certificate | |
| 602 | * warning. Those thumbs are deleted, and the sweep takes them again as | |
| 603 | * the owner. Returns how many went. | |
| 604 | */ | |
| 605 | function retakeStrangerThumbs(record: AppRecord): number { | |
| 606 | if (record.access === "public") return 0; | |
| 607 | const bad = deleteStrangerThumbs(app.db, record.id); | |
| 608 | if (bad.length === 0) return 0; | |
| 609 | remove(bad); | |
| 610 | log.info("thumbs of a private app taken as a stranger, to be retaken", { | |
| 611 | app: record.slug, | |
| 612 | count: bad.length, | |
| 613 | }); | |
| 614 | return bad.length; | |
| 615 | } | |
| 616 | ||
| 595 | 617 | /** |
| 596 | 618 | * Pictures on disk for apps that no longer exist, or previews whose pull |
| 597 | 619 | * closed while no forge was listening, are nobody's: the same sweep the |
| @@ -612,6 +634,7 @@ export function startLooks(app: App, options: LooksOptions): LooksHandle { | ||
| 612 | 634 | rmSync(join(root, appId), { recursive: true, force: true }); |
| 613 | 635 | continue; |
| 614 | 636 | } |
| 637 | retakeStrangerThumbs(record); | |
| 615 | 638 | for (const env of readdirSync(join(root, appId))) { |
| 616 | 639 | if (!isEnvName(env) || env === "production") continue; |
| 617 | 640 | const pull = getPull(app.db, record.repoId, Number(env.slice(5))); |
test/look-refusal.test.ts added +28
| @@ -0,0 +1,28 @@ | ||
| 1 | /** A picture of a browser error page, or of a page off the app's host, is never kept (#7830). */ | |
| 2 | import { describe, expect, test } from "bun:test"; | |
| 3 | import { refusal } from "../src/looks/capture.ts"; | |
| 4 | ||
| 5 | const HOST = "hello.lab.test"; | |
| 6 | ||
| 7 | describe("refusal", () => { | |
| 8 | test("the app's own page is pictured", () => { | |
| 9 | expect(refusal(HOST, undefined, `http://${HOST}:80/`)).toBeUndefined(); | |
| 10 | }); | |
| 11 | ||
| 12 | test("a certificate error is refused, with the browser's word", () => { | |
| 13 | const why = refusal(HOST, "net::ERR_CERT_AUTHORITY_INVALID", "chrome-error://chromewebdata/"); | |
| 14 | expect(why).toContain("ERR_CERT_AUTHORITY_INVALID"); | |
| 15 | }); | |
| 16 | ||
| 17 | test("a browser error page is refused even when navigation reported nothing", () => { | |
| 18 | expect(refusal(HOST, undefined, "chrome-error://chromewebdata/")).toContain("error page"); | |
| 19 | }); | |
| 20 | ||
| 21 | test("a redirect off the host is refused and names the host", () => { | |
| 22 | expect(refusal(HOST, undefined, "https://git.open-platform.sh/login")).toContain("git.open-platform.sh"); | |
| 23 | }); | |
| 24 | ||
| 25 | test("a page with no address is refused", () => { | |
| 26 | expect(refusal(HOST, undefined, "")).toBeDefined(); | |
| 27 | }); | |
| 28 | }); |
test/looks.test.ts +105 −7
| @@ -12,7 +12,7 @@ import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | ||
| 12 | 12 | import { existsSync, mkdirSync, readdirSync, readFileSync, writeFileSync } from "node:fs"; |
| 13 | 13 | import { mkdtemp, rm } from "node:fs/promises"; |
| 14 | 14 | import { tmpdir } from "node:os"; |
| 15 | import { join } from "node:path"; | |
| 15 | import { dirname, join } from "node:path"; | |
| 16 | 16 | import { createApp, type App } from "../src/app.ts"; |
| 17 | 17 | import { enableApp, newApp, openEnvUrl } from "../src/actions/apps.ts"; |
| 18 | 18 | import { addToCrew, createAgent } from "../src/actions/crew.ts"; |
| @@ -237,8 +237,8 @@ describe("the look", () => { | ||
| 237 | 237 | expect(pngSize(join(dataDir, thumb.path))).toEqual([640, 400]); |
| 238 | 238 | const desktop = rows.find((l) => l.kind === "desktop")!; |
| 239 | 239 | expect(pngSize(join(dataDir, desktop.path))).toEqual([1280, 800]); |
| 240 | // The thumb's grant names nobody: the tile a stranger is shown is the | |
| 241 | // page as a stranger sees it. | |
| 240 | // This app is private: a stranger is sent to sign in on another host, | |
| 241 | // so its thumb is taken as the owner, through the same grant door (#7830). | |
| 242 | 242 | const thumbCall = calls.find((c) => c.out.endsWith("-thumb.png"))!; |
| 243 | 243 | const thumbGrant = readGrant( |
| 244 | 244 | dataDir, |
| @@ -246,7 +246,8 @@ describe("the look", () => { | ||
| 246 | 246 | { app: record, env: "production" }, |
| 247 | 247 | ); |
| 248 | 248 | expect(thumbGrant).toBeDefined(); |
| 249 | expect(thumbGrant?.who).toBeUndefined(); | |
| 249 | expect(thumbGrant?.who?.name).toBe("alice"); | |
| 250 | expect(thumb.takenAs).toBe("owner"); | |
| 250 | 251 | const url = new URL(phone.url); |
| 251 | 252 | expect(url.host).toBe(`hello.${DOMAIN}:${servers.port}`); |
| 252 | 253 | expect(url.pathname).toBe("/__forge/grant"); |
| @@ -877,7 +878,69 @@ describe("the look", () => { | ||
| 877 | 878 | expect(after).toContain(`${commitOid}/phone-dark.png?v=`); |
| 878 | 879 | }, 120_000); |
| 879 | 880 | |
| 880 | test("the sweep pictures a running production missing one: a thumb from before thumbs were the page is retaken as a stranger; one that failed is not retried each sweep", async () => { | |
| 881 | test("a private app's thumb taken as a stranger by an older forge is deleted on boot, to be retaken", async () => { | |
Also assert that an owner-taken thumb and a public app's nobody thumb survive the boot prune. |
||
| 882 | const old = "c".repeat(40); | |
| 883 | const path = `looks/${record.id}/production/${old}-thumb.png`; | |
| 884 | mkdirSync(join(dataDir, dirname(path)), { recursive: true }); | |
| 885 | writeFileSync(join(dataDir, path), "certificate warning"); | |
| 886 | recordLook(app.db, { | |
| 887 | appId: record.id, | |
| 888 | env: "production", | |
| 889 | commitOid: old, | |
| 890 | kind: "thumb", | |
| 891 | path, | |
| 892 | takenAs: "nobody", | |
| 893 | }); | |
| 894 | const again = startLooks(app, { localUrl: servers.url, capture: fake }); | |
| 895 | await again.stop(); | |
| 896 | expect( | |
| 897 | looksForEnv(app.db, record.id, "production").some((l) => l.commitOid === old), | |
| 898 | ).toBe(false); | |
| 899 | expect(existsSync(join(dataDir, path))).toBe(false); | |
| 900 | }); | |
| 901 | ||
| 902 | test("the boot prune spares an owner-taken thumb, an attached one, and every thumb of a public app", async () => { | |
| 903 | const put = (oid: string, extra: Partial<Parameters<typeof recordLook>[1]>) => { | |
| 904 | const path = `looks/${record.id}/production/${oid}-thumb.png`; | |
| 905 | mkdirSync(join(dataDir, dirname(path)), { recursive: true }); | |
| 906 | writeFileSync(join(dataDir, path), "a picture"); | |
| 907 | recordLook(app.db, { | |
| 908 | appId: record.id, | |
| 909 | env: "production", | |
| 910 | commitOid: oid, | |
| 911 | kind: "thumb", | |
| 912 | path, | |
| 913 | ...extra, | |
| 914 | }); | |
| 915 | return path; | |
| 916 | }; | |
| 917 | const owned = put("d".repeat(40), { takenAs: "owner" }); | |
| 918 | const attached = put("e".repeat(40), { takenAs: "nobody", attachedBy: alice.id }); | |
| 919 | const stranger = put("f".repeat(40), { takenAs: "nobody" }); | |
| 920 | const alive = (path: string): boolean => existsSync(join(dataDir, path)); | |
| 921 | const rows = (): string[] => | |
| 922 | looksForEnv(app.db, record.id, "production").map((l) => l.path); | |
| 923 | ||
| 924 | // A public app's thumbs are taken as a stranger by design: all stay. | |
| 925 | app.db.run("UPDATE app SET access = 'public' WHERE id = ?1", [record.id]); | |
| 926 | await startLooks(app, { localUrl: servers.url, capture: fake }).stop(); | |
| 927 | app.db.run("UPDATE app SET access = 'private' WHERE id = ?1", [record.id]); | |
| 928 | for (const path of [owned, attached, stranger]) { | |
| 929 | expect(rows()).toContain(path); | |
| 930 | expect(alive(path)).toBe(true); | |
| 931 | } | |
| 932 | ||
| 933 | // Private again: only the stranger's goes, row and file. | |
| 934 | await startLooks(app, { localUrl: servers.url, capture: fake }).stop(); | |
| 935 | expect(rows()).toContain(owned); | |
| 936 | expect(rows()).toContain(attached); | |
| 937 | expect(rows()).not.toContain(stranger); | |
| 938 | expect(alive(owned)).toBe(true); | |
| 939 | expect(alive(attached)).toBe(true); | |
| 940 | expect(alive(stranger)).toBe(false); | |
| 941 | }); | |
| 942 | ||
| 943 | test("the sweep pictures a running production missing one: a thumb from before thumbs were the page is retaken (as the owner, the app being private); one that failed is not retried each sweep", async () => { | |
| 881 | 944 | const status = runtime.envStatus(record, "production")!; |
| 882 | 945 | expect(status.state).toBe("running"); |
| 883 | 946 | const head = status.commitOid; |
| @@ -908,8 +971,8 @@ describe("the look", () => { | ||
| 908 | 971 | readGrant(dataDir, new URL(retaken.url).searchParams.get("t") ?? "", { |
| 909 | 972 | app: record, |
| 910 | 973 | env: "production", |
| 911 | })?.who, | |
| 912 | ).toBeUndefined(); | |
| 974 | })?.who?.name, | |
| 975 | ).toBe("alice"); | |
| 913 | 976 | expect(pngSize(join(dataDir, thumbAtHead()!.path))).toEqual([640, 400]); |
| 914 | 977 | |
| 915 | 978 | // A whole set: nothing to take. |
| @@ -1269,6 +1332,41 @@ describe("the look", () => { | ||
| 1269 | 1332 | }, |
| 1270 | 1333 | 120_000, |
| 1271 | 1334 | ); |
| 1335 | test.skipIf(chromium === undefined)( | |
| 1336 | "a capture that ends on the browser's error page fails with its reason and writes no picture", | |
| 1337 | async () => { | |
| 1338 | const page: ReturnType<typeof Bun.serve> = Bun.serve({ | |
| 1339 | port: 0, | |
| 1340 | hostname: "127.0.0.1", | |
| 1341 | // Off the app's host, on a name that resolves to loopback without DNS. | |
| 1342 | fetch: (request): Response => | |
| 1343 | new URL(request.url).pathname === "/" | |
| 1344 | ? new Response(null, { | |
| 1345 | status: 302, | |
| 1346 | headers: { location: `http://localhost:${page.port}/landed` }, | |
| 1347 | }) | |
| 1348 | : new Response("<!doctype html><p>elsewhere</p>", { | |
| 1349 | headers: { "content-type": "text/html" }, | |
| 1350 | }), | |
| 1351 | }); | |
| 1352 | try { | |
| 1353 | const out = join(dataDir, "refused.png"); | |
| 1354 | const taken = await takeLook({ | |
| 1355 | url: `http://127.0.0.1:${page.port}/`, | |
| 1356 | out, | |
| 1357 | width: 390, | |
| 1358 | height: 844, | |
| 1359 | timeoutMs: 30_000, | |
| 1360 | }); | |
| 1361 | expect(JSON.stringify(taken)).toContain("left the app's host for localhost"); | |
| 1362 | // The guard throws before the screenshot: no picture was written. | |
| 1363 | expect(existsSync(out)).toBe(false); | |
| 1364 | } finally { | |
| 1365 | page.stop(true); | |
| 1366 | } | |
| 1367 | }, | |
| 1368 | 120_000, | |
| 1369 | ); | |
| 1272 | 1370 | test.skipIf(chromium === undefined)( |
| 1273 | 1371 | "a dark capture is truly dark: the same page, light and dark, differs in its pixels", |
| 1274 | 1372 | async () => { |
new above
offline
The refusal path in shoot() is untested; only refusal() is. Add a test where the capture ends on an error page and assert the reason is recorded and the old look stays.