fix(path): normalize manifest-generate-paths to resolve '..' notation (#25366)

[fsdkibe]

… (#25366)

This PR fixes a bug where webhooks would fail to trigger an application refresh if the manifest-generate-paths annotation contained an absolute path with ../ notation.

The fix involves running filepath.Clean() on all paths read from the annotation to normalize and resolve dot-dot references before comparison.

Fixes #25366

Checklist:

• [x] Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• [x] The title of the PR states what changed and the related issues number (used for the release note).
• [x] The title of the PR conforms to the Title of the PR
• [x] I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• [ ] I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• [ ] Does this PR require documentation updates?
• [ ] I've updated documentation as required by this PR.
• [x] I have signed off all my commits as required by DCO
• [x] I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• [ ] My build is green (troubleshooting builds).
• [ ] My new feature complies with the feature status guidelines.
• [x] I have added a brief description of why this PR is necessary and/or what this PR solves.
• [ ] Optional. My organization is added to USERS.md.
• [ ] Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

[bunnyshell[bot]]

:x: Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

• :rocket: /bns:deploy to deploy the environment

[codecov[bot]]

Codecov Report

:x: Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 62.45%. Comparing base (7cdc0f9) to head (d29bcf8). :warning: Report is 10 commits behind head on master.

Files with missing lines	Patch %	Lines
util/app/path/path.go	85.71%	1 Missing and 1 partial :warning:

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #25370   +/-   ##
=======================================
  Coverage   62.45%   62.45%           
=======================================
  Files         351      351           
  Lines       49490    49500   +10     
=======================================
+ Hits        30910    30917    +7     
+ Misses      15621    15619    -2     
- Partials     2959     2964    +5     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[blakepettersson]

This seems good so far, but I would also like us to do an extra check here https://github.com/argoproj/argo-cd/blob/d23501875cb98d1169f6d262c983bb229aea5dac/util/app/path/path.go#L122

Or more specifically here https://github.com/argoproj/argo-cd/blob/d23501875cb98d1169f6d262c983bb229aea5dac/util/security/path_traversal.go#L11

Could we refactor that to make use of the os.Root functionality, as an extra check?

Something like below (this is a rough outline, not a fully working example)?

func EnforceToCurrentRoot(currentRoot, requestedPath string) (string, error) {
	root, err := os.OpenRoot(currentRoot)
       // check error

        requestedDir := filepath.Rel(k.repoRoot, requestedPath)
	_, err = root.Stat(requestedDir)

// more stuff
}

[fsdkibe]

@blakepettersson, I've refactored the security check to use os.OpenRoot as suggested. Could you please review it?

[fsdkibe]

@blakepettersson, I attempted to implement the os.OpenRoot refactor as suggested, but it caused significant regressions, including segmentation faults, in the existing reposerver test suite. It appears many legacy tests rely on mock paths that do not exist on the filesystem, which causes os.OpenRoot to fail where the previous string-based check succeeded.

To keep this PR focused on fixing the reported bug (#25366), I have reverted the security refactor. I strongly suggest handling the migration to os.OpenRoot and the necessary test suite updates in a separate, dedicated PR. What's your suggestion on that?

[bunnyshell[bot]]

:x: Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

• :rocket: /bns:deploy to deploy the environment

[blakepettersson]

I strongly suggest handling the migration to os.OpenRoot and the necessary test suite updates in a separate, dedicated PR. What's your suggestion on that?

Sure, feel free to do it in a separate PR. I'm not inclined to merge this PR without having the security refactor in place though.

[fsdkibe]

I have opened a separate PR (#25515) that implements the stricter filepath.Rel security checks. Please review it and let me know if it should be merged first or if we should cherry-pick it into this branch.