diff --git a/crates/fctl-control/src/api/linear.rs b/crates/fctl-control/src/api/linear.rs index ed502515..5cb3e643 100644 --- a/crates/fctl-control/src/api/linear.rs +++ b/crates/fctl-control/src/api/linear.rs @@ -22,6 +22,7 @@ use crate::linear; use crate::workspaces as ws; use fctl_domain::cookies::Cookie; use fctl_domain::linear as lin; +use fctl_domain::verify::Sensitive; use fctl_proto::tickets::{ ConnectLinearRequest, LinearAccount, LinearLink, LinearTeamList, MyLinear, WorkspaceLinear, }; @@ -370,6 +371,14 @@ pub async fn link(ctx: &mut Ctx, req: &Request) -> AppResult { let thread = http::query(req, "thread").filter(|t| { !t.is_empty() && t.len() <= 64 && t.bytes().all(|b| b.is_ascii_alphanumeric() || b == b'-') }); + let project = clean_slug(http::query(req, "project").as_deref()); + // What the account delegates then starts sessions as them, so a stolen + // session can't link one alone, where the instance asks for that. The + // app confirms before it comes here; Linear's own button comes straight + // here, and goes back to where they confirm and link it again. + if ctx.verified(Sensitive::LinkLinear).is_err() { + return http::redirect(&Back::Person { project }.url("confirm")); + } begin( ctx, &keys, @@ -377,7 +386,7 @@ pub async fn link(ctx: &mut Ctx, req: &Request) -> AppResult { purpose: String::new(), user: user.id, workspace: None, - project: clean_slug(http::query(req, "project").as_deref()), + project, thread, state: String::new(), verifier: String::new(), @@ -607,6 +616,15 @@ async fn linked( Some(&viewer.organization), Some(json!({ "name": viewer.organization_name })), ); + // Best effort, as for a Claude connection: the account is linked, + // whatever the email does. + if ctx.cfg.email.is_some() { + let instance = crate::instance::name(&ctx.db, &ctx.cfg) + .await + .unwrap_or_else(|_| ctx.cfg.instance_name.clone()); + let (subject, text) = linked_notice(&instance, &user.email, &ctx.cfg, &viewer); + crate::email::notice(ctx, &user, &subject, text); + } let reached = crate::delegations::linked( &ctx.env, &ctx.cfg, @@ -624,6 +642,29 @@ async fn linked( Ok(thread.and(reached.first().cloned())) } +/// The email that tells someone a Linear account was linked to theirs: +/// what it delegates to fctl starts sessions as them from then on, so one +/// they didn't link is someone else starting their sessions. +fn linked_notice( + instance: &str, + email: &str, + cfg: &crate::config::Config, + viewer: &linear::Viewer, +) -> (String, String) { + let who = match viewer.email.as_deref() { + Some(e) if !e.is_empty() => format!("{} ({e})", viewer.name), + _ => viewer.name.clone(), + }; + ( + "A Linear account was linked to your account".to_owned(), + format!( + "{who}, in the Linear workspace {}, is now linked to your {instance} account ({email}): what it delegates to fctl there, or mentions fctl in, starts sessions as you, on your Claude subscription, and its replies reach your sessions' agents.\n\nIf you didn't link it, unlink it in Settings now, sign out your other devices there and change your password: {}", + viewer.organization_name, + cfg.url("/settings#linear") + ), + ) +} + // ───────────────────────────── the webhook ───────────────────────────── /// `POST /api/v1/linear/webhook`: Linear says that people delegated an diff --git a/crates/fctl-domain/src/verify.rs b/crates/fctl-domain/src/verify.rs index 82a87436..ab114ba0 100644 --- a/crates/fctl-domain/src/verify.rs +++ b/crates/fctl-domain/src/verify.rs @@ -65,6 +65,12 @@ pub enum Sensitive { /// so a stolen session or token could otherwise have their sessions run /// where it watches, for good. ConnectClaude, + /// Linking a Linear account: what it delegates to fctl or mentions it + /// in then starts sessions as the person, on their subscription, and + /// its replies reach their sessions' agents, so a stolen session or + /// token could otherwise go on starting their sessions from Linear long + /// after it was revoked. + LinkLinear, /// Approving a CLI's sign-in, or confirming it's you for a CLI that is /// signed in already. ApproveCli, @@ -98,6 +104,7 @@ impl Sensitive { DeleteProject => "delete a project", NewToken => "make an access token", ConnectClaude => "connect a Claude subscription", + LinkLinear => "link a Linear account", ApproveCli => "approve the CLI", RemovePasskey => "remove a passkey", Export => "export your data", @@ -318,6 +325,10 @@ mod tests { message(Sensitive::ConnectClaude, APP), "Confirm it's you to connect a Claude subscription, with a code from your authenticator app." ); + assert_eq!( + message(Sensitive::LinkLinear, APP), + "Confirm it's you to link a Linear account, with a code from your authenticator app." + ); } #[test] diff --git a/docs/architecture.md b/docs/architecture.md index 11b50cf2..78dbdc7a 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -1876,7 +1876,13 @@ issue's delegate (the person stays its assignee), and mention. Linear's own button in an agent session that waits for it): a Linear sign-in of theirs, with `read` alone, shows fctl who they are there (`viewer`), which D1 keeps (`linear_accounts`: their Linear id, name, - email and avatar), and fctl revokes the sign-in at once. A Linear + email and avatar), and fctl revokes the sign-in at once. Linking takes + confirming it's you where the instance asks for that + (`Sensitive::LinkLinear`), since what the account delegates then runs + as you: the app confirms before it goes on to Linear, and a link opened + from elsewhere (Linear's button, the CLI) goes back to Settings + (`?linear=confirm`) to confirm and link again. fctl emails the person + once it's linked, when email is set up. A Linear account is one person's: whoever links it last has it. Matching addresses would let anyone whose Linear address matches someone's run their work on that person's subscription, and an address in Linear says diff --git a/docs/security.md b/docs/security.md index 148920e8..17e4243b 100644 --- a/docs/security.md +++ b/docs/security.md @@ -117,11 +117,12 @@ owner and purpose), so a value copied between rows does not decrypt. two-factor sign-in, removing a passkey, adding or registering a runner for a workspace and replacing its token, new access tokens and CLI sign-ins, connecting a Claude subscription (whose account a full login's - sessions show in), exporting your data, deleting accounts, projects and + sessions show in), linking a Linear account (whose delegations start + sessions as you), exporting your data, deleting accounts, projects and workspaces, and, without email, making someone a password reset link. - So a stolen browser session, CLI or - access token can't do them alone. Connecting a Claude subscription also - emails the person, when email is set up. A passkey (which must verify the + So a stolen browser session, CLI or access token can't do them alone. + Connecting a Claude subscription and linking a Linear account also + email the person, when email is set up. A passkey (which must verify the person), a code from the authenticator app or a recovery code confirms it, for 15 minutes, for the browser, CLI or token that confirmed; a sign-in with a passkey or a second factor counts. People with neither a diff --git a/web/src/components/project/tickets.test.tsx b/web/src/components/project/tickets.test.tsx index a661a4f7..623358c7 100644 --- a/web/src/components/project/tickets.test.tsx +++ b/web/src/components/project/tickets.test.tsx @@ -25,6 +25,11 @@ vi.mock("@/lib/api", () => { api: never, workspaces: never, linearLinkUrl: (project?: string) => `/api/v1/linear/link${project ? `?project=${project}` : ""}`, + clickConfirmed: (e: { preventDefault(): void }, href: string) => { + e.preventDefault(); + calls.push({ name: "clickConfirmed", args: [href] }); + return undefined; + }, ApiFailure: class extends Error {}, }; }); @@ -347,6 +352,9 @@ describe("the Tickets tab", () => { expect(c.textContent).toContain("assign one of ENG's issues to fctl"); const link = c.querySelector('a[href="/api/v1/linear/link?project=web"]'); expect(link?.textContent).toContain("Link Linear account"); + // Linking confirms it's you first, where the instance asks for that. + act(() => link?.click()); + expect(calls).toContainEqual({ name: "clickConfirmed", args: ["/api/v1/linear/link?project=web"] }); }); it("moves tickets through the team's states, each with a switch", () => { diff --git a/web/src/components/project/tickets.tsx b/web/src/components/project/tickets.tsx index d1959b71..28508593 100644 --- a/web/src/components/project/tickets.tsx +++ b/web/src/components/project/tickets.tsx @@ -38,7 +38,7 @@ import { toast, } from "@/components/ui"; import type * as G from "@/gen"; -import { ApiFailure, api, linearLinkUrl, workspaces } from "@/lib/api"; +import { ApiFailure, api, clickConfirmed, linearLinkUrl, workspaces } from "@/lib/api"; import { cn } from "@/lib/cn"; import { agoShort, stamp } from "@/lib/format"; import { keys, queryClient, useInstance } from "@/lib/queries"; @@ -1236,7 +1236,13 @@ function YourAccount({ project, status }: { project: G.Project; status?: G.Proje Link your Linear account. fctl starts sessions only for people it knows in {name}, as them. It takes a Linear sign-in, which fctl forgets at once.

- + + clickConfirmed(e, linearLinkUrl(project.slug))?.catch((err: Error) => toast(err.message, "error")) + } + > diff --git a/web/src/lib/api.test.ts b/web/src/lib/api.test.ts index baaf487b..52b60acc 100644 --- a/web/src/lib/api.test.ts +++ b/web/src/lib/api.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import { ApiFailure, api, confirmFirst, setConfirmer } from "./api"; +import { ApiFailure, api, clickConfirmed, confirmFirst, goConfirmed, setConfirmer } from "./api"; const asked = { code: "verification_required", @@ -113,3 +113,56 @@ describe("before a download", () => { await expect(confirmFirst()).resolves.toBe(false); }); }); + +describe("links that take confirming it's you", () => { + const status = (required: boolean, verified: boolean) => ({ + status: 200, + body: { required, verified, methods: ["passkey"] }, + }); + const LINK = "/api/v1/linear/link"; + /** The browser's own navigation, as a spy. */ + function navigation() { + const assign = vi.fn(); + vi.stubGlobal("location", { assign }); + return assign; + } + const click = (keys: Partial> = {}) => ({ + button: 0, + metaKey: false, + ctrlKey: false, + shiftKey: false, + altKey: false, + ...keys, + preventDefault: vi.fn(), + }); + + it("goes once confirmed, and stays when the person cancels", async () => { + const assign = navigation(); + answer(status(true, false)); + confirmer(true); + await goConfirmed(LINK); + expect(assign).toHaveBeenCalledWith(LINK); + for (const u of unregister.splice(0)) u(); + assign.mockClear(); + answer(status(true, false)); + confirmer(false); + await goConfirmed(LINK); + expect(assign).not.toHaveBeenCalled(); + }); + + it("takes a plain click, and leaves one that opens a new tab to the browser", async () => { + const assign = navigation(); + answer(status(false, false)); + const plain = click(); + await clickConfirmed(plain, LINK); + expect(plain.preventDefault).toHaveBeenCalled(); + expect(assign).toHaveBeenCalledWith(LINK); + assign.mockClear(); + for (const keys of [{ metaKey: true }, { ctrlKey: true }, { shiftKey: true }]) { + const e = click(keys); + expect(clickConfirmed(e, LINK)).toBeUndefined(); + expect(e.preventDefault).not.toHaveBeenCalled(); + } + expect(assign).not.toHaveBeenCalled(); + }); +}); diff --git a/web/src/lib/api.ts b/web/src/lib/api.ts index 5779748e..387112fb 100644 --- a/web/src/lib/api.ts +++ b/web/src/lib/api.ts @@ -42,6 +42,30 @@ export async function confirmFirst(): Promise { return (await confirmer?.()) ?? false; } +/** + * Follows a link that takes confirming it's you first, where the instance + * asks for it, as linking a Linear account does: the browser's own + * navigation can't go again by itself once the server refuses it. + */ +export async function goConfirmed(href: string): Promise { + if (await confirmFirst()) location.assign(href); +} + +/** + * A click on a link to `href` that takes confirming it's you first + * ([`goConfirmed`]): a plain click confirms, then goes. One that opens a + * new tab or window (with a modifier key) goes on as the browser does, and + * the server sends that tab back to confirm where it must. + */ +export function clickConfirmed( + e: Pick, + href: string, +): Promise | undefined { + if (e.button !== 0 || e.metaKey || e.ctrlKey || e.shiftKey || e.altKey) return undefined; + e.preventDefault(); + return goConfirmed(href); +} + async function request(method: string, path: string, body?: unknown, again = true): Promise { let res: Response; try { diff --git a/web/src/lib/tickets.ts b/web/src/lib/tickets.ts index 7558c2eb..0713150e 100644 --- a/web/src/lib/tickets.ts +++ b/web/src/lib/tickets.ts @@ -47,6 +47,7 @@ export const LINEAR_OUTCOMES = [ "failed", "other_user", "not_app", + "confirm", ] as const; export type LinearOutcome = (typeof LINEAR_OUTCOMES)[number]; @@ -79,6 +80,11 @@ export function linearOutcome(o: LinearOutcome): { tone: "ok" | "warn" | "error" }; case "failed": return { tone: "error", text: "Linear didn't finish the sign-in. Try again in a minute." }; + case "confirm": + return { + tone: "warn", + text: "Linking a Linear account takes confirming it's you, so nothing changed. Link it again from here.", + }; } } diff --git a/web/src/routes/settings.tsx b/web/src/routes/settings.tsx index c7ecbb30..8614da0c 100644 --- a/web/src/routes/settings.tsx +++ b/web/src/routes/settings.tsx @@ -54,7 +54,7 @@ import { } from "@/components/ui"; import { WorkspaceAvatar } from "@/components/workspace/avatar"; import type * as G from "@/gen"; -import { api, confirmFirst, emails, linearLinkUrl, workspaces as wsApi } from "@/lib/api"; +import { api, clickConfirmed, confirmFirst, emails, linearLinkUrl, workspaces as wsApi } from "@/lib/api"; import { avatarImage } from "@/lib/avatar-image"; import { tokenOnRunnersIn } from "@/lib/claude"; import { cn } from "@/lib/cn"; @@ -1067,7 +1067,13 @@ function LinearAccounts({ returned }: { returned?: LinearOutcome }) { )}