Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 42 additions & 6 deletions pkg/engine/engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -1183,14 +1183,36 @@ func (e *Engine) processResult(
isFalsePositive func(detectors.Result) (bool, string),
) {
ignoreLinePresent := false
if SupportsLineNumbers(data.chunk.SourceType) {
if data.chunk.SourceUpdateLink != nil {
// Source provides its own link update function (e.g., GitHub)
copyChunk := data.chunk
copyMetaDataClone := proto.Clone(data.chunk.SourceMetadata)
if copyMetaData, ok := copyMetaDataClone.(*source_metadatapb.MetaData); ok {
copyChunk.SourceMetadata = copyMetaData
}
fragStart, mdLine, link := FragmentFirstLineAndLink(&copyChunk)
ignoreLinePresent = SetResultLineNumber(&copyChunk, &res, fragStart, mdLine)

// Call source-specific update function
updatedLink := copyChunk.SourceUpdateLink(link, *mdLine)

if err := updateMetaDataLink(copyChunk.SourceMetadata, updatedLink); err != nil {
ctx.Logger().Error(err, "error setting link")
return
}
data.chunk = copyChunk
} else if SupportsLineNumbers(data.chunk.SourceType) {
// Fallback to centralized link updating for sources that haven't migrated yet
// (GitLab, Bitbucket, Azure Repos, etc.)
copyChunk := data.chunk
copyMetaDataClone := proto.Clone(data.chunk.SourceMetadata)
if copyMetaData, ok := copyMetaDataClone.(*source_metadatapb.MetaData); ok {
copyChunk.SourceMetadata = copyMetaData
}
fragStart, mdLine, link := FragmentFirstLineAndLink(&copyChunk)
ignoreLinePresent = SetResultLineNumber(&copyChunk, &res, fragStart, mdLine)
Comment on lines +1204 to +1213
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of duplicating the logic for this else if block, can we add an || in the first if condition? Then when calling the link update function, we can add a smaller if else check.

Copy link
Contributor Author

@amanfcp amanfcp Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the duplication is messy but I was just trying to avoid nested ifs and making as few changes as possible in the previous flow (since it's temporary code if this gets approved)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplication like this can cause maintenance issues. This block already has one nested if so another one wouldn't increases the complexity level IMO


// Use deprecated centralized UpdateLink function
if err := UpdateLink(ctx, copyChunk.SourceMetadata, link, *mdLine); err != nil {
ctx.Logger().Error(err, "error setting link")
return
Expand Down Expand Up @@ -1262,6 +1284,11 @@ func (e *Engine) notifierWorker(ctx context.Context) {
}

// SupportsLineNumbers determines if a line number can be found for a source type.
//
// Deprecated: Prefer source-specific UpdateLink functions (chunk.SourceUpdateLink != nil).
// This function is still used as a fallback for sources that haven't migrated to the
// new pattern (GitLab, Bitbucket, Azure Repos, etc.). Once all sources provide their
// own SourceUpdateLink function, this can be removed entirely.
func SupportsLineNumbers(sourceType sourcespb.SourceType) bool {
switch sourceType {
case sourcespb.SourceType_SOURCE_TYPE_GIT,
Expand Down Expand Up @@ -1360,6 +1387,10 @@ func SetResultLineNumber(chunk *sources.Chunk, result *detectors.Result, fragSta
}

// UpdateLink updates the link of the provided source metadata.
//
// Deprecated: This function is still used as a fallback for sources that don't provide
// their own SourceUpdateLink function (GitLab, Bitbucket, Azure Repos, etc.). New sources
// should implement their own link updating logic and provide it via SourceUpdateLink.
func UpdateLink(ctx context.Context, metadata *source_metadatapb.MetaData, link string, line int64) error {
if metadata == nil {
return fmt.Errorf("metadata is nil when setting the link")
Expand All @@ -1372,17 +1403,22 @@ func UpdateLink(ctx context.Context, metadata *source_metadatapb.MetaData, link

newLink := giturl.UpdateLinkLineNumber(ctx, link, line)

return updateMetaDataLink(metadata, newLink)
}

func updateMetaDataLink(metadata *source_metadatapb.MetaData, link string) error {
// Update the link in metadata
switch meta := metadata.GetData().(type) {
case *source_metadatapb.MetaData_Github:
meta.Github.Link = newLink
meta.Github.Link = link
case *source_metadatapb.MetaData_Gitlab:
meta.Gitlab.Link = newLink
meta.Gitlab.Link = link
case *source_metadatapb.MetaData_Bitbucket:
meta.Bitbucket.Link = newLink
meta.Bitbucket.Link = link
case *source_metadatapb.MetaData_Filesystem:
meta.Filesystem.Link = newLink
meta.Filesystem.Link = link
case *source_metadatapb.MetaData_AzureRepos:
meta.AzureRepos.Link = newLink
meta.AzureRepos.Link = link
default:
return fmt.Errorf("unsupported metadata type")
}
Expand Down
11 changes: 10 additions & 1 deletion pkg/giturl/giturl.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,11 @@ func NormalizeOrgRepoURL(provider provider, repoURL string) (string, error) {
// GenerateLink crafts a link to the specific file from a commit.
// Supports GitHub, GitLab, Bitbucket, and Azure Repos.
// If the provider supports hyperlinks to specific lines, the line number will be included.
//
// Note: The GitHub source now implements its own link generation and updating via function
// pointers (see pkg/sources/github/github.go). This pattern eliminates provider detection
// since each source knows its own type. Other sources still use this function and it
// remains fully supported.
func GenerateLink(repo, commit, file string, line int64) string {
// Some paths contain '%' which breaks |url.Parse| if not encoded.
// https://developer.mozilla.org/en-US/docs/Glossary/Percent-encoding
Expand Down Expand Up @@ -195,7 +200,11 @@ func GenerateLink(repo, commit, file string, line int64) string {
var linePattern = regexp.MustCompile(`L\d+`)

// UpdateLinkLineNumber updates the line number in a repository link.
// Used post-link generation to refine reported issue locations within large scanned blocks.
// Used by sources that don't implement their own UpdateLink function pointer.
//
// Note: The GitHub source now handles link updating via its own UpdateLink function
// attached to chunks. This provides source-specific logic without provider detection.
// This function remains for sources that haven't migrated to the new pattern.
func UpdateLinkLineNumber(ctx context.Context, link string, newLine int64) string {
link = strings.Replace(link, "%", "%25", -1)
link = strings.Replace(link, "[", "%5B", -1)
Expand Down
46 changes: 46 additions & 0 deletions pkg/giturl/helpers.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package giturl

import (
"path/filepath"
"strings"
)

// TrimGitSuffix removes the .git suffix from a repository URL.
func TrimGitSuffix(repo string) string {
return strings.TrimSuffix(repo, ".git")
}

// EncodeSpecialChars encodes special characters in file paths that would
// break URL parsing. Specifically handles %, [, and ].
func EncodeSpecialChars(path string) string {
path = strings.ReplaceAll(path, "%", "%25")
path = strings.ReplaceAll(path, "[", "%5B")
path = strings.ReplaceAll(path, "]", "%5D")
return path
}

// IsGistURL returns true if the repository URL is a GitHub gist.
func IsGistURL(repo string) bool {
return strings.Contains(repo, "gist.github.com")
}

// IsWikiURL returns true if the repository URL is a GitHub wiki.
func IsWikiURL(repo string) bool {
return strings.HasSuffix(repo, ".wiki.git")
}

// TrimWikiSuffix removes the .wiki.git suffix from a repository URL.
func TrimWikiSuffix(repo string) string {
return strings.TrimSuffix(repo, ".wiki.git")
}

Comment on lines +22 to +36
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if it makes sense to have these Github-specific helpers in the giturl package. If they are only used by the Github source, can we move them there?

// CleanGistFilename converts a filename to the format used in gist URLs.
// Dots in filenames are replaced with hyphens.
func CleanGistFilename(filename string) string {
return strings.ReplaceAll(filename, ".", "-")
}

// TrimFileExtension removes the file extension from a path.
func TrimFileExtension(path string) string {
return strings.TrimSuffix(path, filepath.Ext(path))
}
140 changes: 140 additions & 0 deletions pkg/giturl/helpers_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
package giturl

import "testing"

func TestTrimGitSuffix(t *testing.T) {
tests := []struct {
name string
repo string
want string
}{
{"with .git suffix", "https://github.com/owner/repo.git", "https://github.com/owner/repo"},
{"without .git suffix", "https://github.com/owner/repo", "https://github.com/owner/repo"},
{"empty string", "", ""},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := TrimGitSuffix(tt.repo); got != tt.want {
t.Errorf("TrimGitSuffix() = %v, want %v", got, tt.want)
}
})
}
}

func TestEncodeSpecialChars(t *testing.T) {
tests := []struct {
name string
path string
want string
}{
{"no special chars", "path/to/file.go", "path/to/file.go"},
{"percent sign", "path/with%percent.go", "path/with%25percent.go"},
{"square brackets", "path/with[brackets].go", "path/with%5Bbrackets%5D.go"},
{"all special chars", "path%[test].go", "path%25%5Btest%5D.go"},
{"empty string", "", ""},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := EncodeSpecialChars(tt.path); got != tt.want {
t.Errorf("EncodeSpecialChars() = %v, want %v", got, tt.want)
}
})
}
}

func TestIsGistURL(t *testing.T) {
tests := []struct {
name string
repo string
want bool
}{
{"github gist", "https://gist.github.com/user/abc123.git", true},
{"github repo", "https://github.com/owner/repo.git", false},
{"empty string", "", false},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := IsGistURL(tt.repo); got != tt.want {
t.Errorf("IsGistURL() = %v, want %v", got, tt.want)
}
})
}
}

func TestIsWikiURL(t *testing.T) {
tests := []struct {
name string
repo string
want bool
}{
{"github wiki", "https://github.com/owner/repo.wiki.git", true},
{"github repo", "https://github.com/owner/repo.git", false},
{"empty string", "", false},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := IsWikiURL(tt.repo); got != tt.want {
t.Errorf("IsWikiURL() = %v, want %v", got, tt.want)
}
})
}
}

func TestTrimWikiSuffix(t *testing.T) {
tests := []struct {
name string
repo string
want string
}{
{"with .wiki.git suffix", "https://github.com/owner/repo.wiki.git", "https://github.com/owner/repo"},
{"without suffix", "https://github.com/owner/repo.git", "https://github.com/owner/repo.git"},
{"empty string", "", ""},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := TrimWikiSuffix(tt.repo); got != tt.want {
t.Errorf("TrimWikiSuffix() = %v, want %v", got, tt.want)
}
})
}
}

func TestCleanGistFilename(t *testing.T) {
tests := []struct {
name string
filename string
want string
}{
{"single extension", "config.yaml", "config-yaml"},
{"multiple extensions", "config.yaml.example", "config-yaml-example"},
{"no extension", "README", "README"},
{"empty string", "", ""},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := CleanGistFilename(tt.filename); got != tt.want {
t.Errorf("CleanGistFilename() = %v, want %v", got, tt.want)
}
})
}
}

func TestTrimFileExtension(t *testing.T) {
tests := []struct {
name string
path string
want string
}{
{"with extension", "docs/README.md", "docs/README"},
{"without extension", "docs/README", "docs/README"},
{"empty string", "", ""},
{"hidden file", ".gitignore", ""},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := TrimFileExtension(tt.path); got != tt.want {
t.Errorf("TrimFileExtension() = %v, want %v", got, tt.want)
}
})
}
}
Loading
Loading