4268a42 · 29e848a · dd52cf2 · c4b376a · 9b150b4 · f8d173e
@@ -0,0 +1,13 @@
# Sent to sign in by an app, and back again
An app that uses this forge to sign its people in sends a stranger to `/oauth/authorize`. The forge sends them on to `/signin?next=<that request>`; `src/web/pages/asking.ts` reads the `next` the way the consent page does, so only a registered client with a registered return address is ever described.
- **Who is asking:** the sign-in and sign-up pages lead with the return host, the verified part ("**gretchy.store** wants you to sign in"). The name the app registered follows as small secondary text, escaped, never in the lead: dynamic client registration is open, so a name is whatever its registrant typed. The consent page itself still names only the host.
- **Sign up keeps the request:** every link between the two forms and from the header carries the page the person came from (`src/web/return_to.ts`); a new account ends on the authorize request, so the consent is the next page, not the home page.
- **A `next` that is not a registered authorize request** says nothing about any app.
## Terminal
There is no terminal form of this: the redirect is a browser exchange between the app and the forge. The same account reaches the forge's terminal with an SSH key added under Settings → Keys (`ssh <forge>`, the exact command is in `FORGE_APP_SSH`); an account made through an app's sign-up is the same account.
Tests: `test/signin-asking.test.ts` (including a real-browser walk, skipped where no Chromium runs).
@@ -0,0 +1,26 @@
/**
 * Who is asking a person to sign in: when the `next` of the sign-in or
 * sign-up page is an OAuth authorize request, the app that made it.
 * Read the way the consent page reads it, so only a registered client with a
 * registered return address is ever named.
 */
import type { Database } from "bun:sqlite";
import { parseAuthRequest } from "../../core/oauth.ts";
export interface Asking {
  /** The name the app registered. */
  readonly name: string;
  /** Where the person will be sent back to: the app's address. */
  readonly host: string;
}
export function askingOf(db: Database, next: string | undefined): Asking | undefined {
  if (next === undefined) return undefined;
  const url = URL.parse(next, "http://forge.invalid");
  if (url === null || url.pathname !== "/oauth/authorize") return undefined;
  const request = parseAuthRequest(db, (n) => url.searchParams.get(n) ?? "");
  if (!request.ok) return undefined;
  const back = URL.parse(request.value.redirectUri);
  if (back === null) return undefined;
  return { name: request.value.client.name, host: back.host };
}
@@ -65,6 +65,7 @@
import {
import { existsSync } from "node:fs";
import { join } from "node:path";
import { secureFor } from "../../http/cookie_secure.ts";
import { askingOf } from "./asking.ts";
import { SIGNUP_OWN_FORGE, SIGNUP_YOU_GET } from "../../core/contribute.ts";
import { hasAdmin } from "../../spawn/claim.ts";
import { getSignup, type SignupMode } from "../../settings/signup.ts";
@@ -273,6 +274,8 @@
function authForm(
  if (next) query.set("next", next);
  const invite = kind === "signup" ? inviteOf(ctx) : undefined;
  if (invite !== undefined) query.set("invite", invite);
  // The way to the other form keeps where the person was going.
  const asking = askingOf(ctx.app.db, next);
  const action = query.size === 0 ? `/${kind}` : `/${kind}?${query.toString()}`;
  // "Is this the first account?" is a `LIMIT 1` existence check, not a count
  // of the whole user table — and the sign-in screen paints on every session.
@@ -443,6 +446,10 @@
function authForm(
    existsSync(join(ctx.app.config.dataDir, "parent.json")) && unowned;
  return html`<div class="narrow">
    ${pageHead(kind === "signin" ? "Sign in" : getSignup(ctx.app.db) === "request" ? "Ask for an account" : "Create your account")}
    ${when(
      asking,
      (who) => html`<p class="mb" id="asking"><strong>${who.host}</strong> <span class="small muted">(${who.name})</span> wants you to sign in.</p>`,
      
        sentinelcrew
        1d ago
        outdated
      
      HIGH: this shows an attacker-registrable name, bolded, as the actor on the sign-in and sign-up pages. Show the verified host first and the name only as secondary text. It also removes the sign-up copy for this case; check that is wanted.
    
      
        junipercrew
        1d ago
        outdated
      
      Principle 1: one sentence. Drop the second clause on both forms ('Make an account here and you go straight back to it.' / 'You go back to it once you have.'). The page already names the host and the form's button says what happens. On sign-up it also sits directly above signup-you-get, so the page explains itself twice.
    
    )}
      
        junipercrew
        1d ago
        outdated
      
      Principle 1: 'the forge has not checked that' is a note that changes no decision. Show the name as plain muted text or drop it.
    
    ${when(
      kind === "signup",
      () => html`<p class="mb" id="signup-you-get">${SIGNUP_YOU_GET} <a href="/forges/new">${SIGNUP_OWN_FORGE}</a></p>`,
@@ -374,7 +374,7 @@
export async function clickAndFollow(
    steps: readonly string[];
    until: (url: string) => boolean;
    /** Done for another reason the page cannot see (a server got the request). */
    done?: () => boolean;
    done?: (requested: readonly string[]) => boolean;
    timeoutMs?: number;
  },
): Promise<Followed> {
@@ -444,7 +444,7 @@
export async function clickAndFollow(
    });
    const deadline = Date.now() + (input.timeoutMs ?? 15_000);
    const reached = () =>
      requested.some(input.until) || (input.done?.() ?? false);
      requested.some(input.until) || (input.done?.(requested) ?? false);
    let loaded = cdp.once("Page.loadEventFired");
    await cdp.send("Page.navigate", { url: input.url });
    await Promise.race([loaded, Bun.sleep(Math.max(0, deadline - Date.now()))]);
@@ -0,0 +1,131 @@
/**
 * A person sent to sign in by an app (#12174): the sign-in and sign-up pages
 * say who is asking, the way to sign up keeps the authorize request, and a
 * new account finishes on it; the consent leads with the verified return host,
 * never the registered name.
 */
import { afterAll, beforeAll, expect, test } from "bun:test";
import { mkdtemp, rm } from "node:fs/promises";
import { tmpdir } from "node:os";
import { join } from "node:path";
import { createApp, type App } from "../src/app.ts";
import { findChromium } from "../src/looks/capture.ts";
import { chromiumRuns, clickAndFollow } from "./helpers/chrome.ts";
import { getUserByName } from "../src/core/users.ts";
import { serve, type Servers } from "../src/http/server.ts";
const REDIRECT = "https://gretchy.store/auth/callback";
let dataDir: string;
let app: App;
let servers: Servers;
let base: string;
let next: string;
beforeAll(async () => {
  dataDir = await mkdtemp(join(tmpdir(), "forge-signin-asking-"));
  delete process.env.FORGE_EDGE_TOKEN;
  app = createApp({ dataDir, port: 0 });
  servers = await serve(app);
  base = servers.url;
  const res = await fetch(`${base}/oauth/register`, {
    method: "POST",
    headers: { "content-type": "application/json" },
    body: JSON.stringify({ client_name: "Gretchy Store", redirect_uris: [REDIRECT] }),
  });
  const clientId = ((await res.json()) as { client_id: string }).client_id;
  next = `/oauth/authorize?${new URLSearchParams({
    client_id: clientId,
    redirect_uri: REDIRECT,
    response_type: "code",
    code_challenge: "A".repeat(43),
    code_challenge_method: "S256",
    scope: "openid",
    state: "s1",
  })}`;
});
afterAll(async () => {
  servers.stop();
  await rm(dataDir, { recursive: true, force: true });
});
test("authorize sends a stranger to sign in, and both pages name the app and keep the request", async () => {
  const sent = await fetch(`${base}${next}`, { redirect: "manual" });
  expect(sent.status).toBe(303);
  const door = sent.headers.get("location")!;
  expect(door.startsWith("/signin?next=")).toBe(true);
  const signin = await (await fetch(`${base}${door}`)).text();
  expect(signin).toContain("Gretchy Store");
      
        junipercrew
        1d ago
        
      
      Redundant: the assertion on line 54 already contains this string. Remove it, and add an escaping test for a hostile registered name.
    
  expect(signin).toContain("<strong>gretchy.store</strong>");
  expect(signin).toContain("(Gretchy Store)</span> wants you to sign in.");
  expect(signin).not.toContain("<strong>Gretchy Store</strong>");
  const link = `/signup?next=${encodeURIComponent(next)}`;
  expect(signin).toContain(`href="${link.replaceAll("&", "&amp;")}"`);
  const signup = await (await fetch(`${base}${link}`)).text();
  expect(signup).toContain("Gretchy Store");
});
test("signing up with next lands on the authorize request, which names the return host", async () => {
  const made = await fetch(`${base}/signup?next=${encodeURIComponent(next)}`, {
    method: "POST",
    headers: { origin: base },
    body: new URLSearchParams({ username: "newcomer", password: "a-long-enough-passphrase", email: "" }),
    redirect: "manual",
  });
  expect(made.status).toBe(303);
  expect(made.headers.get("location")).toBe(next);
  const cookie = made.headers.get("set-cookie")!.split(";")[0]!;
  const consent = await (await fetch(`${base}${next}`, { headers: { cookie } })).text();
  expect(consent).toContain("Sign in to <strong>gretchy.store</strong>");
});
test("a next that is not a registered authorize request names nobody", async () => {
  const page = await (await fetch(`${base}/signin?next=${encodeURIComponent("/oauth/authorize?client_id=nope")}`)).text();
  expect(page).not.toContain("wants you to sign in");
});
test("a hostile registered name is shown escaped, never as markup", async () => {
  const hostile = "<img src=x onerror=1>";
  const res = await fetch(`${base}/oauth/register`, {
    method: "POST",
    headers: { "content-type": "application/json" },
    body: JSON.stringify({ client_name: hostile, redirect_uris: [REDIRECT] }),
  });
  expect(res.status).toBe(201);
  const clientId = ((await res.json()) as { client_id: string }).client_id;
  const request = `/oauth/authorize?${new URLSearchParams({
    client_id: clientId,
    redirect_uri: REDIRECT,
    response_type: "code",
    code_challenge: "A".repeat(43),
    code_challenge_method: "S256",
  })}`;
  const page = await (await fetch(`${base}/signin?next=${encodeURIComponent(request)}`)).text();
  expect(page).toContain("<strong>gretchy.store</strong>");
  expect(page).toContain("&lt;img src=x onerror=1&gt;");
  expect(page).not.toContain(hostile);
});
const chromium = findChromium();
test.skipIf(chromium === undefined || !chromiumRuns(chromium))("in a browser: stranger, sign in page, Sign up, Create account, back on the consent for the same app", async () => {
  const seen = await clickAndFollow(chromium!, {
    url: `${base}${next}`,
    cookie: { name: "forge_theme", value: "light" },
    steps: [
      `[...document.querySelectorAll("a")].find((a) => a.textContent.includes("Sign up")).click()`,
      `(() => {
        document.querySelector('input[name="username"]').value = "walker";
        document.querySelector('input[name="password"]').value = "a-long-enough-passphrase";
        document.querySelector('form[method="post"] button[type="submit"]').click();
      })()`,
    ],
    until: () => false,
    // The account exists a moment before the redirect to the consent is
    // requested: wait for the redirect, not only the account.
    done: (requested) =>
      getUserByName(app.db, "walker") !== undefined &&
      requested.filter((url) => url.includes("/oauth/authorize")).length > 1,
    timeoutMs: 20_000,
  });
  expect(seen.requested.some((url) => url.includes("/signup?next="))).toBe(true);
  expect(seen.requested.filter((url) => url.includes("/oauth/authorize")).length).toBeGreaterThan(1);
});
		@@ -0,0 +1,13 @@
	1	# Sent to sign in by an app, and back again
	2
	3	An app that uses this forge to sign its people in sends a stranger to `/oauth/authorize`. The forge sends them on to `/signin?next=<that request>`; `src/web/pages/asking.ts` reads the `next` the way the consent page does, so only a registered client with a registered return address is ever described.
	4
	5	- Who is asking: the sign-in and sign-up pages lead with the return host, the verified part ("gretchy.store wants you to sign in"). The name the app registered follows as small secondary text, escaped, never in the lead: dynamic client registration is open, so a name is whatever its registrant typed. The consent page itself still names only the host.
	6	- Sign up keeps the request: every link between the two forms and from the header carries the page the person came from (`src/web/return_to.ts`); a new account ends on the authorize request, so the consent is the next page, not the home page.
	7	- A `next` that is not a registered authorize request says nothing about any app.
	8
	9	## Terminal
	10
	11	There is no terminal form of this: the redirect is a browser exchange between the app and the forge. The same account reaches the forge's terminal with an SSH key added under Settings → Keys (`ssh <forge>`, the exact command is in `FORGE_APP_SSH`); an account made through an app's sign-up is the same account.
	12
	13	Tests: `test/signin-asking.test.ts` (including a real-browser walk, skipped where no Chromium runs).
		@@ -0,0 +1,26 @@
	1	/**
	2	* Who is asking a person to sign in: when the `next` of the sign-in or
	3	* sign-up page is an OAuth authorize request, the app that made it.
	4	* Read the way the consent page reads it, so only a registered client with a
	5	* registered return address is ever named.
	6	*/
	7	import type { Database } from "bun:sqlite";
	8	import { parseAuthRequest } from "../../core/oauth.ts";
	9
	10	export interface Asking {
	11	/** The name the app registered. */
	12	readonly name: string;
	13	/** Where the person will be sent back to: the app's address. */
	14	readonly host: string;
	15	}
	16
	17	export function askingOf(db: Database, next: string \| undefined): Asking \| undefined {
	18	if (next === undefined) return undefined;
	19	const url = URL.parse(next, "http://forge.invalid");
	20	if (url === null \|\| url.pathname !== "/oauth/authorize") return undefined;
	21	const request = parseAuthRequest(db, (n) => url.searchParams.get(n) ?? "");
	22	if (!request.ok) return undefined;
	23	const back = URL.parse(request.value.redirectUri);
	24	if (back === null) return undefined;
	25	return { name: request.value.client.name, host: back.host };
	26	}
		@@ -65,6 +65,7 @@ import {
65	65	import { existsSync } from "node:fs";
66	66	import { join } from "node:path";
67	67	import { secureFor } from "../../http/cookie_secure.ts";
	68	import { askingOf } from "./asking.ts";
68	69	import { SIGNUP_OWN_FORGE, SIGNUP_YOU_GET } from "../../core/contribute.ts";
69	70	import { hasAdmin } from "../../spawn/claim.ts";
70	71	import { getSignup, type SignupMode } from "../../settings/signup.ts";
		@@ -273,6 +274,8 @@ function authForm(
273	274	if (next) query.set("next", next);
274	275	const invite = kind === "signup" ? inviteOf(ctx) : undefined;
275	276	if (invite !== undefined) query.set("invite", invite);
	277	// The way to the other form keeps where the person was going.
	278	const asking = askingOf(ctx.app.db, next);
276	279	const action = query.size === 0 ? `/${kind}` : `/${kind}?${query.toString()}`;
277	280	// "Is this the first account?" is a `LIMIT 1` existence check, not a count
278	281	// of the whole user table — and the sign-in screen paints on every session.
		@@ -443,6 +446,10 @@ function authForm(
443	446	existsSync(join(ctx.app.config.dataDir, "parent.json")) && unowned;
444	447	return html`<div class="narrow">
445	448	${pageHead(kind === "signin" ? "Sign in" : getSignup(ctx.app.db) === "request" ? "Ask for an account" : "Create your account")}
	449	${when(
	450	asking,
	451	(who) => html`<p class="mb" id="asking"><strong>${who.host}</strong> <span class="small muted">(${who.name})</span> wants you to sign in.</p>`,
		sentinelcrew 1d ago outdated HIGH: this shows an attacker-registrable name, bolded, as the actor on the sign-in and sign-up pages. Show the verified host first and the name only as secondary text. It also removes the sign-up copy for this case; check that is wanted. junipercrew 1d ago outdated Principle 1: one sentence. Drop the second clause on both forms ('Make an account here and you go straight back to it.' / 'You go back to it once you have.'). The page already names the host and the form's button says what happens. On sign-up it also sits directly above `signup-you-get`, so the page explains itself twice.
	452	)}
		junipercrew 1d ago outdated Principle 1: 'the forge has not checked that' is a note that changes no decision. Show the name as plain muted text or drop it.
446	453	${when(
447	454	kind === "signup",
448	455	() => html`<p class="mb" id="signup-you-get">${SIGNUP_YOU_GET} <a href="/forges/new">${SIGNUP_OWN_FORGE}</a></p>`,
		@@ -374,7 +374,7 @@ export async function clickAndFollow(
374	374	steps: readonly string[];
375	375	until: (url: string) => boolean;
376	376	/** Done for another reason the page cannot see (a server got the request). */
377		done?: () => boolean;
	377	done?: (requested: readonly string[]) => boolean;
378	378	timeoutMs?: number;
379	379	},
380	380	): Promise<Followed> {
		@@ -444,7 +444,7 @@ export async function clickAndFollow(
444	444	});
445	445	const deadline = Date.now() + (input.timeoutMs ?? 15_000);
446	446	const reached = () =>
447		requested.some(input.until) \|\| (input.done?.() ?? false);
	447	requested.some(input.until) \|\| (input.done?.(requested) ?? false);
448	448	let loaded = cdp.once("Page.loadEventFired");
449	449	await cdp.send("Page.navigate", { url: input.url });
450	450	await Promise.race([loaded, Bun.sleep(Math.max(0, deadline - Date.now()))]);
		@@ -0,0 +1,131 @@
	1	/**
	2	* A person sent to sign in by an app (#12174): the sign-in and sign-up pages
	3	* say who is asking, the way to sign up keeps the authorize request, and a
	4	* new account finishes on it; the consent leads with the verified return host,
	5	* never the registered name.
	6	*/
	7	import { afterAll, beforeAll, expect, test } from "bun:test";
	8	import { mkdtemp, rm } from "node:fs/promises";
	9	import { tmpdir } from "node:os";
	10	import { join } from "node:path";
	11	import { createApp, type App } from "../src/app.ts";
	12	import { findChromium } from "../src/looks/capture.ts";
	13	import { chromiumRuns, clickAndFollow } from "./helpers/chrome.ts";
	14	import { getUserByName } from "../src/core/users.ts";
	15	import { serve, type Servers } from "../src/http/server.ts";
	16
	17	const REDIRECT = "https://gretchy.store/auth/callback";
	18	let dataDir: string;
	19	let app: App;
	20	let servers: Servers;
	21	let base: string;
	22	let next: string;
	23
	24	beforeAll(async () => {
	25	dataDir = await mkdtemp(join(tmpdir(), "forge-signin-asking-"));
	26	delete process.env.FORGE_EDGE_TOKEN;
	27	app = createApp({ dataDir, port: 0 });
	28	servers = await serve(app);
	29	base = servers.url;
	30	const res = await fetch(`${base}/oauth/register`, {
	31	method: "POST",
	32	headers: { "content-type": "application/json" },
	33	body: JSON.stringify({ client_name: "Gretchy Store", redirect_uris: [REDIRECT] }),
	34	});
	35	const clientId = ((await res.json()) as { client_id: string }).client_id;
	36	next = `/oauth/authorize?${new URLSearchParams({
	37	client_id: clientId,
	38	redirect_uri: REDIRECT,
	39	response_type: "code",
	40	code_challenge: "A".repeat(43),
	41	code_challenge_method: "S256",
	42	scope: "openid",
	43	state: "s1",
	44	})}`;
	45	});
	46	afterAll(async () => {
	47	servers.stop();
	48	await rm(dataDir, { recursive: true, force: true });
	49	});
	50
	51	test("authorize sends a stranger to sign in, and both pages name the app and keep the request", async () => {
	52	const sent = await fetch(`${base}${next}`, { redirect: "manual" });
	53	expect(sent.status).toBe(303);
	54	const door = sent.headers.get("location")!;
	55	expect(door.startsWith("/signin?next=")).toBe(true);
	56	const signin = await (await fetch(`${base}${door}`)).text();
	57	expect(signin).toContain("Gretchy Store");
		junipercrew 1d ago Redundant: the assertion on line 54 already contains this string. Remove it, and add an escaping test for a hostile registered name.
	58	expect(signin).toContain("<strong>gretchy.store</strong>");
	59	expect(signin).toContain("(Gretchy Store)</span> wants you to sign in.");
	60	expect(signin).not.toContain("<strong>Gretchy Store</strong>");
	61	const link = `/signup?next=${encodeURIComponent(next)}`;
	62	expect(signin).toContain(`href="${link.replaceAll("&", "&")}"`);
	63	const signup = await (await fetch(`${base}${link}`)).text();
	64	expect(signup).toContain("Gretchy Store");
	65	});
	66
	67	test("signing up with next lands on the authorize request, which names the return host", async () => {
	68	const made = await fetch(`${base}/signup?next=${encodeURIComponent(next)}`, {
	69	method: "POST",
	70	headers: { origin: base },
	71	body: new URLSearchParams({ username: "newcomer", password: "a-long-enough-passphrase", email: "" }),
	72	redirect: "manual",
	73	});
	74	expect(made.status).toBe(303);
	75	expect(made.headers.get("location")).toBe(next);
	76	const cookie = made.headers.get("set-cookie")!.split(";")[0]!;
	77	const consent = await (await fetch(`${base}${next}`, { headers: { cookie } })).text();
	78	expect(consent).toContain("Sign in to <strong>gretchy.store</strong>");
	79	});
	80
	81	test("a next that is not a registered authorize request names nobody", async () => {
	82	const page = await (await fetch(`${base}/signin?next=${encodeURIComponent("/oauth/authorize?client_id=nope")}`)).text();
	83	expect(page).not.toContain("wants you to sign in");
	84	});
	85
	86	test("a hostile registered name is shown escaped, never as markup", async () => {
	87	const hostile = "<img src=x onerror=1>";
	88	const res = await fetch(`${base}/oauth/register`, {
	89	method: "POST",
	90	headers: { "content-type": "application/json" },
	91	body: JSON.stringify({ client_name: hostile, redirect_uris: [REDIRECT] }),
	92	});
	93	expect(res.status).toBe(201);
	94	const clientId = ((await res.json()) as { client_id: string }).client_id;
	95	const request = `/oauth/authorize?${new URLSearchParams({
	96	client_id: clientId,
	97	redirect_uri: REDIRECT,
	98	response_type: "code",
	99	code_challenge: "A".repeat(43),
	100	code_challenge_method: "S256",
	101	})}`;
	102	const page = await (await fetch(`${base}/signin?next=${encodeURIComponent(request)}`)).text();
	103	expect(page).toContain("<strong>gretchy.store</strong>");
	104	expect(page).toContain("<img src=x onerror=1>");
	105	expect(page).not.toContain(hostile);
	106	});
	107
	108	const chromium = findChromium();
	109	test.skipIf(chromium === undefined \|\| !chromiumRuns(chromium))("in a browser: stranger, sign in page, Sign up, Create account, back on the consent for the same app", async () => {
	110	const seen = await clickAndFollow(chromium!, {
	111	url: `${base}${next}`,
	112	cookie: { name: "forge_theme", value: "light" },
	113	steps: [
	114	`[...document.querySelectorAll("a")].find((a) => a.textContent.includes("Sign up")).click()`,
	115	`(() => {
	116	document.querySelector('input[name="username"]').value = "walker";
	117	document.querySelector('input[name="password"]').value = "a-long-enough-passphrase";
	118	document.querySelector('form[method="post"] button[type="submit"]').click();
	119	})()`,
	120	],
	121	until: () => false,
	122	// The account exists a moment before the redirect to the consent is
	123	// requested: wait for the redirect, not only the account.
	124	done: (requested) =>
	125	getUserByName(app.db, "walker") !== undefined &&
	126	requested.filter((url) => url.includes("/oauth/authorize")).length > 1,
	127	timeoutMs: 20_000,
	128	});
	129	expect(seen.requested.some((url) => url.includes("/signup?next="))).toBe(true);
	130	expect(seen.requested.filter((url) => url.includes("/oauth/authorize")).length).toBeGreaterThan(1);
	131	});