From 2c5ec9ce103f2babd5088bceacde421b217b5530 Mon Sep 17 00:00:00 2001 From: devinfoley <139239+devinfoley@users.noreply.github.com> Date: Fri, 9 Oct 2026 05:02:17 +0000 Subject: [PATCH] fix(apps): harden Tailscale health reuse, settings merge, and key cleanup QA review of the Tailscale connector found five defects and a doc claim: - Catalog refresh reused a stored probe summary even after a later health check failed, reporting the connection healthy without contacting Tailscale. Reuse now requires healthStatus ok, a healthCheckedAt no older than the summary, and a probe fingerprint (tailnet, agent tag, secret versions) that still matches the connection. - The health check wrote the whole in-memory config back, restoring a tailnet or tag an operator saved during the probe. It now merges only config.tailscale with a jsonb merge fenced on methodConfig and credentialSecretRefs being unchanged, and reports tailscale_connection_changed (409, degraded) when the fence fails. - A create-key response with a usable id but invalid other fields skipped deletion; the key is now deleted before the shape error is thrown. - A cleanup failure after a tag validation failure was hidden behind the tag error; the cleanup failure is now primary and the superseded error is kept in details.secondary and appended to the message. - A response stream that failed mid-body threw a raw TypeError; it is now normalized to tailscale_request_failed. - The catalog and doc no longer claim agents get network reach from this connection. Tests cover each case in the API unit suite and the embedded-Postgres lifecycle suite. Co-Authored-By: Claude Fable 5.1 --- doc/connections/TAILSCALE.md | 50 +++++-- .../shared/src/app-definitions/tailscale.json | 6 +- packages/shared/src/tailscale.ts | 6 + scripts/ingest-app-definitions.mjs | 6 +- server/src/__tests__/tailscale-api.test.ts | 80 ++++++++++- .../__tests__/tailscale-connection.test.ts | 132 +++++++++++++++++- server/src/services/tailscale-api.ts | 100 +++++++++---- server/src/services/tool-access.ts | 84 +++++++++-- 8 files changed, 395 insertions(+), 69 deletions(-) diff --git a/doc/connections/TAILSCALE.md b/doc/connections/TAILSCALE.md index 0784c57e41..901f61693c 100644 --- a/doc/connections/TAILSCALE.md +++ b/doc/connections/TAILSCALE.md @@ -4,8 +4,9 @@ This connection holds a Tailscale OAuth client for one tailnet. Paperclip uses it server-side to list tailnet devices and to mint short-lived, single-use, tagged auth keys. Later features build on it: tailnet machines as `ssh` environments and sandboxes that join the tailnet for one run. In this version -agents get no tool from the connection. They get network reach, not control of -the tailnet. +the connection only verifies the OAuth client. Agents get no tool and no +tailnet access from it; machine and sandbox access are separate, later +features. Neither Paperclip ID nor Paperclip Connect participates. Cloud and self-hosted instances use the same server-side path. The instance needs outbound HTTPS to @@ -32,7 +33,7 @@ Fields: 1. Add the tags and grants below to the tailnet policy file in [Access controls](https://login.tailscale.com/admin/acls/file). Adjust the `tag:dev-box` and `tag:internal-api` examples to the machines and services - agents should reach. + that the later environment features will expose to agents. ```json { @@ -89,17 +90,37 @@ The check proves three things in order: the client is valid, it can read devices (`devices:core`), and it can mint keys with the agent tag (`auth_keys` plus tag ownership). The test key is reusable `false`, ephemeral `true`, preauthorized `true`, expiry 300 seconds, and it is deleted immediately. It is -deleted even when a later validation step fails. A failed deletion is reported -with its own code; the key still expires on its own within five minutes. +deleted even when a later validation step fails, and also when the create +response carries a usable key id but otherwise fails validation. A failed +deletion is reported as the primary error with its own code, because the +undeleted key is the operator's next concern; the validation failure it +superseded is kept in `details.secondary` and appended to the message. The key +still expires on its own within five minutes. A response stream that fails +mid-body is normalized to `tailscale_request_failed` like any other unusable +response. The redacted result is stored on the connection as `config.tailscale` -(`tailnet`, `scopes`, `tags`, `deviceCount`, `checkedAt`) and shown as the -health message, for example: +(`tailnet`, `scopes`, `tags`, `deviceCount`, `checkedAt`, `probeFingerprint`) +and shown as the health message, for example: ```text Tailscale OAuth client is connected to the client's tailnet. Scopes: auth_keys, devices:core. Tags: tag:paperclip-agent. Devices visible: 12. ``` +The summary is written as a JSON merge of the `tailscale` key only, fenced on +the connection's `methodConfig` and credential secret refs being the ones the +probe used. A settings save or credential change that lands during the probe +wins: the stale evidence is dropped and the check reports +`tailscale_connection_changed` so the operator runs it again on the new values. + +Setup runs the health check and catalog discovery back to back. Discovery +reuses the stored summary instead of minting a second test key only when the +summary is less than a minute old, the connection's latest health check +succeeded (`healthStatus` is `ok` and `healthCheckedAt` is not older than the +summary), and `probeFingerprint` still matches the current tailnet, agent tag, +and secret versions. A failed check, a new setting, or a rotated credential +forces a fresh probe. + Failures map to fixed codes. The message names the scope, tag, or tailnet to fix. Provider response bodies are discarded unread, because they can name tailnets, tags, and key identifiers. @@ -113,8 +134,9 @@ tailnets, tags, and key identifiers. | `tailscale_tailnet_not_found` | 422 | The tailnet name is wrong for this client. Use `-` or the exact name. | | `tailscale_rate_limited` | 502 | Tailscale returned 429. Retry in a minute. | | `tailscale_unreachable` | 502 | The instance could not reach `api.tailscale.com`. | -| `tailscale_test_key_cleanup_failed` | 502 | The test key was minted but not deleted. It expires within five minutes. | -| `tailscale_request_failed` | 502 | Another provider status or an unexpected response shape. | +| `tailscale_test_key_cleanup_failed` | 502 | The test key was minted but not deleted. It expires within five minutes. Any validation failure it superseded is in `details.secondary`. | +| `tailscale_request_failed` | 502 | Another provider status, an unexpected response shape, or a response stream that failed mid-body. | +| `tailscale_connection_changed` | 409 | The tailnet, agent tag, or credentials changed while the check ran. The saved values are kept; run the check again. | ## Boundaries @@ -136,13 +158,15 @@ Deterministic tests: - `server/src/__tests__/tailscale-api.test.ts`: the REST boundary against a fake Tailscale API. Token exchange shape, device listing, key mint and delete, every failure code, cached tokens, deletion of the test key after a later - failure, and redaction of the client secret, token, key secret, and provider - body. + failure or a malformed create response, cleanup failure reported first with + the superseded error as secondary, failed response streams, and redaction of + the client secret, token, key secret, and provider body. - `server/src/__tests__/tailscale-connection.test.ts`: the connection lifecycle through the real vault and setup path on embedded Postgres. Setup runs the check, stores the redacted summary, keeps secrets out of config and health - fields, maps a provider refusal to an actionable 422, and refuses credential - export. + fields, maps a provider refusal to an actionable 422, re-probes on refresh + after a failed check or a changed setting, keeps a settings save that lands + during a probe, and refuses credential export. - `packages/shared/src/app-definitions.test.ts`: catalog shape, fields, defaults, placement, and URL match. diff --git a/packages/shared/src/app-definitions/tailscale.json b/packages/shared/src/app-definitions/tailscale.json index 524f13ef04..cd39a830f5 100644 --- a/packages/shared/src/app-definitions/tailscale.json +++ b/packages/shared/src/app-definitions/tailscale.json @@ -2,7 +2,7 @@ "schemaVersion": 1, "slug": "tailscale", "name": "Tailscale", - "description": "Reach tailnet machines and private services from agent environments.", + "description": "Hold a Tailscale OAuth client so Paperclip can verify tailnet access and mint tagged auth keys server-side.", "categories": [ "developer" ], @@ -22,7 +22,7 @@ "ownershipModes": [ "customer" ], - "whenToUse": "Connect the tailnet whose machines and services agents should reach.", + "whenToUse": "Connect the tailnet that later Paperclip features will use for agent machines and sandboxes.", "defaults": { "serverUrl": "https://api.tailscale.com/api/v2" }, @@ -90,7 +90,7 @@ "docs": "https://tailscale.com/kb/1215/oauth-clients" }, "warnings": [ - "Agents get network reach through this connection; they do not get tools that change the tailnet in this version." + "This version only verifies the OAuth client. Agents get no tools and no tailnet access from this connection yet; machine and sandbox access are separate, later features." ] } ], diff --git a/packages/shared/src/tailscale.ts b/packages/shared/src/tailscale.ts index ddbb04074b..71362fb0c7 100644 --- a/packages/shared/src/tailscale.ts +++ b/packages/shared/src/tailscale.ts @@ -29,6 +29,7 @@ export const TAILSCALE_HEALTH_CODES = [ "tailscale_unreachable", "tailscale_test_key_cleanup_failed", "tailscale_request_failed", + "tailscale_connection_changed", ] as const; export type TailscaleHealthCode = (typeof TAILSCALE_HEALTH_CODES)[number]; @@ -41,6 +42,11 @@ export interface TailscaleConnectionHealth { tags: string[]; deviceCount: number; checkedAt: string; + /** + * Hash of the settings and vault secret versions the check used. The server + * reuses a recent summary only while this still matches the connection. + */ + probeFingerprint?: string; } export function tailscaleScopeCovers(scopes: readonly string[], accepted: readonly string[]): boolean { diff --git a/scripts/ingest-app-definitions.mjs b/scripts/ingest-app-definitions.mjs index 5f8014e6c9..af2f8d3c8b 100644 --- a/scripts/ingest-app-definitions.mjs +++ b/scripts/ingest-app-definitions.mjs @@ -922,11 +922,11 @@ const apps = [ }), { docsUrl: "https://docs.browser-use.com/cloud/api-v4-overview" }, ], - ["tailscale", "Tailscale", "Reach tailnet machines and private services from agent environments.", "developer", "tailscale.com", ["https://login.tailscale.com/*"], + ["tailscale", "Tailscale", "Hold a Tailscale OAuth client so Paperclip can verify tailnet access and mint tagged auth keys server-side.", "developer", "tailscale.com", ["https://login.tailscale.com/*"], method("oauth-client", "rest_api", "api_key", { serverUrl: "https://api.tailscale.com/api/v2" }, "S3", "Create an OAuth client in [Tailscale trust credentials](https://login.tailscale.com/admin/settings/oauth) with the `auth_keys` scope (tags `tag:paperclip-agent` and `tag:paperclip-control`) and the `devices:core` scope, then paste its client ID and secret. Paperclip verifies the client server-side by listing devices and minting one short-lived ephemeral test key that it deletes immediately. The client never leaves the server.", { label: "Tailscale OAuth client", - whenToUse: "Connect the tailnet whose machines and services agents should reach.", + whenToUse: "Connect the tailnet that later Paperclip features will use for agent machines and sandboxes.", grantKinds: ["organization"], credentialFields: [ { ...field("oauthClientId", "OAuth client ID", "k1234567CNTRL"), helperMd: "Copy the client ID shown after you create the OAuth client." }, @@ -941,7 +941,7 @@ const apps = [ { key: "agentTag", label: "Agent tag", type: "text", required: true, advanced: true, defaultValue: "tag:paperclip-agent", placeholder: "tag:paperclip-agent", helperMd: "Tag assigned to machines and sandboxes that join the tailnet for agents. The OAuth client must be allowed to assign it.", validation: { pattern: "^tag:[A-Za-z0-9_-]+$", maxLength: 128 } }, ], consoleLinks: { keys: "https://login.tailscale.com/admin/settings/oauth", docs: "https://tailscale.com/kb/1215/oauth-clients" }, - warnings: ["Agents get network reach through this connection; they do not get tools that change the tailnet in this version."], + warnings: ["This version only verifies the OAuth client. Agents get no tools and no tailnet access from this connection yet; machine and sandbox access are separate, later features."], }), { docsUrl: "https://tailscale.com/kb/1215/oauth-clients" }, ], diff --git a/server/src/__tests__/tailscale-api.test.ts b/server/src/__tests__/tailscale-api.test.ts index 4e18a94055..05bcb17f06 100644 --- a/server/src/__tests__/tailscale-api.test.ts +++ b/server/src/__tests__/tailscale-api.test.ts @@ -16,8 +16,21 @@ type Behavior = { keyTags?: string[]; keyBody?: unknown; unreachable?: boolean; + /** Return HTTP 200 for the device list with a body stream that fails mid-read. */ + devicesStreamFails?: boolean; }; +function failingStreamResponse() { + const encoder = new TextEncoder(); + const body = new ReadableStream({ + pull(controller) { + controller.enqueue(encoder.encode('{"devices":[')); + controller.error(new TypeError(`terminated: ${PROVIDER_BODY}`)); + }, + }); + return new Response(body, { status: 200, headers: { "Content-Type": "application/json" } }); +} + function fakeTailscale(behavior: Behavior = {}) { const calls: { method: string; path: string; headers: Headers; body: string | undefined }[] = []; const request = vi.fn(async (url: string, init: RequestInit) => { @@ -32,6 +45,7 @@ function fakeTailscale(behavior: Behavior = {}) { if (new Headers(init.headers).get("Authorization") !== `Bearer ${TOKEN}`) return Response.json({ message: PROVIDER_BODY }, { status: 401 }); if (path === "/tailnet/-/devices?fields=all" || path === "/tailnet/example.com/devices?fields=all") { if (behavior.devicesStatus) return Response.json({ message: PROVIDER_BODY }, { status: behavior.devicesStatus }); + if (behavior.devicesStreamFails) return failingStreamResponse(); return Response.json({ devices: [{ nodeId: "nFIXTURE1", name: "devbox.fixture.example.ts.net", addresses: ["100.64.0.7"], tags: ["tag:dev-box"], os: "linux" }, { nodeId: "nFIXTURE2", name: "laptop.fixture.example.ts.net", addresses: ["100.64.0.8"] }] }); } if ((path === "/tailnet/-/keys" || path === "/tailnet/example.com/keys") && init.method === "POST") { @@ -172,9 +186,58 @@ describe("Tailscale API boundary", () => { expect(error.message).not.toContain(TEST_KEY_SECRET); }); - it("keeps the first failure when both validation and cleanup fail", async () => { - const error = await failure(api({ keyTags: ["tag:other"], deleteStatus: 500 }).client.verify()); - expect(error).toMatchObject({ code: "tailscale_tag_not_owned" }); + it("reports a cleanup failure first and keeps the superseded validation failure as secondary information", async () => { + const error = await failure(api({ keyTags: ["tag:other"], deleteStatus: 500 }).client.verify({ agentTag: "tag:paperclip-agent" })); + expect(error).toMatchObject({ + status: 502, + code: "tailscale_test_key_cleanup_failed", + operation: "delete_key", + providerStatus: 500, + secondary: { code: "tailscale_tag_not_owned", operation: "create_key" }, + }); + expect(error.details).toMatchObject({ code: "tailscale_test_key_cleanup_failed", secondary: { code: "tailscale_tag_not_owned" } }); + expect(error.message).toContain("could not delete it"); + expect(error.message).toContain("Also: Tailscale issued the test key without tag:paperclip-agent"); + expect(error.message).not.toContain(PROVIDER_BODY); + expect(error.message).not.toContain(TEST_KEY_SECRET); + }); + + it("deletes a key whose create response carries a usable id but fails validation", async () => { + const { client, calls } = api({ keyBody: { id: "kTESTKEY1", key: TEST_KEY_SECRET, capabilities: "bogus" } }); + const error = await failure(client.verify()); + expect(error).toMatchObject({ status: 502, code: "tailscale_request_failed", operation: "create_key", secondary: null }); + expect(calls.map((call) => `${call.method} ${call.path}`)).toEqual([ + "POST /oauth/token", + "GET /tailnet/-/devices?fields=all", + "POST /tailnet/-/keys", + "DELETE /tailnet/-/keys/kTESTKEY1", + ]); + }); + + it("reports the cleanup failure when a malformed key response cannot be deleted either", async () => { + const { client, calls } = api({ keyBody: { id: "kTESTKEY1", capabilities: "bogus" }, deleteStatus: 500 }); + const error = await failure(client.verify()); + expect(error).toMatchObject({ + code: "tailscale_test_key_cleanup_failed", + secondary: { code: "tailscale_request_failed", operation: "create_key" }, + }); + expect(calls.map((call) => `${call.method} ${call.path}`)).toContain("DELETE /tailnet/-/keys/kTESTKEY1"); + }); + + it("skips deletion only when the malformed key response has no usable id", async () => { + const { client, calls } = api({ keyBody: { nope: true } }); + await failure(client.verify()); + expect(calls.filter((call) => call.method === "DELETE")).toHaveLength(0); + }); + + it("normalizes a response stream that fails mid-body into a fixed error", async () => { + const { client, calls } = api({ devicesStreamFails: true }); + const error = await failure(client.verify()); + expect(error).toMatchObject({ status: 502, code: "tailscale_request_failed", operation: "list_devices", providerStatus: null }); + expect(error.message).toBe("Tailscale ended the response early during list devices. Try again in a minute."); + expect(error.message).not.toContain(PROVIDER_BODY); + // Nothing was minted, so there is nothing to clean up. + expect(calls.filter((call) => call.method === "POST" && call.path.endsWith("/keys"))).toHaveLength(0); }); it("maps rate limits, unreachable hosts, and malformed bodies without echoing provider content", async () => { @@ -191,7 +254,16 @@ describe("Tailscale API boundary", () => { }); it("never puts the client secret, token, or provider body into an error", async () => { - for (const behavior of [{ tokenStatus: 401 }, { devicesStatus: 403 }, { createStatus: 400 }, { deleteStatus: 403 }, { unreachable: true }] satisfies Behavior[]) { + for (const behavior of [ + { tokenStatus: 401 }, + { devicesStatus: 403 }, + { createStatus: 400 }, + { deleteStatus: 403 }, + { unreachable: true }, + { devicesStreamFails: true }, + { keyTags: ["tag:other"], deleteStatus: 500 }, + { keyBody: { id: "kTESTKEY1", key: TEST_KEY_SECRET, capabilities: "bogus" }, deleteStatus: 500 }, + ] satisfies Behavior[]) { const error = await failure(api(behavior).client.verify()); const serialized = `${error.message} ${JSON.stringify(error.details)} ${error.stack ?? ""}`; expect(serialized).not.toContain(CLIENT_SECRET); diff --git a/server/src/__tests__/tailscale-connection.test.ts b/server/src/__tests__/tailscale-connection.test.ts index 7516034918..06ba8b245c 100644 --- a/server/src/__tests__/tailscale-connection.test.ts +++ b/server/src/__tests__/tailscale-connection.test.ts @@ -18,6 +18,8 @@ import { } from "./helpers/embedded-postgres.js"; const support = await getEmbeddedPostgresTestSupport(); +const asRecord = (value: unknown): Record => + value && typeof value === "object" && !Array.isArray(value) ? (value as Record) : {}; const actor = { actorType: "user" as const, actorId: "tailscale-reviewer" }; const CLIENT_ID = "kFIXTURECNTRL"; const CLIENT_SECRET = "tskey-client-fixture-secret-value"; @@ -49,7 +51,13 @@ const SECRETS = [CLIENT_SECRET, TOKEN, TEST_KEY_SECRET, PROVIDER_BODY]; membershipRole: "admin", status: "active", }); - const behavior = { tokenStatus: 0, createStatus: 0, scope: "auth_keys devices:core" }; + const behavior: { + tokenStatus: number; + createStatus: number; + scope: string; + /** When set, the device list waits on this promise before it answers. */ + devicesGate: Promise | null; + } = { tokenStatus: 0, createStatus: 0, scope: "auth_keys devices:core", devicesGate: null }; const calls: { method: string; path: string; body: string | undefined }[] = []; const request = vi.fn(async (url: string, init: RequestInit) => { const parsed = new URL(url); @@ -67,6 +75,7 @@ const SECRETS = [CLIENT_SECRET, TOKEN, TEST_KEY_SECRET, PROVIDER_BODY]; return Response.json({ message: PROVIDER_BODY }, { status: 401 }); } if (/^\/tailnet\/[^/]+\/devices\?fields=all$/.test(path)) { + if (behavior.devicesGate) await behavior.devicesGate; return Response.json({ devices: [{ nodeId: "nFIXTURE1", name: "devbox.fixture.example.ts.net", addresses: ["100.64.0.7"] }] }); } if (/^\/tailnet\/[^/]+\/keys$/.test(path) && init.method === "POST") { @@ -89,6 +98,36 @@ const SECRETS = [CLIENT_SECRET, TOKEN, TEST_KEY_SECRET, PROVIDER_BODY]; return { company, access, behavior, calls, request }; } + const PROBE = [ + "POST /oauth/token", + "GET /tailnet/-/devices?fields=all", + "POST /tailnet/-/keys", + "DELETE /tailnet/-/keys/kTESTKEY1", + ]; + + async function connect(fx: Awaited>, configValues?: Record) { + const connected = await fx.access.connectGalleryApp( + fx.company.id, + { + galleryKey: "tailscale", + connectionMethodKey: "oauth-client", + ...(configValues ? { configValues } : {}), + credentialValues: { + "credentials.oauthClientId": CLIENT_ID, + "credentials.oauthClientSecret": CLIENT_SECRET, + }, + }, + actor, + ); + fx.calls.length = 0; + return connected; + } + + async function row(connectionId: string) { + const [found] = await db.select().from(toolConnections).where(eq(toolConnections.id, connectionId)); + return found; + } + function expectRedacted(value: unknown) { const text = JSON.stringify(value) ?? ""; for (const secret of SECRETS) expect(text).not.toContain(secret); @@ -225,6 +264,97 @@ const SECRETS = [CLIENT_SECRET, TOKEN, TEST_KEY_SECRET, PROVIDER_BODY]; expectRedacted(row); }); + it("reuses the setup probe for an immediate refresh, but probes again once a check has failed", async () => { + const fx = await fixture(); + const connected = await connect(fx); + const stored = await row(connected.connectionId); + expect(stored.healthStatus).toBe("ok"); + expect(asRecord(stored.config.tailscale).probeFingerprint).toMatch(/^[0-9a-f]{32}$/); + + // Unchanged connection whose latest check succeeded: no second test key. + const reused = await fx.access.refreshCatalog(connected.connectionId, actor); + expect(fx.calls).toEqual([]); + expect(reused.connection.healthStatus).toBe("ok"); + expect(reused.connection.healthMessage).toContain("Tailscale OAuth client is connected"); + + // A failed check must not be hidden by a refresh inside the reuse window. + fx.behavior.tokenStatus = 401; + await expect(fx.access.checkHealth(connected.connectionId, actor)).rejects.toMatchObject({ status: 422, details: { code: "tailscale_client_invalid" } }); + expect((await row(connected.connectionId)).healthStatus).toBe("error"); + fx.calls.length = 0; + await expect(fx.access.refreshCatalog(connected.connectionId, actor)).rejects.toMatchObject({ status: 422, details: { code: "tailscale_client_invalid" } }); + expect(fx.calls.map((call) => `${call.method} ${call.path}`)).toEqual(["POST /oauth/token"]); + const failed = await row(connected.connectionId); + expect(failed.healthStatus).toBe("error"); + expect(failed.healthMessage).toContain("rejected the OAuth client"); + + // Once the provider accepts the client again, the refresh revalidates in full. + fx.behavior.tokenStatus = 0; + fx.calls.length = 0; + const recovered = await fx.access.refreshCatalog(connected.connectionId, actor); + expect(fx.calls.map((call) => `${call.method} ${call.path}`)).toEqual(PROBE); + expect(recovered.connection.healthStatus).toBe("ok"); + }); + + it("probes again on refresh when the tailnet or agent tag changed since the last check", async () => { + const fx = await fixture(); + const connected = await connect(fx); + const stored = await row(connected.connectionId); + await fx.access.updateConnection(connected.connectionId, { + config: { ...stored.config, methodConfig: { tailnet: "example.com", agentTag: "tag:paperclip-lab" } }, + }); + expect((await row(connected.connectionId)).healthStatus).toBe("ok"); + const refreshed = await fx.access.refreshCatalog(connected.connectionId, actor); + expect(fx.calls.map((call) => `${call.method} ${call.path}`)).toEqual([ + "POST /oauth/token", + "GET /tailnet/example.com/devices?fields=all", + "POST /tailnet/example.com/keys", + "DELETE /tailnet/example.com/keys/kTESTKEY1", + ]); + expect(JSON.parse(fx.calls[2]!.body!).capabilities.devices.create.tags).toEqual(["tag:paperclip-lab"]); + expect(refreshed.connection.healthMessage).toContain("tailnet example.com"); + expect(refreshed.connection.healthMessage).toContain("Tags: tag:paperclip-lab"); + expect((await row(connected.connectionId)).config.tailscale).toMatchObject({ tailnet: "example.com", tags: ["tag:paperclip-lab"] }); + }); + + it("keeps a settings save that lands during a probe and discards the stale evidence", async () => { + const fx = await fixture(); + const connected = await connect(fx); + const before = await row(connected.connectionId); + const previousSummary = before.config.tailscale; + + let release!: () => void; + fx.behavior.devicesGate = new Promise((resolve) => { release = resolve; }); + const pendingCheck = fx.access.checkHealth(connected.connectionId, actor).then( + () => null, + (error: unknown) => error as { status: number; message: string; details: Record }, + ); + await vi.waitFor(() => expect(fx.calls.map((call) => `${call.method} ${call.path}`)).toContain("GET /tailnet/-/devices?fields=all")); + + // The operator saves a new tailnet while the device list is still pending. + await fx.access.updateConnection(connected.connectionId, { + config: { ...before.config, methodConfig: { tailnet: "example.com", agentTag: "tag:paperclip-agent" } }, + }); + fx.behavior.devicesGate = null; + release(); + + const failure = await pendingCheck; + expect(failure).toMatchObject({ status: 409, details: { code: "tailscale_connection_changed" } }); + const after = await row(connected.connectionId); + expect(asRecord(after.config.methodConfig).tailnet).toBe("example.com"); + expect(after.config.tailscale).toEqual(previousSummary); + expect(after.healthStatus).toBe("degraded"); + expect(after.healthMessage).toContain("Run the check again"); + expectRedacted(after); + + // The next check runs on the saved values and replaces the summary. + fx.calls.length = 0; + const checked = await fx.access.checkHealth(connected.connectionId, actor); + expect(checked.connection.healthStatus).toBe("ok"); + expect(fx.calls.map((call) => `${call.method} ${call.path}`)).toContain("GET /tailnet/example.com/devices?fields=all"); + expect((await row(connected.connectionId)).config.tailscale).toMatchObject({ tailnet: "example.com" }); + }); + it("refuses to export the OAuth client to an agent", async () => { const { company, access } = await fixture(); const connected = await access.connectGalleryApp( diff --git a/server/src/services/tailscale-api.ts b/server/src/services/tailscale-api.ts index 9adc8ec01f..79fa4cb141 100644 --- a/server/src/services/tailscale-api.ts +++ b/server/src/services/tailscale-api.ts @@ -33,8 +33,23 @@ export class TailscaleApiError extends HttpError { readonly code: TailscaleHealthCode, readonly operation: TailscaleOperation, readonly providerStatus: number | null = null, + /** An earlier failure that this error supersedes, kept as secondary information. */ + readonly secondary: { code: TailscaleHealthCode; operation: TailscaleOperation; message: string } | null = null, ) { - super(status, message, { code, operation, providerStatus }); + super(status, message, { code, operation, providerStatus, ...(secondary ? { secondary } : {}) }); + } + + /** Return this error carrying `earlier` as secondary information, with its message appended. */ + withSecondary(earlier: unknown): TailscaleApiError { + if (!(earlier instanceof TailscaleApiError)) return this; + return new TailscaleApiError( + this.status, + `${this.message} Also: ${earlier.message}`, + this.code, + this.operation, + this.providerStatus, + { code: earlier.code, operation: earlier.operation, message: earlier.message }, + ); } } @@ -85,6 +100,8 @@ const keySchema = z.object({ }) .optional(), }); +/** The minimum needed to delete a key whose response otherwise failed validation. */ +const keyIdSchema = z.object({ id: z.string().min(1) }); export interface TailscaleCreateAuthKeyInput { tags: string[]; @@ -162,14 +179,29 @@ function malformed(operation: TailscaleOperation): TailscaleApiError { return new TailscaleApiError(502, `Tailscale returned an unexpected response during ${operation.replace(/_/g, " ")}.`, "tailscale_request_failed", operation); } +function truncated(operation: TailscaleOperation): TailscaleApiError { + return new TailscaleApiError(502, `Tailscale ended the response early during ${operation.replace(/_/g, " ")}. Try again in a minute.`, "tailscale_request_failed", operation); +} + async function readJson(response: Response, operation: TailscaleOperation): Promise { if (response.status === 204 || !response.body) return null; - const reader = response.body.getReader(); + let reader: ReadableStreamDefaultReader; + try { + reader = response.body.getReader(); + } catch { + throw truncated(operation); + } const parts: Uint8Array[] = []; let bytes = 0; try { for (;;) { - const part = await reader.read(); + let part: ReadableStreamReadResult; + try { + part = await reader.read(); + } catch { + // A stream that fails mid-body may carry a provider message; normalize it. + throw truncated(operation); + } if (part.done) break; bytes += part.value.length; if (bytes > MAX_RESPONSE_BYTES) throw malformed(operation); @@ -266,23 +298,34 @@ export function tailscaleApi(options: TailscaleApiOptions) { return parsed.data.devices; }, async createAuthKey(input: TailscaleCreateAuthKeyInput): Promise { - const parsed = keySchema.safeParse( - await authorized("create_key", `${tailnetPath}/keys`, "POST", { - capabilities: { - devices: { - create: { - reusable: input.reusable, - ephemeral: input.ephemeral, - preauthorized: input.preauthorized, - tags: input.tags, - }, + const raw = await authorized("create_key", `${tailnetPath}/keys`, "POST", { + capabilities: { + devices: { + create: { + reusable: input.reusable, + ephemeral: input.ephemeral, + preauthorized: input.preauthorized, + tags: input.tags, }, }, - expirySeconds: input.expirySeconds, - description: input.description, - }, input.tags[0]), - ); - if (!parsed.success) throw malformed("create_key"); + }, + expirySeconds: input.expirySeconds, + description: input.description, + }, input.tags[0]); + const parsed = keySchema.safeParse(raw); + if (!parsed.success) { + // The key may exist even when the rest of the response is unusable. + // Delete it when the response carries a usable id, then report the shape. + const id = keyIdSchema.safeParse(raw); + if (id.success) { + try { + await api.deleteKey(id.data.id); + } catch (cleanup) { + throw cleanup instanceof TailscaleApiError ? cleanup.withSecondary(malformed("create_key")) : cleanup; + } + } + throw malformed("create_key"); + } return { id: parsed.data.id, key: parsed.data.key ?? null, @@ -300,7 +343,9 @@ export function tailscaleApi(options: TailscaleApiOptions) { * Connection check: token exchange, device list (proves devices:core), then * mint and immediately delete a one-off ephemeral tagged key (proves * auth_keys and tag ownership). The test key is deleted even when a later - * validation step fails; a cleanup failure is reported as its own code. + * validation step fails. A cleanup failure is reported as the primary error + * because an undeleted key is the operator's next concern; the validation + * failure it superseded is kept as secondary information. */ async verify(input: { agentTag?: string } = {}): Promise { const agentTag = input.agentTag?.trim() || TAILSCALE_DEFAULT_AGENT_TAG; @@ -318,20 +363,15 @@ export function tailscaleApi(options: TailscaleApiOptions) { expirySeconds: TAILSCALE_TEST_KEY_EXPIRY_SECONDS, description: "Paperclip connection check (deleted immediately)", }); - let pending: unknown = null; - try { - if (key.tags.length && !key.tags.includes(agentTag)) { - throw new TailscaleApiError(422, `Tailscale issued the test key without ${agentTag}. Add that tag to the OAuth client's auth_keys scope and reconnect.`, "tailscale_tag_not_owned", "create_key"); - } - } catch (error) { - pending = error; - } + const validation: TailscaleApiError | null = key.tags.length && !key.tags.includes(agentTag) + ? new TailscaleApiError(422, `Tailscale issued the test key without ${agentTag}. Add that tag to the OAuth client's auth_keys scope and reconnect.`, "tailscale_tag_not_owned", "create_key") + : null; try { await api.deleteKey(key.id); - } catch (error) { - pending ??= error; + } catch (cleanup) { + throw cleanup instanceof TailscaleApiError ? cleanup.withSecondary(validation) : cleanup; } - if (pending) throw pending; + if (validation) throw validation; return { tailnet, scopes, diff --git a/server/src/services/tool-access.ts b/server/src/services/tool-access.ts index ce63f15c3d..78da9315d5 100644 --- a/server/src/services/tool-access.ts +++ b/server/src/services/tool-access.ts @@ -2895,6 +2895,7 @@ function healthFailureHttpStatus(failure: { if (failure.code === "tool_connection_transport_unsupported") return 422; if (failure.code === "cognee_access_unverified" || failure.code === "memory_api_key_rejected") return 422; if (failure.code.endsWith("_endpoint_rejected")) return 422; + if (failure.code === "tailscale_connection_changed") return 409; if (isActionableTailscaleCode(failure.code)) return 422; return 502; } @@ -2908,9 +2909,45 @@ function isTailscaleHealth(value: unknown): value is TailscaleConnectionHealth { && typeof record.deviceCount === "number" && typeof record.checkedAt === "string"; } +/** The settings and vault secret versions a Tailscale check depends on. */ +function tailscaleProbeInputs(connection: typeof toolConnections.$inferSelect) { + const methodConfig = asRecord(asRecord(connection.config).methodConfig); + const tailnet = typeof methodConfig.tailnet === "string" && methodConfig.tailnet.trim() ? methodConfig.tailnet.trim() : TAILSCALE_DEFAULT_TAILNET; + const agentTag = typeof methodConfig.agentTag === "string" && methodConfig.agentTag.trim() ? methodConfig.agentTag.trim() : TAILSCALE_DEFAULT_AGENT_TAG; + const refs = connection.credentialSecretRefs + .map((ref) => [ref.configPath, ref.secretId, ref.versionSelector ?? "latest"] as const) + .sort((a, b) => a[0].localeCompare(b[0])); + const fingerprint = createHash("sha256").update(JSON.stringify({ tailnet, agentTag, refs })).digest("hex").slice(0, 32); + return { methodConfig, tailnet, agentTag, fingerprint }; +} + +/** + * A stored Tailscale summary stands in for a new probe only when it is recent, + * the connection's latest health check succeeded, and the settings and + * credentials it was computed from are unchanged. A failed check after a good + * probe, a new tailnet or tag, or rotated credentials all force a fresh probe. + */ +function tailscaleProbeIsReusable(connection: typeof toolConnections.$inferSelect): boolean { + if (connection.healthStatus !== "ok") return false; + const recent = asRecord(connection.config).tailscale; + if (!isTailscaleHealth(recent) || typeof recent.probeFingerprint !== "string") return false; + const checkedAt = Date.parse(recent.checkedAt); + if (!Number.isFinite(checkedAt) || Date.now() - checkedAt >= TAILSCALE_PROBE_REUSE_MS) return false; + const lastHealthAt = connection.healthCheckedAt?.getTime() ?? Number.NaN; + // The summary must come from the check that produced the current "ok" status. + if (!Number.isFinite(lastHealthAt) || lastHealthAt < checkedAt) return false; + return recent.probeFingerprint === tailscaleProbeInputs(connection).fingerprint; +} + /** Tailscale codes the operator fixes in the provider console, not transient provider failures. */ function isActionableTailscaleCode(code: string): boolean { - return code.startsWith("tailscale_") && !["tailscale_unreachable", "tailscale_rate_limited", "tailscale_request_failed", "tailscale_test_key_cleanup_failed"].includes(code); + return code.startsWith("tailscale_") && ![ + "tailscale_unreachable", + "tailscale_rate_limited", + "tailscale_request_failed", + "tailscale_test_key_cleanup_failed", + "tailscale_connection_changed", + ].includes(code); } function unsupportedToolConnectionTransport() { @@ -7498,24 +7535,41 @@ export function toolAccessService( actorId: null, }); }; - const methodConfig = asRecord(asRecord(connection.config).methodConfig); - const tailnet = typeof methodConfig.tailnet === "string" && methodConfig.tailnet.trim() ? methodConfig.tailnet.trim() : TAILSCALE_DEFAULT_TAILNET; - const agentTag = typeof methodConfig.agentTag === "string" && methodConfig.agentTag.trim() ? methodConfig.agentTag.trim() : TAILSCALE_DEFAULT_AGENT_TAG; + const { methodConfig, tailnet, agentTag, fingerprint } = tailscaleProbeInputs(connection); const [clientId, clientSecret] = await Promise.all([ resolve("credentials.oauthClientId"), resolve("credentials.oauthClientSecret"), ]); - const health = await tailscaleApi({ + const verified = await tailscaleApi({ clientId, clientSecret, tailnet, request: (url, init) => requestRemoteHttpEndpoint(new URL(url), init), }).verify({ agentTag }); - await db + const health: TailscaleConnectionHealth = { ...verified, probeFingerprint: fingerprint }; + // Merge only the summary into the stored config, and only while the + // settings and credentials the probe used are still the ones on the row. + // A save that lands during the probe wins; the stale evidence is dropped. + const [stored] = await db .update(toolConnections) - .set({ config: { ...connection.config, tailscale: health }, updatedAt: new Date() }) - .where(eq(toolConnections.id, connection.id)); - connection.config = { ...connection.config, tailscale: health }; + .set({ + config: sql`${toolConnections.config} || ${JSON.stringify({ tailscale: health })}::jsonb`, + updatedAt: new Date(), + }) + .where( + and( + eq(toolConnections.id, connection.id), + sql`coalesce(${toolConnections.config} -> 'methodConfig', '{}'::jsonb) = ${JSON.stringify(methodConfig)}::jsonb`, + sql`${toolConnections.credentialSecretRefs} = ${JSON.stringify(connection.credentialSecretRefs)}::jsonb`, + ), + ) + .returning({ config: toolConnections.config }); + if (!stored) { + throw conflict("Tailscale settings or credentials changed while the check was running. Run the check again.", { + code: "tailscale_connection_changed", + }); + } + connection.config = stored.config; return health; } @@ -7542,12 +7596,12 @@ export function toolAccessService( return []; } if (isTailscaleConnection(connection)) { - // Network reach only: no agent-facing tools in this version. Setup runs - // the health probe and catalog discovery back to back; reuse a summary - // from the last minute so one setup mints one test key, not two. - const recent = asRecord(connection.config).tailscale; - const checkedAt = isTailscaleHealth(recent) ? Date.parse(recent.checkedAt) : Number.NaN; - if (!(Number.isFinite(checkedAt) && Date.now() - checkedAt < TAILSCALE_PROBE_REUSE_MS)) { + // No agent-facing tools in this version. Setup runs the health probe and + // catalog discovery back to back; reuse the summary of a check that just + // succeeded on these exact settings so one setup mints one test key, not + // two. Anything else (a failed check, new settings, rotated credentials) + // probes again. + if (!tailscaleProbeIsReusable(connection)) { await validateTailscaleConnection(connection); } return [];