diff --git a/pkg/parser/github_urls.go b/pkg/parser/github_urls.go index bdf8d077619..a04e5cbe69f 100644 --- a/pkg/parser/github_urls.go +++ b/pkg/parser/github_urls.go @@ -132,7 +132,7 @@ func parseActionsRunURL(parsedURL *url.URL, host, owner, repo string, parts []st func parseNumberedURL(host, owner, repo string, urlType GitHubURLType, label, numberText string) (*GitHubURLComponents, error) { number, err := strconv.ParseInt(numberText, 10, 32) if err != nil { - return nil, fmt.Errorf("invalid %s number: %s", label, numberText) + return nil, fmt.Errorf("invalid %s number %s: %w", label, numberText, err) } return &GitHubURLComponents{ Host: host, @@ -185,7 +185,7 @@ func parseRunURL(host, owner, repo string, parts []string) (*GitHubURLComponents runID, err := strconv.ParseInt(parts[0], 10, 64) if err != nil { - return nil, fmt.Errorf("invalid run ID: %s", parts[0]) + return nil, fmt.Errorf("invalid run ID %s: %w", parts[0], err) } components := &GitHubURLComponents{ @@ -200,7 +200,7 @@ func parseRunURL(host, owner, repo string, parts []string) (*GitHubURLComponents if len(parts) >= 3 && parts[1] == "job" { jobID, err := strconv.ParseInt(parts[2], 10, 64) if err != nil { - return nil, fmt.Errorf("invalid job ID: %s", parts[2]) + return nil, fmt.Errorf("invalid job ID %s: %w", parts[2], err) } components.JobID = jobID } diff --git a/pkg/parser/github_urls_test.go b/pkg/parser/github_urls_test.go index de06b1b1379..b2884feae1c 100644 --- a/pkg/parser/github_urls_test.go +++ b/pkg/parser/github_urls_test.go @@ -3,6 +3,7 @@ package parser import ( + "errors" "strconv" "strings" "testing" @@ -966,3 +967,42 @@ func TestParseGitHubURL_AdditionalEdgeCases(t *testing.T) { }) } } + +// TestParseGitHubURL_NumErrorWrapping verifies that malformed numeric IDs in URLs +// wrap the underlying *strconv.NumError so callers can inspect it via errors.As. +func TestParseGitHubURL_NumErrorWrapping(t *testing.T) { + tests := []struct { + name string + url string + }{ + { + name: "numbered URL - invalid PR number", + url: "https://github.com/owner/repo/pull/abc", + }, + { + name: "numbered URL - invalid issue number", + url: "https://github.com/owner/repo/issues/abc", + }, + { + name: "run URL - invalid run ID", + url: "https://github.com/owner/repo/actions/runs/notanumber", + }, + { + name: "run URL - invalid job ID", + url: "https://github.com/owner/repo/actions/runs/12345/job/notanumber", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := ParseGitHubURL(tt.url) + if err == nil { + t.Fatal("expected error, got nil") + } + var numErr *strconv.NumError + if !errors.As(err, &numErr) { + t.Errorf("errors.As(*strconv.NumError) = false; want true so NumError is reachable via %v", err) + } + }) + } +} diff --git a/pkg/parser/path_rel_error.go b/pkg/parser/path_rel_error.go new file mode 100644 index 00000000000..84c984a4647 --- /dev/null +++ b/pkg/parser/path_rel_error.go @@ -0,0 +1,13 @@ +package parser + +// pathRelError is returned when filepath.Rel fails during a security path check. +// Its Error() method omits the internal absolute paths from the filepath.Rel +// diagnostic so they do not leak into user-facing error messages, while Unwrap() +// preserves the upstream error for errors.Is / errors.As traversal. +type pathRelError struct { + msg string + err error +} + +func (e *pathRelError) Error() string { return e.msg } +func (e *pathRelError) Unwrap() error { return e.err } diff --git a/pkg/parser/remote_fetch_wasm.go b/pkg/parser/remote_fetch_wasm.go index 87415c90aea..66eca14bed3 100644 --- a/pkg/parser/remote_fetch_wasm.go +++ b/pkg/parser/remote_fetch_wasm.go @@ -106,8 +106,15 @@ func ResolveIncludePath(filePath, baseDir string, cache *ImportCache) (string, e normalizedSecurityBase := filepath.Clean(securityBase) normalizedFullPath := filepath.Clean(fullPath) - relativePath, err := filepath.Rel(normalizedSecurityBase, normalizedFullPath) - if err != nil || relativePath == ".." || strings.HasPrefix(relativePath, ".."+string(filepath.Separator)) || filepath.IsAbs(relativePath) { + relativePath, relErr := filepath.Rel(normalizedSecurityBase, normalizedFullPath) + if relErr != nil { + allowedFolder := filepath.Base(normalizedSecurityBase) + return "", &pathRelError{ + msg: fmt.Sprintf("security: path %s must be within %s folder", filePath, allowedFolder), + err: relErr, + } + } + if relativePath == ".." || strings.HasPrefix(relativePath, ".."+string(filepath.Separator)) || filepath.IsAbs(relativePath) { allowedFolder := filepath.Base(normalizedSecurityBase) return "", fmt.Errorf("security: path %s must be within %s folder (resolves to: %s)", filePath, allowedFolder, relativePath) } diff --git a/pkg/parser/remote_resolve_path.go b/pkg/parser/remote_resolve_path.go index 4a20d813568..0b19b8b2922 100644 --- a/pkg/parser/remote_resolve_path.go +++ b/pkg/parser/remote_resolve_path.go @@ -176,8 +176,16 @@ func resolveAndValidateLocalIncludePath(filePath, resolveBase, securityBase stri fullPath := filepath.Join(resolveBase, filePath) normalizedSecurityBase := filepath.Clean(securityBase) normalizedFullPath := filepath.Clean(fullPath) - relativePath, err := filepath.Rel(normalizedSecurityBase, normalizedFullPath) - if err != nil || relativePath == ".." || strings.HasPrefix(relativePath, ".."+string(filepath.Separator)) || filepath.IsAbs(relativePath) { + relativePath, relErr := filepath.Rel(normalizedSecurityBase, normalizedFullPath) + if relErr != nil { + allowedFolder := filepath.Base(normalizedSecurityBase) + remoteLog.Printf("Security: Path escapes allowed folder: %s (rel error: %v)", filePath, relErr) + return "", &pathRelError{ + msg: fmt.Sprintf("security: path %s must be within %s folder", filePath, allowedFolder), + err: relErr, + } + } + if relativePath == ".." || strings.HasPrefix(relativePath, ".."+string(filepath.Separator)) || filepath.IsAbs(relativePath) { allowedFolder := filepath.Base(normalizedSecurityBase) remoteLog.Printf("Security: Path escapes allowed folder: %s (resolves to: %s)", filePath, relativePath) return "", fmt.Errorf("security: path %s must be within %s folder (resolves to: %s)", filePath, allowedFolder, relativePath) diff --git a/pkg/parser/schedule_parser.go b/pkg/parser/schedule_parser.go index 0694b708c42..8ec13fbe166 100644 --- a/pkg/parser/schedule_parser.go +++ b/pkg/parser/schedule_parser.go @@ -228,7 +228,10 @@ func (p *ScheduleParser) minimumIntervalTokenCount(hasWeekdaysSuffix bool) int { func (p *ScheduleParser) parseLongIntervalParts() (int, string, error) { intervalStr := p.tokens[1] interval, err := strconv.Atoi(intervalStr) - if err != nil || interval < 1 { + if err != nil { + return 0, "", fmt.Errorf("invalid interval '%s', must be a positive integer: %w", intervalStr, err) + } + if interval < 1 { return 0, "", fmt.Errorf("invalid interval '%s', must be a positive integer", intervalStr) } diff --git a/pkg/parser/schedule_parser_test.go b/pkg/parser/schedule_parser_test.go index 58ead907040..7d53d867209 100644 --- a/pkg/parser/schedule_parser_test.go +++ b/pkg/parser/schedule_parser_test.go @@ -3,6 +3,7 @@ package parser import ( + "strconv" "testing" "github.com/stretchr/testify/assert" @@ -1096,6 +1097,41 @@ func TestParseSchedule(t *testing.T) { } } +// TestParseLongIntervalParts_NumErrorWrapping verifies the error-wrapping contract +// introduced in parseLongIntervalParts: +// - a non-numeric interval string wraps *strconv.NumError so callers can inspect it +// - zero and negative intervals use a fresh error (no upstream NumError to wrap) +func TestParseLongIntervalParts_NumErrorWrapping(t *testing.T) { + p := &ScheduleParser{} + + t.Run("non-numeric interval wraps NumError", func(t *testing.T) { + p.tokens = []string{"every", "abc", "minutes"} + _, _, err := p.parseLongIntervalParts() + require.Error(t, err) + var numErr *strconv.NumError + assert.ErrorAs(t, err, &numErr, + "errors.As(*strconv.NumError) should be true for non-numeric interval") + }) + + t.Run("zero interval does not wrap NumError", func(t *testing.T) { + p.tokens = []string{"every", "0", "minutes"} + _, _, err := p.parseLongIntervalParts() + require.Error(t, err) + var numErr *strconv.NumError + assert.NotErrorAs(t, err, &numErr, + "errors.As(*strconv.NumError) should be false for out-of-range interval") + }) + + t.Run("negative interval does not wrap NumError", func(t *testing.T) { + p.tokens = []string{"every", "-5", "minutes"} + _, _, err := p.parseLongIntervalParts() + require.Error(t, err) + var numErr *strconv.NumError + assert.NotErrorAs(t, err, &numErr, + "errors.As(*strconv.NumError) should be false for negative interval") + }) +} + // TestErrUnsupportedSyntaxIs verifies that all four "explicitly unsupported // syntax" branches wrap ErrUnsupportedSyntax so that errors.Is works, and // that an ordinary parse error (unrecognised keyword) does not.