diff --git a/apps/cli/src/commands/self.ts b/apps/cli/src/commands/self.ts index 518d1c0..1edafe8 100644 --- a/apps/cli/src/commands/self.ts +++ b/apps/cli/src/commands/self.ts @@ -51,6 +51,38 @@ export function readManifest(env: NodeJS.ProcessEnv = process.env): InstallManif } } +/** + * Which surface flags to hand the installer when re-running it to upgrade. + * + * The installer with no flag auto-detects a desktop session, so a CLI-only + * install is deliberately left to that detection: a machine that has grown a + * desktop since it was first installed gets the desktop app on its next + * update, which is what "update" is expected to mean. Pinning it to + * `--cli-only` — as this used to — meant an install made from a tty or over + * SSH could never gain the desktop app, no matter how many times it updated, + * and said nothing about why. + * + * A desktop install is pinned the other way, to `--desktop`. Updating one from + * a tty (a cron job, an SSH session) has no DISPLAY to detect, so auto-detect + * would quietly downgrade it to CLI-only and strip the app out from under a + * running desktop. + * + * An explicit flag on the update command always wins, so either direction + * stays reachable by hand. + */ +export function installerFlags(manifest: InstallManifest, parsed?: ParsedArgv): string[] { + if (parsed && hasFlag(parsed, '--cli-only')) return ['--cli-only'] + if (parsed && hasFlag(parsed, '--desktop')) return ['--desktop'] + return manifest.desktop ? ['--desktop'] : [] +} + +/** What `Surfaces:` prints, so an update never silently skips the desktop app. */ +function describeSurfaces(flags: string[]): string { + if (flags.includes('--cli-only')) return 'CLI only (--cli-only)' + if (flags.includes('--desktop')) return 'CLI and desktop app' + return 'CLI, plus the desktop app if this machine has a desktop session' +} + /** * Re-runs the installer that put this copy here. It is idempotent, so * updating is installing again, and the manifest remembers where it came @@ -73,8 +105,10 @@ export async function runUpdate(parsed: ParsedArgv, output: Output): Promise { + it('leaves a CLI-only install to the installer to auto-detect', () => { + // The bug this exists to prevent: pinning --cli-only meant an install made + // from a tty could never gain the desktop app, however often it updated. + expect(installerFlags(manifest(false))).toEqual([]) + }) + + it('pins a desktop install to --desktop', () => { + // Updating from a cron job or an SSH session has no DISPLAY to detect, so + // auto-detect would strip the app out from under a working desktop. + expect(installerFlags(manifest(true))).toEqual(['--desktop']) + }) + + it('honours an explicit --cli-only on a desktop install', () => { + const parsed = parseArgv(['update', '--cli-only']) + expect(installerFlags(manifest(true), parsed)).toEqual(['--cli-only']) + }) + + it('honours an explicit --desktop on a CLI-only install', () => { + const parsed = parseArgv(['update', '--desktop']) + expect(installerFlags(manifest(false), parsed)).toEqual(['--desktop']) + }) + + it('prefers --cli-only when both are somehow passed', () => { + const parsed = parseArgv(['update', '--cli-only', '--desktop']) + expect(installerFlags(manifest(false), parsed)).toEqual(['--cli-only']) + }) + + it('adds no flag for a plain update of a CLI-only install', () => { + const parsed = parseArgv(['update']) + expect(installerFlags(manifest(false), parsed)).toEqual([]) + }) +}) diff --git a/apps/cli/src/self-update.ts b/apps/cli/src/self-update.ts index 5a6cb48..91eb5f4 100644 --- a/apps/cli/src/self-update.ts +++ b/apps/cli/src/self-update.ts @@ -3,7 +3,7 @@ import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs' import { dirname, join } from 'node:path' import { VERSION } from './help.js' import type { Output } from './output.js' -import { readManifest, stateDirectory } from './commands/self.js' +import { installerFlags, readManifest, stateDirectory } from './commands/self.js' /** * Startup update check. @@ -105,7 +105,7 @@ export async function autoUpdate(command: string | null, output: Output, force = return 'failed' } - const run = spawnSync('sh', ['-s', '--', ...(manifest.desktop ? [] : ['--cli-only'])], { + const run = spawnSync('sh', ['-s', '--', ...installerFlags(manifest)], { input: script.stdout, stdio: ['pipe', 'ignore', 'inherit'], }) diff --git a/apps/desktop/electron/main/bundle-path.test.ts b/apps/desktop/electron/main/bundle-path.test.ts new file mode 100644 index 0000000..74ced17 --- /dev/null +++ b/apps/desktop/electron/main/bundle-path.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, it } from 'vitest' +import { join } from 'node:path' +import { contentTypeFor, resolveBundlePath } from './bundle-path.js' + +const ROOT = '/opt/diskpush/out' + +describe('resolveBundlePath', () => { + it('serves index.html for the origin root', () => { + expect(resolveBundlePath(ROOT, '/')).toBe(join(ROOT, 'index.html')) + }) + + it('resolves the root-absolute asset paths Next emits', () => { + // The whole reason the bundle scheme exists: under file:// these resolved + // against the filesystem root and every one of them 404'd. + expect(resolveBundlePath(ROOT, '/_next/static/css/app.css')).toBe(join(ROOT, '_next/static/css/app.css')) + expect(resolveBundlePath(ROOT, '/_next/static/chunks/main-app.js')).toBe(join(ROOT, '_next/static/chunks/main-app.js')) + }) + + it('decodes percent-escapes in a filename', () => { + expect(resolveBundlePath(ROOT, '/_next/a%20b.css')).toBe(join(ROOT, '_next/a b.css')) + }) + + it('refuses to escape the bundle', () => { + for (const attempt of ['/../../../etc/passwd', '/_next/../../etc/passwd', '/%2e%2e/%2e%2e/etc/passwd']) { + expect(resolveBundlePath(ROOT, attempt)).toBeNull() + } + }) + + it('refuses a malformed escape and an embedded NUL', () => { + expect(resolveBundlePath(ROOT, '/%')).toBeNull() + expect(resolveBundlePath(ROOT, '/index.html%00.png')).toBeNull() + }) + + it('does not treat a sibling directory as inside the bundle', () => { + // String-prefix checks without a separator let /opt/diskpush/outside through. + expect(resolveBundlePath('/opt/diskpush/out', '/../outside/secret')).toBeNull() + }) +}) + +describe('contentTypeFor', () => { + it('types the assets a Next export is made of', () => { + // Served as octet-stream, a stylesheet is a stylesheet the renderer ignores. + expect(contentTypeFor('/out/_next/static/css/app.css')).toBe('text/css; charset=utf-8') + expect(contentTypeFor('/out/_next/static/chunks/main.js')).toBe('text/javascript; charset=utf-8') + expect(contentTypeFor('/out/index.html')).toBe('text/html; charset=utf-8') + expect(contentTypeFor('/out/_next/static/media/geist.woff2')).toBe('font/woff2') + }) + + it('ignores case in the extension', () => { + expect(contentTypeFor('/out/LOGO.PNG')).toBe('image/png') + }) + + it('falls back for anything unrecognised', () => { + expect(contentTypeFor('/out/data.bin')).toBe('application/octet-stream') + expect(contentTypeFor('/out/LICENSE')).toBe('application/octet-stream') + }) +}) diff --git a/apps/desktop/electron/main/bundle-path.ts b/apps/desktop/electron/main/bundle-path.ts new file mode 100644 index 0000000..6660380 --- /dev/null +++ b/apps/desktop/electron/main/bundle-path.ts @@ -0,0 +1,59 @@ +import { extname, join, normalize, sep } from 'node:path' + +/** + * Maps a bundle-scheme pathname to a file inside the exported renderer, or + * null when it escapes. + * + * Kept apart from index.ts so the tests can import it: index.ts registers a + * privileged scheme at module scope, so importing it outside Electron throws. + */ +export function resolveBundlePath(root: string, pathname: string): string | null { + let decoded: string + try { + decoded = decodeURIComponent(pathname) + } catch { + // A malformed escape is not a path we should guess at. + return null + } + // A NUL byte truncates the path in some syscalls; refuse rather than normalise. + if (decoded.includes('\0')) return null + + const target = normalize(join(root, decoded === '/' ? 'index.html' : decoded)) + // A crafted ../ must not turn the bundle scheme into a reader for the disk. + if (target !== root && !target.startsWith(root + sep)) return null + return target +} + +/** + * Content types for what a Next export actually contains. + * + * The bundle is read with fs rather than `net.fetch('file://…')` because the + * packaged app is an asar archive: fs is asar-aware, so it reads straight out + * of it, and nothing here has to know whether the app is packaged. That means + * the response carries no type of its own, and a stylesheet served as + * octet-stream is a stylesheet the renderer ignores. + */ +const CONTENT_TYPES: Record = { + '.html': 'text/html; charset=utf-8', + '.js': 'text/javascript; charset=utf-8', + '.mjs': 'text/javascript; charset=utf-8', + '.css': 'text/css; charset=utf-8', + '.json': 'application/json; charset=utf-8', + '.txt': 'text/plain; charset=utf-8', + '.svg': 'image/svg+xml', + '.png': 'image/png', + '.jpg': 'image/jpeg', + '.jpeg': 'image/jpeg', + '.gif': 'image/gif', + '.webp': 'image/webp', + '.ico': 'image/x-icon', + '.woff': 'font/woff', + '.woff2': 'font/woff2', + '.ttf': 'font/ttf', + '.otf': 'font/otf', + '.map': 'application/json; charset=utf-8', +} + +export function contentTypeFor(path: string): string { + return CONTENT_TYPES[extname(path).toLowerCase()] ?? 'application/octet-stream' +} diff --git a/apps/desktop/electron/main/index.ts b/apps/desktop/electron/main/index.ts index 1890343..13e5b64 100644 --- a/apps/desktop/electron/main/index.ts +++ b/apps/desktop/electron/main/index.ts @@ -1,6 +1,8 @@ -import { join } from 'node:path' +import { readFile } from 'node:fs/promises' +import { join, normalize } from 'node:path' import { fileURLToPath } from 'node:url' -import { app, BrowserWindow, shell } from 'electron' +import { app, BrowserWindow, protocol, shell } from 'electron' +import { contentTypeFor, resolveBundlePath } from './bundle-path.js' import { registerIpc } from './ipc.js' import { checkForUpdates } from './services/updater.js' import { closeAllSessions } from './services/sessions.js' @@ -19,6 +21,41 @@ const here = join(fileURLToPath(import.meta.url), '..') const isDev = !app.isPackaged && process.env.DISKPUSH_DEV === '1' const DEV_URL = 'http://localhost:3210' +/** + * The exported renderer is served over a real scheme rather than loaded from + * file://. + * + * Next emits root-absolute asset URLs (`/_next/static/...`) and refuses to emit + * relative ones — `next/font` rejects an assetPrefix without a leading slash. + * Under file:// those resolve against the filesystem root, so every stylesheet + * and chunk 404s: the window shows unstyled prerendered HTML that never + * hydrates. A standard scheme gives the bundle an origin, so the same absolute + * paths resolve inside it, and CSP `'self'` means the bundle instead of the + * whole disk. + */ +const APP_SCHEME = 'diskpush-app' +const APP_ORIGIN = `${APP_SCHEME}://bundle` + +// Must run before the app is ready, hence module scope rather than whenReady. +protocol.registerSchemesAsPrivileged([ + { scheme: APP_SCHEME, privileges: { standard: true, secure: true, supportFetchAPI: true } }, +]) + +/** Serves the exported renderer, and nothing outside it. */ +function serveBundle(): void { + const root = normalize(join(here, '..', '..', 'out')) + protocol.handle(APP_SCHEME, async (request) => { + const target = resolveBundlePath(root, new URL(request.url).pathname) + if (!target) return new Response('Forbidden', { status: 403 }) + try { + const body = await readFile(target) + return new Response(body, { headers: { 'content-type': contentTypeFor(target) } }) + } catch { + return new Response('Not found', { status: 404 }) + } + }) +} + function createWindow(): BrowserWindow { const window = new BrowserWindow({ width: 1360, @@ -43,7 +80,7 @@ function createWindow(): BrowserWindow { // Navigation is pinned to the app itself. A renderer compromise should not // be able to point the window at somewhere else. window.webContents.on('will-navigate', (event, url) => { - const allowed = isDev ? url.startsWith(DEV_URL) : url.startsWith('file://') + const allowed = isDev ? url.startsWith(DEV_URL) : url.startsWith(APP_ORIGIN) if (!allowed) event.preventDefault() }) @@ -78,13 +115,14 @@ function createWindow(): BrowserWindow { }) if (isDev) void window.loadURL(DEV_URL) - else void window.loadFile(join(here, '..', '..', 'out', 'index.html')) + else void window.loadURL(`${APP_ORIGIN}/index.html`) return window } app.whenReady().then(() => { registerIpc() + if (!isDev) serveBundle() createWindow() // Not awaited: a slow or unreachable GitHub must not delay the window. diff --git a/scripts/smoke-desktop.mjs b/scripts/smoke-desktop.mjs index 53da5ce..4e61ec0 100755 --- a/scripts/smoke-desktop.mjs +++ b/scripts/smoke-desktop.mjs @@ -12,12 +12,90 @@ * *format* compatibility at runtime, and the unit tests never start Electron. */ import { spawnSync } from 'node:child_process' -import { existsSync } from 'node:fs' +import { existsSync, readFileSync } from 'node:fs' import { dirname, join, resolve } from 'node:path' const desktop = resolve(import.meta.dirname, '..', 'apps', 'desktop') const electron = join(desktop, 'node_modules', 'electron', 'dist', 'electron') +/** + * Every asset the exported page asks for must resolve to a real file through + * the same function that serves it. + * + * The launch check below cannot see this: it dies at the display, so the + * renderer never runs. That is exactly how v0.2.0 shipped a window in which + * the stylesheet and all five chunks 404ed — Next emits root-absolute URLs + * (`/_next/static/...`), the app loaded the page over file://, and those + * resolved against the filesystem root instead of the bundle. The result was + * unstyled prerendered HTML that never hydrated, and nothing failed. + */ +async function checkRendererAssets() { + const out = join(desktop, 'out') + const index = join(out, 'index.html') + if (!existsSync(index)) { + console.error('out/index.html is missing; run pnpm --filter @diskpush/desktop build first.') + process.exit(1) + } + + const helper = join(desktop, 'dist-electron', 'main', 'bundle-path.js') + if (!existsSync(helper)) { + console.error('dist-electron is missing; run pnpm --filter @diskpush/desktop build first.') + process.exit(1) + } + const { resolveBundlePath } = await import(helper) + const html = readFileSync(index, 'utf8') + // Only root-absolute references: those are the ones file:// resolved wrongly. + const referenced = [...html.matchAll(/(?:href|src)="(\/[^"]*)"/g)].map((m) => m[1]) + + if (referenced.length === 0) { + console.error('FAIL: no assets referenced by out/index.html — the export looks broken.') + process.exit(1) + } + + const missing = referenced.filter((url) => { + const target = resolveBundlePath(out, url.split(/[?#]/)[0]) + return !target || !existsSync(target) + }) + + if (missing.length > 0) { + console.error(`FAIL: ${missing.length} of ${referenced.length} renderer assets do not resolve inside the bundle.\n`) + missing.slice(0, 10).forEach((url) => console.error(` ${url}`)) + console.error('\nThe window would render unstyled and never hydrate.') + process.exit(1) + } + + console.log(`ok: all ${referenced.length} renderer assets resolve inside the bundle.`) +} + +/** + * The renderer must not be loaded over file:// again. + * + * Checking that the assets resolve is not enough on its own: they resolve + * whatever the window does with them. What broke the app was the *origin* — + * loadFile gives the page a file:// origin, under which the root-absolute URLs + * above point at the filesystem root rather than the bundle. So pin the + * mechanism, not just the paths. + */ +function checkRendererDelivery() { + const main = readFileSync(join(desktop, 'dist-electron', 'main', 'index.js'), 'utf8') + + if (!main.includes('registerSchemesAsPrivileged')) { + console.error('FAIL: the main process no longer registers a scheme for the bundle.') + process.exit(1) + } + if (/\.loadFile\s*\(/.test(main)) { + console.error('FAIL: the main process loads the renderer with loadFile, which gives it a file:// origin.') + console.error("Next's root-absolute asset URLs resolve against the filesystem root there, so the") + console.error('window renders unstyled and never hydrates. Serve the bundle over its scheme instead.') + process.exit(1) + } + + console.log('ok: the renderer is served over its own scheme, not file://.') +} + +await checkRendererAssets() +checkRendererDelivery() + if (!existsSync(electron)) { console.error('electron binary not found; run pnpm install first.') process.exit(1)