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 ? (