diff --git a/.github/workflows/smoke-project.lock.yml b/.github/workflows/smoke-project.lock.yml index dd2c2acb155..3dcba967766 100644 --- a/.github/workflows/smoke-project.lock.yml +++ b/.github/workflows/smoke-project.lock.yml @@ -2195,7 +2195,7 @@ jobs: GH_AW_PROJECT_URL: "https://github.com/orgs/github/projects/24068" GH_AW_PROJECT_GITHUB_TOKEN: ${{ secrets.GH_AW_PROJECT_GITHUB_TOKEN }} with: - github-token: ${{ secrets.GH_AW_PROJECT_GITHUB_TOKEN }} + github-token: ${{ secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }} script: | const { setupGlobals } = require('${{ runner.temp }}/gh-aw/actions/setup_globals.cjs'); setupGlobals(core, github, context, exec, io, getOctokit); diff --git a/actions/setup/js/handler_auth.cjs b/actions/setup/js/handler_auth.cjs index a6cfacdfa3b..f5135dc3b4e 100644 --- a/actions/setup/js/handler_auth.cjs +++ b/actions/setup/js/handler_auth.cjs @@ -14,7 +14,7 @@ * 2. global github — step-level token set in the github-script with.github-token * * The step-level token itself follows (as set by the Go compiler): - * project token > global safe-outputs.github-token > magic secrets + * global safe-outputs.github-token > magic secrets */ /** diff --git a/actions/setup/js/safe_output_handler_manager.cjs b/actions/setup/js/safe_output_handler_manager.cjs index 2520014ae46..538cac42408 100644 --- a/actions/setup/js/safe_output_handler_manager.cjs +++ b/actions/setup/js/safe_output_handler_manager.cjs @@ -29,6 +29,7 @@ const { checkRateLimitHeadroom } = require("./rate_limit_helpers.cjs"); const { redactSensitiveConfig } = require("./safe_outputs_config_redact.cjs"); const nodePath = require("path"); const fs = require("fs"); +const GITHUB_TOKEN_CONFIG_KEY = "github-token"; /** * Handler map configuration @@ -101,6 +102,9 @@ const STANDALONE_STEP_TYPES = new Set(["upload_asset", "noop"]); */ const CODE_PUSH_TYPES = new Set(["push_to_pull_request_branch", "create_pull_request"]); +/** @type {Set} Project-safe-output handlers that should default to GH_AW_PROJECT_GITHUB_TOKEN when no per-handler github-token is configured. */ +const PROJECT_HANDLER_TYPES = new Set(["create_project", "create_project_status_update", "update_project"]); + // Threat-detection warn-mode requirement IDs from safe-outputs specification: // - WTD2: Convertible outputs must be mapped to a reviewable type. // - WTD3: Non-reviewable outputs must be aborted. @@ -283,6 +287,37 @@ function loadConfig() { /** @type {Set} Handler types that participate in the PR review buffer */ const PR_REVIEW_HANDLER_TYPES = new Set(["create_pull_request_review_comment", "submit_pull_request_review"]); +/** + * Wrap a handler so project-safe-output execution can temporarily bind global.github + * to the handler-specific authenticated client. + * + * @param {string} type - Safe-output handler type + * @param {Function} messageHandler - Loaded handler function + * @param {Object|null} handlerGithubClient - Optional per-handler GitHub client + * @returns {Function} Wrapped handler function + */ +function wrapWithClientRebinding(type, messageHandler, handlerGithubClient) { + if (!PROJECT_HANDLER_TYPES.has(type) || !handlerGithubClient) { + return messageHandler; + } + return async (...args) => { + /** @type {any} */ + const globalState = global; + const hadGithub = Object.prototype.hasOwnProperty.call(globalState, "github"); + const previousGithub = globalState.github; + globalState.github = handlerGithubClient; + try { + return await messageHandler(...args); + } finally { + if (hadGithub) { + globalState.github = previousGithub; + } else { + delete globalState.github; + } + } + }; +} + /** * Load and initialize handlers for enabled safe output types * Calls each handler's factory function (main) to get message processors @@ -306,6 +341,10 @@ async function loadHandlers(config, prReviewBufferRegistry, resolvedAllowedMenti // Call the factory function with config to get the message handler const handlerConfig = { ...(config[type] || {}) }; + if (PROJECT_HANDLER_TYPES.has(type) && !handlerConfig[GITHUB_TOKEN_CONFIG_KEY] && process.env.GH_AW_PROJECT_GITHUB_TOKEN) { + handlerConfig[GITHUB_TOKEN_CONFIG_KEY] = process.env.GH_AW_PROJECT_GITHUB_TOKEN; + } + // Pass top-level mentions policy through so handlers can preserve // the same allowed mention aliases used during collection. if (handlerConfig.mentions == null && config.mentions != null) { @@ -320,7 +359,14 @@ async function loadHandlers(config, prReviewBufferRegistry, resolvedAllowedMenti handlerConfig._prReviewBufferRegistry = prReviewBufferRegistry; } - const messageHandler = await handlerModule.main(handlerConfig); + /** @type {any|null} */ + let handlerGithubClient = null; + /** @type {any} */ + const globalState = global; + if (handlerConfig[GITHUB_TOKEN_CONFIG_KEY] && typeof globalState.getOctokit === "function") { + handlerGithubClient = globalState.getOctokit(handlerConfig[GITHUB_TOKEN_CONFIG_KEY]); + } + const messageHandler = await handlerModule.main(handlerConfig, handlerGithubClient); if (typeof messageHandler !== "function") { // This is a fatal error - the handler is misconfigured @@ -330,7 +376,7 @@ async function loadHandlers(config, prReviewBufferRegistry, resolvedAllowedMenti throw error; } - messageHandlers.set(type, messageHandler); + messageHandlers.set(type, wrapWithClientRebinding(type, messageHandler, handlerGithubClient)); core.info(`✓ Loaded and initialized handler for: ${type}`); } else { core.warning(`Handler module ${type} does not export a main function`); diff --git a/actions/setup/js/safe_output_handler_manager.test.cjs b/actions/setup/js/safe_output_handler_manager.test.cjs index a5c5260fe45..8fd1750adf9 100644 --- a/actions/setup/js/safe_output_handler_manager.test.cjs +++ b/actions/setup/js/safe_output_handler_manager.test.cjs @@ -277,20 +277,141 @@ describe("Safe Output Handler Manager", () => { max: 1, mentions: mentionsConfig, allowedMentionAliases: ["copilot", "octocat"], - }) + }), + null ); expect(createIssueMainSpy).toHaveBeenCalledWith( expect.objectContaining({ max: 1, mentions: mentionsConfig, allowedMentionAliases: ["copilot", "octocat"], - }) + }), + null ); } finally { addCommentMainSpy.mockRestore(); createIssueMainSpy.mockRestore(); } }); + + it("injects GH_AW_PROJECT_GITHUB_TOKEN for project handlers when github-token is missing", async () => { + process.env.GH_AW_PROJECT_GITHUB_TOKEN = "projects-token"; + global.getOctokit = vi.fn().mockReturnValue({ client: "project-client" }); + + const updateProjectModule = require("./update_project.cjs"); + const updateProjectMainSpy = vi.spyOn(updateProjectModule, "main").mockImplementation(async () => async () => ({ success: true })); + + try { + const handlers = await loadHandlers({ + update_project: { project: "https://github.com/orgs/myorg/projects/1" }, + }); + + expect(handlers.has("update_project")).toBe(true); + expect(global.getOctokit).toHaveBeenCalledWith("projects-token"); + expect(updateProjectMainSpy).toHaveBeenCalledWith( + expect.objectContaining({ + project: "https://github.com/orgs/myorg/projects/1", + "github-token": "projects-token", + }), + expect.objectContaining({ client: "project-client" }) + ); + } finally { + updateProjectMainSpy.mockRestore(); + delete process.env.GH_AW_PROJECT_GITHUB_TOKEN; + delete global.getOctokit; + } + }); + + it("preserves explicit project handler github-token over GH_AW_PROJECT_GITHUB_TOKEN fallback", async () => { + process.env.GH_AW_PROJECT_GITHUB_TOKEN = "fallback-project-token"; + global.getOctokit = vi.fn().mockReturnValue({ client: "project-client" }); + + const updateProjectModule = require("./update_project.cjs"); + const updateProjectMainSpy = vi.spyOn(updateProjectModule, "main").mockImplementation(async () => async () => ({ success: true })); + + try { + await loadHandlers({ + update_project: { + project: "https://github.com/orgs/myorg/projects/1", + "github-token": "explicit-project-token", + }, + }); + + expect(global.getOctokit).toHaveBeenCalledWith("explicit-project-token"); + expect(updateProjectMainSpy).toHaveBeenCalledWith( + expect.objectContaining({ + "github-token": "explicit-project-token", + }), + expect.objectContaining({ client: "project-client" }) + ); + } finally { + updateProjectMainSpy.mockRestore(); + delete process.env.GH_AW_PROJECT_GITHUB_TOKEN; + delete global.getOctokit; + } + }); + + it("wraps project handlers so global.github is set to the project client during execution", async () => { + process.env.GH_AW_PROJECT_GITHUB_TOKEN = "projects-token"; + const projectClient = { client: "project-client" }; + global.getOctokit = vi.fn().mockReturnValue(projectClient); + + const previousGithub = { client: "shared-client" }; + global.github = previousGithub; + let seenGithub = null; + + const updateProjectModule = require("./update_project.cjs"); + const updateProjectMainSpy = vi.spyOn(updateProjectModule, "main").mockImplementation(async () => async () => { + // outer function: handler factory at loadHandlers() time; inner function: message handler at processMessages() time + seenGithub = global.github; + return { success: true }; + }); + + try { + const handlers = await loadHandlers({ + update_project: { project: "https://github.com/orgs/myorg/projects/1" }, + }); + const handler = handlers.get("update_project"); + expect(typeof handler).toBe("function"); + + await handler({ type: "update_project" }); + + expect(seenGithub).toBe(projectClient); + expect(global.github).toBe(previousGithub); + } finally { + updateProjectMainSpy.mockRestore(); + delete process.env.GH_AW_PROJECT_GITHUB_TOKEN; + delete global.getOctokit; + global.github = previousGithub; + } + }); + + it("restores global.github to absent state after wrapped project handler execution", async () => { + process.env.GH_AW_PROJECT_GITHUB_TOKEN = "projects-token"; + const projectClient = { client: "project-client" }; + global.getOctokit = vi.fn().mockReturnValue(projectClient); + delete global.github; + + const updateProjectModule = require("./update_project.cjs"); + const updateProjectMainSpy = vi.spyOn(updateProjectModule, "main").mockImplementation(async () => async () => ({ success: true })); + + try { + const handlers = await loadHandlers({ + update_project: { project: "https://github.com/orgs/myorg/projects/1" }, + }); + const handler = handlers.get("update_project"); + expect(typeof handler).toBe("function"); + + await handler({ type: "update_project" }); + + expect("github" in global).toBe(false); + } finally { + updateProjectMainSpy.mockRestore(); + delete process.env.GH_AW_PROJECT_GITHUB_TOKEN; + delete global.getOctokit; + delete global.github; + } + }); }); describe("loadHandlers - path traversal sanitization", () => { diff --git a/pkg/workflow/compiler_safe_outputs_steps.go b/pkg/workflow/compiler_safe_outputs_steps.go index f714c56d3a4..d90e2ca3bd2 100644 --- a/pkg/workflow/compiler_safe_outputs_steps.go +++ b/pkg/workflow/compiler_safe_outputs_steps.go @@ -272,24 +272,18 @@ func (c *Compiler) buildHandlerManagerStep(data *WorkflowData) ([]string, error) } // With section for github-token - // Use the standard safe outputs token for all operations. - // If project operations are configured, prefer the project token for the github-script client. - // Rationale: update_project/create_project_status_update call the Projects v2 GraphQL API, which - // cannot be accessed with the default GITHUB_TOKEN. GH_AW_PROJECT_GITHUB_TOKEN is the required - // token for Projects v2 operations. + // Use the standard safe-outputs token for the shared github-script client. + // Project operations use GH_AW_PROJECT_GITHUB_TOKEN from env with dedicated handler logic. steps = append(steps, " with:\n") // Token precedence for the handler manager step: - // 1. Project token (if project operations are configured) - already set above - // 2. Safe-outputs level token (so.GitHubToken) - // 3. Magic secret fallback via getEffectiveSafeOutputGitHubToken() + // 1. Safe-outputs level token (so.GitHubToken) + // 2. Magic secret fallback via getEffectiveSafeOutputGitHubToken() // // Note: We do NOT fall back to per-output tokens (add-comment, create-issue, etc.) // because those are specific to their operations. The handler manager needs a // general-purpose token for the github-script client. configToken := "" - if projectToken != "" { - configToken = projectToken - } else if data.SafeOutputs != nil && data.SafeOutputs.GitHubToken != "" { + if data.SafeOutputs != nil && data.SafeOutputs.GitHubToken != "" { configToken = data.SafeOutputs.GitHubToken } c.addSafeOutputGitHubTokenForConfig(&steps, data, configToken) diff --git a/pkg/workflow/safe_outputs_handler_manager_token_test.go b/pkg/workflow/safe_outputs_handler_manager_token_test.go index e6a0bc82fe7..4e1b6205a2d 100644 --- a/pkg/workflow/safe_outputs_handler_manager_token_test.go +++ b/pkg/workflow/safe_outputs_handler_manager_token_test.go @@ -3,6 +3,9 @@ package workflow import ( + "encoding/json" + "reflect" + "sort" "strings" "testing" @@ -233,7 +236,23 @@ func TestHandlerManagerProjectGitHubTokenEnvVar(t *testing.T) { }, }, expectedEnvVarValue: "GH_AW_PROJECT_GITHUB_TOKEN: ${{ secrets.PROJECTS_PAT }}", - expectedWithToken: "github-token: ${{ secrets.PROJECTS_PAT }}", + expectedWithToken: "github-token: ${{ secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }}", + shouldHaveToken: true, + }, + { + name: "update-project with custom github-token and safe-outputs github-token", + frontmatter: map[string]any{ + "name": "Test Workflow", + "safe-outputs": map[string]any{ + "github-token": "${{ secrets.SAFE_OUTPUTS_TOKEN }}", + "update-project": map[string]any{ + "github-token": "${{ secrets.PROJECTS_PAT }}", + "project": "https://github.com/orgs/myorg/projects/1", + }, + }, + }, + expectedEnvVarValue: "GH_AW_PROJECT_GITHUB_TOKEN: ${{ secrets.PROJECTS_PAT }}", + expectedWithToken: "github-token: ${{ secrets.SAFE_OUTPUTS_TOKEN }}", shouldHaveToken: true, }, { @@ -247,7 +266,7 @@ func TestHandlerManagerProjectGitHubTokenEnvVar(t *testing.T) { }, }, expectedEnvVarValue: "GH_AW_PROJECT_GITHUB_TOKEN: ${{ secrets.GH_AW_PROJECT_GITHUB_TOKEN }}", - expectedWithToken: "github-token: ${{ secrets.GH_AW_PROJECT_GITHUB_TOKEN }}", + expectedWithToken: "github-token: ${{ secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }}", shouldHaveToken: true, }, { @@ -277,7 +296,7 @@ func TestHandlerManagerProjectGitHubTokenEnvVar(t *testing.T) { }, }, expectedEnvVarValue: "GH_AW_PROJECT_GITHUB_TOKEN: ${{ secrets.STATUS_PAT }}", - expectedWithToken: "github-token: ${{ secrets.STATUS_PAT }}", + expectedWithToken: "github-token: ${{ secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }}", shouldHaveToken: true, }, { @@ -291,7 +310,7 @@ func TestHandlerManagerProjectGitHubTokenEnvVar(t *testing.T) { }, }, expectedEnvVarValue: "GH_AW_PROJECT_GITHUB_TOKEN: ${{ secrets.CREATE_PAT }}", - expectedWithToken: "github-token: ${{ secrets.CREATE_PAT }}", + expectedWithToken: "github-token: ${{ secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }}", shouldHaveToken: true, }, { @@ -313,7 +332,7 @@ func TestHandlerManagerProjectGitHubTokenEnvVar(t *testing.T) { }, }, expectedEnvVarValue: "GH_AW_PROJECT_GITHUB_TOKEN: ${{ secrets.UPDATE_PAT }}", - expectedWithToken: "github-token: ${{ secrets.UPDATE_PAT }}", + expectedWithToken: "github-token: ${{ secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }}", shouldHaveToken: true, }, { @@ -351,7 +370,7 @@ func TestHandlerManagerProjectGitHubTokenEnvVar(t *testing.T) { "Expected environment variable %q to be set in handler manager step", tt.expectedEnvVarValue) - // Check that the github-script token matches the effective project token + // Check that the github-script token uses safe-outputs token precedence. assert.Contains(t, yamlStr, tt.expectedWithToken, "Expected github-script token %q to be set in handler manager step", tt.expectedWithToken) @@ -363,3 +382,148 @@ func TestHandlerManagerProjectGitHubTokenEnvVar(t *testing.T) { }) } } + +// TestHandlerManagerGitHubTokenIsolationAcrossSafeOutputHandlers verifies that the shared +// github-script client token remains sourced from safe-outputs.github-token regardless of which +// safe-output handler is configured alongside project-specific token settings. +func TestHandlerManagerGitHubTokenIsolationAcrossSafeOutputHandlers(t *testing.T) { + const safeOutputsToken = "${{ secrets.SAFE_OUTPUTS_TOKEN }}" + const handlerToken = "${{ secrets.HANDLER_TOKEN }}" + + projectHandlers := map[string]bool{ + "update_project": true, + "create_project_status_update": true, + "create_project": true, + } + + handlerNames := make([]string, 0, len(handlerRegistry)) + for handlerName := range handlerRegistry { + handlerNames = append(handlerNames, handlerName) + } + sort.Strings(handlerNames) + + for _, handlerName := range handlerNames { + t.Run(handlerName, func(t *testing.T) { + safeOutputs := &SafeOutputsConfig{ + GitHubToken: safeOutputsToken, + } + enableHandlerForIsolationTest(t, safeOutputs, handlerName, handlerToken) + + compiler := NewCompiler() + workflowData := &WorkflowData{ + Name: "test-workflow", + SafeOutputs: safeOutputs, + } + + steps, err := compiler.buildHandlerManagerStep(workflowData) + require.NoError(t, err) + + yamlStr := strings.Join(steps, "") + handlerConfig := extractHandlerManagerConfigFromYAML(t, yamlStr) + _, ok := handlerConfig[handlerName] + assert.True(t, ok, "expected handler %q to be present in GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG", handlerName) + + assert.Contains(t, yamlStr, "github-token: "+safeOutputsToken) + assert.NotContains(t, yamlStr, "github-token: "+handlerToken) + if projectHandlers[handlerName] { + assert.Contains(t, yamlStr, "GH_AW_PROJECT_GITHUB_TOKEN: "+handlerToken) + } else { + assert.NotContains(t, yamlStr, "GH_AW_PROJECT_GITHUB_TOKEN:") + } + }) + } +} + +func enableHandlerForIsolationTest(t *testing.T, safeOutputs *SafeOutputsConfig, handlerName, token string) { + t.Helper() + + switch handlerName { + case "dispatch_repository": + safeOutputs.DispatchRepository = &DispatchRepositoryConfig{ + Tools: map[string]*DispatchRepositoryToolConfig{ + "default": { + Workflow: "safe-outputs-dispatch", + EventType: "safe-outputs-dispatch", + Repository: "github/gh-aw", + Max: strPtr("1"), + GitHubToken: token, + }, + }, + } + return + case "create_report_incomplete_issue": + safeOutputs.ReportIncomplete = &ReportIncompleteConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{ + Max: strPtr("1"), + GitHubToken: token, + }, + } + return + } + + configValue := reflect.ValueOf(safeOutputs).Elem() + configType := configValue.Type() + + //nolint:intrange // explicit bounds loop keeps compatibility with reviewers/tools that don't accept range-over-int + for i := 0; i < configType.NumField(); i++ { + field := configType.Field(i) + yamlTag := strings.Split(field.Tag.Get("yaml"), ",")[0] + if yamlTag == "" || yamlTag == "-" { + continue + } + if strings.ReplaceAll(yamlTag, "-", "_") != handlerName { + continue + } + + require.Equal(t, reflect.Pointer, field.Type.Kind(), "handler field %s must be a pointer type", field.Name) + handlerValue := reflect.New(field.Type.Elem()) + initializeHandlerConfigForIsolationTest(handlerValue.Elem(), token) + configValue.Field(i).Set(handlerValue) + return + } + + require.Failf(t, "missing handler field", "no SafeOutputsConfig field found for handler %q", handlerName) +} + +func initializeHandlerConfigForIsolationTest(handlerStruct reflect.Value, token string) { + baseField := handlerStruct.FieldByName("BaseSafeOutputConfig") + if baseField.IsValid() { + if maxField := baseField.FieldByName("Max"); maxField.IsValid() && maxField.CanSet() { + maxField.Set(reflect.ValueOf(strPtr("1"))) + } + if tokenField := baseField.FieldByName("GitHubToken"); tokenField.IsValid() && tokenField.CanSet() { + tokenField.SetString(token) + } + } + if tokenField := handlerStruct.FieldByName("GitHubToken"); tokenField.IsValid() && tokenField.CanSet() && tokenField.Kind() == reflect.String { + tokenField.SetString(token) + } + + // project URL is required for update-project and create-project-status-update + if projectField := handlerStruct.FieldByName("Project"); projectField.IsValid() && projectField.CanSet() && projectField.Kind() == reflect.String { + projectField.SetString("https://github.com/orgs/myorg/projects/1") + } +} + +func extractHandlerManagerConfigFromYAML(t *testing.T, yamlStr string) map[string]map[string]any { + t.Helper() + + const prefix = "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG: " + for line := range strings.SplitSeq(yamlStr, "\n") { + trimmed := strings.TrimSpace(line) + if !strings.HasPrefix(trimmed, prefix) { + continue + } + + configJSON := strings.TrimPrefix(trimmed, prefix) + configJSON = strings.Trim(configJSON, "\"") + configJSON = strings.ReplaceAll(configJSON, "\\\"", "\"") + + var config map[string]map[string]any + require.NoError(t, json.Unmarshal([]byte(configJSON), &config), "handler config JSON should be valid") + return config + } + + require.Fail(t, "missing GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG in compiled YAML") + return nil +}