Skip to content

Fix code scanning alert - Shell command built from environment values #198

@josecelano

Description

@josecelano

Tracking issue for:

The Gitrepo class contains a two calls to execSync:

  • execSync(git -C ${this.getDirPath()} status, {stdio: 'ignore'})
  • execSync(git -C ${this.dir.getDirPath()} log -n 0, {stdio: 'ignore'})

https://github.com/Nautilus-Cyberneering/git-queue/blob/main/src/git-repo.ts

Since we use a class for the dir we are safe against shell injection as long as the class validates the dir:

https://github.com/Nautilus-Cyberneering/git-queue/blob/d47bd36bdad81487862ae73f3464a89c1a4fe9bd/src/git-repo-dir.ts#L9

Anyway, we could refactor it in order to avoid getting alerts from Code scanning.

They suggest:

var cp = require("child_process"),
  path = require("path");
function cleanupTemp() {
  let cmd = "rm",
    args = ["-rf", path.join(__dirname, "temp")];
  cp.execFileSync(cmd, args); // GOOD
}

instead of:

var cp = require("child_process"),
  path = require("path");
function cleanupTemp() {
  let cmd = "rm -rf " + path.join(__dirname, "temp");
  cp.execSync(cmd); // BAD
}

For the Librarian we build a wrapper to execute console commands.

I think it's a good idea because:

  • It forces the user to pass the arguments for the console command with other function parameters that could be escaped.
  • We can check easily check that all console commands are executed via the wrapper.
  • If the validation of the object fails or stops working we are still safe against shell injection because we escape the arguments. For example, if the dir class validation is accidentally removed the console command will fail with a malicious string.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions