Fix error location, double prefix, and confusing paths for nested safe-outputs validation errors #14831
Conversation
- Fix nested section indentation level calculation in json_path_locator.go - Remove redundant FormatErrorMessage wrapper in compile_helpers.go - Remove redundant FormatValidationError wrapper in main.go compile command - Update main error handler to detect console.FormatError pattern Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
All tests passing. Both issues resolved: - Error location now correct (line 6 instead of line 4) - No double check mark prefix (clean IDE-parseable format) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot keep working |
|
@copilot file path looks suspicious: ../../../../../tmp/test.md switch to absolute path if it contains .. |
The PR is complete and working correctly. Both issues have been fixed:
All tests pass:
Example output: |
|
@copilot add more tests with syntax errors at various tree depth |
When a relative path contains "..", display the absolute path instead for better clarity. This prevents confusing paths like "../../../../../tmp/test.md" and shows "/tmp/test.md" instead. Files within the working directory still show clean relative paths like ".github/workflows/test.md". Addresses feedback from @pelikhan Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in commit ac40d83. When relative paths contain "..", the absolute path is now displayed instead. Before: After: Files within the repository still show clean relative paths: |
Added two new test files to validate error location accuracy: 1. safe_outputs_error_location_test.go: - 12 test cases for errors at depths 1-4 - Tests original bug case (depth 2 nested under missing-tool) - Tests complex workflows with multiple sections - Tests edge cases (empty sections, arrays, comments) 2. schema_validation_depth_test.go: - 6 end-to-end integration tests - Tests errors at root, safe-outputs, handlers, tools, and permissions - Validates complete error formatting pipeline All 18 new test cases pass, covering: - Depth 1: Root level properties - Depth 2: Properties in safe-outputs, tools, permissions - Depth 3: Properties in handlers (create-issue, create-discussion, github-token) - Depth 4: Very deeply nested properties Addresses feedback from @pelikhan Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added comprehensive tests in commit 49f1d58. Created 18 new test cases covering syntax errors at depths 1-4: Test files:
Coverage:
All tests pass, including the original bug case where errors in |
…ng directory Updated TestFormatErrorWithAbsolutePaths to expect absolute paths when files are outside the working directory, consistent with the new behavior of ToRelativePath() that avoids confusing paths with ".." components. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🛡️ Security Posture AnalysisThis PR contains changes that may affect the security posture. Please review the following concerns: 🟠 Input validation: dangerous-property filtering removedLocation: Change Detected: -// validateExpressionForDangerousProps checks if an expression contains dangerous JavaScript
-// property names that could be used for prototype pollution or traversal attacks.
-// This matches the JavaScript runtime validation in actions/setup/js/runtime_import.cjs
-// Returns an error if dangerous properties are found.
-func validateExpressionForDangerousProps(expression string) error {
- ...
-}
-
// validateSingleExpression validates a single literal expression
func validateSingleExpression(expression string, opts ExpressionValidationOptions) error {
expression = strings.TrimSpace(expression)
- // First, check for dangerous JavaScript property names that could be used for
- // prototype pollution or traversal attacks (PR #14826)
- if err := validateExpressionForDangerousProps(expression); err != nil {
- return err
- }
-
// Check if this expression is in the allowed list
@@
-// DangerousPropertyNames contains JavaScript built-in property names that are blocked
-// in GitHub Actions expressions to prevent prototype pollution and traversal attacks.
-// This list matches the DANGEROUS_PROPS list in actions/setup/js/runtime_import.cjs
-// See PR #14826 for context on these security measures.
-var DangerousPropertyNames = []string{
- "constructor",
- "__proto__",
- "prototype",
- "__defineGetter__",
- "__defineSetter__",
- "__lookupGetter__",
- "__lookupSetter__",
- "hasOwnProperty",
- "isPrototypeOf",
- "propertyIsEnumerable",
- "toString",
- "valueOf",
- "toLocaleString",
-}Security Impact: Removing the dangerous-property check and the corresponding constant means expressions that use Recommendation: Reintroduce the dangerous-property validation (and the Summary
Note: This is an automated analysis. Please verify these findings and determine if the changes are intentional and justified.
|
There was a problem hiding this comment.
Pull request overview
This PR improves schema/safe-outputs validation error reporting by fixing incorrect source locations, eliminating duplicated error prefixes, and making displayed file paths clearer (especially for files outside the working directory). It also adds new tests to cover error-location behavior across multiple nesting depths.
Changes:
- Fix indentation-level handling in nested YAML section detection to report correct error locations.
- Reduce redundant error message formatting/wrapping to avoid double “✗” prefixes.
- Adjust path rendering to prefer absolute paths when a relative path would include
.., plus add depth-focused tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/parser/schema_validation_depth_test.go | Adds end-to-end tests asserting correct line reporting at multiple nesting depths. |
| pkg/parser/safe_outputs_error_location_test.go | Adds focused tests for locating “additional properties” errors under safe-outputs at depths 1–4. |
| pkg/parser/json_path_locator.go | Fixes nested section indentation/level calculations used for JSON-path-to-YAML location mapping. |
| pkg/console/console_test.go | Updates tests for new ToRelativePath behavior and absolute-path expectations. |
| pkg/console/console.go | Changes ToRelativePath to return absolute paths when relative paths would contain ... |
| pkg/cli/compile_helpers.go | Stops wrapping compile errors with FormatErrorMessage, assuming they’re already formatted. |
| cmd/gh-aw/main.go | Simplifies compile error propagation and adds detection to avoid double formatting at the top-level error handler. |
Comments suppressed due to low confidence (1)
pkg/console/console_test.go:365
- This absolute-path check is POSIX-specific (
strings.HasPrefix(lines[0], "/")) and will fail on Windows where absolute paths don't start with/. Since the repo has Windows CI, preferfilepath.IsAbs(...)(and possiblyfilepath.VolumeNamehandling) to keep the test cross-platform.
// Since tmpDir is outside the working directory (in /tmp), the path should be absolute
// to avoid confusing relative paths with ".." components
lines := strings.Split(output, "\n")
if !strings.HasPrefix(lines[0], "/") {
t.Errorf("Expected output to start with absolute path for files outside working directory, got: %s", lines[0])
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 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") |
There was a problem hiding this comment.
These assertions infer “relative path” by checking !strings.HasPrefix(result, "/"), which is not valid on Windows (absolute paths typically start with a drive letter, so an absolute path could still pass). Since CI runs windows-latest, use filepath.IsAbs(result) and/or validate the absence of .. segments after filepath.Clean instead of relying on / prefixes.
This issue also appears on line 360 of the same file.
| // 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") |
| // If the relative path contains "..", use absolute path instead for clarity | ||
| if strings.Contains(relPath, "..") { | ||
| return path | ||
| } |
There was a problem hiding this comment.
ToRelativePath checks strings.Contains(relPath, ".."), which will also match legitimate directory/file names that include .. (e.g. some..dir/file.md) and incorrectly force an absolute path. Consider detecting .. as a path segment instead (split on filepath.Separator / use filepath.Clean + component scan), or check for patterns like .. at segment boundaries (relPath == "..", prefix ".."+sep, or contains sep+".."+sep).
| if err := CompileWorkflowWithValidation(compiler, file, verbose, false, false, false, false, false); err != nil { | ||
| // Always show compilation errors on new line | ||
| fmt.Fprintln(os.Stderr, console.FormatErrorMessage(err.Error())) | ||
| // Note: Don't wrap in FormatErrorMessage as the error is already formatted by console.FormatError | ||
| fmt.Fprintln(os.Stderr, err.Error()) | ||
| stats.Errors++ |
There was a problem hiding this comment.
compileSingleFile now prints err.Error() directly. CompileWorkflowWithValidation can return plain/unformatted errors (e.g. lockfile YAML validation failures or security-scan wrapper errors in pkg/cli/compile_validation.go), so this is a UX regression vs the previous console.FormatErrorMessage(...) behavior. Suggest conditionally formatting only when the message isn't already in console.FormatError/suggestions/✗ form (similar to main()), or format these non-console.FormatError errors at the source before returning.
| // Get current working directory and construct a path within it | ||
| wd, _ := os.Getwd() | ||
| return filepath.Join(wd, "pkg/console/test.md") |
There was a problem hiding this comment.
The test builds an absolute path from os.Getwd() but ignores the returned error. If Getwd() fails in some environments, this will silently generate a bad path and can make the test behave unpredictably; please handle the error (e.g., require.NoError(t, err) / t.Fatalf(...)).
The compiler was reporting validation errors at the wrong line, adding duplicate "✗" prefixes, and showing confusing relative paths with ".." components. For nested safe-outputs configurations, errors pointed to parent keys instead of the actual invalid property.
Example:
Previously reported error at line 4, now correctly reports at line 6.
Changes
Parser location tracking (
pkg/parser/json_path_locator.go)findNestedSection(): changedlineLevel == currentLevel*2tolineLevel == currentLevelError message formatting (
pkg/cli/compile_helpers.go,cmd/gh-aw/main.go)console.FormatErrorMessage()wrappers that added duplicate "✗" prefixesconsole.FormatErrorpattern (file:line:column:) and skip re-wrappingPath display clarity (
pkg/console/console.go,pkg/console/console_test.go)ToRelativePath()to use absolute paths when relative paths contain ".." for better clarityComprehensive test coverage (
pkg/parser/safe_outputs_error_location_test.go,pkg/parser/schema_validation_depth_test.go)Before:
After (file outside repository):
After (file inside repository):
Test Coverage
New test files:
safe_outputs_error_location_test.go- 12 unit tests for various nesting depthsschema_validation_depth_test.go- 6 integration tests for end-to-end validationTest scenarios:
All tests validate correct line numbers, column positions, and error message formatting.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.