fix: runtime guard for empty SHA in action pins + remove invalid ruby/setup-ruby@v1.319.0 entry #47048
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -122,15 +122,7 @@ func getActionPins() []ActionPin { | |
| actionPinsOnce.Do(func() { | ||
| actionPinsLog.Print("Unmarshaling action pins from embedded JSON (first call, will be cached)") | ||
|
|
||
| data := loadActionPinsData(actionPinsJSON) | ||
|
|
||
| pins := slices.Collect(maps.Values(data.Entries)) | ||
|
|
||
|
|
@@ -157,6 +149,27 @@ func getActionPins() []ActionPin { | |
| return cachedActionPins | ||
| } | ||
|
|
||
| // loadActionPinsData unmarshals embedded action pin data. | ||
| // Panics if the embedded JSON is invalid or any entry has an empty SHA, because | ||
| // those conditions indicate corrupted release data that would produce invalid workflow YAML. | ||
| func loadActionPinsData(raw []byte) ActionPinsData { | ||
| var data ActionPinsData | ||
| if err := json.Unmarshal(raw, &data); err != nil { | ||
| actionPinsLog.Printf("Failed to unmarshal action pins JSON: %v", err) | ||
| panic(fmt.Sprintf("failed to load action pins: %v", err)) | ||
| } | ||
|
|
||
| if n := countPinKeyMismatches(data.Entries); n > 0 { | ||
| actionPinsLog.Printf("Found %d key/version mismatches in action_pins.json", n) | ||
| } | ||
|
|
||
| if emptyKeys := collectEntriesWithEmptySHA(data.Entries); len(emptyKeys) > 0 { | ||
| panic(fmt.Sprintf("action_pins.json has %d entries with empty SHA %v — these would produce invalid workflow YAML (e.g. 'owner/repo@ # version'); remove or fix these entries before releasing", len(emptyKeys), emptyKeys)) | ||
| } | ||
|
|
||
| return data | ||
| } | ||
|
|
||
| // countPinKeyMismatches returns the number of entries where the key version does not | ||
| // match pin.Version, logging each mismatch for diagnostics. | ||
| func countPinKeyMismatches(entries map[string]ActionPin) int { | ||
|
|
@@ -175,6 +188,20 @@ func countPinKeyMismatches(entries map[string]ActionPin) int { | |
| return count | ||
| } | ||
|
|
||
| // collectEntriesWithEmptySHA returns the keys of entries whose SHA field is empty, | ||
| // logging each offending entry for diagnostics. | ||
| func collectEntriesWithEmptySHA(entries map[string]ActionPin) []string { | ||
| var keys []string | ||
| for key, pin := range entries { | ||
| if pin.SHA == "" { | ||
| keys = append(keys, key) | ||
| actionPinsLog.Printf("ERROR: Empty SHA in action_pins.json: key=%s repo=%s version=%s", key, pin.Repo, pin.Version) | ||
| } | ||
| } | ||
| slices.Sort(keys) | ||
| return keys | ||
| } | ||
|
|
||
| // buildByRepoIndex groups pins by repository and sorts each group by version descending. | ||
| func buildByRepoIndex(pins []ActionPin) map[string][]ActionPin { | ||
| byRepo := make(map[string][]ActionPin, len(pins)) | ||
|
|
@@ -226,7 +253,12 @@ func getLatestActionPinReference(repo string) string { | |
|
|
||
| // FormatPinnedActionReference formats a pinned action reference with repo, SHA, and version comment. | ||
| // Example: "actions/checkout@abc123 # v4.1.0" | ||
| // Panics if sha is empty, because that would emit invalid workflow YAML and indicates | ||
| // a programming error or corrupted action pin data that should already have been rejected. | ||
| func FormatPinnedActionReference(repo, sha, version string) string { | ||
| if sha == "" { | ||
|
Contributor
There was a problem hiding this comment.
💡 Suggested fixReturn an error instead of panicking: func FormatPinnedActionReference(repo, sha, version string) (string, error) {
if sha == "" {
return "", fmt.Errorf("FormatPinnedActionReference: empty SHA for repo=%s version=%s", repo, version)
}
return repo + "@" + sha + " # " + version, nil
}Panicking is appropriate for truly unrecoverable programmer errors. An empty SHA in a data file is a recoverable data error. The load-time guard in
Contributor
Author
There was a problem hiding this comment. I kept the existing panic in 9d2aee1 for the same reason as the load-time guard: emitting |
||
| panic(fmt.Sprintf("FormatPinnedActionReference called with empty SHA for repo=%s version=%s — this would produce invalid workflow YAML", repo, version)) | ||
| } | ||
|
Comment on lines
254
to
+261
Contributor
Author
There was a problem hiding this comment. Addressed in 9d2aee1: the |
||
| return repo + "@" + sha + " # " + version | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,6 +63,59 @@ func TestCountPinKeyMismatches_ReturnsOnlyVersionMismatches(t *testing.T) { | |
| }) | ||
| } | ||
|
|
||
| func TestCollectEntriesWithEmptySHA_ReturnsOnlyEmptySHAEntries(t *testing.T) { | ||
| t.Run("returns empty slice for empty entries", func(t *testing.T) { | ||
| assert.Empty(t, collectEntriesWithEmptySHA(map[string]ActionPin{}), "Expected empty input to produce empty result") | ||
| }) | ||
|
|
||
| t.Run("returns empty slice when all entries have non-empty SHAs", func(t *testing.T) { | ||
| entries := map[string]ActionPin{ | ||
| "actions/checkout@v5": {Repo: "actions/checkout", Version: "v5", SHA: "abc123"}, | ||
| "actions/setup-go@v4": {Repo: "actions/setup-go", Version: "v4", SHA: "def456"}, | ||
| } | ||
| assert.Empty(t, collectEntriesWithEmptySHA(entries), "Expected empty result when all SHAs are populated") | ||
| }) | ||
|
|
||
| t.Run("returns key of entry with empty SHA", func(t *testing.T) { | ||
| entries := map[string]ActionPin{ | ||
| "actions/checkout@v5": {Repo: "actions/checkout", Version: "v5", SHA: "abc123"}, | ||
| "ruby/setup-ruby@v1.319.0": {Repo: "ruby/setup-ruby", Version: "v1.319.0", SHA: ""}, | ||
| } | ||
| assert.Equal(t, []string{"ruby/setup-ruby@v1.319.0"}, collectEntriesWithEmptySHA(entries), "Expected the empty-SHA entry key to be returned") | ||
| }) | ||
|
|
||
| t.Run("returns sorted keys of multiple entries with empty SHA", func(t *testing.T) { | ||
| entries := map[string]ActionPin{ | ||
| "actions/checkout@v5": {Repo: "actions/checkout", Version: "v5", SHA: "abc123"}, | ||
| "ruby/setup-ruby@v1.319.0": {Repo: "ruby/setup-ruby", Version: "v1.319.0", SHA: ""}, | ||
| "owner/repo@v2": {Repo: "owner/repo", Version: "v2", SHA: ""}, | ||
| } | ||
| assert.Equal(t, []string{"owner/repo@v2", "ruby/setup-ruby@v1.319.0"}, collectEntriesWithEmptySHA(entries), "Expected sorted keys of empty-SHA entries") | ||
| }) | ||
| } | ||
|
|
||
| func TestLoadActionPinsData_PanicsWhenEntrySHAIsEmpty(t *testing.T) { | ||
| fixture := []byte(`{ | ||
| "entries": { | ||
| "ruby/setup-ruby@v1.319.0": { | ||
| "repo": "ruby/setup-ruby", | ||
| "version": "v1.319.0", | ||
| "sha": "" | ||
| } | ||
| } | ||
| }`) | ||
|
|
||
| assert.Panics(t, func() { | ||
| loadActionPinsData(fixture) | ||
| }, "Expected loadActionPinsData to panic when embedded pin data contains an empty SHA") | ||
| } | ||
|
|
||
| func TestFormatPinnedActionReference_PanicsWhenSHAIsEmpty(t *testing.T) { | ||
| assert.Panics(t, func() { | ||
| FormatPinnedActionReference("ruby/setup-ruby", "", "v1.319.0") | ||
| }, "Expected FormatPinnedActionReference to panic when SHA is empty") | ||
| } | ||
|
|
||
| func TestInitWarnings_InitializesAndPreservesMap(t *testing.T) { | ||
| t.Run("initializes nil warnings map", func(t *testing.T) { | ||
| ctx := &PinContext{} | ||
|
Contributor
There was a problem hiding this comment. [/tdd] The load-time guard added to 💡 Suggested approachExtract the load-from-JSON path into a small internal helper (or make the test inject a fixture JSON) so the integration between the guard and the loader can be covered: func TestGetActionPins_PanicsOnEmptySHAInEmbeddedData(t *testing.T) {
assert.Panics(t, func() { parseActionPins(jsonWithEmptySHA) })
}This closes the gap between the helper tests and the call-site guard. @copilot please address this.
Contributor
Author
There was a problem hiding this comment. Addressed in 9d2aee1: I extracted the embedded JSON load path into |
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
[/diagnosing-bugs] Using
panicfor a defense-in-depth guard is a strong signal, but in a library loaded at startup it will crash the entire binary for any caller — including future callers who may never reach the formatting path. Consider returning an error instead (or logging + returning a sentinel string) so the host program can handle the case gracefully.💡 Alternative signature
The load-time guard in
getActionPinsalready prevents empty-SHA entries from reaching this function in normal usage; the panic here only fires on programming errors, which makes the tradeoff reasonable — just worth calling out explicitly in a comment so future readers understand the intent.@copilot please address this.
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.
Kept the panic contract in 9d2aee1, but documented the intent explicitly: embedded data is rejected at load time, and this call-site guard remains a defense-in-depth invariant check for programming errors or corrupted pin data.