Rteco 1539 cargo implementation phase 1 - #510
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…h env Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… fails Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… in build-info Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- close a non-nil response body when Aql returns a transport error (M1) - apply partial page results before surfacing a mid-batch error, since enrichment is best-effort (M3) - recompute the still-missing count from the build-info so it reflects unique crates and can never go negative across modules (M2) - add tests for the transport-error and partial-apply-on-error paths (M4) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The CLI layer (getCommandName) splits the sub-command out of the args into commandName, but runNativeCargo forwarded only the remaining args, so 'jf cargo build' invoked bare 'cargo' with no sub-command. Prepend the sub-command via cargoInvocationArgs before exec. Verified: 'jf cargo build' now runs 'cargo build'. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds native Cargo support for Artifactory repository setup, authenticated command execution, Cargo metadata collection, crate artifact mapping, build-info publication, and checksum enrichment. ChangesCargo integration
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (14)
artifactory/commands/cargo/repo_test.go (1)
36-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a case for the
GetRepositoryerror path.
resolveDeploymentReporeturns the input repo unchanged whenGetRepositoryfails.fakeRepoGetteralready supportserr, but no test sets it.🧪 Proposed test case
// virtual without default -> "" if got := resolveDeploymentRepo("virt", fakeRepoGetter{rclass: "virtual"}); got != "" { t.Errorf("virtual no-default: got %q, want empty", got) } + // lookup failure -> repo used as-is + if got := resolveDeploymentRepo("unknown", fakeRepoGetter{err: errors.New("404")}); got != "unknown" { + t.Errorf("lookup error: got %q, want unknown", got) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/cargo/repo_test.go` around lines 36 - 49, The TestResolveDeploymentRepo test should cover the GetRepository failure path by configuring fakeRepoGetter with a non-nil err and asserting resolveDeploymentRepo returns the original repository name unchanged.artifactory/commands/cargo/publish.go (3)
293-316: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCreate the services manager once and reuse it.
targetRepo,setBuildProperties, andenrichChecksumsAndFetcheach callartutils.CreateServiceManager. A single publish creates up to three managers, each with its own HTTP client and connection pool. Cache one instance onCargoCommandand reuse it.♻️ Proposed helper
// serviceManager returns a lazily created, cached services manager. func (c *CargoCommand) serviceManager() (artifactory.ArtifactoryServicesManager, error) { if c.servicesManager != nil { return c.servicesManager, nil } sm, err := artutils.CreateServiceManager(c.serverDetails, -1, 0, false) if err != nil { return nil, err } c.servicesManager = sm return sm, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/cargo/publish.go` around lines 293 - 316, Cache a single services manager on CargoCommand and reuse it across targetRepo, setBuildProperties, and enrichChecksumsAndFetch. Add a lazy serviceManager helper that returns the cached instance or creates and stores one, then replace each direct artutils.CreateServiceManager call with this helper while preserving existing error handling.
442-450: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the loop variable that shadows the
pathpackage.The file imports
pathand uses it indirOf. The loop variablepathshadows that import inside this block. Rename it toreqPathfor clarity and to satisfy shadow linters.♻️ Proposed change
for di := range bi.Modules[idx].Dependencies { - for _, path := range bi.Modules[idx].Dependencies[di].RequestedBy { - for ei := range path { - if path[ei] == oldId { - path[ei] = moduleName + for _, reqPath := range bi.Modules[idx].Dependencies[di].RequestedBy { + for ei := range reqPath { + if reqPath[ei] == oldId { + reqPath[ei] = moduleName } } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/cargo/publish.go` around lines 442 - 450, Rename the inner loop variable in the dependency update block to reqPath, and update its indexed accesses accordingly, so the imported path package remains unshadowed for uses such as dirOf.
41-59: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDrop a value flag that has no value.
When a value flag is the last argument, the function emits the flag alone.
cargo metadata --featuresthen fails with a missing-value error, which aborts build-info collection. Omit the flag instead.♻️ Proposed change
case metadataValueFlags[a]: - out = append(out, a) if i+1 < len(args) { - out = append(out, args[i+1]) + out = append(out, a, args[i+1]) i++ }Update the matching expectation in
artifactory/commands/cargo/publish_test.goLines 220-224.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/cargo/publish.go` around lines 41 - 59, Update metadataFlagsFromArgs so metadata value flags are appended only when a following argument exists; when such a flag is the final argument, omit it entirely. Adjust the corresponding expectation in the metadata flag test in publish_test.go to reflect the omitted flag.artifactory/commands/cargo/artifacts_test.go (1)
9-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a case for a crate filename without a version.
crateRepoPathhas a distinct branch when no hyphen is followed by a digit. That branch returns an empty version and uses the whole base as the name. No test covers it.🧪 Proposed test case
if path != "crates/my-crate/my-crate-0.2.0.crate" || name != "my-crate" || version != "0.2.0" { t.Errorf("hyphenated: got (%q,%q,%q)", path, name, version) } + // no version token + path, name, version = crateRepoPath("mycrate.crate") + if path != "crates/mycrate/mycrate.crate" || name != "mycrate" || version != "" { + t.Errorf("no version: got (%q,%q,%q)", path, name, version) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/cargo/artifacts_test.go` around lines 9 - 20, Add a test case in TestCrateRepoPath for a crate filename without a version, such as a base name with no hyphen followed by a digit. Assert that crateRepoPath returns an empty version, uses the full base name as the crate name, and constructs the corresponding crates/<name>/<filename> path.artifactory/commands/cargo/checksums_test.go (1)
229-316: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
extraNamespath ofenrichAndLookup.The tests only exercise
enrichMissingChecksums, which passesnilextras. The publish flow depends onenrichAndLookupfolding the published crate name into the same AQL and returning its checksum. Add a test that passes a non-emptyextraNamesand asserts both the query content and the returned map entry.🧪 Proposed test
func TestEnrichAndLookup_IncludesExtraNames(t *testing.T) { bi := &entities.BuildInfo{Modules: []entities.Module{ {Dependencies: []entities.Dependency{{Id: "serde-1.0.197.crate"}}}, }} resp := `{"results":[ {"name":"serde-1.0.197.crate","actual_sha1":"a","sha256":"b","actual_md5":"c"}, {"name":"mycrate-0.1.0.crate","actual_sha1":"d","sha256":"e","actual_md5":"f"}]}` fake := &fakeAql{responses: []string{resp}} byName, err := enrichAndLookup(bi, "cargo-local", fake, []string{"mycrate-0.1.0.crate"}) require.NoError(t, err) require.Len(t, fake.queries, 1) assert.Contains(t, fake.queries[0], "mycrate-0.1.0.crate") assert.Equal(t, "e", byName["mycrate-0.1.0.crate"].Sha256) assert.Equal(t, "b", bi.Modules[0].Dependencies[0].Sha256) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/cargo/checksums_test.go` around lines 229 - 316, Add a test for enrichAndLookup that supplies a non-empty extraNames list, verifies the extra crate name is included in the generated AQL query, and confirms its checksum is returned in the lookup map. Also assert that the dependency checksum from the same response is applied to BuildInfo, matching the existing fakeAql response pattern.artifactory/commands/cargo/publish_test.go (1)
51-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for repeated
-pselectors.
packageNamesFromArgsexists specifically to collect several-pvalues, andnewCollectorpasses the full list to the collector. Only the singularpackageNameFromArgsis tested here.🧪 Proposed test
func TestPackageNamesFromArgs(t *testing.T) { got := packageNamesFromArgs([]string{"build", "-p", "a", "--package=b", "-p=c"}) want := []string{"a", "b", "c"} if !reflect.DeepEqual(got, want) { t.Errorf("got %v, want %v", got, want) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/cargo/publish_test.go` around lines 51 - 67, Add a TestPackageNamesFromArgs test covering multiple package selectors in one argument list, including separated and equals forms such as -p, --package=b, and -p=c. Call packageNamesFromArgs and assert the collected names preserve input order and match []string{"a", "b", "c"}, using the appropriate deep-equality assertion.artifactory/commands/cargo/checksums.go (1)
154-163: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInternal review discussion is left in shipped code comments. Both comments name a reviewer and describe the history of the change instead of the current behavior. Rewrite them to describe what the code does.
artifactory/commands/cargo/checksums.go#L154-L163: remove the sentence about the previous two-AQL implementation and the reviewer name; keep the description of the single batched query and its return value.artifactory/commands/cargo/publish.go#L141-L148: remove "(Naveen: avoid the extra AQL query)" and keep the explanation that the published-crate name joins the same AQL batch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/cargo/checksums.go` around lines 154 - 163, Rewrite the comments for enrichAndLookup in artifactory/commands/cargo/checksums.go:154-163 to describe only the single batched AQL behavior and returned checksum map, removing implementation history and the reviewer reference. In artifactory/commands/cargo/publish.go:141-148, remove the “Naveen: avoid the extra AQL query” text while retaining the explanation that the published-crate name is included in the same AQL batch.artifactory/commands/cargo/setup.go (1)
113-116: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
global-credential-providersoverwrites any user list.
setNestedreplaces the whole value. If a user configuredglobal-credential-providers = ["cargo:libsecret", "cargo:token"], this run reduces it to["cargo:token"]and disables their keyring provider for all registries.Merge instead: keep the existing entries and append
cargo:tokenonly when it is absent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/cargo/setup.go` around lines 113 - 116, Update the cargo configuration setup around setNested’s registry global-credential-providers assignment to preserve existing user-configured providers, append cargo:token only if absent, and avoid overwriting the list. Keep the resulting configuration compatible with the existing setNested flow and retain all other registry settings unchanged.artifactory/commands/cargo/setup_test.go (1)
97-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the stale deploy-registry cleanup.
ConfigureNativeRegistrydeletes[registries.jfrog-local]from both files whendeployRepois empty. No test exercises that path. Add a case that runs with a deploy repo and then re-runs without one, and assert thatjfrog-localis absent fromconfig.tomlandcredentials.toml.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/cargo/setup_test.go` around lines 97 - 120, Extend TestConfigureNativeRegistry_WithDeployRepo to re-run ConfigureNativeRegistry with an empty deployRepo after the initial deploy-repository setup, then decode both config.toml and credentials.toml and assert that jfrogDeployRegistryName is absent from their registries maps while the existing resolution registry remains covered.artifactory/commands/cargo/login_test.go (1)
180-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
resolveAuthEnv.The helpers are tested individually, but
resolveAuthEnvholds the branch logic: config-discovered matches, the--registryfallback, and the conditionalCARGO_REGISTRY_GLOBAL_CREDENTIAL_PROVIDERSentry. Add cases for a matching registry, a non-matching registry with--registrysupplied, and an existing provider override in the environment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/cargo/login_test.go` around lines 180 - 195, Extend TestBuildAuthEnv or the relevant test suite with coverage for resolveAuthEnv, exercising a config-discovered registry match, a non-matching configured registry when --registry is supplied as fallback, and an existing CARGO_REGISTRY_GLOBAL_CREDENTIAL_PROVIDERS environment override. Assert each result includes the expected resolved authentication environment and preserves the conditional provider entry.artifactory/commands/setup/setup.go (2)
209-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSuccess message omits the deploy repository.
For Cargo the run can configure two repositories. The message reports only
sc.repoName, so the user does not learn which local repository was configured for publishing. Includesc.deployRepoNamewhen it is not empty.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/setup/setup.go` around lines 209 - 213, Update the success message construction around sc.repoName and sc.deployRepoName so Cargo configurations report both repositories: retain the existing repository text and append the deploy repository when sc.deployRepoName is non-empty. Preserve the current output for configurations without a deploy repository and for Docker or Podman.
217-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid matching
SelectRepositoryInteractivelyerrors by message text.
SelectRepositoryInteractivelyreturnslen(filteredRepos) == 0as the messageno repositories were found that match the following criteria: ...; changing that wording makes the Cargo fallback path fall through as an unrecoverable setup error. Query the repository list (or use an exported sentinel/typed error fromjfrog-cli-core) and branch on zero matches instead of onstrings.Contains(err.Error(), ...).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/setup/setup.go` around lines 217 - 221, Replace the message-based no-repository detection around the Cargo fallback with a structured zero-match check: query or reuse the filtered repository list, or use an exported sentinel/typed error from SelectRepositoryInteractively, and branch when no repositories match. Remove reliance on noMatchingRepositoriesErrSubstring and strings.Contains while preserving the manual repository-name prompt for zero matches and existing error handling for other failures.artifactory/commands/cargo/exec_test.go (1)
11-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that stdin is wired through.
The
GetCmddoc states thatcmd.Stdin = os.Stdinfixes interactivecargo login. No test covers it, so a future change can drop the line without failing the suite. Addif cmd.Stdin != os.Stdin { ... }.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/cargo/exec_test.go` around lines 11 - 52, Extend TestCargoRunConfigGetCmd to assert that GetCmd wires standard input through by checking cmd.Stdin equals os.Stdin, alongside the existing command configuration assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@artifactory/commands/cargo/command.go`:
- Around line 110-116: Update runNativeCargo so its log.Debug call does not
expose secret values contained in cargo arguments, including tokens passed to
cargo login and cargo owner --token; redact those known secret-bearing arguments
before logging while preserving the actual unredacted args passed to runCmd.
- Around line 17-25: Update applyEnv to avoid leaving credential values in the
parent process environment: have it track each variable’s prior state and return
a restoration closure that uses os.Unsetenv or os.Setenv as appropriate, then
defer that cleanup immediately after the build-info collection call. Prefer
passing the auth environment explicitly if the collector API supports it, and
preserve existing handling for non-credential variables.
In `@artifactory/commands/cargo/login.go`:
- Around line 70-81: The doc comment for needsRemoteAccess must reflect that
commandBucket maps both install and build into the "deps" bucket, so remote
access is enabled for install, build, and publish. Update the comment’s command
list and related wording while leaving the function logic unchanged.
- Around line 113-130: Update registryHostMatches to require both a
case-insensitive host match and an Artifactory URL path-prefix match, using the
parsed artifactoryURL path as the base path for the index URL. Preserve
sparse+/git+ stripping and reject invalid or hostless URLs, ensuring unrelated
paths on the same host do not match.
In `@artifactory/commands/cargo/publish.go`:
- Around line 484-527: Update parseCargoRegistries to traverse from workingDir
upward through its parent directories, reading both .cargo/config.toml and
legacy .cargo/config at each level, while preserving global $CARGO_HOME
configuration as the lowest-precedence source. Apply discovered project
configurations from ancestors to the current directory so deeper entries
override parent entries, and ensure both filenames use readRegistriesInto.
- Around line 347-358: Update the property construction around the props
assignment before SetProps to use the repository’s properties builder or
escaping mechanism for build.name, build.number, build.timestamp, and
build.project values. Ensure values containing semicolons or equals signs remain
single property values, while preserving the existing optional project inclusion
and SetProps call.
In `@artifactory/commands/cargo/setup.go`:
- Around line 162-189: Update mergeTomlFile so TOML encoding completes
successfully before replacing the existing file; avoid calling os.Create on the
target before toml.NewEncoder.Encode can fail. Encode into an in-memory buffer
or same-directory temporary file, then atomically write or rename the result
into place while preserving the existing error handling and file permissions.
- Around line 138-155: Update mergeTomlFile to accept an os.FileMode parameter
and create files with os.OpenFile using the requested permissions before
writing. Pass 0600 at the credentials.toml call site in the setup flow, remove
the post-write os.Chmod handling, and update the config.toml call site to pass
0644.
In `@artifactory/commands/setup/setup.go`:
- Around line 288-299: Update the local repository selection flow around
SelectRepositoryInteractively so users can decline the optional publishing
configuration even when matching repositories exist. Add an explicit skip choice
or a preceding confirmation prompt, and leave sc.deployRepoName unset when
skipped while preserving the existing no-matching-repositories handling.
In `@go.mod`:
- Around line 206-211: Remove both LOCAL-ONLY replace directives for
build-info-go and jfrog-cli-core/v2 from go.mod, then update their require
entries to the intended released versions so builds no longer depend on
unreleased pseudo-versions.
---
Nitpick comments:
In `@artifactory/commands/cargo/artifacts_test.go`:
- Around line 9-20: Add a test case in TestCrateRepoPath for a crate filename
without a version, such as a base name with no hyphen followed by a digit.
Assert that crateRepoPath returns an empty version, uses the full base name as
the crate name, and constructs the corresponding crates/<name>/<filename> path.
In `@artifactory/commands/cargo/checksums_test.go`:
- Around line 229-316: Add a test for enrichAndLookup that supplies a non-empty
extraNames list, verifies the extra crate name is included in the generated AQL
query, and confirms its checksum is returned in the lookup map. Also assert that
the dependency checksum from the same response is applied to BuildInfo, matching
the existing fakeAql response pattern.
In `@artifactory/commands/cargo/checksums.go`:
- Around line 154-163: Rewrite the comments for enrichAndLookup in
artifactory/commands/cargo/checksums.go:154-163 to describe only the single
batched AQL behavior and returned checksum map, removing implementation history
and the reviewer reference. In artifactory/commands/cargo/publish.go:141-148,
remove the “Naveen: avoid the extra AQL query” text while retaining the
explanation that the published-crate name is included in the same AQL batch.
In `@artifactory/commands/cargo/exec_test.go`:
- Around line 11-52: Extend TestCargoRunConfigGetCmd to assert that GetCmd wires
standard input through by checking cmd.Stdin equals os.Stdin, alongside the
existing command configuration assertions.
In `@artifactory/commands/cargo/login_test.go`:
- Around line 180-195: Extend TestBuildAuthEnv or the relevant test suite with
coverage for resolveAuthEnv, exercising a config-discovered registry match, a
non-matching configured registry when --registry is supplied as fallback, and an
existing CARGO_REGISTRY_GLOBAL_CREDENTIAL_PROVIDERS environment override. Assert
each result includes the expected resolved authentication environment and
preserves the conditional provider entry.
In `@artifactory/commands/cargo/publish_test.go`:
- Around line 51-67: Add a TestPackageNamesFromArgs test covering multiple
package selectors in one argument list, including separated and equals forms
such as -p, --package=b, and -p=c. Call packageNamesFromArgs and assert the
collected names preserve input order and match []string{"a", "b", "c"}, using
the appropriate deep-equality assertion.
In `@artifactory/commands/cargo/publish.go`:
- Around line 293-316: Cache a single services manager on CargoCommand and reuse
it across targetRepo, setBuildProperties, and enrichChecksumsAndFetch. Add a
lazy serviceManager helper that returns the cached instance or creates and
stores one, then replace each direct artutils.CreateServiceManager call with
this helper while preserving existing error handling.
- Around line 442-450: Rename the inner loop variable in the dependency update
block to reqPath, and update its indexed accesses accordingly, so the imported
path package remains unshadowed for uses such as dirOf.
- Around line 41-59: Update metadataFlagsFromArgs so metadata value flags are
appended only when a following argument exists; when such a flag is the final
argument, omit it entirely. Adjust the corresponding expectation in the metadata
flag test in publish_test.go to reflect the omitted flag.
In `@artifactory/commands/cargo/repo_test.go`:
- Around line 36-49: The TestResolveDeploymentRepo test should cover the
GetRepository failure path by configuring fakeRepoGetter with a non-nil err and
asserting resolveDeploymentRepo returns the original repository name unchanged.
In `@artifactory/commands/cargo/setup_test.go`:
- Around line 97-120: Extend TestConfigureNativeRegistry_WithDeployRepo to
re-run ConfigureNativeRegistry with an empty deployRepo after the initial
deploy-repository setup, then decode both config.toml and credentials.toml and
assert that jfrogDeployRegistryName is absent from their registries maps while
the existing resolution registry remains covered.
In `@artifactory/commands/cargo/setup.go`:
- Around line 113-116: Update the cargo configuration setup around setNested’s
registry global-credential-providers assignment to preserve existing
user-configured providers, append cargo:token only if absent, and avoid
overwriting the list. Keep the resulting configuration compatible with the
existing setNested flow and retain all other registry settings unchanged.
In `@artifactory/commands/setup/setup.go`:
- Around line 209-213: Update the success message construction around
sc.repoName and sc.deployRepoName so Cargo configurations report both
repositories: retain the existing repository text and append the deploy
repository when sc.deployRepoName is non-empty. Preserve the current output for
configurations without a deploy repository and for Docker or Podman.
- Around line 217-221: Replace the message-based no-repository detection around
the Cargo fallback with a structured zero-match check: query or reuse the
filtered repository list, or use an exported sentinel/typed error from
SelectRepositoryInteractively, and branch when no repositories match. Remove
reliance on noMatchingRepositoriesErrSubstring and strings.Contains while
preserving the manual repository-name prompt for zero matches and existing error
handling for other failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3850195d-753e-4def-bc65-3efc6707dfec
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
artifactory/commands/cargo/artifacts.goartifactory/commands/cargo/artifacts_test.goartifactory/commands/cargo/checksums.goartifactory/commands/cargo/checksums_test.goartifactory/commands/cargo/command.goartifactory/commands/cargo/command_test.goartifactory/commands/cargo/exec.goartifactory/commands/cargo/exec_test.goartifactory/commands/cargo/login.goartifactory/commands/cargo/login_test.goartifactory/commands/cargo/publish.goartifactory/commands/cargo/publish_test.goartifactory/commands/cargo/repo.goartifactory/commands/cargo/repo_test.goartifactory/commands/cargo/setup.goartifactory/commands/cargo/setup_test.goartifactory/commands/setup/setup.goartifactory/commands/setup/setup_test.gogo.mod
| func applyEnv(env []string) { | ||
| for _, kv := range env { | ||
| if i := strings.IndexByte(kv, '='); i > 0 { | ||
| if err := os.Setenv(kv[:i], kv[i+1:]); err != nil { | ||
| log.Debug("cargo: could not set env " + kv[:i] + ": " + err.Error()) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
applyEnv writes credentials into the parent process environment.
applyEnv calls os.Setenv for each entry, including CARGO_REGISTRIES_<NAME>_TOKEN. The value stays in the jf process environment after collection completes. Every later child process in the same run inherits the token, and any code that dumps the environment can expose it.
Restrict the scope: pass the auth variables to the build-info collector explicitly if its API accepts an environment, or restore the previous values with os.Unsetenv/os.Setenv in a defer right after collection.
🔒 Minimal containment
- applyEnv(extraEnv)
+ restore := applyEnv(extraEnv)
+ defer restore()applyEnv returns a closure that restores each key to its prior value.
Also applies to: 84-86
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 20-20: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first.
Context: log.Debug("cargo: could not set env " + kv[:i] + ": " + err.Error())
Note: [CWE-117] Improper Output Neutralization for Logs.
(log-injection-request-data-concat-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@artifactory/commands/cargo/command.go` around lines 17 - 25, Update applyEnv
to avoid leaving credential values in the parent process environment: have it
track each variable’s prior state and return a restoration closure that uses
os.Unsetenv or os.Setenv as appropriate, then defer that cleanup immediately
after the build-info collection call. Prefer passing the auth environment
explicitly if the collector API supports it, and preserve existing handling for
non-credential variables.
| func (c *CargoCommand) runNativeCargo(extraEnv []string) error { | ||
| cargoExe := "cargo" | ||
| args := cargoInvocationArgs(c.commandName, c.args) | ||
| cfg := &CargoRunConfig{Exe: cargoExe, Args: args, Dir: c.workingDir, ExtraEnv: extraEnv} | ||
| log.Debug(fmt.Sprintf("cargo: running '%s %v'", cargoExe, args)) | ||
| return runCmd(cfg) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
The debug log can print a secret.
log.Debug prints the full argument list for every cargo sub-command. cargo login <token> and cargo owner --token <token> pass secrets as arguments, so the token reaches the debug log verbatim. Redact known secret-bearing arguments before logging, or log only the sub-command name.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@artifactory/commands/cargo/command.go` around lines 110 - 116, Update
runNativeCargo so its log.Debug call does not expose secret values contained in
cargo arguments, including tokens passed to cargo login and cargo owner --token;
redact those known secret-bearing arguments before logging while preserving the
actual unredacted args passed to runCmd.
| // needsRemoteAccess reports whether jf should inject registry auth for the command. Only the two | ||
| // build-info-collecting commands (install, publish) are jf-integrated and get token injection so | ||
| // they can resolve/upload against Artifactory. All other commands are pass-throughs and rely on the | ||
| // user's cargo credentials (e.g. from `jf setup cargo`). | ||
| func needsRemoteAccess(cmd string) bool { | ||
| switch commandBucket(cmd) { | ||
| case "deps", "publish": | ||
| return true | ||
| default: | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The doc comment does not match the code.
The comment says "Only the two build-info-collecting commands (install, publish)". commandBucket maps build to deps, so needsRemoteAccess returns true for three commands: install, build, and publish. Update the comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@artifactory/commands/cargo/login.go` around lines 70 - 81, The doc comment
for needsRemoteAccess must reflect that commandBucket maps both install and
build into the "deps" bucket, so remote access is enabled for install, build,
and publish. Update the comment’s command list and related wording while leaving
the function logic unchanged.
| // registryHostMatches reports whether a cargo registry index URL points at the same | ||
| // host as the configured Artifactory server URL. Strips cargo's "sparse+"/"git+" prefixes. | ||
| func registryHostMatches(indexURL, artifactoryURL string) bool { | ||
| strip := func(s string) string { | ||
| s = strings.TrimPrefix(s, "sparse+") | ||
| s = strings.TrimPrefix(s, "git+") | ||
| return s | ||
| } | ||
| iu, err := url.Parse(strip(indexURL)) | ||
| if err != nil || iu.Host == "" { | ||
| return false | ||
| } | ||
| au, err := url.Parse(artifactoryURL) | ||
| if err != nil || au.Host == "" { | ||
| return false | ||
| } | ||
| return strings.EqualFold(iu.Host, au.Host) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Host-only matching sends the credential to unrelated registries.
registryHostMatches compares only the host. If a user hosts another Cargo index on the same host as the Artifactory server (a different path or a reverse-proxied service), that registry also receives the Artifactory token. Compare the URL path prefix of ArtifactoryUrl as well, so only indexes under the Artifactory base path match.
Also applies to: 149-154
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@artifactory/commands/cargo/login.go` around lines 113 - 130, Update
registryHostMatches to require both a case-insensitive host match and an
Artifactory URL path-prefix match, using the parsed artifactoryURL path as the
base path for the index URL. Preserve sparse+/git+ stripping and reject invalid
or hostless URLs, ensuring unrelated paths on the same host do not match.
| timestamp := strconv.FormatInt(time.Now().UnixMilli(), 10) | ||
| props := fmt.Sprintf("build.name=%s;build.number=%s;build.timestamp=%s", name, number, timestamp) | ||
| if c.buildConfiguration != nil { | ||
| if projectKey := c.buildConfiguration.GetProject(); projectKey != "" { | ||
| props += fmt.Sprintf(";build.project=%s", projectKey) | ||
| } | ||
| } | ||
|
|
||
| _, err = sm.SetProps(services.PropsParams{Reader: reader, Props: props, UseDebugLogs: true, IsRecursive: true}) | ||
| if err != nil { | ||
| return fmt.Errorf("set properties: %w", err) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find existing property-building helpers and how other commands build build.* props.
rg -nP --type=go -C3 'build\.name=|build\.timestamp=' -g '!**/cargo/**'
fd -t f -e go . --exec rg -nP 'func (NewProperties|ParseProperties|\(.*Properties\) (AddProperty|ToEncodedString))' {} \;Repository: jfrog/jfrog-cli-artifactory
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -e
echo "Repository files:"
git ls-files | sed -n '1,80p'
echo
echo "Locate publish.go:"
fd -t f 'publish\.go$' .
echo
echo "Inspect relevant lines:"
if [ -f artifactory/commands/cargo/publish.go ]; then
nl -ba artifactory/commands/cargo/publish.go | sed -n '320,370p'
fi
echo
echo "Find Build* helpers/usages in codebase:"
rg -n --type=go 'Build(Props|Properties|Info|Details)|PropsParams|ParseProperties|NewProperties|SetProps|build\.name=|build\.number=|build\.timestamp=' .Repository: jfrog/jfrog-cli-artifactory
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -e
echo "cargo publish lines:"
awk 'NR>=320 && NR<=370 {printf "%6d\t%s\n", NR, $0}' artifactory/commands/cargo/publish.go
echo
echo "cargo publish imports:"
awk 'NR>=1 && NR<=60 {printf "%6d\t%s\n", NR, $0}' artifactory/commands/cargo/publish.go
echo
echo "createBuildPropsForArtifact definition:"
awk 'NR>=280 && NR<=310 {printf "%6d\t%s\n", NR, $0}' artifactory/commands/cargo/publish.go
echo
echo "CreateBuildProperties helper:"
rg -n --type=go -C5 'func CreateBuildProperties|func BuildProps' artifactory -g '!**/cargo/**' | head -n 200
echo
echo "ParseProperties helpers/usages:"
rg -n --type=go 'func ParseProperties|ParseProperties\(' . | head -n 120
echo
echo "git diff stat:"
git diff --stat -- artifactory/commands/cargo/publish.go || trueRepository: jfrog/jfrog-cli-artifactory
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -u
echo "cargo publish imports:"
awk '/^import \(/,/^\)/{print}' artifactory/commands/cargo/publish.go
awk 'NR>=1 && NR<=60 {printf "%6d\t%s\n", NR, $0}' artifactory/commands/cargo/publish.go
echo
echo "cargo publish SetProps block:"
awk 'NR>=320 && NR<=370 {printf "%6d\t%s\n", NR, $0}' artifactory/commands/cargo/publish.go
echo
echo "cargo publish createBuildPropsForArtifact:"
awk 'NR>=280 && NR<=310 {printf "%6d\t%s\n", NR, $0}' artifactory/commands/cargo/publish.go
echo
echo "CreateBuildProperties helper definitions:"
rg -n --type=go -C4 'func CreateBuildProperties|func CreateBuildProps|BuildProps.*configuration|CreateBuildPropsFromConfiguration' artifactory | head -n 160
echo
echo "ParseProperties helpers/usages:"
rg -n --type=go 'func ParseProperties|func \*Properties ToEncodedString|func NewProperties|ParseProperties\(' . | head -n 140
echo
echo "Command flag definitions:"
rg -n --type=go -C4 'build-name|build-number|build\.name|build\.number' artifactory/commands/cargo artifactory/cli | head -n 220Repository: jfrog/jfrog-cli-artifactory
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -u
printf 'files:\n'
git ls-files artifactory/commands/cargo/publish.go artifactory/commands/cargo | sort | sed -n '1,80p'
printf '\nfile size:\n'
wc -l artifactory/commands/cargo/publish.go
printf '\nimports:\n'
sed -n '1,70p' artifactory/commands/cargo/publish.go
printf '\nSetProps block:\n'
sed -n '330,370p' artifactory/commands/cargo/publish.go
printf '\ncreate/props functions:\n'
sed -n '270,310p' artifactory/commands/cargo/publish.go
printf '\nhelper references:\n'
rg -n --type=go 'CreateBuildProperties|CreateBuildPropsFromConfiguration|ParseProperties|NewProperties|ToEncodedString' artifactory | head -n 120
printf '\nflags:\n'
rg -n --type=go -C3 'BuildName|BuildNumber|build-name|build-number' artifactory/commands/cargo/artifactory/cli | head -n 220Repository: jfrog/jfrog-cli-artifactory
Length of output: 157
Escape build property values before passing them to SetProps.
build.name, build.number, and build.project come from command flags/configuration. Values containing ; or = split or overwrite properties, so generate these with a properties builder that escapes values before calling sm.SetProps.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@artifactory/commands/cargo/publish.go` around lines 347 - 358, Update the
property construction around the props assignment before SetProps to use the
repository’s properties builder or escaping mechanism for build.name,
build.number, build.timestamp, and build.project values. Ensure values
containing semicolons or equals signs remain single property values, while
preserving the existing optional project inclusion and SetProps call.
| // parseCargoRegistries returns registry name -> index URL, merging cargo's config sources the way | ||
| // cargo resolves them: the user-global $CARGO_HOME/config.toml (default ~/.cargo/config.toml) — this | ||
| // is what `jf setup cargo` writes — as a base, overlaid by the project-local | ||
| // <workingDir>/.cargo/config.toml (project entries win). This lets `jf cargo` locate registries | ||
| // whether they came from `jf setup cargo` (global) or a project-committed .cargo/config.toml. | ||
| func parseCargoRegistries(workingDir string) map[string]string { | ||
| out := map[string]string{} | ||
| // Global (lowest precedence) — written by `jf setup cargo`. | ||
| if home, err := cargoHome(); err == nil && home != "" { | ||
| readRegistriesInto(filepath.Join(home, "config.toml"), out) | ||
| } | ||
| // Project-local (highest precedence) overlays the global entries. | ||
| readRegistriesInto(filepath.Join(workingDir, ".cargo", "config.toml"), out) | ||
| return out | ||
| } | ||
|
|
||
| // readRegistriesInto parses one cargo config.toml and merges its [registries.<name>] index URLs | ||
| // into out (existing keys are overwritten). Missing/invalid files are skipped (debug-logged). | ||
| func readRegistriesInto(configPath string, out map[string]string) { | ||
| data, err := os.ReadFile(configPath) | ||
| if err != nil { | ||
| log.Debug("cargo: could not read " + configPath + ": " + err.Error()) | ||
| return | ||
| } | ||
| var cfg cargoConfigToml | ||
| if err := toml.Unmarshal(data, &cfg); err != nil { | ||
| log.Debug("cargo: could not parse " + configPath + ": " + err.Error()) | ||
| return | ||
| } | ||
| for name, reg := range cfg.Registries { | ||
| if reg.Index != "" { | ||
| out[name] = reg.Index | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // cargoRegistryIndexURL reads <workingDir>/.cargo/config.toml and returns the | ||
| // index URL of [registries.<registryName>]. Returns "" on any error or if absent. | ||
| func cargoRegistryIndexURL(workingDir, registryName string) string { | ||
| if registryName == "" { | ||
| return "" | ||
| } | ||
| return parseCargoRegistries(workingDir)[registryName] | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repository files matching publish.go/config.toml related names:\n'
fd -a 'publish\.go$|config\.toml$|.*cargo.*\.go$' . | sed 's#^\./##' | head -200
printf '\nRelevant symbol references:\n'
rg -n "parseCargoRegistries|readRegistriesInto|cargoRegistryIndexURL|cargoHome|Registries|registries" -S .
printf '\nTarget file outline:\n'
ast-grep outline artifactory/commands/cargo/publish.go --match parseCargoRegistries --view expanded || true
printf '\nRelevant target file sections:\n'
sed -n '440,540p' artifactory/commands/cargo/publish.go
printf '\nCargo config-related implementation sections:\n'
rg -n -C 3 "func (parseCargoRegistries|readRegistriesInto|cargoRegistryIndexURL|cargoHome)" -S .Repository: jfrog/jfrog-cli-artifactory
Length of output: 15609
🌐 Web query:
Cargo documentation cargo config file paths .cargo/config.toml .cargo/config current directory parent directories
💡 Result:
Cargo searches for configuration files by traversing upward from the current working directory, including the current directory and all its parent directories [1][2]. The lookup process is as follows: 1. Cargo probes for files named.cargo/config.toml or.cargo/config in the current directory and every parent directory, continuing until it reaches the root directory [1][3]. 2. It then checks the CARGO_HOME directory ($HOME/.cargo/config.toml on Unix or %USERPROFILE%.cargo\config.toml on Windows) [1][2]. Important rules and behaviors: - Hierarchical Merging: Cargo unifies the configuration files found in these locations. Settings defined closer to the current working directory take precedence over those found in parent directories, while the global CARGO_HOME configuration has the lowest precedence [1][3]. - File Naming: Both.cargo/config.toml and.cargo/config are valid. If both exist in the same.cargo directory, Cargo will use the file without the.toml extension [1][2]. - Workspace Limitations: When Cargo is invoked from a workspace root, it does not automatically read configuration files located within individual workspace member crates [1][3]. - Probing Behavior: Because the search is based on the current working directory, invoking Cargo with --manifest-path or from a different directory than the project root can result in different configuration files being discovered [4][5]. For managing complex projects, the official recommendation is to keep extra configuration files at the same level as the discovered.cargo/config.toml within your project or to use a new directory at the same level, such as.config/ [1][2].
Citations:
- 1: https://doc.rust-lang.org/cargo/reference/config.html
- 2: https://doc.rust-lang.org/1.84.0/cargo/reference/config.html
- 3: https://dev-doc.rust-lang.org/cargo/reference/config.html
- 4: Cargo does not always search for .cargo/config file in project root rust-lang/cargo#2930
- 5: Cargo config search meta issue. rust-lang/cargo#9769
🌐 Web query:
site:doc.rust-lang.org/cargo/reference/config.html default-config "search" ".cargo/config.toml" ".cargo/config"
💡 Result:
Cargo searches for configuration files by traversing upward from the current working directory, as well as checking a global location [1]. Lookup Order and Merging Cargo probes for configuration files in the following order, merging them as it goes: 1. All.cargo/config.toml files in the current directory and every parent directory, starting from the current directory and moving toward the root [1]. 2. The global configuration file located at $CARGO_HOME/config.toml (which defaults to $HOME/.cargo/config.toml on Unix systems or %USERPROFILE%.cargo\config.toml on Windows) [1]. Precedence and Conflicts - For scalars (numbers, strings, booleans), configuration files found in deeper directories take precedence over those in ancestor directories, with the global home directory having the lowest priority [1]. - For arrays, values are joined together, with higher-precedence items placed later in the merged array [1]. File Naming Conventions Cargo supports both.cargo/config.toml and.cargo/config [1]. While.cargo/config.toml is the preferred extension introduced in version 1.39, Cargo will prioritize the older.cargo/config file if both exist at the same location [1].
Citations:
Walk parent Cargo configs and support the legacy config filename.
parseCargoRegistries only reads $CARGO_HOME/config.toml and <workingDir>/.cargo/config.toml. Cargo searches upward from the current directory for both .cargo/config.toml and .cargo/config, then reads global config, with deeper/local entries overriding ancestors/global entries. This lookup misses workspace-root configs when running from a member crate and any legacy .cargo/config, so registry indices can resolve to "" and disable repository resolution, checksum enrichment, and build properties. Add parent-directory traversal that reads both file names and applies highest-precedence local entries last.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 504-504: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first.
Context: log.Debug("cargo: could not read " + configPath + ": " + err.Error())
Note: [CWE-117] Improper Output Neutralization for Logs.
(log-injection-request-data-concat-go)
[warning] 509-509: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first.
Context: log.Debug("cargo: could not parse " + configPath + ": " + err.Error())
Note: [CWE-117] Improper Output Neutralization for Logs.
(log-injection-request-data-concat-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@artifactory/commands/cargo/publish.go` around lines 484 - 527, Update
parseCargoRegistries to traverse from workingDir upward through its parent
directories, reading both .cargo/config.toml and legacy .cargo/config at each
level, while preserving global $CARGO_HOME configuration as the
lowest-precedence source. Apply discovered project configurations from ancestors
to the current directory so deeper entries override parent entries, and ensure
both filenames use readRegistriesInto.
| credsPath := filepath.Join(home, "credentials.toml") | ||
| if err = mergeTomlFile(credsPath, func(m map[string]interface{}) { | ||
| for _, reg := range registries { | ||
| setNested(m, []string{"registries", reg, "token"}, credential) | ||
| } | ||
| if deployRepo == "" { | ||
| // Mirror the config.toml cleanup: drop any stale jfrog-local credentials so the two | ||
| // files stay in sync (a leftover token here would otherwise reference a registry | ||
| // that no longer exists in config.toml). | ||
| deleteNested(m, []string{"registries", jfrogDeployRegistryName}) | ||
| } | ||
| }); err != nil { | ||
| return fmt.Errorf("failed to write cargo credentials %q: %w", credsPath, err) | ||
| } | ||
| // credentials.toml holds a secret — restrict permissions (best-effort). | ||
| if err = os.Chmod(credsPath, 0600); err != nil { | ||
| log.Debug("cargo: could not chmod credentials.toml: " + err.Error()) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Create credentials.toml with 0600 instead of chmod after write.
mergeTomlFile uses os.Create, which creates the file with mode 0666 masked by umask. The token is written before os.Chmod runs, so the secret exists on disk with world-readable permissions during that window. If os.Chmod fails, the file stays readable and only a debug log is emitted.
Pass the wanted mode into mergeTomlFile and create the file with os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, perm).
🔒 Proposed fix
- if err = mergeTomlFile(credsPath, func(m map[string]interface{}) {
+ if err = mergeTomlFile(credsPath, 0600, func(m map[string]interface{}) {
for _, reg := range registries {
setNested(m, []string{"registries", reg, "token"}, credential)
}
@@
}); err != nil {
return fmt.Errorf("failed to write cargo credentials %q: %w", credsPath, err)
}
- // credentials.toml holds a secret — restrict permissions (best-effort).
- if err = os.Chmod(credsPath, 0600); err != nil {
- log.Debug("cargo: could not chmod credentials.toml: " + err.Error())
- }
+ // Existing files keep their previous mode, so still narrow them (best-effort).
+ if err = os.Chmod(credsPath, 0600); err != nil {
+ log.Debug("cargo: could not chmod credentials.toml: " + err.Error())
+ }Outside the selected range, update the helper:
func mergeTomlFile(path string, perm os.FileMode, apply func(map[string]interface{})) error {
...
f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, perm)
...
}The config.toml call site then passes 0644.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| credsPath := filepath.Join(home, "credentials.toml") | |
| if err = mergeTomlFile(credsPath, func(m map[string]interface{}) { | |
| for _, reg := range registries { | |
| setNested(m, []string{"registries", reg, "token"}, credential) | |
| } | |
| if deployRepo == "" { | |
| // Mirror the config.toml cleanup: drop any stale jfrog-local credentials so the two | |
| // files stay in sync (a leftover token here would otherwise reference a registry | |
| // that no longer exists in config.toml). | |
| deleteNested(m, []string{"registries", jfrogDeployRegistryName}) | |
| } | |
| }); err != nil { | |
| return fmt.Errorf("failed to write cargo credentials %q: %w", credsPath, err) | |
| } | |
| // credentials.toml holds a secret — restrict permissions (best-effort). | |
| if err = os.Chmod(credsPath, 0600); err != nil { | |
| log.Debug("cargo: could not chmod credentials.toml: " + err.Error()) | |
| } | |
| credsPath := filepath.Join(home, "credentials.toml") | |
| if err = mergeTomlFile(credsPath, 0600, func(m map[string]interface{}) { | |
| for _, reg := range registries { | |
| setNested(m, []string{"registries", reg, "token"}, credential) | |
| } | |
| if deployRepo == "" { | |
| // Mirror the config.toml cleanup: drop any stale jfrog-local credentials so the two | |
| // files stay in sync (a leftover token here would otherwise reference a registry | |
| // that no longer exists in config.toml). | |
| deleteNested(m, []string{"registries", jfrogDeployRegistryName}) | |
| } | |
| }); err != nil { | |
| return fmt.Errorf("failed to write cargo credentials %q: %w", credsPath, err) | |
| } | |
| // Existing files keep their previous mode, so still narrow them (best-effort). | |
| if err = os.Chmod(credsPath, 0600); err != nil { | |
| log.Debug("cargo: could not chmod credentials.toml: " + err.Error()) | |
| } |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 153-153: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first.
Context: log.Debug("cargo: could not chmod credentials.toml: " + err.Error())
Note: [CWE-117] Improper Output Neutralization for Logs.
(log-injection-request-data-concat-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@artifactory/commands/cargo/setup.go` around lines 138 - 155, Update
mergeTomlFile to accept an os.FileMode parameter and create files with
os.OpenFile using the requested permissions before writing. Pass 0600 at the
credentials.toml call site in the setup flow, remove the post-write os.Chmod
handling, and update the config.toml call site to pass 0644.
| func mergeTomlFile(path string, apply func(map[string]interface{})) error { | ||
| m := map[string]interface{}{} | ||
| data, err := os.ReadFile(path) | ||
| switch { | ||
| case err == nil && len(data) > 0: | ||
| if uerr := toml.Unmarshal(data, &m); uerr != nil { | ||
| return fmt.Errorf("parse existing TOML: %w", uerr) | ||
| } | ||
| case err != nil && !os.IsNotExist(err): | ||
| return err | ||
| } | ||
|
|
||
| apply(m) | ||
|
|
||
| if err = os.MkdirAll(filepath.Dir(path), 0755); err != nil { | ||
| return err | ||
| } | ||
| f, err := os.Create(path) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer func() { | ||
| if cerr := f.Close(); cerr != nil { | ||
| log.Debug("cargo: close " + path + ": " + cerr.Error()) | ||
| } | ||
| }() | ||
| return toml.NewEncoder(f).Encode(m) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Encode failure leaves a truncated config file.
os.Create truncates the target before the encode runs. If toml.NewEncoder(f).Encode(m) fails, the user keeps an empty or partial config.toml or credentials.toml, and their unrelated settings are lost.
Encode into a buffer first, then write the buffer, or write to a temp file in the same directory and rename it into place.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 184-184: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first.
Context: log.Debug("cargo: close " + path + ": " + cerr.Error())
Note: [CWE-117] Improper Output Neutralization for Logs.
(log-injection-request-data-concat-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@artifactory/commands/cargo/setup.go` around lines 162 - 189, Update
mergeTomlFile so TOML encoding completes successfully before replacing the
existing file; avoid calling os.Create on the target before
toml.NewEncoder.Encode can fail. Encode into an in-memory buffer or
same-directory temporary file, then atomically write or rename the result into
place while preserving the existing error handling and file permissions.
| local, err := utils.SelectRepositoryInteractively( | ||
| sc.serverDetails, | ||
| services.RepositoriesFilterParams{RepoType: utils.Local.String(), PackageType: packageType, ProjectKey: sc.projectKey}, | ||
| "Select a local repository for publishing crates (optional):") | ||
| if err != nil { | ||
| if !strings.Contains(err.Error(), noMatchingRepositoriesErrSubstring) { | ||
| return err | ||
| } | ||
| log.Info(fmt.Sprintf("No local %s repository was found; configuring resolution only (publishing not configured).", packageType)) | ||
| return nil | ||
| } | ||
| sc.deployRepoName = local |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The "optional" local repository cannot be declined.
The prompt text says the local repository is optional, but SelectRepositoryInteractively returns an error only when no repository matches. If one or more local Cargo repositories exist, the user must select one and cannot skip publishing configuration. Add an explicit skip entry, or ask a yes/no question before the selection.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@artifactory/commands/setup/setup.go` around lines 288 - 299, Update the local
repository selection flow around SelectRepositoryInteractively so users can
decline the optional publishing configuration even when matching repositories
exist. Add an explicit skip choice or a preceding confirmation prompt, and leave
sc.deployRepoName unset when skipped while preserving the existing
no-matching-repositories handling.
|
|
||
| // LOCAL-ONLY (uncommitted, REVERT before PR): use local build-info-go for cargo flexpack work | ||
| replace github.com/jfrog/build-info-go => github.com/jfrog/build-info-go v1.13.1-0.20260728061823-0ba531230559 | ||
|
|
||
| // LOCAL-ONLY (uncommitted, REVERT before PR): use local jfrog-cli-core for the project.Cargo enum (jf setup cargo) | ||
| replace github.com/jfrog/jfrog-cli-core/v2 => github.com/jfrog/jfrog-cli-core/v2 v2.60.1-0.20260717063801-dfcd55d10923 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'jfrog/(build-info-go|jfrog-cli-core)' go.mod
rg -nP 'jfrog/(build-info-go|jfrog-cli-core)' go.sum | head -20Repository: jfrog/jfrog-cli-artifactory
Length of output: 1185
Remove the LOCAL-ONLY replace directives before merge.
The manifest still contains the uncommitted local replace directives for github.com/jfrog/build-info-go and github.com/jfrog/jfrog-cli-core/v2. These override the declared require entries and make CI builds depend on unreleased pseudo-versions. Delete both directives and update the require versions to the intended releases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@go.mod` around lines 206 - 211, Remove both LOCAL-ONLY replace directives for
build-info-go and jfrog-cli-core/v2 from go.mod, then update their require
entries to the intended released versions so builds no longer depend on
unreleased pseudo-versions.
1 participant
Summary by CodeRabbit