Restrain from using insecure literals for stop-commands token

[quapka]

The {token} in the workflow command ::stop-commands:{token} has to be a sufficiently random and secure value. Definitely not a string literal. See the docs for more details. Quick grepping through then code base shows three such insecure occurrences:

$ rg stop-commands
setup/src/installer.ts
240:  console.log('::stop-commands::SetupHaskellStopCommands');

setup/lib/installer.js
211:    console.log('::stop-commands::SetupHaskellStopCommands');

setup/dist/index.js
11908:    console.log('::stop-commands::SetupHaskellStopCommands');

All three code snippets seem to be copy-paste of the same logic, where some choco command is called and probably causes some unwanted commands to be run. As such I don't see it being a security issue per se, but still worth fixing to use the best practice for stop-commands. I'm not familiar enough with the codebase to propose a PR, but something like the following patch might do the job:

diff --git a/setup/src/installer.ts b/setup/src/installer.ts
index 591470f..38b267f 100644
--- a/setup/src/installer.ts
+++ b/setup/src/installer.ts
@@ -8,6 +8,7 @@ import {ghcup_version, OS, Tool} from './opts';
 import process from 'process';
 import * as glob from '@actions/glob';
 import * as fs from 'fs';
+import randomBytes from 'crypto';
 
 // Don't throw on non-zero.
 const exec = async (cmd: string, args?: string[]): Promise<number> =>
@@ -237,7 +238,8 @@ async function apt(tool: Tool, version: string): Promise<void> {
 async function choco(tool: Tool, version: string): Promise<void> {
   core.info(`Attempting to install ${tool} ${version} using chocolatey`);
   // Choco tries to invoke `add-path` command on earlier versions of ghc, which has been deprecated and fails the step, so disable command execution during this.
-  console.log('::stop-commands::SetupHaskellStopCommands');
+  const stopOrResumeCommandsToken = randomBytes(32).toString('hex');
+  console.log(`::stop-commands::${stopOrResumeCommandsToken}`);
   const args = [
     'choco',
     'install',
@@ -250,7 +252,7 @@ async function choco(tool: Tool, version: string): Promise<void> {
   ];
   if ((await exec('powershell', args)) !== 0)
     await exec('powershell', [...args, '--pre']);
-  console.log('::SetupHaskellStopCommands::'); // Re-enable command execution
+  console.log(`::${stopOrResumeCommandsToken}::`); // Re-enable command execution
   // Add GHC to path automatically because it does not add until the end of the step and we check the path.
 
   const chocoPath = await getChocoPath(tool, version);

A proper cryptographic module for generating secure random tokens should be used. As a sidenote it is worth thinking of adding a security policy for the project.

[hazelweakly]

Thanks for the report! That documentation wasn't there when the stop commands were first added (and, annoyingly, their suggested deterministic-but-random method of getting a token doesn't seem easily possible to do inside the action automatically).

I'll modify the action to use a more secure token for stopping/enabling command execution (although honestly almost all of the logic of the action should be wrapped in stop/resume because it's mostly just downloading scripts and then executing them).

---

It looks like adding a security policy amounts to adding a SECURITY.md file and nothing more. I can certainly do that, but it would just be "open an issue" for now as the scope of the action is so limited already.

[quapka]

Thanks for the report! That documentation wasn't there when the stop commands were first added (and, annoyingly, their suggested deterministic-but-random method of getting a token doesn't seem easily possible to do inside the action automatically).

Yes, the respective part of the docs has been updated only recently.

I'll modify the action to use a more secure token for stopping/enabling command execution (although honestly almost all of the logic of the action should be wrapped in stop/resume because it's mostly just downloading scripts and then executing them).

I see, I'm for the safer option.

---

It looks like adding a security policy amounts to adding a SECURITY.md file and nothing more. I can certainly do that, but it would just be "open an issue" for now as the scope of the action is so limited already.

Doesn't sound like much of a difference, but I think it is better since it is explicit :)