Repository navigation
fix: report malformed --chart-repos and --helm-repo-extra-args instead of panicking - #879
vignesh2027 wants to merge 2 commits into
Conversation
…d of panicking processCharts split every --chart-repos and --helm-repo-extra-args value on "=" and read the second element without checking that it exists. A value without a repo name, such as --chart-repos https://charts.bitnami.com/bitnami or --helm-repo-extra-args --force-update, made lint, install and lint-and-install crash with "index out of range [1] with length 1". Use strings.Cut and return an error that names the bad value and the expected format. Signed-off-by: vigneshwar <applemacbook6sep2004@gmail.com>
cpanato
left a comment
cpanato
left a comment
There was a problem hiding this comment.
This fixes a real panic, and the valid-input parsing preserves the previous behavior. Local go test ./... passes, and the regression test reproduces the panic against the original implementation. Please address the credential disclosure in the new error message before merging.
| repoArgs[name] = repoExtraArgs | ||
| name, extraArgs, found := strings.Cut(repo, "=") | ||
| if !found { | ||
| return nil, fmt.Errorf("invalid helm repo extra args %q: expected format 'name=args'", repo) |
There was a problem hiding this comment.
[P2] Avoid echoing the full malformed repository arguments here. These arguments can contain credentials: for example, --helm-repo-extra-args='--username demo --password secret' (missing the required name= prefix) now prints the password in the normal CLI error, even with --debug and --print-config disabled. I reproduced this using a placeholder password. Previously this input panicked without printing the argument value. Please report the expected name=args format without including the raw value, and add a regression test that verifies a password is absent from the error.
There was a problem hiding this comment.
Good catch, thanks. In ad4c0ee the errors no longer include the value. They name the flag and the 1-based position of the entry, for example invalid --helm-repo-extra-args entry 1: expected format 'name=args'. I did the same for --chart-repos, since a URL like https://user:pass@host without a name= prefix has the same problem.
TestLintChartsInvalidRepoConfig now has two cases with a placeholder password, one in the extra args and one in a repo URL, and checks that it is absent from the error. Both fail on the previous commit.
The values of --chart-repos and --helm-repo-extra-args can carry credentials, so name the malformed entry by its position instead. Signed-off-by: vigneshwar <applemacbook6sep2004@gmail.com>
What this PR does / why we need it:
ct lint,ct installandct lint-and-installpanic when a--chart-reposor--helm-repo-extra-argsvalue has noname=prefix.processChartssplits each value on=and reads the second element without checking that there is one.To reproduce, run either of these against any chart:
Both print the list of charts to be processed and then crash with
panic: runtime error: index out of range [1] with length 1and a goroutine trace. The same happens when the value comes fromct.yamlor fromCT_CHART_REPOS.With this change the values are split with
strings.Cut, and a value without=returns a normal error that names it and shows the expected format:Valid values are parsed exactly as before. I added a table test to
chart_test.gothat panics on main and passes with the fix.Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged):Special notes for your reviewer:
This crash was reported in #248 for
--chart-reposand again in a later comment there for--helm-repo-extra-args. That issue was closed once the first reporter found their config mistake, so I have not used "fixes" here.