Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions companion/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ upstream hardened its loopback gate.

| | |
|---|---|
| **Pairing** | A six-digit code shown on the computer, valid two minutes, five attempts. Redeeming it returns a device token stored only as a SHA-256 digest. |
| **Authorisation** | Every request needs that token. A rebinding page cannot obtain one. |
| **Pairing** | A high-entropy QR credential plus a six-digit manual fallback, valid two minutes and single-use. Redeeming either returns a device token stored only as a SHA-256 digest. |
| **Authorisation** | Every request needs that token. Full cloud-desktop access is a separate per-device capability, off by default. A rebinding page cannot obtain either. |
| **The allowlist** | Default deny, per method and path (`src/routes.ts`) — the list is every request the app makes, and nothing else. General bot/room PATCH routes stay closed; read state and approval grants use narrow verbs. A route that appears in the harness later is closed to devices until someone adds it here on purpose. |
| **Scrubbing** | `resumeCursors` — the harness's own provider session ids — never reach a device, whether or not the harness still sends them. |
| **Discovery** | Bonjour, so a phone finds the computer by name instead of by typed address. |
Expand Down
21 changes: 20 additions & 1 deletion companion/src/control.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,17 @@ export function createControlServer(options: ControlOptions): Server {
options.devices.closePairing();
return json(res, 200, companionState(options));
}
const cloudDesktop = path.match(/^\/devices\/([\w-]+)\/cloud-desktop$/);
if (cloudDesktop && (method === "POST" || method === "DELETE")) {
try {
if (!options.devices.setCloudDesktopAccess(cloudDesktop[1], method === "POST")) {
return json(res, 404, { error: "no such device" });
}
} catch {
return json(res, 500, { error: "could not save cloud desktop access" });
}
return json(res, 200, companionState(options));
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
const revoke = path.match(/^\/devices\/([\w-]+)$/);
if (revoke && method === "DELETE") {
if (!options.devices.revoke(revoke[1])) return json(res, 404, { error: "no such device" });
Expand Down Expand Up @@ -279,7 +290,9 @@ function render(s) {
(s.devices.length
? "<ul>" + s.devices.map((d) =>
"<li><div class='grow'><div class=name>" + esc(d.name) + "</div>" +
"<div class=dim>Last seen " + ago(d.lastSeenAt) + "</div></div>" +
"<div class=dim>Last seen " + ago(d.lastSeenAt) + "</div>" +
"<button data-cloud='" + esc(d.id) + "' data-allowed='" + (d.cloudDesktopAccess ? "1" : "0") + "'>" +
(d.cloudDesktopAccess ? "Cloud desktop on" : "Allow cloud desktop") + "</button></div>" +
"<button data-revoke='" + esc(d.id) + "'>Remove</button></li>").join("") + "</ul>"
: "<p class=dim>No phones are paired yet.</p>");

Expand All @@ -288,6 +301,12 @@ function render(s) {
for (const b of document.querySelectorAll("[data-revoke]")) {
b.addEventListener("click", async () => render(await api("/devices/" + b.dataset.revoke, "DELETE")));
}
for (const b of document.querySelectorAll("[data-cloud]")) {
b.addEventListener("click", async () => render(await api(
"/devices/" + b.dataset.cloud + "/cloud-desktop",
b.dataset.allowed === "1" ? "DELETE" : "POST"
)));
}
if (s.pairing) {
const tick = () => {
const left = Math.max(0, Math.round((s.pairing.expiresAt - Date.now()) / 1000));
Expand Down
22 changes: 22 additions & 0 deletions companion/src/devices.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ export interface DeviceRecord {
tokenHash: string;
createdAt: number;
lastSeenAt: number;
/** Full interactive access to a bot's cloud desktop. Deliberately off on
* every new and migrated device until the computer owner enables it. */
cloudDesktopAccess: boolean;
}

/** What the UI is allowed to see: a device without its secret. */
Expand Down Expand Up @@ -100,6 +103,7 @@ function normalizeDevice(record: Partial<DeviceRecord> & { id: string; tokenHash
name: cleanDeviceName(record.name),
createdAt,
lastSeenAt: timestamp(record.lastSeenAt, createdAt),
cloudDesktopAccess: record.cloudDesktopAccess === true,
};
}

Expand Down Expand Up @@ -209,6 +213,7 @@ export class DeviceRegistry {
tokenHash: sha256(token),
createdAt: Date.now(),
lastSeenAt: Date.now(),
cloudDesktopAccess: false,
};
this.devices.push(device);
// Unlike the lastSeenAt write below, this one must not be swallowed. A
Expand Down Expand Up @@ -260,6 +265,23 @@ export class DeviceRegistry {
this.persist();
return true;
}

/** Grant or remove the one capability that crosses from companion actions
* into full desktop control. This is per device so a watch-only phone does
* not inherit a different phone's permission. */
setCloudDesktopAccess(id: string, allowed: boolean): boolean {
const device = this.devices.find((candidate) => candidate.id === id);
if (!device) return false;
const previous = device.cloudDesktopAccess;
device.cloudDesktopAccess = allowed;
try {
this.persist();
} catch (error) {
device.cloudDesktopAccess = previous;
throw error;
}
return true;
}
}

/**
Expand Down
2 changes: 1 addition & 1 deletion companion/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ const companion = createServer(
harnessPort: HARNESS_PORT,
// `authenticate` also stamps lastSeenAt, which is what makes the control
// page able to say when a phone was last heard from.
authenticate: (token) => Boolean(devices.authenticate(token)),
authenticate: (token) => devices.authenticate(token),
redeem: (code, deviceName) => devices.redeem(code, deviceName),
serverName: machineName,
}),
Expand Down
17 changes: 14 additions & 3 deletions companion/src/proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,15 @@
import { request as httpRequest, type IncomingMessage, type ServerResponse } from "node:http";

import { bearerToken } from "./devices.ts";
import { denyReason } from "./routes.ts";
import { denyReason, isCloudDesktopJoin } from "./routes.ts";
import { createSseScrubber, isJson, scrub } from "./wire.ts";

/** What the forwarding handler needs from the process around it. */
export interface ProxyOptions {
/** Where the harness is listening on loopback. */
harnessPort: number;
/** Does this bearer token belong to a paired device? */
authenticate: (token: string | undefined) => boolean;
authenticate: (token: string | undefined) => { cloudDesktopAccess: boolean } | null;
/** Redeem a pairing code. Handled here and never forwarded: the harness
* has no such route and no idea devices exist — pairing is the sidecar's
* own concern, and the one thing a device does before it has a token. */
Expand Down Expand Up @@ -131,17 +131,28 @@ export function createProxyHandler(options: ProxyOptions) {
return sendJson(res, 403, { error: "forbidden: cross-origin request" });
}

const token = bearerToken(req.headers.authorization);
const device = options.authenticate(token);
const denial = denyReason({
path,
method,
// `bearerToken` is the registry's own parser, imported rather than
// reimplemented: this file used to have a second one, and two parsers
// that disagree about what a credential looks like means the header a
// phone sends authenticates on one code path and not the other.
authenticated: options.authenticate(bearerToken(req.headers.authorization)),
authenticated: Boolean(device),
});
if (denial) return sendJson(res, denial.status, { error: denial.error });

// Pairing a phone grants the ordinary companion surface, not a browser
// session with every credential that may exist inside the cloud desktop.
// The computer owner enables this capability per device, off by default.
if (isCloudDesktopJoin(method, path) && !device?.cloudDesktopAccess) {
return sendJson(res, 403, {
error: "cloud desktop access is off for this phone — enable it in OpenMausBot → Settings → Companion",
});
}

// Pairing terminates here. Forwarding it would hand the harness a route
// it does not have, and the 404 would read to a phone as "wrong address".
if (method === "POST" && path === "/api/pair") {
Expand Down
15 changes: 15 additions & 0 deletions companion/src/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,18 @@ export interface RouteRequest {
authenticated: boolean;
}

/** The one companion route that crosses into full interactive desktop
* control. Both the allowlist and capability gate consume this classifier so
* their security decisions cannot drift apart. */
export const CLOUD_DESKTOP_JOIN_ROUTE = {
method: "POST",
path: /^\/api\/bots\/[\w-]+\/computer\/join$/,
} as const;

export function isCloudDesktopJoin(method: string, path: string): boolean {
return method === CLOUD_DESKTOP_JOIN_ROUTE.method && CLOUD_DESKTOP_JOIN_ROUTE.path.test(path);
}

/** Every request the iOS app makes, and nothing else.
*
* Ids are `[\w-]+`, matching the harness's own route patterns. The paths
Expand Down Expand Up @@ -61,6 +73,9 @@ const ALLOWED: ReadonlyArray<{ method: string; path: RegExp }> = [
{ method: "POST", path: /^\/api\/bots\/[\w-]+\/tasks\/[\w-]+$/ },
{ method: "PATCH", path: /^\/api\/bots\/[\w-]+\/tasks\/[\w-]+$/ },
{ method: "DELETE", path: /^\/api\/bots\/[\w-]+\/tasks\/[\w-]+$/ },
// Full cloud desktop access. The route is narrow and the proxy applies a
// second, per-device capability check before it reaches the harness.
CLOUD_DESKTOP_JOIN_ROUTE,

// rooms
{ method: "POST", path: /^\/api\/groups\/[\w-]+\/messages$/ },
Expand Down
36 changes: 34 additions & 2 deletions companion/test/control.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@
// inside that boundary, and any page on the internet can aim a form at it.
// These tests pin the rule that keeps that from mattering.
import { type Server } from "node:http";
import { afterAll, beforeAll, describe, expect, it } from "vitest";
import { afterAll, beforeAll, describe, expect, it, vi } from "vitest";

import { createControlServer, originIsLoopback } from "../src/control.ts";
import { DeviceRegistry } from "../src/devices.ts";

let control: Server;
let port = 0;
let devices: DeviceRegistry;

const ask = async (
method: string,
Expand All @@ -28,8 +29,9 @@ const ask = async (
};

beforeAll(async () => {
devices = new DeviceRegistry();
control = createControlServer({
devices: new DeviceRegistry(),
devices,
companionPort: 8810,
discovery: () => ({ advertising: false, name: "Test computer" }),
});
Expand All @@ -43,6 +45,36 @@ afterAll(async () => {
});

describe("origins the control server will change state for", () => {
it("controls cloud desktop access per paired device", async () => {
const { code } = devices.openPairing();
const paired = devices.redeem(code, "iPhone");
if ("error" in paired) throw new Error(paired.error);

expect(paired.device.cloudDesktopAccess).toBe(false);
expect((await ask("POST", `/devices/${paired.device.id}/cloud-desktop`)).status).toBe(200);
expect(devices.authenticate(paired.token)?.cloudDesktopAccess).toBe(true);
expect((await ask("DELETE", `/devices/${paired.device.id}/cloud-desktop`)).status).toBe(200);
expect(devices.authenticate(paired.token)?.cloudDesktopAccess).toBe(false);
expect((await ask("POST", "/devices/missing/cloud-desktop")).status).toBe(404);
});

it("reports a permission write failure without dropping the control server", async () => {
const [device] = devices.list();
const save = vi.spyOn(devices, "setCloudDesktopAccess").mockImplementation(() => {
throw new Error("ENOSPC: no space left on device");
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
try {
const failed = await ask("POST", `/devices/${device.id}/cloud-desktop`);
expect(failed).toEqual({
status: 500,
body: { error: "could not save cloud desktop access" },
});
expect((await ask("GET", "/state")).status).toBe(200);
} finally {
save.mockRestore();
}
});

it("refuses a state change from a foreign page", async () => {
// The attack this exists for: a form POST needs no preflight, and the
// Host header on it is the loopback one this server already approves.
Expand Down
27 changes: 27 additions & 0 deletions companion/test/devices.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ describe("DeviceRegistry", () => {
delete stored.devices[0].name;
delete stored.devices[0].lastSeenAt;
delete stored.devices[0].createdAt;
delete stored.devices[0].cloudDesktopAccess;
writeFileSync(file, JSON.stringify(stored));

const reloaded = new DeviceRegistry();
Expand All @@ -68,6 +69,7 @@ describe("DeviceRegistry", () => {
expect(listed.name).toBe("Companion");
expect(Number.isFinite(listed.lastSeenAt)).toBe(true);
expect(Number.isFinite(listed.createdAt)).toBe(true);
expect(listed.cloudDesktopAccess).toBe(false);
// and the token it was paired with still works
expect(reloaded.authenticate(token)?.id).toBe(device.id);
});
Expand Down Expand Up @@ -126,6 +128,31 @@ describe("DeviceRegistry", () => {
expect(registry.count()).toBe(1);
});

it("keeps cloud desktop access off until enabled for that device", () => {
const registry = new DeviceRegistry();
const { token, device } = pair(registry);

expect(device.cloudDesktopAccess).toBe(false);
expect(registry.authenticate(token)?.cloudDesktopAccess).toBe(false);
expect(registry.setCloudDesktopAccess(device.id, true)).toBe(true);
expect(registry.authenticate(token)?.cloudDesktopAccess).toBe(true);
expect(new DeviceRegistry().authenticate(token)?.cloudDesktopAccess).toBe(true);
expect(registry.setCloudDesktopAccess(device.id, false)).toBe(true);
expect(registry.authenticate(token)?.cloudDesktopAccess).toBe(false);
expect(registry.setCloudDesktopAccess("missing", true)).toBe(false);
});

it("rolls cloud desktop access back when it cannot be saved", () => {
const registry = new DeviceRegistry();
const { token, device } = pair(registry);
(registry as unknown as { persist: () => void }).persist = () => {
throw new Error("ENOSPC: no space left on device");
};

expect(() => registry.setCloudDesktopAccess(device.id, true)).toThrow("ENOSPC");
expect(registry.authenticate(token)?.cloudDesktopAccess).toBe(false);
});

it("uses a high-entropy QR credential and burns the manual fallback with it", () => {
const registry = new DeviceRegistry();
const { code, token } = registry.openPairing();
Expand Down
27 changes: 25 additions & 2 deletions companion/test/proxy-response.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const deeplyNested = (() => {
let harness: Server;
let sidecar: Server;
let sidecarPort = 0;
let cloudDesktopAccess = true;
/** What the stub harness answers with next. Set per test. */
let respond: (res: ServerResponse) => void = (res) => res.end();

Expand All @@ -33,8 +34,9 @@ const close = (server: Server | undefined): Promise<void> =>
new Promise((resolve) => (server ? server.close(() => resolve()) : resolve()));

/** A request as a paired device makes it. */
const device = async (path = "/api/bots"): Promise<{ status: number; text: string }> => {
const device = async (path = "/api/bots", method = "GET"): Promise<{ status: number; text: string }> => {
const res = await fetch(`http://127.0.0.1:${sidecarPort}${path}`, {
method,
headers: { authorization: `Bearer ${TOKEN}` },
});
return { status: res.status, text: await res.text() };
Expand All @@ -47,7 +49,7 @@ beforeAll(async () => {
sidecar = createServer(
createProxyHandler({
harnessPort,
authenticate: (t) => t === TOKEN,
authenticate: (t) => (t === TOKEN ? { cloudDesktopAccess } : null),
redeem: () => ({ error: "not used here" }),
serverName: () => "Test computer",
}),
Expand All @@ -61,6 +63,27 @@ afterAll(async () => {
});

describe("preparing a harness response for a device", () => {
it("requires the Mac to enable cloud desktop for this phone", async () => {
cloudDesktopAccess = false;
try {
const { status, text } = await device("/api/bots/b1/computer/join", "POST");
expect(status).toBe(403);
expect(text).toContain("enable it in OpenMausBot");
} finally {
cloudDesktopAccess = true;
}
});

it("forwards only the enabled device's request for a fresh viewer", async () => {
respond = (res) => {
res.writeHead(200, { "content-type": "application/json" });
res.end(JSON.stringify({ joinUrl: "https://desktop.example/session/fresh", state: "ready" }));
};
const { status, text } = await device("/api/bots/b1/computer/join", "POST");
expect(status).toBe(200);
expect(JSON.parse(text).joinUrl).toBe("https://desktop.example/session/fresh");
});

it("never forwards a body it could not scrub", async () => {
// `scrub` recurses once per level, so a deeply nested body throws
// RangeError while JSON.parse handles it without complaint. That gap is
Expand Down
Loading
Loading