Conversation
Signed-off-by: yugal07 <yashsadhwani544@gmail.com>
Signed-off-by: yugal07 <yashsadhwani544@gmail.com>
📝 WalkthroughWalkthroughThese changes introduce a path normalization utility function and apply it across event-related getter methods to ensure consistent path string formatting. The normalization handles empty paths, relative paths, proc-style paths, and resolves path separators and dot components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/utils/struct_event.go (1)
151-153: Optional: Consider normalizingGetCwdfor consistency.
GetCwdreturns the rawCwdfield without normalization. If the current working directory could contain relative components (.,..) or redundant slashes, consider applyingNormalizePathhere as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/utils/struct_event.go` around lines 151 - 153, GetCwd currently returns the raw StructEvent.Cwd which may contain relative components or redundant slashes; update GetCwd to return a normalized path by calling the NormalizePath helper (or equivalent) on e.Cwd before returning so callers always receive a canonical path; locate the method named GetCwd on type StructEvent and wrap/replace the plain return of e.Cwd with a call to NormalizePath(e.Cwd) (handle empty/nil cases consistently with other uses of NormalizePath).pkg/utils/normalize_path_test.go (1)
7-82: Good test coverage; consider adding edge cases.The table-driven tests cover the main scenarios well. Consider adding these edge cases to strengthen coverage:
".hidden"→"/.hidden"(dot-prefixed filename, not current directory)"/abc/fd/3"→"/abc/fd/3"(non-numeric prefix should NOT get/procprepended)"/../../../etc"→"/etc"(path.Clean handles escaping root)"///"→"/"(multiple slashes only)📝 Suggested additional test cases
{ name: "path with dot components", input: "/usr/./bin/../lib", expected: "/usr/lib", }, + { + name: "dot-prefixed filename", + input: ".hidden", + expected: "/.hidden", + }, + { + name: "non-numeric prefix not treated as proc path", + input: "/abc/fd/3", + expected: "/abc/fd/3", + }, + { + name: "path escaping root via ..", + input: "/../../../etc", + expected: "/etc", + }, + { + name: "multiple slashes only", + input: "///", + expected: "/", + }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/utils/normalize_path_test.go` around lines 7 - 82, Add the suggested edge-case table-driven tests to TestNormalizePath to strengthen coverage: include cases for input ".hidden" expecting "/.hidden", "/abc/fd/3" expecting "/abc/fd/3" (ensure non-numeric prefix doesn't get /proc), "/../../../etc" expecting "/etc" (path.Clean behavior), and "///" expecting "/"; add these entries to the existing tests slice used by NormalizePath so they run alongside the existing cases.pkg/utils/path.go (1)
11-15: Consider applying normalization toFimEventImpl.GetPathfor consistency.The
FimEventImpl(inpkg/hostfimsensor/fimsensor_interface.go, lines 60-62) implementsGetPath()but returns the raw path without normalization. In contrast, bothStructEventandDatasourceEventnormalize their paths usingNormalizePath()in their respectiveGetPath()implementations. This inconsistency could lead to different path formats depending on which event type is used, potentially causing issues downstream.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/utils/path.go` around lines 11 - 15, FimEventImpl.GetPath currently returns the raw path while StructEvent.GetPath and DatasourceEvent.GetPath call NormalizePath; update FimEventImpl.GetPath to call NormalizePath(path) (or the equivalent NormalizePath method) before returning to ensure consistent normalized paths across event types; locate the method named GetPath on the FimEventImpl type in fimsensor_interface.go and mirror the normalization logic used by StructEvent.GetPath and DatasourceEvent.GetPath.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/utils/path.go`:
- Around line 16-34: Normalizing paths in NormalizePath() changes outputs of
GetOldPath() and GetNewPath(), which are used for hash calculations in
pkg/containerprofilemanager/v1/event_reporting.go (symlink/hardlink hashing) and
will break compatibility with existing stored profile hashes; update the code by
either: 1) preserving previous hashing behavior for compatibility (use original
unnormalized path values when computing hashes in the functions that call
GetOldPath/GetNewPath in event_reporting.go), or 2) implementing a
versioning/migration strategy (add a profile hash version field and migrate or
accept dual-hash checks) so existing profiles remain valid; locate
NormalizePath, GetOldPath/GetNewPath, and the hash computation sites in
event_reporting.go (around the symlink/hardlink handling) and change the hashing
callers to use the pre-normalized path or add versioned hash handling/migration
logic accordingly.
---
Nitpick comments:
In `@pkg/utils/normalize_path_test.go`:
- Around line 7-82: Add the suggested edge-case table-driven tests to
TestNormalizePath to strengthen coverage: include cases for input ".hidden"
expecting "/.hidden", "/abc/fd/3" expecting "/abc/fd/3" (ensure non-numeric
prefix doesn't get /proc), "/../../../etc" expecting "/etc" (path.Clean
behavior), and "///" expecting "/"; add these entries to the existing tests
slice used by NormalizePath so they run alongside the existing cases.
In `@pkg/utils/path.go`:
- Around line 11-15: FimEventImpl.GetPath currently returns the raw path while
StructEvent.GetPath and DatasourceEvent.GetPath call NormalizePath; update
FimEventImpl.GetPath to call NormalizePath(path) (or the equivalent
NormalizePath method) before returning to ensure consistent normalized paths
across event types; locate the method named GetPath on the FimEventImpl type in
fimsensor_interface.go and mirror the normalization logic used by
StructEvent.GetPath and DatasourceEvent.GetPath.
In `@pkg/utils/struct_event.go`:
- Around line 151-153: GetCwd currently returns the raw StructEvent.Cwd which
may contain relative components or redundant slashes; update GetCwd to return a
normalized path by calling the NormalizePath helper (or equivalent) on e.Cwd
before returning so callers always receive a canonical path; locate the method
named GetCwd on type StructEvent and wrap/replace the plain return of e.Cwd with
a call to NormalizePath(e.Cwd) (handle empty/nil cases consistently with other
uses of NormalizePath).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09f248d7-6f5d-4997-8ef8-8d8a2033056a
📒 Files selected for processing (4)
pkg/utils/datasource_event.gopkg/utils/normalize_path_test.gopkg/utils/path.gopkg/utils/struct_event.go
| func NormalizePath(p string) string { | ||
| if p == "" { | ||
| return "" | ||
| } | ||
|
|
||
| if p == "." { | ||
| return "/" | ||
| } | ||
|
|
||
| if headlessProcRegex.MatchString(p) { | ||
| p = "/proc" + p | ||
| } | ||
|
|
||
| if !strings.HasPrefix(p, "/") { | ||
| p = "/" + p | ||
| } | ||
|
|
||
| return path.Clean(p) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for all usages of GetOldPath, GetNewPath, GetPath, GetFullPath, GetExePath
# to identify potential breaking changes where path comparisons or hashes are performed
echo "=== Checking hash/identifier calculations using path getters ==="
rg -n -C3 'CalculateSHA256|Hash.*Get(Old|New|Full)?Path|Get(Old|New|Full)?Path.*Hash'
echo ""
echo "=== Checking string comparisons on path getters ==="
rg -n -C3 'strings\.(HasPrefix|HasSuffix|Contains|Compare).*Get(Old|New|Full|Exe)?Path'
echo ""
echo "=== Checking equality comparisons on path getters ==="
rg -n -C3 'Get(Old|New|Full|Exe)?Path\(\)\s*[!=]='Repository: kubescape/node-agent
Length of output: 8890
🏁 Script executed:
# Find all usages of NormalizePath
echo "=== All usages of NormalizePath ==="
rg -n "NormalizePath" --type go
echo ""
echo "=== LinkEvent interface/struct definition ==="
rg -n "type LinkEvent|func.*GetOldPath|func.*GetNewPath" --type go -A2Repository: kubescape/node-agent
Length of output: 2143
Implementation looks correct; verify downstream hash compatibility.
The normalization logic is well-structured and correctly handles the documented cases. However, the path normalization is applied in the GetOldPath() and GetNewPath() methods (see pkg/utils/datasource_event.go lines 493, 503 and pkg/utils/struct_event.go lines 282, 290), which are used directly in hash calculations for symlinks and hardlinks in pkg/containerprofilemanager/v1/event_reporting.go (lines 103, 116). Since these getters now return normalized paths, existing stored profile hashes will no longer match hashes computed from new events.
This is a breaking change: paths that were previously relative (now get / prefix), . (now become /), or headless proc patterns (now get /proc prefix) will all produce different hash values.
Consider whether a data migration or versioning strategy is needed for existing profiles.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/utils/path.go` around lines 16 - 34, Normalizing paths in NormalizePath()
changes outputs of GetOldPath() and GetNewPath(), which are used for hash
calculations in pkg/containerprofilemanager/v1/event_reporting.go
(symlink/hardlink hashing) and will break compatibility with existing stored
profile hashes; update the code by either: 1) preserving previous hashing
behavior for compatibility (use original unnormalized path values when computing
hashes in the functions that call GetOldPath/GetNewPath in event_reporting.go),
or 2) implementing a versioning/migration strategy (add a profile hash version
field and migrate or accept dual-hash checks) so existing profiles remain valid;
locate NormalizePath, GetOldPath/GetNewPath, and the hash computation sites in
event_reporting.go (around the symlink/hardlink handling) and change the hashing
callers to use the pre-normalized path or add versioned hash handling/migration
logic accordingly.
2 participants
Summary by CodeRabbit
New Features
Tests