You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(fixhandler): normalize a padded base path before storing it - #4822
NewFixHandler trimmed FixInfo.BasePath for its own checks but stored the padded value, so resourceBasePath compared each resource's source path against the padded string, the containment check failed and the resource fell back to the report-wide root. The value is now trimmed once, before it is checked and stored. kubescape fix and Kubescape.Fix already trim it; this covers direct NewFixHandler callers.
How to Test
go test ./core/pkg/fixhandler/ -run 'TestNewFixHandler_PaddedBasePath|TestResourceBasePath'
TestNewFixHandler_PaddedBasePath builds a directory report under a temp dir, passes that dir padded with whitespace as the base path and checks that a resource below it keeps its own source path. On master the handler returns the report-wide root instead.
Base paths with surrounding spaces or tabs are now handled consistently, allowing resources within the configured path to resolve correctly.
Paths that contain only whitespace continue to be treated as empty and are rejected. This avoids inconsistent path handling when whitespace appears before or after a configured base path.
NewFixHandler trimmed FixInfo.BasePath for its own checks but stored the padded value,
so resourceBasePath compared every resource's source path against the padded string,
the containment check failed and the resource fell back to the report-wide root. The
value is trimmed once, before it is checked and stored.
Resolvedkubescape#4816
Signed-off-by: Youssef Omar Bouden <[email protected]>
No actionable comments were generated in the recent review.
Walkthrough
NewFixHandler now trims BasePath before storing it and uses the normalized value in subsequent checks. A test covers a whitespace-padded path and a resource source path beneath it.
Changes: Base path normalization
Layer / File(s)
Summary
Normalize and verify the base path core/pkg/fixhandler/fixhandler.go, core/pkg/fixhandler/fixhandler_test.go
NewFixHandler stores the trimmed BasePath and retains the error for an empty path after trimming. The test checks that a resource source path beneath a padded base path is preserved.
Direct NewFixHandler callers that pass a padded base path will now keep a resource's own source path when it lies below that base. No actionable merge risk remains.
Pre-merge checks: 4
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title names the change: NewFixHandler now trims a padded FixInfo.BasePath before it stores the value. The raw summary and the PR objectives both describe this exact change in core/pkg/fixhandler/f…
Linked Issues check
Issue #4816 requires two coding changes: (1) normalize FixInfo.BasePath once before it is stored in the handler, and (2) add a test with a padded, valid base path and a resource whose source path lies…
Out of Scope Changes check
The whole-PR diff against the merge base touches only core/pkg/fixhandler/fixhandler.go and core/pkg/fixhandler/fixhandler_test.go. The fixhandler.go change is limited to trimming and storing BasePath…
Full details: Title check
Explanation
The title names the change: NewFixHandler now trims a padded FixInfo.BasePath before it stores the value. The raw summary and the PR objectives both describe this exact change in core/pkg/fixhandler/fixhandler.go, and the new test covers the padded base path. The title is specific and follows the conventional prefix style. It does not mention the test, which is expected.
Full details: Linked Issues check
Explanation
Issue #4816 requires two coding changes: (1) normalize FixInfo.BasePath once before it is stored in the handler, and (2) add a test with a padded, valid base path and a resource whose source path lies below it. The diff in core/pkg/fixhandler/fixhandler.go assigns fixInfo.BasePath = strings.TrimSpace(fixInfo.BasePath) before the empty-path check and the EvalSymlinks calls. The stored value is therefore trimmed, so resourceBasePath's containment check receives the trimmed path. The diff in fixhandler_test.go adds TestNewFixHandler_PaddedBasePath. It passes a whitespace-padded trusted directory as BasePath, asserts that handler.fixInfo.BasePath equals the trimmed value, and asserts that resourceBasePath returns the resource's own source path under that base. Both requirements are implemented. I did not execute the test; the verdict rests on the source diff.
Full details: Out of Scope Changes check
Explanation
The whole-PR diff against the merge base touches only core/pkg/fixhandler/fixhandler.go and core/pkg/fixhandler/fixhandler_test.go. The fixhandler.go change is limited to trimming and storing BasePath, which is the objective of #4816. The added test directly verifies that objective. No unrelated changes were found in the reviewed diff.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
File: No configuration file from this repository applied
Commit: Unavailable
Layers: defaults
Review profile: CHILL
Plan: Advanced
Run ID: d3f1b273-500a-4bdc-9e2d-a37659e62c87
Commits
Reviewing files that changed from the base of the PR and between f30072c and c1f0a2c.
Files selected for processing: 2
core/pkg/fixhandler/fixhandler.go
core/pkg/fixhandler/fixhandler_test.go
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Reviewed head c1f0a2cc5b86c38861cc92c3e051926b9293d4a1 against master at f30072c676b13c0df4caf16898c2258a2b28d57b (PR remains open and non-draft). The change is needed: the constructor uses a trimmed base path for initial validation but retained the padded value, so resourceBasePath later rejects valid contained source paths and falls back to the report-wide root. Trimming once before storing fixes that mismatch; the regression test covers the padded valid path and preserves the whitespace-only rejection.
History: #2637 established symlink-aware path containment; #4796 added the initial BasePath validation and recorded this direct-constructor edge case as #4816. The bounded PR/issue searches found no duplicate or superseding change (searches are not exhaustive). No blockers found. The reviewed head CI passed, including the cross-platform build/test job, DCO, GitGuardian, CodeRabbit, and secret-validator; run-system-tests was skipped. I did not run tests locally because this workspace has no repository checkout, and I did not execute PR code in the shared environment.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
matthyx
left a comment
Overview
NewFixHandlertrimmedFixInfo.BasePathfor its own checks but stored the padded value, soresourceBasePathcompared each resource's source path against the padded string, the containment check failed and the resource fell back to the report-wide root. The value is now trimmed once, before it is checked and stored.kubescape fixandKubescape.Fixalready trim it; this covers directNewFixHandlercallers.How to Test
TestNewFixHandler_PaddedBasePathbuilds a directory report under a temp dir, passes that dir padded with whitespace as the base path and checks that a resource below it keeps its own source path. On master the handler returns the report-wide root instead.Related issues/PRs:
Summary by CodeRabbit