wip weights

[michaeldwan]

No description provided.

[ask-bonk]

LGTM

github run

[ask-bonk]

I've reviewed the managed weights PR. The code is generally well-structured with good documentation. I've identified a few actionable issues around security and correctness.

[ask-bonk]

This function relies on path validation happening elsewhere (//nolint:gosec // path is under the configured source dir), but this creates a maintenance risk. If validation in computeInventory is bypassed or has a bug, this becomes a path traversal vulnerability. Consider adding explicit validation here:

Suggested change

	abs := filepath.Join(s.dir, filepath.FromSlash(path))
	abs := filepath.Join(s.dir, filepath.FromSlash(path))
	abs = filepath.Clean(abs)
	if !strings.HasPrefix(abs, s.dir+string(filepath.Separator)) && abs != s.dir {
	return nil, fmt.Errorf("path escapes source directory: %s", path)
	}

[ask-bonk]

The size parameter is ignored (underscore _). This parameter should be validated against the actual bytes written to detect truncation or corruption. Consider either:

1. Validating the size matches after writing:

Suggested change

	func (s *FileStore) PutFile(ctx context.Context, expectedDigest string, _ int64, r io.Reader) error {
	func (s *FileStore) PutFile(ctx context.Context, expectedDigest string, expectedSize int64, r io.Reader) error {
	...
	n, err := io.Copy(tmp, reader)
	if err != nil {
	_ = tmp.Close()
	return fmt.Errorf("write %s: %w", expectedDigest, err)
	}
	if n != expectedSize {
	_ = tmp.Close()
	return fmt.Errorf("size mismatch for %s: expected %d, got %d", expectedDigest, expectedSize, n)
	}

2. Or remove the unused parameter to avoid confusion.

[ask-bonk]

URLs are constructed using fmt.Sprintf with user-controlled path components. While s.repo and s.ref are validated, the path argument in fetchFile could contain special characters that require URL escaping. Consider using url.PathEscape:

Suggested change

	url := fmt.Sprintf("%s/api/models/%s/revision/%s", s.baseURL, s.repo, s.ref)
	func (s *HFSource) fetchFile(ctx context.Context, ref, path string) (io.ReadCloser, error) {
	escapedPath := url.PathEscape(path)
	url := fmt.Sprintf("%s/%s/resolve/%s/%s", s.baseURL, s.repo, ref, escapedPath)
	return s.doGet(ctx, url)
	}

[ask-bonk]

The writeLayer function checks ctx.Err() when writing files (line 428), but not when writing directory headers. For consistency and correctness in highly concurrent scenarios, consider adding a context check before each directory header write:

Suggested change

	for _, dir := range collectDirs(files) {
	for _, dir := range collectDirs(files) {
	if err := ctx.Err(); err != nil {
	return err
	}
	if err := tw.WriteHeader(deterministicDirHeader(dir)); err != nil {
	return fmt.Errorf("write dir header %s: %w", dir, err)
	}
	}

[ask-bonk]

Posted review with 4 suggestions targeting security and correctness issues:

1. Path traversal validation in FileSource.Open - adds explicit validation to prevent directory escape
2. Size validation in PutFile - validates the size parameter instead of ignoring it
3. URL path escaping in HuggingFace source - escapes path components when constructing URLs
4. Context cancellation in writeLayer - adds consistent context checking for directory headers

github run

[ask-bonk]

I'm Bonk, and I've done a quick review of your PR. This PR introduces managed weights functionality for Cog, implementing a weight import/push/pull system using OCI artifacts.

I've identified 4 actionable issues around security and correctness that should be addressed:

[ask-bonk]

The size parameter is ignored (underscore _). This parameter should be validated against the actual bytes written to detect truncation or corruption. Consider either:

1. Validating the size matches after writing:

Suggested change

	func (s *FileStore) PutFile(ctx context.Context, expectedDigest string, _ int64, r io.Reader) error {
	func (s *FileStore) PutFile(ctx context.Context, expectedDigest string, expectedSize int64, r io.Reader) error {
	...
	n, err := io.Copy(tmp, reader)
	if err != nil {
	_ = tmp.Close()
	return fmt.Errorf("write %s: %w", expectedDigest, err)
	}
	if n != expectedSize {
	_ = tmp.Close()
	return fmt.Errorf("size mismatch for %s: expected %d, got %d", expectedDigest, expectedSize, n)
	}

2. Or remove the unused parameter to avoid confusion.

[ask-bonk]

URLs are constructed using fmt.Sprintf with user-controlled path components. While s.repo and s.ref are validated, the path argument in fetchFile could contain special characters that require URL escaping. Consider using url.PathEscape:

Suggested change

	func (s *HFSource) fetchFile(ctx context.Context, ref, path string) (io.ReadCloser, error) {
	func (s *HFSource) fetchFile(ctx context.Context, ref, path string) (io.ReadCloser, error) {
	escapedPath := url.PathEscape(path)
	url := fmt.Sprintf("%s/%s/resolve/%s/%s", s.baseURL, s.repo, ref, escapedPath)
	return s.doGet(ctx, url)
	}

[ask-bonk]

The writeLayer function checks ctx.Err() when writing files (line 428), but not when writing directory headers. For consistency and correctness in highly concurrent scenarios, consider adding a context check before each directory header write:

Suggested change

	for _, dir := range collectDirs(files) {
	for _, dir := range collectDirs(files) {
	if err := ctx.Err(); err != nil {
	return err
	}
	if err := tw.WriteHeader(deterministicDirHeader(dir)); err != nil {
	return fmt.Errorf("write dir header %s: %w", dir, err)
	}
	}

[ask-bonk]

Posted a review on PR #2974 with 4 suggestions addressing security and correctness issues:

1. Path traversal validation in FileSource.Open - adds explicit validation to prevent directory escape
2. Size validation in PutFile - validates the size parameter instead of ignoring it
3. URL path escaping in HuggingFace source - escapes path components when constructing URLs
4. Context cancellation in writeLayer - adds consistent context checking for directory headers

The review is available at: #2974 (review)

github run

[ask-bonk]

I'm Bonk, and I've done a quick review of your PR. This PR introduces managed weights functionality for Cog, implementing a weight import/push/pull system using OCI artifacts.

I've identified 1 actionable issue that should be addressed:

[ask-bonk]

URLs are constructed using path.Join which cleans paths but does not URL-escape special characters. The filePath segment (passed through from Open) could contain characters like #, %, or spaces that need escaping in URL paths. While url.URL.String() does some escaping, it doesn't handle all edge cases properly.

Consider using url.PathEscape on the filePath segment:

Suggested change

	u.Path = path.Join(u.Path, path.Join(segments...))
	func (s *HFSource) buildURLWithQuery(query string, segments ...string) string {
	u := *s.baseURL // shallow copy
	// Escape the last segment (filePath) which may contain special chars
	if len(segments) > 0 {
	segments[len(segments)-1] = url.PathEscape(segments[len(segments)-1])
	}
	u.Path = path.Join(u.Path, path.Join(segments...))
	u.RawQuery = query
	return u.String()
	}

[ask-bonk]

I've completed the review of PR #2974.

Summary: This PR introduces managed weights functionality for Cog, implementing a weight import/push/pull system using OCI artifacts.

Issues Found: 1 actionable issue

From the 4 issues identified in previous reviews:

• ✅ Path traversal validation - Secure by design (uses fs.FS which prevents traversal)
• ✅ Size validation in PutFile - Fixed (lines 157-159 in file.go)
• ✅ Context cancellation in writeLayer - Fixed
• ⚠️ URL path escaping in HuggingFace - Still needs attention

I posted a review with a suggestion for the URL path escaping issue in pkg/model/weightsource/huggingface.go:398. The filePath parameter passed through from Open() may contain special characters (like #, %, spaces) that need URL escaping, but path.Join only cleans paths without escaping them.

Review URL: #2974 (review)

github run

[ask-bonk]

All tests pass and all issues from previous reviews have been addressed:

1. Path traversal validation - Fixed using os.DirFS
2. Size validation - Fixed in PutFile
3. Context cancellation - Fixed in writeLayer
4. URL path escaping - Working correctly (verified by TestHFSource_Open_EscapesPathComponents)

The new drift detection feature is well-tested and cleanly implemented.

LGTM

github run

[ask-bonk]

All previously identified issues have been verified as fixed:

1. Path traversal ✅ - Uses os.DirFS at line 56
2. Size validation ✅ - Validated at lines 157-159
3. Context cancellation ✅ - Checked at lines 422, 431
4. URL path escaping ✅ - Test passes

All tests pass and no linting issues.

LGTM

github run

[ask-bonk]

All issues from previous reviews have been verified as fixed:

1. Path traversal validation ✅ - Uses os.DirFS(path) at line 56 in file.go
2. Size validation ✅ - Validated at lines 157-159 in file.go
3. Context cancellation ✅ - Checked at lines 421-424 and 431 in packer.go
4. URL path escaping ✅ - Working correctly (verified by passing test TestHFSource_Open_EscapesPathComponents)

All tests pass successfully.

LGTM

github run

[ask-bonk]

All previously identified issues have been verified as fixed:

1. Path traversal validation ✅ - Uses os.DirFS(path) at line 56 in pkg/model/weightsource/file.go
2. Size validation ✅ - Validated at lines 157-159 in pkg/weights/store/file.go
3. Context cancellation ✅ - Checked at lines 421-424 and 431 in pkg/model/packer.go
4. URL path escaping ✅ - Working correctly (verified by passing test TestHFSource_Open_EscapesPathComponents)

All tests pass and no linting issues detected.

LGTM

github run

[ask-bonk]

LGTM

github run