Skip to content

Commit 48c05ba

Browse files
varunsh-codervarunsh-coder
authored
Merge pull request #2536 from step-security/shubham/main
fix: handle quote delimiter for pin actions -> prod
2 parents 336876c + 20ab155 commit 48c05ba

File tree

4 files changed

+89
-27
lines changed

4 files changed

+89
-27
lines changed

‎remediation/workflow/pin/pinactions.go‎

Lines changed: 58 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -80,37 +80,67 @@ func PinAction(action, inputYaml string, exemptedActions []string, pinToImmutabl
8080
return inputYaml, updated
8181
}
8282

83-
pinnedAction := fmt.Sprintf("%s@%s # %s", leftOfAt[0], commitSHA, tagOrBranch)
83+
// pinnedAction := fmt.Sprintf("%s@%s # %s", leftOfAt[0], commitSHA, tagOrBranch)
84+
// build separately so we can quote only the ref, not the comment
85+
pinnedRef := fmt.Sprintf("%s@%s", leftOfAt[0], commitSHA)
86+
comment := fmt.Sprintf(" # %s", tagOrBranch)
87+
fullPinned := pinnedRef + comment
8488

8589
// if the action with version is immutable, then pin the action with version instead of sha
8690
pinnedActionWithVersion := fmt.Sprintf("%s@%s", leftOfAt[0], tagOrBranch)
8791
if pinToImmutable && semanticTagRegex.MatchString(tagOrBranch) && IsImmutableAction(pinnedActionWithVersion) {
88-
pinnedAction = pinnedActionWithVersion
89-
}
92+
// strings.ReplaceAll is not suitable here because it would incorrectly replace substrings
93+
// For example, if we want to replace "actions/checkout@v1" to "actions/checkout@v1.2.3", it would also incorrectly match and replace in "actions/checkout@v1.2.3"
94+
// making new string to "actions/checkout@v1.2.3.2.3"
95+
//
96+
// Instead, we use a regex pattern that ensures we only replace complete action references:
97+
// Pattern: (<action>@<version>)($|\s|"|')
98+
// - Group 1 (<action>@<version>): Captures the exact action reference
99+
// - Group 2 ($|\s|"|'): Captures the delimiter that follows (end of line, whitespace, or quotes)
100+
//
101+
// Examples:
102+
// - "actions/checkout@v1.2.3" - No match (no delimiter after v1)
103+
// - "actions/checkout@v1 " - Matches (space delimiter)
104+
// - "actions/checkout@v1"" - Matches (quote delimiter)
105+
// - "actions/checkout@v1" - Matches (quote delimiter)
106+
// - "actions/checkout@v1\n" - Matches (newline is considered whitespace \s)
107+
108+
actionRegex := regexp.MustCompile(`(` + regexp.QuoteMeta(action) + `)($|\s|"|')`)
109+
inputYaml = actionRegex.ReplaceAllString(inputYaml, pinnedActionWithVersion+"$2")
110+
111+
inputYaml, _ = removePreviousActionComments(pinnedActionWithVersion, inputYaml)
112+
return inputYaml, !strings.EqualFold(action, pinnedActionWithVersion)
113+
}
114+
115+
updated = !strings.EqualFold(action, fullPinned)
116+
117+
// 1) Double-quoted form: "owner/repo@oldRef"
118+
doubleQuotedPattern := `"` + regexp.QuoteMeta(action) + `"` + `($|\s|"|')`
119+
doubleQuotedRe := regexp.MustCompile(doubleQuotedPattern)
120+
inputYaml = doubleQuotedRe.ReplaceAllString(
121+
inputYaml,
122+
fmt.Sprintf(`"%s"%s$1`, pinnedRef, comment),
123+
)
124+
inputYaml, _ = removePreviousActionComments(fmt.Sprintf(`"%s"%s`, pinnedRef, comment), inputYaml)
125+
126+
// 2) Single-quoted form: 'owner/repo@oldRef'
127+
singleQuotedPattern := `'` + regexp.QuoteMeta(action) + `'` + `($|\s|"|')`
128+
singleQuotedRe := regexp.MustCompile(singleQuotedPattern)
129+
inputYaml = singleQuotedRe.ReplaceAllString(
130+
inputYaml,
131+
fmt.Sprintf(`'%s'%s$1`, pinnedRef, comment),
132+
)
133+
inputYaml, _ = removePreviousActionComments(fmt.Sprintf(`'%s'%s`, pinnedRef, comment), inputYaml)
134+
135+
// 3) Unquoted form: owner/repo@oldRef
136+
unqPattern := `\b` + regexp.QuoteMeta(action) + `\b` + `($|\s|"|')`
137+
unqRe := regexp.MustCompile(unqPattern)
138+
inputYaml = unqRe.ReplaceAllString(
139+
inputYaml,
140+
fullPinned+`$1`,
141+
)
142+
inputYaml, _ = removePreviousActionComments(fullPinned, inputYaml)
90143

91-
updated = !strings.EqualFold(action, pinnedAction)
92-
93-
// strings.ReplaceAll is not suitable here because it would incorrectly replace substrings
94-
// For example, if we want to replace "actions/checkout@v1" to "actions/checkout@v1.2.3", it would also incorrectly match and replace in "actions/checkout@v1.2.3"
95-
// making new string to "actions/checkout@v1.2.3.2.3"
96-
//
97-
// Instead, we use a regex pattern that ensures we only replace complete action references:
98-
// Pattern: (<action>@<version>)($|\s|"|')
99-
// - Group 1 (<action>@<version>): Captures the exact action reference
100-
// - Group 2 ($|\s|"|'): Captures the delimiter that follows (end of line, whitespace, or quotes)
101-
//
102-
// Examples:
103-
// - "actions/checkout@v1.2.3" - No match (no delimiter after v1)
104-
// - "actions/checkout@v1 " - Matches (space delimiter)
105-
// - "actions/checkout@v1"" - Matches (quote delimiter)
106-
// - "actions/checkout@v1" - Matches (quote delimiter)
107-
// - "actions/checkout@v1\n" - Matches (newline is considered whitespace \s)
108-
actionRegex := regexp.MustCompile(`(` + regexp.QuoteMeta(action) + `)($|\s|"|')`)
109-
inputYaml = actionRegex.ReplaceAllString(inputYaml, pinnedAction+"$2")
110-
yamlWithPreviousActionCommentsRemoved, wasModified := removePreviousActionComments(pinnedAction, inputYaml)
111-
if wasModified {
112-
return yamlWithPreviousActionCommentsRemoved, updated
113-
}
114144
return inputYaml, updated
115145
}
116146

@@ -126,11 +156,12 @@ func removePreviousActionComments(pinnedAction, inputYaml string) (string, bool)
126156
inputYaml = stringParts[0]
127157
for idx := 1; idx < len(stringParts); idx++ {
128158
trimmedString := strings.SplitN(stringParts[idx], "\n", 2)
159+
inputYaml = inputYaml + pinnedAction
129160
if len(trimmedString) > 1 {
130161
if strings.Contains(trimmedString[0], "#") {
131162
updated = true
132163
}
133-
inputYaml = inputYaml + pinnedAction + "\n" + trimmedString[1]
164+
inputYaml = inputYaml + "\n" + trimmedString[1]
134165
}
135166
}
136167
}

‎remediation/workflow/pin/pinactions_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,7 @@ func TestPinActions(t *testing.T) {
295295
{fileName: "immutableaction-1.yml", wantUpdated: true, pinToImmutable: true},
296296
{fileName: "exemptaction.yml", wantUpdated: true, exemptedActions: []string{"actions/checkout", "rohith/*"}, pinToImmutable: true},
297297
{fileName: "donotpintoimmutable.yml", wantUpdated: true, pinToImmutable: false},
298+
{fileName: "invertedcommas.yml", wantUpdated: true, pinToImmutable: false},
298299
}
299300
for _, tt := range tests {
300301
input, err := ioutil.ReadFile(path.Join(inputDirectory, tt.fileName))

‎testfiles/pinactions/input/invertedcommas.yml‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
name: "close issue"
2+
3+
on:
4+
push:
5+
6+
jobs:
7+
closeissue:
8+
runs-on: ubuntu-latest
9+
10+
steps:
11+
- name: Close Issue
12+
uses: "peter-evans/close-issue@v1"
13+
with:
14+
issue-number: 1
15+
comment: Auto-closing issue

‎testfiles/pinactions/output/invertedcommas.yml‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
name: "close issue"
2+
3+
on:
4+
push:
5+
6+
jobs:
7+
closeissue:
8+
runs-on: ubuntu-latest
9+
10+
steps:
11+
- name: Close Issue
12+
uses: "peter-evans/close-issue@a700eac5bf2a1c7a8cb6da0c13f93ed96fd53dbe" # v1.0.3
13+
with:
14+
issue-number: 1
15+
comment: Auto-closing issue

0 commit comments

Comments
 (0)