fix(dotnet): should work with paths that contain spaces

[asinino]

Please review the code before merging it.

[nx-cloud]

☁️ Nx Cloud Report

CI ran the following commands for commit 4ee665d. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

✅ Successfully ran 5 targets

• nx run-many --target=e2e --all
• nx run-many --all --target=build
• nx affected --target=build --base=origin/master --parallel
• nx affected --target=test --base=origin/master --parallel
• nx affected --target=lint --base=origin/master --parallel

---

Sent with 💌 from NxCloud.

[AgentEnder]

Hey @asinino, I've been looking over the code and I'm not sure why the format executor is failing on this branch. It seems like the isNet60OrHigher flag is set incorrectly, I'll look into this a bit

[AgentEnder]

Why are we swapping to spawnSync over execSync here? This is more complex maintainability-wise and shouldn't be needed.

[AgentEnder]

I'm not totally opposed to spawnSync actually, some stuff does work out better this way.

[asinino]

Sorry, had a lengthy meeting couldn't look into this.

I converted to spawnSync to avoid converting all parameters to string first, spawnSync let us pass each parameter on the array so it is automatically escaped.
With execSync I had some problems passing on "Security Hotspot" tests because it allowed the user to pass arbitrary commands to execSync, like appending in the last parameter: "&& rm -rf *" this would delete everything on the current folder. Using the spawnSync command prevents the user to do this.

[AgentEnder]

So with nx-plugins and other dev tooling in general, the hotspots that pop up from sonarqube can be mostly ignored. The reason being that those commands are being run by the developer, on their own machine. It's not really an attack point, as someone would already have to be able to run arbitrary commands to exercise it.

The solution for most things that come up like that, are just for me to hit the "not a problem" button while reviewing the PR.

[asinino]

That would be a lot easier because I had to change the method a lot, I'll take it into account next time, for this one I was kinda in a rush to have a deployable version because the previous one was broken.

[AgentEnder]

Adding 'inherit' here causes the returned buffers to be empty

[asinino]

I have a new solution for this have to discuss with you, I didn't merge it yet. This should resolve the problem:

  private execute(params: string[], stdio: StdioOptions = 'pipe'): Buffer {
    const proc = spawnSync(this.cliCommand.command, params, {
      stdio,
      cwd: this.cwd || process.cwd(),
    });

    let result = Buffer.alloc(0);
    if (proc?.stdout) {
      result = Buffer.concat([result, proc.stdout]);
    }
    if (proc?.stderr) {
      result = Buffer.concat([result, proc.stderr]);
    }
    return result;
  }

[AgentEnder]

This would work, but its fine to duplicate the spawnSync call for this. The idea behind execute is that we are doing some kind of processing to the Buffer on our side, and we need it. If we are displaying the outputs of the command to the user, its the wrong method to use.

[asinino]

I agree with different calls for spawnSync with different behavior, it's easier to not mess it up.

[AgentEnder]

This method needs the 'stdio: inherit' so that things like nx build show terminal output.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[github-actions]

🎉 This PR is included in version 1.9.5 🎉

The release is available on:

• v1.9.5
• GitHub release

Your semantic-release bot 📦🚀

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.