fix(transport): sanitize invalid NODE_OPTIONS preloads for workers (#2391)

* fix(transport): sanitize invalid NODE_OPTIONS preloads for workers

* test(transport): cover NODE_OPTIONS preload sanitization branches

Author: mcollina

lib/transport.js

@@ -1,8 +1,10 @@
 'use strict'
 
 const { createRequire } = require('module')
+const { existsSync } = require('node:fs')
 const getCallers = require('./caller')
 const { join, isAbsolute, sep } = require('node:path')
+const { fileURLToPath } = require('node:url')
 const sleep = require('atomic-sleep')
 const onExit = require('on-exit-leak-free')
 const ThreadStream = require('thread-stream')
@@ -35,6 +37,77 @@ function hasPreloadFlags () {
   return false
 }
 
+function sanitizeNodeOptions (nodeOptions) {
+  const tokens = nodeOptions.match(/(?:[^\s"']+|"[^"]*"|'[^']*')+/g)
+  if (!tokens) {
+    return nodeOptions
+  }
+
+  const sanitized = []
+  let changed = false
+
+  for (let i = 0; i < tokens.length; i++) {
+    const token = tokens[i]
+
+    if (token === '--require' || token === '-r' || token === '--import') {
+      const next = tokens[i + 1]
+      if (next && shouldDropPreload(next)) {
+        changed = true
+        i++
+        continue
+      }
+
+      sanitized.push(token)
+      if (next) {
+        sanitized.push(next)
+        i++
+      }
+      continue
+    }
+
+    if (token.startsWith('--require=') || token.startsWith('-r=') || token.startsWith('--import=')) {
+      const value = token.slice(token.indexOf('=') + 1)
+      if (shouldDropPreload(value)) {
+        changed = true
+        continue
+      }
+    }
+
+    sanitized.push(token)
+  }
+
+  return changed ? sanitized.join(' ') : nodeOptions
+}
+
+function shouldDropPreload (value) {
+  const unquoted = stripQuotes(value)
+  if (!unquoted) {
+    return false
+  }
+
+  let path = unquoted
+  if (path.startsWith('file://')) {
+    try {
+      path = fileURLToPath(path)
+    } catch {
+      return false
+    }
+  }
+
+  return isAbsolute(path) && !existsSync(path)
+}
+
+function stripQuotes (value) {
+  const first = value[0]
+  const last = value[value.length - 1]
+
+  if ((first === '"' && last === '"') || (first === "'" && last === "'")) {
+    return value.slice(1, -1)
+  }
+
+  return value
+}
+
 function buildStream (filename, workerData, workerOpts, sync, name) {
   // When pino is loaded during a preload phase (via --import or --require),
   // pass empty execArgv to prevent infinite spawning. Each worker would
@@ -46,6 +119,19 @@ function buildStream (filename, workerData, workerOpts, sync, name) {
     }
   }
 
+  if (!workerOpts.env && process.env.NODE_OPTIONS) {
+    const nodeOptions = sanitizeNodeOptions(process.env.NODE_OPTIONS)
+    if (nodeOptions !== process.env.NODE_OPTIONS) {
+      workerOpts = {
+        ...workerOpts,
+        env: {
+          ...process.env,
+          NODE_OPTIONS: nodeOptions
+        }
+      }
+    }
+  }
+
   workerOpts = { ...workerOpts, name }
 
   const stream = new ThreadStream({

test/fixtures/transport-invalid-node-options.js

@@ -0,0 +1,26 @@
+'use strict'
+
+const pino = require('../../')
+const { join } = require('node:path')
+
+const destination = process.argv[2]
+
+process.env.NODE_OPTIONS = `--require ${join(__dirname, 'this-file-does-not-exist.js')}`
+
+const transport = pino.transport({
+  target: join(__dirname, 'to-file-transport.js'),
+  options: { destination }
+})
+
+const logger = pino(transport)
+transport.on('ready', () => {
+  logger.info('hello with invalid node options preload')
+  setTimeout(() => {
+    transport.end()
+  }, 50)
+})
+
+transport.on('error', (err) => {
+  process.stderr.write(`${err.stack}\n`)
+  process.exitCode = 1
+})

test/transport/node-options.test.js

@@ -0,0 +1,116 @@
+'use strict'
+
+const test = require('node:test')
+const assert = require('node:assert')
+const { EventEmitter } = require('node:events')
+const { join } = require('node:path')
+const { pathToFileURL } = require('node:url')
+const proxyquire = require('proxyquire')
+
+function buildTransportWithFakeThreadStream () {
+  let lastCtorOpts
+
+  class FakeThreadStream extends EventEmitter {
+    constructor (opts) {
+      super()
+      this._closed = false
+      lastCtorOpts = opts
+    }
+
+    unref () {}
+    ref () {}
+    flushSync () {}
+    end () {
+      this._closed = true
+      this.emit('close')
+    }
+
+    get closed () {
+      return this._closed
+    }
+  }
+
+  const transport = proxyquire('../../lib/transport', {
+    'thread-stream': FakeThreadStream
+  })
+
+  return {
+    transport,
+    getLastCtorOpts () {
+      return lastCtorOpts
+    }
+  }
+}
+
+test('pino.transport sanitizes missing absolute preload in NODE_OPTIONS', () => {
+  const previous = process.env.NODE_OPTIONS
+  const missing = join(__dirname, '..', 'fixtures', 'missing-preload.js')
+  process.env.NODE_OPTIONS = `--require ${missing} --trace-warnings`
+
+  const { transport, getLastCtorOpts } = buildTransportWithFakeThreadStream()
+  transport({ target: join(__dirname, '..', 'fixtures', 'to-file-transport.js') })
+
+  assert.equal(getLastCtorOpts().workerOpts.env.NODE_OPTIONS, '--trace-warnings')
+
+  if (previous === undefined) {
+    delete process.env.NODE_OPTIONS
+  } else {
+    process.env.NODE_OPTIONS = previous
+  }
+})
+
+test('pino.transport sanitizes missing file:// preload in NODE_OPTIONS', () => {
+  const previous = process.env.NODE_OPTIONS
+  const missingFileUrl = pathToFileURL(join(__dirname, '..', 'fixtures', 'missing-import.mjs')).href
+  process.env.NODE_OPTIONS = `--import=${missingFileUrl}`
+
+  const { transport, getLastCtorOpts } = buildTransportWithFakeThreadStream()
+  transport({ target: join(__dirname, '..', 'fixtures', 'to-file-transport.js') })
+
+  assert.equal(getLastCtorOpts().workerOpts.env.NODE_OPTIONS, '')
+
+  if (previous === undefined) {
+    delete process.env.NODE_OPTIONS
+  } else {
+    process.env.NODE_OPTIONS = previous
+  }
+})
+
+test('pino.transport keeps relative preload flags in NODE_OPTIONS', () => {
+  const previous = process.env.NODE_OPTIONS
+  process.env.NODE_OPTIONS = '--require ./relative-preload.js'
+
+  const { transport, getLastCtorOpts } = buildTransportWithFakeThreadStream()
+  transport({ target: join(__dirname, '..', 'fixtures', 'to-file-transport.js') })
+
+  assert.equal(getLastCtorOpts().workerOpts.env, undefined)
+
+  if (previous === undefined) {
+    delete process.env.NODE_OPTIONS
+  } else {
+    process.env.NODE_OPTIONS = previous
+  }
+})
+
+test('pino.transport does not override explicit worker.env', () => {
+  const previous = process.env.NODE_OPTIONS
+  process.env.NODE_OPTIONS = `--require ${join(__dirname, '..', 'fixtures', 'missing-preload.js')}`
+
+  const explicitEnv = { NODE_OPTIONS: '--trace-warnings' }
+
+  const { transport, getLastCtorOpts } = buildTransportWithFakeThreadStream()
+  transport({
+    target: join(__dirname, '..', 'fixtures', 'to-file-transport.js'),
+    worker: {
+      env: explicitEnv
+    }
+  })
+
+  assert.equal(getLastCtorOpts().workerOpts.env, explicitEnv)
+
+  if (previous === undefined) {
+    delete process.env.NODE_OPTIONS
+  } else {
+    process.env.NODE_OPTIONS = previous
+  }
+})

test/transport/preload.test.js

@@ -41,3 +41,14 @@ test('pino.transport works when loaded via --import preload (space separated)',
   const result = JSON.parse(await readFile(destination))
   assert.equal(result.msg, 'hello from main')
 })
+
+test('pino.transport ignores missing absolute preload from NODE_OPTIONS in worker', async () => {
+  const destination = file()
+  const main = join(__dirname, '..', 'fixtures', 'transport-invalid-node-options.js')
+
+  await execa(process.argv[0], [main, destination], { timeout: 10000 })
+
+  await watchFileCreated(destination)
+  const result = JSON.parse(await readFile(destination))
+  assert.equal(result.msg, 'hello with invalid node options preload')
+})