fix(config)!: stop loading the config file from the working directory - #1152
fix(config)!: stop loading the config file from the working directory#1152pbeckham wants to merge 1 commit into
Conversation
The --config-file default fell back to the bare name "kosli" whenever $HOME/.kosli.yml was absent, which viper resolves against the current working directory. A checkout could therefore ship kosli.yml and set any flag for a command run with a real KOSLI_API_TOKEN: host or http-proxy sends the bearer token to a host of the repository's choosing, and the snapshot k8s kubeconfig flag runs a kubeconfig exec credential plugin, which --dry-run does not prevent. The default is now always the home config file, whether or not it exists. A config file in the working directory is loaded only when the user names it with --config-file or KOSLI_CONFIG_FILE. When no home directory can be resolved there is no default config file at all, rather than a bare name that reopens the working-directory search, and `kosli config` says so instead of writing a config file into the working directory. A warning names an ignored working-directory config so the change is not silent. It fires only for a file that sets org, api-token or host: a kosli.yml in a repository root is far more often a flow template, which was never loaded as CLI config. Refs kosli-dev/server#6778, kosli-dev/server#6779 BREAKING CHANGE: a kosli.{yaml,yml,json,toml} file in the current working directory is no longer loaded automatically. Pass --config-file, set KOSLI_CONFIG_FILE, or move the settings to $HOME/.kosli.yml with `kosli config`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @pbeckham's task in 6m 57s —— View job Review: stop loading the Kosli config file from the working directory
Static analysis
VerdictThe core fix is right and the reasoning in the code comments is unusually good. No blocking issues. The findings are all about the warning — the compatibility net, not the fix itself. Since the warning is the only thing standing between this breaking change and a silently broken pipeline, its coverage matters more than usual. Findings
Worth calling out
Only finding 1 seems worth resolving before merge — the others are safe to fold in or defer. |
pbeckham
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // workingDirConfigNames are the config file names the CLI used to load | ||
| // implicitly from the current working directory, before that became | ||
| // kosli-dev/server#6778 and #6779. | ||
| var workingDirConfigNames = []string{"kosli.yaml", "kosli.yml", "kosli.json", "kosli.toml"} |
There was a problem hiding this comment.
The warning covers 4 of the 12 names that used to be loaded, and orders them differently from viper.
The old code path was v.SetConfigName("kosli") + v.AddConfigPath("."), so viper searched kosli.<ext> for every entry in viper.SupportedExts — in viper 1.21 that is json, toml, yaml, yml, properties, props, prop, hcl, tfvars, dotenv, env, ini — and returned the first match in that order.
Two consequences:
- A pipeline relying on
kosli.properties,kosli.env,kosli.ini,kosli.hclorkosli.tfvarsloses its config with no warning at all — precisely the silent breakage this function exists to prevent. - If both
kosli.jsonandkosli.ymlexist, the loop reportskosli.ymlfirst, but the file viper actually loaded before waskosli.json. A user who follows the advice (--config-file kosli.yml) gets a different config than they had, still silently.
Deriving the list from viper keeps both the coverage and the precedence honest, and it can't drift when viper adds a format:
| // workingDirConfigNames are the config file names the CLI used to load | |
| // implicitly from the current working directory, before that became | |
| // kosli-dev/server#6778 and #6779. | |
| var workingDirConfigNames = []string{"kosli.yaml", "kosli.yml", "kosli.json", "kosli.toml"} | |
| // workingDirConfigNames are the config file names the CLI used to load | |
| // implicitly from the current working directory, before that became | |
| // kosli-dev/server#6778 and #6779. viper searched a "kosli" config name in | |
| // every extension it supports and took the first match, so the list is | |
| // derived from viper in viper's own order. | |
| var workingDirConfigNames = func() []string { | |
| names := make([]string, 0, len(viper.SupportedExts)) | |
| for _, ext := range viper.SupportedExts { | |
| names = append(names, "kosli."+ext) | |
| } | |
| return names | |
| }() |
TestWorkingDirConfigIsNotLoaded would then be worth extending with one non-YAML/JSON case (kosli.env or kosli.properties) to pin the wider set.
| if !v.IsSet("org") && !v.IsSet("api-token") && !v.IsSet("host") { | ||
| continue | ||
| } |
There was a problem hiding this comment.
The key set omits the two settings the advisories were actually about.
http-proxy and kubeconfig are the vectors named in the PR description (#6778, #6779), yet a working-directory config that sets only those — or only max-api-retries — is now ignored and silent. Someone whose kosli.yml held http-proxy for a corporate proxy loses it with no signal, which is the exact failure mode the warning was added to avoid.
The flow-template exclusion still holds if the added keys are ones no template uses. Note that trail, artifacts, template and version must stay out of the set for that reason — trail is both a CLI flag and a template key, so adding it would break the flow template shape case in TestWarnsAboutIgnoredWorkingDirConfig.
| if !v.IsSet("org") && !v.IsSet("api-token") && !v.IsSet("host") { | |
| continue | |
| } | |
| // http-proxy, kubeconfig and max-api-retries are global settings a | |
| // working-directory config plausibly held. trail, artifacts, template | |
| // and version stay out: they are flow template keys, and a template | |
| // was never loaded as CLI config, so it must not warn. | |
| if !v.IsSet("org") && !v.IsSet("api-token") && !v.IsSet("host") && | |
| !v.IsSet("http-proxy") && !v.IsSet("kubeconfig") && !v.IsSet("max-api-retries") { | |
| continue | |
| } |
| } | ||
| dir, file := filepath.Split(global.ConfigFile) | ||
| file = strings.TrimSuffix(file, filepath.Ext(file)) | ||
|
|
||
| // Set the base name of the config file, without the file extension. | ||
| v.SetConfigName(file) | ||
| if global.ConfigFile != "" { | ||
| dir, file := filepath.Split(global.ConfigFile) | ||
| file = strings.TrimSuffix(file, filepath.Ext(file)) | ||
|
|
||
| // Set as many paths as you like where viper should look for the | ||
| // config file. By default, we are looking in the current working directory. | ||
| if dir == "" { | ||
| dir = "." | ||
| } | ||
| v.AddConfigPath(dir) | ||
|
|
||
| // Attempt to read the config file, gracefully ignoring errors | ||
| // caused by a config file not being found. Return an error | ||
| // if we cannot parse the config file. | ||
| logger.Debug("processing config file [%s]", global.ConfigFile) | ||
| if err := v.ReadInConfig(); err != nil { | ||
| // It's okay if there isn't a config file | ||
| if _, ok := err.(viper.ConfigFileNotFoundError); !ok { | ||
| return fmt.Errorf("failed to parse config file [%s] : %v", global.ConfigFile, err) | ||
| } else { | ||
| logger.Debug("config file [%s] not found. Skipping.", global.ConfigFile) | ||
| // Set the base name of the config file, without the file extension. | ||
| v.SetConfigName(file) | ||
|
|
||
| // A relative path is resolved against the current working directory, | ||
| // which is what a user asks for by naming one. The default is absolute, | ||
| // so it never reaches that case. | ||
| if dir == "" { | ||
| dir = "." | ||
| } | ||
| v.AddConfigPath(dir) | ||
|
|
||
| // Attempt to read the config file, gracefully ignoring errors | ||
| // caused by a config file not being found. Return an error | ||
| // if we cannot parse the config file. | ||
| logger.Debug("processing config file [%s]", global.ConfigFile) | ||
| if err := v.ReadInConfig(); err != nil { | ||
| // It's okay if there isn't a config file | ||
| if _, ok := err.(viper.ConfigFileNotFoundError); !ok { | ||
| return fmt.Errorf("failed to parse config file [%s] : %v", global.ConfigFile, err) | ||
| } else { | ||
| logger.Debug("config file [%s] not found. Skipping.", global.ConfigFile) | ||
| } | ||
| } | ||
| } else { | ||
| logger.Debug("no default config file location could be determined. Skipping.") | ||
| } | ||
|
|
There was a problem hiding this comment.
Nothing pins that the primary path still works. The if global.ConfigFile != "" wrapper is the load path for $HOME/.kosli.yml, but every test in the new suite stubs the default at a path that does not exist, and RootCommandTestSuite only exercises an explicit --config-file. A future edit that broke reading the home config (e.g. inverting this condition) would leave the whole suite green.
Worth one more case in WorkingDirConfigTestSuite: stub the default at a temp file containing host: https://home.example, run version, assert global.Host. It reuses stubHomeConfig almost verbatim and closes the one gap in an otherwise well-targeted suite.
Two smaller notes on this block:
warnAboutIgnoredWorkingDirConfigruns beforebindFlags, sologger.QuietEnabledstill reflects only the--quietcommand-line flag.KOSLI_QUIET=true(orquietin the home config) therefore does not suppress this warning, while--quietdoes. Minor, but it makes--quietand its env var behave differently for one message.- With
DOCSset, the default is the literal$HOME/.kosli.yml,dirbecomes$HOME/, and viper'sabsPathifyexpands it — so docs generation now reads the real home config where it previously searched./kosli.*. Harmless for docgen output, but it is a behaviour change that no test covers.
| // An empty path means no home directory could be resolved. Continuing would | ||
| // write the config into the current working directory, which is never where | ||
| // the default config file belongs. | ||
| if path == "" { |
There was a problem hiding this comment.
Good guard — this is the one place that would otherwise have written a .kosli.yml into whatever directory the user happened to be in.
Two nits: the format string has no arguments, so errors.New says the same thing without the fmt.Errorf indirection (errors is already imported in this package's sibling files, though not here — fmt is fine if you'd rather not add the import). And since config already rejects --config-file in its PreRunE, "use --config-file on each command instead" reads for a moment as advice for this command; "…or pass --config-file to the commands you run" removes the ambiguity.
| func TestWorkingDirConfigTestSuite(t *testing.T) { | ||
| suite.Run(t, new(WorkingDirConfigTestSuite)) | ||
| } | ||
|
|
||
| // TestConfigCommandFailsWithoutHomeDirectory pins the other side of an empty | ||
| // default config path: `kosli config` must say so rather than silently writing | ||
| // a config file into the current working directory. | ||
| func (suite *WorkingDirConfigTestSuite) TestConfigCommandFailsWithoutHomeDirectory() { | ||
| mockConfigGetter := new(MockConfigGetter) | ||
| mockConfigGetter.Mock.On("defaultConfigFilePath").Return("") | ||
| defaultConfigFilePathFunc = mockConfigGetter.defaultConfigFilePath | ||
| suite.T().Chdir(suite.T().TempDir()) | ||
|
|
||
| _, _, _, _, err := executeCommandC("config --org some-org") | ||
|
|
||
| suite.Require().Error(err) | ||
| suite.Contains(err.Error(), "Could not determine your home directory") | ||
| _, statErr := os.Stat(defaultConfigFilename) | ||
| suite.Require().Error(statErr, "no config file may be written into the working directory") | ||
| } |
There was a problem hiding this comment.
TestConfigCommandFailsWithoutHomeDirectory sits below the suite.Run entry point, so the file reads as if the suite ended at line 157. It still runs, but move it above TestWorkingDirConfigTestSuite to match the rest of the suite and the convention in the other test files here.
Also note this test parses an untrusted, repository-controlled file on every command run in that directory (warnAboutIgnoredWorkingDirConfig → v.ReadInConfig()). That is a deliberate and much smaller surface than applying the values, and the unparsable file case pins that a parse error stays quiet — but it does mean an arbitrarily large or pathological kosli.yml in a checkout is still fed to a parser. A cheap os.Stat size ceiling (skip files over, say, 1 MB) before ReadInConfig would close that without changing any behaviour a real config file has.
1 participant
Closes kosli-dev/server#6778 and kosli-dev/server#6779.
The
--config-filedefault fell back to the bare namekosliwhenever$HOME/.kosli.ymlwas absent, and viper resolves that against the current working directory. A checkout could therefore shipkosli.ymland set any Kosli flag for a command run with a real API token. The default is now always the home config file, and a working-directory config is loaded only when the user names it.Why
Two confirmed vulnerabilities, both reachable by a developer or CI job running an ordinary Kosli command inside an untrusted checkout with
KOSLI_API_TOKENin the environment. A repository-controlledhostorhttp-proxysendsAuthorization: Bearer <real token>to a host of the repository's choosing. A repository-controlledkubeconfigonsnapshot k8sruns a kubeconfigexeccredential plugin, which is arbitrary command execution, and--dry-rundoes not prevent it. Exposure is widest where tokens live: a CI runner has no$HOME/.kosli.yml, the state that triggered the fallback.What changed
getConfigFileFlagDefaultno longer degrades to a relative name, so viper's search path is always the home directory.initializeskips the config read when no default path resolves, rather than reopening the working-directory search, andkosli configfails clearly instead of writing into the working directory.Behaviour is unchanged for
--config-file,KOSLI_CONFIG_FILE,$HOME/.kosli.ymlandKOSLI_*environment variables. So the removal does not break a pipeline silently, an ignored working-directory config now produces a warning naming both the file and the fix. It fires only for a file settingorg,api-tokenorhost, because a repository-rootkosli.ymlis far more often a flow template, which was never loaded as CLI config.Verification
New
WorkingDirConfigTestSuiteincmd/kosli/configWorkingDir_test.go.TestWorkingDirConfigIsNotLoadedpins that none of the four config extensions can sethostfrom the working directory, andTestWarnsAboutIgnoredWorkingDirConfigpins the warning text and that a flow-template shaped file stays silent. Two further tests pin that--config-fileandKOSLI_CONFIG_FILEstill load a named file, and one thatkosli configwrites nothing into the working directory.Both reported scenarios were reproduced against a build of
a5ecf036and re-run against this branch: the bearer token now stops at the default host, and the kubeconfig credential plugin does not execute. The Test workflow gates this PR withmake test_integration_fulland golangci-lint.Follow-up
faq/faq.mdin the docs repo documents the working-directory default and needs that sentence removed. The generatedclient_referencepages pick up the new--config-filedefault on their next regeneration.