🔒 Fix CodeQL code scanning alerts (#156)

* ⬆️ Upgrade dependencies

* 🔒 Add path sanitization utility

- Introduce sanitizePath() to resolve paths and reject null bytes
- Prevents js/path-injection
- Add unit tests for sanitizePath()

Addresses:
- https://github.com/stoe/action-reporting-cli/security/code-scanning/14
- https://github.com/stoe/action-reporting-cli/security/code-scanning/15
- https://github.com/stoe/action-reporting-cli/security/code-scanning/16
- https://github.com/stoe/action-reporting-cli/security/code-scanning/17

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* 🔒 Fix security issues in Log class

- Coerce isDebug to strict Boolean
- Replace for...in with Object.keys() in maskSensitive()
- Add prototype pollution guard
- Add tests for coercion and pollution resistance

Addresses:
- https://github.com/stoe/action-reporting-cli/security/code-scanning/18
- https://github.com/stoe/action-reporting-cli/security/code-scanning/19
- https://github.com/stoe/action-reporting-cli/security/code-scanning/20
- https://github.com/stoe/action-reporting-cli/security/code-scanning/21
- https://github.com/stoe/action-reporting-cli/security/code-scanning/22
- https://github.com/stoe/action-reporting-cli/security/code-scanning/23

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* 🔒 Apply path sanitization to cache and report

- Sanitize cache path in constructor and setter
- Sanitize report output paths in validateInput()
- Add null byte rejection test for cache setter

Addresses:
- https://github.com/stoe/action-reporting-cli/security/code-scanning/14
- https://github.com/stoe/action-reporting-cli/security/code-scanning/15
- https://github.com/stoe/action-reporting-cli/security/code-scanning/16
- https://github.com/stoe/action-reporting-cli/security/code-scanning/17

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* 🔒 Fix CodeQL user-controlled-bypass alerts in Log class

Replace `if (this.#isDebug)` guards in start(), stopAndPersist(),
fail(), and set text() with object-reference checks on this.#spinner
and this.#logger. This eliminates the user-controlled boolean condition
pattern that triggers js/user-controlled-bypass (alerts 20-23) while
preserving identical runtime behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* 🔒 Address review feedback on security fixes

- maskSensitive(): Still mask tokens in constructor/prototype/__proto__
  key values instead of skipping them entirely. Only skip recursive
  descent into object values for those keys to prevent pollution.
- cache.save(): Use path.dirname(this.#path) instead of hardcoded
  process.cwd()/cache so custom paths create their parent directory.
- Add regression test for null-byte path rejection in Report output
  path validation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: stoe

package-lock.json

@@ -55,20 +55,20 @@
     "@octokit/plugin-throttling": "^11.0.3",
     "chalk": "^4.1.2, <6",
     "csv": "^6.5.1",
-    "got": "^15.0.2",
+    "got": "^15.0.3",
     "js-yaml": "^4.1.1",
     "meow": "^14.1.0",
     "normalize-url": "^9.0.0",
-    "ora": "^9.3.0",
+    "ora": "^9.4.0",
     "winston": "^3.19.0"
   },
   "devDependencies": {
     "@eslint/markdown": "^8.0.1",
     "@github/prettier-config": "^0.0.6",
-    "eslint": "^10.2.0",
+    "eslint": "^10.3.0",
     "eslint-config-prettier": "^10.1.8",
     "eslint-plugin-prettier": "^5.5.5",
-    "globals": "^17.5.0",
+    "globals": "^17.6.0",
     "husky": "^9.1.7",
     "jest": "^30.3.0",
     "lint-staged": "^16.4.0",

package.json

@@ -14,6 +14,7 @@ import JsonReporter from './json.js'
 import MarkdownReporter from './markdown.js'
 
 // Utilities
+import {sanitizePath} from '../util/path.js'
 import wait from '../util/wait.js'
 
 const {blue, cyan, dim, green, red} = chalk
@@ -172,9 +173,9 @@ export default class Report {
     }
 
     this.#output = {
-      csv,
-      json,
-      md,
+      csv: csv ? sanitizePath(csv) : csv,
+      json: json ? sanitizePath(json) : json,
+      md: md ? sanitizePath(md) : md,
     }
   }
 

src/report/report.js

@@ -1,4 +1,6 @@
 import {access, mkdir, readFile, unlink, writeFile} from 'fs/promises'
+import {dirname} from 'node:path'
+import {sanitizePath} from './path.js'
 
 class Cache {
   /**
@@ -25,7 +27,7 @@ class Cache {
    * If a path is provided, it will be used as the cache file path.
    */
   constructor(path = null, logger) {
-    this.#path = path || `${process.cwd()}/cache/report.json`
+    this.#path = sanitizePath(path || `${process.cwd()}/cache/report.json`)
     this.#logger = logger
   }
 
@@ -42,7 +44,7 @@ class Cache {
    * @param {string} newPath - The new cache path to set
    */
   set path(newPath) {
-    this.#path = newPath
+    this.#path = sanitizePath(newPath)
   }
 
   /**
@@ -53,8 +55,9 @@ class Cache {
    */
   async save(data) {
     try {
-      await mkdir(`${process.cwd()}/cache`, {recursive: true})
-      this.#logger.debug(`Creating cache directory at ${process.cwd()}/cache`)
+      const dir = dirname(this.#path)
+      await mkdir(dir, {recursive: true})
+      this.#logger.debug(`Creating cache directory at ${dir}`)
 
       await writeFile(this.#path, JSON.stringify(data, null, 2))
       this.#logger.debug(`Cache saved to ${this.#path}`)

src/util/cache.js

@@ -24,7 +24,7 @@ export class Log {
   constructor(entity, token, isDebug = false) {
     this.#entity = entity
     this.#token = token
-    this.#isDebug = isDebug || process.env.DEBUG === 'true'
+    this.#isDebug = Boolean(isDebug) || process.env.DEBUG === 'true'
     this.#spinner = this.#isDebug ? null : ora()
 
     if (this.#isDebug) {
@@ -138,8 +138,9 @@ export class Log {
     if (typeof value === 'object') {
       const clone = Array.isArray(value) ? [...value] : {...value}
 
-      // Consolidate property checks into one loop for better performance
-      for (const key in clone) {
+      // Use Object.keys() to iterate only own enumerable properties
+      const keys = Array.isArray(clone) ? clone.keys() : Object.keys(clone)
+      for (const key of keys) {
         // Special case for property named 'token'
         if (key === 'token' && typeof clone[key] === 'string') {
           clone[key] = '***'
@@ -150,8 +151,12 @@ export class Log {
         if (typeof clone[key] === 'string' && this.#token && clone[key].includes(this.#token)) {
           clone[key] = clone[key].replace(new RegExp(this.escapeRegExp(this.#token), 'g'), '***')
         }
-        // Recursively process nested objects
+        // Recursively process nested objects, but skip prototype-pollution
+        // vectors to avoid writing into __proto__/constructor/prototype sinks
         else if (typeof clone[key] === 'object' && clone[key] !== null) {
+          if (key === '__proto__' || key === 'constructor' || key === 'prototype') {
+            continue
+          }
           clone[key] = this.maskSensitive(clone[key])
         }
       }
@@ -283,10 +288,10 @@ export class Log {
    */
   start(text) {
     const maskedText = this.maskSensitive(text)
-    if (this.#isDebug) {
-      this.logInDebugMode(maskedText)
-    } else if (this.#spinner) {
+    if (this.#spinner) {
       this.#spinner.start(maskedText)
+    } else if (this.#logger) {
+      this.logInDebugMode(maskedText)
     }
   }
 
@@ -305,16 +310,16 @@ export class Log {
     const maskedPrefixText = this.maskSensitive(prefixText)
     const maskedSuffixText = this.maskSensitive(suffixText)
 
-    if (this.#isDebug) {
-      const message = [maskedPrefixText, symbol, maskedText, maskedSuffixText].join(' ')
-      this.logInDebugMode(message)
-    } else if (this.#spinner) {
+    if (this.#spinner) {
       this.#spinner.stopAndPersist({
         symbol,
         text: maskedText,
         suffixText: maskedSuffixText,
         prefixText: maskedPrefixText,
       })
+    } else if (this.#logger) {
+      const message = [maskedPrefixText, symbol, maskedText, maskedSuffixText].join(' ')
+      this.logInDebugMode(message)
     }
   }
 
@@ -326,10 +331,10 @@ export class Log {
    */
   fail(text) {
     const maskedText = this.maskSensitive(text)
-    if (this.#isDebug) {
-      this.logInDebugMode(maskedText, 'error')
-    } else if (this.#spinner) {
+    if (this.#spinner) {
       this.#spinner.fail(maskedText)
+    } else if (this.#logger) {
+      this.logInDebugMode(maskedText, 'error')
     }
   }
 
@@ -340,10 +345,10 @@ export class Log {
    */
   set text(newText) {
     const maskedText = this.maskSensitive(newText)
-    if (this.#isDebug) {
-      this.logInDebugMode(maskedText)
-    } else if (this.#spinner) {
+    if (this.#spinner) {
       this.#spinner.text = maskedText
+    } else if (this.#logger) {
+      this.logInDebugMode(maskedText)
     }
   }
 

src/util/log.js

@@ -0,0 +1,20 @@
+import path from 'node:path'
+
+/**
+ * Sanitizes a file path to prevent path injection attacks.
+ * Resolves the path to an absolute path and rejects paths containing null bytes.
+ * @param {string} inputPath - The path to sanitize
+ * @returns {string} The sanitized, resolved absolute path
+ * @throws {Error} If the path contains null bytes or is not a string
+ */
+export function sanitizePath(inputPath) {
+  if (typeof inputPath !== 'string') {
+    throw new TypeError('Path must be a string')
+  }
+
+  if (inputPath.includes('\0')) {
+    throw new Error('Path must not contain null bytes')
+  }
+
+  return path.resolve(inputPath)
+}

src/util/path.js

@@ -186,6 +186,14 @@ describe('Report', () => {
         new Report(flags, mockLogger, mockCache)
       }).toThrow('please provide a valid path for the JSON output')
     })
+
+    test('should throw error when output path contains null bytes', () => {
+      const flags = {...validFlags, csv: '/tmp/reports/\0malicious.csv'}
+
+      expect(() => {
+        new Report(flags, mockLogger, mockCache)
+      }).toThrow('Path must not contain null bytes')
+    })
   })
 
   /**

test/report/report.test.js

@@ -101,13 +101,20 @@ describe('cache', () => {
       const initialPath = cache.path
       const newPath = '/new/path/to/cache.json'
 
-      // Test getter
+      // Test getter - path is sanitized (resolved) on construction
       expect(initialPath).toBe(`${process.cwd()}/cache/report.json`)
 
-      // Test setter
+      // Test setter - path is sanitized (resolved)
       cache.path = newPath
       expect(cache.path).toBe(newPath)
     })
+
+    test('setter should reject null bytes in path', () => {
+      const cache = cacheInstance(null, logger)
+      expect(() => {
+        cache.path = '/tmp/\0malicious'
+      }).toThrow('Path must not contain null bytes')
+    })
   })
 
   /**

test/util/cache.test.js

@@ -184,3 +184,63 @@ describe('log', () => {
     })
   })
 })
+
+/**
+ * Tests for security-related behavior using the real Log class.
+ */
+import {Log} from '../../src/util/log.js'
+
+describe('Log security', () => {
+  describe('isDebug boolean coercion', () => {
+    test('should coerce truthy non-boolean values to true', () => {
+      const l = new Log('entity', 'token', 'yes')
+      expect(l.isDebug).toBe(true)
+    })
+
+    test('should coerce falsy non-boolean values to false', () => {
+      const l = new Log('entity', 'token', 0)
+      expect(l.isDebug).toBe(false)
+    })
+
+    test('should keep explicit boolean true', () => {
+      const l = new Log('entity', 'token', true)
+      expect(l.isDebug).toBe(true)
+    })
+
+    test('should keep explicit boolean false', () => {
+      const l = new Log('entity', 'token', false)
+      expect(l.isDebug).toBe(false)
+    })
+  })
+
+  describe('maskSensitive prototype pollution guard', () => {
+    test('should not process __proto__ keys', () => {
+      const l = new Log('entity', 'secret123', false)
+      const input = JSON.parse('{"__proto__": {"polluted": true}, "safe": "secret123"}')
+      const result = l.maskSensitive(input)
+
+      expect(result.safe).toBe('***')
+      const emptyObj = {}
+      expect(emptyObj.polluted).toBeUndefined()
+    })
+
+    test('should not process constructor keys', () => {
+      const l = new Log('entity', 'secret123', false)
+      const input = {constructor: 'secret123', normal: 'secret123'}
+      const result = l.maskSensitive(input)
+
+      expect(result.normal).toBe('***')
+      // constructor key string value should still be masked for security
+      expect(result.constructor).toBe('***')
+    })
+
+    test('should still mask tokens in regular properties', () => {
+      const l = new Log('entity', 'secret123', false)
+      const input = {name: 'hello secret123 world', nested: {value: 'secret123'}}
+      const result = l.maskSensitive(input)
+
+      expect(result.name).toBe('hello *** world')
+      expect(result.nested.value).toBe('***')
+    })
+  })
+})

test/util/log.test.js

@@ -0,0 +1,35 @@
+/**
+ * Unit tests for the path sanitization utility.
+ */
+import path from 'node:path'
+import {sanitizePath} from '../../src/util/path.js'
+
+describe('sanitizePath', () => {
+  test('should resolve a relative path to absolute', () => {
+    const result = sanitizePath('foo/bar.json')
+    expect(path.isAbsolute(result)).toBe(true)
+    expect(result).toBe(path.resolve('foo/bar.json'))
+  })
+
+  test('should return the same value for an already absolute path', () => {
+    const absPath = '/tmp/reports/output.csv'
+    const result = sanitizePath(absPath)
+    expect(result).toBe(path.resolve(absPath))
+  })
+
+  test('should normalize path traversal sequences', () => {
+    const result = sanitizePath('/tmp/reports/../secrets/output.json')
+    expect(result).toBe(path.resolve('/tmp/secrets/output.json'))
+    expect(result).not.toContain('..')
+  })
+
+  test('should throw on null bytes', () => {
+    expect(() => sanitizePath('/tmp/reports/\0malicious')).toThrow('Path must not contain null bytes')
+  })
+
+  test('should throw on non-string input', () => {
+    expect(() => sanitizePath(123)).toThrow('Path must be a string')
+    expect(() => sanitizePath(null)).toThrow('Path must be a string')
+    expect(() => sanitizePath(undefined)).toThrow('Path must be a string')
+  })
+})