-
Notifications
You must be signed in to change notification settings - Fork 533
Remediate dynamic-regexp custom-linter findings across pkg/agentdrain, stringutil, parser, cli, workflow #50995
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
d904855
7c858d4
abe6357
adc3a80
7da62de
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 |
|---|---|---|
|
|
@@ -8,6 +8,15 @@ import ( | |
|
|
||
| var activationOutputsCodemodLog = logger.New("cli:codemod_activation_outputs") | ||
|
|
||
| // activationOutputPatterns maps each activation output name to its precompiled, | ||
| // word-boundary-anchored replacement pattern. Using precompiled constant patterns | ||
| // avoids building regexps from dynamic strings. | ||
| var activationOutputPatterns = map[string]*regexp.Regexp{ | ||
| "text": regexp.MustCompile(`needs\.activation\.outputs\.text\b`), | ||
| "title": regexp.MustCompile(`needs\.activation\.outputs\.title\b`), | ||
| "body": regexp.MustCompile(`needs\.activation\.outputs\.body\b`), | ||
| } | ||
|
|
||
| // getActivationOutputsCodemod creates a codemod for transforming needs.activation.outputs.* to steps.sanitized.outputs.* | ||
| func getActivationOutputsCodemod() Codemod { | ||
| return Codemod{ | ||
|
|
@@ -26,10 +35,15 @@ func getActivationOutputsCodemod() Codemod { | |
| for _, output := range outputs { | ||
| newReplacement := "steps.sanitized.outputs." + output | ||
|
|
||
| // Use regex with word boundary to prevent partial matches | ||
| // Use the precompiled pattern with a word boundary to prevent partial matches | ||
| // This ensures we don't match things like "needs.activation.outputs.text_custom" | ||
| // The pattern matches the old expression followed by a non-word character or end of string | ||
| pattern := regexp.MustCompile(`needs\.activation\.outputs\.` + output + `\b`) | ||
| pattern, ok := activationOutputPatterns[output] | ||
|
github-actions[bot] marked this conversation as resolved.
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 map lookup silently skips rewriting an output if it's missing from 💡 Detailspattern, ok := activationOutputPatterns[output]
if !ok {
continue
}Previously every entry in the Suggest deriving |
||
| if !ok { | ||
| // This should never happen: activationOutputPatterns covers exactly the outputs slice. | ||
| // Log a warning so any future drift is visible rather than silently skipped. | ||
| activationOutputsCodemodLog.Printf("WARNING: no precompiled pattern for output %q; skipping", output) | ||
| continue | ||
| } | ||
|
|
||
| // Check if pattern exists in content | ||
| if pattern.MatchString(result) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -181,8 +181,7 @@ func matchesPathAtLevel(line string, pathSegments []PathSegment, level int, arra | |
| switch segment.Type { | ||
| case "key": | ||
| // Look for "key:" pattern | ||
| keyPattern := regexp.MustCompile(`^` + regexp.QuoteMeta(segment.Value) + `\s*:`) | ||
| if keyPattern.MatchString(trimmedLine) { | ||
| if matchesPathSegmentKey(trimmedLine, segment.Value) { | ||
| // Found the key - return position after the colon | ||
| colonIndex := strings.Index(line, ":") | ||
| if colonIndex != -1 { | ||
|
|
@@ -279,8 +278,7 @@ func findFirstAdditionalProperty(yamlContent string, propertyNames []string) JSO | |
| // Check if this line contains any of the additional properties | ||
| for _, propName := range propertyNames { | ||
| // Look for "propName:" pattern at the start of the trimmed line | ||
| keyPattern := regexp.MustCompile(`^` + regexp.QuoteMeta(propName) + `\s*:`) | ||
| if keyPattern.MatchString(trimmedLine) { | ||
| if matchesPathSegmentKey(trimmedLine, propName) { | ||
| // Found the property - return position of the property name | ||
| propIndex := strings.Index(line, propName) | ||
| if propIndex != -1 { | ||
|
|
@@ -409,8 +407,11 @@ func findNestedSectionStart(lines []string, pathSegments []PathSegment) (int, in | |
| } | ||
|
|
||
| func matchesPathSegmentKey(trimmedLine, key string) bool { | ||
| keyPattern := regexp.MustCompile(`^` + regexp.QuoteMeta(key) + `\s*:`) | ||
| return keyPattern.MatchString(trimmedLine) | ||
| if !strings.HasPrefix(trimmedLine, key) { | ||
| return false | ||
| } | ||
| rest := strings.TrimLeft(trimmedLine[len(key):], " \t") | ||
|
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. Whitespace handling narrowed from 💡 DetailsThe original code used rest := strings.TrimLeft(trimmedLine[len(key):], " \t")
return strings.HasPrefix(rest, ":")
Fix: use |
||
| return strings.HasPrefix(rest, ":") | ||
|
github-actions[bot] marked this conversation as resolved.
|
||
| } | ||
|
|
||
| func findNestedSectionEnd(lines []string, foundLine, baseIndentLevel int) int { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -163,9 +163,16 @@ func buildSanitizePreservePattern(opts *SanitizeOptions) string { | |
| // When the caller has requested preservation of special chars, unwanted chars are | ||
| // replaced with hyphens; otherwise they are removed entirely. | ||
| func applySanitizePattern(result, allowedChars string, preserveSpecialChars bool) string { | ||
| // allowedChars is always produced by buildSanitizePreservePattern, which only | ||
| // ever returns one of the combinations already precompiled in sanitizePatterns. | ||
| // Fall back to the base alphanumeric-and-hyphen pattern for safety. | ||
| pattern, ok := sanitizePatterns[allowedChars] | ||
| if !ok { | ||
| pattern = regexp.MustCompile(`[^` + allowedChars + `]+`) | ||
| // allowedChars is always produced by buildSanitizePreservePattern; this branch | ||
| // means a new combination was added there but not to sanitizePatterns. Log a | ||
| // warning so the gap is visible rather than silently using the wrong pattern. | ||
| sanitizeLog.Printf("WARNING: no precompiled sanitize pattern for %q; falling back to a-z0-9-", allowedChars) | ||
| pattern = sanitizePatterns["a-z0-9-"] | ||
|
github-actions[bot] marked this conversation as resolved.
github-actions[bot] marked this conversation as resolved.
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 fallback now silently degrades to the base 💡 Detailspattern, ok := sanitizePatterns[allowedChars]
if !ok {
pattern = sanitizePatterns["a-z0-9-"]
}Previously, an unmapped Consider asserting/panicking on the branch (since it should be unreachable given all current combinations are precompiled), or add a unit test that iterates over the reachable outputs and asserts every one exists in , so any future combination gap fails CI instead of shipping silently.
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 fallback now silently degrades to the base 💡 Detailspattern, ok := sanitizePatterns[allowedChars]
if !ok {
pattern = sanitizePatterns["a-z0-9-"]
}Previously, an unmapped Consider asserting/panicking on the |
||
| } | ||
| if preserveSpecialChars { | ||
| return pattern.ReplaceAllString(result, "-") | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.