Potential fix for code scanning alert no. 2: Indirect uncontrolled command line

[eschultink]

Potential fix for https://github.com/Worklytics/datastore-backup/security/code-scanning/2

In general, to fix this kind of issue you should avoid passing a single concatenated string to child_process.exec/execSync when it includes untrusted data. Instead, use spawn/spawnSync/execFile/execFileSync with an array of arguments. This prevents the shell from interpreting metacharacters and removes the need to manually escape arguments.

For this specific code:

• Replace child_process.execSync(datastoreRestoreCommand(...)) with a safe helper that:
  • Builds an array of arguments for gcloud.
  • Calls child_process.spawnSync('gcloud', args, { encoding: 'utf8' }) (or similar).
  • Throws or logs an error if the command fails.
• Change datastoreRestoreCommand and datastoreStatusCommand from returning shell command strings to returning arrays of arguments (or small objects containing {cmd, args}) that can be passed directly to spawnSync / printed out for the user.
• In index.js, where these helpers are used just for printing example commands (restore action, status monitoring lines), reconstruct printable command strings by joining the safe arg arrays with spaces, rather than using the old string-building helpers.

Concretely, in lib/datastore-backup.js:

1. Add small helpers:
   • buildDatastoreRestoreArgs(kinds, project, bucket, timestamp, options) → returns an array like:
     ['datastore', 'import', '--project', project, '--kinds', 'Kind1,Kind2', '--async', backupMetadataFile, ...(account args)].
   • buildDatastoreStatusArgs(project, options) → returns:
     ['datastore', 'operations', 'list', '--project', project, ...(account args)].
   • execGcloud(args) → calls spawnSync('gcloud', args, { encoding: 'utf8' }) and returns stdout (or throws on error).
2. Update testRestoreFromBackup to:
   • Call execGcloud(buildDatastoreRestoreArgs(...)).
   • Call execGcloud(buildDatastoreStatusArgs(...)).
3. Change datastoreRestoreCommand and datastoreStatusCommand to be string-formatting wrappers around the arg builders, for use in index.js where the user sees and copies commands. They should no longer be used with execSync.

In index.js:

• Leave the high-level logic intact but rely on the updated datastoreRestoreCommand/datastoreStatusCommand that return full, ready-to-run gcloud commands as strings. This keeps the CLI output behavior unchanged.
• No change is needed for the backup flow using the Datastore client library; that doesn’t use shell commands.
• The list command still uses execSync('gsutil ls ' + bucketPrefix ...), which is a similar pattern but not currently in the flagged path. Since we are constrained to address the provided alert and its path, and changing that might be considered functional behavior, we will not modify it here.

This preserves functionality (the commands and printed examples remain the same) while ensuring the actually executed gcloud commands are invoked without a shell, eliminating the indirect command line injection risk.

---

Suggested fixes powered by Copilot Autofix. Review carefully before merging.

[Copilot]

Pull request overview

This PR addresses a code-scanning alert for indirect command injection by switching from shell-based command execution to spawnSync with argument arrays for gcloud invocations.

Changes:

• Introduced execGcloud() to run gcloud safely without a shell and to surface failures.
• Added argument builders for datastore restore/status commands and updated restore test flow to use them.
• Converted restore/status “command” helpers into printable string builders (rather than executable shell strings).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

spawnSync can return status === null when the process is terminated by a signal; the current error message will report “code null” and lose the actual reason. Consider including result.signal (and formatting the message differently when status is null) so failures are diagnosable.

Suggested change

	throw new Error(`gcloud ${args.join(' ')} failed with code ${result.status}: ${stderr || stdout}`);
	const status = result.status;
	const signal = result.signal;
	const reason = (status === null && signal)
	? `terminated by signal ${signal}`
	: `failed with code ${status}`;
	throw new Error(`gcloud ${args.join(' ')} ${reason}: ${stderr || stdout}`);

[Copilot]

Building a copy-pasteable command by joining args with spaces can produce an unsafe/misleading shell command when any arg contains whitespace or shell metacharacters (project IDs, account emails, or GCS paths with unusual characters). Since these strings are intended for users to paste into a shell, consider shell-escaping/quoting each arg (e.g., via a small escape helper or a vetted “shell-escape” utility) before joining.

[Copilot]

Account handling is inconsistent between the two arg builders: restore uses !_.isUndefined(options.account) while status uses a truthy check (if (options && options.account)). This can lead to different behavior for empty-string accounts or other falsy-but-present values. Consider using the same predicate in both builders to keep behavior predictable.