Move eval to parseInt

[mawburn]

Summary

Using parseInt is preferred over eval for handling environment variables because parseInt safely converts a string to an integer without executing any code. In contrast, eval can execute arbitrary code, which poses a significant security risk if the environment variable contains malicious input. By using parseInt, we mitigate the risk of code injection attacks and ensure that only numerical values are processed.

While this isn't used in a whole lot of places and the chances of a malicious environment variable is low, it's also a significant performance boost.

Running this script:

const { performance } = require('perf_hooks')

const numStr = '12345'
const iterations = 10000000

const measureTime = (fn) => {
  const start = performance.now()
  fn()
  const end = performance.now()
  return end - start
}

const evalTest = () => {
  for (let i = 0; i < iterations; i++) {
    eval(numStr)
  }
}

const parseIntTest = () => {
  for (let i = 0; i < iterations; i++) {
    parseInt(numStr, 10)
  }
}

const evalTime = measureTime(evalTest)
const parseIntTime = measureTime(parseIntTest)

console.log(`eval time: ${evalTime.toFixed(2)} ms`)
console.log(`parseInt time: ${parseIntTime.toFixed(2)} ms`)
console.log(`parseInt is ${(evalTime / parseIntTime).toFixed(2)} times faster than eval`)

I get this output on my local machine (MBA M1-series first gen):

eval time: 1745.03 ms
parseInt time: 39.69 ms
parseInt is 43.97 times faster than eval

Change Type

Please delete any irrelevant options.

• Bug fix (non-breaking change which fixes an issue)

Testing

Tests should pass as normal and no changes should be seen within the app. It should work the same, unless there were environment variable based code being injected.

Checklist

Please delete any irrelevant options.

• My code adheres to this project's style guidelines
• I have performed a self-review of my own code
• Local unit tests pass with my changes

Copilot Summary

This pull request includes changes to improve the security and robustness of the codebase. The most important changes involve replacing the eval function with parseInt in several files to parse environment variables. This change improves security by avoiding the potential execution of malicious code.

Changes to environment variable parsing:

• api/models/Session.js: Replaced eval with parseInt for parsing the REFRESH_TOKEN_EXPIRY environment variable.
• api/models/userMethods.js: Replaced eval with parseInt for parsing the SESSION_EXPIRY environment variable.
• api/server/services/AuthService.js: Replaced eval with parseInt for parsing the REFRESH_TOKEN_EXPIRY environment variable.

Changes to utility functions:

• api/server/utils/math.js: Replaced eval with parseInt in the math function and updated the error message to reflect the change.

[Qubhis]

I assume that eval is correct in this place since there is a regex above checking for a valid string containing only digits and mathematical characters, i.e. numerical equation.

[mawburn]

The parseInt will turn it into a base 10 number and isNan will check if it succeeded.

[Qubhis]

The general issue with this is to change the .env values for REFRESH_TOKEN_EXPIRY and SESSION_EXPIRY as they are written as calculation, e.g. 1000 * 60 * 15

While this is more readable in env, I would rather include comments to the .env file than execute eval in unsafe places. Ot at least used the math function which checks for the string validity

[danny-avila]

While I think this update is necessary I'm concerned of backwards compatibility. Thinking how to handle this gracefully so it doesn't create so many tickets.

[Qubhis]

I see. So, what about keeping only the math function and use it instead of eval. Then, the math function gonna check for the string if it contains only digits and then it uses parseInt, otherwise it would use the eval (as there is the regex checking for the valid string equation). There gonna be an announcement that users should migrate their env values to contain only digits, for performance improvements. And the breaking change would be then introduced only when releasing a major version (in future). ?

[mawburn]

The general issue with this is to change the .env values for REFRESH_TOKEN_EXPIRY and SESSION_EXPIRY as they are written as calculation, e.g. 1000 * 60 * 15

I honestly didn't think of that. Good catch. This change isn't really worth it in that case. The alternatives are slower, this is only executed a couple times during the lifecycle of the app, and the attack vector is low.

It's not bad to use 1000 * 60 * 15 when setting a variable, but it's bad practice putting it in ENV files... but it's already established in the ENV files that this is ok.

If your ENV vars are a legitimate attack vector, then you have much more serious problems. 🤣