fix: Potential fix for code scanning alert no. 14: Uncontrolled data used in path expression

[skyoo2003]

Potential fix for https://github.com/skyoo2003/devcloud/security/code-scanning/14

Best fix: strengthen safePath to validate each untrusted path segment before joining and then perform a robust relative-path containment check after resolving absolute paths.

In internal/services/s3/store.go:

• Add validation in safePath(parts ...string) to reject:
  • empty segments
  • absolute segments
  • segments containing .. traversal semantics (part == ".." or starts with "../" patterns after cleaning)
• Build candidate path from baseDir + validated parts.
• Resolve both baseDir and candidate to absolute paths and verify containment using filepath.Rel (reject if rel starts with .. or is absolute).
• Keep existing behavior (allow nested keys like a/b/c by preserving valid subpaths), but reject traversal/escape inputs deterministically.

No changes are required in provider.go for this finding; centralizing enforcement in safePath fixes all call paths (GetObject, PutObject, DeleteObject, etc.).

Suggested fixes powered by Copilot Autofix. Review carefully before merging.

[sourcery-ai]

Reviewer's Guide

Strengthens the S3 FileStore path-safety helper to defensively validate each path segment and enforce a robust absolute-path containment check, ensuring all object operations are protected against directory traversal and path escape attacks.

File-Level Changes

Change	Details	Files
Harden path validation and containment logic in the FileStore safePath helper to prevent directory traversal and enforce absolute-path containment.	Validate each incoming path segment, rejecting empty strings, absolute segments, and any segment that normalizes to, or starts with, a parent-directory traversal (..). Normalize accepted segments with filepath.Clean and accumulate them into a new slice before joining. Resolve the FileStore base directory to an absolute path and join it with the validated segments to produce an absolute candidate path. Use filepath.Rel to compute the relative path from the base directory to the candidate and reject results that indicate escaping the base directory (..-prefixed or absolute). Change safePath to return the fully resolved absolute candidate path instead of a potentially relative cleaned path.	internal/services/s3/store.go

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• The new safePath now rejects empty path segments and always returns an absolute path, which is a behavior change from the previous implementation; double-check callers that may rely on passing empty segments or on getting back a path relative to fs.baseDir.
• You’re recomputing filepath.Abs(fs.baseDir) on every safePath call; consider normalizing and storing an absolute baseDir once in NewFileStore to avoid repeated work and to simplify the containment check.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The new `safePath` now rejects empty path segments and always returns an absolute path, which is a behavior change from the previous implementation; double-check callers that may rely on passing empty segments or on getting back a path relative to `fs.baseDir`.
- You’re recomputing `filepath.Abs(fs.baseDir)` on every `safePath` call; consider normalizing and storing an absolute `baseDir` once in `NewFileStore` to avoid repeated work and to simplify the containment check.

## Individual Comments

### Comment 1
<location path="internal/services/s3/store.go" line_range="30-31" />
<code_context>
-	if !strings.HasPrefix(cleaned, fs.baseDir+string(filepath.Separator)) && cleaned != fs.baseDir {
-		return "", fmt.Errorf("path traversal detected: %s", cleaned)
+	cleanParts := make([]string, 0, len(parts))
+	for _, part := range parts {
+		if part == "" {
+			return "", fmt.Errorf("invalid empty path segment")
+		}
</code_context>
<issue_to_address>
**question (bug_risk):** Rejecting empty path segments may be a breaking behavior change compared to the previous implementation.

`filepath.Join` previously ignored empty segments, but the new logic returns an error when `part == ""`, changing semantics for callers that may legitimately pass empty strings (e.g., conditional appends). If this behavior change isn’t deliberate, consider skipping empty segments or normalizing `parts` before this check. If it is deliberate, please verify that no existing callers rely on the old behavior.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}