fix: guard GetCredentialFunc against nil store and fix Ingest error cleanup#1115
Closed
TerryHowe wants to merge 5 commits into
Closed
fix: guard GetCredentialFunc against nil store and fix Ingest error cleanup#1115TerryHowe wants to merge 5 commits into
TerryHowe wants to merge 5 commits into
Conversation
Use named return assignment instead of explicit return values so the deferred os.Remove(path) receives the correct path rather than empty string when Chmod or io.Copy fails. Signed-off-by: Terry Howe <terrylhowe@gmail.com>
Signed-off-by: Terry Howe <terrylhowe@gmail.com>
A nil credentials.Store passed to GetCredentialFunc would cause a deferred panic inside the returned closure at authentication time, making it hard to diagnose. Add an early nil check that returns a no-op CredentialFunc yielding EmptyCredential when store is nil. Add TestGetCredentialFunc_NilStore to cover this case. Signed-off-by: Terry Howe <terrylhowe@gmail.com>
TerryHowe
requested review from
Wwwsylvia,
sabre1041 and
shizhMSFT
as code owners
March 9, 2026 21:13
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1115 +/- ##
==========================================
+ Coverage 83.05% 83.30% +0.24%
==========================================
Files 81 81
Lines 5684 5689 +5
==========================================
+ Hits 4721 4739 +18
+ Misses 595 581 -14
- Partials 368 369 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Use a package-level var to make the chmod call mockable, enabling a test that exercises the permission-failure branch and verifies temp file cleanup. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Terry Howe <terrylhowe@gmail.com>
TerryHowe
changed the title
This was referenced May 23, 2026
Member
Author
|
Splitting this into two focused PRs since the fixes are independent:
Closing in favor of those. |
TerryHowe
added a commit
that referenced
this pull request
May 24, 2026
## Summary A `nil` `credentials.Store` passed to `GetCredentialFunc` would cause a deferred panic inside the returned closure at authentication time, making it hard to diagnose. This PR adds an early nil check that returns a no-op `CredentialFunc` yielding `EmptyCredential` when `store` is `nil`, matching the documented semantics. Also fixes a small typo in the `buildRepositoryBlobMountURL` doc comment (missing space). Split out from #1115 — the other half (Ingest temp-file cleanup) is in a separate PR. ## Test plan - [x] Added `TestGetCredentialFunc_NilStore` to verify nil store returns `EmptyCredential` without error - [x] Unit tests pass (`make test`) --------- Signed-off-by: Terry Howe <terrylhowe@gmail.com>
TerryHowe
added a commit
that referenced
this pull request
Jun 22, 2026
## Summary `Ingest` in `registry/remote/internal/ioutil` creates a temp file and uses a deferred cleanup to remove it on error. The error paths for `Chmod` and `io.Copy` previously used bare `return "", err` returns, which bypassed the named return value `ingestErr` — so the deferred cleanup never triggered and temp files were leaked. The fix switches both error paths to assign `ingestErr` and use a plain `return`, ensuring the defer always sees the error and removes the temp file. A package-level `fileChmod` var is introduced so the chmod-failure branch can be exercised in tests. Split out from #1115 — the other half (GetCredentialFunc nil guard) is in a separate PR. ## Test plan - [x] Added comprehensive tests for `Ingest`: success, name prefix, invalid dir, reader error with cleanup, empty content, and chmod error with cleanup - [x] Unit tests pass (`make test`) --------- Signed-off-by: Terry Howe <terrylhowe@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
This PR fixes two bugs in the
registry/remotepackage and adds missing test coverage.GetCredentialFuncnil store panic (registry/remote/auth.go): WhenGetCredentialFuncwas called with anilstore, it would panic at runtime when the returned function tried to call methods on the nil store. The fix adds a nil guard that returns a function yieldingEmptyCredentialwith no error, matching the documented semantics.Ingesttemp file leak on error (registry/remote/internal/ioutil/ioutil.go): TheIngestfunction creates a temp file and uses a deferred cleanup to remove it on error. However, the error paths forChmodandio.Copypreviously used barereturn "", errreturns which bypass the named return valueingestErr, so the deferred cleanup never triggered and temp files were leaked. The fix switches both error paths to assigningestErrand use a plainreturn, ensuring the defer always sees the error and removes the temp file.Typo fix (
registry/remote/url.go): Fixed missing space in thebuildRepositoryBlobMountURLcomment (buildRepositoryBlobMountURLbuilds→buildRepositoryBlobMountURL builds).Test plan
TestGetCredentialFunc_NilStoreto verify nil store returnsEmptyCredentialwithout errorIngest: success, name prefix, invalid dir, reader error with cleanup, empty content, and chmod error with cleanupmake test)