From 524b9de9fb3dd8f0795a9534c4cdbbb7621eb1ee Mon Sep 17 00:00:00 2001 From: David McKay Date: Mon, 12 Oct 2026 21:55:40 -0700 Subject: [PATCH] Ask for a sign-in's verification code only after the password The private sign-in form asked for a username, a password and "a verification code, if the site asks for one" all at once, so a person had to guess whether a code was coming and type one before the site had sent it. The form now asks only for the username and password. The computer types them, submits, and if the site then shows a one-time-code field it reports codeRequested and stops. The request moves to a new code_required state, the form asks for the code the site just sent, and POST /api/sign-in-requests/:id/code has the computer type it into the page that is still waiting (new /sign-in/code on the computer, on the acting list like /sign-in/fill). A wrong code keeps the request in code_required with the site's refusal as its outcome; a site that falls back to its login page reopens the login form; an unanswered code expires with the request. Take over and cancel still work from the code step, and an abandoned code attempt reopens at the code step. The Bot is told only the final outcome, never the code. A typed login the site accepted is saved when the code is asked for, and a code is never saved. Audit: computer.sign_in_code_requested for the step, and computer.sign_in_failed carries step "login" or "code". Migration 0056 widens the status check to allow code_required. --- CHANGELOG.md | 17 +- agent-computer/src/authorisation.ts | 1 + agent-computer/src/index.ts | 22 +- agent-computer/src/sign-in.ts | 148 +- agent-computer/tests/authorisation.test.ts | 2 + agent-computer/tests/sign-in.test.ts | 145 +- app/src/components/passwords/sign-in-form.tsx | 112 +- app/src/lib/passwords/mutations.ts | 31 +- app/src/lib/passwords/queries.ts | 2 + .../sign-in-code-after-password.test.tsx | 130 + server/drizzle/0056_sign_in_code_step.sql | 2 + server/drizzle/meta/0056_snapshot.json | 9863 +++++++++++++++++ server/drizzle/meta/_journal.json | 7 + server/src/audit.ts | 2 + server/src/computer/gateway.ts | 21 +- server/src/computer/schema.ts | 19 +- server/src/db/schema/passwords.ts | 2 +- server/src/passwords/routes.ts | 16 +- server/src/passwords/service.ts | 288 +- server/src/passwords/store.ts | 8 +- server/src/passwords/types.ts | 2 + server/tests/passwords-sign-in.test.ts | 302 +- .../tests/passwords-store.integration.test.ts | 61 + 23 files changed, 11071 insertions(+), 132 deletions(-) create mode 100644 app/tests/sign-in-code-after-password.test.tsx create mode 100644 server/drizzle/0056_sign_in_code_step.sql create mode 100644 server/drizzle/meta/0056_snapshot.json diff --git a/CHANGELOG.md b/CHANGELOG.md index ee3543b05..37b87f06a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased -**Before upgrading.** Four things change for an existing deployment: +**Before upgrading.** Five things change for an existing deployment: - A hosted computer's browser is now headed by default. It uses a little more memory than headless did; set `COMPUTER_BROWSER_MODE=headless` (Helm: `computers.browserMode: headless`) to keep the old behaviour. @@ -28,6 +28,21 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. comes back on their next conversation with that Bot (one they turned off and kept stays off). `OPENBOT_MEMORY_SOURCE_DEFAULTS=false` and `OPENBOT_PROACTIVE_DEFAULTS=false` (Helm: `config.memory.*`) prevent it. +- Migration `0056_sign_in_code_step` widens the check on `sign_in_requests.status` to allow + `code_required` (below). A server and a computer must be upgraded together: an older computer has + no `/sign-in/code`. + +### A sign-in's verification code is asked for after the password + +The private sign-in form used to ask for a username, a password and "a verification code, if the +site asks for one" all at once, so a person had to guess whether a code was coming and type one +before the site had sent it. Now the form asks only for the username and password. If the site, +once it has the password, shows a one-time-code field, the request waits in a new `code_required` +state and the form asks for the code the site just sent, on its own. A wrong code keeps asking; a +code nobody enters expires with the request, and the person can still take over the browser or +cancel. The Bot is told only how it ended, never the code, and a saved login still never stores one. +The step is audited as `computer.sign_in_code_requested`, and a failed code as +`computer.sign_in_failed` with `step: "code"`. ### Fewer "Verify you are human" pages diff --git a/agent-computer/src/authorisation.ts b/agent-computer/src/authorisation.ts index 0bd8a1bd5..ddf51b047 100644 --- a/agent-computer/src/authorisation.ts +++ b/agent-computer/src/authorisation.ts @@ -75,6 +75,7 @@ const ACTING_PATHS = new Set([ "/files/write", // Typing a person's login into the page. Refused while they hold the wheel, like any other typing. "/sign-in/fill", + "/sign-in/code", ]); export function actsOnTheComputer(pathname: string): boolean { diff --git a/agent-computer/src/index.ts b/agent-computer/src/index.ts index 01ce9a2f5..e05165acb 100644 --- a/agent-computer/src/index.ts +++ b/agent-computer/src/index.ts @@ -51,7 +51,12 @@ import { } from "./secret-masking"; import { type BotSession, createSessions } from "./sessions"; import { createShell } from "./shell"; -import { fillSignIn, parseSignInFill } from "./sign-in"; +import { + fillSignIn, + fillSignInCode, + parseSignInCode, + parseSignInFill, +} from "./sign-in"; import { startVirtualDisplay } from "./virtual-display"; import { applyWebBotAuthPush } from "./web-bot-auth"; import { @@ -958,6 +963,21 @@ serve({ return json(result); } + /** + * The verification code a site asked for after the password, from the same private form. + * + * Handled exactly as the login above: read once, typed by `fillSignInCode` into the page that is + * waiting for it, and dropped. + */ + if (url.pathname === "/sign-in/code" && request.method === "POST") { + const input = parseSignInCode(await request.json().catch(() => null)); + if (typeof input === "string") return json({ error: input }, 400); + const target = await currentPage(botId); + const result = await fillSignInCode(target, input); + if (result.submitted) session.snapshotId += 1; + return json(result); + } + if ( ["/control/take", "/control/release", "/control/cancel"].includes( url.pathname, diff --git a/agent-computer/src/sign-in.ts b/agent-computer/src/sign-in.ts index 9641017b3..a2ca58375 100644 --- a/agent-computer/src/sign-in.ts +++ b/agent-computer/src/sign-in.ts @@ -3,8 +3,13 @@ * * The private sign-in form posts a username and password to the server, the server posts them here, * and this types them. The Bot is never involved: it does not pick the fields, it does not see the - * values, and what comes back says only whether a form was submitted and whether a password field is - * still showing. + * values, and what comes back says only whether a form was submitted, whether a password field is + * still showing, and whether the site went on to ask for a verification code. + * + * A verification code is never asked for up front. It comes after the password, and only when the + * site, having accepted the password, shows a one-time-code field: `fillSignIn` reports that and + * stops, the person is asked for the code the site just sent, and `fillSignInCode` types it into the + * page that is still waiting for it. * * Three refusals, before anything is typed: * - the page must be on the exact origin the person was asked to sign in to, so a login cannot be @@ -23,12 +28,24 @@ export type SignInFillInput = { origin: string; username?: string; password: string; - code?: string; }; export type SignInFillResult = { submitted: boolean; passwordFieldVisible: boolean; + /** The site accepted the password and is now showing a one-time-code field, on this origin. */ + codeRequested: boolean; + url: string; + error?: string; +}; + +export type SignInCodeInput = { origin: string; code: string }; + +export type SignInCodeResult = { + submitted: boolean; + /** Whether a code field is still showing after submitting: the plainest sign the code was refused. */ + codeFieldVisible: boolean; + passwordFieldVisible: boolean; url: string; error?: string; }; @@ -58,16 +75,27 @@ export function parseSignInFill(body: unknown): SignInFillInput | string { return "A sign-in needs a password."; if (input.username !== undefined && typeof input.username !== "string") return "The username must be text."; - if (input.code !== undefined && typeof input.code !== "string") - return "The code must be text."; + // A `code` is not read: one is only ever typed after the site asks for it, by `fillSignInCode`. return { origin: originOfUrl(input.origin), password: input.password, ...(input.username ? { username: input.username } : {}), - ...(input.code ? { code: input.code } : {}), }; } +/** Parse the verification-code body, the same way. */ +export function parseSignInCode(body: unknown): SignInCodeInput | string { + if (!body || typeof body !== "object") return "A sign-in needs a JSON body."; + const input = body as Record; + if (typeof input.origin !== "string" || !originOfUrl(input.origin)) + return "A sign-in names the website's origin."; + if (typeof input.code !== "string" || !input.code.trim()) + return "A verification code is required."; + if (input.code.length > 64) + return "A verification code is at most 64 characters."; + return { origin: originOfUrl(input.origin), code: input.code.trim() }; +} + /** * Find the login fields on the page and mark them for the locators below. * @@ -123,23 +151,39 @@ function markLoginFields(attribute: string) { return { username: Boolean(username), password: Boolean(password) }; } -/** Find a one-time-code field. Runs in the page. */ +/** + * Find a one-time-code field. Runs in the page. + * + * The site's own `autocomplete="one-time-code"` first; otherwise a visible text-like field whose name, + * id or label reads as a verification code. Any earlier mark is cleared, so a field that has gone + * away is never typed into. + */ function markCodeField(attribute: string) { + for (const node of document.querySelectorAll(`[${attribute}="code"]`)) + node.removeAttribute(attribute); const inputs = [ ...document.querySelectorAll("input"), ].filter((input) => { const box = input.getBoundingClientRect(); + const style = getComputedStyle(input); return ( box.width > 0 && box.height > 0 && + style.visibility !== "hidden" && + style.display !== "none" && !input.disabled && - input.type !== "hidden" + !input.readOnly && + ["text", "tel", "number", "password", ""].includes(input.type) ); }); const code = inputs.find((input) => /one-time-code/.test(input.autocomplete)) ?? - inputs.find((input) => - /code|otp|totp|2fa|mfa|verif|token/i.test(`${input.name} ${input.id}`), + inputs.find( + (input) => + input.type !== "password" && + /code|otp|totp|2fa|mfa|verif|passcode/i.test( + `${input.name} ${input.id} ${input.getAttribute("aria-label") ?? ""}`, + ), ); if (code) code.setAttribute(attribute, "code"); return Boolean(code); @@ -188,8 +232,13 @@ async function settle(page: Page) { .catch(() => undefined); } +/** A password field still showing, other than a one-time-code field a site drew as one. */ async function passwordVisible(page: Page) { - return (await page.locator("input[type=password]:visible").count()) > 0; + return ( + (await page + .locator(`input[type=password]:visible:not([${LOGIN_ATTRIBUTE}="code"])`) + .count()) > 0 + ); } async function typeSecret(field: Locator, value: string) { @@ -204,6 +253,7 @@ export async function fillSignIn( const answer = (error: string, submitted = false): SignInFillResult => ({ submitted, passwordFieldVisible: false, + codeRequested: false, url: page.url(), error, }); @@ -246,30 +296,70 @@ export async function fillSignIn( await typeSecret(passwordField, input.password); await submitFrom(page, passwordField); await settle(page); - if (input.code) { - if (originOfUrl(page.url()) !== input.origin) - return answer( - `The sign-in moved to ${originOfUrl(page.url()) || "another page"} before the code was asked for. Take over the browser to finish.`, - true, - ); - let hasCode = await page.evaluate(markCodeField, LOGIN_ATTRIBUTE); - if (!hasCode) { - await page.waitForTimeout(2_000); - hasCode = await page.evaluate(markCodeField, LOGIN_ATTRIBUTE); - } - if (hasCode) { - const codeField = page.locator(`[${LOGIN_ATTRIBUTE}="code"]`); - await typeSecret(codeField, input.code); - await submitFrom(page, codeField); - await settle(page); - } + // A password field still showing is a refused login; the server says so. Only past that is a + // code field the site asking for a second step. The code field is looked for first, so one a + // site draws as a password input is not mistaken for the login's own, and is then given a + // moment to appear. + let codeRequested = await page.evaluate(markCodeField, LOGIN_ATTRIBUTE); + const refused = await passwordVisible(page); + if (!codeRequested && !refused) { + await page.waitForTimeout(2_000); + codeRequested = await page.evaluate(markCodeField, LOGIN_ATTRIBUTE); } + if (codeRequested && originOfUrl(page.url()) !== input.origin) + return answer( + `The sign-in moved to ${originOfUrl(page.url()) || "another page"} to ask for a verification code. Take over the browser to finish signing in there.`, + true, + ); return { submitted: true, - passwordFieldVisible: await passwordVisible(page), + passwordFieldVisible: codeRequested ? false : refused, + codeRequested, url: page.url(), }; } catch { return answer("The login could not be typed into the page."); } } + +/** + * Type the verification code a site asked for into the page that is waiting for it. + * + * Only after `fillSignIn` reported `codeRequested`, and with the same refusals: the page must still + * be on the origin the person was asked to sign in to, and must still be showing a code field. + */ +export async function fillSignInCode( + page: Page, + input: SignInCodeInput, +): Promise { + const answer = (error: string, submitted = false): SignInCodeResult => ({ + submitted, + codeFieldVisible: false, + passwordFieldVisible: false, + url: page.url(), + error, + }); + if (originOfUrl(page.url()) !== input.origin) + return answer( + `The Bot's browser is not on ${input.origin} any more. Take over the browser to finish signing in.`, + ); + try { + if (!(await page.evaluate(markCodeField, LOGIN_ATTRIBUTE))) + return answer("The site is not asking for a verification code any more."); + const codeField = page.locator(`[${LOGIN_ATTRIBUTE}="code"]`); + await typeSecret(codeField, input.code); + await submitFrom(page, codeField); + await settle(page); + const codeFieldVisible = + originOfUrl(page.url()) === input.origin && + (await page.evaluate(markCodeField, LOGIN_ATTRIBUTE)); + return { + submitted: true, + codeFieldVisible, + passwordFieldVisible: await passwordVisible(page), + url: page.url(), + }; + } catch { + return answer("The verification code could not be typed into the page."); + } +} diff --git a/agent-computer/tests/authorisation.test.ts b/agent-computer/tests/authorisation.test.ts index 02927ebcc..873ad4aa0 100644 --- a/agent-computer/tests/authorisation.test.ts +++ b/agent-computer/tests/authorisation.test.ts @@ -120,6 +120,8 @@ describe("what the wheel stops while a person is driving", () => { "/scroll", "/exec", "/files/write", + "/sign-in/fill", + "/sign-in/code", ]) { expect(actsOnTheComputer(path)).toBeTrue(); } diff --git a/agent-computer/tests/sign-in.test.ts b/agent-computer/tests/sign-in.test.ts index f2f45456e..6f24af292 100644 --- a/agent-computer/tests/sign-in.test.ts +++ b/agent-computer/tests/sign-in.test.ts @@ -7,7 +7,13 @@ import { SECRET_ATTRIBUTE, sensitiveRefs, } from "../src/secret-masking"; -import { fillSignIn, originOfUrl, parseSignInFill } from "../src/sign-in"; +import { + fillSignIn, + fillSignInCode, + originOfUrl, + parseSignInCode, + parseSignInFill, +} from "../src/sign-in"; /** * The computer's half of the private sign-in, against a real Chromium and a real login page served @@ -22,6 +28,7 @@ import { fillSignIn, originOfUrl, parseSignInFill } from "../src/sign-in"; */ const asked = process.env.OPENBOT_COMPUTER_BROWSER === "1"; const PASSWORD = "hunter2-not-for-the-model"; +const CODE = "482913"; let browser: Browser; let server: ReturnType; let origin = ""; @@ -57,6 +64,40 @@ beforeAll(async () => { return page(`
`); + // A site that asks for a one-time code only once the password is accepted, the way MFA works. + if (url.pathname === "/mfa-login") + return page(`
+ + +
`); + if (url.pathname === "/mfa-session" && request.method === "POST") { + const form = await request.formData(); + return form.get("password") === PASSWORD + ? Response.redirect(`${origin}/mfa-code`, 303) + : Response.redirect(`${origin}/mfa-login`, 303); + } + if (url.pathname === "/mfa-code") + return page(`

We sent a code to your phone.

+
+ +
`); + // The same, with the code field drawn as a password input, as some sites do. + if (url.pathname === "/masked-login") + return page(`
+ +
`); + if (url.pathname === "/masked-session" && request.method === "POST") + return page(`
+ +
`); + if (url.pathname === "/mfa-verify" && request.method === "POST") { + const form = await request.formData(); + return form.get("otp") === CODE + ? Response.redirect(`${origin}/home`, 303) + : page(`

That code is not right.

+
+
`); + } if (url.pathname === "/home") return page("

Welcome back

"); return new Response("not found", { status: 404 }); }, @@ -94,6 +135,23 @@ describe("parsing what the server sends", () => { }); expect(originOfUrl("about:blank")).toBe(""); }); + + test("a code is never part of the login, and comes on its own once the site asks", () => { + // Sent with the password by an older client: dropped, so it can never be typed early. + expect(parseSignInFill({ origin, password: PASSWORD, code: CODE })).toEqual( + { origin, password: PASSWORD }, + ); + expect(parseSignInCode({ origin, code: "" })).toBe( + "A verification code is required.", + ); + expect(parseSignInCode({ origin: "nope", code: CODE })).toBe( + "A sign-in names the website's origin.", + ); + expect(parseSignInCode({ origin: `${origin}/x`, code: CODE })).toEqual({ + origin, + code: CODE, + }); + }); }); describe.skipIf(!asked)("typing a login", () => { @@ -107,6 +165,7 @@ describe.skipIf(!asked)("typing a login", () => { expect(result).toEqual({ submitted: true, passwordFieldVisible: false, + codeRequested: false, url: `${origin}/home`, }); expect(JSON.stringify(result)).not.toContain(PASSWORD); @@ -154,6 +213,90 @@ describe.skipIf(!asked)("typing a login", () => { }); }); +describe.skipIf(!asked)( + "a site that asks for a code after the password", + () => { + test("stops at the code page and says so, then types the code it is given and finishes", async () => { + const tab = await open("/mfa-login"); + const login = await fillSignIn(tab, { + origin, + username: "a@example.test", + password: PASSWORD, + }); + // The password went in and the site moved on to ask for a code. Nothing was guessed at it. + expect(login).toEqual({ + submitted: true, + passwordFieldVisible: false, + codeRequested: true, + url: `${origin}/mfa-code`, + }); + expect(await tab.locator("input[name=otp]").inputValue()).toBe(""); + + const finished = await fillSignInCode(tab, { origin, code: CODE }); + expect(finished).toEqual({ + submitted: true, + codeFieldVisible: false, + passwordFieldVisible: false, + url: `${origin}/home`, + }); + expect(JSON.stringify([login, finished])).not.toContain(PASSWORD); + expect(JSON.stringify([login, finished])).not.toContain(CODE); + await tab.close(); + }); + + test("a code field drawn as a password input is the code step, not a refused login", async () => { + const tab = await open("/masked-login"); + const login = await fillSignIn(tab, { + origin, + username: "a@example.test", + password: PASSWORD, + }); + expect(login).toMatchObject({ + submitted: true, + passwordFieldVisible: false, + codeRequested: true, + }); + const finished = await fillSignInCode(tab, { origin, code: CODE }); + expect(finished.url).toBe(`${origin}/home`); + await tab.close(); + }); + + test("a code the site refuses leaves its code field showing, which is the failure signal", async () => { + const tab = await open("/mfa-login"); + await fillSignIn(tab, { + origin, + username: "a@example.test", + password: PASSWORD, + }); + const refused = await fillSignInCode(tab, { origin, code: "000000" }); + expect(refused).toMatchObject({ + submitted: true, + codeFieldVisible: true, + }); + // And the right one still finishes from there. + const finished = await fillSignInCode(tab, { origin, code: CODE }); + expect(finished).toMatchObject({ codeFieldVisible: false }); + expect(finished.url).toBe(`${origin}/home`); + await tab.close(); + }); + + test("types no code when the page is not asking for one, or is on another origin", async () => { + const tab = await open("/home"); + expect(await fillSignInCode(tab, { origin, code: CODE })).toMatchObject({ + submitted: false, + error: "The site is not asking for a verification code any more.", + }); + const elsewhere = await fillSignInCode(tab, { + origin: "https://example.com", + code: CODE, + }); + expect(elsewhere.submitted).toBe(false); + expect(elsewhere.error).toContain("not on https://example.com"); + await tab.close(); + }); + }, +); + describe.skipIf(!asked)("secrets typed into the page are not read back", () => { test("the aria snapshot shows a password's value, and the masked snapshot does not", async () => { const tab = await browser.newPage(); diff --git a/app/src/components/passwords/sign-in-form.tsx b/app/src/components/passwords/sign-in-form.tsx index 01597ac4b..6eb73a1ca 100644 --- a/app/src/components/passwords/sign-in-form.tsx +++ b/app/src/components/passwords/sign-in-form.tsx @@ -1,4 +1,9 @@ -import { IconKey, IconLock, IconPointer } from "@tabler/icons-react"; +import { + IconKey, + IconLock, + IconPointer, + IconShieldCheck, +} from "@tabler/icons-react"; import { useMutation, useQuery } from "@tanstack/react-query"; import { Link } from "@tanstack/react-router"; import { useId, useState } from "react"; @@ -19,6 +24,7 @@ import { cancelSignInMutationOptions, confirmSavedLoginMutationOptions, finishSignInMutationOptions, + submitSignInCodeMutationOptions, submitSignInMutationOptions, takeOverSignInMutationOptions, } from "@/lib/passwords/mutations"; @@ -37,6 +43,10 @@ import { queryClient } from "@/query-client"; * component's state until it is sent and is cleared the moment it is. * * Four answers: type a login, confirm a saved one, take over the browser, or decline. + * + * A verification code is never asked for here up front. If the site, once it has the password, sends + * one, the server reports `code_required` and this form asks for that code alone, then sends it on + * its own, cleared from state the same way. */ export function SignInForm({ requestId }: { requestId: string }) { const request = useQuery({ @@ -48,6 +58,7 @@ export function SignInForm({ requestId }: { requestId: string }) { : 3_000, }); const submit = useMutation(submitSignInMutationOptions(queryClient)); + const submitCode = useMutation(submitSignInCodeMutationOptions(queryClient)); const confirmSaved = useMutation( confirmSavedLoginMutationOptions(queryClient), ); @@ -70,11 +81,13 @@ export function SignInForm({ requestId }: { requestId: string }) { const data = request.data; const busy = submit.isPending || + submitCode.isPending || confirmSaved.isPending || takeOver.isPending || finish.isPending || cancel.isPending; const failure = + submitCode.error ?? submit.error ?? confirmSaved.error ?? takeOver.error ?? @@ -90,6 +103,73 @@ export function SignInForm({ requestId }: { requestId: string }) { ); + if (data.status === "code_required") + return ( + +
{ + event.preventDefault(); + const value = code.trim(); + if (!value) return; + submitCode.mutate( + { id: requestId, code: value }, + // Cleared whatever happened: a code is good once, and a wrong one is retyped. + { onSettled: () => setCode("") }, + ); + }} + > + + {failure || data.outcome ? ( +

+ {failure?.message ?? data.outcome} +

+ ) : null} +
+ + + +
+
+ + ); + if (data.status === "taken_over") return ( - confirmSaved.mutate( - { - id: requestId, - loginId: login.id, - ...(code ? { code } : {}), - }, - { onSettled: () => setCode("") }, - ) + confirmSaved.mutate({ + id: requestId, + loginId: login.id, + }) } > Confirm and sign in @@ -194,16 +270,12 @@ export function SignInForm({ requestId }: { requestId: string }) { input: { ...(username ? { username } : {}), password, - ...(code ? { code } : {}), save, }, }, { // Cleared whatever happened: a failed attempt is retyped, never kept. - onSettled: () => { - setPassword(""); - setCode(""); - }, + onSettled: () => setPassword(""), }, ); }} @@ -236,18 +308,6 @@ export function SignInForm({ requestId }: { requestId: string }) { onChange={(event) => setPassword(event.target.value)} /> - {data.passwordManager ? (