Enable ESLint rules for Node Security (#2029)

* enable eslint-plugin-security

* fix SAST warning in ticket cache

* fixed SAST warnings in config

* fixed all SAST warnings in backend

* enable eslint SAST rules for logger

* fixed other SAST warnings

* fixed some more SAST warnings

* backend self review

* PR feedback

backend/lib/app.js

@@ -23,6 +23,12 @@ const { healthCheck } = require('./healthz')
 const { port, metricsPort } = config
 const periodSeconds = config.readinessProbe?.periodSeconds || 10
 
+// protect against Prototype Pollution vulnerabilities
+for (const ctor of [Object, Function, Array, String, Number, Boolean]) {
+  Object.freeze(ctor)
+  Object.freeze(ctor.prototype)
+}
+
 // resolve pathnames
 const PUBLIC_DIRNAME = resolve(join(__dirname, '..', 'public'))
 const INDEX_FILENAME = join(PUBLIC_DIRNAME, 'index.html')

backend/lib/cache/tickets.js

@@ -9,8 +9,8 @@ const _ = require('lodash')
 const logger = require('../logger')
 
 function init () {
-  const issues = {}
-  const commentsForIssues = {} // we could also think of getting rid of the comments cache
+  const issues = new Map()
+  const commentsForIssues = new Map() // we could also think of getting rid of the comments cache
   const emitter = new EventEmitter()
 
   function on (eventName, listener) {
@@ -38,15 +38,16 @@ function init () {
   }
 
   function getIssues () {
-    return _.values(issues)
+    return Array.from(issues.values())
   }
 
   function getCommentsForIssue ({ issueNumber }) {
-    return _.values(getCommentsForIssueCache({ issueNumber }))
+    const comments = getCommentsForIssueCache({ issueNumber })
+    return Array.from(comments.values())
   }
 
   function getIssue (number) {
-    return issues[number]
+    return issues.get(number)
   }
 
   function getIssueNumbers () {
@@ -62,23 +63,25 @@ function init () {
   }
 
   function getCommentsForIssueCache ({ issueNumber }) {
-    if (!commentsForIssues[issueNumber]) {
-      commentsForIssues[issueNumber] = {}
+    if (!commentsForIssues.has(issueNumber)) {
+      commentsForIssues.set(issueNumber, new Map())
     }
-    return commentsForIssues[issueNumber]
+    return commentsForIssues.get(issueNumber)
   }
 
   function addOrUpdateIssues ({ issues }) {
-    _.forEach(issues, issue => addOrUpdateIssue({ issue }))
+    for (const issue of issues) {
+      addOrUpdateIssue({ issue })
+    }
   }
 
   function addOrUpdateIssue ({ issue }) {
-    updateIfNewer('issue', issues, issue, 'number')
+    updateIfNewer('issue', issues, issue)
   }
 
   function addOrUpdateComment ({ issueNumber, comment }) {
     const comments = getCommentsForIssueCache({ issueNumber })
-    updateIfNewer('comment', comments, comment, 'id')
+    updateIfNewer('comment', comments, comment)
   }
 
   function removeIssue ({ issue }) {
@@ -87,37 +90,41 @@ function init () {
 
     const comments = getCommentsForIssueCache({ issueNumber })
 
-    _.unset(issues, issueNumber)
-    _.unset(commentsForIssues, issueNumber)
+    issues.delete(issueNumber)
+    commentsForIssues.delete(issueNumber)
 
     emitIssueDeleted(issue)
-    _.forEach(comments, emitCommmentDeleted)
+    for (const comment of comments.values()) {
+      emitCommmentDeleted(comment)
+    }
   }
 
   function removeComment ({ issueNumber, comment }) {
     const identifier = comment.metadata.id
     logger.trace('removing comment', identifier, 'of issue', issueNumber)
     const commentsForIssuesCache = getCommentsForIssueCache({ issueNumber })
-    _.unset(commentsForIssuesCache, identifier)
+    commentsForIssuesCache.delete(identifier)
     emitCommmentDeleted(comment)
   }
 
-  function updateIfNewer (kind, cachedList, item, itemIdentifier) {
-    const identifier = item.metadata[itemIdentifier]
-    const cachedItem = cachedList[identifier]
+  function updateIfNewer (kind, cachedMap, item) {
+    const identifier = kind === 'issue'
+      ? item.metadata.number
+      : item.metadata.id
+    const cachedItem = cachedMap.get(identifier)
     if (cachedItem) {
       if (isCachedItemOlder(cachedItem, item)) {
         if (!_.isEqual(cachedItem, item)) {
           logger.trace('updating', kind, identifier)
-          cachedList[identifier] = item
+          cachedMap.set(identifier, item)
           emitModified(kind, item)
         }
       } else {
         logger.warn(`skipped updating ${kind} with id ${identifier} as it was older`)
       }
     } else {
       logger.trace('adding new', kind, identifier)
-      cachedList[identifier] = item
+      cachedMap.set(identifier, item)
       emitAdded(kind, item)
     }
     return item

backend/lib/config/gardener.js

@@ -131,30 +131,22 @@ function parseConfigValue (value, type) {
   }
 }
 
-function getValueFromFile (filePath, type) {
-  try {
-    const value = fs.readFileSync(filePath, 'utf8')
-    return parseConfigValue(value, type)
-  } catch (error) {
-    return undefined
-  }
-}
-
-function getValueFromEnvironmentOrFile (env, environmentVariableName, filePath, type) {
-  const value = parseConfigValue(env[environmentVariableName], type)
-  if (value !== undefined) {
-    return value
-  }
-
-  if (filePath) {
-    return getValueFromFile(filePath, type)
-  }
-}
-
 module.exports = {
   assignConfigFromEnvironmentAndFileSystem (config, env) {
-    for (const { environmentVariableName, configPath, filePath, type = 'String' } of configMappings) {
-      const value = getValueFromEnvironmentOrFile(env, environmentVariableName, filePath, type)
+    for (const configMapping of configMappings) {
+      const {
+        environmentVariableName,
+        configPath,
+        filePath,
+        type = 'String'
+      } = configMapping
+      let rawValue = env[environmentVariableName] // eslint-disable-line security/detect-object-injection
+      if (!rawValue && filePath) {
+        try {
+          rawValue = fs.readFileSync(filePath, 'utf8') // eslint-disable-line security/detect-non-literal-fs-filename
+        } catch (err) { /* ignore error */ }
+      }
+      const value = parseConfigValue(rawValue, type)
 
       if (value !== undefined) {
         _.set(config, configPath, value)
@@ -224,6 +216,7 @@ module.exports = {
     return config
   },
   readConfig (path) {
-    return yaml.load(fs.readFileSync(path, 'utf8'))
+    const data = fs.readFileSync(path, 'utf8') // eslint-disable-line security/detect-non-literal-fs-filename
+    return yaml.load(data)
   }
 }

backend/lib/hooks.js

@@ -50,11 +50,12 @@ class LifecycleHooks {
     this.io = io(server, cache)
     // register watches
     for (const [key, watch] of Object.entries(watches)) {
-      if (informers[key]) {
+      const informer = informers[key] // eslint-disable-line security/detect-object-injection
+      if (informer) {
         if (key === 'leases') {
-          watch(this.io, informers[key], { signal: this.ac.signal })
+          watch(this.io, informer, { signal: this.ac.signal })
         } else {
-          watch(this.io, informers[key])
+          watch(this.io, informer)
         }
       }
     }

backend/lib/middleware.js

@@ -55,10 +55,10 @@ function errorToLocals (err, req) {
     ? { name, stack }
     : { name }
   let code = 500
-  let reason = STATUS_CODES[code]
+  let reason = _.get(STATUS_CODES, [code])
   if (isHttpError(err)) {
     code = err.statusCode
-    reason = STATUS_CODES[code]
+    reason = _.get(STATUS_CODES, [code])
   }
   if (code < 100 || code >= 600) {
     code = 500

backend/lib/routes/config.js

@@ -41,8 +41,9 @@ router.route('/')
 function sanitizeFrontendConfig (frontendConfig) {
   const converter = markdown.createConverter()
   const convertAndSanitize = (obj, key) => {
-    if (obj[key]) {
-      obj[key] = converter.makeSanitizedHtml(obj[key])
+    const value = obj[key] // eslint-disable-line security/detect-object-injection -- key is a local fixed string
+    if (value) {
+      obj[key] = converter.makeSanitizedHtml(value) // eslint-disable-line security/detect-object-injection -- key is a local fixed string
     }
   }
 

backend/lib/routes/terminals.js

@@ -8,7 +8,6 @@
 
 const express = require('express')
 const { terminals, authorization } = require('../services')
-const _ = require('lodash')
 const { UnprocessableEntity } = require('http-errors')
 const { metricsRoute } = require('../middleware')
 
@@ -41,10 +40,33 @@ router.route('/')
 
       const { method, params: body } = req.body
 
-      if (!_.includes(['create', 'fetch', 'list', 'config', 'remove', 'heartbeat', 'listProjectTerminalShortcuts'], method)) {
-        throw new UnprocessableEntity(`${method} not allowed for terminals`)
+      let terminalOperation
+      switch (method) {
+        case 'create':
+          terminalOperation = terminals.create
+          break
+        case 'fetch':
+          terminalOperation = terminals.fetch
+          break
+        case 'list':
+          terminalOperation = terminals.list
+          break
+        case 'config':
+          terminalOperation = terminals.config
+          break
+        case 'remove':
+          terminalOperation = terminals.remove
+          break
+        case 'heartbeat':
+          terminalOperation = terminals.heartbeat
+          break
+        case 'listProjectTerminalShortcuts':
+          terminalOperation = terminals.listProjectTerminalShortcuts
+          break
+        default:
+          throw new UnprocessableEntity(`${method} not allowed for terminals`)
       }
-      res.send(await terminals[method]({ user, body }))
+      res.send(await terminalOperation.call(terminals, { user, body }))
     } catch (err) {
       next(err)
     }