-
Notifications
You must be signed in to change notification settings - Fork 368
Fix upload-asset allowed-exts parsing and JS extension normalization #27591
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f9f2d71
8531139
1cba0d6
4c5b845
ae57838
4a004f3
13dc34f
cfc57d4
062f1ed
fb416b5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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}`; | ||
| } | ||
|
|
||
| /** | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test file uses ES module |
||
| * @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, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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, | ||
| }); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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) | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Smoke test review comment — |
||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| 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 | ||||||||||||
| } | ||||||||||||
|
||||||||||||
| } | |
| } | |
| if strings.Trim(normalized, ".") == "" { | |
| return "" | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 Smoke test agent was here — this is a great catch! The bare-dot edge case is easy to overlook. Agreed that rejecting . and .. would be a safer default.
📰 BREAKING: Report filed by Smoke Copilot · ● 1.1M
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The normalizeAllowedExtension function returns "." for a single-dot input, which could create misleading allowlists. Consider rejecting bare dots: if strings.Trim(normalized, ".") == "" { return "" }
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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 ignores empty allowed-exts entries", | |
| input: map[string]any{ | |
| "upload-asset": map[string]any{ | |
| "allowed-exts": []any{".", " ", "\t", " GIF ", "."}, | |
| }, | |
| }, | |
| expected: &UploadAssetsConfig{ | |
| BranchName: "assets/${{ github.workflow }}", | |
| MaxSizeKB: 10240, | |
| AllowedExts: []string{".gif"}, | |
| BaseSafeOutputConfig: BaseSafeOutputConfig{}, | |
| }, | |
| }, | |
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 Smoke test review comment — The regex
[\s\S]*is correct for matching multiline expressions. Consider naming the exportedisGitHubExpressionhelper with a_prefix or JSDoc@internalif it's only used internally, to clarify the public API surface. ✅ Logic looks solid!