diff --git a/actions/setup/js/allowed_extensions_helpers.cjs b/actions/setup/js/allowed_extensions_helpers.cjs new file mode 100644 index 00000000000..c721860fce8 --- /dev/null +++ b/actions/setup/js/allowed_extensions_helpers.cjs @@ -0,0 +1,49 @@ +// @ts-check + +/** + * @param {string} value + * @returns {boolean} + */ +function isGitHubExpression(value) { + const trimmed = value.trim(); + return /^\$\{\{[\s\S]*\}\}$/.test(trimmed); +} + +/** + * @param {string} extValue + * @returns {string} + */ +function normalizeAllowedExtension(extValue) { + const trimmed = extValue.trim(); + if (!trimmed) { + return ""; + } + if (isGitHubExpression(trimmed)) { + return trimmed; + } + const normalized = trimmed.toLowerCase(); + return normalized.startsWith(".") ? normalized : `.${normalized}`; +} + +/** + * @param {string | undefined} envValue + * @returns {{rawValues: string[], normalizedValues: string[], hasUnresolvedExpression: boolean} | null} + */ +function parseAllowedExtensionsEnv(envValue) { + if (!envValue) { + return null; + } + + const rawValues = envValue.split(",").map(extValue => extValue.trim()); + return { + rawValues, + normalizedValues: rawValues.map(normalizeAllowedExtension).filter(Boolean), + hasUnresolvedExpression: rawValues.some(isGitHubExpression), + }; +} + +module.exports = { + isGitHubExpression, + normalizeAllowedExtension, + parseAllowedExtensionsEnv, +}; diff --git a/actions/setup/js/allowed_extensions_helpers.test.cjs b/actions/setup/js/allowed_extensions_helpers.test.cjs new file mode 100644 index 00000000000..28fb1c016c5 --- /dev/null +++ b/actions/setup/js/allowed_extensions_helpers.test.cjs @@ -0,0 +1,46 @@ +import { describe, expect, it } from "vitest"; +import { isGitHubExpression, normalizeAllowedExtension, parseAllowedExtensionsEnv } from "./allowed_extensions_helpers.cjs"; + +describe("allowed_extensions_helpers", () => { + describe("isGitHubExpression", () => { + it("returns true for full GitHub Actions expression", () => { + expect(isGitHubExpression("${{ inputs.allowed_exts }}")).toBe(true); + }); + + it("returns false for non-expression text", () => { + expect(isGitHubExpression("prefix ${{ inputs.allowed_exts }}")).toBe(false); + }); + }); + + describe("normalizeAllowedExtension", () => { + it("normalizes case, trims spaces, and adds missing dot", () => { + expect(normalizeAllowedExtension(" PNG ")).toBe(".png"); + }); + + it("returns empty string for blank input", () => { + expect(normalizeAllowedExtension(" ")).toBe(""); + }); + }); + + describe("parseAllowedExtensionsEnv", () => { + it("returns null when env value is undefined", () => { + expect(parseAllowedExtensionsEnv(undefined)).toBeNull(); + }); + + it("parses and normalizes literal extension values", () => { + expect(parseAllowedExtensionsEnv("TXT, md")).toEqual({ + rawValues: ["TXT", "md"], + normalizedValues: [".txt", ".md"], + hasUnresolvedExpression: false, + }); + }); + + it("detects unresolved GitHub Actions expressions", () => { + expect(parseAllowedExtensionsEnv(".txt,${{ inputs.allowed_exts }}")).toEqual({ + rawValues: [".txt", "${{ inputs.allowed_exts }}"], + normalizedValues: [".txt", "${{ inputs.allowed_exts }}"], + hasUnresolvedExpression: true, + }); + }); + }); +}); diff --git a/actions/setup/js/safe_outputs_handlers.cjs b/actions/setup/js/safe_outputs_handlers.cjs index 710744b1bd7..032909d62c9 100644 --- a/actions/setup/js/safe_outputs_handlers.cjs +++ b/actions/setup/js/safe_outputs_handlers.cjs @@ -18,6 +18,7 @@ const { ERR_CONFIG, ERR_SYSTEM, ERR_VALIDATION } = require("./error_codes.cjs"); const { findRepoCheckout } = require("./find_repo_checkout.cjs"); const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); const { getOrGenerateTemporaryId } = require("./temporary_id.cjs"); +const { parseAllowedExtensionsEnv } = require("./allowed_extensions_helpers.cjs"); /** * Create handlers for safe output tools @@ -127,15 +128,18 @@ function createHandlers(server, appendSafeOutput, config = {}) { // Check file extension - read from environment variable if available const ext = path.extname(filePath).toLowerCase(); - const allowedExts = process.env.GH_AW_ASSETS_ALLOWED_EXTS - ? process.env.GH_AW_ASSETS_ALLOWED_EXTS.split(",").map(ext => ext.trim()) + const parsedAllowedExts = parseAllowedExtensionsEnv(process.env.GH_AW_ASSETS_ALLOWED_EXTS); + if (parsedAllowedExts?.hasUnresolvedExpression) { + throw new Error(`${ERR_CONFIG}: GH_AW_ASSETS_ALLOWED_EXTS contains unresolved GitHub Actions expression. Ensure expressions resolve before safe outputs validation.`); + } + const allowedExts = parsedAllowedExts + ? parsedAllowedExts.normalizedValues : [ // Default set as specified in problem statement ".png", ".jpg", ".jpeg", ]; - if (!allowedExts.includes(ext)) { throw new Error(`${ERR_VALIDATION}: File extension '${ext}' is not allowed. Allowed extensions: ${allowedExts.join(", ")}`); } diff --git a/actions/setup/js/safe_outputs_handlers.test.cjs b/actions/setup/js/safe_outputs_handlers.test.cjs index a45b7ef1f29..95ab380f7df 100644 --- a/actions/setup/js/safe_outputs_handlers.test.cjs +++ b/actions/setup/js/safe_outputs_handlers.test.cjs @@ -265,6 +265,42 @@ describe("safe_outputs_handlers", () => { expect(result.content[0].type).toBe("text"); }); + it("should normalize custom allowed extensions", () => { + process.env.GH_AW_ASSETS_BRANCH = "test-branch"; + process.env.GH_AW_ASSETS_ALLOWED_EXTS = "TXT, md"; + + const testFile = path.join(testWorkspaceDir, "test.txt"); + fs.writeFileSync(testFile, "test content"); + + const args = { path: testFile }; + const result = handlers.uploadAssetHandler(args); + + expect(mockAppendSafeOutput).toHaveBeenCalled(); + expect(result.content[0].type).toBe("text"); + }); + + it("should reject unresolved GitHub expression in allowed extensions", () => { + process.env.GH_AW_ASSETS_BRANCH = "test-branch"; + process.env.GH_AW_ASSETS_ALLOWED_EXTS = "${{ inputs.allowed_exts }}"; + + const testFile = path.join(testWorkspaceDir, "test.txt"); + fs.writeFileSync(testFile, "test content"); + + const args = { path: testFile }; + expect(() => handlers.uploadAssetHandler(args)).toThrow("contains unresolved GitHub Actions expression"); + }); + + it("should reject unresolved expression even when literal extension also matches", () => { + process.env.GH_AW_ASSETS_BRANCH = "test-branch"; + process.env.GH_AW_ASSETS_ALLOWED_EXTS = ".txt,${{ inputs.allowed_exts }}"; + + const testFile = path.join(testWorkspaceDir, "test.txt"); + fs.writeFileSync(testFile, "test content"); + + const args = { path: testFile }; + expect(() => handlers.uploadAssetHandler(args)).toThrow("contains unresolved GitHub Actions expression"); + }); + it("should reject file exceeding size limit", () => { process.env.GH_AW_ASSETS_BRANCH = "test-branch"; process.env.GH_AW_ASSETS_MAX_SIZE_KB = "1"; // 1 KB limit diff --git a/actions/setup/setup.sh b/actions/setup/setup.sh index 8a28dc977b6..30082b5a01f 100755 --- a/actions/setup/setup.sh +++ b/actions/setup/setup.sh @@ -280,6 +280,7 @@ SAFE_OUTPUTS_FILES=( "safe_outputs_tools_loader.cjs" "safe_outputs_config.cjs" "safe_outputs_handlers.cjs" + "allowed_extensions_helpers.cjs" "safe_outputs_append.cjs" "mcp_server_core.cjs" "mcp_logger.cjs" diff --git a/pkg/workflow/publish_assets.go b/pkg/workflow/publish_assets.go index e35d3b808c6..26c6f9fd421 100644 --- a/pkg/workflow/publish_assets.go +++ b/pkg/workflow/publish_assets.go @@ -3,6 +3,7 @@ package workflow import ( "errors" "fmt" + "regexp" "strings" "github.com/github/gh-aw/pkg/constants" @@ -11,6 +12,27 @@ import ( ) var publishAssetsLog = logger.New("workflow:publish_assets") +var githubExpressionPattern = regexp.MustCompile(`(?s)^\$\{\{.*\}\}$`) + +func isGitHubExpression(value string) bool { + trimmed := strings.TrimSpace(value) + return githubExpressionPattern.MatchString(trimmed) +} + +func normalizeAllowedExtension(extension string) string { + trimmed := strings.TrimSpace(extension) + if trimmed == "" { + return "" + } + if isGitHubExpression(trimmed) { + return trimmed + } + normalized := strings.ToLower(trimmed) + if !strings.HasPrefix(normalized, ".") { + normalized = "." + normalized + } + return normalized +} // UploadAssetsConfig holds configuration for publishing assets to an orphaned git branch type UploadAssetsConfig struct { @@ -59,9 +81,18 @@ func (c *Compiler) parseUploadAssetConfig(outputMap map[string]any) *UploadAsset if allowedExts, exists := configMap["allowed-exts"]; exists { if allowedExtsArray, ok := allowedExts.([]any); ok { var extStrings []string + seen := make(map[string]struct{}) for _, ext := range allowedExtsArray { if extStr, ok := ext.(string); ok { - extStrings = append(extStrings, extStr) + normalized := normalizeAllowedExtension(extStr) + if normalized == "" { + continue + } + if _, exists := seen[normalized]; exists { + continue + } + seen[normalized] = struct{}{} + extStrings = append(extStrings, normalized) } } if len(extStrings) > 0 { diff --git a/pkg/workflow/publish_assets_test.go b/pkg/workflow/publish_assets_test.go index d6cd0341a91..80550bd3d0b 100644 --- a/pkg/workflow/publish_assets_test.go +++ b/pkg/workflow/publish_assets_test.go @@ -46,6 +46,34 @@ func TestParseUploadAssetConfig(t *testing.T) { BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("5")}, }, }, + { + name: "upload-asset config normalizes allowed-exts without dots", + input: map[string]any{ + "upload-asset": map[string]any{ + "allowed-exts": []any{"png", " SVG ", ".jpg", "png"}, + }, + }, + expected: &UploadAssetsConfig{ + BranchName: "assets/${{ github.workflow }}", + MaxSizeKB: 10240, + AllowedExts: []string{".png", ".svg", ".jpg"}, + BaseSafeOutputConfig: BaseSafeOutputConfig{}, + }, + }, + { + name: "upload-asset config preserves github actions expressions in allowed-exts", + input: map[string]any{ + "upload-asset": map[string]any{ + "allowed-exts": []any{"${{ inputs.allowed_exts }}", " PNG "}, + }, + }, + expected: &UploadAssetsConfig{ + BranchName: "assets/${{ github.workflow }}", + MaxSizeKB: 10240, + AllowedExts: []string{"${{ inputs.allowed_exts }}", ".png"}, + BaseSafeOutputConfig: BaseSafeOutputConfig{}, + }, + }, { name: "no upload-asset config", input: map[string]any{}, @@ -88,6 +116,14 @@ func TestParseUploadAssetConfig(t *testing.T) { if len(result.AllowedExts) != len(tt.expected.AllowedExts) { t.Errorf("AllowedExts length: expected %d, got %d", len(tt.expected.AllowedExts), len(result.AllowedExts)) } + for i := range tt.expected.AllowedExts { + if i >= len(result.AllowedExts) { + break + } + if result.AllowedExts[i] != tt.expected.AllowedExts[i] { + t.Errorf("AllowedExts[%d]: expected %s, got %s", i, tt.expected.AllowedExts[i], result.AllowedExts[i]) + } + } }) } } diff --git a/pkg/workflow/upload_assets_config_test.go b/pkg/workflow/upload_assets_config_test.go index 9cacb1b6b8c..0be8919ec1d 100644 --- a/pkg/workflow/upload_assets_config_test.go +++ b/pkg/workflow/upload_assets_config_test.go @@ -68,3 +68,29 @@ func TestUploadAssetsConfigCustomExtensions(t *testing.T) { t.Errorf("Expected custom max size 1024, got %d", config.MaxSizeKB) } } + +func TestUploadAssetsConfigNormalizesExtensions(t *testing.T) { + compiler := NewCompiler() + + outputMap := map[string]any{ + "upload-asset": map[string]any{ + "allowed-exts": []any{"png", " SVG ", ".jpg", "png"}, + }, + } + + config := compiler.parseUploadAssetConfig(outputMap) + if config == nil { + t.Fatal("Expected config to be created") + } + + expectedExts := []string{".png", ".svg", ".jpg"} + if len(config.AllowedExts) != len(expectedExts) { + t.Fatalf("Expected %d normalized extensions, got %d", len(expectedExts), len(config.AllowedExts)) + } + + for i, ext := range expectedExts { + if config.AllowedExts[i] != ext { + t.Errorf("Expected extension %s at position %d, got %s", ext, i, config.AllowedExts[i]) + } + } +}