diff --git a/.github/workflows/release.lock.yml b/.github/workflows/release.lock.yml index 85e445009dc..d3c78451e6b 100644 --- a/.github/workflows/release.lock.yml +++ b/.github/workflows/release.lock.yml @@ -6272,13 +6272,13 @@ jobs: - name: Download Go modules run: go mod download - name: Generate SBOM (SPDX format) - uses: anchore/sbom-action@fbfd9c6c189226748411491745178e0c2017392d # v0 + uses: anchore/sbom-action@fbfd9c6c189226748411491745178e0c2017392d # v0.20.10 with: artifact-name: sbom.spdx.json format: spdx-json output-file: sbom.spdx.json - name: Generate SBOM (CycloneDX format) - uses: anchore/sbom-action@fbfd9c6c189226748411491745178e0c2017392d # v0 + uses: anchore/sbom-action@fbfd9c6c189226748411491745178e0c2017392d # v0.20.10 with: artifact-name: sbom.cdx.json format: cyclonedx-json diff --git a/pkg/workflow/bundler_validation.go b/pkg/workflow/bundler_validation.go index 5f01676c434..67c8a0de2e7 100644 --- a/pkg/workflow/bundler_validation.go +++ b/pkg/workflow/bundler_validation.go @@ -215,6 +215,105 @@ func ValidateEmbeddedResourceRequires(sources map[string]string) error { return nil } +// validateNoExecSync checks that GitHub Script mode scripts do not use execSync +// GitHub Script mode should use exec instead for better async/await handling +// Returns an error if execSync is found, otherwise returns nil +func validateNoExecSync(scriptName string, content string, mode RuntimeMode) error { + // Only validate GitHub Script mode + if mode != RuntimeModeGitHubScript { + return nil + } + + bundlerValidationLog.Printf("Validating no execSync in GitHub Script: %s (%d bytes)", scriptName, len(content)) + + // Regular expression to match execSync usage + // Matches: execSync(...) with various patterns + execSyncRegex := regexp.MustCompile(`\bexecSync\s*\(`) + + lines := strings.Split(content, "\n") + var foundUsages []string + + for lineNum, line := range lines { + trimmed := strings.TrimSpace(line) + + // Skip comment lines + if strings.HasPrefix(trimmed, "//") || strings.HasPrefix(trimmed, "/*") || strings.HasPrefix(trimmed, "*") { + continue + } + + // Check for execSync usage + if execSyncRegex.MatchString(line) { + foundUsages = append(foundUsages, fmt.Sprintf("line %d: %s", lineNum+1, strings.TrimSpace(line))) + } + } + + if len(foundUsages) > 0 { + bundlerValidationLog.Printf("Validation failed: found %d execSync usage(s) in %s", len(foundUsages), scriptName) + return fmt.Errorf("GitHub Script mode script '%s' contains %d execSync usage(s):\n %s\n\nGitHub Script mode should use exec instead of execSync for better async/await handling", + scriptName, len(foundUsages), strings.Join(foundUsages, "\n ")) + } + + bundlerValidationLog.Printf("Validation successful: no execSync usage found in %s", scriptName) + return nil +} + +// validateNoGitHubScriptGlobals checks that Node.js mode scripts do not use GitHub Actions globals +// Node.js scripts should not rely on actions/github-script globals like core.*, exec.*, or github.* +// Returns an error if GitHub Actions globals are found, otherwise returns nil +func validateNoGitHubScriptGlobals(scriptName string, content string, mode RuntimeMode) error { + // Only validate Node.js mode + if mode != RuntimeModeNodeJS { + return nil + } + + bundlerValidationLog.Printf("Validating no GitHub Actions globals in Node.js script: %s (%d bytes)", scriptName, len(content)) + + // Regular expressions to match GitHub Actions globals + // Matches: core.method, exec.method, github.property + coreGlobalRegex := regexp.MustCompile(`\bcore\.\w+`) + execGlobalRegex := regexp.MustCompile(`\bexec\.\w+`) + githubGlobalRegex := regexp.MustCompile(`\bgithub\.\w+`) + + lines := strings.Split(content, "\n") + var foundUsages []string + + for lineNum, line := range lines { + trimmed := strings.TrimSpace(line) + + // Skip comment lines and type references + if strings.HasPrefix(trimmed, "//") || strings.HasPrefix(trimmed, "/*") || strings.HasPrefix(trimmed, "*") { + continue + } + if strings.Contains(trimmed, "/// 0 { + bundlerValidationLog.Printf("Validation failed: found %d GitHub Actions global usage(s) in %s", len(foundUsages), scriptName) + return fmt.Errorf("Node.js mode script '%s' contains %d GitHub Actions global usage(s):\n %s\n\nNode.js scripts should not use GitHub Actions globals (core.*, exec.*, github.*)", + scriptName, len(foundUsages), strings.Join(foundUsages, "\n ")) + } + + bundlerValidationLog.Printf("Validation successful: no GitHub Actions globals found in %s", scriptName) + return nil +} + // normalizePath normalizes a file path by resolving . and .. components func normalizePath(path string) string { // Split path into parts diff --git a/pkg/workflow/bundler_validation_script_registry_test.go b/pkg/workflow/bundler_validation_script_registry_test.go new file mode 100644 index 00000000000..41f12445ba4 --- /dev/null +++ b/pkg/workflow/bundler_validation_script_registry_test.go @@ -0,0 +1,243 @@ +package workflow + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestValidateNoExecSync_GitHubScriptMode(t *testing.T) { + tests := []struct { + name string + scriptName string + content string + mode RuntimeMode + expectError bool + }{ + { + name: "GitHub Script mode with execSync should fail", + scriptName: "test_script", + content: ` +const { execSync } = require("child_process"); +const result = execSync("ls -la"); +`, + mode: RuntimeModeGitHubScript, + expectError: true, + }, + { + name: "GitHub Script mode with exec should pass", + scriptName: "test_script", + content: ` +const { exec } = require("@actions/exec"); +await exec.exec("ls -la"); +`, + mode: RuntimeModeGitHubScript, + expectError: false, + }, + { + name: "GitHub Script mode without exec should pass", + scriptName: "test_script", + content: ` +const fs = require("fs"); +const data = fs.readFileSync("file.txt"); +`, + mode: RuntimeModeGitHubScript, + expectError: false, + }, + { + name: "Node.js mode with execSync should pass (not checked)", + scriptName: "test_script", + content: ` +const { execSync } = require("child_process"); +const result = execSync("ls -la"); +`, + mode: RuntimeModeNodeJS, + expectError: false, + }, + { + name: "GitHub Script mode with execSync in comment should pass", + scriptName: "test_script", + content: ` +// Don't use execSync, use exec instead +const { exec } = require("@actions/exec"); +`, + mode: RuntimeModeGitHubScript, + expectError: false, + }, + { + name: "GitHub Script mode with multiple execSync calls should fail", + scriptName: "test_script", + content: ` +const { execSync } = require("child_process"); +execSync("git status"); +const output = execSync("git diff"); +`, + mode: RuntimeModeGitHubScript, + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateNoExecSync(tt.scriptName, tt.content, tt.mode) + if tt.expectError { + assert.Error(t, err, "Expected validation to fail") + assert.Contains(t, err.Error(), "execSync", "Error should mention execSync") + } else { + assert.NoError(t, err, "Expected validation to pass") + } + }) + } +} + +func TestValidateNoGitHubScriptGlobals_NodeJSMode(t *testing.T) { + tests := []struct { + name string + scriptName string + content string + mode RuntimeMode + expectError bool + }{ + { + name: "Node.js mode with core.* should fail", + scriptName: "test_script", + content: ` +const fs = require("fs"); +core.info("This is a message"); +`, + mode: RuntimeModeNodeJS, + expectError: true, + }, + { + name: "Node.js mode with exec.* should fail", + scriptName: "test_script", + content: ` +const fs = require("fs"); +await exec.exec("ls -la"); +`, + mode: RuntimeModeNodeJS, + expectError: true, + }, + { + name: "Node.js mode with github.* should fail", + scriptName: "test_script", + content: ` +const fs = require("fs"); +const repo = github.context.repo; +`, + mode: RuntimeModeNodeJS, + expectError: true, + }, + { + name: "Node.js mode without GitHub Actions globals should pass", + scriptName: "test_script", + content: ` +const fs = require("fs"); +const data = fs.readFileSync("file.txt"); +console.log("Processing data"); +`, + mode: RuntimeModeNodeJS, + expectError: false, + }, + { + name: "GitHub Script mode with core.* should pass (not checked)", + scriptName: "test_script", + content: ` +core.info("This is a message"); +core.setOutput("result", "value"); +`, + mode: RuntimeModeGitHubScript, + expectError: false, + }, + { + name: "Node.js mode with GitHub Actions globals in comment should pass", + scriptName: "test_script", + content: ` +// Don't use core.info in Node.js scripts +console.log("Use console.log instead"); +`, + mode: RuntimeModeNodeJS, + expectError: false, + }, + { + name: "Node.js mode with type reference should pass", + scriptName: "test_script", + content: ` +/// +const fs = require("fs"); +`, + mode: RuntimeModeNodeJS, + expectError: false, + }, + { + name: "Node.js mode with multiple GitHub Actions globals should fail", + scriptName: "test_script", + content: ` +const fs = require("fs"); +core.info("Message"); +exec.exec("ls"); +const repo = github.context.repo; +`, + mode: RuntimeModeNodeJS, + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateNoGitHubScriptGlobals(tt.scriptName, tt.content, tt.mode) + if tt.expectError { + assert.Error(t, err, "Expected validation to fail") + } else { + assert.NoError(t, err, "Expected validation to pass") + } + }) + } +} + +func TestScriptRegistry_RegisterWithMode_Validation(t *testing.T) { + t.Run("GitHub Script mode with execSync should panic", func(t *testing.T) { + registry := NewScriptRegistry() + invalidScript := ` +const { execSync } = require("child_process"); +execSync("ls -la"); +` + assert.Panics(t, func() { + registry.RegisterWithMode("invalid_script", invalidScript, RuntimeModeGitHubScript) + }, "Should panic when registering GitHub Script with execSync") + }) + + t.Run("Node.js mode with GitHub Actions globals should panic", func(t *testing.T) { + registry := NewScriptRegistry() + invalidScript := ` +const fs = require("fs"); +core.info("This should not be here"); +` + assert.Panics(t, func() { + registry.RegisterWithMode("invalid_script", invalidScript, RuntimeModeNodeJS) + }, "Should panic when registering Node.js script with GitHub Actions globals") + }) + + t.Run("Valid GitHub Script mode should not panic", func(t *testing.T) { + registry := NewScriptRegistry() + validScript := ` +const { exec } = require("@actions/exec"); +core.info("This is valid for GitHub Script mode"); +` + assert.NotPanics(t, func() { + registry.RegisterWithMode("valid_script", validScript, RuntimeModeGitHubScript) + }, "Should not panic with valid GitHub Script") + }) + + t.Run("Valid Node.js mode should not panic", func(t *testing.T) { + registry := NewScriptRegistry() + validScript := ` +const fs = require("fs"); +const { execSync } = require("child_process"); +console.log("This is valid for Node.js mode"); +` + assert.NotPanics(t, func() { + registry.RegisterWithMode("valid_script", validScript, RuntimeModeNodeJS) + }, "Should not panic with valid Node.js script") + }) +} diff --git a/pkg/workflow/script_registry.go b/pkg/workflow/script_registry.go index df0af06afcb..f6ded9369b5 100644 --- a/pkg/workflow/script_registry.go +++ b/pkg/workflow/script_registry.go @@ -53,6 +53,7 @@ package workflow import ( + "fmt" "sync" "github.com/githubnext/gh-aw/pkg/logger" @@ -107,6 +108,7 @@ func (r *ScriptRegistry) Register(name string, source string) { // RegisterWithMode adds a script source to the registry with a specific runtime mode. // The script will be bundled lazily on first access via Get(). +// Performs compile-time validation to ensure the script follows runtime mode conventions. // // Parameters: // - name: Unique identifier for the script (e.g., "create_issue", "add_comment") @@ -115,6 +117,13 @@ func (r *ScriptRegistry) Register(name string, source string) { // // If a script with the same name already exists, it will be overwritten. // This is useful for testing but should be avoided in production. +// +// Compile-time validations: +// - GitHub Script mode: validates no execSync usage (should use exec instead) +// - Node.js mode: validates no GitHub Actions globals (core.*, exec.*, github.*) +// +// Panics if validation fails, as this indicates a programming error that should be +// caught during development and testing. func (r *ScriptRegistry) RegisterWithMode(name string, source string, mode RuntimeMode) { r.mu.Lock() defer r.mu.Unlock() @@ -123,6 +132,17 @@ func (r *ScriptRegistry) RegisterWithMode(name string, source string, mode Runti registryLog.Printf("Registering script: %s (%d bytes, mode: %s)", name, len(source), mode) } + // Perform compile-time validation based on runtime mode + if err := validateNoExecSync(name, source, mode); err != nil { + // This is a programming error that should be caught during development + panic(fmt.Sprintf("Script registration validation failed: %v", err)) + } + + if err := validateNoGitHubScriptGlobals(name, source, mode); err != nil { + // This is a programming error that should be caught during development + panic(fmt.Sprintf("Script registration validation failed: %v", err)) + } + r.scripts[name] = &scriptEntry{ source: source, mode: mode, diff --git a/specs/validation-architecture.md b/specs/validation-architecture.md index 3afadc98f05..e6b248b755f 100644 --- a/specs/validation-architecture.md +++ b/specs/validation-architecture.md @@ -183,6 +183,44 @@ Domain-specific validation is organized into separate files based on functional - ✅ Include/import validation - ✅ Template region validation +#### 9. **JavaScript Bundler Validation**: `bundler_validation.go` + +**Location**: `pkg/workflow/bundler_validation.go` (360 lines) + +**Purpose**: Validates JavaScript code for runtime mode compatibility and bundling correctness + +**Validation Functions**: +- `validateNoLocalRequires()` - Ensures all local require() statements are bundled (GitHub Script mode) +- `validateNoModuleReferences()` - Ensures no module.exports or exports remain (GitHub Script mode) +- `validateNoExecSync()` - Ensures GitHub Script mode scripts use exec instead of execSync +- `validateNoGitHubScriptGlobals()` - Ensures Node.js scripts don't use GitHub Actions globals (core.*, exec.*, github.*) +- `ValidateEmbeddedResourceRequires()` - Validates embedded JavaScript dependencies exist + +**Pattern**: Runtime mode-specific validation with compile-time checks + +**Validation Enforcement**: +- Compile-time: Triggered during script registration in `RegisterWithMode()` +- Build-time: Scripts violating rules cause panics during package initialization +- Runtime: Bundled scripts are validated before execution + +**When to add validation here**: +- ✅ JavaScript runtime mode compatibility checks +- ✅ GitHub Script vs Node.js script validation +- ✅ Module system validation (require, exports) +- ✅ GitHub Actions API usage validation +- ✅ Child process execution validation (exec vs execSync) + +**Design Rationale**: +The bundler validation enforces two key constraints: +1. **GitHub Script mode**: Should not use `execSync` (use async `exec` from `@actions/exec` instead) +2. **Node.js mode**: Should not use GitHub Actions globals (`core.*`, `exec.*`, `github.*`) + +These rules ensure that scripts follow platform conventions: +- GitHub Script mode runs inline in GitHub Actions YAML with GitHub-specific globals available +- Node.js mode runs as standalone scripts with standard Node.js APIs only + +Validation happens at registration time (via panic) to catch errors during development/testing rather than at runtime. + ## Decision Tree: Where to Add New Validation Use this decision tree to determine where to place new validation logic: