From 7c650b584b4fb6c6bfe3733181b6e339b92e5dd4 Mon Sep 17 00:00:00 2001 From: Pranav Gaikwad Date: Thu, 16 May 2024 16:48:31 -0400 Subject: [PATCH] fix a bug with absolute / relative paths Signed-off-by: Pranav Gaikwad --- provider/internal/builtin/service_client.go | 102 +++++++++--------- .../internal/builtin/service_client_test.go | 32 +++--- provider/lib.go | 13 ++- provider_container_settings.json | 2 +- provider_local_external_images.json | 8 +- provider_pod_local_settings.json | 8 +- rule-example.yaml | 2 +- 7 files changed, 90 insertions(+), 77 deletions(-) diff --git a/provider/internal/builtin/service_client.go b/provider/internal/builtin/service_client.go index 8577a887..18c3c56f 100644 --- a/provider/internal/builtin/service_client.go +++ b/provider/internal/builtin/service_client.go @@ -58,31 +58,25 @@ func (p *builtinServiceClient) Evaluate(ctx context.Context, cap string, conditi if err != nil { return response, fmt.Errorf("unable to find files using pattern `%s`: %v", c.Pattern, err) } - matchingFiles = p.filterByIncludedPaths(matchingFiles) - if len(matchingFiles) != 0 { - response.Matched = true - } response.TemplateContext = map[string]interface{}{"filepaths": matchingFiles} for _, match := range matchingFiles { - if filepath.IsAbs(match) { - response.Incidents = append(response.Incidents, provider.IncidentContext{ - FileURI: uri.File(match), - }) - continue - + absPath := match + if !filepath.IsAbs(match) { + absPath, err = filepath.Abs(match) + if err != nil { + p.log.V(5).Error(err, "failed to get absolute path to file", "path", match) + absPath = match + } } - ab, err := filepath.Abs(match) - if err != nil { - //TODO: Probably want to log or something to let us know we can't get absolute path here. - fmt.Printf("\n%v\n\n", err) - ab = match + if !p.isFileIncluded(absPath) { + continue } response.Incidents = append(response.Incidents, provider.IncidentContext{ - FileURI: uri.File(ab), + FileURI: uri.File(absPath), }) - } + response.Matched = len(response.Incidents) > 0 return response, nil case "filecontent": c := cond.Filecontent @@ -123,16 +117,21 @@ func (p *builtinServiceClient) Evaluate(ctx context.Context, cap string, conditi continue } - ab, err := filepath.Abs(pieces[0]) + absPath, err := filepath.Abs(pieces[0]) if err != nil { - ab = pieces[0] + absPath = pieces[0] } + + if !p.isFileIncluded(absPath) { + continue + } + lineNumber, err := strconv.Atoi(pieces[1]) if err != nil { return response, fmt.Errorf("cannot convert line number string to integer") } response.Incidents = append(response.Incidents, provider.IncidentContext{ - FileURI: uri.File(ab), + FileURI: uri.File(absPath), LineNumber: &lineNumber, Variables: map[string]interface{}{ "matchingText": pieces[2], @@ -156,7 +155,6 @@ func (p *builtinServiceClient) Evaluate(ctx context.Context, cap string, conditi if err != nil { return response, fmt.Errorf("unable to find XML files: %v", err) } - xmlFiles = p.filterByIncludedPaths(xmlFiles) for _, file := range xmlFiles { nodes, err := queryXMLFile(file, query) if err != nil { @@ -166,15 +164,15 @@ func (p *builtinServiceClient) Evaluate(ctx context.Context, cap string, conditi if len(nodes) != 0 { response.Matched = true for _, node := range nodes { - ab, err := filepath.Abs(file) + absPath, err := filepath.Abs(file) if err != nil { - ab = file + absPath = file } - if paths := p.filterByIncludedPaths([]string{ab}); len(paths) == 0 { + if !p.isFileIncluded(absPath) { continue } incident := provider.IncidentContext{ - FileURI: uri.File(ab), + FileURI: uri.File(absPath), Variables: map[string]interface{}{ "matchingXML": node.OutputXML(false), "innerText": node.InnerText(), @@ -185,7 +183,7 @@ func (p *builtinServiceClient) Evaluate(ctx context.Context, cap string, conditi if content == "" { content = node.Data } - location, err := p.getLocation(ctx, ab, content) + location, err := p.getLocation(ctx, absPath, content) if err == nil { incident.CodeLocation = &location lineNo := int(location.StartPosition.Line) @@ -210,7 +208,6 @@ func (p *builtinServiceClient) Evaluate(ctx context.Context, cap string, conditi if err != nil { return response, fmt.Errorf("unable to find XML files: %v", err) } - xmlFiles = p.filterByIncludedPaths(xmlFiles) for _, file := range xmlFiles { nodes, err := queryXMLFile(file, query) if err != nil { @@ -224,12 +221,15 @@ func (p *builtinServiceClient) Evaluate(ctx context.Context, cap string, conditi if attr.Name.Local == "public-id" { if regex.MatchString(attr.Value) { response.Matched = true - ab, err := filepath.Abs(file) + absPath, err := filepath.Abs(file) if err != nil { - ab = file + absPath = file + } + if !p.isFileIncluded(absPath) { + continue } response.Incidents = append(response.Incidents, provider.IncidentContext{ - FileURI: uri.File(ab), + FileURI: uri.File(absPath), Variables: map[string]interface{}{ "matchingXML": node.OutputXML(false), "innerText": node.InnerText(), @@ -254,7 +254,6 @@ func (p *builtinServiceClient) Evaluate(ctx context.Context, cap string, conditi if err != nil { return response, fmt.Errorf("unable to find files using pattern `%s`: %v", pattern, err) } - jsonFiles = p.filterByIncludedPaths(jsonFiles) for _, file := range jsonFiles { f, err := os.Open(file) if err != nil { @@ -273,18 +272,21 @@ func (p *builtinServiceClient) Evaluate(ctx context.Context, cap string, conditi if len(list) != 0 { response.Matched = true for _, node := range list { - ab, err := filepath.Abs(file) + absPath, err := filepath.Abs(file) if err != nil { - ab = file + absPath = file + } + if !p.isFileIncluded(absPath) { + continue } incident := provider.IncidentContext{ - FileURI: uri.File(ab), + FileURI: uri.File(absPath), Variables: map[string]interface{}{ "matchingJSON": node.InnerText(), "data": node.Data, }, } - location, err := p.getLocation(ctx, ab, node.InnerText()) + location, err := p.getLocation(ctx, absPath, node.InnerText()) if err == nil { incident.CodeLocation = &location lineNo := int(location.StartPosition.Line) @@ -459,9 +461,9 @@ func queryXMLFile(filePath string, query *xpath.Expr) (nodes []*xmlquery.Node, e // filterByIncludedPaths given a list of file paths, // filters-out the ones not present in includedPaths -func (b *builtinServiceClient) filterByIncludedPaths(paths []string) []string { +func (b *builtinServiceClient) isFileIncluded(absolutePath string) bool { if b.includedPaths == nil || len(b.includedPaths) == 0 { - return paths + return true } getSegments := func(path string) []string { @@ -476,23 +478,15 @@ func (b *builtinServiceClient) filterByIncludedPaths(paths []string) []string { return segments } - validatedPaths := []string{} - for _, p := range paths { - absPath := p - if path, err := filepath.Abs(p); err == nil { - absPath = path - } - pathSegments := getSegments(absPath) - for _, includedPath := range b.includedPaths { - includedPathSegments := getSegments(includedPath) - if len(pathSegments) >= len(includedPathSegments) && - strings.HasPrefix(strings.Join(pathSegments, ""), - strings.Join(includedPathSegments, "")) { - validatedPaths = append(validatedPaths, absPath) - } else { - b.log.V(5).Info("path excluded from search", "path", absPath) - } + pathSegments := getSegments(absolutePath) + for _, includedPath := range b.includedPaths { + includedPathSegments := getSegments(includedPath) + if len(pathSegments) >= len(includedPathSegments) && + strings.HasPrefix(strings.Join(pathSegments, ""), + strings.Join(includedPathSegments, "")) { + return true } } - return validatedPaths + b.log.V(5).Info("excluding file from search", "file", absolutePath) + return false } diff --git a/provider/internal/builtin/service_client_test.go b/provider/internal/builtin/service_client_test.go index 43bf6270..8051ef54 100644 --- a/provider/internal/builtin/service_client_test.go +++ b/provider/internal/builtin/service_client_test.go @@ -64,45 +64,45 @@ func Test_builtinServiceClient_getLocation(t *testing.T) { func Test_builtinServiceClient_filterByIncludedPaths(t *testing.T) { tests := []struct { name string - inputPaths []string + inputPath string includedPaths []string - want []string + want bool }{ { name: "no included paths given, match all", - inputPaths: []string{"/test/a/b/", "/test/a/c/"}, + inputPath: "/test/a/b", includedPaths: []string{}, - want: []string{"/test/a/b/", "/test/a/c/"}, + want: true, }, { name: "included file path doesn't match", - inputPaths: []string{"/test/a/b/file.py"}, + inputPath: "/test/a/b/file.py", includedPaths: []string{"/test/a/c/file.py"}, - want: []string{}, + want: false, }, { name: "included file path matches", - inputPaths: []string{"/test/a/b/file.py"}, + inputPath: "/test/a/b/file.py", includedPaths: []string{"/test/a/b/file.py"}, - want: []string{"/test/a/b/file.py"}, + want: true, }, { - name: "input dir path is equal to included path", - inputPaths: []string{"/test/a/b/"}, + name: "input dir path is equivalent to included paths", + inputPath: "/test/a/b/", includedPaths: []string{"////test/a/b//"}, - want: []string{"/test/a/b"}, + want: true, }, { name: "input dir path is a sub-tree of included path", - inputPaths: []string{"/test/a/b/c/d/", "///test/a/b/c/e/file.java"}, + inputPath: "///test/a/b/c/e/", includedPaths: []string{"////test/a/b//"}, - want: []string{"/test/a/b/c/d", "/test/a/b/c/e/file.java"}, + want: true, }, { name: "input dir path is not equal to included path and is not a sub-tree", - inputPaths: []string{"/test/a/b/c/d/", "///test/a/b/c/e/file.java", "/test/a/d/e/f/"}, + inputPath: "///test/a/b/c/e/file.java", includedPaths: []string{"////test/a/d//"}, - want: []string{"/test/a/d/e/f"}, + want: false, }, } for _, tt := range tests { @@ -116,7 +116,7 @@ func Test_builtinServiceClient_filterByIncludedPaths(t *testing.T) { includedPaths: tt.includedPaths, log: testr.New(t), } - if got := b.filterByIncludedPaths(tt.inputPaths); !reflect.DeepEqual(got, tt.want) { + if got := b.isFileIncluded(tt.inputPath); !reflect.DeepEqual(got, tt.want) { t.Errorf("builtinServiceClient.filterByIncludedPaths() = %v, want %v", got, tt.want) } }) diff --git a/provider/lib.go b/provider/lib.go index 6d00f1ea..b26a5c34 100644 --- a/provider/lib.go +++ b/provider/lib.go @@ -150,11 +150,18 @@ func GetIncludedPathsFromConfig(i InitConfig, allowFilePaths bool) []string { if includedPaths, ok := i.ProviderSpecificConfig[IncludedPathsConfigKey].([]interface{}); ok { for _, ipathRaw := range includedPaths { if ipath, ok := ipathRaw.(string); ok { - if stat, err := os.Stat(filepath.Join(i.Location, ipath)); err == nil { + absPath := ipath + if !filepath.IsAbs(ipath) { + if ab, err := filepath.Abs( + filepath.Join(i.Location, ipath)); err == nil { + absPath = ab + } + } + if stat, err := os.Stat(absPath); err == nil { if allowFilePaths || stat.IsDir() { - validatedPaths = append(validatedPaths, ipath) + validatedPaths = append(validatedPaths, absPath) } else { - validatedPaths = append(validatedPaths, filepath.Dir(ipath)) + validatedPaths = append(validatedPaths, filepath.Dir(absPath)) } } } diff --git a/provider_container_settings.json b/provider_container_settings.json index ec9d7674..3f140f8e 100644 --- a/provider_container_settings.json +++ b/provider_container_settings.json @@ -114,7 +114,7 @@ { "location": "examples/builtin/", "providerSpecificConfig": { - "includedPaths": "inclusion_tests/dir-0" + "includedPaths": ["inclusion_tests/dir-0"] } } ] diff --git a/provider_local_external_images.json b/provider_local_external_images.json index 05fa5be5..6da3d6d4 100644 --- a/provider_local_external_images.json +++ b/provider_local_external_images.json @@ -110,7 +110,13 @@ {"location": "external-providers/java-external-provider/examples/java"}, {"location": "external-providers/java-external-provider/examples/customers-tomcat-legacy"}, {"location": "examples/python/"}, - {"location": "examples/golang/"} + {"location": "examples/golang/"}, + { + "location": "examples/builtin/", + "providerSpecificConfig": { + "includedPaths": ["inclusion_tests/dir-0"] + } + } ] } ] diff --git a/provider_pod_local_settings.json b/provider_pod_local_settings.json index c6519743..1fe9f01e 100644 --- a/provider_pod_local_settings.json +++ b/provider_pod_local_settings.json @@ -110,7 +110,13 @@ {"location": "examples/java/"}, {"location": "examples/python/"}, {"location": "examples/golang/"}, - {"location": "examples/customers-tomcat-legacy/"} + {"location": "examples/customers-tomcat-legacy/"}, + { + "location": "examples/builtin/", + "providerSpecificConfig": { + "includedPaths": ["inclusion_tests/dir-0"] + } + } ] } ] diff --git a/rule-example.yaml b/rule-example.yaml index e19bd491..4d614909 100644 --- a/rule-example.yaml +++ b/rule-example.yaml @@ -276,7 +276,7 @@ - builtin.file: pattern: inclusion-test.json - builtin.filecontent: - pattern: "\"inclusionTestNode\"" + pattern: "inclusionTestNode" - category: optional description: | This is same as java-io-file-usage but for the builtin providers. There are multiple instances of the same incidents in different directories.