diff --git a/src/__tests__/remote-connection.test.ts b/src/__tests__/remote-connection.test.ts index 8ba8895b3..f43370dd4 100644 --- a/src/__tests__/remote-connection.test.ts +++ b/src/__tests__/remote-connection.test.ts @@ -37,6 +37,7 @@ import type { AgentDeviceClient } from '../agent-device-client.ts'; afterEach(() => { vi.clearAllMocks(); vi.restoreAllMocks(); + vi.unstubAllEnvs(); }); const unexpectedCommandCall = async (): Promise => { @@ -233,7 +234,6 @@ test('connect proxy writes normal remote state with generated non-secret profile assert.equal(state.leaseId, undefined); assert.deepEqual(state.daemon, { baseUrl: 'http://proxy.example.test/agent-device', - authToken: 'proxy-secret', transport: 'http', }); assert.match(state.remoteConfigPath, /remote-connections\/generated\/proxy-[a-f0-9]{16}\.json$/); @@ -277,7 +277,6 @@ test('connect daemon-base-url shortcut uses proxy profile for direct proxy URLs' assert.match(state.clientId ?? '', /^[a-f0-9]{16}$/); assert.deepEqual(state.daemon, { baseUrl: 'http://127.0.0.1:4310/agent-device', - authToken: 'proxy-secret', transport: 'http', }); assert.equal(state.leaseId, undefined); @@ -1883,7 +1882,16 @@ test('connect --force stops replaced Metro companion after state is updated', as const stateDir = path.join(tempRoot, '.state'); const oldRemoteConfigPath = path.join(tempRoot, 'old-remote.json'); const newRemoteConfigPath = path.join(tempRoot, 'new-remote.json'); - fs.writeFileSync(oldRemoteConfigPath, JSON.stringify({ daemonBaseUrl: 'https://old.example' })); + fs.writeFileSync( + oldRemoteConfigPath, + JSON.stringify({ + daemonBaseUrl: 'https://old.example', + // Recoverable from the previous connection's own profile (plan 007 + // rule 1) so the forced release authenticates against old.example with + // its own credential, not the new connection's. + daemonAuthToken: 'test-old-not-a-real-token', + }), + ); fs.writeFileSync(newRemoteConfigPath, JSON.stringify({ daemonBaseUrl: 'https://new.example' })); writeRemoteConnectionState({ stateDir, @@ -1922,6 +1930,7 @@ test('connect --force stops replaced Metro companion after state is updated', as stateDir, remoteConfig: newRemoteConfigPath, daemonBaseUrl: 'https://new.example', + daemonAuthToken: 'test-new-not-a-real-token', tenant: 'acme', runId: 'run-new', session: 'adc-android', @@ -1944,6 +1953,7 @@ test('connect --force stops replaced Metro companion after state is updated', as assert.equal(releaseRequest?.leaseId, 'lease-old'); assert.equal(releaseRequest?.daemonBaseUrl, 'https://old.example'); assert.equal(releaseRequest?.daemonTransport, 'http'); + assert.equal(releaseRequest?.daemonAuthToken, 'test-old-not-a-real-token'); assert.equal(readRemoteConnectionState({ stateDir, session: 'adc-android' })?.runId, 'run-new'); fs.rmSync(tempRoot, { recursive: true, force: true }); }); @@ -1953,7 +1963,13 @@ test('connect --force without a session replaces the active generated connection const stateDir = path.join(tempRoot, '.state'); const oldRemoteConfigPath = path.join(tempRoot, 'old-remote.json'); const newRemoteConfigPath = path.join(tempRoot, 'new-remote.json'); - fs.writeFileSync(oldRemoteConfigPath, JSON.stringify({ daemonBaseUrl: 'https://old.example' })); + fs.writeFileSync( + oldRemoteConfigPath, + JSON.stringify({ + daemonBaseUrl: 'https://old.example', + daemonAuthToken: 'test-old-not-a-real-token', + }), + ); fs.writeFileSync(newRemoteConfigPath, JSON.stringify({ daemonBaseUrl: 'https://new.example' })); writeRemoteConnectionState({ stateDir, @@ -1992,6 +2008,7 @@ test('connect --force without a session replaces the active generated connection stateDir, remoteConfig: newRemoteConfigPath, daemonBaseUrl: 'https://new.example', + daemonAuthToken: 'test-new-not-a-real-token', tenant: 'acme', runId: 'run-new', platform: 'android', @@ -2014,6 +2031,7 @@ test('connect --force without a session replaces the active generated connection assert.equal(activeState?.runId, 'run-new'); assert.equal(activeState?.remoteConfigPath, newRemoteConfigPath); assert.equal(releaseRequest?.leaseId, 'lease-old'); + assert.equal(releaseRequest?.daemonAuthToken, 'test-old-not-a-real-token'); assert.deepEqual(vi.mocked(stopMetroCompanion).mock.calls[0]?.[0], { projectRoot: '/tmp/old-project', profileKey: oldRemoteConfigPath, @@ -2024,6 +2042,475 @@ test('connect --force without a session replaces the active generated connection fs.rmSync(tempRoot, { recursive: true, force: true }); }); +test("connect --force releases the previous lease with the previous connection's own token, not the new one", async () => { + const tempRoot = mkdtempForTestSync('agent-device-connect-force-prev-token-'); + const stateDir = path.join(tempRoot, '.state'); + const oldRemoteConfigPath = path.join(tempRoot, 'old-remote.json'); + const newRemoteConfigPath = path.join(tempRoot, 'new-remote.json'); + fs.writeFileSync( + oldRemoteConfigPath, + JSON.stringify({ + daemonBaseUrl: 'https://old.example', + // Token A: belongs to the previous (old) connection's own profile. + daemonAuthToken: 'test-old-not-a-real-token', + }), + ); + fs.writeFileSync(newRemoteConfigPath, JSON.stringify({ daemonBaseUrl: 'https://new.example' })); + writeRemoteConnectionState({ + stateDir, + state: { + version: 1, + session: 'adc-android', + remoteConfigPath: oldRemoteConfigPath, + remoteConfigHash: hashRemoteConfigFile(oldRemoteConfigPath), + tenant: 'acme', + runId: 'run-old', + leaseId: 'lease-old', + leaseBackend: 'android-instance', + daemon: { baseUrl: 'https://old.example' }, + connectedAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + }, + }); + let releaseRequest: Parameters[0] | undefined; + + await captureStdout(async () => { + await connectCommand({ + positionals: [], + flags: { + json: true, + help: false, + version: false, + force: true, + stateDir, + remoteConfig: newRemoteConfigPath, + daemonBaseUrl: 'https://new.example', + // Token B: the new connection's credential; must never reach old.example. + daemonAuthToken: 'test-new-not-a-real-token', + tenant: 'acme', + runId: 'run-new', + session: 'adc-android', + platform: 'android', + }, + client: createTestClient({ + release: async (request) => { + releaseRequest = request; + return { released: true }; + }, + }), + }); + }); + + assert.equal(releaseRequest?.leaseId, 'lease-old'); + assert.equal(releaseRequest?.daemonBaseUrl, 'https://old.example'); + assert.equal(releaseRequest?.daemonAuthToken, 'test-old-not-a-real-token'); + assert.notEqual(releaseRequest?.daemonAuthToken, 'test-new-not-a-real-token'); + fs.rmSync(tempRoot, { recursive: true, force: true }); +}); + +test('connect --force skips releasing the previous lease when its token cannot be recovered and the endpoint differs', async () => { + const tempRoot = mkdtempForTestSync('agent-device-connect-force-unreleasable-'); + const stateDir = path.join(tempRoot, '.state'); + const oldRemoteConfigPath = path.join(tempRoot, 'old-remote.json'); + const newRemoteConfigPath = path.join(tempRoot, 'new-remote.json'); + // No daemonAuthToken in the previous connection's own profile: its + // credential cannot be recovered, and the new endpoint differs. + fs.writeFileSync(oldRemoteConfigPath, JSON.stringify({ daemonBaseUrl: 'https://old.example' })); + fs.writeFileSync(newRemoteConfigPath, JSON.stringify({ daemonBaseUrl: 'https://new.example' })); + writeRemoteConnectionState({ + stateDir, + state: { + version: 1, + session: 'adc-android', + remoteConfigPath: oldRemoteConfigPath, + remoteConfigHash: hashRemoteConfigFile(oldRemoteConfigPath), + tenant: 'acme', + runId: 'run-old', + leaseId: 'lease-old', + leaseBackend: 'android-instance', + daemon: { baseUrl: 'https://old.example' }, + connectedAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + }, + }); + let releaseCalled = false; + + const stdout = await captureStdout(async () => { + await connectCommand({ + positionals: [], + flags: { + json: false, + help: false, + version: false, + force: true, + stateDir, + remoteConfig: newRemoteConfigPath, + daemonBaseUrl: 'https://new.example', + daemonAuthToken: 'test-new-not-a-real-token', + tenant: 'acme', + runId: 'run-new', + session: 'adc-android', + platform: 'android', + }, + client: createTestClient({ + release: async () => { + releaseCalled = true; + return { released: true }; + }, + }), + }); + }); + + assert.equal(releaseCalled, false); + assert.match(stdout, /Could not release the previous lease lease-old/); + assert.match(stdout, /tenant acme, run run-old/); + assert.match(stdout, /old\.example/); + // Reconnect still succeeds despite the orphaned previous lease. + assert.equal(readRemoteConnectionState({ stateDir, session: 'adc-android' })?.runId, 'run-new'); + fs.rmSync(tempRoot, { recursive: true, force: true }); +}); + +test('connect --force does not misclassify an env-sourced new token as the previous connection’s own credential', async () => { + const tempRoot = mkdtempForTestSync('agent-device-connect-force-env-token-'); + const stateDir = path.join(tempRoot, '.state'); + const oldRemoteConfigPath = path.join(tempRoot, 'old-remote.json'); + const newRemoteConfigPath = path.join(tempRoot, 'new-remote.json'); + // Neither config file declares daemonAuthToken; the only source of a token + // anywhere is the environment, which is global and belongs to the *new* + // connection, not provably to old.example. + fs.writeFileSync(oldRemoteConfigPath, JSON.stringify({ daemonBaseUrl: 'https://old.example' })); + fs.writeFileSync(newRemoteConfigPath, JSON.stringify({ daemonBaseUrl: 'https://new.example' })); + // Token B, supplied through the environment — not via --daemon-auth-token — + // which is precisely the gap a merged-profile read would miss. + vi.stubEnv('AGENT_DEVICE_DAEMON_AUTH_TOKEN', 'test-env-not-a-real-token'); + writeRemoteConnectionState({ + stateDir, + state: { + version: 1, + session: 'adc-android', + remoteConfigPath: oldRemoteConfigPath, + remoteConfigHash: hashRemoteConfigFile(oldRemoteConfigPath), + tenant: 'acme', + runId: 'run-old', + leaseId: 'lease-old', + leaseBackend: 'android-instance', + daemon: { baseUrl: 'https://old.example' }, + connectedAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + }, + }); + let releaseRequest: Parameters[0] | undefined; + + const stdout = await captureStdout(async () => { + await connectCommand({ + positionals: [], + flags: { + json: false, + help: false, + version: false, + force: true, + stateDir, + remoteConfig: newRemoteConfigPath, + daemonBaseUrl: 'https://new.example', + // No daemonAuthToken flag: the ambient token below flows in purely + // through the environment, matching production's resolution chain. + tenant: 'acme', + runId: 'run-new', + session: 'adc-android', + platform: 'android', + }, + client: createTestClient({ + release: async (request) => { + releaseRequest = request; + return { released: true }; + }, + }), + }); + }); + + assert.equal(releaseRequest, undefined); + assert.match(stdout, /Could not release the previous lease lease-old/); + assert.match(stdout, /tenant acme, run run-old/); + assert.match(stdout, /old\.example/); + assert.equal(readRemoteConnectionState({ stateDir, session: 'adc-android' })?.runId, 'run-new'); + fs.rmSync(tempRoot, { recursive: true, force: true }); +}); + +test('connect --force does not treat a re-pointed config path’s token as the previous endpoint’s own', async () => { + const tempRoot = mkdtempForTestSync('agent-device-connect-force-repointed-path-'); + const stateDir = path.join(tempRoot, '.state'); + // ONE path, reused. The previous connection was made against old.example + // through this file; the file is then edited in place to describe a + // different endpoint with a different credential. Distinct old/new paths + // cannot express this: the leak is that `remoteConfigPath` still resolves, + // and still parses, while no longer describing the connection it is being + // consulted about. + const remoteConfigPath = path.join(tempRoot, 'remote.json'); + fs.writeFileSync( + remoteConfigPath, + JSON.stringify({ + daemonBaseUrl: 'https://old.example', + daemonAuthToken: 'test-old-not-a-real-token', + }), + ); + writeRemoteConnectionState({ + stateDir, + state: { + version: 1, + session: 'adc-android', + remoteConfigPath, + // Recorded while the file still described old.example — the fact that + // makes the later edit detectable. + remoteConfigHash: hashRemoteConfigFile(remoteConfigPath), + tenant: 'acme', + runId: 'run-old', + leaseId: 'lease-old', + leaseBackend: 'android-instance', + daemon: { baseUrl: 'https://old.example' }, + connectedAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + }, + }); + // The edit: same path, now endpoint B with token B. + fs.writeFileSync( + remoteConfigPath, + JSON.stringify({ + daemonBaseUrl: 'https://new.example', + daemonAuthToken: 'test-new-not-a-real-token', + }), + ); + let releaseRequest: Parameters[0] | undefined; + + const stdout = await captureStdout(async () => { + await connectCommand({ + positionals: [], + flags: { + json: false, + help: false, + version: false, + force: true, + stateDir, + remoteConfig: remoteConfigPath, + daemonBaseUrl: 'https://new.example', + tenant: 'acme', + runId: 'run-new', + session: 'adc-android', + platform: 'android', + }, + client: createTestClient({ + release: async (request) => { + releaseRequest = request; + return { released: true }; + }, + }), + }); + }); + + // No request at all — not merely a request carrying a different token. + assert.equal(releaseRequest, undefined); + assert.match(stdout, /Could not release the previous lease lease-old/); + assert.match(stdout, /old\.example/); + assert.equal(readRemoteConnectionState({ stateDir, session: 'adc-android' })?.runId, 'run-new'); + fs.rmSync(tempRoot, { recursive: true, force: true }); +}); + +test('connect --force does not trust an unchanged profile token for a CLI-overridden previous endpoint', async () => { + const tempRoot = mkdtempForTestSync('agent-device-connect-force-cli-override-'); + const stateDir = path.join(tempRoot, '.state'); + const remoteConfigPath = path.join(tempRoot, 'remote.json'); + // The profile has always described endpoint B. The previous connection used + // explicit CLI credentials for endpoint A, so the unchanged file hash proves + // only which file was loaded — not that its token authenticated endpoint A. + fs.writeFileSync( + remoteConfigPath, + JSON.stringify({ + daemonBaseUrl: 'https://new.example', + daemonAuthToken: 'test-new-not-a-real-token', + }), + ); + writeRemoteConnectionState({ + stateDir, + state: { + version: 1, + session: 'adc-android', + remoteConfigPath, + remoteConfigHash: hashRemoteConfigFile(remoteConfigPath), + tenant: 'acme', + runId: 'run-old', + leaseId: 'lease-old', + leaseBackend: 'android-instance', + // Effective previous endpoint A came from --daemon-base-url, overriding + // the profile's endpoint B when this state was recorded. + daemon: { baseUrl: 'https://old.example' }, + connectedAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + }, + }); + let releaseRequest: Parameters[0] | undefined; + + const stdout = await captureStdout(async () => { + await connectCommand({ + positionals: [], + flags: { + json: false, + help: false, + version: false, + force: true, + stateDir, + remoteConfig: remoteConfigPath, + daemonBaseUrl: 'https://new.example', + daemonAuthToken: 'test-new-not-a-real-token', + tenant: 'acme', + runId: 'run-new', + session: 'adc-android', + platform: 'android', + }, + client: createTestClient({ + release: async (request) => { + releaseRequest = request; + return { released: true }; + }, + }), + }); + }); + + assert.equal(releaseRequest, undefined); + assert.match(stdout, /Could not release the previous lease lease-old/); + assert.match(stdout, /old\.example/); + assert.equal(readRemoteConnectionState({ stateDir, session: 'adc-android' })?.runId, 'run-new'); + fs.rmSync(tempRoot, { recursive: true, force: true }); +}); + +test('connect --force still releases with a rotated credential when the config keeps the same endpoint', async () => { + const tempRoot = mkdtempForTestSync('agent-device-connect-force-rotated-token-'); + const stateDir = path.join(tempRoot, '.state'); + const remoteConfigPath = path.join(tempRoot, 'remote.json'); + const newRemoteConfigPath = path.join(tempRoot, 'new-remote.json'); + fs.writeFileSync( + remoteConfigPath, + JSON.stringify({ + daemonBaseUrl: 'https://old.example', + daemonAuthToken: 'test-old-not-a-real-token', + }), + ); + fs.writeFileSync(newRemoteConfigPath, JSON.stringify({ daemonBaseUrl: 'https://new.example' })); + writeRemoteConnectionState({ + stateDir, + state: { + version: 1, + session: 'adc-android', + remoteConfigPath, + remoteConfigHash: hashRemoteConfigFile(remoteConfigPath), + tenant: 'acme', + runId: 'run-old', + leaseId: 'lease-old', + leaseBackend: 'android-instance', + daemon: { baseUrl: 'https://old.example' }, + connectedAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + }, + }); + // The file changed — so the hash no longer matches — but it still describes + // old.example, so the rotated token is still that endpoint's own credential + // and must still release its lease. This is the case an edit-detecting rule + // must not break: refusing here would orphan leases on every key rotation. + fs.writeFileSync( + remoteConfigPath, + JSON.stringify({ + daemonBaseUrl: 'https://old.example', + daemonAuthToken: 'test-rotated-not-a-real-token', + }), + ); + let releaseRequest: Parameters[0] | undefined; + + await captureStdout(async () => { + await connectCommand({ + positionals: [], + flags: { + json: true, + help: false, + version: false, + force: true, + stateDir, + remoteConfig: newRemoteConfigPath, + daemonBaseUrl: 'https://new.example', + tenant: 'acme', + runId: 'run-new', + session: 'adc-android', + platform: 'android', + }, + client: createTestClient({ + release: async (request) => { + releaseRequest = request; + return { released: true }; + }, + }), + }); + }); + + assert.equal(releaseRequest?.leaseId, 'lease-old'); + assert.equal(releaseRequest?.daemonBaseUrl, 'https://old.example'); + assert.equal(releaseRequest?.daemonAuthToken, 'test-rotated-not-a-real-token'); + fs.rmSync(tempRoot, { recursive: true, force: true }); +}); + +test('connect --force reuses the ambient token to release the previous lease when the endpoint is unchanged', async () => { + const tempRoot = mkdtempForTestSync('agent-device-connect-force-same-endpoint-'); + const stateDir = path.join(tempRoot, '.state'); + const remoteConfigPath = path.join(tempRoot, 'remote.json'); + // No daemonAuthToken on the profile itself: the ambient flag is the only + // source, matching an ordinary same-profile --force reconnect. + fs.writeFileSync(remoteConfigPath, JSON.stringify({ daemonBaseUrl: 'https://daemon.example' })); + writeRemoteConnectionState({ + stateDir, + state: { + version: 1, + session: 'adc-android', + remoteConfigPath, + remoteConfigHash: hashRemoteConfigFile(remoteConfigPath), + tenant: 'acme', + runId: 'run-old', + leaseId: 'lease-old', + leaseBackend: 'android-instance', + daemon: { baseUrl: 'https://daemon.example' }, + connectedAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + }, + }); + let releaseRequest: Parameters[0] | undefined; + + await captureStdout(async () => { + await connectCommand({ + positionals: [], + flags: { + json: true, + help: false, + version: false, + force: true, + stateDir, + remoteConfig: remoteConfigPath, + daemonBaseUrl: 'https://daemon.example', + daemonAuthToken: 'test-ambient-not-a-real-token', + tenant: 'acme', + runId: 'run-new', + session: 'adc-android', + platform: 'android', + }, + client: createTestClient({ + release: async (request) => { + releaseRequest = request; + return { released: true }; + }, + }), + }); + }); + + assert.equal(releaseRequest?.leaseId, 'lease-old'); + assert.equal(releaseRequest?.daemonBaseUrl, 'https://daemon.example'); + assert.equal(releaseRequest?.daemonAuthToken, 'test-ambient-not-a-real-token'); + fs.rmSync(tempRoot, { recursive: true, force: true }); +}); + test('disconnect tolerates prior close and removes local connection state', async () => { const tempRoot = mkdtempForTestSync('agent-device-disconnect-'); const stateDir = path.join(tempRoot, '.state'); @@ -2269,7 +2756,6 @@ test('disconnect releases proxy lease with provider client and device metadata', leaseId: 'abc123abc123abc1', daemon: { baseUrl: 'http://proxy.example.test/agent-device', - authToken: 'proxy-secret', }, leaseBackend: 'ios-instance', leaseProvider: 'proxy', @@ -2290,6 +2776,10 @@ test('disconnect releases proxy lease with provider client and device metadata', version: false, stateDir, shutdown: true, + // Not persisted on connection state (ADR 0007): the caller supplies + // it per-command via flag/env/CLI-session, mirroring how the CLI + // dispatcher resolves it before invoking this handler. + daemonAuthToken: 'test-not-a-real-token', }, client: createTestClient({ release: async (request) => { @@ -2306,7 +2796,7 @@ test('disconnect releases proxy lease with provider client and device metadata', assert.equal(releaseRequest?.leaseId, 'abc123abc123abc1'); assert.equal(releaseRequest?.leaseBackend, 'ios-instance'); assert.equal(releaseRequest?.daemonBaseUrl, 'http://proxy.example.test/agent-device'); - assert.equal(releaseRequest?.daemonAuthToken, 'proxy-secret'); + assert.equal(releaseRequest?.daemonAuthToken, 'test-not-a-real-token'); assert.equal(readRemoteConnectionState({ stateDir, session: 'adc-proxy' }), null); fs.rmSync(tempRoot, { recursive: true, force: true }); }); diff --git a/src/cli/commands/connection-presentation.ts b/src/cli/commands/connection-presentation.ts index c4ed9674f..14bf9a67e 100644 --- a/src/cli/commands/connection-presentation.ts +++ b/src/cli/commands/connection-presentation.ts @@ -20,6 +20,11 @@ export type LeasePreparationNotice = { nextSteps: string[]; }; +export type PreviousLeaseReleaseNotice = { + status: 'unreleased'; + message: string; +}; + export function buildLeasePreparationNotice( state: RemoteConnectionState, verification?: ConnectVerification, @@ -77,14 +82,16 @@ export function renderConnectSuccess(options: { state: RemoteConnectionState; readiness?: ConnectReadiness; runtimePreparation?: RuntimePreparationNotice; + previousLeaseNotice?: PreviousLeaseReleaseNotice; }): string { - const { state, readiness, runtimePreparation } = options; + const { state, readiness, runtimePreparation, previousLeaseNotice } = options; if (!readiness) { const leasePreparation = buildLeasePreparationNotice(state); return [ `Configured remote session "${state.session}" tenant "${state.tenant}" run "${state.runId}"${state.leaseId ? ` lease ${state.leaseId}` : ''}.`, leasePreparation?.message, runtimePreparation?.message, + previousLeaseNotice?.message, ] .filter((line): line is string => Boolean(line)) .join('\n'); @@ -104,6 +111,7 @@ export function renderConnectSuccess(options: { lines.push(...readiness.nextSteps.map((step) => ` ${step}`)); lines.push(...(readiness.notes ?? [])); if (runtimePreparation) lines.push(runtimePreparation.message); + if (previousLeaseNotice) lines.push(previousLeaseNotice.message); return lines.join('\n'); } @@ -111,10 +119,10 @@ export function serializeConnectionState(options: { state: RemoteConnectionState; runtimePreparation?: RuntimePreparationNotice; readiness?: ConnectReadiness; + previousLeaseNotice?: PreviousLeaseReleaseNotice; }): Record { - const { state, runtimePreparation, readiness } = options; + const { state, runtimePreparation, readiness, previousLeaseNotice } = options; const leasePreparation = buildLeasePreparationNotice(state, readiness); - const nextSteps = readiness?.nextSteps ?? leasePreparation?.nextSteps ?? []; return { connected: true, session: state.session, @@ -129,34 +137,56 @@ export function serializeConnectionState(options: { remoteConfig: state.remoteConfigPath, remoteConfigHash: state.remoteConfigHash, daemonBaseUrlFingerprint: fingerprint(state.daemon?.baseUrl), - liveSession: { - status: state.leaseId ? 'created' : 'not-created', - ...(state.leaseId ? { leaseId: state.leaseId } : {}), - }, - ...(readiness - ? { - verification: { - status: connectionVerificationStatus(readiness), - service: readiness.service, - message: readiness.verificationMessage, - ...(readiness.project ? { project: readiness.project } : {}), - }, - ...(readiness.device ? { device: readiness.device } : {}), - ...(readiness.app ? { app: readiness.app } : {}), - nextSteps, - ...(readiness.notes ? { notes: readiness.notes } : {}), - } - : {}), + liveSession: buildLiveSessionField(state), + ...buildReadinessFields(readiness, leasePreparation), metro: state.metro ? { prepared: true, projectRoot: state.metro.projectRoot } : { prepared: false }, - ...(leasePreparation ? { leasePreparation } : {}), - ...(runtimePreparation ? { runtimePreparation } : {}), + ...buildConnectionNoticeFields({ leasePreparation, runtimePreparation, previousLeaseNotice }), connectedAt: state.connectedAt, updatedAt: state.updatedAt, }; } +function buildLiveSessionField(state: RemoteConnectionState): Record { + return { + status: state.leaseId ? 'created' : 'not-created', + ...(state.leaseId ? { leaseId: state.leaseId } : {}), + }; +} + +function buildReadinessFields( + readiness: ConnectReadiness | undefined, + leasePreparation: LeasePreparationNotice | undefined, +): Record { + if (!readiness) return {}; + return { + verification: { + status: connectionVerificationStatus(readiness), + service: readiness.service, + message: readiness.verificationMessage, + ...(readiness.project ? { project: readiness.project } : {}), + }, + ...(readiness.device ? { device: readiness.device } : {}), + ...(readiness.app ? { app: readiness.app } : {}), + nextSteps: readiness.nextSteps ?? leasePreparation?.nextSteps ?? [], + ...(readiness.notes ? { notes: readiness.notes } : {}), + }; +} + +function buildConnectionNoticeFields(options: { + leasePreparation?: LeasePreparationNotice; + runtimePreparation?: RuntimePreparationNotice; + previousLeaseNotice?: PreviousLeaseReleaseNotice; +}): Record { + const { leasePreparation, runtimePreparation, previousLeaseNotice } = options; + return { + ...(leasePreparation ? { leasePreparation } : {}), + ...(runtimePreparation ? { runtimePreparation } : {}), + ...(previousLeaseNotice ? { previousLeaseNotice } : {}), + }; +} + function renderDevice(device: NonNullable): string { const osVersion = 'osVersion' in device ? device.osVersion : undefined; const os = [device.platform, osVersion].filter(Boolean).join(' '); diff --git a/src/cli/commands/connection-runtime.ts b/src/cli/commands/connection-runtime.ts index bc10ba64f..51a769b36 100644 --- a/src/cli/commands/connection-runtime.ts +++ b/src/cli/commands/connection-runtime.ts @@ -3,6 +3,10 @@ import { resolveDaemonPaths } from '../../daemon/config.ts'; import { stopReactDevtoolsCompanion } from '../../client/client-react-devtools-companion.ts'; import { stopMetroTunnel } from '../../metro/metro.ts'; import { resolveRemoteConfigProfile } from '../../remote/remote-config.ts'; +// Provenance-preserving file-only read (no ambient env defaults merged in) — +// see resolvePreviousOwnDaemonAuthToken below for why this must not be +// resolveRemoteConfigProfile. +import { readRemoteConfigFile } from '../../remote/remote-config-core.ts'; import { deviceFieldsFromPublicPlatform, isIosFamily, @@ -32,6 +36,7 @@ import { readMetroPrepareKind } from '../../commands/metro/prepare-kind.ts'; import { connectionProviderRequiresRemoteDaemon } from '../connection/provider-policy.ts'; import { readCloudDeviceFeatureProfileFields } from '../connection/profile-fields.ts'; import { isCloudWebDriverProviderName } from '@agent-device/provider-webdriver'; +import type { PreviousLeaseReleaseNotice } from './connection-presentation.ts'; const leaseDeferredCommands = new Set([ 'artifacts', @@ -483,6 +488,9 @@ export async function stopReactDevtoolsCleanup(options: { export async function releaseRemoteConnectionLease( client: AgentDeviceClient, state: RemoteConnectionState, + // The daemon bearer token is never persisted on `state` (ADR 0007); callers + // pass the token already resolved via the flag/env/CLI-session chain. + daemonAuthToken?: string, ): Promise<{ released: boolean; provider?: CloudProviderSessionResult }> { if (!state.leaseId) return { released: false }; const result = await client.leases.release({ @@ -491,7 +499,7 @@ export async function releaseRemoteConnectionLease( leaseId: state.leaseId, leaseBackend: state.leaseBackend, daemonBaseUrl: state.daemon?.baseUrl, - daemonAuthToken: state.daemon?.authToken, + daemonAuthToken, daemonTransport: state.daemon?.transport, daemonServerMode: state.daemon?.serverMode, leaseProvider: state.leaseProvider, @@ -501,18 +509,143 @@ export async function releaseRemoteConnectionLease( return result; } +// A forced reconnect releases the *previous* connection's lease, which must be +// authenticated against the *previous* endpoint's own credential — never the +// new connection's token (that would send an unrelated endpoint's secret to +// an endpoint it was never issued for). See plans/007 for the full rule. +type PreviousLeaseAuthResolution = + | { canAuthenticate: true; daemonAuthToken?: string } + | { canAuthenticate: false }; + +function resolvePreviousLeaseAuth(options: { + previous: RemoteConnectionState; + nextDaemonBaseUrl?: string; + ambientDaemonAuthToken?: string; + cwd: string; + env: Record; +}): PreviousLeaseAuthResolution { + const ownToken = resolvePreviousOwnDaemonAuthToken(options.previous, options.cwd, options.env); + if (ownToken) return { canAuthenticate: true, daemonAuthToken: ownToken }; + if (options.previous.daemon?.baseUrl === options.nextDaemonBaseUrl) { + // Same endpoint: the ambient credential plausibly belongs to it too. + return { canAuthenticate: true, daemonAuthToken: options.ambientDaemonAuthToken }; + } + return { canAuthenticate: false }; +} + +function resolvePreviousOwnDaemonAuthToken( + previous: RemoteConnectionState, + cwd: string, + env: Record, +): string | undefined { + try { + // readRemoteConfigFile, not resolveRemoteConfigProfile: the latter merges + // ambient environment defaults (e.g. AGENT_DEVICE_DAEMON_AUTH_TOKEN) into + // the profile, which would let the *new* connection's env-sourced token + // masquerade as a credential that provably belongs to the *previous* + // endpoint. Only a token the previous config file itself declares counts + // here; the env fallback is rule 2's job, gated on matching endpoints. + const { profile } = readRemoteConfigFile({ + configPath: previous.remoteConfigPath, + cwd, + env, + }); + if (!profile.daemonAuthToken) return undefined; + // The path alone is not provenance. `remoteConfigPath` names a file *now*, + // while the claim being made is about what that file declared when the + // previous connection was established — and a config path is routinely + // reused (edited in place, re-pointed at a second environment) between the + // two. Without this check, "connect to A from ./remote.json, re-point + // ./remote.json at B, connect --force" reads B's token as A's own and + // sends it to A during lease release: the same cross-endpoint leak the + // env-merge fix closed, arriving through the file instead. + return previousConfigStillSpeaksForPreviousEndpoint(previous, profile.daemonBaseUrl) + ? profile.daemonAuthToken + : undefined; + } catch { + // A missing/unparseable previous config is the "cannot authenticate" + // case handled by the caller, not an error to propagate here. + return undefined; + } +} + +/** + * Whether the previous connection's config file can still vouch for a token as + * belonging to the previous connection's endpoint. + * + * The file must explicitly declare the same endpoint recorded in the previous + * connection state. A matching file hash proves only that the file itself did + * not change; it does not prove that its endpoint/token were effective when + * CLI flags may have overridden them. Endpoint equality is the provenance + * boundary and also preserves the benign rotated-credential case. + * + * The endpoint comparison runs both sides through + * `buildRemoteConnectionDaemonState`, the same normalizer that produced the + * stored `daemon.baseUrl`, so it compares like with like rather than raw + * strings that differ only by a trailing slash. + * + * A file that changed and no longer declares an endpoint at all cannot vouch + * for anything: the caller then falls back to rule 2 (matching endpoints) or + * reports the lease as unreleasable, which is a warning and an orphaned lease + * — the correct price for not sending a credential somewhere it may not belong. + */ +function previousConfigStillSpeaksForPreviousEndpoint( + previous: RemoteConnectionState, + declaredDaemonBaseUrl: string | undefined, +): boolean { + const declared = buildRemoteConnectionDaemonState({ + daemonBaseUrl: declaredDaemonBaseUrl, + })?.baseUrl; + return declared !== undefined && declared === previous.daemon?.baseUrl; +} + export async function releasePreviousLease( client: AgentDeviceClient, previous: RemoteConnectionState, -): Promise { - if (!previous.leaseId) return; + options: { + nextDaemonBaseUrl?: string; + ambientDaemonAuthToken?: string; + cwd: string; + env: Record; + }, +): Promise { + if (!previous.leaseId) return undefined; + const auth = resolvePreviousLeaseAuth({ + previous, + nextDaemonBaseUrl: options.nextDaemonBaseUrl, + ambientDaemonAuthToken: options.ambientDaemonAuthToken, + cwd: options.cwd, + env: options.env, + }); + if (!auth.canAuthenticate) { + return buildUnreleasedPreviousLeaseNotice( + previous, + 'no credential known to belong to that endpoint was available', + ); + } try { - await releaseRemoteConnectionLease(client, previous); + await releaseRemoteConnectionLease(client, previous, auth.daemonAuthToken); + return undefined; } catch { - // Reconnect must succeed even if the old lease was already released. + // Reconnect must still succeed; surface the failure instead of hiding it. + return buildUnreleasedPreviousLeaseNotice(previous, 'the release request failed'); } } +function buildUnreleasedPreviousLeaseNotice( + previous: RemoteConnectionState, + reason: string, +): PreviousLeaseReleaseNotice { + return { + status: 'unreleased', + message: + `Could not release the previous lease ${previous.leaseId} ` + + `(tenant ${previous.tenant}, run ${previous.runId}) ` + + `at ${previous.daemon?.baseUrl ?? 'its daemon'}: ${reason}. ` + + 'It was left in place — release it manually if it is still active.', + }; +} + async function releaseAcquiredLeaseOnWriteFailure( client: AgentDeviceClient, state: RemoteConnectionState, diff --git a/src/cli/commands/connection.ts b/src/cli/commands/connection.ts index 91989a9a7..ebcd76c82 100644 --- a/src/cli/commands/connection.ts +++ b/src/cli/commands/connection.ts @@ -43,6 +43,7 @@ import { presentConnectReadiness, renderConnectSuccess, serializeConnectionState, + type PreviousLeaseReleaseNotice, type RuntimePreparationNotice, } from './connection-presentation.ts'; @@ -81,14 +82,20 @@ export const connectCommand: ClientCommandHandler = async ({ positionals, flags, remoteConfigPath: resolved.remoteConfigPath, }); writeRemoteConnectionState({ stateDir, state }); - await cleanupForcedPreviousConnection(client, stateDir, connectFlags, context.previous); + const previousLeaseNotice = await cleanupForcedPreviousConnection( + client, + stateDir, + connectFlags, + context.previous, + state.daemon?.baseUrl, + ); const runtimePreparation = buildRuntimePreparationNotice(connectFlags, state); const readiness = presentConnectReadiness(state, verification); writeCommandOutput( connectFlags, - serializeConnectionState({ state, runtimePreparation, readiness }), - () => renderConnectSuccess({ state, runtimePreparation, readiness }), + serializeConnectionState({ state, runtimePreparation, readiness, previousLeaseNotice }), + () => renderConnectSuccess({ state, runtimePreparation, readiness, previousLeaseNotice }), ); return true; }; @@ -240,11 +247,17 @@ async function cleanupForcedPreviousConnection( stateDir: string, flags: CliFlags, previous: RemoteConnectionState | null, -): Promise { - if (!previous || !flags.force) return; + nextDaemonBaseUrl: string | undefined, +): Promise { + if (!previous || !flags.force) return undefined; await stopMetroCleanup(previous.metro); await stopReactDevtoolsCleanup({ stateDir, state: previous }); - await releasePreviousLease(client, previous); + return await releasePreviousLease(client, previous, { + nextDaemonBaseUrl, + ambientDaemonAuthToken: flags.daemonAuthToken, + cwd: process.cwd(), + env: process.env, + }); } function readRemoteConfigConnectionMetadata( @@ -286,7 +299,7 @@ export const disconnectCommand: ClientCommandHandler = async ({ flags, client }) let released = false; if (state.leaseId) { try { - const release = await releaseRemoteConnectionLease(client, state); + const release = await releaseRemoteConnectionLease(client, state, flags.daemonAuthToken); released = release.released; providerData ??= release.provider; } catch { diff --git a/src/remote/__tests__/remote-connection-state.test.ts b/src/remote/__tests__/remote-connection-state.test.ts new file mode 100644 index 000000000..96587bca7 --- /dev/null +++ b/src/remote/__tests__/remote-connection-state.test.ts @@ -0,0 +1,102 @@ +import { test } from 'vitest'; +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import { mkdtempForTest } from '../../__tests__/test-utils/tmp-dir.ts'; +import { + buildRemoteConnectionDaemonState, + hashRemoteConfigFile, + resolveRemoteConnectionDefaults, + writeRemoteConnectionState, + type RemoteConnectionState, +} from '../remote-connection-state.ts'; + +// Regression coverage for ADR 0007: generated connection profiles must strip +// the daemon bearer token. `connect` used to write it straight into the +// persisted connection-state file; these tests guard against that shadow +// coming back. + +const FAKE_DAEMON_TOKEN = 'test-not-a-real-daemon-token'; + +test('buildRemoteConnectionDaemonState does not persist the daemon auth token', () => { + const daemon = buildRemoteConnectionDaemonState({ + daemonBaseUrl: 'https://daemon.example.test', + daemonAuthToken: FAKE_DAEMON_TOKEN, + daemonTransport: 'http', + daemonServerMode: 'http', + }); + + assert.equal(Object.hasOwn(daemon ?? {}, 'authToken'), false); + assert.equal(daemon?.baseUrl, 'https://daemon.example.test'); + assert.equal(daemon?.transport, 'http'); + assert.equal(daemon?.serverMode, 'http'); +}); + +test('written connection state contains no daemon auth token', async () => { + const tempRoot = await mkdtempForTest('agent-device-remote-connection-state-write-'); + const stateDir = path.join(tempRoot, '.state'); + const remoteConfigPath = path.join(tempRoot, 'remote.json'); + fs.writeFileSync(remoteConfigPath, '{}'); + + const daemon = buildRemoteConnectionDaemonState({ + daemonBaseUrl: 'https://daemon.example.test', + daemonAuthToken: FAKE_DAEMON_TOKEN, + daemonTransport: 'http', + daemonServerMode: 'http', + }); + const now = new Date().toISOString(); + const state: RemoteConnectionState = { + version: 1, + session: 'adc-write-test', + remoteConfigPath, + remoteConfigHash: hashRemoteConfigFile(remoteConfigPath), + daemon, + tenant: 'acme', + runId: 'run-1', + connectedAt: now, + updatedAt: now, + }; + + writeRemoteConnectionState({ stateDir, state }); + + const writtenPath = path.join(stateDir, 'remote-connections', 'adc-write-test.json'); + const written = fs.readFileSync(writtenPath, 'utf8'); + assert.equal(written.includes(FAKE_DAEMON_TOKEN), false); + assert.equal(written.includes('authToken'), false); +}); + +test('resolveRemoteConnectionDefaults falls back to the environment token', async () => { + const tempRoot = await mkdtempForTest('agent-device-remote-connection-state-defaults-'); + const stateDir = path.join(tempRoot, '.state'); + const remoteConfigPath = path.join(tempRoot, 'remote.json'); + fs.writeFileSync(remoteConfigPath, '{}'); + + const daemon = buildRemoteConnectionDaemonState({ + daemonBaseUrl: 'https://daemon.example.test', + daemonAuthToken: undefined, + daemonTransport: 'http', + daemonServerMode: 'http', + }); + const now = new Date().toISOString(); + const state: RemoteConnectionState = { + version: 1, + session: 'adc-env-fallback', + remoteConfigPath, + remoteConfigHash: hashRemoteConfigFile(remoteConfigPath), + daemon, + tenant: 'acme', + runId: 'run-1', + connectedAt: now, + updatedAt: now, + }; + writeRemoteConnectionState({ stateDir, state }); + + const defaults = resolveRemoteConnectionDefaults({ + stateDir, + session: 'adc-env-fallback', + cwd: tempRoot, + env: { AGENT_DEVICE_DAEMON_AUTH_TOKEN: FAKE_DAEMON_TOKEN }, + }); + + assert.equal(defaults?.flags.daemonAuthToken, FAKE_DAEMON_TOKEN); +}); diff --git a/src/remote/remote-config-core.ts b/src/remote/remote-config-core.ts index 948e53ce7..1a768065a 100644 --- a/src/remote/remote-config-core.ts +++ b/src/remote/remote-config-core.ts @@ -12,7 +12,15 @@ import { AppError } from '@agent-device/kernel/errors'; import { resolveUserPath } from '../utils/path-resolution.ts'; import { parseSourceValue } from '../utils/source-value.ts'; -function readRemoteConfigFile(options: RemoteConfigProfileOptions): ResolvedRemoteConfigProfile { +// Deliberately narrower than `resolveRemoteConfigProfile`: this reads only +// what the config *file itself* declares, with no ambient environment +// defaults merged in. Callers that need to know a credential provably +// belongs to a specific profile (not "some token was available from +// somewhere") must use this, not the env-merged resolver, or provenance is +// lost — an env var is global and cannot say which endpoint it belongs to. +export function readRemoteConfigFile( + options: RemoteConfigProfileOptions, +): ResolvedRemoteConfigProfile { const env = options.env ?? process.env; const resolvedPath = resolveRemoteConfigPath(options); if (!fs.existsSync(resolvedPath)) { diff --git a/src/remote/remote-connection-state.ts b/src/remote/remote-connection-state.ts index 81bae0e3f..89f5dea1b 100644 --- a/src/remote/remote-connection-state.ts +++ b/src/remote/remote-connection-state.ts @@ -19,7 +19,6 @@ export type RemoteConnectionState = { remoteConfigHash: string; daemon?: { baseUrl?: string; - authToken?: string; transport?: CliFlags['daemonTransport']; serverMode?: CliFlags['daemonServerMode']; }; @@ -94,7 +93,6 @@ export function buildRemoteConnectionDaemonState( ): RemoteConnectionState['daemon'] { return { baseUrl: sanitizeDaemonBaseUrl(flags.daemonBaseUrl), - authToken: flags.daemonAuthToken, transport: flags.daemonTransport, serverMode: flags.daemonServerMode, }; @@ -154,7 +152,11 @@ export function resolveRemoteConnectionDefaults(options: { ...profile, remoteConfig: state.remoteConfigPath, daemonBaseUrl: state.daemon?.baseUrl ?? profile.daemonBaseUrl, - daemonAuthToken: state.daemon?.authToken ?? profile.daemonAuthToken, + // Deliberately not sourced from state: the daemon bearer token is never + // persisted to the connection-state file (ADR 0007). It is resolved + // from the profile here, and from the flag/env/CLI-session chain in + // resolveRemoteAuth (src/cli/auth-session.ts) at command dispatch time. + daemonAuthToken: profile.daemonAuthToken, daemonTransport: state.daemon?.transport ?? profile.daemonTransport, daemonServerMode: state.daemon?.serverMode ?? profile.daemonServerMode, ...leaseScopeToCommandFlags(leaseScope), diff --git a/test/integration/smoke-provider-cli-disconnect.test.ts b/test/integration/smoke-provider-cli-disconnect.test.ts index ff739cd15..de8e3daec 100644 --- a/test/integration/smoke-provider-cli-disconnect.test.ts +++ b/test/integration/smoke-provider-cli-disconnect.test.ts @@ -48,6 +48,10 @@ function createProviderEnv(fixture: ProviderDaemonFixture): NodeJS.ProcessEnv { BROWSERSTACK_USERNAME: 'browser-user', BROWSERSTACK_ACCESS_KEY: 'browser-key', AGENT_DEVICE_TEST_RPC_LOG_PATH: fixture.rpcLogPath, + // Generated connection state never persists the daemon bearer token + // (ADR 0007), so commands after `connect` must resolve it from the + // environment/CLI chain, same as real usage. + AGENT_DEVICE_DAEMON_AUTH_TOKEN: 'test-daemon-token', NODE_OPTIONS: [process.env.NODE_OPTIONS, `--import=${fetchFixtureUrl}`] .filter(Boolean) .join(' '), diff --git a/website/docs/docs/remote-proxy.md b/website/docs/docs/remote-proxy.md index 869acfa15..bf97512b6 100644 --- a/website/docs/docs/remote-proxy.md +++ b/website/docs/docs/remote-proxy.md @@ -31,12 +31,11 @@ By default the proxy binds `127.0.0.1`. Use `--host 0.0.0.0` only when you inten ## Remote Client -On the machine running the agent, connect to the public tunnel origin with the `/agent-device` base path and the printed token: +On the machine running the agent, connect to the public tunnel origin with the `/agent-device` base path and the printed token. The generated connection profile never stores the token (only routing metadata), so export it once and every command in the session picks it up: ```bash -agent-device connect proxy \ - --daemon-base-url https://example.trycloudflare.com/agent-device \ - --daemon-auth-token +export AGENT_DEVICE_DAEMON_AUTH_TOKEN= +agent-device connect proxy --daemon-base-url https://example.trycloudflare.com/agent-device agent-device devices --platform ios agent-device open MyApp --platform ios agent-device snapshot --platform ios @@ -44,6 +43,8 @@ agent-device close agent-device disconnect ``` +Passing `--daemon-auth-token ` instead of exporting the environment variable also works, but only authenticates the single command it is passed to; subsequent commands need the token again through the env var, a `daemonAuthToken` entry in your remote config profile, or a repeated `--daemon-auth-token` flag. + `connect proxy` stores the proxy profile and client identity. Device leases are automatic on `open` and expire after five minutes without commands. `close` releases the active session and device lease; `disconnect` clears local connection state. Multiple agents can share one proxy when each uses the normal `connect proxy`, `open`, commands, `close`, and `disconnect` flow. A busy device error means another agent owns the device until it closes or its inactivity lease expires.