-
Notifications
You must be signed in to change notification settings - Fork 109
Fix error location, double prefix, and confusing paths for nested safe-outputs validation errors #14831
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
Fix error location, double prefix, and confusing paths for nested safe-outputs validation errors #14831
Changes from all commits
3cb4a6f
fe51796
9f5bed1
ac40d83
49f1d58
f36ac76
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 |
|---|---|---|
|
|
@@ -47,6 +47,7 @@ func applyStyle(style lipgloss.Style, text string) string { | |
| } | ||
|
|
||
| // ToRelativePath converts an absolute path to a relative path from the current working directory | ||
| // If the relative path contains "..", returns the absolute path instead for clarity | ||
| func ToRelativePath(path string) string { | ||
| if !filepath.IsAbs(path) { | ||
| return path | ||
|
|
@@ -64,6 +65,11 @@ func ToRelativePath(path string) string { | |
| return path | ||
| } | ||
|
|
||
| // If the relative path contains "..", use absolute path instead for clarity | ||
| if strings.Contains(relPath, "..") { | ||
| return path | ||
| } | ||
|
Comment on lines
+68
to
+71
|
||
|
|
||
| return relPath | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |||||||||||||||||
| package console | ||||||||||||||||||
|
|
||||||||||||||||||
| import ( | ||||||||||||||||||
| "os" | ||||||||||||||||||
| "path/filepath" | ||||||||||||||||||
| "strings" | ||||||||||||||||||
| "testing" | ||||||||||||||||||
|
|
@@ -298,11 +299,28 @@ func TestToRelativePath(t *testing.T) { | |||||||||||||||||
| }, | ||||||||||||||||||
| }, | ||||||||||||||||||
| { | ||||||||||||||||||
| name: "absolute path converted to relative", | ||||||||||||||||||
| name: "absolute path with .. in relative form returns absolute", | ||||||||||||||||||
| path: "/tmp/gh-aw/test.md", | ||||||||||||||||||
| expectedFunc: func(result, expected string) bool { | ||||||||||||||||||
| // Should be a relative path that doesn't start with / | ||||||||||||||||||
| return !strings.HasPrefix(result, "/") && strings.HasSuffix(result, "test.md") | ||||||||||||||||||
| // When relative path would contain "..", should return absolute path | ||||||||||||||||||
| // The relative path from /home/runner/work/gh-aw/gh-aw to /tmp/gh-aw/test.md | ||||||||||||||||||
| // would be ../../../../../tmp/gh-aw/test.md (contains ..) | ||||||||||||||||||
| // So we should get the absolute path back | ||||||||||||||||||
| return result == "/tmp/gh-aw/test.md" | ||||||||||||||||||
| }, | ||||||||||||||||||
| }, | ||||||||||||||||||
| { | ||||||||||||||||||
| name: "absolute path within working directory converted to relative", | ||||||||||||||||||
| path: func() string { | ||||||||||||||||||
| // Get current working directory and construct a path within it | ||||||||||||||||||
| wd, _ := os.Getwd() | ||||||||||||||||||
| return filepath.Join(wd, "pkg/console/test.md") | ||||||||||||||||||
|
Comment on lines
+315
to
+317
|
||||||||||||||||||
| }(), | ||||||||||||||||||
| expectedFunc: func(result, expected string) bool { | ||||||||||||||||||
| // Absolute path within working directory should be converted to relative | ||||||||||||||||||
| // without ".." in the path | ||||||||||||||||||
| // The result should not start with / and should not contain .. | ||||||||||||||||||
| return !strings.HasPrefix(result, "/") && !strings.Contains(result, "..") && strings.HasSuffix(result, "test.md") | ||||||||||||||||||
|
Comment on lines
+320
to
+323
|
||||||||||||||||||
| // Absolute path within working directory should be converted to relative | |
| // without ".." in the path | |
| // The result should not start with / and should not contain .. | |
| return !strings.HasPrefix(result, "/") && !strings.Contains(result, "..") && strings.HasSuffix(result, "test.md") | |
| // Absolute path within working directory should be converted to a relative path | |
| // that does not traverse upwards ("..") and is not absolute on any platform. | |
| cleaned := filepath.Clean(result) | |
| return !filepath.IsAbs(result) && !strings.Contains(cleaned, "..") && strings.HasSuffix(cleaned, "test.md") |
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.
compileSingleFilenow printserr.Error()directly.CompileWorkflowWithValidationcan return plain/unformatted errors (e.g. lockfile YAML validation failures or security-scan wrapper errors inpkg/cli/compile_validation.go), so this is a UX regression vs the previousconsole.FormatErrorMessage(...)behavior. Suggest conditionally formatting only when the message isn't already inconsole.FormatError/suggestions/✗ form (similar tomain()), or format these non-console.FormatErrorerrors at the source before returning.