fix(dev): validate lock and worker metadata before trusting it

Author: danielroe

packages/nuxt-cli/src/utils/dev-server.ts

@@ -106,15 +106,26 @@ export async function findNitroDevWorker(cwd: string, buildDir?: string): Promis
 }
 
 const{ socketPath, host, port }=dev.workerAddress
-if(socketPath){
+if(typeofsocketPath==='string'&&socketPath){
 return{pid: dev.pid, socketPath }
 }
-if(port){
+if(typeofport==='number'&&Number.isInteger(port)&&port>0&&port<=65_535&&isLocalHost(host)){
 return{pid: dev.pid,url: `http://${host||'localhost'}:${port}`}
 }
 }
 }
 
+constLOCAL_HOSTS=newSet(['','localhost','127.0.0.1','::1','[::1]','0.0.0.0','::','[::]'])
+
+/**
+ * Whether an address recorded by Nitro belongs to this machine. The worker is
+ * always local, and the file naming it lives in the project, so an address
+ * pointing anywhere else is not one we should send task payloads to.
+ */
+functionisLocalHost(host: string|undefined): boolean{
+returnLOCAL_HOSTS.has(host??'')
+}
+
 exportfunctionnoDevServerMessage(what: string): string{
 return`No running Nuxt dev server found. Start one with ${styleText('cyan','nuxt dev')}, or pass an absolute URL to ${styleText('cyan',what)}.`
 }

packages/nuxt-cli/src/utils/lockfile.ts

@@ -36,6 +36,10 @@ const OUTPUT_LOCK_DIRNAME = 'node_modules/.cache/nuxt'
 // PID recycling safety net. Locks older than this cannot be trusted because a
 // recycled PID could match a dead build's record.
 constMAX_LOCK_AGE_MS=24*60*60*1000
+// A lock written by a machine whose clock runs slightly ahead of ours is still
+// plausible; anything further into the future is not, and would otherwise keep
+// `isLockActive` true indefinitely.
+constMAX_LOCK_CLOCK_SKEW_MS=5*60*1000
 
 exportfunctionisProcessAlive(pid: number): boolean{
 try{
@@ -119,13 +123,91 @@ export function getTakeoverPid(buildDir: string): number | undefined {
 
 functionreadLockFile(lockPath: string): LockInfo|undefined{
 try{
-returnJSON.parse(readFileSync(lockPath,'utf-8'))asLockInfo
+returnparseLockInfo(JSON.parse(readFileSync(lockPath,'utf-8')))
 }
 catch{
 returnundefined
 }
 }
 
+constLOCK_COMMANDS=newSet<LockInfo['command']>(['dev','build','analyze'])
+constMAX_LOCK_STRING_LENGTH=1024
+// C0 and C1 control characters, which would otherwise reach the terminal when a
+// lock is described to the user.
+// eslint-disable-next-line no-control-regex
+constCONTROL_CHARS_RE=/[\u0000-\u001F\u007F-\u009F]/g
+constLOCK_HOSTNAME_RE=/^[\w.:[\]-]{1,253}$/
+
+functionlockPid(value: unknown): number|undefined{
+returntypeofvalue==='number'&&Number.isInteger(value)&&value>0&&value<=2**31-1
+ ? value
+ : undefined
+}
+
+functionlockText(value: unknown): string|undefined{
+returntypeofvalue==='string'
+ ? value.slice(0,MAX_LOCK_STRING_LENGTH).replace(CONTROL_CHARS_RE,'')
+ : undefined
+}
+
+functionlockURL(value: unknown): string|undefined{
+consttext=lockText(value)
+if(!text){
+returnundefined
+}
+try{
+consturl=newURL(text)
+returnurl.protocol==='http:'||url.protocol==='https:' ? text : undefined
+}
+catch{
+returnundefined
+}
+}
+
+/**
+ * Validate a parsed `nuxt.lock` document.
+ *
+ * A lock lives in the build directory, so it can arrive with a cloned project
+ * and is read before any of that project's code runs. Everything taken from it
+ * is either signalled, connected to, or printed, so a record that does not have
+ * the exact shape written by {@link acquireLock} is discarded rather than
+ * repaired.
+ */
+exportfunctionparseLockInfo(raw: unknown): LockInfo|undefined{
+if(!raw||typeofraw!=='object'||Array.isArray(raw)){
+returnundefined
+}
+constinput=rawasRecord<string,unknown>
+
+constpid=lockPid(input.pid)
+constcommand=input.commandasLockInfo['command']
+conststartedAt=input.startedAt
+if(!pid||!LOCK_COMMANDS.has(command)||typeofstartedAt!=='number'||!Number.isFinite(startedAt)){
+returnundefined
+}
+if(startedAt>Date.now()+MAX_LOCK_CLOCK_SKEW_MS){
+returnundefined
+}
+
+constport=input.port
+consthostname=lockText(input.hostname)
+constparentPid=lockPid(input.parentPid)
+consttakenOverBy=lockPid(input.takenOverBy)
+
+return{
+ pid,
+ startedAt,
+ command,
+cwd: lockText(input.cwd)??'',
+interactive: input.interactive===true,
+ ...typeofport==='number'&&Number.isInteger(port)&&port>0&&port<=65_535 ? { port } : {},
+ ...hostname&&LOCK_HOSTNAME_RE.test(hostname) ? { hostname } : {},
+ ...lockURL(input.url) ? {url: lockURL(input.url)} : {},
+ ...parentPid ? { parentPid } : {},
+ ...takenOverBy ? { takenOverBy } : {},
+}
+}
+
 /**
  * Replace a lock we own. Writing a sibling temp file and renaming it into place
  * keeps the window where a reader could see a truncated file from existing: a

packages/nuxt-cli/test/unit/utils/untrusted-lock.spec.ts

@@ -0,0 +1,141 @@
+import{mkdtemp,rm,writeFile}from'node:fs/promises'
+import{tmpdir}from'node:os'
+import{join}from'node:path'
+importprocessfrom'node:process'
+
+import{afterEach,beforeEach,describe,expect,it,vi}from'vitest'
+
+import{takeOverDevServer}from'../../../src/dev/takeover'
+import{findDevServer,findNitroDevWorker}from'../../../src/utils/dev-server'
+import{parseLockInfo,readLock}from'../../../src/utils/lockfile'
+
+lettempDir: string
+
+beforeEach(async()=>{
+tempDir=awaitmkdtemp(join(tmpdir(),'nuxt-untrusted-lock-'))
+deleteprocess.env.NUXT_IGNORE_LOCK
+deleteprocess.env.NUXT_LOCK
+})
+
+afterEach(async()=>{
+vi.restoreAllMocks()
+awaitrm(tempDir,{recursive: true,force: true})
+})
+
+functionbaseLock(overrides: Record<string,unknown>={}){
+return{
+pid: process.pid,
+startedAt: Date.now(),
+command: 'dev',
+cwd: '/tmp/project',
+interactive: false,
+ ...overrides,
+}
+}
+
+asyncfunctionwriteLock(contents: unknown): Promise<string>{
+awaitwriteFile(join(tempDir,'nuxt.lock'),typeofcontents==='string' ? contents : JSON.stringify(contents))
+returntempDir
+}
+
+describe('parseLockInfo',()=>{
+it('should reject a lock with a non-positive pid',()=>{
+expect(parseLockInfo(baseLock({pid: -1}))).toBeUndefined()
+expect(parseLockInfo(baseLock({pid: 0}))).toBeUndefined()
+expect(parseLockInfo(baseLock({pid: 1.5}))).toBeUndefined()
+expect(parseLockInfo(baseLock({pid: '123'}))).toBeUndefined()
+})
+
+it('should reject a lock with an unknown command',()=>{
+expect(parseLockInfo(baseLock({command: 'rm -rf /'}))).toBeUndefined()
+})
+
+it('should reject anything that is not an object',()=>{
+expect(parseLockInfo(null)).toBeUndefined()
+expect(parseLockInfo([baseLock()])).toBeUndefined()
+expect(parseLockInfo('dev')).toBeUndefined()
+})
+
+it('should drop a negative parent pid rather than the whole lock',()=>{
+expect(parseLockInfo(baseLock({parentPid: -1}))?.parentPid).toBeUndefined()
+expect(parseLockInfo(baseLock({takenOverBy: -1}))?.takenOverBy).toBeUndefined()
+})
+
+it('should drop an out-of-range port',()=>{
+expect(parseLockInfo(baseLock({port: 0}))?.port).toBeUndefined()
+expect(parseLockInfo(baseLock({port: 70_000}))?.port).toBeUndefined()
+expect(parseLockInfo(baseLock({port: 3000}))?.port).toBe(3000)
+})
+
+it('should drop a url that is not http or https',()=>{
+expect(parseLockInfo(baseLock({url: 'file:///etc/passwd'}))?.url).toBeUndefined()
+expect(parseLockInfo(baseLock({url: 'not a url'}))?.url).toBeUndefined()
+expect(parseLockInfo(baseLock({url: 'http://localhost:3000'}))?.url).toBe('http://localhost:3000')
+})
+
+it('should strip control characters from displayed strings',()=>{
+constinfo=parseLockInfo(baseLock({cwd: '/tmp/\u001B[2Jproject\u0007'}))
+expect(info?.cwd).toBe('/tmp/[2Jproject')
+})
+
+it('should reject a lock timestamped far into the future',()=>{
+expect(parseLockInfo(baseLock({startedAt: Number.MAX_VALUE}))).toBeUndefined()
+expect(parseLockInfo(baseLock({startedAt: Date.now()+60*60*1000}))).toBeUndefined()
+expect(parseLockInfo(baseLock({startedAt: Date.now()+1000}))).toBeDefined()
+})
+
+it('should cap the length of strings it keeps',()=>{
+expect(parseLockInfo(baseLock({cwd: 'a'.repeat(5000)}))?.cwd).toHaveLength(1024)
+})
+})
+
+describe('reading an untrusted lock',()=>{
+it('should ignore a lock file that is not valid json',async()=>{
+expect(readLock(awaitwriteLock('}{'))).toBeUndefined()
+})
+
+it('should ignore a lock claiming a negative pid',async()=>{
+expect(readLock(awaitwriteLock(baseLock({pid: -1,port: 3000})))).toBeUndefined()
+})
+
+it('should never signal a process group during takeover',async()=>{
+constkill=vi.spyOn(process,'kill')
+constresult=awaittakeOverDevServer(
+awaitwriteLock(baseLock({pid: -1,port: 3000,url: 'http://localhost:3000'})),
+{takeover: true},
+)
+
+expect(result.action).toBe('none')
+for(constcallofkill.mock.calls){
+expect(call[0]).toBeGreaterThan(0)
+}
+})
+
+it('should not resolve a dev server url pointing at another scheme',async()=>{
+constdir=awaitwriteLock(baseLock({pid: process.ppid,url: 'file:///etc/passwd'}))
+awaitexpect(findDevServer(dir,dir)).resolves.toBeUndefined()
+})
+
+it('should resolve a dev server recorded with an http url',async()=>{
+constdir=awaitwriteLock(baseLock({pid: process.ppid,url: 'http://localhost:3000'}))
+awaitexpect(findDevServer(dir,dir)).resolves.toMatchObject({url: 'http://localhost:3000'})
+})
+})
+
+describe('findNitroDevWorker',()=>{
+it('should ignore a worker address on a remote host',async()=>{
+awaitwriteFile(join(tempDir,'nitro.json'),JSON.stringify({
+dev: {pid: process.pid,workerAddress: {host: 'evil.example.com',port: 80}},
+}))
+
+awaitexpect(findNitroDevWorker(tempDir,tempDir)).resolves.toBeUndefined()
+})
+
+it('should accept a loopback worker address',async()=>{
+awaitwriteFile(join(tempDir,'nitro.json'),JSON.stringify({
+dev: {pid: process.pid,workerAddress: {host: '127.0.0.1',port: 3000}},
+}))
+
+awaitexpect(findNitroDevWorker(tempDir,tempDir)).resolves.toMatchObject({url: 'http://127.0.0.1:3000'})
+})
+})