fix(secrets): prevent shell injection in secret key names and mask values

- Validate secret key names against alphanumeric allowlist before shell interpolation
- Apply validation in both SecretSourceService.fetchSecret() and legacy queryOverride()
- Mask fetched secret values with core.setSecret() to prevent log exposure
- Add 20 new tests for validation and masking

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

src/model/orchestrator/options/orchestrator-query-override.ts

@@ -1,7 +1,8 @@
+import * as core from '@actions/core';
 import Input from '../../input';
 import { GenericInputReader } from '../../input-readers/generic-input-reader';
 import OrchestratorOptions from './orchestrator-options';
-import { SecretSourceService } from '../services/secrets/secret-source-service';
+import { SecretSourceService, validateSecretKey } from '../services/secrets/secret-source-service';
 import OrchestratorLogger from '../services/core/orchestrator-logger';
 
 const formatFunction = (value: string, arguments_: any[]) => {
@@ -49,9 +50,19 @@ class OrchestratorQueryOverride {
       throw new Error(`Should not be trying to run override query on ${query}`);
     }
 
-    return await GenericInputReader.Run(
+    // Validate the query key before interpolating it into a shell command
+    validateSecretKey(query);
+
+    const result = await GenericInputReader.Run(
       formatFunction(OrchestratorOptions.inputPullCommand, [{ key: 0, value: query }]),
     );
+
+    // Mask the fetched secret value so it does not appear in GitHub Actions logs
+    if (result && result.trim().length > 0) {
+      core.setSecret(result);
+    }
+
+    return result;
   }
 
   /**
@@ -80,10 +91,7 @@ class OrchestratorQueryOverride {
         if (definitions.length > 0) {
           OrchestratorLogger.log(`Loaded ${definitions.length} secret source(s) from ${secretSource}`);
           for (const key of queries) {
-            OrchestratorQueryOverride.queryOverrides[key] = await SecretSourceService.fetchSecret(
-              definitions[0],
-              key,
-            );
+            OrchestratorQueryOverride.queryOverrides[key] = await SecretSourceService.fetchSecret(definitions[0], key);
           }
         }
 

src/model/orchestrator/services/secrets/secret-source-service.test.ts

@@ -1,7 +1,14 @@
 import fs from 'node:fs';
-import { SecretSourceService } from './secret-source-service';
+import * as core from '@actions/core';
+import { SecretSourceService, validateSecretKey } from './secret-source-service';
 
 jest.mock('node:fs');
+jest.mock('@actions/core', () => ({
+  setSecret: jest.fn(),
+  info: jest.fn(),
+  warning: jest.fn(),
+  error: jest.fn(),
+}));
 jest.mock('../core/orchestrator-system', () => ({
   OrchestratorSystem: {
     Run: jest.fn().mockResolvedValue(''),
@@ -23,6 +30,69 @@ describe('SecretSourceService', () => {
     jest.clearAllMocks();
   });
 
+  describe('validateSecretKey', () => {
+    it('should accept alphanumeric keys', () => {
+      expect(validateSecretKey('MY_SECRET_KEY')).toBe('MY_SECRET_KEY');
+    });
+
+    it('should accept keys with hyphens', () => {
+      expect(validateSecretKey('my-secret-key')).toBe('my-secret-key');
+    });
+
+    it('should accept keys with dots', () => {
+      expect(validateSecretKey('my.secret.key')).toBe('my.secret.key');
+    });
+
+    it('should accept keys with forward slashes', () => {
+      expect(validateSecretKey('path/to/secret')).toBe('path/to/secret');
+    });
+
+    it('should accept keys with mixed valid characters', () => {
+      expect(validateSecretKey('my-app/prod_db.password')).toBe('my-app/prod_db.password');
+    });
+
+    it('should reject keys with semicolons (shell injection)', () => {
+      expect(() => validateSecretKey('; rm -rf /')).toThrow('Invalid secret key name');
+    });
+
+    it('should reject keys with backticks (command substitution)', () => {
+      expect(() => validateSecretKey('`whoami`')).toThrow('Invalid secret key name');
+    });
+
+    it('should reject keys with dollar signs (variable expansion)', () => {
+      expect(() => validateSecretKey('$HOME')).toThrow('Invalid secret key name');
+    });
+
+    it('should reject keys with pipe characters', () => {
+      expect(() => validateSecretKey('key | cat /etc/passwd')).toThrow('Invalid secret key name');
+    });
+
+    it('should reject keys with ampersands', () => {
+      expect(() => validateSecretKey('key && echo pwned')).toThrow('Invalid secret key name');
+    });
+
+    it('should reject keys with newlines', () => {
+      expect(() => validateSecretKey('key\nmalicious')).toThrow('Invalid secret key name');
+    });
+
+    it('should reject keys with quotes', () => {
+      expect(() => validateSecretKey('"key"')).toThrow('Invalid secret key name');
+      expect(() => validateSecretKey("'key'")).toThrow('Invalid secret key name');
+    });
+
+    it('should reject keys with parentheses (subshell)', () => {
+      expect(() => validateSecretKey('$(whoami)')).toThrow('Invalid secret key name');
+    });
+
+    it('should reject empty keys', () => {
+      expect(() => validateSecretKey('')).toThrow('Invalid secret key name');
+    });
+
+    it('should reject keys with spaces', () => {
+      expect(() => validateSecretKey('key with spaces')).toThrow('Invalid secret key name');
+    });
+  });
+
   describe('isPremadeSource', () => {
     it('should return true for aws-secrets-manager', () => {
       expect(SecretSourceService.isPremadeSource('aws-secrets-manager')).toBe(true);
@@ -111,11 +181,7 @@ describe('SecretSourceService', () => {
       const result = await SecretSourceService.fetchSecret(source, 'MY_SECRET');
 
       expect(result).toBe('my-secret-value');
-      expect(OrchestratorSystem.Run).toHaveBeenCalledWith(
-        expect.stringContaining('MY_SECRET'),
-        false,
-        true,
-      );
+      expect(OrchestratorSystem.Run).toHaveBeenCalledWith(expect.stringContaining('MY_SECRET'), false, true);
     });
 
     it('should parse JSON output when parseOutput is json-field', async () => {
@@ -157,6 +223,71 @@ describe('SecretSourceService', () => {
 
       expect(result).toBe('');
     });
+
+    it('should reject keys with shell injection characters', async () => {
+      const source = SecretSourceService.resolveSource('aws-secrets-manager')!;
+
+      await expect(SecretSourceService.fetchSecret(source, '; rm -rf /')).rejects.toThrow('Invalid secret key name');
+    });
+
+    it('should reject keys with command substitution', async () => {
+      const source = SecretSourceService.resolveSource('aws-secrets-manager')!;
+
+      await expect(SecretSourceService.fetchSecret(source, '$(whoami)')).rejects.toThrow('Invalid secret key name');
+    });
+
+    it('should reject keys with backtick command substitution', async () => {
+      const source = SecretSourceService.resolveSource('aws-secrets-manager')!;
+
+      await expect(SecretSourceService.fetchSecret(source, '`cat /etc/passwd`')).rejects.toThrow(
+        'Invalid secret key name',
+      );
+    });
+
+    it('should accept keys with valid path-like patterns', async () => {
+      const { OrchestratorSystem } = require('../core/orchestrator-system');
+      OrchestratorSystem.Run.mockResolvedValue('secret-value');
+
+      const source = SecretSourceService.resolveSource('aws-secrets-manager')!;
+      const result = await SecretSourceService.fetchSecret(source, 'prod/database/password');
+
+      expect(result).toBe('secret-value');
+    });
+
+    it('should mask fetched secret values with core.setSecret', async () => {
+      const { OrchestratorSystem } = require('../core/orchestrator-system');
+      OrchestratorSystem.Run.mockResolvedValue('super-secret-value');
+
+      const source = SecretSourceService.resolveSource('aws-secrets-manager')!;
+      await SecretSourceService.fetchSecret(source, 'MY_SECRET');
+
+      expect(core.setSecret).toHaveBeenCalledWith('super-secret-value');
+    });
+
+    it('should not mask empty secret values', async () => {
+      const { OrchestratorSystem } = require('../core/orchestrator-system');
+      OrchestratorSystem.Run.mockResolvedValue('');
+
+      const source = SecretSourceService.resolveSource('aws-secrets-manager')!;
+      await SecretSourceService.fetchSecret(source, 'MY_SECRET');
+
+      expect(core.setSecret).not.toHaveBeenCalled();
+    });
+
+    it('should mask JSON-extracted secret values', async () => {
+      const { OrchestratorSystem } = require('../core/orchestrator-system');
+      OrchestratorSystem.Run.mockResolvedValue(JSON.stringify({ value: 'json-secret' }));
+
+      const source = {
+        name: 'test-source',
+        command: 'fetch {0}',
+        parseOutput: 'json-field' as const,
+        jsonField: 'value',
+      };
+      await SecretSourceService.fetchSecret(source, 'KEY');
+
+      expect(core.setSecret).toHaveBeenCalledWith('json-secret');
+    });
   });
 
   describe('fetchFromEnv', () => {
@@ -171,6 +302,19 @@ describe('SecretSourceService', () => {
       const result = SecretSourceService.fetchFromEnv('NONEXISTENT_KEY_12345');
       expect(result).toBe('');
     });
+
+    it('should mask env var values with core.setSecret', () => {
+      process.env.TEST_MASK_KEY = 'masked-env-value';
+      SecretSourceService.fetchFromEnv('TEST_MASK_KEY');
+      expect(core.setSecret).toHaveBeenCalledWith('masked-env-value');
+      delete process.env.TEST_MASK_KEY;
+    });
+
+    it('should not mask empty env var values', () => {
+      const result = SecretSourceService.fetchFromEnv('NONEXISTENT_KEY_99999');
+      expect(result).toBe('');
+      expect(core.setSecret).not.toHaveBeenCalled();
+    });
   });
 
   describe('fetchAll', () => {
@@ -189,9 +333,7 @@ describe('SecretSourceService', () => {
 
     it('should fetch all keys from premade source', async () => {
       const { OrchestratorSystem } = require('../core/orchestrator-system');
-      OrchestratorSystem.Run
-        .mockResolvedValueOnce('secret-1')
-        .mockResolvedValueOnce('secret-2');
+      OrchestratorSystem.Run.mockResolvedValueOnce('secret-1').mockResolvedValueOnce('secret-2');
 
       const results = await SecretSourceService.fetchAll('aws-parameter-store', ['param1', 'param2']);
 

src/model/orchestrator/services/secrets/secret-source-service.ts

@@ -1,4 +1,5 @@
 import fs from 'node:fs';
+import * as core from '@actions/core';
 import OrchestratorLogger from '../core/orchestrator-logger';
 import { OrchestratorSystem } from '../core/orchestrator-system';
 
@@ -12,17 +13,47 @@ export interface SecretSourceDefinition {
   jsonField?: string;
 }
 
+/**
+ * Validate that a secret key name contains only safe characters.
+ * Prevents shell injection when keys are interpolated into commands.
+ *
+ * Allowed characters: alphanumeric, hyphens, underscores, dots, forward slashes.
+ *
+ * @param key - The secret key name to validate
+ * @returns The validated key (unchanged)
+ * @throws Error if the key contains disallowed characters
+ */
+export function validateSecretKey(key: string): string {
+  if (!/^[a-zA-Z0-9\-_./]+$/.test(key)) {
+    throw new Error(
+      `Invalid secret key name: "${key}". Keys may only contain alphanumeric characters, hyphens, underscores, dots, and forward slashes.`,
+    );
+  }
+
+  return key;
+}
+
+/**
+ * Mask a secret value so it does not appear in GitHub Actions logs.
+ * Empty or whitespace-only values are skipped (core.setSecret would be a no-op).
+ */
+function maskSecretValue(value: string): void {
+  if (value.trim().length > 0) {
+    core.setSecret(value);
+  }
+}
+
 /**
  * Premade secret sources and custom YAML-based secret source definitions.
  *
  * Premade sources are string shortcuts that expand to shell commands:
- *   - `aws-secrets-manager` — AWS Secrets Manager
- *   - `aws-parameter-store` — AWS Systems Manager Parameter Store
- *   - `gcp-secret-manager` — Google Cloud Secret Manager
- *   - `azure-key-vault` — Azure Key Vault (requires AZURE_VAULT_NAME env var)
- *   - `hashicorp-vault` — HashiCorp Vault KV v2 (requires VAULT_ADDR, optionally VAULT_MOUNT)
- *   - `hashicorp-vault-kv1` — HashiCorp Vault KV v1 (requires VAULT_ADDR, optionally VAULT_MOUNT)
- *   - `env` — Read from environment variables (no shell command needed)
+ *   - `aws-secrets-manager` -- AWS Secrets Manager
+ *   - `aws-parameter-store` -- AWS Systems Manager Parameter Store
+ *   - `gcp-secret-manager` -- Google Cloud Secret Manager
+ *   - `azure-key-vault` -- Azure Key Vault (requires AZURE_VAULT_NAME env var)
+ *   - `hashicorp-vault` -- HashiCorp Vault KV v2 (requires VAULT_ADDR, optionally VAULT_MOUNT)
+ *   - `hashicorp-vault-kv1` -- HashiCorp Vault KV v1 (requires VAULT_ADDR, optionally VAULT_MOUNT)
+ *   - `env` -- Read from environment variables (no shell command needed)
  *
  * Custom YAML format:
  *   sources:
@@ -76,7 +107,7 @@ export class SecretSourceService {
       command: 'vault read -mount="${VAULT_MOUNT:-secret}" -field=value {0}',
       parseOutput: 'raw',
     },
-    'vault': {
+    vault: {
       // Short alias for hashicorp-vault (KV v2)
       name: 'vault',
       command: 'vault kv get -mount="${VAULT_MOUNT:-secret}" -field=value {0}',
@@ -157,29 +188,41 @@ export class SecretSourceService {
   /**
    * Fetch a secret value using the given source definition.
    *
+   * Validates the key against an allowlist pattern before interpolating it
+   * into the command string to prevent shell injection. The fetched secret
+   * value is masked via core.setSecret() so it does not leak in logs.
+   *
    * @param source - The secret source definition to use
    * @param key - The secret key to fetch
    * @returns The secret value, or empty string on failure
    */
   static async fetchSecret(source: SecretSourceDefinition, key: string): Promise<string> {
+    // Validate the key to prevent shell injection
+    validateSecretKey(key);
+
     const command = source.command.replace(/\{0\}/g, key);
 
     try {
       const output = await OrchestratorSystem.Run(command, false, true);
 
+      let value: string;
+
       if (source.parseOutput === 'json-field' && source.jsonField) {
         try {
           const parsed = JSON.parse(output);
-
-          return parsed[source.jsonField] || '';
+          value = parsed[source.jsonField] || '';
         } catch {
           OrchestratorLogger.logWarning(`Failed to parse JSON output from ${source.name} for key ${key}`);
-
-          return output.trim();
+          value = output.trim();
         }
+      } else {
+        value = output.trim();
       }
 
-      return output.trim();
+      // Mask the secret value so it does not appear in GitHub Actions logs
+      maskSecretValue(value);
+
+      return value;
     } catch (error: any) {
       OrchestratorLogger.logWarning(`Failed to fetch secret '${key}' from ${source.name}: ${error.message}`);
 
@@ -189,9 +232,13 @@ export class SecretSourceService {
 
   /**
    * Fetch a secret from an environment variable. No shell command needed.
+   * The value is masked via core.setSecret() so it does not leak in logs.
    */
   static fetchFromEnv(key: string): string {
-    return process.env[key] || '';
+    const value = process.env[key] || '';
+    maskSecretValue(value);
+
+    return value;
   }
 
   /**
@@ -201,10 +248,7 @@ export class SecretSourceService {
    * @param keys - List of secret keys to fetch
    * @returns Map of key -> value
    */
-  static async fetchAll(
-    sourceName: string,
-    keys: string[],
-  ): Promise<Record<string, string>> {
+  static async fetchAll(sourceName: string, keys: string[]): Promise<Record<string, string>> {
     const results: Record<string, string> = {};
 
     if (sourceName === 'env') {
@@ -218,7 +262,9 @@ export class SecretSourceService {
     const source = SecretSourceService.resolveSource(sourceName);
     if (!source) {
       OrchestratorLogger.logWarning(
-        `Unknown secret source '${sourceName}'. Available sources: ${SecretSourceService.getAvailableSources().join(', ')}`,
+        `Unknown secret source '${sourceName}'. Available sources: ${SecretSourceService.getAvailableSources().join(
+          ', ',
+        )}`,
       );
 
       return results;
@@ -254,19 +300,31 @@ export class SecretSourceService {
         }
 
         current = {
-          name: trimmed.replace('- name:', '').trim().replace(/^['"]|['"]$/g, ''),
+          name: trimmed
+            .replace('- name:', '')
+            .trim()
+            .replace(/^['"]|['"]$/g, ''),
           parseOutput: 'raw',
         };
         continue;
       }
 
       if (current && trimmed.startsWith('command:')) {
-        current.command = trimmed.replace('command:', '').trim().replace(/^['"]|['"]$/g, '');
+        current.command = trimmed
+          .replace('command:', '')
+          .trim()
+          .replace(/^['"]|['"]$/g, '');
       } else if (current && trimmed.startsWith('parseOutput:')) {
-        const value = trimmed.replace('parseOutput:', '').trim().replace(/^['"]|['"]$/g, '');
+        const value = trimmed
+          .replace('parseOutput:', '')
+          .trim()
+          .replace(/^['"]|['"]$/g, '');
         current.parseOutput = value as 'raw' | 'json-field';
       } else if (current && trimmed.startsWith('jsonField:')) {
-        current.jsonField = trimmed.replace('jsonField:', '').trim().replace(/^['"]|['"]$/g, '');
+        current.jsonField = trimmed
+          .replace('jsonField:', '')
+          .trim()
+          .replace(/^['"]|['"]$/g, '');
       }
     }