From 355039658822026321c54258aec283b6266810b2 Mon Sep 17 00:00:00 2001 From: Michael Johnston Date: Wed, 19 Aug 2026 16:11:09 -0700 Subject: [PATCH 1/7] Add private remote attachment retrieval --- docs/developer/local-service-security.md | 22 ++ docs/user/privacy/index.html | 2 +- scripts/app-layout.ts | 1 + scripts/markover.ts | 13 +- src/main.ts | 21 ++ src/remote-attachments.ts | 204 +++++++++++++++++ src/remote-client.ts | 29 +++ src/remote-gateway.ts | 43 +++- test/remote-client.test.ts | 37 ++- test/remote-creation-journal.test.ts | 2 +- test/remote-gateway.test.ts | 272 +++++++++++++++++++++++ 11 files changed, 638 insertions(+), 8 deletions(-) create mode 100644 src/remote-attachments.ts diff --git a/docs/developer/local-service-security.md b/docs/developer/local-service-security.md index ba8bd4e7..077f8e93 100644 --- a/docs/developer/local-service-security.md +++ b/docs/developer/local-service-security.md @@ -125,6 +125,7 @@ The authenticated remote surface is deliberately smaller than local protocol ```text GET /health +GET /reviews//attachments/ POST /reviews POST /reviews/pending POST /reviews//handoff @@ -159,9 +160,30 @@ grants, Serve configuration, login/consent, HTTPS host selection, and certificate issuance remain manual; Markover never enables Funnel or a direct Tailscale-IP listener. +Remote author handoffs project each referenced managed image to a private +attachment route and remove the canonical filesystem path from that response. +The remote client accepts only that exact review-and-attachment route and +resolves it against its already-pinned canonical HTTPS profile; an absolute, +cross-origin, queried, or fragmented gateway value fails closed. Projection and download +reload the current artifact, require one exact attachment reference, reuse the +managed attachment directory/basename and double-realpath allowlist, reject +symlinks, and open the checked file without following links. Before returning +bytes, the gateway verifies the bounded file length, SHA-256 checksum, and PNG +or JPEG signature. Stored JSON and reviewer-agent artifacts are not rewritten. +The download route repeats the same Serve capability check before route +selection and returns private, non-cacheable, nosniff responses. + +The advanced-user pilot gate remains the finite two-host acceptance in issue +#187. The investigated canonical configuration is Tailscale Standalone 1.102.2 +with a Unix-socket Serve target and `--accept-app-caps`; general-user promotion +still requires the live two-host test, exact-device authorization/revocation, +manual Safari consent where required, and a fresh-machine compatibility +doctor. The minimum supported capability-forwarding version is 1.92. + Primary implementation and evidence: - [`src/remote-gateway.ts`](../../src/remote-gateway.ts) +- [`src/remote-attachments.ts`](../../src/remote-attachments.ts) - [`src/main.ts`](../../src/main.ts) - [`test/remote-gateway.test.ts`](../../test/remote-gateway.test.ts) - [`test/local-service.test.ts`](../../test/local-service.test.ts) diff --git a/docs/user/privacy/index.html b/docs/user/privacy/index.html index fbb732bc..33a3fc1f 100644 --- a/docs/user/privacy/index.html +++ b/docs/user/privacy/index.html @@ -59,7 +59,7 @@

How the local API is protected

Optional remote client

Allow the authorized remote Markover client is off by default. When you enable it on canonical Markover, Tailscale Serve can carry review commands from the one tailnet host you authorize into the same Markover app, settings, and review store on this Mac. Markover does not configure your tailnet policy, drive Tailscale login, expose a Tailscale-IP listener, or enable Funnel.

-

The remote path can receive the complete requested Markdown source, its source path, checksum, review purpose, and agent-supplied Git, pull-request, and thread provenance. Lifecycle commands can return review source, feedback, source-edit proposals, attachment metadata, and provenance to that authorized host. Attachment bytes are not available through this first gateway slice.

+

The remote path can receive the complete requested Markdown source, its source path, checksum, review purpose, and agent-supplied Git, pull-request, and thread provenance. Lifecycle commands can return review source, feedback, source-edit proposals, attachment metadata, and provenance to that authorized host. A remote handoff replaces each checked screenshot's canonical file path with a private HTTPS URL; downloading that URL repeats the same Tailscale capability check and verifies the managed file's location, length, type, and checksum before returning bytes.

A remotely supplied source path is only a locator for the returning agent. Canonical Markover marks its source and project state unavailable and does not read that path or run Git beside it, even if the same absolute path exists on this Mac.

Your Tailscale hostname is public certificate metadata. Tailscale HTTPS certificate issuance records the canonical machine name in public Certificate Transparency logs. Choose a machine name that contains no sensitive information.

Disabling the setting closes Markover's private socket after its current bounded request finishes. The same Tailscale Serve configuration cannot reach Markover again until you re-enable it. Another ordinary macOS account cannot access the owner-only socket; processes already running as you, administrators, and root remain inside the account trust boundary.

diff --git a/scripts/app-layout.ts b/scripts/app-layout.ts index 0c70e336..4f045ba1 100644 --- a/scripts/app-layout.ts +++ b/scripts/app-layout.ts @@ -38,6 +38,7 @@ export const runtimeModuleNames = [ 'review-format', 'review-link-copy', 'review-project-context', + 'remote-attachments', 'remote-gateway', 'review-store', 'review-url', diff --git a/scripts/markover.ts b/scripts/markover.ts index eabafb9d..144f0e78 100644 --- a/scripts/markover.ts +++ b/scripts/markover.ts @@ -18,6 +18,7 @@ import { readRemoteHealth, RemoteClientError, requestRemoteJson, + validateRemoteAttachmentUrls, type RemoteHealth, type RemoteJsonRequestOptions } from '../src/remote-client' @@ -1710,7 +1711,7 @@ export async function executeCommand( readSessionDiscoverySetting: readDiscoverySetting = readSessionDiscoverySetting, settingsPath = path.join(path.dirname(endpointPath), 'settings.json') } = options - const requestAuthorJson = ( + const requestAuthorJson = async ( method: string, requestPath: string, body?: unknown, @@ -1719,8 +1720,9 @@ export async function executeCommand( mutation?: boolean timeoutMilliseconds?: number } = {} - ): Promise => profile - ? remoteRequest(profile, method, requestPath, body ?? null, { + ): Promise => { + if (profile) { + const response = await remoteRequest(profile, method, requestPath, body ?? null, { ...(requestOptions.headers ? { headers: requestOptions.headers } : {}), @@ -1729,7 +1731,9 @@ export async function executeCommand( : { mutation: requestOptions.mutation }), preflight: false }) - : requestJson( + return validateRemoteAttachmentUrls(profile, response) + } + return requestJson( endpointPath, method, requestPath, @@ -1738,6 +1742,7 @@ export async function executeCommand( ? undefined : { timeoutMilliseconds: requestOptions.timeoutMilliseconds } ) + } if (parsed.command === 'open') { const sourcePath = path.resolve(parsed.sourcePath) const journal = profile diff --git a/src/main.ts b/src/main.ts index 0b7e7e4e..9af8a277 100644 --- a/src/main.ts +++ b/src/main.ts @@ -1881,6 +1881,27 @@ async function setRemoteGatewayEnabled(enabled: boolean): Promise { store.settings.discoverAgentThreadFromLocalSessions ), routingReady: assertRemoteGatewayRoutingReady, + async loadAttachment(reviewId, attachmentId) { + const artifact = await reviewStore.load(reviewId) + internalAttachments.replaceReview(reviewId, artifact) + const matches: ReviewAttachment[] = [] + const visit = (node: ReviewNode): void => { + for (const attachment of node.attachments || []) { + if (attachment.id === attachmentId) matches.push(attachment) + } + node.children.forEach(visit) + } + visit(artifact.root) + if (matches.length !== 1) return null + const filePath = await internalAttachments.resolve(reviewId, attachmentId) + return filePath + ? { + attachment: matches[0] as ReviewAttachment, + attachmentRoot: path.join(reviewStore.directory, reviewId, 'attachments'), + filePath + } + : null + }, scheme: addressedInstance.scheme }) } diff --git a/src/remote-attachments.ts b/src/remote-attachments.ts new file mode 100644 index 00000000..fe2de139 --- /dev/null +++ b/src/remote-attachments.ts @@ -0,0 +1,204 @@ +import { createHash } from 'node:crypto' +import { constants } from 'node:fs' +import fs from 'node:fs/promises' +import path from 'node:path' + +import { MAXIMUM_BODY_BYTES } from './local-service' + +const REVIEW_ID_PATTERN = /^mko_[a-zA-Z0-9]{6,32}$/ +const ATTACHMENT_ID_PATTERN = /^img-[a-zA-Z0-9]{1,64}$/ +const CHECKSUM_PATTERN = /^sha256:[a-f0-9]{64}$/ + +export interface RemoteAttachmentSource { + attachment: ReviewAttachment + attachmentRoot: string + filePath: string +} + +export type LoadRemoteAttachment = ( + reviewId: string, + attachmentId: string +) => Promise + +export interface VerifiedRemoteAttachment { + bytes: Buffer + mimeType: 'image/jpeg' | 'image/png' +} + +export class RemoteAttachmentError extends Error { + readonly code: string + + constructor(code: string, message: string) { + super(message) + this.name = 'RemoteAttachmentError' + this.code = code + } +} + +function attachmentError(code: string, message: string): RemoteAttachmentError { + return new RemoteAttachmentError(code, message) +} + +function isRecord(value: unknown): value is Record { + return value !== null && typeof value === 'object' && !Array.isArray(value) +} + +function attachmentEntries(value: unknown): ReviewAttachment[] { + if (!isRecord(value) || !isRecord(value.root)) return [] + const entries: ReviewAttachment[] = [] + const visit = (node: unknown): void => { + if (!isRecord(node)) return + if (Array.isArray(node.attachments)) { + for (const attachment of node.attachments) { + if (isRecord(attachment)) entries.push(attachment as unknown as ReviewAttachment) + } + } + if (Array.isArray(node.children)) node.children.forEach(visit) + } + visit(value.root) + return entries +} + +function attachmentPath(reviewId: string, attachmentId: string): string { + return `/reviews/${encodeURIComponent(reviewId)}/attachments/${encodeURIComponent(attachmentId)}` +} + +function checkedMetadata( + reviewId: string, + attachmentId: string, + source: RemoteAttachmentSource | null +): RemoteAttachmentSource { + if (!source || source.attachment.id !== attachmentId) { + throw attachmentError( + 'REMOTE_ATTACHMENT_NOT_FOUND', + `Review ${reviewId} does not reference attachment ${attachmentId}.` + ) + } + if ( + source.attachment.type !== 'image' || + !['image/jpeg', 'image/png'].includes(source.attachment.mimeType ?? '') || + !source.attachment.checksum || + !CHECKSUM_PATTERN.test(source.attachment.checksum) + ) { + throw attachmentError( + 'REMOTE_ATTACHMENT_METADATA_INVALID', + `Attachment ${attachmentId} does not have complete private-download metadata.` + ) + } + return source +} + +export async function readVerifiedRemoteAttachment( + reviewId: string, + attachmentId: string, + load: LoadRemoteAttachment, + maximumBytes = MAXIMUM_BODY_BYTES +): Promise { + if (!REVIEW_ID_PATTERN.test(reviewId) || !ATTACHMENT_ID_PATTERN.test(attachmentId)) { + throw attachmentError('REMOTE_ATTACHMENT_NOT_FOUND', 'Attachment not found.') + } + const source = checkedMetadata( + reviewId, + attachmentId, + await load(reviewId, attachmentId) + ) + const noFollow = constants.O_NOFOLLOW + let handle: fs.FileHandle | null = null + try { + const [realRoot, realFile] = await Promise.all([ + fs.realpath(source.attachmentRoot), + fs.realpath(source.filePath) + ]) + const relative = path.relative(realRoot, realFile) + if ( + path.dirname(realFile) !== realRoot || + !relative || + relative.startsWith('..') || + path.isAbsolute(relative) || + !new RegExp(`^${attachmentId.replace('-', '\\-')}\\.[a-z0-9]+$`).test( + path.basename(realFile) + ) + ) { + throw attachmentError('REMOTE_ATTACHMENT_NOT_FOUND', 'Attachment not found.') + } + const pathStats = await fs.lstat(realFile) + if (!pathStats.isFile() || pathStats.isSymbolicLink()) { + throw attachmentError('REMOTE_ATTACHMENT_NOT_FOUND', 'Attachment not found.') + } + handle = await fs.open(realFile, constants.O_RDONLY | noFollow) + const before = await handle.stat() + if (!before.isFile() || before.size < 1 || before.size > maximumBytes) { + throw attachmentError( + before.size > maximumBytes + ? 'REMOTE_ATTACHMENT_TOO_LARGE' + : 'REMOTE_ATTACHMENT_LENGTH_MISMATCH', + 'The attachment length is invalid.' + ) + } + const bytes = await handle.readFile() + const after = await handle.stat() + if ( + bytes.byteLength !== before.size || + after.size !== before.size || + after.dev !== before.dev || + after.ino !== before.ino + ) { + throw attachmentError( + 'REMOTE_ATTACHMENT_LENGTH_MISMATCH', + 'The attachment changed while it was being read.' + ) + } + const checksum = `sha256:${createHash('sha256').update(bytes).digest('hex')}` + if (checksum !== source.attachment.checksum) { + throw attachmentError( + 'REMOTE_ATTACHMENT_CHECKSUM_MISMATCH', + 'The attachment checksum does not match its review metadata.' + ) + } + const png = bytes.subarray(0, 8).equals( + Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]) + ) + const jpeg = bytes[0] === 0xff && bytes[1] === 0xd8 && bytes[2] === 0xff + const mimeType = source.attachment.mimeType as 'image/jpeg' | 'image/png' + if ((mimeType === 'image/png' && !png) || (mimeType === 'image/jpeg' && !jpeg)) { + throw attachmentError( + 'REMOTE_ATTACHMENT_TYPE_MISMATCH', + 'The attachment bytes do not match their declared image type.' + ) + } + return { bytes, mimeType } + } catch (error) { + if (error instanceof RemoteAttachmentError) throw error + throw attachmentError('REMOTE_ATTACHMENT_NOT_FOUND', 'Attachment not found.') + } finally { + await handle?.close().catch(() => undefined) + } +} + +export async function projectRemoteAttachments( + artifact: unknown, + load: LoadRemoteAttachment +): Promise { + if (!isRecord(artifact) || !isRecord(artifact.review)) return artifact + const reviewId = artifact.review.id + if (typeof reviewId !== 'string' || !REVIEW_ID_PATTERN.test(reviewId)) return artifact + const entries = attachmentEntries(artifact) + if (!entries.length) return artifact + const seen = new Set() + for (const attachment of entries) { + if (!ATTACHMENT_ID_PATTERN.test(attachment.id) || seen.has(attachment.id)) { + throw attachmentError( + 'REMOTE_ATTACHMENT_METADATA_INVALID', + 'The review contains invalid or duplicate attachment metadata.' + ) + } + seen.add(attachment.id) + await readVerifiedRemoteAttachment(reviewId, attachment.id, load) + } + const projected = structuredClone(artifact) + for (const attachment of attachmentEntries(projected)) { + delete attachment.path + attachment.url = attachmentPath(reviewId, attachment.id) + } + return projected +} diff --git a/src/remote-client.ts b/src/remote-client.ts index bc25d03b..b1d1f25a 100644 --- a/src/remote-client.ts +++ b/src/remote-client.ts @@ -116,6 +116,35 @@ function requestUrl(profile: RemoteProfile, requestPath: string): URL { return url } +export function validateRemoteAttachmentUrls( + profile: RemoteProfile, + value: unknown +): unknown { + if (!isRecord(value) || !isRecord(value.review) || typeof value.review.id !== 'string') { + return value + } + const reviewId = value.review.id + const visit = (entry: unknown): void => { + if (!isRecord(entry)) return + if (Array.isArray(entry.attachments)) { + for (const attachment of entry.attachments) { + if ( + !isRecord(attachment) || + typeof attachment.id !== 'string' || + Object.hasOwn(attachment, 'path') || + typeof attachment.url !== 'string' + ) throw invalidResponse() + const expectedPath = `/reviews/${encodeURIComponent(reviewId)}/attachments/${encodeURIComponent(attachment.id)}` + if (attachment.url !== expectedPath) throw invalidResponse() + attachment.url = new URL(expectedPath, profile.baseUrl).href + } + } + if (Array.isArray(entry.children)) entry.children.forEach(visit) + } + visit(value.root) + return value +} + async function sendRemoteJson( profile: RemoteProfile, method: string, diff --git a/src/remote-gateway.ts b/src/remote-gateway.ts index 1f4e4c77..9a3a835b 100644 --- a/src/remote-gateway.ts +++ b/src/remote-gateway.ts @@ -16,6 +16,11 @@ import { MAXIMUM_BODY_BYTES } from './local-service' import { reviewUrl } from './review-url' +import { + projectRemoteAttachments, + readVerifiedRemoteAttachment, + type LoadRemoteAttachment +} from './remote-attachments' export const REMOTE_GATEWAY_CAPABILITY = 'lastobelus.com/cap/markover-remote-client' @@ -54,6 +59,7 @@ export interface RemoteGatewayOptions { localToken: string discoveryPolicy: () => boolean routingReady: () => Promise + loadAttachment: LoadRemoteAttachment maximumResponseBytes?: number scheme?: string platform?: NodeJS.Platform @@ -235,6 +241,10 @@ function validatedAttemptHeaders(request: IncomingMessage): { function allowedRemotePath(method: string | undefined, pathname: string): boolean { if (method === 'GET' && pathname === '/health') return true + if ( + method === 'GET' && + /^\/reviews\/mko_[a-zA-Z0-9]{6,32}\/attachments\/img-[a-zA-Z0-9]{1,64}$/.test(pathname) + ) return true if (method !== 'POST') return false if ( pathname === '/reviews' || @@ -412,6 +422,7 @@ export async function startRemoteGateway({ localToken, discoveryPolicy, routingReady, + loadAttachment, maximumResponseBytes = MAXIMUM_REMOTE_RESPONSE_BYTES, scheme = 'markover', platform = process.platform, @@ -508,6 +519,26 @@ export async function startRemoteGateway({ return } + const attachmentRoute = /^\/reviews\/(mko_[a-zA-Z0-9]{6,32})\/attachments\/(img-[a-zA-Z0-9]{1,64})$/.exec( + url.pathname + ) + if (request.method === 'GET' && attachmentRoute) { + const attachment = await readVerifiedRemoteAttachment( + attachmentRoute[1] as string, + attachmentRoute[2] as string, + loadAttachment + ) + response.writeHead(200, { + 'cache-control': 'private, no-store', + connection: 'close', + 'content-length': attachment.bytes.byteLength, + 'content-type': attachment.mimeType, + 'x-content-type-options': 'nosniff' + }) + response.end(attachment.bytes) + return + } + const attempt = url.pathname === '/reviews' ? validatedAttemptHeaders(request) : null @@ -542,13 +573,23 @@ export async function startRemoteGateway({ internalHeaders, maximumResponseBytes ) - const body = proxied.statusCode >= 200 && proxied.statusCode < 300 + let body = proxied.statusCode >= 200 && proxied.statusCode < 300 ? url.pathname === '/reviews' ? responseWithReviewUrl(proxied.body, scheme) : url.pathname === '/reviews/pending' ? pendingResponseWithUrls(proxied.body, scheme) : proxied.body : proxied.body + if ( + proxied.statusCode >= 200 && + proxied.statusCode < 300 && + /^\/reviews\/mko_[a-zA-Z0-9]{6,32}\/(?:handoff|edit|revise)$/.test(url.pathname) + ) { + body = await projectRemoteAttachments( + body, + loadAttachment + ) + } assertResponseBound(body, maximumResponseBytes) sendJson(response, proxied.statusCode, body) } catch (error) { diff --git a/test/remote-client.test.ts b/test/remote-client.test.ts index c8ece04f..62c2172f 100644 --- a/test/remote-client.test.ts +++ b/test/remote-client.test.ts @@ -5,11 +5,13 @@ import type https from 'node:https' import type { RequestOptions as HttpsRequestOptions } from 'node:https' import test from 'node:test' + import { readRemoteHealth, RemoteClientError, type RemoteClientTiming, - requestRemoteJson + requestRemoteJson, + validateRemoteAttachmentUrls } from '../src/remote-client' import { loadRemoteProfile, @@ -450,3 +452,36 @@ test('connect and response timeouts use independently injectable bounds', async (error: unknown) => hasRemoteError(error, 'REQUEST_UNCERTAIN') ) }) + +test('remote attachment URLs stay on the pinned canonical HTTPS origin', () => { + const artifact = { + review: { id: 'mko_aaa11111' }, + root: { + attachments: [{ + id: 'img-1', + url: '/reviews/mko_aaa11111/attachments/img-1' + }], + children: [] + } + } + assert.equal(validateRemoteAttachmentUrls(profile, artifact), artifact) + assert.equal( + artifact.root.attachments[0]?.url, + 'https://canonical.example.ts.net/reviews/mko_aaa11111/attachments/img-1' + ) + for (const attachment of [ + { id: 'img-1', path: '/private/img-1.png', url: '/reviews/mko_aaa11111/attachments/img-1' }, + { id: 'img-1', url: 'https://other.example.ts.net/reviews/mko_aaa11111/attachments/img-1' }, + { id: 'img-1', url: 'https://canonical.example.ts.net/reviews/mko_other111/attachments/img-1' }, + { id: 'img-1', url: '/reviews/mko_aaa11111/attachments/img-2' }, + { id: 'img-1', url: '//other.example.ts.net/reviews/mko_aaa11111/attachments/img-1' } + ]) { + assert.throws( + () => validateRemoteAttachmentUrls(profile, { + review: { id: 'mko_aaa11111' }, + root: { attachments: [attachment], children: [] } + }), + (error: unknown) => hasRemoteError(error, 'INVALID_RESPONSE') + ) + } +}) diff --git a/test/remote-creation-journal.test.ts b/test/remote-creation-journal.test.ts index 6c727606..4c81fc52 100644 --- a/test/remote-creation-journal.test.ts +++ b/test/remote-creation-journal.test.ts @@ -139,7 +139,7 @@ test('concurrent digest appends preserve the complete history', async (t) => { ]) const resumed = await firstJournal.acquire(input) - assert.deepEqual(resumed.entry.requestDigests, [digestA, digestB]) + assert.deepEqual([...resumed.entry.requestDigests].sort(), [digestA, digestB]) }) test('a dead process lock is reclaimed before appending a digest', async (t) => { diff --git a/test/remote-gateway.test.ts b/test/remote-gateway.test.ts index d2303ffe..adccf670 100644 --- a/test/remote-gateway.test.ts +++ b/test/remote-gateway.test.ts @@ -8,6 +8,7 @@ import path from 'node:path' import test, { type TestContext } from 'node:test' import type { ResolvedInstance } from '../src/instance' +import { InternalAttachmentAllowlist } from '../src/internal-protocol' import { MAXIMUM_BODY_BYTES, startLocalService @@ -25,6 +26,10 @@ import { startRemoteGateway, type RemoteGateway } from '../src/remote-gateway' +import { + projectRemoteAttachments, + readVerifiedRemoteAttachment +} from '../src/remote-attachments' import { reviewChecksum } from '../src/review-format' import { ReviewStore, type ReviewArtifact } from '../src/review-store' import { createServiceIdentity } from '../src/service-endpoint' @@ -58,6 +63,8 @@ function capabilityHeader( return { [REMOTE_GATEWAY_CAPABILITY_HEADER]: JSON.stringify(value) } } +const missingAttachment = () => Promise.resolve(null) + function responseErrorCode(body: unknown): unknown { if (!body || typeof body !== 'object' || Array.isArray(body)) return null const error = (body as Record).error @@ -106,6 +113,56 @@ async function requestGateway( }) } +async function requestGatewayBytes( + socketPath: string, + requestPath: string, + headers: Record = {} +): Promise<{ body: Buffer; headers: http.IncomingHttpHeaders; statusCode: number | undefined }> { + return new Promise((resolve, reject) => { + const request = http.request({ + socketPath, + method: 'GET', + path: requestPath, + headers + }, (response) => { + const chunks: Uint8Array[] = [] + response.on('data', (chunk: Buffer) => { chunks.push(chunk) }) + response.on('end', () => { + resolve({ + body: Buffer.concat(chunks), + headers: response.headers, + statusCode: response.statusCode + }) + }) + }) + request.on('error', reject) + request.end() + }) +} + +async function interruptGatewayResponse( + socketPath: string, + requestPath: string, + headers: Record +): Promise { + return new Promise((resolve, reject) => { + const request = http.request({ + socketPath, + method: 'GET', + path: requestPath, + headers + }, (response) => { + response.once('data', () => { + response.destroy() + resolve() + }) + response.once('error', reject) + }) + request.on('error', reject) + request.end() + }) +} + async function gatewayFixture( t: TestContext, options: { @@ -131,6 +188,7 @@ async function gatewayFixture( }) let routingChecks = 0 const socketPath = path.join(directory, 'state', 'remote.sock') + const attachments = new InternalAttachmentAllowlist(store.directory) const gateway = await startRemoteGateway({ socketPath, localPort: service.port, @@ -139,6 +197,27 @@ async function gatewayFixture( routingReady() { routingChecks += 1 return Promise.resolve() + }, + async loadAttachment(reviewId, attachmentId) { + const artifact = await store.load(reviewId) + attachments.replaceReview(reviewId, artifact) + const matches: ReviewAttachment[] = [] + const visit = (node: ReviewNode): void => { + for (const attachment of node.attachments || []) { + if (attachment.id === attachmentId) matches.push(attachment) + } + node.children.forEach(visit) + } + visit(artifact.root) + if (matches.length !== 1) return null + const filePath = await attachments.resolve(reviewId, attachmentId) + return filePath + ? { + attachment: matches[0] as ReviewAttachment, + attachmentRoot: path.join(store.directory, reviewId, 'attachments'), + filePath + } + : null } }) t.after(async () => { @@ -464,6 +543,195 @@ test('remote create rejects origin claims, attachment metadata, and digest drift assert.deepEqual(await fixture.store.list(), []) }) +test('remote handoff projects checked private attachments and streams retryable bytes', async (t) => { + const fixture = await gatewayFixture(t) + const createBody = Buffer.from(JSON.stringify({ + tree: tree(), + metadata: { contextSummary: 'Check the private screenshot.' } + })) + const created = await requestGateway( + fixture.socketPath, + 'POST', + '/reviews', + createHeaders(createBody), + createBody + ) + assert.equal(created.statusCode, 201) + + const png = Buffer.concat([ + Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]), + Buffer.from('private-image') + ]) + const saved = await fixture.store.saveAttachmentFile('mko_aaa11111', 'png', png) + const artifact = await fixture.store.load('mko_aaa11111') + const annotated = structuredClone(artifact) + const target = annotated.root.children[0] as ReviewNode + target.attachments = [{ + id: saved.id, + type: 'image', + mimeType: 'image/png', + path: saved.path, + checksum: digest(png) + }] + await fixture.store.updateTree('mko_aaa11111', annotated) + + const handoff = await requestGateway( + fixture.socketPath, + 'POST', + '/reviews/mko_aaa11111/handoff', + capabilityHeader() + ) + assert.equal(handoff.statusCode, 200) + const projected = handoff.body as ReviewArtifact + const projectedAttachment = projected.root.children[0]?.attachments?.[0] + assert.deepEqual(projectedAttachment, { + id: 'img-1', + type: 'image', + mimeType: 'image/png', + checksum: digest(png), + url: '/reviews/mko_aaa11111/attachments/img-1' + }) + assert.equal( + (await fixture.store.load('mko_aaa11111')).root.children[0]?.attachments?.[0]?.path, + saved.path + ) + + const route = '/reviews/mko_aaa11111/attachments/img-1' + const first = await requestGatewayBytes(fixture.socketPath, route, capabilityHeader()) + const retry = await requestGatewayBytes(fixture.socketPath, route, capabilityHeader()) + assert.equal(first.statusCode, 200) + assert.equal(first.headers['content-type'], 'image/png') + assert.equal(first.headers['content-length'], String(png.byteLength)) + assert.deepEqual(first.body, png) + assert.deepEqual(retry.body, png) + + await interruptGatewayResponse(fixture.socketPath, route, capabilityHeader()) + const afterInterruption = await requestGatewayBytes( + fixture.socketPath, + route, + capabilityHeader() + ) + assert.equal(afterInterruption.statusCode, 200) + assert.deepEqual(afterInterruption.body, png) + + const denied = await requestGatewayBytes(fixture.socketPath, route) + assert.equal(denied.statusCode, 403) + assert.equal(responseErrorCode(JSON.parse(denied.body.toString('utf8'))), 'REMOTE_CAPABILITY_REQUIRED') +}) + +test('private attachment checks reject corrupt metadata, paths, bytes, and links', async (t) => { + const directory = await fs.mkdtemp(path.join(os.tmpdir(), 'markover-remote-attachment-test-')) + t.after(() => fs.rm(directory, { recursive: true, force: true })) + const png = Buffer.concat([ + Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]), + Buffer.from('checked') + ]) + const filePath = path.join(directory, 'img-1.png') + await fs.writeFile(filePath, png) + const attachment: ReviewAttachment = { + id: 'img-1', + type: 'image', + mimeType: 'image/png', + path: filePath, + checksum: digest(png) + } + const load = () => Promise.resolve({ + attachment, + attachmentRoot: directory, + filePath + }) + assert.deepEqual( + (await readVerifiedRemoteAttachment('mko_aaa11111', 'img-1', load)).bytes, + png + ) + + attachment.checksum = digest('wrong') + await assert.rejects( + readVerifiedRemoteAttachment('mko_aaa11111', 'img-1', load), + (error: unknown) => error instanceof Error && Reflect.get(error, 'code') === 'REMOTE_ATTACHMENT_CHECKSUM_MISMATCH' + ) + attachment.checksum = digest(png) + attachment.mimeType = 'image/jpeg' + await assert.rejects( + readVerifiedRemoteAttachment('mko_aaa11111', 'img-1', load), + (error: unknown) => error instanceof Error && Reflect.get(error, 'code') === 'REMOTE_ATTACHMENT_TYPE_MISMATCH' + ) + attachment.mimeType = 'image/png' + + const emptyPath = path.join(directory, 'img-2.png') + await fs.writeFile(emptyPath, Buffer.alloc(0)) + await assert.rejects( + readVerifiedRemoteAttachment('mko_aaa11111', 'img-2', () => Promise.resolve({ + attachment: { ...attachment, id: 'img-2' }, + attachmentRoot: directory, + filePath: emptyPath + })), + (error: unknown) => error instanceof Error && Reflect.get(error, 'code') === 'REMOTE_ATTACHMENT_LENGTH_MISMATCH' + ) + + const artifact = { + review: { id: 'mko_aaa11111' }, + root: { + attachments: [attachment, { ...attachment }], + children: [] + } + } + await assert.rejects( + projectRemoteAttachments(artifact, load), + (error: unknown) => error instanceof Error && Reflect.get(error, 'code') === 'REMOTE_ATTACHMENT_METADATA_INVALID' + ) + const linkPath = path.join(directory, 'img-3.png') + await fs.symlink(filePath, linkPath) + await assert.rejects( + readVerifiedRemoteAttachment('mko_aaa11111', 'img-3', () => Promise.resolve({ + attachment: { ...attachment, id: 'img-3' }, + attachmentRoot: directory, + filePath: linkPath + })), + (error: unknown) => error instanceof Error && Reflect.get(error, 'code') === 'REMOTE_ATTACHMENT_NOT_FOUND' + ) + const swapPath = path.join(directory, 'img-swap.png') + await fs.writeFile(swapPath, png) + await assert.rejects( + readVerifiedRemoteAttachment('mko_aaa11111', 'img-swap', async () => { + await fs.unlink(swapPath) + await fs.symlink(filePath, swapPath) + return { + attachment: { ...attachment, id: 'img-swap' }, + attachmentRoot: directory, + filePath: swapPath + } + }), + (error: unknown) => error instanceof Error && Reflect.get(error, 'code') === 'REMOTE_ATTACHMENT_NOT_FOUND' + ) + + const reviewsRoot = path.join(directory, 'reviews') + const firstRoot = path.join(reviewsRoot, 'mko_aaa11111', 'attachments') + const otherRoot = path.join(reviewsRoot, 'mko_bbb22222', 'attachments') + await fs.mkdir(firstRoot, { recursive: true }) + await fs.mkdir(otherRoot, { recursive: true }) + const orphan = path.join(firstRoot, 'img-4.png') + const crossReview = path.join(otherRoot, 'img-4.png') + await fs.writeFile(crossReview, png) + const allowlist = new InternalAttachmentAllowlist(reviewsRoot) + assert.equal(allowlist.register('mko_aaa11111', 'img-4', crossReview), false) + assert.equal( + allowlist.register('mko_aaa11111', 'img-4', path.join(firstRoot, '..', 'img-4.png')), + false + ) + assert.equal(allowlist.register('mko_aaa11111', 'img-4', orphan), true) + assert.equal(await allowlist.resolve('mko_aaa11111', 'img-4'), null) + const duplicateTree = tree() + const [firstNode] = duplicateTree.root.children + assert.ok(firstNode) + firstNode.attachments = [ + { id: 'img-4', path: orphan }, + { id: 'img-4', path: orphan } + ] + allowlist.replaceReview('mko_aaa11111', duplicateTree) + assert.equal(await allowlist.resolve('mko_aaa11111', 'img-4'), null) +}) + test('socket lifecycle hardens modes, rejects live ownership, recovers stale sockets, and removes its own socket', async (t) => { const directory = await fs.mkdtemp( path.join(os.tmpdir(), 'markover-remote-socket-test-') @@ -480,6 +748,7 @@ test('socket lifecycle hardens modes, rejects live ownership, recovers stale soc localPort: 1234, localToken: 'B'.repeat(43), discoveryPolicy: () => false, + loadAttachment: missingAttachment, routingReady: () => Promise.resolve(), uid: owner.uid + 1 }), @@ -497,6 +766,7 @@ test('socket lifecycle hardens modes, rejects live ownership, recovers stale soc localPort: 1234, localToken: 'B'.repeat(43), discoveryPolicy: () => false, + loadAttachment: missingAttachment, routingReady: () => Promise.resolve() }), (error: unknown) => ( @@ -527,6 +797,7 @@ test('socket lifecycle hardens modes, rejects live ownership, recovers stale soc localPort: local.port, localToken: identity.token, discoveryPolicy: () => false, + loadAttachment: missingAttachment, routingReady: () => Promise.resolve() }) const parentMode = (await fs.stat(stateRoot)).mode & 0o777 @@ -561,6 +832,7 @@ test('gateway caps responses from the canonical mutation service', async (t) => localPort: address.port, localToken: 'B'.repeat(43), discoveryPolicy: () => false, + loadAttachment: missingAttachment, routingReady: () => Promise.resolve(), maximumResponseBytes: 64 }) From 8651cd70f7d23726d6ba2888745e620452b08f1c Mon Sep 17 00:00:00 2001 From: Michael Johnston Date: Wed, 19 Aug 2026 18:41:51 -0700 Subject: [PATCH 2/7] Allow pinned remote HTTPS ports --- scripts/markover.ts | 2 +- src/remote-client.ts | 2 +- src/remote-profile.ts | 2 +- test/remote-client.test.ts | 13 ++++++++++++- 4 files changed, 15 insertions(+), 4 deletions(-) diff --git a/scripts/markover.ts b/scripts/markover.ts index 144f0e78..c1a0b1a8 100644 --- a/scripts/markover.ts +++ b/scripts/markover.ts @@ -215,7 +215,7 @@ export function helpPayload() { installation: 'The install-free release launcher needs no installation; it downloads and caches the matching app on first local use. Configured remote author commands do not download or launch a local app.' }, remoteCanonical: { - configuration: `Set ${REMOTE_PROFILE_ENVIRONMENT_VARIABLE} to a JSON file containing exactly {"baseUrl":"https://.ts.net/"}.`, + configuration: `Set ${REMOTE_PROFILE_ENVIRONMENT_VARIABLE} to a JSON file containing exactly {"baseUrl":"https://.ts.net[:]/"}.`, commands: ['open', 'pending', 'get', 'edit', 'revise', 'done'], behavior: 'A valid profile uses canonical Markover on the configured host without downloading, launching, or storing a second Markover app on the client Mac. Thread and Git discovery remain local; attachments and reviewer mode are unavailable.' }, diff --git a/src/remote-client.ts b/src/remote-client.ts index b1d1f25a..8ef3237d 100644 --- a/src/remote-client.ts +++ b/src/remote-client.ts @@ -225,7 +225,7 @@ async function sendRemoteJson( const requestOptions: HttpsRequestOptions = { protocol: 'https:', hostname: url.hostname, - port: 443, + port: url.port ? Number(url.port) : 443, method, path: `${url.pathname}${url.search}`, headers, diff --git a/src/remote-profile.ts b/src/remote-profile.ts index 658dca02..b122b010 100644 --- a/src/remote-profile.ts +++ b/src/remote-profile.ts @@ -43,7 +43,7 @@ export function parseRemoteProfile(value: unknown): RemoteProfile { url.protocol !== 'https:' || url.username !== '' || url.password !== '' || - url.port !== '' || + (url.port !== '' && Number(url.port) < 1) || url.pathname !== '/' || url.search !== '' || url.hash !== '' || diff --git a/test/remote-client.test.ts b/test/remote-client.test.ts index 62c2172f..0cc434d8 100644 --- a/test/remote-client.test.ts +++ b/test/remote-client.test.ts @@ -173,7 +173,7 @@ function creationReceiptDigest(value: unknown): unknown { : null } -test('remote profile is opt-in and accepts only an exact root HTTPS ts.net URL', async () => { +test('remote profile is opt-in and accepts only an exact root HTTPS ts.net endpoint', async () => { assert.equal(await loadRemoteProfile({ environment: {} }), null) let readPath = '' @@ -187,6 +187,9 @@ test('remote profile is opt-in and accepts only an exact root HTTPS ts.net URL', } }), { baseUrl: 'https://canonical.example.ts.net/' }) assert.equal(readPath, '/profiles/canonical.json') + assert.deepEqual(parseRemoteProfile({ + baseUrl: 'https://canonical.example.ts.net:8443/' + }), { baseUrl: 'https://canonical.example.ts.net:8443/' }) for (const baseUrl of [ 'http://canonical.example.ts.net/', @@ -194,6 +197,8 @@ test('remote profile is opt-in and accepts only an exact root HTTPS ts.net URL', 'https://canonical.example.ts.net/reviews', 'https://canonical.example.ts.net/?details=1', 'https://canonical.example.ts.net/#health', + 'https://canonical.example.ts.net:0/', + 'https://canonical.example.ts.net:65536/', 'https://127.0.0.1/', 'https://example.com/' ]) { @@ -221,6 +226,12 @@ test('health pins protocol identity and exposes the boolean discovery snapshot', assert.equal(request.options.path, '/health') assert.equal(request.options.agent, false) + const portTransport = fakeTransport([{ statusCode: 200, body: validHealth }]) + await readRemoteHealth({ + baseUrl: 'https://canonical.example.ts.net:8443/' + }, { request: portTransport.request }) + assert.equal(portTransport.captured[0]?.options.port, 8443) + for (const incompatible of [ { ...validHealth, protocol: { name: 'other', version: 1 } }, { ...validHealth, protocol: { name: 'markover-remote', version: 2 } }, From dbc2cbb65f8213cab176c801f2243433b1b50c44 Mon Sep 17 00:00:00 2001 From: Michael Johnston Date: Wed, 19 Aug 2026 19:47:12 -0700 Subject: [PATCH 3/7] Use authenticated loopback ingress for remote Markover --- DECISIONS.md | 47 +- .../2026-08-18__remote-canonical-markover.md | 129 ++--- docs/developer/local-service-security.md | 63 ++- docs/user/privacy/index.html | 6 +- scripts/app-layout.ts | 2 + scripts/markover.ts | 7 +- src/main.ts | 12 +- src/remote-client.ts | 235 ++++++++- src/remote-gateway-auth.ts | 342 +++++++++++++ src/remote-gateway-credential.ts | 151 ++++++ src/remote-gateway.ts | 293 +++++++----- src/remote-profile.ts | 30 +- test/bootstrap.test.ts | 5 +- test/markover-cli.test.ts | 32 +- test/remote-client.test.ts | 358 +++++++++++++- test/remote-gateway.test.ts | 451 ++++++++++++++---- 16 files changed, 1782 insertions(+), 381 deletions(-) create mode 100644 src/remote-gateway-auth.ts create mode 100644 src/remote-gateway-credential.ts diff --git a/DECISIONS.md b/DECISIONS.md index b65dd885..b52f3500 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -863,13 +863,13 @@ each choice from that baseline without duplicating the live decisions below. release, compatibility/accessibility, and Help-surface work. 22. **Remote ingress is separate from local protocol 2.** The selected remote-client path terminates at one default-off, canonical-only, - mode-restricted Unix socket and then reuses the existing authenticated + proof-authenticated loopback gateway and then reuses the existing authenticated mutation core and single `ReviewStore`. It does not change the plain loopback service, publish its bearer, bind a Tailscale IP, enable Funnel, create a second store, or claim isolation from same-account or privileged - processes. Tailscale Serve and one forwarded application capability form - the later network-facing gate; the portable/store slice creates no - reachable ingress. + processes. Tailscale Serve, one forwarded application capability, and a + separate scoped gateway credential form the network-facing gate; the + portable/store slice creates no reachable ingress. 23. **Remote creation is derived, idempotent, and path-untrusted.** The trusted producer boundary accepts only `agent` and `remote-agent`, while portable readers preserve every unknown nonblank origin. A `remote-agent` review @@ -881,40 +881,51 @@ each choice from that baseline without duplicating the live decisions below. closed when an invalid, incompatible, or unreadable managed artifact makes the scan incomplete; absence is never inferred from an omitted review. - **Audit — Retain (Local service authorization 22–23).** A separate socket - preserves the selected account boundary without weakening local protocol - 2, and store-owned digest recovery prevents duplicate primary review data + **Audit — Retain (Local service authorization 22–23).** A separate scoped + credential preserves the selected account boundary without exposing the local + protocol-2 bearer, and store-owned digest recovery prevents duplicate primary review data after an uncertain response. Evidence: [remote canonical implementation plan](doc/plans/2026-08-18__remote-canonical-markover.md), [review store tests](test/review-store.test.ts), [local service tests](test/local-service.test.ts), and [project context tests](test/review-project-context.test.ts). 24. **Remote ingress is live, explicit, and narrower than local protocol 2.** A - default-off persisted setting creates one owner-only Unix socket only for a + default-off persisted setting creates one fixed `127.0.0.1` listener only for a non-smoke canonical instance whose configured checkout, blessed branch, and exact `markover:` handler are healthy. The gateway requires the one - forwarded `lastobelus.com/cap/markover-remote-client` app capability before - route selection or body reads, admits only remote health and the author + forwarded `lastobelus.com/cap/markover-remote-client` app capability and a + short-lived, single-use request proof derived from its dedicated gateway + credential before route selection or body reads, + admits only remote health, checked attachment retrieval, and the author agent's create/recovery, pending, handoff, edit, revise, and done routes, and returns authenticated `404` for everything else. Health exposes only the remote protocol, canonical role and scheme, and the current session discovery policy. 25. **The gateway reuses the bearer-protected mutation engine without sharing - its bearer.** Tailscale Serve terminates HTTPS and forwards the selected app - capability to the owner-mode socket; the gateway then uses the in-process + either bearer.** Tailscale Serve terminates HTTPS and forwards the selected app + capability to the loopback listener. Health returns a short-lived server + challenge signed by the separate owner-only gateway credential; after + verifying it, the remote client signs the nonce, method, path, and exact + body digest without transmitting that credential. The gateway consumes the + challenge once, signs its JSON response, and then uses the in-process local-service identity for the loopback hop. Remote creation requires a 256-bit idempotency key and exact-body digest, derives `remote-agent`, rejects client origin and attachment claims, rechecks canonical routing, and returns canonical review URLs. Remote request JSON is bounded at 16 MiB - and response JSON at 32 MiB; disabling stops admission, drains the single - active request, and - removes only the socket created by that gateway instance. Markover does not + and response JSON at 32 MiB. Handoffs replace canonical attachment paths + with five-minute grants bound to the review, attachment, and issuing gateway + instance; attachment responses are signed over their exact bytes, and the + shared client verifies that proof, MIME type, and projected checksum. A + restart invalidates outstanding attachment grants. Disabling stops admission, drains the single + active request, and closes its listener. Markover does not configure tailnet grants, drive login, bind a Tailscale IP, or enable Funnel. - **Audit — Retain (Local service authorization 24–25).** The socket keeps - other ordinary macOS accounts outside HTTP parsing while the exact Serve - capability supplies the tailnet gate; the fixed route projection prevents + **Audit — Retain (Local service authorization 24–25).** The scoped proof + denies other ordinary macOS accounts that can reach loopback and prevents + an occupant of the fixed port from impersonating Markover or harvesting a + reusable secret, while the + exact Serve capability supplies the tailnet gate; the fixed route projection prevents a remote author credential from becoming the full local bearer. Evidence: [remote gateway tests](test/remote-gateway.test.ts), [local service tests](test/local-service.test.ts), and the diff --git a/doc/plans/2026-08-18__remote-canonical-markover.md b/doc/plans/2026-08-18__remote-canonical-markover.md index df9be661..d4440a7b 100644 --- a/doc/plans/2026-08-18__remote-canonical-markover.md +++ b/doc/plans/2026-08-18__remote-canonical-markover.md @@ -15,8 +15,8 @@ Build one opt-in **remote-client → remote-canonical seam**: Remote client host Markover CLI → HTTPS to the canonical host’s exact *.ts.net name → Tailscale Serve (tailnet-only; never Funnel) - → mode-restricted Unix socket owned by canonical Markover - → constrained gateway in the canonical app + → fixed loopback-only gateway in canonical Markover + → exact Tailscale capability plus scoped challenge-response proof → existing authenticated local-service mutation core → existing queues, ReviewStore, SettingsStore, renderer, and UI ``` @@ -30,7 +30,7 @@ This is a narrow feature, not configuration-only support. The first delivery is a two-host pilot. General-user promotion follows only after the installed Tailscale variants, setup doctor, revocation, and privacy disclosures are proven. Unsupported transport fails closed; it never falls back to Funnel, -direct Tailscale-IP binding, a second store, or a loopback gateway. +direct Tailscale-IP binding, a second store, or an uncredentialed loopback gateway. ## Product decisions @@ -66,10 +66,11 @@ direct Tailscale-IP binding, a second store, or a loopback gateway. `discoverAgentThreadFromLocalSessions` setting. That snapshot governs local discovery on the remote client host for the command; explicit thread identity remains authoritative. No remote-client settings writer or settings route is added. -7. **The gateway setting is canonical and live.** Enable creates the owned - socket. Disable stops new requests, drains the bounded active request, - closes the gateway, and removes only its socket. Development and smoke - instances cannot activate it. +7. **The gateway setting is canonical and live.** Enable loads or creates one + owner-only, scoped gateway credential and binds only fixed + `127.0.0.1:39831`. Disable stops new requests, drains the bounded active + request, and closes the gateway. Development and smoke instances cannot + activate it. ## Tailscale evidence and boundary @@ -86,18 +87,14 @@ Expose a service listening on a Unix socket (Linux/macOS/BSD only): tailscale serve unix:/var/run/myservice.sock ``` -It also advertises `--accept-app-caps`. This is stronger pilot evidence than -an inference from the online examples. Tailscale’s macOS restriction on -directly serving files and directories does not establish a restriction on -reverse-proxying an HTTP service through a Unix socket; those are different -Serve targets. Two things keep the live Serve → Markover-socket hop as -mandatory acceptance: the current online -[Serve CLI reference](https://tailscale.com/docs/reference/tailscale-cli/serve) -does not document the Unix target as clearly as the installed CLI -(documentation drift, recorded here rather than treated as a restriction), -and the process that dials the socket at request time is the network -extension, not the CLI that printed the help — extension reachability of a -socket under the user’s state root is proven only by the live hop. +It also advertises `--accept-app-caps`. The live pilot proved that the +Standalone macOS network extension accepts a Unix target but cannot connect to +a socket beneath the user's Application Support tree: its proxy log reports +`operation not permitted`, and its App Sandbox entitlements do not grant that +tree. Direct owner access to the same healthy Markover socket succeeds. The +pilot therefore uses Tailscale's documented loopback HTTP reverse proxy while +keeping the exact capability gate and adding challenge-response authentication +from a separate scoped credential to preserve the other-account boundary. Use Tailscale Serve, never Funnel: @@ -109,19 +106,27 @@ Use Tailscale Serve, never Funnel: `lastobelus.com/cap/markover-remote-client`; forwarding requires Tailscale 1.92 or newer ([Serve identity headers](https://tailscale.com/docs/features/tailscale-serve#identity-headers)). -- Bind TCP 443 and the capability to the exact remote-client host selector and exact - the canonical host destination. Prefer a readable host alias over a raw Tailscale IP. A +- Bind one dedicated HTTPS port and the capability to the exact remote-client + host selector and exact canonical host destination. Do not replace or + modify another handler already at the root HTTPS endpoint. Prefer a readable host alias over a raw Tailscale IP. A user-wide selector is too broad because the canonical host and the remote client host share one user identity ([grant selectors](https://tailscale.com/docs/reference/syntax/grants)). -- Accept the Serve capability before route selection or body parsing. The - network never receives `service.token`. +- Accept the Serve capability and a fresh request proof from the dedicated + gateway credential before route selection or body parsing. Neither the + gateway credential nor `service.token` crosses the network. Serve strips client-supplied identity/capability headers before forwarding its -own. The socket restores the existing OS-account boundary for direct local -access: its parent is `0700` and the socket is owner-only. Another ordinary -macOS account cannot reach HTTP parsing. A process running as the same -account, an administrator, or root remains inside Markover’s documented local -trust boundary and is not claimed to be isolated. +own. Loopback is reachable by another local account, so the capability header +alone is insufficient: that account could forge it. Canonical Markover stores +a stable gateway credential in owner-only `remote-gateway.token`, and the +remote client reads the same credential from an owner-only profile. Health +proves server possession before the client sends a nonce-, method-, path-, and +body-bound proof; the gateway consumes the nonce once and authenticates its +JSON response. Another ordinary account can reach HTTP but cannot authenticate, +and a process occupying the fixed port cannot forge health or harvest the +shared credential. A process running as +the same account, an administrator, or root remains inside Markover’s +documented local trust boundary and is not claimed to be isolated. Any Tailscale login or HTTPS-consent URL is printed for manual Safari use. Markover does not drive OAuth or edit tailnet policy. HTTPS certificate @@ -135,7 +140,7 @@ The first implementation stack must deliberately amend existing records: - [`DECISIONS.md`](../../DECISIONS.md) retains local protocol 2 as plain loopback HTTP with its protected bearer. It adds a separate, default-off remote - ingress and records why its mode-restricted socket preserves the + ingress and records why a separate scoped credential preserves the other-account boundary that an uncredentialed loopback gateway would lose. - The retained “no automatic review upload” statement gains one exception: a user-enabled remote client sends a requested review from an exactly @@ -150,14 +155,14 @@ The first implementation stack must deliberately amend existing records: Tailscale dependency, canonical-host storage, remote-source boundary, and certificate name disclosure. -The Unix socket protects the remote ingress boundary without replacing the -local bearer service. Glossary changes wait until PR finalization, when +The dedicated gateway credential protects the loopback ingress without replacing +or exposing the local bearer service. Glossary changes wait until PR finalization, when recurring terms can be judged from actual implementation. ## Complexity-brake dispositions -- Gateway protocol: **narrow** to one capability, one canonical socket, and a - fixed author-agent route set. +- Gateway protocol: **narrow** to one capability, one scoped challenge-response + credential, one fixed loopback listener, and a fixed author-agent route set. - Remote-client journal: **narrow** to an invocation fingerprint, raw key, the digest history of sent request bodies, canonical URL, operation state, and eventual receipt; no review body or discovered metadata. @@ -209,7 +214,7 @@ serialized atomic settings writer. | Request bound | Reuse the existing 16 MiB JSON limit. | | Attachment containment | Reuse `InternalAttachmentAllowlist` rules from persisted artifact entries ([internal-protocol.ts](../../src/internal-protocol.ts#L114)). | | Store origin input | Reuse; the producer boundary, not the store type, changes. | -| Gateway, owned socket, remote health | New. Existing endpoint publication is port-only. | +| Gateway, scoped credential, remote health | New. Existing local endpoint publication uses a different rotating credential. | | HTTPS client and remote profile | New. Existing client uses `node:http`. | | Response bound and remote timeout | New. Current responses are unbounded and the local timeout is 2000 ms. | | Creation receipt and key recovery | New. Store create always allocates a fresh ID. | @@ -227,7 +232,8 @@ startup requires all of: validate; - exact `markover:` handler inspection that actually executes and is healthy; - the default-off canonical setting enabled; and -- an owned, mode-restricted socket path under the canonical state root. +- a fixed loopback-only gateway port plus an owner-only scoped credential + under the canonical state root. Remote create fails if routing inspection is vacuous or unhealthy. The canonical host returns `markover://review/` only after that check. Each remote `pending` @@ -236,7 +242,8 @@ item also contains its canonical-host-produced URL. The setting lands through all coordinated settings seams: defaults, normalization, IPC key/validation, and dialog wiring. Tests prove the value is not silently discarded by `normalizeSettings` and exercise enable, disable, -active-request drain, stale socket, smoke, unset environment, and shutdown. +active-request drain, occupied port, credential modes, smoke, unset +environment, and shutdown. ## Remote route boundary @@ -389,17 +396,21 @@ Run against the canonical host and remote client host, not two development store 1. Prove canonical doctor/descriptor/handler health, non-smoke identity, and Funnel absent. **Record** the installed Tailscale variant and version and - **require** version ≥ 1.92 with the `unix:` Serve target and + **require** version ≥ 1.92 with loopback HTTP proxying and `--accept-app-caps` advertised by the installed CLI (at investigation - time: Standalone 1.102.2, `io.tailscale.ipn.macsys`). Prove exact socket - and parent modes, Serve targeting that socket, no LAN/Tailscale-IP - Markover listener, and a live Serve → socket health hop. -2. Grant exact remote-client host alias → exact canonical-host TCP 443 plus the app capability. + time: Standalone 1.102.2, `io.tailscale.ipn.macsys`). Prove the exact + loopback bind and credential modes, additive Serve targeting that port, no + LAN/Tailscale-IP Markover listener, and a live Serve → loopback health hop. +2. Grant exact remote-client host alias → the dedicated canonical-host HTTPS port plus the app capability. Prove another same-user tailnet node cannot pass Serve authorization; prove spoofed remote headers are stripped. As another ordinary local macOS - account, prove socket access fails before HTTP. Record that the canonical host - same-account/root processes remain inside the documented boundary. -3. Configure the remote client host’s exact HTTPS profile. Complete auth/consent only in + account, prove forged proxy headers still fail without a valid challenge + proof. Prove a fixed-port occupant cannot forge health, obtain the shared + credential, replay a proof after restart, or forge a JSON response. + Record that the canonical host same-account/root processes remain inside + the documented boundary. +3. Copy the scoped gateway credential through a private channel into an owner-only remote + profile with the exact HTTPS base URL. Complete auth/consent only in Safari. Prove HTTP, IP URL, redirect, certificate/name, protocol, and canonical-role failures occur before review bytes. 4. Toggle canonical-host discovery off/on. Prove each remote-health snapshot governs @@ -412,8 +423,11 @@ Run against the canonical host and remote client host, not two development store no source/Git discovery and returns unavailable/Other — asserted on the create/publication path itself, before any UI interaction. 7. Open the URL on the canonical host. Add feedback and a screenshot while editing; `get` - on the remote client host returns all feedback and a private checked URL, omits the canonical-host - path, leaves stored JSON unchanged, and denies another node. + on the remote client host returns all feedback and a short-lived, + attachment-and-gateway-scoped private URL, omits the canonical-host path, + leaves stored JSON unchanged, and denies another node. Fetch the bytes + through the shared remote client and prove its response authentication, + MIME, and projected-checksum checks reject a fixed-port imposter. 8. Exercise `edit → get → revise` on the same review and PR-observed `done` on matching reviews only; no source file on the canonical host changes. 9. Lose the initial response after commit and after publication. Rerun the @@ -430,9 +444,9 @@ Run against the canonical host and remote client host, not two development store `--pr-status` caveat. 10. Disable/re-enable the gateway and quit/relaunch the canonical host with editing and pending-agent reviews. Prove the remote client host fails closed while unavailable and the - unchanged Serve configuration reaches the recreated socket afterward. + unchanged Serve configuration reaches the recreated loopback listener afterward. -Acceptance requires the real two-host grant, Serve-to-socket hop, deep +Acceptance requires the real two-host grant, Serve-to-loopback hop, deep link, attachment bytes, enable/disable, and restart. ## Four-PR implementation stack @@ -458,7 +472,7 @@ Done when: missing/mismatched/duplicate receipts, no notification replay, and unchanged ordinary local create. -Excludes socket, Tailscale, remote CLI, attachments, and reachable ingress. +Excludes gateway transport, Tailscale, remote CLI, attachments, and reachable ingress. ### Slice B — Canonical gateway and trust boundary @@ -467,11 +481,13 @@ Base: Slice A. Done when: - the setting lands through defaults, normalizer, IPC validation, and dialog; - its value persists and safely owns live socket lifecycle under the + its value persists and safely owns live loopback lifecycle under the strengthened canonical predicate; -- parent/socket modes, other-account denial, stale ownership, active drain, - shutdown, smoke, and unset-environment cases are covered; -- the gateway requires the Serve capability before route/body handling, +- credential modes, exact loopback bind, occupied port, other-account denial, + active drain, shutdown, smoke, and unset-environment cases are covered; +- the gateway requires the Serve capability and a fresh scoped request proof + before route/body handling, proves JSON responses, and never transmits the + shared gateway credential; implements only the fixed author allowlist plus create recovery, and returns authenticated `404` elsewhere; - remote health exposes protocol/canonical identity and the canonical host’s discovery @@ -529,11 +545,11 @@ and a second canonical store. ## General-user promotion After the pilot, an advanced bring-your-own-Tailscale release requires a -fresh-machine doctor proving supported platforms, installed Serve socket/cap +fresh-machine doctor proving supported platforms, installed Serve loopback/cap behavior, exact-device authorization/revocation, Safari-only auth, privacy/CT disclosures, and failure without fallback. The support matrix uses both current official documentation and executable capability tests; it does not -extrapolate from the canonical host or conflate file serving with Unix-socket service +extrapolate from the canonical host or conflate file serving with reverse proxying. Cross-user collaboration, policy automation, internet relay, and OIDC remain @@ -546,7 +562,8 @@ separate decisions. | Sync Application Support | Decline | Competing writers and non-atomic lifecycle sync. | | Canonical Markover on the remote client host | Decline | Breaks one canonical app/store/UI. | | Copy local service credentials | Decline | Rotating full-access capability becomes a network secret. | -| Stable loopback gateway | Decline | Other local accounts can connect and forge proxy headers unless another credential is added; the socket preserves the existing account boundary. | +| Uncredentialed loopback gateway | Decline | Other local accounts can connect and forge proxy headers. | +| Challenge-response loopback gateway | Accept for pilot | The shared credential restores the account boundary without crossing loopback or exposing the full local-service credential; single-use proofs also survive fixed-port takeover safely. | | Direct Tailscale-IP listener | Decline | Widens listener and duplicates TLS/network identity. | | Tailscale SSH wrapper | Defer | Broader shell authority and incomplete review/attachment contract. | | OIDC/tsidp | Defer | Adds an unnecessary second auth lifecycle for the pilot. | diff --git a/docs/developer/local-service-security.md b/docs/developer/local-service-security.md index 077f8e93..fb1ce076 100644 --- a/docs/developer/local-service-security.md +++ b/docs/developer/local-service-security.md @@ -104,21 +104,31 @@ The default-off `remoteCanonicalGatewayEnabled` setting is the only application switch for remote ingress. It can create a listener only when the running app is canonical rather than development or smoke, the configured canonical checkout and blessed branch still validate, and exact `markover:` handler -inspection executes and reports healthy. The listener is -`remote-gateway.sock` beneath canonical Application Support. Its parent is -owner-only `0700`; the socket is owner-only `0600`. Startup removes only an -inactive socket owned by the current account, refuses live or unowned paths, -and records the created socket identity so shutdown cannot unlink a -replacement. If a saved opt-in cannot meet those checks during startup, +inspection executes and reports healthy. The gateway listens on the fixed +backend `127.0.0.1:39831`, never on a LAN or Tailscale address. Startup refuses +an occupied port. If a saved opt-in cannot meet those checks during startup, Markover turns it off and continues with the local app and service available. -Tailscale Serve terminates HTTPS and proxies HTTP to that Unix socket. Markover +Tailscale Serve terminates HTTPS and proxies HTTP to that loopback listener. Markover requires exactly one forwarded `Tailscale-App-Capabilities` JSON header with a nonempty `lastobelus.com/cap/markover-remote-client` grant. Missing, malformed, wrong, duplicate, or additional forwarded capabilities receive one bounded -rejection before URL parsing or body reads. The socket modes provide the -other-account boundary for direct local access; same-account processes, -administrators, and root remain inside the documented trust boundary. +rejection before URL parsing or body reads. The gateway also requires its own +secret from the remote profile. The canonical credential is stable across +restarts in owner-only `remote-gateway.token`; the remote profile must also be +an owner-only regular file. The secret is never sent over HTTPS or loopback. +Instead, health returns a short-lived nonce plus a server proof. The client +verifies that proof, signs the nonce, method, path, and exact body digest, and +the gateway consumes a valid nonce once before reading the body. JSON responses +carry a proof bound to the nonce, status, and exact response bytes. + +This challenge-response exchange preserves the other-account boundary because +a local account can reach loopback and forge proxy headers but cannot read +either protected file. It also prevents a process that occupies the fixed port +while Markover is stopped from impersonating health, harvesting the shared +secret, replaying a request into a restarted gateway, or forging a JSON +response. Same-account processes, administrators, and root remain inside the +documented trust boundary. The authenticated remote surface is deliberately smaller than local protocol 2: @@ -140,8 +150,10 @@ quit. Remote health returns only protocol name/version, canonical role and scheme, and the current `discoverAgentThreadFromLocalSessions` policy. It omits the process, executable path, port, instance ID, and filesystem paths. -The remote socket, Tailscale hop, and client never receive or publish -`service.token`. After capability and route checks the gateway uses the +The remote gateway, Tailscale hop, and client never receive or publish +`service.token`. The dedicated gateway credential authorizes only this fixed +remote surface; it is not accepted by local protocol 2. After capability, +challenge proof, and route checks the gateway uses the canonical process's in-memory local-service identity for a loopback request, preserving the existing mutation queues, renderer barriers, `ReviewStore`, notifications, and shutdown behavior. Remote create @@ -155,7 +167,7 @@ routing before accepting review bytes, and returns canonical-host-produced Request JSON is capped at 16 MiB and response JSON at 32 MiB, leaving room for the review envelope Markover adds during creation. Only one remote request is active at a time. Disable and shutdown stop admission, drain that bounded -request, close the listener, and remove only its recorded socket. Tailscale +request and close the loopback listener. Tailscale grants, Serve configuration, login/consent, HTTPS host selection, and certificate issuance remain manual; Markover never enables Funnel or a direct Tailscale-IP listener. @@ -163,26 +175,35 @@ Tailscale-IP listener. Remote author handoffs project each referenced managed image to a private attachment route and remove the canonical filesystem path from that response. The remote client accepts only that exact review-and-attachment route and -resolves it against its already-pinned canonical HTTPS profile; an absolute, -cross-origin, queried, or fragmented gateway value fails closed. Projection and download +resolves it against its already-pinned canonical HTTPS profile; an absolute or +cross-origin gateway value fails closed, as does any missing, expired, +malformed, or additional query authorization. Projection and download reload the current artifact, require one exact attachment reference, reuse the managed attachment directory/basename and double-realpath allowlist, reject symlinks, and open the checked file without following links. Before returning bytes, the gateway verifies the bounded file length, SHA-256 checksum, and PNG or JPEG signature. Stored JSON and reviewer-agent artifacts are not rewritten. -The download route repeats the same Serve capability check before route -selection and returns private, non-cacheable, nosniff responses. +The projected URL carries a five-minute proof bound only to that review, +attachment, and running gateway instance; it does not reveal the shared gateway +credential or authorize JSON operations. A restart invalidates outstanding +URLs, and a new handoff returns fresh ones. The download route repeats the Serve +capability check before route selection and returns private, non-cacheable, +nosniff responses with a proof over the exact bytes. The shared remote client +checks that proof, MIME type, and projected attachment checksum. The advanced-user pilot gate remains the finite two-host acceptance in issue #187. The investigated canonical configuration is Tailscale Standalone 1.102.2 -with a Unix-socket Serve target and `--accept-app-caps`; general-user promotion -still requires the live two-host test, exact-device authorization/revocation, -manual Safari consent where required, and a fresh-machine compatibility -doctor. The minimum supported capability-forwarding version is 1.92. +with a loopback HTTP Serve target and `--accept-app-caps`; general-user +promotion still requires the live two-host test, exact-device +authorization/revocation, secure credential transfer, manual Safari consent +where required, and a fresh-machine compatibility doctor. The minimum +supported capability-forwarding version is 1.92. Primary implementation and evidence: - [`src/remote-gateway.ts`](../../src/remote-gateway.ts) +- [`src/remote-gateway-auth.ts`](../../src/remote-gateway-auth.ts) +- [`src/remote-gateway-credential.ts`](../../src/remote-gateway-credential.ts) - [`src/remote-attachments.ts`](../../src/remote-attachments.ts) - [`src/main.ts`](../../src/main.ts) - [`test/remote-gateway.test.ts`](../../test/remote-gateway.test.ts) diff --git a/docs/user/privacy/index.html b/docs/user/privacy/index.html index 33a3fc1f..92a7269b 100644 --- a/docs/user/privacy/index.html +++ b/docs/user/privacy/index.html @@ -58,11 +58,11 @@

How the local API is protected

Optional remote client

-

Allow the authorized remote Markover client is off by default. When you enable it on canonical Markover, Tailscale Serve can carry review commands from the one tailnet host you authorize into the same Markover app, settings, and review store on this Mac. Markover does not configure your tailnet policy, drive Tailscale login, expose a Tailscale-IP listener, or enable Funnel.

-

The remote path can receive the complete requested Markdown source, its source path, checksum, review purpose, and agent-supplied Git, pull-request, and thread provenance. Lifecycle commands can return review source, feedback, source-edit proposals, attachment metadata, and provenance to that authorized host. A remote handoff replaces each checked screenshot's canonical file path with a private HTTPS URL; downloading that URL repeats the same Tailscale capability check and verifies the managed file's location, length, type, and checksum before returning bytes.

+

Allow the authorized remote Markover client is off by default. When you enable it on canonical Markover, Tailscale Serve can carry review commands from the one tailnet host you authorize into the same Markover app, settings, and review store on this Mac. The gateway binds only to loopback and requires the exact Tailscale application capability plus cryptographic proof of a separate secret protected to your account on each Mac. The secret itself is not sent with requests. Markover does not configure your tailnet policy, drive Tailscale login, expose a Tailscale-IP listener, or enable Funnel.

+

The remote path can receive the complete requested Markdown source, its source path, checksum, review purpose, and agent-supplied Git, pull-request, and thread provenance. Lifecycle commands can return review source, feedback, source-edit proposals, attachment metadata, and provenance to that authorized host. A remote handoff replaces each checked screenshot's canonical file path with a short-lived private HTTPS URL scoped to that attachment and running gateway. Downloading it requires the Tailscale authorization; Markover verifies the managed file's location, length, type, and checksum before returning authenticated bytes. Restarting the gateway invalidates outstanding attachment URLs.

A remotely supplied source path is only a locator for the returning agent. Canonical Markover marks its source and project state unavailable and does not read that path or run Git beside it, even if the same absolute path exists on this Mac.

Your Tailscale hostname is public certificate metadata. Tailscale HTTPS certificate issuance records the canonical machine name in public Certificate Transparency logs. Choose a machine name that contains no sensitive information.
-

Disabling the setting closes Markover's private socket after its current bounded request finishes. The same Tailscale Serve configuration cannot reach Markover again until you re-enable it. Another ordinary macOS account cannot access the owner-only socket; processes already running as you, administrators, and root remain inside the account trust boundary.

+

Disabling the setting closes Markover's private loopback gateway after its current bounded request finishes. The same Tailscale Serve configuration cannot reach Markover again until you re-enable it. Another ordinary macOS account can connect to loopback but cannot authenticate without the protected gateway credential; processes already running as you, administrators, and root remain inside the account trust boundary.

diff --git a/scripts/app-layout.ts b/scripts/app-layout.ts index 4f045ba1..554afcee 100644 --- a/scripts/app-layout.ts +++ b/scripts/app-layout.ts @@ -40,6 +40,8 @@ export const runtimeModuleNames = [ 'review-project-context', 'remote-attachments', 'remote-gateway', + 'remote-gateway-auth', + 'remote-gateway-credential', 'review-store', 'review-url', 'review-url-dispatcher', diff --git a/scripts/markover.ts b/scripts/markover.ts index c1a0b1a8..5adbcee6 100644 --- a/scripts/markover.ts +++ b/scripts/markover.ts @@ -215,9 +215,9 @@ export function helpPayload() { installation: 'The install-free release launcher needs no installation; it downloads and caches the matching app on first local use. Configured remote author commands do not download or launch a local app.' }, remoteCanonical: { - configuration: `Set ${REMOTE_PROFILE_ENVIRONMENT_VARIABLE} to a JSON file containing exactly {"baseUrl":"https://.ts.net[:]/"}.`, + configuration: `Set ${REMOTE_PROFILE_ENVIRONMENT_VARIABLE} to an owner-only JSON file containing exactly {"baseUrl":"https://.ts.net[:]/","token":""}.`, commands: ['open', 'pending', 'get', 'edit', 'revise', 'done'], - behavior: 'A valid profile uses canonical Markover on the configured host without downloading, launching, or storing a second Markover app on the client Mac. Thread and Git discovery remain local; attachments and reviewer mode are unavailable.' + behavior: 'A valid profile uses canonical Markover on the configured host without downloading, launching, or storing a second Markover app on the client Mac. Thread and Git discovery remain local; checked attachment URLs carry short-lived, attachment-scoped access and reviewer mode is unavailable.' }, workflow: [ 'Create the Markdown file before opening it.', @@ -1728,8 +1728,7 @@ export async function executeCommand( : {}), ...(requestOptions.mutation === undefined ? {} - : { mutation: requestOptions.mutation }), - preflight: false + : { mutation: requestOptions.mutation }) }) return validateRemoteAttachmentUrls(profile, response) } diff --git a/src/main.ts b/src/main.ts index 9af8a277..6b6e3292 100644 --- a/src/main.ts +++ b/src/main.ts @@ -82,11 +82,15 @@ import { reviewPullRequestIdentity } from './pull-request' import { ReviewAutosave } from './review-autosave' import { remoteGatewayActivationEligible, + REMOTE_GATEWAY_PORT, remoteGatewayHostEligible, - remoteGatewaySocketPath, startRemoteGateway, type RemoteGateway } from './remote-gateway' +import { + loadOrCreateRemoteGatewayCredential, + remoteGatewayCredentialPath +} from './remote-gateway-credential' import { discoverReviewProjectContext, restoreReviewProjectContexts, @@ -1873,10 +1877,14 @@ async function setRemoteGatewayEnabled(enabled: boolean): Promise { if (!service || !identity || !store) { throw new Error('Remote review ingress requires the canonical local service.') } + const gatewayToken = await loadOrCreateRemoteGatewayCredential({ + credentialPath: remoteGatewayCredentialPath(addressedInstance.stateRoot) + }) remoteGateway = await startRemoteGateway({ - socketPath: remoteGatewaySocketPath(addressedInstance.stateRoot), + gatewayToken, localPort: service.port, localToken: identity.token, + port: REMOTE_GATEWAY_PORT, discoveryPolicy: () => ( store.settings.discoverAgentThreadFromLocalSessions ), diff --git a/src/remote-client.ts b/src/remote-client.ts index 8ef3237d..8a941d75 100644 --- a/src/remote-client.ts +++ b/src/remote-client.ts @@ -6,6 +6,17 @@ import { MAXIMUM_REMOTE_RESPONSE_BYTES, REMOTE_GATEWAY_PROTOCOL_VERSION } from './remote-gateway' +import { + remoteContentDigest, + remoteRequestAuthorization, + type RemoteGatewayChallenge, + REMOTE_GATEWAY_CONTENT_DIGEST_HEADER, + REMOTE_GATEWAY_RESPONSE_AUTH_HEADER, + verifyRemoteAttachmentAccess, + verifyRemoteAttachmentResponseAuthorization, + verifyRemoteGatewayChallenge, + verifyRemoteResponseAuthorization +} from './remote-gateway-auth' import type { RemoteProfile } from './remote-profile' const DEFAULT_CONNECT_TIMEOUT_MILLISECONDS = 5_000 @@ -20,6 +31,7 @@ export interface RemoteHealth { role: 'canonical' scheme: 'markover' discoverAgentThreadFromLocalSessions: boolean + authorization: RemoteGatewayChallenge } export class RemoteClientError extends Error { @@ -58,11 +70,19 @@ export interface RemoteClientOptions { } export interface RemoteJsonRequestOptions extends RemoteClientOptions { + authorization?: RemoteGatewayChallenge | undefined headers?: Readonly> | undefined mutation?: boolean | undefined preflight?: boolean | undefined } +export interface RemoteAttachmentReference { + checksum: string + id: string + mimeType: string + url: string +} + function isRecord(value: unknown): value is Record { return value !== null && typeof value === 'object' && !Array.isArray(value) } @@ -134,9 +154,13 @@ export function validateRemoteAttachmentUrls( Object.hasOwn(attachment, 'path') || typeof attachment.url !== 'string' ) throw invalidResponse() - const expectedPath = `/reviews/${encodeURIComponent(reviewId)}/attachments/${encodeURIComponent(attachment.id)}` - if (attachment.url !== expectedPath) throw invalidResponse() - attachment.url = new URL(expectedPath, profile.baseUrl).href + const { url: attachmentUrl } = validatedRemoteAttachmentUrl( + profile, + reviewId, + attachment.id, + attachment.url + ) + attachment.url = attachmentUrl.href } } if (Array.isArray(entry.children)) entry.children.forEach(visit) @@ -145,6 +169,165 @@ export function validateRemoteAttachmentUrls( return value } +function validatedRemoteAttachmentUrl( + profile: RemoteProfile, + reviewId: string, + attachmentId: string, + value: string +): { access: string; url: URL } { + const expectedPath = `/reviews/${encodeURIComponent(reviewId)}/attachments/${encodeURIComponent(attachmentId)}` + let url: URL + try { + url = new URL(value, profile.baseUrl) + } catch { + throw invalidResponse() + } + const access = url.searchParams.get('access') + if ( + url.origin !== new URL(profile.baseUrl).origin || + url.pathname !== expectedPath || + url.searchParams.size !== 1 || + url.hash !== '' || + access === null || + !verifyRemoteAttachmentAccess( + profile.token, + reviewId, + attachmentId, + access + ) + ) throw invalidResponse() + return { access, url } +} + +export async function readRemoteAttachment( + profile: RemoteProfile, + reviewId: string, + attachment: RemoteAttachmentReference, + { + connectTimeoutMilliseconds = DEFAULT_CONNECT_TIMEOUT_MILLISECONDS, + maximumResponseBytes = MAXIMUM_REMOTE_RESPONSE_BYTES, + request: requestTransport = https.request, + responseTimeoutMilliseconds = DEFAULT_RESPONSE_TIMEOUT_MILLISECONDS, + timing = { setTimeout, clearTimeout } + }: RemoteClientOptions = {} +): Promise { + if ( + !positiveFinite(connectTimeoutMilliseconds) || + !positiveFinite(responseTimeoutMilliseconds) || + !positiveFinite(maximumResponseBytes) || + !/^sha256:[a-f0-9]{64}$/.test(attachment.checksum) || + !/^(?:image\/png|image\/jpeg)$/.test(attachment.mimeType) + ) throw invalidResponse() + const { access, url } = validatedRemoteAttachmentUrl( + profile, + reviewId, + attachment.id, + attachment.url + ) + + return new Promise((resolve, reject) => { + let complete = false + let connectTimer: Timer | null = null + let responseTimer: Timer | null = null + const clearTimers = () => { + if (connectTimer !== null) timing.clearTimeout(connectTimer) + if (responseTimer !== null) timing.clearTimeout(responseTimer) + connectTimer = null + responseTimer = null + } + const settle = (action: () => void) => { + if (complete) return + complete = true + clearTimers() + action() + } + const rejectUnavailable = () => { + settle(() => { reject(unavailable()) }) + } + + let request: ReturnType + try { + request = requestTransport({ + protocol: 'https:', + hostname: url.hostname, + port: url.port ? Number(url.port) : 443, + method: 'GET', + path: `${url.pathname}${url.search}`, + headers: { connection: 'close' }, + agent: false + }, (response) => { + if (connectTimer !== null) timing.clearTimeout(connectTimer) + connectTimer = null + const statusCode = response.statusCode ?? null + const contentType = response.headers['content-type'] + let size = 0 + const chunks: Buffer[] = [] + response.on('data', (chunk: Buffer | string) => { + if (complete) return + const bytes = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk) + size += bytes.byteLength + if (size > maximumResponseBytes) { + request.destroy() + settle(() => { reject(new RemoteClientError( + 'RESPONSE_TOO_LARGE', + 'Canonical Markover returned a response that is too large.', + statusCode + )) }) + return + } + chunks.push(bytes) + }) + response.once('aborted', rejectUnavailable) + response.once('error', rejectUnavailable) + response.once('end', () => { + if (complete) return + const bytes = Buffer.concat(chunks) + if ( + !response.complete || + statusCode !== 200 || + contentType !== attachment.mimeType || + remoteContentDigest(bytes) !== attachment.checksum || + !verifyRemoteAttachmentResponseAuthorization( + profile.token, + access, + statusCode, + bytes, + response.headers[REMOTE_GATEWAY_RESPONSE_AUTH_HEADER] + ) + ) { + settle(() => { reject(invalidResponse(statusCode)) }) + return + } + settle(() => { resolve(bytes) }) + }) + }) + } catch { + rejectUnavailable() + return + } + connectTimer = timing.setTimeout(() => { + request.destroy() + rejectUnavailable() + }, connectTimeoutMilliseconds) + request.once('socket', (socket) => { + const connected = () => { + if (connectTimer !== null) timing.clearTimeout(connectTimer) + connectTimer = null + } + if (socket.connecting) socket.once('secureConnect', connected) + else connected() + }) + request.once('finish', () => { + responseTimer = timing.setTimeout(() => { + request.destroy() + rejectUnavailable() + }, responseTimeoutMilliseconds) + }) + request.once('error', rejectUnavailable) + request.end() + }) +} + async function sendRemoteJson( profile: RemoteProfile, method: string, @@ -158,7 +341,8 @@ async function sendRemoteJson( timing = { setTimeout, clearTimeout }, headers: suppliedHeaders = {}, mutation = method !== 'GET' - }: RemoteJsonRequestOptions + }: RemoteJsonRequestOptions, + authorization: RemoteGatewayChallenge | null = null ): Promise { if ( !positiveFinite(connectTimeoutMilliseconds) || @@ -192,6 +376,18 @@ async function sendRemoteJson( headers['content-type'] = 'application/json' headers['content-length'] = Buffer.byteLength(contents) } + const requestBytes = Buffer.from(contents ?? '') + if (authorization) { + const contentDigest = remoteContentDigest(requestBytes) + headers[REMOTE_GATEWAY_CONTENT_DIGEST_HEADER] = contentDigest + headers.authorization = remoteRequestAuthorization( + profile.token, + authorization, + method, + path, + contentDigest + ) + } return new Promise((resolve, reject) => { let complete = false @@ -273,9 +469,23 @@ async function sendRemoteJson( rejectTransport() return } + const responseBytes = Buffer.concat(chunks) + if ( + authorization && + !verifyRemoteResponseAuthorization( + profile.token, + authorization.nonce, + statusCode ?? 0, + responseBytes, + response.headers[REMOTE_GATEWAY_RESPONSE_AUTH_HEADER] + ) + ) { + rejectInvalidResponse(invalidResponse(statusCode)) + return + } let parsed: unknown try { - parsed = JSON.parse(Buffer.concat(chunks).toString('utf8')) + parsed = JSON.parse(responseBytes.toString('utf8')) } catch { rejectInvalidResponse(invalidResponse(statusCode)) return @@ -356,7 +566,8 @@ export async function readRemoteHealth( health.protocol.version !== REMOTE_GATEWAY_PROTOCOL_VERSION || health.role !== 'canonical' || health.scheme !== 'markover' || - typeof health.discoverAgentThreadFromLocalSessions !== 'boolean' + typeof health.discoverAgentThreadFromLocalSessions !== 'boolean' || + !verifyRemoteGatewayChallenge(profile.token, health.authorization) ) { throw new RemoteClientError( 'INCOMPATIBLE_REMOTE_CANONICAL', @@ -374,6 +585,7 @@ export async function requestRemoteJson( body: unknown = null, options: RemoteJsonRequestOptions = {} ): Promise { + let authorization = options.authorization ?? null if (options.preflight !== false) { const { connectTimeoutMilliseconds, @@ -382,7 +594,7 @@ export async function requestRemoteJson( responseTimeoutMilliseconds, timing } = options - await readRemoteHealth(profile, { + const health = await readRemoteHealth(profile, { ...(connectTimeoutMilliseconds === undefined ? {} : { connectTimeoutMilliseconds }), @@ -393,6 +605,13 @@ export async function requestRemoteJson( : { responseTimeoutMilliseconds }), ...(timing === undefined ? {} : { timing }) }) + authorization = health.authorization + } + if (!authorization) { + throw new RemoteClientError( + 'INVALID_REMOTE_REQUEST', + 'A fresh remote Markover authorization challenge is required.' + ) } - return sendRemoteJson(profile, method, path, body, options) + return sendRemoteJson(profile, method, path, body, options, authorization) } diff --git a/src/remote-gateway-auth.ts b/src/remote-gateway-auth.ts new file mode 100644 index 00000000..4ce4e89a --- /dev/null +++ b/src/remote-gateway-auth.ts @@ -0,0 +1,342 @@ +import { createHash, createHmac, randomBytes, timingSafeEqual } from 'node:crypto' +import type { IncomingHttpHeaders } from 'node:http' + +import { CAPABILITY_TOKEN_PATTERN } from './service-endpoint' + +export const REMOTE_GATEWAY_AUTHORIZATION_HEADER = 'authorization' +export const REMOTE_GATEWAY_CONTENT_DIGEST_HEADER = 'markover-content-digest' +export const REMOTE_GATEWAY_RESPONSE_AUTH_HEADER = 'markover-response-auth' +export const REMOTE_GATEWAY_AUTHORIZATION_SCHEME = 'Markover-HMAC-v1' +export const REMOTE_GATEWAY_CHALLENGE_LIFETIME_MILLISECONDS = 30_000 + +const DIGEST_PATTERN = /^sha256:[a-f0-9]{64}$/ +const AUTHORIZATION_PATTERN = /^Markover-HMAC-v1 ([A-Za-z0-9_-]{43})\.([a-f0-9]{64})$/ +const ATTACHMENT_ACCESS_PATTERN = /^(\d{13})\.([a-f0-9]{64})$/ +const REMOTE_ATTACHMENT_ACCESS_LIFETIME_MILLISECONDS = 5 * 60_000 + +export interface RemoteGatewayChallenge { + expiresAt: number + nonce: string + proof: string + scheme: typeof REMOTE_GATEWAY_AUTHORIZATION_SCHEME +} + +function hmac(token: string, value: string): string { + return createHmac('sha256', token).update(value).digest('hex') +} + +function equalHex(left: string, right: string): boolean { + return left.length === right.length && + timingSafeEqual(Buffer.from(left), Buffer.from(right)) +} + +export function remoteContentDigest(bytes: Uint8Array): string { + return `sha256:${createHash('sha256').update(bytes).digest('hex')}` +} + +function challengeProofInput(nonce: string, expiresAt: number): string { + return `markover-remote-server-v1\n${nonce}\n${String(expiresAt)}` +} + +function requestProofInput( + nonce: string, + method: string, + requestPath: string, + contentDigest: string +): string { + return [ + 'markover-remote-request-v1', + nonce, + method, + requestPath, + contentDigest + ].join('\n') +} + +function responseProofInput( + nonce: string, + statusCode: number, + contentDigest: string +): string { + return [ + 'markover-remote-response-v1', + nonce, + String(statusCode), + contentDigest + ].join('\n') +} + +export function createRemoteGatewayChallenge( + token: string, + now = Date.now(), + nonce = randomBytes(32).toString('base64url') +): RemoteGatewayChallenge { + if (!CAPABILITY_TOKEN_PATTERN.test(token) || !CAPABILITY_TOKEN_PATTERN.test(nonce)) { + throw new Error('Remote gateway challenge input is invalid.') + } + const expiresAt = now + REMOTE_GATEWAY_CHALLENGE_LIFETIME_MILLISECONDS + return { + expiresAt, + nonce, + proof: hmac(token, challengeProofInput(nonce, expiresAt)), + scheme: REMOTE_GATEWAY_AUTHORIZATION_SCHEME + } +} + +export function verifyRemoteGatewayChallenge( + token: string, + value: unknown, + now = Date.now() +): value is RemoteGatewayChallenge { + if ( + value === null || + typeof value !== 'object' || + Array.isArray(value) + ) return false + const challenge = value as Record + if ( + Object.keys(challenge).length !== 4 || + challenge.scheme !== REMOTE_GATEWAY_AUTHORIZATION_SCHEME || + typeof challenge.nonce !== 'string' || + !CAPABILITY_TOKEN_PATTERN.test(challenge.nonce) || + typeof challenge.expiresAt !== 'number' || + !Number.isSafeInteger(challenge.expiresAt) || + challenge.expiresAt <= now || + challenge.expiresAt > now + REMOTE_GATEWAY_CHALLENGE_LIFETIME_MILLISECONDS || + typeof challenge.proof !== 'string' || + !/^[a-f0-9]{64}$/.test(challenge.proof) || + !CAPABILITY_TOKEN_PATTERN.test(token) + ) return false + return equalHex( + challenge.proof, + hmac(token, challengeProofInput(challenge.nonce, challenge.expiresAt)) + ) +} + +export function remoteRequestAuthorization( + token: string, + challenge: RemoteGatewayChallenge, + method: string, + requestPath: string, + contentDigest: string +): string { + if (!DIGEST_PATTERN.test(contentDigest)) { + throw new Error('Remote request digest is invalid.') + } + return `${REMOTE_GATEWAY_AUTHORIZATION_SCHEME} ${challenge.nonce}.${hmac( + token, + requestProofInput(challenge.nonce, method, requestPath, contentDigest) + )}` +} + +export class RemoteGatewayChallengeStore { + private readonly challenges = new Map() + + constructor( + private readonly token: string, + private readonly now: () => number = Date.now + ) {} + + create(): RemoteGatewayChallenge { + const now = this.now() + for (const [nonce, expiresAt] of this.challenges) { + if (expiresAt <= now) this.challenges.delete(nonce) + } + while (this.challenges.size >= 32) { + const oldest = this.challenges.keys().next().value + if (oldest === undefined) break + this.challenges.delete(oldest) + } + const challenge = createRemoteGatewayChallenge(this.token, now) + this.challenges.set(challenge.nonce, challenge.expiresAt) + return challenge + } + + authorize( + headers: IncomingHttpHeaders, + method: string, + requestPath: string + ): { contentDigest: string; nonce: string } | null { + const authorization = headers[REMOTE_GATEWAY_AUTHORIZATION_HEADER] + const contentDigest = headers[REMOTE_GATEWAY_CONTENT_DIGEST_HEADER] + if ( + typeof authorization !== 'string' || + typeof contentDigest !== 'string' || + !DIGEST_PATTERN.test(contentDigest) + ) return null + const matched = AUTHORIZATION_PATTERN.exec(authorization) + if (!matched) return null + const nonce = matched[1] as string + const suppliedProof = matched[2] as string + const expiresAt = this.challenges.get(nonce) + if (expiresAt === undefined || expiresAt <= this.now()) { + this.challenges.delete(nonce) + return null + } + const expectedProof = hmac( + this.token, + requestProofInput(nonce, method, requestPath, contentDigest) + ) + if (!equalHex(suppliedProof, expectedProof)) return null + this.challenges.delete(nonce) + return { contentDigest, nonce } + } +} + +export function remoteResponseAuthorization( + token: string, + nonce: string, + statusCode: number, + bytes: Uint8Array +): string { + return hmac( + token, + responseProofInput(nonce, statusCode, remoteContentDigest(bytes)) + ) +} + +export function verifyRemoteResponseAuthorization( + token: string, + nonce: string, + statusCode: number, + bytes: Uint8Array, + value: unknown +): boolean { + return typeof value === 'string' && + /^[a-f0-9]{64}$/.test(value) && + equalHex(value, remoteResponseAuthorization(token, nonce, statusCode, bytes)) +} + +function attachmentProofInput( + reviewId: string, + attachmentId: string, + expiresAt: number +): string { + return [ + 'markover-remote-attachment-v1', + reviewId, + attachmentId, + String(expiresAt) + ].join('\n') +} + +export function createRemoteAttachmentAccess( + token: string, + reviewId: string, + attachmentId: string, + expiresAt: number +): string { + return `${String(expiresAt)}.${hmac( + token, + attachmentProofInput(reviewId, attachmentId, expiresAt) + )}` +} + +export function verifyRemoteAttachmentAccess( + token: string, + reviewId: string, + attachmentId: string, + value: string | null, + now = Date.now() +): boolean { + if (value === null) return false + const matched = ATTACHMENT_ACCESS_PATTERN.exec(value) + if (!matched) return false + const expiresAt = Number(matched[1]) + const suppliedProof = matched[2] as string + if ( + !Number.isSafeInteger(expiresAt) || + expiresAt <= now || + expiresAt > now + REMOTE_ATTACHMENT_ACCESS_LIFETIME_MILLISECONDS + ) return false + return equalHex( + suppliedProof, + hmac(token, attachmentProofInput(reviewId, attachmentId, expiresAt)) + ) +} + +export class RemoteAttachmentAccessStore { + private readonly access = new Set() + + constructor( + private readonly token: string, + private readonly now: () => number = Date.now + ) {} + + create(reviewId: string, attachmentId: string): string { + this.prune() + const value = createRemoteAttachmentAccess( + this.token, + reviewId, + attachmentId, + this.now() + REMOTE_ATTACHMENT_ACCESS_LIFETIME_MILLISECONDS + ) + this.access.add(value) + return value + } + + verify(reviewId: string, attachmentId: string, value: string | null): boolean { + this.prune() + return value !== null && + this.access.has(value) && + verifyRemoteAttachmentAccess( + this.token, + reviewId, + attachmentId, + value, + this.now() + ) + } + + private prune(): void { + const now = this.now() + for (const value of this.access) { + const expiresAt = Number(ATTACHMENT_ACCESS_PATTERN.exec(value)?.[1] ?? 0) + if (expiresAt <= now) this.access.delete(value) + } + } +} + +function attachmentResponseProofInput( + access: string, + statusCode: number, + contentDigest: string +): string { + return [ + 'markover-remote-attachment-response-v1', + access, + String(statusCode), + contentDigest + ].join('\n') +} + +export function remoteAttachmentResponseAuthorization( + token: string, + access: string, + statusCode: number, + bytes: Uint8Array +): string { + return hmac( + token, + attachmentResponseProofInput( + access, + statusCode, + remoteContentDigest(bytes) + ) + ) +} + +export function verifyRemoteAttachmentResponseAuthorization( + token: string, + access: string, + statusCode: number, + bytes: Uint8Array, + value: unknown +): boolean { + return typeof value === 'string' && + /^[a-f0-9]{64}$/.test(value) && + equalHex( + value, + remoteAttachmentResponseAuthorization(token, access, statusCode, bytes) + ) +} diff --git a/src/remote-gateway-credential.ts b/src/remote-gateway-credential.ts new file mode 100644 index 00000000..1d2a3314 --- /dev/null +++ b/src/remote-gateway-credential.ts @@ -0,0 +1,151 @@ +import { randomBytes } from 'node:crypto' +import fs from 'node:fs/promises' +import path from 'node:path' + +import { CAPABILITY_TOKEN_PATTERN } from './service-endpoint' + +export const REMOTE_GATEWAY_CREDENTIAL_NAME = 'remote-gateway.token' + +interface RemoteGatewayCredential { + version: 1 + token: string +} + +export class RemoteGatewayCredentialError extends Error { + readonly code: string + + constructor(code: string, message: string) { + super(message) + this.name = 'RemoteGatewayCredentialError' + this.code = code + } +} + +function isRecord(value: unknown): value is Record { + return value !== null && typeof value === 'object' && !Array.isArray(value) +} + +function errorCode(error: unknown): unknown { + return isRecord(error) ? error.code : null +} + +function parseCredential(value: unknown): RemoteGatewayCredential | null { + if ( + !isRecord(value) || + Object.keys(value).length !== 2 || + value.version !== 1 || + typeof value.token !== 'string' || + !CAPABILITY_TOKEN_PATTERN.test(value.token) + ) return null + return { version: 1, token: value.token } +} + +export function remoteGatewayCredentialPath(stateRoot: string): string { + return path.join(stateRoot, REMOTE_GATEWAY_CREDENTIAL_NAME) +} + +async function readCredential( + credentialPath: string, + uid: number, + platform: NodeJS.Platform +): Promise { + let stats + try { + stats = await fs.lstat(credentialPath) + } catch (error) { + if (errorCode(error) === 'ENOENT') return null + throw error + } + if ( + !stats.isFile() || + stats.isSymbolicLink() || + (platform !== 'win32' && (stats.uid !== uid || (stats.mode & 0o077) !== 0)) + ) { + throw new RemoteGatewayCredentialError( + 'REMOTE_GATEWAY_CREDENTIAL_UNSAFE', + 'The remote gateway credential is not an owner-only regular file.' + ) + } + let parsed: unknown + try { + parsed = JSON.parse(await fs.readFile(credentialPath, 'utf8')) + } catch { + throw new RemoteGatewayCredentialError( + 'REMOTE_GATEWAY_CREDENTIAL_INVALID', + 'The remote gateway credential is invalid.' + ) + } + const credential = parseCredential(parsed) + if (!credential) { + throw new RemoteGatewayCredentialError( + 'REMOTE_GATEWAY_CREDENTIAL_INVALID', + 'The remote gateway credential is invalid.' + ) + } + return credential.token +} + +export interface LoadRemoteGatewayCredentialOptions { + credentialPath: string + platform?: NodeJS.Platform + uid?: number + token?: () => string +} + +export async function loadOrCreateRemoteGatewayCredential({ + credentialPath, + platform = process.platform, + uid = typeof process.getuid === 'function' ? process.getuid() : -1, + token = () => randomBytes(32).toString('base64url') +}: LoadRemoteGatewayCredentialOptions): Promise { + if (platform !== 'win32' && uid < 0) { + throw new RemoteGatewayCredentialError( + 'REMOTE_GATEWAY_CREDENTIAL_PLATFORM_UNSUPPORTED', + 'The remote gateway credential requires an operating-system account.' + ) + } + const parent = path.dirname(credentialPath) + await fs.mkdir(parent, { recursive: true, mode: 0o700 }) + const parentStats = await fs.lstat(parent) + if ( + !parentStats.isDirectory() || + parentStats.isSymbolicLink() || + (platform !== 'win32' && parentStats.uid !== uid) + ) { + throw new RemoteGatewayCredentialError( + 'REMOTE_GATEWAY_CREDENTIAL_PARENT_UNSAFE', + 'The remote gateway credential parent must be an owned directory.' + ) + } + if (platform !== 'win32') await fs.chmod(parent, 0o700) + + const existing = await readCredential(credentialPath, uid, platform) + if (existing) return existing + + const generated = token() + if (!CAPABILITY_TOKEN_PATTERN.test(generated)) { + throw new RemoteGatewayCredentialError( + 'REMOTE_GATEWAY_CREDENTIAL_INVALID', + 'The generated remote gateway credential is invalid.' + ) + } + const contents = `${JSON.stringify({ version: 1, token: generated }, null, 2)}\n` + try { + await fs.writeFile(credentialPath, contents, { + encoding: 'utf8', + flag: 'wx', + flush: true, + mode: 0o600 + }) + if (platform !== 'win32') await fs.chmod(credentialPath, 0o600) + return generated + } catch (error) { + if (errorCode(error) !== 'EEXIST') throw error + const raced = await readCredential(credentialPath, uid, platform) + if (raced) return raced + throw new RemoteGatewayCredentialError( + 'REMOTE_GATEWAY_CREDENTIAL_INVALID', + 'The remote gateway credential is invalid.' + ) + } +} diff --git a/src/remote-gateway.ts b/src/remote-gateway.ts index 9a3a835b..99851adc 100644 --- a/src/remote-gateway.ts +++ b/src/remote-gateway.ts @@ -1,11 +1,8 @@ -import fs from 'node:fs/promises' import http, { type IncomingHttpHeaders, type IncomingMessage, type ServerResponse } from 'node:http' -import net from 'node:net' -import path from 'node:path' import type { ResolvedInstance } from './instance' import { @@ -16,6 +13,14 @@ import { MAXIMUM_BODY_BYTES } from './local-service' import { reviewUrl } from './review-url' +import { + RemoteAttachmentAccessStore, + remoteAttachmentResponseAuthorization, + RemoteGatewayChallengeStore, + remoteContentDigest, + remoteResponseAuthorization, + REMOTE_GATEWAY_RESPONSE_AUTH_HEADER +} from './remote-gateway-auth' import { projectRemoteAttachments, readVerifiedRemoteAttachment, @@ -31,7 +36,8 @@ export const REMOTE_GATEWAY_REQUEST_DIGEST_HEADER = 'markover-request-digest' export const REMOTE_GATEWAY_PROTOCOL_VERSION = 1 export const MAXIMUM_REMOTE_RESPONSE_BYTES = MAXIMUM_BODY_BYTES * 2 -export const REMOTE_GATEWAY_SOCKET_NAME = 'remote-gateway.sock' +export const REMOTE_GATEWAY_HOST = '127.0.0.1' +export const REMOTE_GATEWAY_PORT = 39_831 const IDEMPOTENCY_KEY_PATTERN = /^[A-Za-z0-9_-]{43}$/ const REQUEST_DIGEST_PATTERN = /^sha256:[a-f0-9]{64}$/ @@ -43,27 +49,23 @@ interface ProxyResponse { statusCode: number } -interface SocketIdentity { - dev: number - ino: number -} - export interface RemoteGateway { - socketPath: string + host: typeof REMOTE_GATEWAY_HOST + port: number close: () => Promise } export interface RemoteGatewayOptions { - socketPath: string + gatewayToken: string localPort: number localToken: string discoveryPolicy: () => boolean routingReady: () => Promise loadAttachment: LoadRemoteAttachment maximumResponseBytes?: number + now?: () => number + port?: number scheme?: string - platform?: NodeJS.Platform - uid?: number } export class RemoteGatewayError extends Error { @@ -88,10 +90,6 @@ function gatewayError(code: string, message: string): RemoteGatewayError { return new RemoteGatewayError(code, message) } -export function remoteGatewaySocketPath(stateRoot: string): string { - return path.join(stateRoot, REMOTE_GATEWAY_SOCKET_NAME) -} - export function remoteGatewayActivationEligible( instance: ResolvedInstance, smoke: boolean, @@ -119,15 +117,27 @@ export function remoteGatewayHostEligible( function sendJson( response: ServerResponse, statusCode: number, - body: unknown + body: unknown, + authorization?: { nonce: string; token: string } ): void { const contents = `${JSON.stringify(body)}\n` + const bytes = Buffer.from(contents) response.writeHead(statusCode, { connection: 'close', - 'content-length': Buffer.byteLength(contents), - 'content-type': 'application/json; charset=utf-8' + 'content-length': bytes.byteLength, + 'content-type': 'application/json; charset=utf-8', + ...(authorization + ? { + [REMOTE_GATEWAY_RESPONSE_AUTH_HEADER]: remoteResponseAuthorization( + authorization.token, + authorization.nonce, + statusCode, + bytes + ) + } + : {}) }) - response.end(contents) + response.end(bytes) } function assertResponseBound(body: unknown, maximumBytes: number): void { @@ -335,103 +345,56 @@ function pendingResponseWithUrls(body: unknown, scheme: string): unknown { } } -async function socketIsListening(socketPath: string): Promise { - return new Promise((resolve, reject) => { - const socket = net.createConnection(socketPath) - socket.once('connect', () => { - socket.destroy() - resolve(true) - }) - socket.once('error', (error: NodeJS.ErrnoException) => { - socket.destroy() - if (error.code === 'ECONNREFUSED' || error.code === 'ENOENT') { - resolve(false) - } else { - reject(error) - } - }) - }) -} - -async function prepareSocketPath( - socketPath: string, - uid: number -): Promise { - const parent = path.dirname(socketPath) - await fs.mkdir(parent, { recursive: true, mode: 0o700 }) - const parentStats = await fs.lstat(parent) - if (!parentStats.isDirectory() || parentStats.isSymbolicLink()) { - throw gatewayError( - 'REMOTE_GATEWAY_PARENT_INVALID', - 'The remote gateway parent must be an owned directory.' - ) - } - if (parentStats.uid !== uid) { - throw gatewayError( - 'REMOTE_GATEWAY_PARENT_UNOWNED', - 'The remote gateway parent belongs to another account.' - ) - } - await fs.chmod(parent, 0o700) - - let socketStats - try { - socketStats = await fs.lstat(socketPath) - } catch (error) { - if (errorCode(error) === 'ENOENT') return - throw error - } - if (!socketStats.isSocket() || socketStats.uid !== uid) { - throw gatewayError( - 'REMOTE_GATEWAY_SOCKET_UNOWNED', - 'The remote gateway path is not an owned stale socket.' - ) - } - if (await socketIsListening(socketPath)) { - throw gatewayError( - 'REMOTE_GATEWAY_IN_USE', - 'Another process is already listening on the remote gateway socket.' - ) +function authorizeProjectedAttachmentUrls( + value: unknown, + accessStore: RemoteAttachmentAccessStore +): unknown { + if (!isRecord(value) || !isRecord(value.review) || typeof value.review.id !== 'string') { + return value } - await fs.unlink(socketPath) -} - -async function removeOwnedSocket( - socketPath: string, - identity: SocketIdentity, - uid: number -): Promise { - try { - const stats = await fs.lstat(socketPath) - if ( - stats.isSocket() && - stats.uid === uid && - stats.dev === identity.dev && - stats.ino === identity.ino - ) { - await fs.unlink(socketPath) + const reviewId = value.review.id + const visit = (entry: unknown): void => { + if (!isRecord(entry)) return + if (Array.isArray(entry.attachments)) { + for (const attachment of entry.attachments) { + if (!isRecord(attachment) || typeof attachment.id !== 'string') continue + const expectedPath = `/reviews/${encodeURIComponent(reviewId)}/attachments/${encodeURIComponent(attachment.id)}` + if (attachment.url !== expectedPath) continue + const access = accessStore.create( + reviewId, + attachment.id + ) + attachment.url = `${expectedPath}?access=${access}` + } } - } catch (error) { - if (errorCode(error) !== 'ENOENT') throw error + if (Array.isArray(entry.children)) entry.children.forEach(visit) } + visit(value.root) + return value } export async function startRemoteGateway({ - socketPath, + gatewayToken, localPort, localToken, discoveryPolicy, routingReady, loadAttachment, maximumResponseBytes = MAXIMUM_REMOTE_RESPONSE_BYTES, - scheme = 'markover', - platform = process.platform, - uid = typeof process.getuid === 'function' ? process.getuid() : -1 + now = Date.now, + port = REMOTE_GATEWAY_PORT, + scheme = 'markover' }: RemoteGatewayOptions): Promise { - if (platform === 'win32' || uid < 0) { + if (!IDEMPOTENCY_KEY_PATTERN.test(gatewayToken)) { throw gatewayError( - 'REMOTE_GATEWAY_PLATFORM_UNSUPPORTED', - 'The remote gateway requires an owner-mode Unix socket.' + 'REMOTE_GATEWAY_CREDENTIAL_INVALID', + 'The remote gateway credential is invalid.' + ) + } + if (!Number.isInteger(port) || port < 0 || port > 65_535) { + throw gatewayError( + 'REMOTE_GATEWAY_PORT_INVALID', + 'The remote gateway port is invalid.' ) } if (!Number.isInteger(localPort) || localPort < 1 || localPort > 65_535) { @@ -448,10 +411,11 @@ export async function startRemoteGateway({ } await routingReady() - await prepareSocketPath(socketPath, uid) let accepting = true let activeRequest: Promise | null = null + const challenges = new RemoteGatewayChallengeStore(gatewayToken, now) + const attachmentAccess = new RemoteAttachmentAccessStore(gatewayToken, now) const handleRequest = async ( request: IncomingMessage, @@ -487,6 +451,7 @@ export async function startRemoteGateway({ let resolveActive: () => void = () => {} activeRequest = new Promise((resolve) => { resolveActive = resolve }) + let responseAuthorization: { nonce: string; token: string } | undefined try { let url: URL try { @@ -497,15 +462,14 @@ export async function startRemoteGateway({ }) return } - if (url.search || !allowedRemotePath(request.method, url.pathname)) { - sendJson(response, 404, { - error: { code: 'NOT_FOUND', message: 'Route not found.' } - }) - return - } - - await routingReady() if (request.method === 'GET' && url.pathname === '/health') { + if (url.search) { + sendJson(response, 404, { + error: { code: 'NOT_FOUND', message: 'Route not found.' } + }) + return + } + await routingReady() sendJson(response, 200, { status: 'ok', protocol: { @@ -514,7 +478,8 @@ export async function startRemoteGateway({ }, role: 'canonical', scheme, - discoverAgentThreadFromLocalSessions: discoveryPolicy() + discoverAgentThreadFromLocalSessions: discoveryPolicy(), + authorization: challenges.create() }) return } @@ -523,6 +488,23 @@ export async function startRemoteGateway({ url.pathname ) if (request.method === 'GET' && attachmentRoute) { + if ( + url.searchParams.size !== 1 || + !attachmentAccess.verify( + attachmentRoute[1] as string, + attachmentRoute[2] as string, + url.searchParams.get('access') + ) + ) { + sendJson(response, 403, { + error: { + code: 'REMOTE_ATTACHMENT_AUTHORIZATION_REQUIRED', + message: 'Remote attachment authorization required.' + } + }) + return + } + await routingReady() const attachment = await readVerifiedRemoteAttachment( attachmentRoute[1] as string, attachmentRoute[2] as string, @@ -533,16 +515,59 @@ export async function startRemoteGateway({ connection: 'close', 'content-length': attachment.bytes.byteLength, 'content-type': attachment.mimeType, - 'x-content-type-options': 'nosniff' + 'x-content-type-options': 'nosniff', + [REMOTE_GATEWAY_RESPONSE_AUTH_HEADER]: remoteAttachmentResponseAuthorization( + gatewayToken, + url.searchParams.get('access') as string, + 200, + attachment.bytes + ) }) response.end(attachment.bytes) return } + const requestAuthorization = challenges.authorize( + request.headers, + request.method || 'POST', + `${url.pathname}${url.search}` + ) + if (!requestAuthorization) { + sendJson(response, 403, { + error: { + code: 'REMOTE_CREDENTIAL_REQUIRED', + message: 'Remote Markover credential proof required.' + } + }) + return + } + responseAuthorization = { + nonce: requestAuthorization.nonce, + token: gatewayToken + } + + if (url.search || !allowedRemotePath(request.method, url.pathname)) { + sendJson(response, 404, { + error: { code: 'NOT_FOUND', message: 'Route not found.' } + }, responseAuthorization) + return + } + + await routingReady() + const attempt = url.pathname === '/reviews' ? validatedAttemptHeaders(request) : null const requestBytes = await readRequestBytes(request) + if (remoteContentDigest(requestBytes) !== requestAuthorization.contentDigest) { + sendJson(response, 400, { + error: { + code: 'REMOTE_CONTENT_DIGEST_MISMATCH', + message: 'Remote request content digest does not match.' + } + }, responseAuthorization) + return + } let internalPath = url.pathname let internalBody = requestBytes let internalHeaders: Record = {} @@ -589,9 +614,10 @@ export async function startRemoteGateway({ body, loadAttachment ) + body = authorizeProjectedAttachmentUrls(body, attachmentAccess) } assertResponseBound(body, maximumResponseBytes) - sendJson(response, proxied.statusCode, body) + sendJson(response, proxied.statusCode, body, responseAuthorization) } catch (error) { const code = errorCode(error) const statusCode = code === 'BODY_TOO_LARGE' @@ -609,7 +635,7 @@ export async function startRemoteGateway({ code: typeof code === 'string' ? code : 'REMOTE_GATEWAY_FAILURE', message: error instanceof Error ? error.message : String(error) } - }) + }, responseAuthorization) } finally { resolveActive() activeRequest = null @@ -622,25 +648,30 @@ export async function startRemoteGateway({ server.requestTimeout = REQUEST_TIMEOUT_MILLISECONDS server.headersTimeout = REQUEST_TIMEOUT_MILLISECONDS - let startedSocketIdentity: SocketIdentity | null = null try { await new Promise((resolve, reject) => { server.once('error', reject) - server.listen(socketPath, resolve) + server.listen({ + exclusive: true, + host: REMOTE_GATEWAY_HOST, + port + }, resolve) }) - await fs.chmod(socketPath, 0o600) - const stats = await fs.lstat(socketPath) - if (!stats.isSocket() || stats.uid !== uid) { + const address = server.address() + if ( + !address || + typeof address === 'string' || + address.address !== REMOTE_GATEWAY_HOST + ) { throw gatewayError( - 'REMOTE_GATEWAY_SOCKET_INVALID', - 'The remote gateway did not create an owned Unix socket.' + 'REMOTE_GATEWAY_BIND_INVALID', + 'The remote gateway did not bind the exact loopback address.' ) } - const socketIdentity = { dev: stats.dev, ino: stats.ino } - startedSocketIdentity = socketIdentity let closePromise: Promise | null = null return { - socketPath, + host: REMOTE_GATEWAY_HOST, + port: address.port, close: () => { accepting = false closePromise ||= new Promise((resolve, reject) => { @@ -648,7 +679,6 @@ export async function startRemoteGateway({ server.closeIdleConnections() }).then(async () => { await activeRequest - await removeOwnedSocket(socketPath, socketIdentity, uid) }) return closePromise } @@ -662,8 +692,11 @@ export async function startRemoteGateway({ } server.close(() => { resolve() }) }) - if (startedSocketIdentity) { - await removeOwnedSocket(socketPath, startedSocketIdentity, uid) + if (errorCode(error) === 'EADDRINUSE') { + throw gatewayError( + 'REMOTE_GATEWAY_IN_USE', + 'Another process is already listening on the remote gateway port.' + ) } throw error } diff --git a/src/remote-profile.ts b/src/remote-profile.ts index b122b010..ce6ee505 100644 --- a/src/remote-profile.ts +++ b/src/remote-profile.ts @@ -1,9 +1,12 @@ import fs from 'node:fs/promises' +import { CAPABILITY_TOKEN_PATTERN } from './service-endpoint' + export const REMOTE_PROFILE_ENVIRONMENT_VARIABLE = 'MARKOVER_REMOTE_PROFILE' export interface RemoteProfile { baseUrl: string + token: string } export class RemoteProfileError extends Error { @@ -26,8 +29,10 @@ function invalidProfile(): RemoteProfileError { export function parseRemoteProfile(value: unknown): RemoteProfile { if ( !isRecord(value) || - Object.keys(value).length !== 1 || - typeof value.baseUrl !== 'string' + Object.keys(value).length !== 2 || + typeof value.baseUrl !== 'string' || + typeof value.token !== 'string' || + !CAPABILITY_TOKEN_PATTERN.test(value.token) ) { throw invalidProfile() } @@ -65,23 +70,40 @@ export function parseRemoteProfile(value: unknown): RemoteProfile { throw invalidProfile() } - return { baseUrl: url.href } + return { baseUrl: url.href, token: value.token } } export interface LoadRemoteProfileOptions { environment?: NodeJS.ProcessEnv + inspectFile?: (profilePath: string) => Promise<{ + isFile: () => boolean + isSymbolicLink: () => boolean + mode: number + uid: number + }> + platform?: NodeJS.Platform readFile?: (profilePath: string) => Promise + uid?: number } export async function loadRemoteProfile({ environment = process.env, - readFile = async (profilePath) => fs.readFile(profilePath, 'utf8') + inspectFile = (profilePath) => fs.lstat(profilePath), + platform = process.platform, + readFile = async (profilePath) => fs.readFile(profilePath, 'utf8'), + uid = typeof process.getuid === 'function' ? process.getuid() : -1 }: LoadRemoteProfileOptions = {}): Promise { const profilePath = environment[REMOTE_PROFILE_ENVIRONMENT_VARIABLE] if (profilePath === undefined || profilePath === '') return null let value: unknown try { + const stats = await inspectFile(profilePath) + if ( + !stats.isFile() || + stats.isSymbolicLink() || + (platform !== 'win32' && (uid < 0 || stats.uid !== uid || (stats.mode & 0o077) !== 0)) + ) throw invalidProfile() value = JSON.parse(await readFile(profilePath)) } catch { throw invalidProfile() diff --git a/test/bootstrap.test.ts b/test/bootstrap.test.ts index a026e844..2f18a686 100644 --- a/test/bootstrap.test.ts +++ b/test/bootstrap.test.ts @@ -60,7 +60,10 @@ test('configured remote author commands bypass app bootstrap on Intel', async () return Promise.resolve('/Applications/Markover.app') }, loadProfile() { - return Promise.resolve({ baseUrl: 'https://canonical.example.ts.net/' }) + return Promise.resolve({ + baseUrl: 'https://canonical.example.ts.net/', + token: 'A'.repeat(43) + }) }, run(args) { commands.push(args || []) diff --git a/test/markover-cli.test.ts b/test/markover-cli.test.ts index 25545fef..fbfec95b 100644 --- a/test/markover-cli.test.ts +++ b/test/markover-cli.test.ts @@ -25,6 +25,7 @@ import type { CanonicalDoctorResult } from '../src/canonical-maintenance' import type { LinkHandlerMutationResult } from '../src/link-handler' import type { AddressedDevelopmentBundle } from '../scripts/development-bundle' import { guidance } from '../src/agent-guidance' +import { createRemoteGatewayChallenge } from '../src/remote-gateway-auth' import { LocalServiceError } from '../src/local-client' import { RemoteClientError } from '../src/remote-client' import { startLocalService, type LocalService } from '../src/local-service' @@ -1282,7 +1283,8 @@ test('remote profile routes all author commands without resolving or starting a let discoveryCalls = 0 const options: ExecuteCommandOptions = { loadRemoteProfile: () => Promise.resolve({ - baseUrl: 'https://canonical.example.ts.net/' + baseUrl: 'https://canonical.example.ts.net/', + token: 'A'.repeat(43) }), readRemoteHealth() { healthReads += 1 @@ -1291,7 +1293,12 @@ test('remote profile routes all author commands without resolving or starting a protocol: { name: 'markover-remote', version: 1 }, role: 'canonical', scheme: 'markover', - discoverAgentThreadFromLocalSessions: false + discoverAgentThreadFromLocalSessions: false, + authorization: createRemoteGatewayChallenge( + 'A'.repeat(43), + Date.now(), + 'N'.repeat(43) + ) }) }, remoteJournalRoot: path.join(directory, 'journal'), @@ -1311,7 +1318,6 @@ test('remote profile routes all author commands without resolving or starting a }, requestRemote(_profile, method, requestPath, body, requestOptions) { assert.ok(requestOptions) - assert.equal(requestOptions.preflight, false) requests.push({ method, path: requestPath, body }) if (requestPath === '/reviews') { assert.equal(typeof requestOptions.headers?.['idempotency-key'], 'string') @@ -1416,14 +1422,20 @@ test('uncertain remote open recovers by key before rereading the source', async let discoveryCalls = 0 const options: ExecuteCommandOptions = { loadRemoteProfile: () => Promise.resolve({ - baseUrl: 'https://canonical.example.ts.net/' + baseUrl: 'https://canonical.example.ts.net/', + token: 'A'.repeat(43) }), readRemoteHealth: () => Promise.resolve({ status: 'ok', protocol: { name: 'markover-remote', version: 1 }, role: 'canonical', scheme: 'markover', - discoverAgentThreadFromLocalSessions: true + discoverAgentThreadFromLocalSessions: true, + authorization: createRemoteGatewayChallenge( + 'A'.repeat(43), + Date.now(), + 'N'.repeat(43) + ) }), remoteJournalRoot: path.join(directory, 'journal'), discoverMetadata() { @@ -1481,14 +1493,20 @@ test('remote open recovers a delayed own attempt through digest history', async let discoveryCalls = 0 const options: ExecuteCommandOptions = { loadRemoteProfile: () => Promise.resolve({ - baseUrl: 'https://canonical.example.ts.net/' + baseUrl: 'https://canonical.example.ts.net/', + token: 'A'.repeat(43) }), readRemoteHealth: () => Promise.resolve({ status: 'ok', protocol: { name: 'markover-remote', version: 1 }, role: 'canonical', scheme: 'markover', - discoverAgentThreadFromLocalSessions: true + discoverAgentThreadFromLocalSessions: true, + authorization: createRemoteGatewayChallenge( + 'A'.repeat(43), + Date.now(), + 'N'.repeat(43) + ) }), remoteJournalRoot: path.join(directory, 'journal'), discoverMetadata() { diff --git a/test/remote-client.test.ts b/test/remote-client.test.ts index 0cc434d8..e250b664 100644 --- a/test/remote-client.test.ts +++ b/test/remote-client.test.ts @@ -5,14 +5,23 @@ import type https from 'node:https' import type { RequestOptions as HttpsRequestOptions } from 'node:https' import test from 'node:test' - import { + readRemoteAttachment, readRemoteHealth, RemoteClientError, type RemoteClientTiming, requestRemoteJson, validateRemoteAttachmentUrls } from '../src/remote-client' +import { + createRemoteAttachmentAccess, + createRemoteGatewayChallenge, + remoteAttachmentResponseAuthorization, + remoteContentDigest, + remoteRequestAuthorization, + remoteResponseAuthorization, + type RemoteGatewayChallenge +} from '../src/remote-gateway-auth' import { loadRemoteProfile, parseRemoteProfile, @@ -20,19 +29,34 @@ import { type RemoteProfile } from '../src/remote-profile' -const profile: RemoteProfile = { baseUrl: 'https://canonical.example.ts.net/' } +const token = 'A'.repeat(43) +const profile: RemoteProfile = { + baseUrl: 'https://canonical.example.ts.net/', + token +} +const fixedNow = Date.now() +const fixedNonce = 'N'.repeat(43) const validHealth = { status: 'ok', protocol: { name: 'markover-remote', version: 1 }, role: 'canonical', scheme: 'markover', - discoverAgentThreadFromLocalSessions: true + discoverAgentThreadFromLocalSessions: true, + authorization: createRemoteGatewayChallenge(token, fixedNow, fixedNonce) +} + +function validAuthorization(): RemoteGatewayChallenge { + return createRemoteGatewayChallenge(token, fixedNow, fixedNonce) } interface ResponsePlan { + attachmentAccess?: string body: unknown contentType?: string + omitResponseAuthorization?: boolean rawBody?: string + rawBytes?: Uint8Array + responseToken?: string statusCode: number } @@ -92,18 +116,44 @@ function fakeTransport(plans: readonly Plan[]): { if ('stall' in plan) return const responseEvents = new EventEmitter() + const responseBody = Buffer.from( + plan.rawBytes ?? plan.rawBody ?? JSON.stringify(plan.body) + ) + const requestHeaders = options.headers as Record | undefined + const requestAuthorization = requestHeaders?.authorization + const nonce = typeof requestAuthorization === 'string' + ? /^Markover-HMAC-v1 ([A-Za-z0-9_-]{43})\./.exec(requestAuthorization)?.[1] + : undefined + const responseAuthorization = plan.omitResponseAuthorization + ? undefined + : nonce !== undefined + ? remoteResponseAuthorization( + plan.responseToken ?? token, + nonce, + plan.statusCode, + responseBody + ) + : plan.attachmentAccess !== undefined + ? remoteAttachmentResponseAuthorization( + plan.responseToken ?? token, + plan.attachmentAccess, + plan.statusCode, + responseBody + ) + : undefined const response = Object.assign(responseEvents, { complete: true, headers: { - 'content-type': plan.contentType ?? 'application/json; charset=utf-8' + 'content-type': plan.contentType ?? 'application/json; charset=utf-8', + ...(responseAuthorization === undefined + ? {} + : { 'markover-response-auth': responseAuthorization }) }, resume: () => response, statusCode: plan.statusCode }) callback(response as unknown as IncomingMessage) - responseEvents.emit('data', Buffer.from( - plan.rawBody ?? JSON.stringify(plan.body) - )) + responseEvents.emit('data', responseBody) responseEvents.emit('end') }) return request @@ -179,17 +229,26 @@ test('remote profile is opt-in and accepts only an exact root HTTPS ts.net endpo let readPath = '' assert.deepEqual(await loadRemoteProfile({ environment: { MARKOVER_REMOTE_PROFILE: '/profiles/canonical.json' }, + inspectFile: () => Promise.resolve({ + isFile: () => true, + isSymbolicLink: () => false, + mode: 0o600, + uid: 42 + }), readFile: (profilePath) => { readPath = profilePath return Promise.resolve(JSON.stringify({ - baseUrl: 'https://Canonical.Example.ts.net' + baseUrl: 'https://Canonical.Example.ts.net', + token })) - } - }), { baseUrl: 'https://canonical.example.ts.net/' }) + }, + uid: 42 + }), { baseUrl: 'https://canonical.example.ts.net/', token }) assert.equal(readPath, '/profiles/canonical.json') assert.deepEqual(parseRemoteProfile({ - baseUrl: 'https://canonical.example.ts.net:8443/' - }), { baseUrl: 'https://canonical.example.ts.net:8443/' }) + baseUrl: 'https://canonical.example.ts.net:8443/', + token + }), { baseUrl: 'https://canonical.example.ts.net:8443/', token }) for (const baseUrl of [ 'http://canonical.example.ts.net/', @@ -203,17 +262,62 @@ test('remote profile is opt-in and accepts only an exact root HTTPS ts.net endpo 'https://example.com/' ]) { assert.throws( - () => parseRemoteProfile({ baseUrl }), + () => parseRemoteProfile({ baseUrl, token }), (error: unknown) => error instanceof RemoteProfileError, baseUrl ) } assert.throws(() => parseRemoteProfile({ baseUrl: profile.baseUrl, + token, redirectUrl: 'https://other.example.ts.net/' }), RemoteProfileError) }) +test('remote profile requires a regular owner-only file', async () => { + const cases = [ + { + name: 'unsafe mode', + mode: 0o640, + isSymbolicLink: false, + uid: 42 + }, + { + name: 'symlink', + mode: 0o600, + isSymbolicLink: true, + uid: 42 + }, + { + name: 'unowned file', + mode: 0o600, + isSymbolicLink: false, + uid: 7 + } + ] + for (const file of cases) { + await assert.rejects( + loadRemoteProfile({ + environment: { MARKOVER_REMOTE_PROFILE: '/profiles/canonical.json' }, + inspectFile: () => Promise.resolve({ + isFile: () => true, + isSymbolicLink: () => file.isSymbolicLink, + mode: file.mode, + uid: file.uid + }), + platform: 'darwin', + readFile: () => Promise.resolve(JSON.stringify({ + baseUrl: profile.baseUrl, + token + })), + uid: 42 + }), + (error: unknown) => error instanceof RemoteProfileError, + file.name + ) + } +}) + test('health pins protocol identity and exposes the boolean discovery snapshot', async () => { const transport = fakeTransport([{ statusCode: 200, body: validHealth }]) const health = await readRemoteHealth(profile, { request: transport.request }) @@ -225,10 +329,16 @@ test('health pins protocol identity and exposes the boolean discovery snapshot', assert.equal(request.options.port, 443) assert.equal(request.options.path, '/health') assert.equal(request.options.agent, false) + assert.equal( + (request.options.headers as Record | undefined)?.authorization, + undefined + ) + assert.doesNotMatch(JSON.stringify(request.options.headers), new RegExp(token)) const portTransport = fakeTransport([{ statusCode: 200, body: validHealth }]) await readRemoteHealth({ - baseUrl: 'https://canonical.example.ts.net:8443/' + baseUrl: 'https://canonical.example.ts.net:8443/', + token }, { request: portTransport.request }) assert.equal(portTransport.captured[0]?.options.port, 8443) @@ -237,13 +347,29 @@ test('health pins protocol identity and exposes the boolean discovery snapshot', { ...validHealth, protocol: { name: 'markover-remote', version: 2 } }, { ...validHealth, role: 'development' }, { ...validHealth, scheme: 'markover-dev' }, - { ...validHealth, discoverAgentThreadFromLocalSessions: 'yes' } + { ...validHealth, discoverAgentThreadFromLocalSessions: 'yes' }, + { + ...validHealth, + authorization: { + ...validHealth.authorization, + proof: '0'.repeat(64) + } + }, + { + ...validHealth, + authorization: createRemoteGatewayChallenge( + token, + fixedNow - 60_000, + fixedNonce + ) + } ]) { const rejected = fakeTransport([{ statusCode: 200, body: incompatible }]) await assert.rejects( readRemoteHealth(profile, { request: rejected.request }), (error: unknown) => hasRemoteError(error, 'INCOMPATIBLE_REMOTE_CANONICAL', 200) ) + assert.equal(rejected.captured.length, 1) } }) @@ -258,13 +384,48 @@ test('remote requests preflight without leaking private headers and bound respon '/reviews', { source: '# Review' }, { - headers: { 'idempotency-key': 'private-key' }, + headers: { + authorization: 'Bearer attacker-controlled', + 'idempotency-key': 'private-key', + 'markover-content-digest': 'sha256:attacker-controlled', + 'x-private-header': 'private-value' + }, request: transport.request } ), { reviewId: 'mko_12345678' }) assert.equal(transport.captured.length, 2) + assert.doesNotMatch( + JSON.stringify(transport.captured[0]?.options.headers), + new RegExp(token) + ) + assert.doesNotMatch( + JSON.stringify(transport.captured[1]?.options.headers), + new RegExp(token) + ) + assert.equal( + (transport.captured[0]?.options.headers as Record | undefined)?.authorization, + undefined + ) assert.equal(transport.captured[0]?.options.headers && Object.hasOwn(transport.captured[0].options.headers, 'idempotency-key'), false) + assert.equal(transport.captured[0]?.options.headers && + Object.hasOwn(transport.captured[0].options.headers, 'x-private-header'), false) + assert.equal( + (transport.captured[1]?.options.headers as Record | undefined)?.authorization, + remoteRequestAuthorization( + token, + validHealth.authorization, + 'POST', + '/reviews', + remoteContentDigest(Buffer.from(JSON.stringify({ source: '# Review' }))) + ) + ) + assert.equal( + (transport.captured[1]?.options.headers as Record | undefined)?.[ + 'markover-content-digest' + ] ?? '', + remoteContentDigest(Buffer.from(JSON.stringify({ source: '# Review' }))) + ) assert.equal( (transport.captured[1]?.options.headers as Record | undefined)?.[ 'idempotency-key' @@ -279,6 +440,7 @@ test('remote requests preflight without leaking private headers and bound respon }]) await assert.rejects( requestRemoteJson(profile, 'GET', '/reviews', null, { + authorization: validAuthorization(), maximumResponseBytes: 8, preflight: false, request: oversized.request @@ -286,6 +448,30 @@ test('remote requests preflight without leaking private headers and bound respon (error: unknown) => hasRemoteError(error, 'RESPONSE_TOO_LARGE', 200) ) assert.equal(oversized.captured[0]?.destroyed, true) + + for (const responsePlan of [ + { + statusCode: 200, + body: { reviews: [] }, + omitResponseAuthorization: true + }, + { + statusCode: 200, + body: { reviews: [] }, + responseToken: 'X'.repeat(43) + } + ]) { + const forged = fakeTransport([responsePlan]) + await assert.rejects( + requestRemoteJson(profile, 'GET', '/reviews', null, { + authorization: validAuthorization(), + mutation: false, + preflight: false, + request: forged.request + }), + (error: unknown) => hasRemoteError(error, 'INVALID_RESPONSE', 200) + ) + } }) test('remote client does not follow redirects and preserves structured service errors', async () => { @@ -298,6 +484,7 @@ test('remote client does not follow redirects and preserves structured service e const conflict = fakeTransport([{ statusCode: 409, body: { error: details } }]) await assert.rejects( requestRemoteJson(profile, 'POST', '/reviews', {}, { + authorization: validAuthorization(), preflight: false, request: conflict.request }), @@ -315,6 +502,7 @@ test('remote client does not follow redirects and preserves structured service e }]) await assert.rejects( requestRemoteJson(profile, 'GET', '/health', null, { + authorization: validAuthorization(), preflight: false, request: redirect.request }), @@ -333,6 +521,7 @@ test('remote client does not follow redirects and preserves structured service e }]) await assert.rejects( requestRemoteJson(profile, 'POST', '/reviews/pending', {}, { + authorization: validAuthorization(), preflight: false, request: uncertain.request }), @@ -345,6 +534,7 @@ test('remote client does not follow redirects and preserves structured service e }]) await assert.rejects( requestRemoteJson(profile, 'POST', '/reviews/mko_missing/edit', {}, { + authorization: validAuthorization(), preflight: false, request: notFound.request }), @@ -360,6 +550,7 @@ test('remote client rejects malformed JSON, content types, errors, and request b }]) await assert.rejects( requestRemoteJson(profile, 'GET', '/reviews', null, { + authorization: validAuthorization(), preflight: false, request: malformedJson.request }), @@ -373,6 +564,7 @@ test('remote client rejects malformed JSON, content types, errors, and request b }]) await assert.rejects( requestRemoteJson(profile, 'POST', '/reviews', {}, { + authorization: validAuthorization(), preflight: false, request: malformedMutation.request }), @@ -386,6 +578,7 @@ test('remote client rejects malformed JSON, content types, errors, and request b }]) await assert.rejects( requestRemoteJson(profile, 'GET', '/reviews', null, { + authorization: validAuthorization(), preflight: false, request: wrongContentType.request }), @@ -398,6 +591,7 @@ test('remote client rejects malformed JSON, content types, errors, and request b }]) await assert.rejects( requestRemoteJson(profile, 'GET', '/reviews', null, { + authorization: validAuthorization(), preflight: false, request: malformedError.request }), @@ -405,7 +599,10 @@ test('remote client rejects malformed JSON, content types, errors, and request b ) await assert.rejects( - requestRemoteJson(profile, 'POST', '/reviews', 1n, { preflight: false }), + requestRemoteJson(profile, 'POST', '/reviews', 1n, { + authorization: validAuthorization(), + preflight: false + }), (error: unknown) => hasRemoteError(error, 'INVALID_REMOTE_REQUEST') ) }) @@ -414,6 +611,7 @@ test('remote transport failures distinguish pre-send failure from uncertain muta const preSend = fakeTransport([{ error: true, afterSend: false }]) await assert.rejects( requestRemoteJson(profile, 'POST', '/reviews', {}, { + authorization: validAuthorization(), preflight: false, request: preSend.request }), @@ -423,6 +621,7 @@ test('remote transport failures distinguish pre-send failure from uncertain muta const afterSend = fakeTransport([{ error: true, afterSend: true }]) await assert.rejects( requestRemoteJson(profile, 'POST', '/reviews', {}, { + authorization: validAuthorization(), preflight: false, request: afterSend.request }), @@ -434,6 +633,7 @@ test('connect and response timeouts use independently injectable bounds', async const connectTransport = fakeTransport([{ stall: true, afterSend: false }]) const connectTiming = manualTiming() const connectPromise = requestRemoteJson(profile, 'POST', '/reviews', {}, { + authorization: validAuthorization(), connectTimeoutMilliseconds: 11, preflight: false, request: connectTransport.request, @@ -450,6 +650,7 @@ test('connect and response timeouts use independently injectable bounds', async const responseTransport = fakeTransport([{ stall: true, afterSend: true }]) const responseTiming = manualTiming() const responsePromise = requestRemoteJson(profile, 'POST', '/reviews', {}, { + authorization: validAuthorization(), connectTimeoutMilliseconds: 33, preflight: false, request: responseTransport.request, @@ -465,12 +666,19 @@ test('connect and response timeouts use independently injectable bounds', async }) test('remote attachment URLs stay on the pinned canonical HTTPS origin', () => { + const expiresAt = Date.now() + 60_000 + const access = createRemoteAttachmentAccess( + token, + 'mko_aaa11111', + 'img-1', + expiresAt + ) const artifact = { review: { id: 'mko_aaa11111' }, root: { attachments: [{ id: 'img-1', - url: '/reviews/mko_aaa11111/attachments/img-1' + url: `/reviews/mko_aaa11111/attachments/img-1?access=${access}` }], children: [] } @@ -478,14 +686,43 @@ test('remote attachment URLs stay on the pinned canonical HTTPS origin', () => { assert.equal(validateRemoteAttachmentUrls(profile, artifact), artifact) assert.equal( artifact.root.attachments[0]?.url, - 'https://canonical.example.ts.net/reviews/mko_aaa11111/attachments/img-1' + `https://canonical.example.ts.net/reviews/mko_aaa11111/attachments/img-1?access=${access}` ) for (const attachment of [ - { id: 'img-1', path: '/private/img-1.png', url: '/reviews/mko_aaa11111/attachments/img-1' }, - { id: 'img-1', url: 'https://other.example.ts.net/reviews/mko_aaa11111/attachments/img-1' }, - { id: 'img-1', url: 'https://canonical.example.ts.net/reviews/mko_other111/attachments/img-1' }, - { id: 'img-1', url: '/reviews/mko_aaa11111/attachments/img-2' }, - { id: 'img-1', url: '//other.example.ts.net/reviews/mko_aaa11111/attachments/img-1' } + { + id: 'img-1', + path: '/private/img-1.png', + url: `/reviews/mko_aaa11111/attachments/img-1?access=${access}` + }, + { + id: 'img-1', + url: `https://other.example.ts.net/reviews/mko_aaa11111/attachments/img-1?access=${access}` + }, + { + id: 'img-1', + url: `https://canonical.example.ts.net/reviews/mko_other111/attachments/img-1?access=${access}` + }, + { + id: 'img-1', + url: `/reviews/mko_aaa11111/attachments/img-2?access=${access}` + }, + { + id: 'img-1', + url: `/reviews/mko_aaa11111/attachments/img-1?access=${access}&extra=1` + }, + { + id: 'img-1', + url: `/reviews/mko_aaa11111/attachments/img-1?access=${createRemoteAttachmentAccess( + token, + 'mko_aaa11111', + 'img-1', + Date.now() - 1 + )}` + }, + { + id: 'img-1', + url: '/reviews/mko_aaa11111/attachments/img-1' + } ]) { assert.throws( () => validateRemoteAttachmentUrls(profile, { @@ -496,3 +733,76 @@ test('remote attachment URLs stay on the pinned canonical HTTPS origin', () => { ) } }) + +test('remote attachment retrieval verifies the scoped response and checksum', async () => { + const bytes = Buffer.concat([ + Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]), + Buffer.from('checked') + ]) + const access = createRemoteAttachmentAccess( + token, + 'mko_aaa11111', + 'img-1', + Date.now() + 60_000 + ) + const attachment = { + checksum: remoteContentDigest(bytes), + id: 'img-1', + mimeType: 'image/png', + url: `/reviews/mko_aaa11111/attachments/img-1?access=${access}` + } + const valid = fakeTransport([{ + attachmentAccess: access, + body: null, + contentType: 'image/png', + rawBytes: bytes, + statusCode: 200 + }]) + assert.deepEqual(await readRemoteAttachment( + profile, + 'mko_aaa11111', + attachment, + { request: valid.request } + ), bytes) + const captured = valid.captured[0] + assert.ok(captured) + assert.equal( + captured.options.path, + `/reviews/mko_aaa11111/attachments/img-1?access=${access}` + ) + assert.doesNotMatch(JSON.stringify(captured.options.headers), new RegExp(token)) + + for (const plan of [ + { + attachmentAccess: access, + body: null, + contentType: 'image/png', + rawBytes: Buffer.from('tampered'), + statusCode: 200 + }, + { + attachmentAccess: access, + body: null, + contentType: 'image/png', + rawBytes: bytes, + responseToken: 'X'.repeat(43), + statusCode: 200 + }, + { + attachmentAccess: access, + body: null, + contentType: 'image/png', + omitResponseAuthorization: true, + rawBytes: bytes, + statusCode: 200 + } + ]) { + const rejected = fakeTransport([plan]) + await assert.rejects( + readRemoteAttachment(profile, 'mko_aaa11111', attachment, { + request: rejected.request + }), + (error: unknown) => hasRemoteError(error, 'INVALID_RESPONSE', 200) + ) + } +}) diff --git a/test/remote-gateway.test.ts b/test/remote-gateway.test.ts index adccf670..5214daa3 100644 --- a/test/remote-gateway.test.ts +++ b/test/remote-gateway.test.ts @@ -1,5 +1,4 @@ import assert from 'node:assert/strict' -import { spawnSync } from 'node:child_process' import { createHash } from 'node:crypto' import fs from 'node:fs/promises' import http from 'node:http' @@ -20,12 +19,29 @@ import { remoteGatewayHostEligible, REMOTE_GATEWAY_CAPABILITY, REMOTE_GATEWAY_CAPABILITY_HEADER, + REMOTE_GATEWAY_HOST, REMOTE_GATEWAY_IDEMPOTENCY_HEADER, REMOTE_GATEWAY_REQUEST_DIGEST_HEADER, RemoteGatewayError, startRemoteGateway, type RemoteGateway } from '../src/remote-gateway' +import { + createRemoteAttachmentAccess, + remoteContentDigest, + remoteRequestAuthorization, + RemoteAttachmentAccessStore, + type RemoteGatewayChallenge, + RemoteGatewayChallengeStore, + REMOTE_GATEWAY_CONTENT_DIGEST_HEADER, + verifyRemoteAttachmentResponseAuthorization, + verifyRemoteGatewayChallenge +} from '../src/remote-gateway-auth' +import { + loadOrCreateRemoteGatewayCredential, + RemoteGatewayCredentialError, + remoteGatewayCredentialPath +} from '../src/remote-gateway-credential' import { projectRemoteAttachments, readVerifiedRemoteAttachment @@ -41,10 +57,12 @@ interface GatewayFixture { directory: string gateway: RemoteGateway routingChecks: () => number - socketPath: string + port: number store: ReviewStore } +const GATEWAY_TOKEN = 'G'.repeat(43) + function tree(): ReviewTree { const source = '# Remote review\n\nReview this from the remote client host.\n' return parseMarkdown(source, reviewChecksum(source), { @@ -81,8 +99,8 @@ function createHeaders(bytes: Uint8Array): Record { } } -async function requestGateway( - socketPath: string, +async function requestGatewayRaw( + port: number, method: string, requestPath: string, headers: Record = {}, @@ -90,7 +108,8 @@ async function requestGateway( ): Promise<{ body: unknown; statusCode: number | undefined }> { return new Promise((resolve, reject) => { const request = http.request({ - socketPath, + host: REMOTE_GATEWAY_HOST, + port, method, path: requestPath, headers: { @@ -113,14 +132,87 @@ async function requestGateway( }) } +async function requestGateway( + port: number, + method: string, + requestPath: string, + headers: Record = {}, + body: Uint8Array = Buffer.alloc(0) +): Promise<{ body: unknown; headers: http.IncomingHttpHeaders; statusCode: number | undefined }> { + if ( + method === 'GET' && requestPath === '/health' || + !Object.hasOwn(headers, REMOTE_GATEWAY_CAPABILITY_HEADER) + ) { + const response = await requestGatewayRaw(port, method, requestPath, headers, body) + return { ...response, headers: {} } + } + const health = await requestGatewayRaw( + port, + 'GET', + '/health', + capabilityHeader() + ) + assert.equal(health.statusCode, 200) + assert.ok(health.body && typeof health.body === 'object') + const challenge = (health.body as Record).authorization + assert.equal(verifyRemoteGatewayChallenge(GATEWAY_TOKEN, challenge), true) + const contentDigest = remoteContentDigest(body) + return requestGatewayWithResponseHeaders(port, method, requestPath, { + ...headers, + authorization: remoteRequestAuthorization( + GATEWAY_TOKEN, + challenge as RemoteGatewayChallenge, + method, + requestPath, + contentDigest + ), + [REMOTE_GATEWAY_CONTENT_DIGEST_HEADER]: contentDigest + }, body) +} + +async function requestGatewayWithResponseHeaders( + port: number, + method: string, + requestPath: string, + headers: Record = {}, + body: Uint8Array = Buffer.alloc(0) +): Promise<{ body: unknown; headers: http.IncomingHttpHeaders; statusCode: number | undefined }> { + return new Promise((resolve, reject) => { + const request = http.request({ + host: REMOTE_GATEWAY_HOST, + port, + method, + path: requestPath, + headers: { + 'content-length': body.byteLength, + ...headers + } + }, (response) => { + const chunks: Uint8Array[] = [] + response.on('data', (chunk: Buffer) => { chunks.push(chunk) }) + response.on('end', () => { + const bytes = Buffer.concat(chunks) + resolve({ + body: bytes.byteLength ? JSON.parse(bytes.toString('utf8')) : null, + headers: response.headers, + statusCode: response.statusCode + }) + }) + }) + request.on('error', reject) + request.end(body) + }) +} + async function requestGatewayBytes( - socketPath: string, + port: number, requestPath: string, headers: Record = {} ): Promise<{ body: Buffer; headers: http.IncomingHttpHeaders; statusCode: number | undefined }> { return new Promise((resolve, reject) => { const request = http.request({ - socketPath, + host: REMOTE_GATEWAY_HOST, + port, method: 'GET', path: requestPath, headers @@ -141,13 +233,14 @@ async function requestGatewayBytes( } async function interruptGatewayResponse( - socketPath: string, + port: number, requestPath: string, headers: Record ): Promise { return new Promise((resolve, reject) => { const request = http.request({ - socketPath, + host: REMOTE_GATEWAY_HOST, + port, method: 'GET', path: requestPath, headers @@ -187,10 +280,9 @@ async function gatewayFixture( } }) let routingChecks = 0 - const socketPath = path.join(directory, 'state', 'remote.sock') const attachments = new InternalAttachmentAllowlist(store.directory) const gateway = await startRemoteGateway({ - socketPath, + gatewayToken: GATEWAY_TOKEN, localPort: service.port, localToken: identity.token, discoveryPolicy: () => options.discoveryPolicy ?? false, @@ -217,8 +309,9 @@ async function gatewayFixture( attachmentRoot: path.join(store.directory, reviewId, 'attachments'), filePath } - : null - } + : null + }, + port: 0 }) t.after(async () => { await gateway.close() @@ -229,8 +322,8 @@ async function gatewayFixture( changes, directory, gateway, + port: gateway.port, routingChecks: () => routingChecks, - socketPath, store } } @@ -311,12 +404,80 @@ test('capability parser accepts one exact forwarded app capability', () => { }), false) }) -test('gateway authenticates before routes and bodies and exposes bounded health', async (t) => { +test('gateway challenges prove credential ownership and reject replay and expiry', () => { + let now = 1_000_000 + const store = new RemoteGatewayChallengeStore(GATEWAY_TOKEN, () => now) + const challenge = store.create() + assert.equal(verifyRemoteGatewayChallenge(GATEWAY_TOKEN, challenge, now), true) + assert.equal(verifyRemoteGatewayChallenge('X'.repeat(43), challenge, now), false) + + const bytes = Buffer.from('{"review":true}') + const contentDigest = remoteContentDigest(bytes) + const authorization = remoteRequestAuthorization( + GATEWAY_TOKEN, + challenge, + 'POST', + '/reviews', + contentDigest + ) + const headers = { + authorization, + [REMOTE_GATEWAY_CONTENT_DIGEST_HEADER]: contentDigest + } + assert.deepEqual(store.authorize(headers, 'POST', '/reviews'), { + contentDigest, + nonce: challenge.nonce + }) + assert.equal(store.authorize(headers, 'POST', '/reviews'), null) + + const wrongPathChallenge = store.create() + const wrongPathHeaders = { + authorization: remoteRequestAuthorization( + GATEWAY_TOKEN, + wrongPathChallenge, + 'POST', + '/reviews', + contentDigest + ), + [REMOTE_GATEWAY_CONTENT_DIGEST_HEADER]: contentDigest + } + assert.equal(store.authorize(wrongPathHeaders, 'POST', '/reviews/pending'), null) + assert.deepEqual(store.authorize(wrongPathHeaders, 'POST', '/reviews'), { + contentDigest, + nonce: wrongPathChallenge.nonce + }) + + const expired = store.create() + now = expired.expiresAt + assert.equal(verifyRemoteGatewayChallenge(GATEWAY_TOKEN, expired, now), false) + assert.equal(store.authorize({ + authorization: remoteRequestAuthorization( + GATEWAY_TOKEN, + expired, + 'POST', + '/reviews', + contentDigest + ), + [REMOTE_GATEWAY_CONTENT_DIGEST_HEADER]: contentDigest + }, 'POST', '/reviews'), null) +}) + +test('attachment access is scoped to the issuing gateway instance', () => { + const now = Date.now() + const issuingGateway = new RemoteAttachmentAccessStore(GATEWAY_TOKEN, () => now) + const restartedGateway = new RemoteAttachmentAccessStore(GATEWAY_TOKEN, () => now) + const access = issuingGateway.create('mko_aaa11111', 'img-1') + assert.equal(issuingGateway.verify('mko_aaa11111', 'img-1', access), true) + assert.equal(issuingGateway.verify('mko_aaa11111', 'img-2', access), false) + assert.equal(restartedGateway.verify('mko_aaa11111', 'img-1', access), false) +}) + +test('gateway checks capability and proof before routes and bodies and exposes bounded health', async (t) => { const fixture = await gatewayFixture(t, { discoveryPolicy: true }) const checksAfterStartup = fixture.routingChecks() const denied = await requestGateway( - fixture.socketPath, + fixture.port, 'POST', '/reviews', {}, @@ -329,22 +490,40 @@ test('gateway authenticates before routes and bodies and exposes bounded health' message: 'Remote Markover capability required.' } }) + const capabilityOnly = { + [REMOTE_GATEWAY_CAPABILITY_HEADER]: capabilityHeader()[ + REMOTE_GATEWAY_CAPABILITY_HEADER + ] as string + } + const missingCredential = await requestGatewayRaw( + fixture.port, + 'POST', + '/reviews', + capabilityOnly, + Buffer.from('{') + ) + assert.equal(missingCredential.statusCode, 403) + assert.equal(responseErrorCode(missingCredential.body), 'REMOTE_CREDENTIAL_REQUIRED') assert.equal(fixture.routingChecks(), checksAfterStartup) const health = await requestGateway( - fixture.socketPath, + fixture.port, 'GET', '/health', capabilityHeader() ) assert.equal(health.statusCode, 200) - assert.deepEqual(health.body, { - status: 'ok', - protocol: { name: 'markover-remote', version: 1 }, - role: 'canonical', - scheme: 'markover', - discoverAgentThreadFromLocalSessions: true - }) + assert.ok(health.body && typeof health.body === 'object') + const healthBody = health.body as Record + assert.equal(healthBody.status, 'ok') + assert.deepEqual(healthBody.protocol, { name: 'markover-remote', version: 1 }) + assert.equal(healthBody.role, 'canonical') + assert.equal(healthBody.scheme, 'markover') + assert.equal(healthBody.discoverAgentThreadFromLocalSessions, true) + assert.equal( + verifyRemoteGatewayChallenge(GATEWAY_TOKEN, healthBody.authorization), + true + ) assert.doesNotMatch(JSON.stringify(health.body), /path|process|instanceId|port/) for (const route of [ @@ -357,7 +536,7 @@ test('gateway authenticates before routes and bodies and exposes bounded health' ['GET', '/health?details=1'] ] as const) { const response = await requestGateway( - fixture.socketPath, + fixture.port, route[0], route[1], capabilityHeader(), @@ -382,7 +561,7 @@ test('remote create pins origin, recovers by key, and returns canonical URLs', a })) const created = await requestGateway( - fixture.socketPath, + fixture.port, 'POST', '/reviews', createHeaders(body), @@ -405,7 +584,7 @@ test('remote create pins origin, recovers by key, and returns canonical URLs', a assert.deepEqual(fixture.changes, ['created']) const recovered = await requestGateway( - fixture.socketPath, + fixture.port, 'POST', '/reviews', createHeaders(body) @@ -424,7 +603,7 @@ test('remote create pins origin, recovers by key, and returns canonical URLs', a metadata: { contextSummary: 'A conflicting retry.' } })) const conflict = await requestGateway( - fixture.socketPath, + fixture.port, 'POST', '/reviews', createHeaders(conflictingBody), @@ -439,7 +618,7 @@ test('remote create pins origin, recovers by key, and returns canonical URLs', a assert.equal(conflictError.creationReceipt.requestDigest, digest(body)) const handedOff = await requestGateway( - fixture.socketPath, + fixture.port, 'POST', '/reviews/mko_aaa11111/handoff', capabilityHeader() @@ -448,7 +627,7 @@ test('remote create pins origin, recovers by key, and returns canonical URLs', a assert.equal((handedOff.body as ReviewArtifact).review.status, 'pending-agent') const pending = await requestGateway( - fixture.socketPath, + fixture.port, 'POST', '/reviews/pending', capabilityHeader(), @@ -484,7 +663,7 @@ test('remote response headroom carries every maximum-size create through handoff assert.equal(MAXIMUM_REMOTE_RESPONSE_BYTES, MAXIMUM_BODY_BYTES * 2) const created = await requestGateway( - fixture.socketPath, + fixture.port, 'POST', '/reviews', createHeaders(body), @@ -493,7 +672,7 @@ test('remote response headroom carries every maximum-size create through handoff assert.equal(created.statusCode, 201) const handedOff = await requestGateway( - fixture.socketPath, + fixture.port, 'POST', '/reviews/mko_aaa11111/handoff', capabilityHeader() @@ -511,7 +690,7 @@ test('remote create rejects origin claims, attachment metadata, and digest drift ] as const) { const bytes = Buffer.from(JSON.stringify(body)) const response = await requestGateway( - fixture.socketPath, + fixture.port, 'POST', '/reviews', createHeaders(bytes), @@ -526,7 +705,7 @@ test('remote create rejects origin claims, attachment metadata, and digest drift metadata: { contextSummary: 'Digest mismatch.' } })) const mismatched = await requestGateway( - fixture.socketPath, + fixture.port, 'POST', '/reviews', { @@ -550,7 +729,7 @@ test('remote handoff projects checked private attachments and streams retryable metadata: { contextSummary: 'Check the private screenshot.' } })) const created = await requestGateway( - fixture.socketPath, + fixture.port, 'POST', '/reviews', createHeaders(createBody), @@ -576,7 +755,7 @@ test('remote handoff projects checked private attachments and streams retryable await fixture.store.updateTree('mko_aaa11111', annotated) const handoff = await requestGateway( - fixture.socketPath, + fixture.port, 'POST', '/reviews/mko_aaa11111/handoff', capabilityHeader() @@ -584,39 +763,90 @@ test('remote handoff projects checked private attachments and streams retryable assert.equal(handoff.statusCode, 200) const projected = handoff.body as ReviewArtifact const projectedAttachment = projected.root.children[0]?.attachments?.[0] - assert.deepEqual(projectedAttachment, { - id: 'img-1', - type: 'image', - mimeType: 'image/png', - checksum: digest(png), - url: '/reviews/mko_aaa11111/attachments/img-1' - }) + assert.ok(projectedAttachment) + assert.equal(projectedAttachment.id, 'img-1') + assert.equal(projectedAttachment.type, 'image') + assert.equal(projectedAttachment.mimeType, 'image/png') + assert.equal(projectedAttachment.checksum, digest(png)) + assert.equal(Object.hasOwn(projectedAttachment, 'path'), false) + assert.equal(typeof projectedAttachment.url, 'string') + const projectedUrl = new URL( + projectedAttachment.url as string, + 'http://markover.invalid' + ) + assert.equal( + projectedUrl.pathname, + '/reviews/mko_aaa11111/attachments/img-1' + ) + assert.equal(projectedUrl.searchParams.size, 1) + assert.match(projectedUrl.searchParams.get('access') || '', /^\d{13}\.[a-f0-9]{64}$/) assert.equal( (await fixture.store.load('mko_aaa11111')).root.children[0]?.attachments?.[0]?.path, saved.path ) - const route = '/reviews/mko_aaa11111/attachments/img-1' - const first = await requestGatewayBytes(fixture.socketPath, route, capabilityHeader()) - const retry = await requestGatewayBytes(fixture.socketPath, route, capabilityHeader()) + const route = `${projectedUrl.pathname}${projectedUrl.search}` + const first = await requestGatewayBytes(fixture.port, route, capabilityHeader()) + const retry = await requestGatewayBytes(fixture.port, route, capabilityHeader()) assert.equal(first.statusCode, 200) assert.equal(first.headers['content-type'], 'image/png') assert.equal(first.headers['content-length'], String(png.byteLength)) + assert.equal( + verifyRemoteAttachmentResponseAuthorization( + GATEWAY_TOKEN, + projectedUrl.searchParams.get('access') as string, + 200, + first.body, + first.headers['markover-response-auth'] + ), + true + ) assert.deepEqual(first.body, png) assert.deepEqual(retry.body, png) - await interruptGatewayResponse(fixture.socketPath, route, capabilityHeader()) + await interruptGatewayResponse(fixture.port, route, capabilityHeader()) const afterInterruption = await requestGatewayBytes( - fixture.socketPath, + fixture.port, route, capabilityHeader() ) assert.equal(afterInterruption.statusCode, 200) assert.deepEqual(afterInterruption.body, png) - const denied = await requestGatewayBytes(fixture.socketPath, route) + const denied = await requestGatewayBytes(fixture.port, route) assert.equal(denied.statusCode, 403) assert.equal(responseErrorCode(JSON.parse(denied.body.toString('utf8'))), 'REMOTE_CAPABILITY_REQUIRED') + + const missingAccess = await requestGatewayBytes( + fixture.port, + projectedUrl.pathname, + capabilityHeader() + ) + assert.equal(missingAccess.statusCode, 403) + assert.equal( + responseErrorCode(JSON.parse(missingAccess.body.toString('utf8'))), + 'REMOTE_ATTACHMENT_AUTHORIZATION_REQUIRED' + ) + projectedUrl.searchParams.set('access', `${projectedUrl.searchParams.get('access')}x`) + const tampered = await requestGatewayBytes( + fixture.port, + `${projectedUrl.pathname}${projectedUrl.search}`, + capabilityHeader() + ) + assert.equal(tampered.statusCode, 403) + + const expiredAccess = createRemoteAttachmentAccess( + GATEWAY_TOKEN, + 'mko_aaa11111', + 'img-1', + Date.now() - 1 + ) + const expired = await requestGatewayBytes( + fixture.port, + `${projectedUrl.pathname}?access=${expiredAccess}`, + capabilityHeader() + ) + assert.equal(expired.statusCode, 403) }) test('private attachment checks reject corrupt metadata, paths, bytes, and links', async (t) => { @@ -732,80 +962,89 @@ test('private attachment checks reject corrupt metadata, paths, bytes, and links assert.equal(await allowlist.resolve('mko_aaa11111', 'img-4'), null) }) -test('socket lifecycle hardens modes, rejects live ownership, recovers stale sockets, and removes its own socket', async (t) => { - const directory = await fs.mkdtemp( - path.join(os.tmpdir(), 'markover-remote-socket-test-') - ) - t.after(() => fs.rm(directory, { recursive: true, force: true })) - const stateRoot = path.join(directory, 'state') - const socketPath = path.join(stateRoot, 'remote.sock') - await fs.mkdir(stateRoot) - - const owner = await fs.stat(stateRoot) +test('loopback lifecycle validates ports, rejects an occupied port, and closes its exact listener', async () => { await assert.rejects( startRemoteGateway({ - socketPath, + gatewayToken: GATEWAY_TOKEN, localPort: 1234, localToken: 'B'.repeat(43), discoveryPolicy: () => false, loadAttachment: missingAttachment, routingReady: () => Promise.resolve(), - uid: owner.uid + 1 + port: -1 }), (error: unknown) => ( error instanceof RemoteGatewayError && - error.code === 'REMOTE_GATEWAY_PARENT_UNOWNED' + error.code === 'REMOTE_GATEWAY_PORT_INVALID' ) ) - const active = http.createServer() - await new Promise((resolve) => active.listen(socketPath, resolve)) + const gateway = await startRemoteGateway({ + gatewayToken: GATEWAY_TOKEN, + localPort: 1234, + localToken: 'B'.repeat(43), + discoveryPolicy: () => false, + loadAttachment: missingAttachment, + routingReady: () => Promise.resolve(), + port: 0 + }) + assert.equal(gateway.host, REMOTE_GATEWAY_HOST) + assert.ok(gateway.port > 0) + await assert.rejects( startRemoteGateway({ - socketPath, + gatewayToken: GATEWAY_TOKEN, localPort: 1234, localToken: 'B'.repeat(43), discoveryPolicy: () => false, loadAttachment: missingAttachment, - routingReady: () => Promise.resolve() + routingReady: () => Promise.resolve(), + port: gateway.port }), (error: unknown) => ( error instanceof RemoteGatewayError && error.code === 'REMOTE_GATEWAY_IN_USE' ) ) - assert.equal((await fs.lstat(socketPath)).isSocket(), true) - await new Promise((resolve, reject) => { - active.close((error) => { if (error) reject(error); else resolve() }) - }) - await fs.unlink(socketPath).catch(() => undefined) - - const stale = spawnSync('python3', [ - '-c', - 'import socket,sys; s=socket.socket(socket.AF_UNIX); s.bind(sys.argv[1])', - socketPath - ], { encoding: 'utf8' }) - assert.equal(stale.status, 0, stale.stderr) - assert.equal((await fs.lstat(socketPath)).isSocket(), true) + await gateway.close() + await gateway.close() + await assert.rejects(requestGateway( + gateway.port, + 'GET', + '/health', + capabilityHeader() + )) +}) - const identity = createServiceIdentity() - const store = new ReviewStore(path.join(directory, 'reviews')) - const local = await startLocalService({ identity, store }) - t.after(() => local.close()) - const gateway = await startRemoteGateway({ - socketPath, - localPort: local.port, - localToken: identity.token, - discoveryPolicy: () => false, - loadAttachment: missingAttachment, - routingReady: () => Promise.resolve() +test('gateway credential is stable, owner-only, and fails closed when exposed', async (t) => { + const directory = await fs.mkdtemp( + path.join(os.tmpdir(), 'markover-remote-credential-test-') + ) + t.after(() => fs.rm(directory, { recursive: true, force: true })) + const credentialPath = remoteGatewayCredentialPath( + path.join(directory, 'state') + ) + const first = await loadOrCreateRemoteGatewayCredential({ + credentialPath, + token: () => GATEWAY_TOKEN }) - const parentMode = (await fs.stat(stateRoot)).mode & 0o777 - const socketMode = (await fs.stat(socketPath)).mode & 0o777 - assert.equal(parentMode, 0o700) - assert.equal(socketMode, 0o600) - await gateway.close() - await assert.rejects(fs.access(socketPath)) + const second = await loadOrCreateRemoteGatewayCredential({ + credentialPath, + token: () => 'X'.repeat(43) + }) + assert.equal(first, GATEWAY_TOKEN) + assert.equal(second, GATEWAY_TOKEN) + assert.equal((await fs.stat(path.dirname(credentialPath))).mode & 0o777, 0o700) + assert.equal((await fs.stat(credentialPath)).mode & 0o777, 0o600) + + await fs.chmod(credentialPath, 0o644) + await assert.rejects( + loadOrCreateRemoteGatewayCredential({ credentialPath }), + (error: unknown) => ( + error instanceof RemoteGatewayCredentialError && + error.code === 'REMOTE_GATEWAY_CREDENTIAL_UNSAFE' + ) + ) }) test('gateway caps responses from the canonical mutation service', async (t) => { @@ -828,18 +1067,19 @@ test('gateway caps responses from the canonical mutation service', async (t) => const address = local.address() assert.ok(address && typeof address === 'object') const gateway = await startRemoteGateway({ - socketPath: path.join(directory, 'remote.sock'), + gatewayToken: GATEWAY_TOKEN, localPort: address.port, localToken: 'B'.repeat(43), discoveryPolicy: () => false, loadAttachment: missingAttachment, routingReady: () => Promise.resolve(), - maximumResponseBytes: 64 + maximumResponseBytes: 64, + port: 0 }) t.after(() => gateway.close()) const response = await requestGateway( - gateway.socketPath, + gateway.port, 'POST', '/reviews/pending', capabilityHeader(), @@ -849,7 +1089,7 @@ test('gateway caps responses from the canonical mutation service', async (t) => assert.equal(responseErrorCode(response.body), 'RESPONSE_TOO_LARGE') }) -test('disable drains the one active request before removing the socket', async (t) => { +test('disable drains the one active request before closing the loopback listener', async (t) => { let releaseAction: () => void = () => {} let markActionStarted: () => void = () => {} const actionStarted = new Promise((resolve) => { markActionStarted = resolve }) @@ -866,7 +1106,7 @@ test('disable drains the one active request before removing the socket', async ( metadata: { contextSummary: 'Drain active request.' } })) await requestGateway( - fixture.socketPath, + fixture.port, 'POST', '/reviews', createHeaders(body), @@ -874,7 +1114,7 @@ test('disable drains the one active request before removing the socket', async ( ) const handoff = requestGateway( - fixture.socketPath, + fixture.port, 'POST', '/reviews/mko_aaa11111/handoff', capabilityHeader() @@ -887,5 +1127,10 @@ test('disable drains the one active request before removing the socket', async ( releaseAction() assert.equal((await handoff).statusCode, 200) await closing - await assert.rejects(fs.access(fixture.socketPath)) + await assert.rejects(requestGateway( + fixture.port, + 'GET', + '/health', + capabilityHeader() + )) }) From edc1731d8d945778f0cae0aa2ddf1623b002fb43 Mon Sep 17 00:00:00 2001 From: Michael Johnston Date: Thu, 20 Aug 2026 17:56:30 -0700 Subject: [PATCH 4/7] Allow bounded remote clock skew --- src/remote-client.ts | 5 ++++- src/remote-gateway-auth.ts | 16 +++++++++++----- test/remote-client.test.ts | 18 ++++++++++++++++-- test/remote-gateway.test.ts | 10 +++++++++- 4 files changed, 40 insertions(+), 9 deletions(-) diff --git a/src/remote-client.ts b/src/remote-client.ts index 8a941d75..a9dd9108 100644 --- a/src/remote-client.ts +++ b/src/remote-client.ts @@ -10,6 +10,7 @@ import { remoteContentDigest, remoteRequestAuthorization, type RemoteGatewayChallenge, + REMOTE_GATEWAY_CLOCK_SKEW_TOLERANCE_MILLISECONDS, REMOTE_GATEWAY_CONTENT_DIGEST_HEADER, REMOTE_GATEWAY_RESPONSE_AUTH_HEADER, verifyRemoteAttachmentAccess, @@ -193,7 +194,9 @@ function validatedRemoteAttachmentUrl( profile.token, reviewId, attachmentId, - access + access, + Date.now(), + REMOTE_GATEWAY_CLOCK_SKEW_TOLERANCE_MILLISECONDS ) ) throw invalidResponse() return { access, url } diff --git a/src/remote-gateway-auth.ts b/src/remote-gateway-auth.ts index 4ce4e89a..59286afa 100644 --- a/src/remote-gateway-auth.ts +++ b/src/remote-gateway-auth.ts @@ -8,6 +8,7 @@ export const REMOTE_GATEWAY_CONTENT_DIGEST_HEADER = 'markover-content-digest' export const REMOTE_GATEWAY_RESPONSE_AUTH_HEADER = 'markover-response-auth' export const REMOTE_GATEWAY_AUTHORIZATION_SCHEME = 'Markover-HMAC-v1' export const REMOTE_GATEWAY_CHALLENGE_LIFETIME_MILLISECONDS = 30_000 +export const REMOTE_GATEWAY_CLOCK_SKEW_TOLERANCE_MILLISECONDS = 60_000 const DIGEST_PATTERN = /^sha256:[a-f0-9]{64}$/ const AUTHORIZATION_PATTERN = /^Markover-HMAC-v1 ([A-Za-z0-9_-]{43})\.([a-f0-9]{64})$/ @@ -101,8 +102,10 @@ export function verifyRemoteGatewayChallenge( !CAPABILITY_TOKEN_PATTERN.test(challenge.nonce) || typeof challenge.expiresAt !== 'number' || !Number.isSafeInteger(challenge.expiresAt) || - challenge.expiresAt <= now || - challenge.expiresAt > now + REMOTE_GATEWAY_CHALLENGE_LIFETIME_MILLISECONDS || + challenge.expiresAt <= now - REMOTE_GATEWAY_CLOCK_SKEW_TOLERANCE_MILLISECONDS || + challenge.expiresAt > now + + REMOTE_GATEWAY_CHALLENGE_LIFETIME_MILLISECONDS + + REMOTE_GATEWAY_CLOCK_SKEW_TOLERANCE_MILLISECONDS || typeof challenge.proof !== 'string' || !/^[a-f0-9]{64}$/.test(challenge.proof) || !CAPABILITY_TOKEN_PATTERN.test(token) @@ -237,7 +240,8 @@ export function verifyRemoteAttachmentAccess( reviewId: string, attachmentId: string, value: string | null, - now = Date.now() + now = Date.now(), + clockSkewToleranceMilliseconds = 0 ): boolean { if (value === null) return false const matched = ATTACHMENT_ACCESS_PATTERN.exec(value) @@ -246,8 +250,10 @@ export function verifyRemoteAttachmentAccess( const suppliedProof = matched[2] as string if ( !Number.isSafeInteger(expiresAt) || - expiresAt <= now || - expiresAt > now + REMOTE_ATTACHMENT_ACCESS_LIFETIME_MILLISECONDS + expiresAt <= now - clockSkewToleranceMilliseconds || + expiresAt > now + + REMOTE_ATTACHMENT_ACCESS_LIFETIME_MILLISECONDS + + clockSkewToleranceMilliseconds ) return false return equalHex( suppliedProof, diff --git a/test/remote-client.test.ts b/test/remote-client.test.ts index e250b664..5dc18328 100644 --- a/test/remote-client.test.ts +++ b/test/remote-client.test.ts @@ -16,6 +16,7 @@ import { import { createRemoteAttachmentAccess, createRemoteGatewayChallenge, + REMOTE_GATEWAY_CLOCK_SKEW_TOLERANCE_MILLISECONDS, remoteAttachmentResponseAuthorization, remoteContentDigest, remoteRequestAuthorization, @@ -342,6 +343,19 @@ test('health pins protocol identity and exposes the boolean discovery snapshot', }, { request: portTransport.request }) assert.equal(portTransport.captured[0]?.options.port, 8443) + const skewedTransport = fakeTransport([{ + statusCode: 200, + body: { + ...validHealth, + authorization: createRemoteGatewayChallenge( + token, + Date.now() + 500, + fixedNonce + ) + } + }]) + await readRemoteHealth(profile, { request: skewedTransport.request }) + for (const incompatible of [ { ...validHealth, protocol: { name: 'other', version: 1 } }, { ...validHealth, protocol: { name: 'markover-remote', version: 2 } }, @@ -359,7 +373,7 @@ test('health pins protocol identity and exposes the boolean discovery snapshot', ...validHealth, authorization: createRemoteGatewayChallenge( token, - fixedNow - 60_000, + fixedNow - 120_000, fixedNonce ) } @@ -716,7 +730,7 @@ test('remote attachment URLs stay on the pinned canonical HTTPS origin', () => { token, 'mko_aaa11111', 'img-1', - Date.now() - 1 + Date.now() - REMOTE_GATEWAY_CLOCK_SKEW_TOLERANCE_MILLISECONDS - 1 )}` }, { diff --git a/test/remote-gateway.test.ts b/test/remote-gateway.test.ts index 5214daa3..615c274a 100644 --- a/test/remote-gateway.test.ts +++ b/test/remote-gateway.test.ts @@ -33,6 +33,7 @@ import { RemoteAttachmentAccessStore, type RemoteGatewayChallenge, RemoteGatewayChallengeStore, + REMOTE_GATEWAY_CLOCK_SKEW_TOLERANCE_MILLISECONDS, REMOTE_GATEWAY_CONTENT_DIGEST_HEADER, verifyRemoteAttachmentResponseAuthorization, verifyRemoteGatewayChallenge @@ -449,7 +450,14 @@ test('gateway challenges prove credential ownership and reject replay and expiry const expired = store.create() now = expired.expiresAt - assert.equal(verifyRemoteGatewayChallenge(GATEWAY_TOKEN, expired, now), false) + assert.equal( + verifyRemoteGatewayChallenge( + GATEWAY_TOKEN, + expired, + now + REMOTE_GATEWAY_CLOCK_SKEW_TOLERANCE_MILLISECONDS + ), + false + ) assert.equal(store.authorize({ authorization: remoteRequestAuthorization( GATEWAY_TOKEN, From a80044d3446d5ad0c4dba97a94a16265434219ce Mon Sep 17 00:00:00 2001 From: Michael Johnston Date: Fri, 21 Aug 2026 11:16:50 -0700 Subject: [PATCH 5/7] Audit receipts in invalid v1 reviews --- src/review-store.ts | 83 +++++++++++++++++-- test/review-store.test.ts | 167 +++++++++++++++++++++++++++++++++++++- 2 files changed, 240 insertions(+), 10 deletions(-) diff --git a/src/review-store.ts b/src/review-store.ts index 40bc4980..cf05e4e5 100644 --- a/src/review-store.ts +++ b/src/review-store.ts @@ -17,6 +17,8 @@ import { import { decodeReviewArtifact, decodeReviewTree, + REVIEW_FORMAT, + REVIEW_FORMAT_VERSION, reviewCompatibilityUrl, ReviewFormatError } from './review-format' @@ -145,6 +147,10 @@ function isRecord(value: unknown): value is Record { return value !== null && typeof value === 'object' && !Array.isArray(value) } +function owns(value: object, key: string): boolean { + return Object.prototype.hasOwnProperty.call(value, key) +} + function nonblankString(value: unknown): value is string { return typeof value === 'string' && value.trim().length > 0 } @@ -666,9 +672,21 @@ export class ReviewStore { const scan = await this.listWithWarnings() const uninspectable = scan.warnings.filter((warning) => ( warning.reason === 'incompatible' || - warning.reason === 'invalid' || warning.reason === 'unreadable' )) + const invalidReceipts = await Promise.all(scan.warnings + .filter((warning) => warning.reason === 'invalid') + .map(async (warning) => { + try { + const inspected = await this.inspectCreationReceipt(warning.reviewId) + return inspected === null + ? null + : { artifact: null, receipt: inspected, reviewId: warning.reviewId } + } catch { + uninspectable.push(warning) + return null + } + })) if (uninspectable.length) { const reviewIds = uninspectable .map((warning) => warning.reviewId) @@ -681,11 +699,20 @@ export class ReviewStore { { reviewIds } ) } - const matching = scan.reviews.filter((artifact) => ( - artifact.review.creationReceipt?.keyDigest === receipt.keyDigest + const candidates = [ + ...scan.reviews.flatMap((artifact) => { + const storedReceipt = artifact.review.creationReceipt + return storedReceipt + ? [{ artifact, receipt: storedReceipt, reviewId: artifact.review.id }] + : [] + }), + ...invalidReceipts.filter((candidate) => candidate !== null) + ] + const matching = candidates.filter((candidate) => ( + candidate.receipt.keyDigest === receipt.keyDigest )) if (matching.length > 1) { - const reviewIds = matching.map((artifact) => artifact.review.id).sort() + const reviewIds = matching.map((candidate) => candidate.reviewId).sort() throw new ReviewStoreError( 'DUPLICATE_CREATION_RECEIPT', `Creation receipt ${receipt.keyDigest} is duplicated by reviews ${reviewIds.join(', ')}.`, @@ -696,20 +723,60 @@ export class ReviewStore { } const match = matching[0] if (!match) return null - const storedReceipt = match.review.creationReceipt as ReviewCreationReceipt + const storedReceipt = match.receipt if (storedReceipt.requestDigest !== receipt.requestDigest) { throw new ReviewStoreError( 'IDEMPOTENCY_CONFLICT', - `Idempotency key is already owned by review ${match.review.id} with a different request digest.`, + `Idempotency key is already owned by review ${match.reviewId} with a different request digest.`, undefined, undefined, { creationReceipt: cloneJson(storedReceipt), - reviewId: match.review.id + reviewId: match.reviewId } ) } - return cloneJson(match) + if (match.artifact === null) { + throw new ReviewStoreError( + 'CREATION_RECEIPT_SCAN_INCOMPLETE', + `Creation receipt belongs to uninspectable review ${match.reviewId}.`, + undefined, + undefined, + { reviewIds: [match.reviewId] } + ) + } + return cloneJson(match.artifact) + } + + private async inspectCreationReceipt( + reviewId: string + ): Promise { + const artifact: unknown = JSON.parse( + await fs.readFile(this.reviewPath(reviewId), 'utf8') + ) + if ( + !isRecord(artifact) || + artifact.format !== REVIEW_FORMAT || + artifact.version !== REVIEW_FORMAT_VERSION || + !isRecord(artifact.review) || + artifact.review.id !== reviewId || + !REVIEW_ID_PATTERN.test(artifact.review.id) + ) throw new Error('Creation receipt cannot be inspected safely.') + if (!owns(artifact.review, 'creationReceipt')) return null + const storedReceipt = artifact.review.creationReceipt + if ( + !isRecord(storedReceipt) || + storedReceipt.version !== 1 || + typeof storedReceipt.keyDigest !== 'string' || + typeof storedReceipt.requestDigest !== 'string' + ) throw new Error('Creation receipt cannot be inspected safely.') + assertReceiptDigest(storedReceipt.keyDigest, 'The stored key digest') + assertReceiptDigest(storedReceipt.requestDigest, 'The stored request digest') + return { + version: 1, + keyDigest: storedReceipt.keyDigest, + requestDigest: storedReceipt.requestDigest + } } private async createUnserialized({ diff --git a/test/review-store.test.ts b/test/review-store.test.ts index 61695114..bfe0ea70 100644 --- a/test/review-store.test.ts +++ b/test/review-store.test.ts @@ -234,8 +234,160 @@ test('creation receipts fail closed on body, key, and stored receipt conflicts', ) }) +test('receipt scans ignore unrelated invalid v1 fields without rewriting them', async (t) => { + const ids = [ + 'mko_aaa11111', + 'mko_bbb22222', + 'mko_ccc33333', + 'mko_ddd44444', + 'mko_eee55555' + ] + const { directory, store } = await temporaryStore({ + idFactory: () => ids.shift() as string + }) + t.after(() => fs.rm(directory, { recursive: true, force: true })) + + const mutations = [ + (artifact: Record) => { + artifact.discovery = { source: 'prototype' } + }, + (artifact: Record) => { + const review = artifact.review as Record + review.git = { repositoryRoot: '/private/prototype' } + }, + (artifact: Record) => { + const root = artifact.root as Record + root.collapsed = true + }, + (artifact: Record) => { + const review = artifact.review as Record + delete review.origin + delete review.attentionRequestedAt + } + ] + const preserved = new Map() + for (const mutate of mutations) { + const created = await store.create({ + tree: tree('# Prototype\n'), + contextSummary: 'Pre-v1 prototype review.' + }) + const artifact = structuredClone(created) as unknown as Record + mutate(artifact) + const serialized = `${JSON.stringify(artifact, null, 2)}\n` + await fs.writeFile(store.reviewPath(created.review.id), serialized, 'utf8') + preserved.set(created.review.id, serialized) + await assert.rejects( + store.load(created.review.id), + (error: unknown) => hasErrorCode(error, 'INVALID_REVIEW') + ) + } + + const requestBytes = Buffer.from('{"request":1}') + const created = await store.createWithReceipt({ + tree: tree('# Remote\n'), + contextSummary: 'Review from a remote agent.', + origin: 'remote-agent' + }, { + idempotencyKey: 'mko-idempotency-key-with-enough-random-material', + requestBytes, + requestDigest: digest(requestBytes) + }) + assert.equal(created.created, true) + assert.equal(created.artifact.review.id, 'mko_eee55555') + await assert.rejects( + store.recoverCreation({ + idempotencyKey: 'different-mko-idempotency-key-with-enough-material', + requestDigest: digest('different request') + }), + (error: unknown) => hasErrorCode(error, 'RECEIPT_NOT_FOUND') + ) + for (const [reviewId, serialized] of preserved) { + assert.equal(await fs.readFile(store.reviewPath(reviewId), 'utf8'), serialized) + } +}) + +test('valid receipts in invalid v1 artifacts still prevent duplicate creation', async (t) => { + const ids = ['mko_aaa11111', 'mko_bbb22222'] + const { directory, store } = await temporaryStore({ + idFactory: () => ids.shift() as string + }) + t.after(() => fs.rm(directory, { recursive: true, force: true })) + const firstBytes = Buffer.from('{"request":1}') + const firstKey = 'first-mko-idempotency-key-with-enough-random-material' + const first = await store.createWithReceipt({ + tree: tree('# First\n'), + contextSummary: 'First remote review.', + origin: 'remote-agent' + }, { + idempotencyKey: firstKey, + requestBytes: firstBytes, + requestDigest: digest(firstBytes) + }) + const invalid = structuredClone(first.artifact) as unknown as Record + invalid.discovery = { source: 'prototype' } + await fs.writeFile( + store.reviewPath(first.artifact.review.id), + JSON.stringify(invalid), + 'utf8' + ) + + const secondBytes = Buffer.from('{"request":2}') + const second = await store.createWithReceipt({ + tree: tree('# Second\n'), + contextSummary: 'Second remote review.', + origin: 'remote-agent' + }, { + idempotencyKey: 'second-mko-idempotency-key-with-enough-random-material', + requestBytes: secondBytes, + requestDigest: digest(secondBytes) + }) + assert.equal(second.created, true) + await assert.rejects( + store.recoverCreation({ + idempotencyKey: firstKey, + requestDigest: digest(firstBytes) + }), + (error: unknown) => hasErrorCode(error, 'CREATION_RECEIPT_SCAN_INCOMPLETE') + ) + await assert.rejects( + store.recoverCreation({ + idempotencyKey: firstKey, + requestDigest: digest('changed request') + }), + (error: unknown) => hasErrorCode(error, 'IDEMPOTENCY_CONFLICT') + ) + + const duplicate = structuredClone(second.artifact) as ReviewArtifact & { + discovery: unknown + } + duplicate.discovery = { source: 'prototype' } + const firstReceipt = first.artifact.review.creationReceipt + assert.ok(firstReceipt) + duplicate.review.creationReceipt = structuredClone(firstReceipt) + await fs.writeFile( + store.reviewPath(second.artifact.review.id), + JSON.stringify(duplicate), + 'utf8' + ) + await assert.rejects( + store.recoverCreation({ + idempotencyKey: firstKey, + requestDigest: digest(firstBytes) + }), + (error: unknown) => hasErrorCode(error, 'DUPLICATE_CREATION_RECEIPT') + ) +}) + test('receipt operations fail closed when a managed artifact is uninspectable', async (t) => { - const variants = ['invalid', 'incompatible', 'unreadable'] as const + const variants = [ + 'invalid', + 'invalid-envelope', + 'malformed-receipt', + 'malformed-json', + 'non-object', + 'incompatible', + 'unreadable' + ] as const for (const variant of variants) { const directory = await fs.mkdtemp( path.join(os.tmpdir(), `markover-receipt-${variant}-test-`) @@ -263,6 +415,10 @@ test('receipt operations fail closed when a managed artifact is uninspectable', if (variant === 'unreadable') { await fs.chmod(reviewPath, 0) + } else if (variant === 'malformed-json') { + await fs.writeFile(reviewPath, '{not json', 'utf8') + } else if (variant === 'non-object') { + await fs.writeFile(reviewPath, '[]', 'utf8') } else { const damaged = structuredClone(created.artifact) as unknown as Record< string, @@ -272,7 +428,14 @@ test('receipt operations fail closed when a managed artifact is uninspectable', damaged.version = 2 } else { const review = damaged.review as Record - review.status = 'invalid' + if (variant === 'invalid-envelope') { + review.id = 'mko_other111' + } else if (variant === 'malformed-receipt') { + const receipt = review.creationReceipt as Record + receipt.requestDigest = 'sha256:bad' + } else { + review.status = 'invalid' + } } await fs.writeFile(reviewPath, JSON.stringify(damaged), 'utf8') } From 99f173af4f9e7b965c2cd889f2118e30346d5451 Mon Sep 17 00:00:00 2001 From: Michael Johnston Date: Fri, 21 Aug 2026 14:30:08 -0700 Subject: [PATCH 6/7] Expose verified remote attachment downloads --- .../2026-08-18__remote-canonical-markover.md | 11 +- scripts/markover.ts | 122 +++++++++++++++++- test/markover-cli.test.ts | 90 ++++++++++++- 3 files changed, 215 insertions(+), 8 deletions(-) diff --git a/doc/plans/2026-08-18__remote-canonical-markover.md b/doc/plans/2026-08-18__remote-canonical-markover.md index d4440a7b..ca649a9b 100644 --- a/doc/plans/2026-08-18__remote-canonical-markover.md +++ b/doc/plans/2026-08-18__remote-canonical-markover.md @@ -425,9 +425,10 @@ Run against the canonical host and remote client host, not two development store 7. Open the URL on the canonical host. Add feedback and a screenshot while editing; `get` on the remote client host returns all feedback and a short-lived, attachment-and-gateway-scoped private URL, omits the canonical-host path, - leaves stored JSON unchanged, and denies another node. Fetch the bytes - through the shared remote client and prove its response authentication, - MIME, and projected-checksum checks reject a fixed-port imposter. + leaves stored JSON unchanged, and denies another node. Fetch the bytes with + remote `get-attachment` and prove the shared client's response + authentication, MIME, and projected-checksum checks reject a fixed-port + imposter. 8. Exercise `edit → get → revise` on the same review and PR-observed `done` on matching reviews only; no source file on the canonical host changes. 9. Lose the initial response after commit and after publication. Rerun the @@ -532,6 +533,10 @@ Done when: - remote `get` projects only checked referenced attachments as private URLs and leaves persisted/reviewer artifacts unchanged; +- remote `get-attachment` retrieves one selected attachment through the shared + response-authentication, MIME, size, and checksum checks, writes only + verified bytes to a new explicit output file, and prints no credential or + private URL; - focused tests cover traversal, symlink swap, orphan, duplicate and cross-review metadata, checksum/length mismatch, interrupted streaming, unauthorized nodes, and retry; diff --git a/scripts/markover.ts b/scripts/markover.ts index 5adbcee6..e3a61b4b 100644 --- a/scripts/markover.ts +++ b/scripts/markover.ts @@ -15,10 +15,12 @@ import { requestServiceQuit } from '../src/local-client' import { + readRemoteAttachment, readRemoteHealth, RemoteClientError, requestRemoteJson, validateRemoteAttachmentUrls, + type RemoteAttachmentReference, type RemoteHealth, type RemoteJsonRequestOptions } from '../src/remote-client' @@ -115,6 +117,12 @@ export type ParsedCommand = ParsedInstanceTarget & ( | { command: 'element'; action: 'clear' } | { command: 'element'; action: 'highlight'; reference: string } | { command: 'edit'; reviewId: string } + | { + command: 'get-attachment' + reviewId: string + attachmentId: string + outputPath: string + } | { command: 'resolve' reviewId: string @@ -216,7 +224,7 @@ export function helpPayload() { }, remoteCanonical: { configuration: `Set ${REMOTE_PROFILE_ENVIRONMENT_VARIABLE} to an owner-only JSON file containing exactly {"baseUrl":"https://.ts.net[:]/","token":""}.`, - commands: ['open', 'pending', 'get', 'edit', 'revise', 'done'], + commands: ['open', 'pending', 'get', 'get-attachment', 'edit', 'revise', 'done'], behavior: 'A valid profile uses canonical Markover on the configured host without downloading, launching, or storing a second Markover app on the client Mac. Thread and Git discovery remain local; checked attachment URLs carry short-lived, attachment-scoped access and reviewer mode is unavailable.' }, workflow: [ @@ -226,6 +234,7 @@ export function helpPayload() { 'Run open once, then retain the returned reviewId in the agent thread.', 'Give the user a best-effort Markdown link using reviewUrl, include the raw reviewId, put open \'\' alone on its own line as the reliable Terminal handoff, and wait for them to say "Check Markover."', 'Run get once after that instruction; it returns the frozen markover-review JSON.', + 'For a returned attachment, run get-attachment with its review ID and attachment ID; verified bytes are written only to a new explicit output path, and stdout omits the private URL and gateway credential.', 'Before interpreting a returned review, require format markover-review and version 1. For any other header, preserve the artifact, consult the official compatibility catalog named by the diagnostic, recommend the compatible Markover release when listed, and never guess at the body.', 'Before acting, follow review.agentGuidance.fixedContract and review.agentGuidance.interpretationPolicy from that JSON.', 'For every agent-originated open or get-for-review, use one truthful identity route. On a proven Codex surface, read only CODEX_THREAD_ID; on a proven Claude surface, read only CLAUDE_CODE_SESSION_ID. If that applicable value is nonblank, pass it as --thread-id. Otherwise create one fresh mko_handoff_ value with 16–64 random letters or digits and pass it as --handoff-key in the same command. With either route, pass --thread-host-kind for the user-facing product or lookup namespace, --thread-host-provider for the LLM provider or model family, not an intermediate harness, and the local hostname result as --thread-host-machine when available. Pass --thread-host-thread-id only for a distinct host-owned ID you actually observe; never guess a T3 thread ID.', @@ -263,6 +272,11 @@ export function helpPayload() { usage: 'get [--pr-status ]', purpose: 'Freeze one review and print its complete markover-review JSON.' }, + { + name: 'get-attachment', + usage: 'get-attachment --output ', + purpose: 'Retrieve one remote attachment through authenticated MIME, size, and checksum verification and write it to a new file.' + }, { name: 'get-for-review', usage: 'get-for-review [--pr-status ] [--thread-id | --handoff-key ] [--thread-host-kind --thread-host-provider [--thread-host-thread-id ] [--thread-host-machine ]]', @@ -399,6 +413,7 @@ export function parseCommandArguments(args: string[]): ParsedCommand { if ( command !== 'open' && command !== 'get' && + command !== 'get-attachment' && command !== 'get-for-review' && command !== 'submit' && command !== 'revise' && @@ -424,10 +439,31 @@ export function parseCommandArguments(args: string[]): ParsedCommand { } throw commandError( `Unknown command: ${command}`, - 'markover ...' + 'markover ...' ) } + if (command === 'get-attachment') { + const [reviewId, attachmentId, outputOption, outputPath] = rest + if ( + rest.length !== 4 || + !reviewId || + reviewId.startsWith('--') || + !attachmentId || + !/^img-[1-9]\d*$/.test(attachmentId) || + outputOption !== '--output' || + !outputPath || + outputPath === '-' || + outputPath.startsWith('--') + ) { + throw commandError( + 'get-attachment requires one review ID, one attachment ID, and one new output path.', + 'markover get-attachment --output ' + ) + } + return targeted({ command, reviewId, attachmentId, outputPath }) + } + if (command === 'element') { if (rest.length === 1 && rest[0] === 'clear') { return targeted({ command, action: 'clear' as const }) @@ -961,6 +997,7 @@ const remoteAuthorCommands = new Set([ 'open', 'pending', 'get', + 'get-attachment', 'edit', 'revise', 'done' @@ -971,6 +1008,36 @@ function remoteProfileApplies(parsed: ParsedCommand): boolean { remoteAuthorCommands.has(parsed.command) } +function findRemoteAttachment( + node: ReviewNode, + attachmentId: string +): RemoteAttachmentReference | null { + for (const attachment of node.attachments || []) { + if (attachment.id !== attachmentId) continue + if ( + typeof attachment.checksum !== 'string' || + typeof attachment.mimeType !== 'string' || + typeof attachment.url !== 'string' + ) { + throw new RemoteClientError( + 'INVALID_RESPONSE', + 'Canonical Markover returned an invalid response.' + ) + } + return { + checksum: attachment.checksum, + id: attachment.id, + mimeType: attachment.mimeType, + url: attachment.url + } + } + for (const child of node.children) { + const attachment = findRemoteAttachment(child, attachmentId) + if (attachment) return attachment + } + return null +} + function defaultRemoteJournalRoot(): string { return path.join(os.homedir(), '.markover', 'remote-client', 'creation-journal') } @@ -1210,6 +1277,7 @@ export interface ExecuteCommandOptions { body?: unknown, options?: RemoteJsonRequestOptions ) => Promise + readRemoteAttachment?: typeof readRemoteAttachment remoteJournal?: RemoteCreationJournal remoteJournalRoot?: string } @@ -1683,6 +1751,12 @@ export async function executeCommand( const profile = parsed.instance === undefined ? await (options.loadRemoteProfile || loadRemoteProfile)() : null + if (!profile && parsed.command === 'get-attachment') { + throw commandError( + 'get-attachment requires a configured remote author profile.', + 'markover get-attachment --output ' + ) + } if (profile && !remoteProfileApplies(parsed)) { throw commandError( `${parsed.command} is not available through the remote author profile.`, @@ -1742,6 +1816,50 @@ export async function executeCommand( : { timeoutMilliseconds: requestOptions.timeoutMilliseconds } ) } + if (parsed.command === 'get-attachment') { + const response = await requestAuthorJson( + 'POST', + `/reviews/${encodeURIComponent(parsed.reviewId)}/handoff` + ) + let artifact: ReviewArtifact + try { + artifact = decodeReviewArtifact(response, parsed.reviewId) + } catch (error) { + if (error instanceof ReviewFormatError) { + throw new RemoteClientError(error.code, error.message) + } + throw error + } + const attachment = findRemoteAttachment( + artifact.root, + parsed.attachmentId + ) + if (!attachment) { + throw commandError( + `Review ${parsed.reviewId} does not contain attachment ${parsed.attachmentId}.`, + 'markover get-attachment --output ' + ) + } + const bytes = await (options.readRemoteAttachment || readRemoteAttachment)( + profile as RemoteProfile, + parsed.reviewId, + attachment + ) + await fs.writeFile(path.resolve(parsed.outputPath), bytes, { + flag: 'wx', + mode: 0o600 + }) + return { + format: 'markover-remote-attachment', + version: 1, + status: 'written', + reviewId: parsed.reviewId, + attachmentId: attachment.id, + mimeType: attachment.mimeType, + checksum: attachment.checksum, + byteLength: bytes.byteLength + } + } if (parsed.command === 'open') { const sourcePath = path.resolve(parsed.sourcePath) const journal = profile diff --git a/test/markover-cli.test.ts b/test/markover-cli.test.ts index fbfec95b..90fdc24a 100644 --- a/test/markover-cli.test.ts +++ b/test/markover-cli.test.ts @@ -25,7 +25,11 @@ import type { CanonicalDoctorResult } from '../src/canonical-maintenance' import type { LinkHandlerMutationResult } from '../src/link-handler' import type { AddressedDevelopmentBundle } from '../scripts/development-bundle' import { guidance } from '../src/agent-guidance' -import { createRemoteGatewayChallenge } from '../src/remote-gateway-auth' +import { + createRemoteAttachmentAccess, + createRemoteGatewayChallenge, + remoteContentDigest +} from '../src/remote-gateway-auth' import { LocalServiceError } from '../src/local-client' import { RemoteClientError } from '../src/remote-client' import { startLocalService, type LocalService } from '../src/local-service' @@ -115,6 +119,21 @@ test('parses lifecycle commands and PR observations', () => { pullRequestStatus: null } ) + assert.deepEqual( + parseCommandArguments([ + 'get-attachment', + 'mko_aaa11111', + 'img-1', + '--output', + 'screenshot.png' + ]), + { + command: 'get-attachment', + reviewId: 'mko_aaa11111', + attachmentId: 'img-1', + outputPath: 'screenshot.png' + } + ) assert.deepEqual( parseCommandArguments([ 'get-for-review', @@ -1038,7 +1057,7 @@ test('CLI help is strict JSON and misuse gives an exact recovery path', () => { assert.match(misuse.stderr, /Unknown command: wat/) assert.match( misuse.stderr, - /Usage: markover / + /Usage: markover / ) assert.match( misuse.stderr, @@ -1278,6 +1297,22 @@ test('remote profile routes all author commands without resolving or starting a contextSummary: 'Review this remotely.', origin: 'remote-agent' }) + const attachmentBytes = Buffer.from('verified image bytes') + const attachmentChecksum = remoteContentDigest(attachmentBytes) + const attachmentAccess = createRemoteAttachmentAccess( + 'A'.repeat(43), + 'mko_aaa11111', + 'img-1', + Date.now() + 60_000 + ) + artifact.root.attachments = [{ + id: 'img-1', + type: 'image', + label: 'Screenshot', + mimeType: 'image/png', + checksum: attachmentChecksum, + url: `/reviews/mko_aaa11111/attachments/img-1?access=${attachmentAccess}` + }] const requests: Array<{ method: string; path: string; body: unknown }> = [] let healthReads = 0 let discoveryCalls = 0 @@ -1358,6 +1393,15 @@ test('remote profile routes all author commands without resolving or starting a }) } throw new Error(`unexpected remote path ${requestPath}`) + }, + readRemoteAttachment(profile, reviewId, attachment) { + assert.equal(profile.token, 'A'.repeat(43)) + assert.equal(reviewId, 'mko_aaa11111') + assert.equal(attachment.id, 'img-1') + assert.equal(attachment.mimeType, 'image/png') + assert.equal(attachment.checksum, attachmentChecksum) + assert.doesNotMatch(attachment.url, new RegExp(profile.token)) + return Promise.resolve(attachmentBytes) } } @@ -1382,6 +1426,25 @@ test('remote profile routes all author commands without resolving or starting a command: 'get', reviewId: 'mko_aaa11111' }, options), 'mko_aaa11111') + const outputPath = path.join(directory, 'downloaded.png') + const attachmentReceipt = await executeCommand({ + command: 'get-attachment', + reviewId: 'mko_aaa11111', + attachmentId: 'img-1', + outputPath + }, options) + assert.deepEqual(attachmentReceipt, { + format: 'markover-remote-attachment', + version: 1, + status: 'written', + reviewId: 'mko_aaa11111', + attachmentId: 'img-1', + mimeType: 'image/png', + checksum: attachmentChecksum, + byteLength: attachmentBytes.byteLength + }) + assert.deepEqual(await fs.readFile(outputPath), attachmentBytes) + assert.doesNotMatch(JSON.stringify(attachmentReceipt), /access=|canonical\.example|A{20}/) assert.deepEqual(await executeCommand({ command: 'edit', reviewId: 'mko_aaa11111' @@ -1400,18 +1463,39 @@ test('remote profile routes all author commands without resolving or starting a status: 'done' }) - assert.equal(healthReads, 6) + assert.equal(healthReads, 7) assert.equal(discoveryCalls, 1) assert.deepEqual(requests.map((request) => request.path), [ '/reviews', '/reviews/pending', '/reviews/mko_aaa11111/handoff', + '/reviews/mko_aaa11111/handoff', '/reviews/mko_aaa11111/edit', '/reviews/mko_aaa11111/revise', '/reviews/done' ]) }) +test('get-attachment requires remote author configuration before local startup', async () => { + let resolved = false + await assert.rejects( + executeCommand({ + command: 'get-attachment', + reviewId: 'mko_aaa11111', + attachmentId: 'img-1', + outputPath: 'screenshot.png' + }, { + loadRemoteProfile: () => Promise.resolve(null), + resolveTarget() { + resolved = true + throw new Error('must not resolve a local instance') + } + }), + /requires a configured remote author profile/ + ) + assert.equal(resolved, false) +}) + test('uncertain remote open recovers by key before rereading the source', async (t) => { const directory = await fs.mkdtemp(path.join(os.tmpdir(), 'markover-remote-recovery-')) const sourcePath = path.join(directory, 'plan.md') From 3783351e77f0a83818d617eb78a5e4260021cfda Mon Sep 17 00:00:00 2001 From: Michael Johnston Date: Fri, 21 Aug 2026 14:45:49 -0700 Subject: [PATCH 7/7] Align local and remote attachment limits --- .../2026-08-18__remote-canonical-markover.md | 2 ++ docs/developer/local-service-security.md | 7 ++-- scripts/app-layout.ts | 1 + src/attachment-limits.ts | 1 + src/remote-attachments.ts | 14 +++++--- src/remote-gateway.ts | 9 ++++-- src/review-store.ts | 8 +++++ test/remote-gateway.test.ts | 10 ++++++ test/review-store.test.ts | 32 +++++++++++++++++++ 9 files changed, 74 insertions(+), 10 deletions(-) create mode 100644 src/attachment-limits.ts diff --git a/doc/plans/2026-08-18__remote-canonical-markover.md b/doc/plans/2026-08-18__remote-canonical-markover.md index ca649a9b..955cc312 100644 --- a/doc/plans/2026-08-18__remote-canonical-markover.md +++ b/doc/plans/2026-08-18__remote-canonical-markover.md @@ -537,6 +537,8 @@ Done when: response-authentication, MIME, size, and checksum checks, writes only verified bytes to a new explicit output file, and prints no credential or private URL; +- local creation, remote projection, and remote download enforce the same + 32 MiB attachment ceiling; - focused tests cover traversal, symlink swap, orphan, duplicate and cross-review metadata, checksum/length mismatch, interrupted streaming, unauthorized nodes, and retry; diff --git a/docs/developer/local-service-security.md b/docs/developer/local-service-security.md index fb1ce076..1473b9c2 100644 --- a/docs/developer/local-service-security.md +++ b/docs/developer/local-service-security.md @@ -165,9 +165,10 @@ routing before accepting review bytes, and returns canonical-host-produced `markover://review/...` URLs for create/recovery and every pending result. Request JSON is capped at 16 MiB and response JSON at 32 MiB, leaving room for -the review envelope Markover adds during creation. Only one remote request is -active at a time. Disable and shutdown stop admission, drain that bounded -request and close the loopback listener. Tailscale +the review envelope Markover adds during creation. Managed attachments use the +same 32 MiB ceiling at creation, remote projection, and download. Only one +remote request is active at a time. Disable and shutdown stop admission, drain +that bounded request and close the loopback listener. Tailscale grants, Serve configuration, login/consent, HTTPS host selection, and certificate issuance remain manual; Markover never enables Funnel or a direct Tailscale-IP listener. diff --git a/scripts/app-layout.ts b/scripts/app-layout.ts index 554afcee..afb7de67 100644 --- a/scripts/app-layout.ts +++ b/scripts/app-layout.ts @@ -9,6 +9,7 @@ export const runtimeModuleNames = [ 'agent-guidance', 'agent-reviewer-guidance', 'app-menu', + 'attachment-limits', 'async-mutation-tracker', 'claude-thread-titles', 'codex-thread-titles', diff --git a/src/attachment-limits.ts b/src/attachment-limits.ts new file mode 100644 index 00000000..58502bb6 --- /dev/null +++ b/src/attachment-limits.ts @@ -0,0 +1 @@ +export const MAXIMUM_ATTACHMENT_BYTES = 32 * 1024 * 1024 diff --git a/src/remote-attachments.ts b/src/remote-attachments.ts index fe2de139..54d6f67c 100644 --- a/src/remote-attachments.ts +++ b/src/remote-attachments.ts @@ -3,7 +3,7 @@ import { constants } from 'node:fs' import fs from 'node:fs/promises' import path from 'node:path' -import { MAXIMUM_BODY_BYTES } from './local-service' +import { MAXIMUM_ATTACHMENT_BYTES } from './attachment-limits' const REVIEW_ID_PATTERN = /^mko_[a-zA-Z0-9]{6,32}$/ const ATTACHMENT_ID_PATTERN = /^img-[a-zA-Z0-9]{1,64}$/ @@ -92,7 +92,7 @@ export async function readVerifiedRemoteAttachment( reviewId: string, attachmentId: string, load: LoadRemoteAttachment, - maximumBytes = MAXIMUM_BODY_BYTES + maximumBytes = MAXIMUM_ATTACHMENT_BYTES ): Promise { if (!REVIEW_ID_PATTERN.test(reviewId) || !ATTACHMENT_ID_PATTERN.test(attachmentId)) { throw attachmentError('REMOTE_ATTACHMENT_NOT_FOUND', 'Attachment not found.') @@ -177,7 +177,8 @@ export async function readVerifiedRemoteAttachment( export async function projectRemoteAttachments( artifact: unknown, - load: LoadRemoteAttachment + load: LoadRemoteAttachment, + maximumBytes = MAXIMUM_ATTACHMENT_BYTES ): Promise { if (!isRecord(artifact) || !isRecord(artifact.review)) return artifact const reviewId = artifact.review.id @@ -193,7 +194,12 @@ export async function projectRemoteAttachments( ) } seen.add(attachment.id) - await readVerifiedRemoteAttachment(reviewId, attachment.id, load) + await readVerifiedRemoteAttachment( + reviewId, + attachment.id, + load, + maximumBytes + ) } const projected = structuredClone(artifact) for (const attachment of attachmentEntries(projected)) { diff --git a/src/remote-gateway.ts b/src/remote-gateway.ts index 99851adc..8c3f6c38 100644 --- a/src/remote-gateway.ts +++ b/src/remote-gateway.ts @@ -5,6 +5,7 @@ import http, { } from 'node:http' import type { ResolvedInstance } from './instance' +import { MAXIMUM_ATTACHMENT_BYTES } from './attachment-limits' import { INTERNAL_IDEMPOTENCY_KEY_HEADER, INTERNAL_REMOTE_CREATE_PATH, @@ -35,7 +36,7 @@ export const REMOTE_GATEWAY_IDEMPOTENCY_HEADER = 'idempotency-key' export const REMOTE_GATEWAY_REQUEST_DIGEST_HEADER = 'markover-request-digest' export const REMOTE_GATEWAY_PROTOCOL_VERSION = 1 -export const MAXIMUM_REMOTE_RESPONSE_BYTES = MAXIMUM_BODY_BYTES * 2 +export const MAXIMUM_REMOTE_RESPONSE_BYTES = MAXIMUM_ATTACHMENT_BYTES export const REMOTE_GATEWAY_HOST = '127.0.0.1' export const REMOTE_GATEWAY_PORT = 39_831 @@ -508,7 +509,8 @@ export async function startRemoteGateway({ const attachment = await readVerifiedRemoteAttachment( attachmentRoute[1] as string, attachmentRoute[2] as string, - loadAttachment + loadAttachment, + maximumResponseBytes ) response.writeHead(200, { 'cache-control': 'private, no-store', @@ -612,7 +614,8 @@ export async function startRemoteGateway({ ) { body = await projectRemoteAttachments( body, - loadAttachment + loadAttachment, + maximumResponseBytes ) body = authorizeProjectedAttachmentUrls(body, attachmentAccess) } diff --git a/src/review-store.ts b/src/review-store.ts index cf05e4e5..8fbab073 100644 --- a/src/review-store.ts +++ b/src/review-store.ts @@ -2,6 +2,8 @@ import { createHash, randomBytes } from 'node:crypto' import fs from 'node:fs/promises' import path from 'node:path' import { isDeepStrictEqual } from 'node:util' + +import { MAXIMUM_ATTACHMENT_BYTES } from './attachment-limits' import { guidance } from './agent-guidance' import { reviewerGuidance } from './agent-reviewer-guidance' import { @@ -1031,6 +1033,12 @@ export class ReviewStore { `Review ${reviewId} is with the agent and read only.` ) } + if (bytes.byteLength > MAXIMUM_ATTACHMENT_BYTES) { + throw new ReviewStoreError( + 'ATTACHMENT_TOO_LARGE', + 'Attachments must not exceed 32 MiB.' + ) + } const directory = path.join(this.reviewDirectory(reviewId), 'attachments') await fs.mkdir(directory, { recursive: true }) diff --git a/test/remote-gateway.test.ts b/test/remote-gateway.test.ts index 615c274a..5b983c8f 100644 --- a/test/remote-gateway.test.ts +++ b/test/remote-gateway.test.ts @@ -6,6 +6,7 @@ import os from 'node:os' import path from 'node:path' import test, { type TestContext } from 'node:test' +import { MAXIMUM_ATTACHMENT_BYTES } from '../src/attachment-limits' import type { ResolvedInstance } from '../src/instance' import { InternalAttachmentAllowlist } from '../src/internal-protocol' import { @@ -669,6 +670,7 @@ test('remote response headroom carries every maximum-size create through handoff const body = Buffer.from(JSON.stringify(input)) assert.equal(body.byteLength, MAXIMUM_BODY_BYTES) assert.equal(MAXIMUM_REMOTE_RESPONSE_BYTES, MAXIMUM_BODY_BYTES * 2) + assert.equal(MAXIMUM_REMOTE_RESPONSE_BYTES, MAXIMUM_ATTACHMENT_BYTES) const created = await requestGateway( fixture.port, @@ -882,6 +884,14 @@ test('private attachment checks reject corrupt metadata, paths, bytes, and links (await readVerifiedRemoteAttachment('mko_aaa11111', 'img-1', load)).bytes, png ) + await assert.rejects( + projectRemoteAttachments({ + review: { id: 'mko_aaa11111' }, + root: { attachments: [attachment], children: [] } + }, load, png.byteLength - 1), + (error: unknown) => error instanceof Error && + Reflect.get(error, 'code') === 'REMOTE_ATTACHMENT_TOO_LARGE' + ) attachment.checksum = digest('wrong') await assert.rejects( diff --git a/test/review-store.test.ts b/test/review-store.test.ts index bfe0ea70..51e97ab4 100644 --- a/test/review-store.test.ts +++ b/test/review-store.test.ts @@ -5,6 +5,7 @@ import os from 'node:os' import path from 'node:path' import test from 'node:test' +import { MAXIMUM_ATTACHMENT_BYTES } from '../src/attachment-limits' import { reviewChecksum } from '../src/review-format' import { @@ -1699,6 +1700,37 @@ test('attachment allocation is owned, editable, and serialized by the store', as ) }) +test('attachment allocation enforces the shared remote response bound', async (t) => { + const { directory, store } = await temporaryStore({ + idFactory: () => 'mko_aaa11111' + }) + t.after(() => fs.rm(directory, { recursive: true, force: true })) + + const created = await store.create({ + tree: tree(), + contextSummary: 'Check attachment bounds.' + }) + const accepted = await store.saveAttachmentFile( + created.review.id, + 'png', + Buffer.alloc(MAXIMUM_ATTACHMENT_BYTES) + ) + assert.equal((await fs.stat(accepted.path)).size, MAXIMUM_ATTACHMENT_BYTES) + + await assert.rejects( + store.saveAttachmentFile( + created.review.id, + 'png', + Buffer.alloc(MAXIMUM_ATTACHMENT_BYTES + 1) + ), + (error: unknown) => hasErrorCode(error, 'ATTACHMENT_TOO_LARGE') + ) + assert.deepEqual( + await fs.readdir(path.dirname(accepted.path)), + [path.basename(accepted.path)] + ) +}) + test('review deletion policies cover every status and trash the exact directory', async (t) => { const ids = ['mko_aaa11111', 'mko_bbb22222'] const { directory, store } = await temporaryStore({