fix: add path sanitization to prevent directory traversal in downloader

[ilopezluna]

This pull request adds path sanitization to the Huggingface downloader to prevent directory traversal vulnerabilities, ensuring downloaded files cannot escape the intended directory. It also introduces comprehensive tests for the new logic.

[gemini-code-assist]

Code Review

This pull request introduces a sanitizePath function and unit tests to prevent path traversal vulnerabilities when downloading files. A critical issue was identified in the path validation logic at pkg/distribution/huggingface/downloader.go:146, as it fails to correctly handle root directories and relative base paths. The reviewer suggested using filepath.Rel for a more robust and idiomatic implementation.

[sourcery-ai]

Hey - I've left some high level feedback:

• The sanitizePath implementation relies on strings.HasPrefix with a cleaned base path, which can be fragile (e.g., case-insensitive filesystems, unusual separator behavior); consider using filepath.Rel and verifying the result does not start with .. instead of a raw string prefix check to make the containment check more robust across platforms.
• The TestSanitizePath case labeled "absolute path is treated as relative by filepath.Join" assumes behavior that filepath.Join does not have on Unix (an absolute second argument wins), so the expectation for wantErr: false and the constructed wantPath should be revisited, and it may be safer to use t.TempDir() for baseDir to avoid OS-specific absolute path issues.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `sanitizePath` implementation relies on `strings.HasPrefix` with a cleaned base path, which can be fragile (e.g., case-insensitive filesystems, unusual separator behavior); consider using `filepath.Rel` and verifying the result does not start with `..` instead of a raw string prefix check to make the containment check more robust across platforms.
- The `TestSanitizePath` case labeled "absolute path is treated as relative by filepath.Join" assumes behavior that `filepath.Join` does not have on Unix (an absolute second argument wins), so the expectation for `wantErr: false` and the constructed `wantPath` should be revisited, and it may be safer to use `t.TempDir()` for `baseDir` to avoid OS-specific absolute path issues.

---

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.}

[ilopezluna]

@sourcery-ai review

[sourcery-ai]

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

• The new downloader_test.go lives in the huggingface package but only exercises archive.CheckRelative directly; consider either moving these cases into the archive package’s tests or adding tests that go through downloadFileWithProgress/DownloadAll so the behavior is verified at the downloader boundary rather than re-testing a lower-level helper.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The new downloader_test.go lives in the huggingface package but only exercises archive.CheckRelative directly; consider either moving these cases into the archive package’s tests or adding tests that go through downloadFileWithProgress/DownloadAll so the behavior is verified at the downloader boundary rather than re-testing a lower-level helper.

## Individual Comments

### Comment 1
<location path="pkg/distribution/huggingface/downloader.go" line_range="147-149" />
<code_context>
-	// Create local file path (preserve directory structure)
-	localPath := filepath.Join(d.tempDir, file.Path)
+	// Validate file path to prevent directory traversal attacks
+	localPath, err := archive.CheckRelative(d.tempDir, file.Path)
+	if err != nil {
+		return "", fmt.Errorf("invalid file path %q: %w", file.Path, err)
+	}

</code_context>
<issue_to_address>
**🚨 suggestion (security):** Clarify and distinguish security-related path validation errors from other I/O/path errors

`archive.CheckRelative` is a good choice for preventing traversal, but this wraps every failure as an "invalid file path" with a security-tinged message. If `CheckRelative` can fail for non-malicious reasons (e.g., malformed path, internal bug, or I/O issues), callers will misinterpret those as security problems. Either map a specific traversal sentinel error to the current message and use a more neutral message for other errors (e.g., "failed to resolve local path"), or rephrase the message to cover both security validation and generic failures accurately.

Suggested implementation:

```golang
	"errors"

	"github.com/docker/model-runner/pkg/internal/archive"

```

```golang
	// Validate file path and resolve it under tempDir
	localPath, err := archive.CheckRelative(d.tempDir, file.Path)
	if err != nil {
		// Distinguish potential traversal attempts from other path resolution errors
		if errors.Is(err, archive.ErrPathTraversal) {
			return "", fmt.Errorf("invalid file path (possible directory traversal) %q: %w", file.Path, err)
		}
		return "", fmt.Errorf("failed to resolve local path for %q: %w", file.Path, err)
	}

```

I’ve assumed `archive.CheckRelative` exposes a sentinel error like `archive.ErrPathTraversal` that is returned when a directory traversal attempt is detected. If the actual sentinel name or type differs (e.g., `archive.ErrUnsafePath`, `archive.ErrInvalidRelativePath`, or a custom error type), update the `errors.Is(err, archive.ErrPathTraversal)` call to use the correct symbol and import path. If `CheckRelative` does not currently expose a specific traversal sentinel, you may want to add one there to make this distinction robust.
</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.}

[sourcery-ai]

🚨 suggestion (security): Clarify and distinguish security-related path validation errors from other I/O/path errors

archive.CheckRelative is a good choice for preventing traversal, but this wraps every failure as an "invalid file path" with a security-tinged message. If CheckRelative can fail for non-malicious reasons (e.g., malformed path, internal bug, or I/O issues), callers will misinterpret those as security problems. Either map a specific traversal sentinel error to the current message and use a more neutral message for other errors (e.g., "failed to resolve local path"), or rephrase the message to cover both security validation and generic failures accurately.

Suggested implementation:

	"errors"

	"github.com/docker/model-runner/pkg/internal/archive"

	// Validate file path and resolve it under tempDir
	localPath, err := archive.CheckRelative(d.tempDir, file.Path)
	if err != nil {
		// Distinguish potential traversal attempts from other path resolution errors
		if errors.Is(err, archive.ErrPathTraversal) {
			return "", fmt.Errorf("invalid file path (possible directory traversal) %q: %w", file.Path, err)
		}
		return "", fmt.Errorf("failed to resolve local path for %q: %w", file.Path, err)
	}

I’ve assumed archive.CheckRelative exposes a sentinel error like archive.ErrPathTraversal that is returned when a directory traversal attempt is detected. If the actual sentinel name or type differs (e.g., archive.ErrUnsafePath, archive.ErrInvalidRelativePath, or a custom error type), update the errors.Is(err, archive.ErrPathTraversal) call to use the correct symbol and import path. If CheckRelative does not currently expose a specific traversal sentinel, you may want to add one there to make this distinction robust.