Fix upload-asset allowed-exts parsing and JS extension normalization#27591
Fix upload-asset allowed-exts parsing and JS extension normalization#27591
Conversation
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/d805e60c-20aa-4a2f-86e0-c69c378498d0 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Normalizes safe-outputs.upload-asset.allowed-exts values during parsing so configurations that omit leading dots (or include whitespace / mixed casing / duplicates) behave consistently.
Changes:
- Add
normalizeAllowedExtension()to trim whitespace, lowercase, and add a leading.when missing. - Deduplicate
allowed-extsentries while preserving first-seen order during config parsing. - Add unit tests covering normalization behavior in both config and parsing tests.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/publish_assets.go | Normalizes and deduplicates allowed-exts during upload-asset config parsing. |
| pkg/workflow/publish_assets_test.go | Adds a parsing test case asserting normalization/dedup behavior. |
| pkg/workflow/upload_assets_config_test.go | Adds a config-level test for extension normalization behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
| } | ||
| if !strings.HasPrefix(normalized, ".") { | ||
| normalized = "." + normalized | ||
| } |
There was a problem hiding this comment.
normalizeAllowedExtension treats a single dot (".") as a valid extension and will return ".". This is unlikely to be a meaningful file extension and can lead to confusing or overly-broad allowlists if downstream validation uses suffix checks. Consider treating "." (and possibly other non-extension values like "..") as invalid by returning "" so it gets skipped during parsing.
| } | |
| } | |
| if strings.Trim(normalized, ".") == "" { | |
| return "" | |
| } |
There was a problem hiding this comment.
👋 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
| BaseSafeOutputConfig: BaseSafeOutputConfig{}, | ||
| }, | ||
| }, | ||
| { |
There was a problem hiding this comment.
The new normalization behavior is covered for missing dots, whitespace, casing, and duplicates, but there isn't a test case for edge inputs like "." or whitespace-only strings. Adding such a case would clarify the intended behavior (e.g., whether these entries should be ignored) and prevent regressions as normalization rules evolve.
| { | |
| { | |
| 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{}, | |
| }, | |
| }, | |
| { |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/3ef72008-9f47-40a0-9fda-79ba057a572b Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/3ef72008-9f47-40a0-9fda-79ba057a572b Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/3ef72008-9f47-40a0-9fda-79ba057a572b Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/77050c4d-1c90-4348-8809-a8d29c6c1090 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/77050c4d-1c90-4348-8809-a8d29c6c1090 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/77050c4d-1c90-4348-8809-a8d29c6c1090 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
| var publishAssetsLog = logger.New("workflow:publish_assets") | ||
|
|
||
| func normalizeAllowedExtension(extension string) string { | ||
| normalized := strings.ToLower(strings.TrimSpace(extension)) |
There was a problem hiding this comment.
@copilot do not normalize if it is a github actions expression, pass as is the JavaScript will normalize
There was a problem hiding this comment.
Updated in cfc57d4: Go now preserves GitHub Actions expressions in allowed-exts as-is, and JavaScript handles normalization/validation at runtime. Screenshot: N/A (no UI changes).
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/62a39b21-d83d-41b7-8ee0-77c94be0e2e3 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/62a39b21-d83d-41b7-8ee0-77c94be0e2e3 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot make sure the file are copied with setup.sh |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/80136222-62f9-48b4-9c3a-2409a9dc67d2 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in |
|
🚀 Smoke OpenCode MISSION COMPLETE! OpenCode delivered. 🔥 |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
|
|
|
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS
|
|
Smoke Test Results — Run §24733695961
Overall: PARTIAL (1 failure: AW MCP Status tool error; 2 skipped)
|
There was a problem hiding this comment.
💥 Automated smoke test review — all systems nominal! This review was auto-generated by the Claude smoke test agent for run §24733695961.
💥 [THE END] — Illustrated by Smoke Claude · ● 147K
| function isGitHubExpression(value) { | ||
| const trimmed = value.trim(); | ||
| return /^\$\{\{[\s\S]*\}\}$/.test(trimmed); | ||
| } |
There was a problem hiding this comment.
🤖 Smoke test review comment — The regex [\s\S]* is correct for matching multiline expressions. Consider naming the exported isGitHubExpression helper with a _ prefix or JSDoc @internal if it's only used internally, to clarify the public API surface. ✅ Logic looks solid!
|
|
||
| func isGitHubExpression(value string) bool { | ||
| trimmed := strings.TrimSpace(value) | ||
| return githubExpressionPattern.MatchString(trimmed) |
There was a problem hiding this comment.
🤖 Smoke test review comment — isGitHubExpression in Go mirrors the JS helper nicely. The (?s) flag in the regex makes . match newlines, which is correct for multiline expressions like $\{\{ steps.x.outputs\ny }}. 💡 Consider exporting this as IsGitHubExpression if other packages need it in the future.
|
🧪 Smoke Test Results — Run §24733696225
Overall: ✅ PASS (13/13) PR Author:
|
There was a problem hiding this comment.
Overall the PR cleanly refactors extension normalization into shared helpers for both Go and JS sides, with good test coverage. Two minor points: (1) normalizeAllowedExtension should reject a bare . input; (2) confirm the .test.cjs file uses require() rather than ES import to match the CJS helper module.
📰 BREAKING: Report filed by Smoke Copilot · ● 1.1M
| normalized := strings.ToLower(trimmed) | ||
| if !strings.HasPrefix(normalized, ".") { | ||
| normalized = "." + normalized | ||
| } |
There was a problem hiding this comment.
The normalizeAllowedExtension function returns "." for a single-dot input, which could create misleading allowlists. Consider rejecting bare dots: if strings.Trim(normalized, ".") == "" { return "" }
| return normalized.startsWith(".") ? normalized : `.${normalized}`; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
The test file uses ES module import syntax but the helper uses CommonJS (module.exports). Make sure the test runner is configured for CJS or switch the test to require() to avoid module format mismatches.
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
Summary
safe-outputs.upload-asset.allowed-extsvalues during Go parsing.when missing, and deduplicate entries while preserving orderallowed-extsvalues as-is (no normalization)GH_AW_ASSETS_ALLOWED_EXTSin JavaScript for literal extensions (trim/lowercase/add leading dot)GH_AW_ASSETS_ALLOWED_EXTSis already expanded at runtime: unresolved GitHub Actions expressions (${{ ... }}) are rejected with a configuration errorValidation
go test -v -run "TestUploadAssetsConfig(CustomExtensions|Defaults|NormalizesExtensions)|TestParseUploadAssetConfig" ./pkg/workflow/cd actions/setup/js && npm run test:js -- allowed_extensions_helpers.test.cjs safe_outputs_handlers.test.cjsmake agent-finish(fails due to pre-existing lint issues in unrelated files:pkg/actionpins/spec_test.go,pkg/cli/spec_test.go)parallel_validation(Code Review + CodeQL)✨ PR Review Safe Output Test - Run 24733695961