config set echoes secrets and keeps a loose file mode; a storage_url carrying credentials is accepted and echoed #166

New Issue

2026-09-22T00:55:14+02:00

clawbot commented

2026-09-22 00:55:14 +02:00

Found by the security review #73. Severity: low (hardening; the same secrets are already in the config file on that host, so the added exposure is captured output, logs and pasted vaultik info).

What is wrong

config set prints key = value for every key after writing (internal/cli/config.go:407), so setting s3.secret_access_key or age_secret_key this way prints the secret into any captured stdout.
config set leaves a group- or world-readable config as it is. The file always exists at that point and os.WriteFile does not change the mode of an existing file, so the stat-and-mode block at config.go:395-400 has no effect. The warning about loose modes lives in config.Load (internal/config/config.go:265-276), which config set never calls.
ParseStorageURL (internal/storage/url.go:63-111) ignores the userinfo part and any query parameter other than endpoint, region and ssl. If credentials sit in the URL as well as in s3.*, no error is raised and the raw URL is logged at INFO and stored in the local index on first bind (internal/vaultik/storage_bind.go:63-69), printed by vaultik info (internal/vaultik/info.go:52) and repeated in the binding-mismatch error (storage_bind.go:90-91). Silently ignored parameters are also a correctness trap: a misspelt endpoint= sends the backup to the default AWS endpoint.

Acceptable

config set prints only the key name after a successful write.
When the existing file has any of mode bits 0044 set, config set either calls os.Chmod(path, 0o600) after the write or prints the same warning config.Load prints. The ineffective mode block is deleted. Passing a different mode to os.WriteFile is not a fix.
ParseStorageURL returns an error, without repeating the URL, when an s3:// or rclone:// URL has a userinfo part or a query parameter it does not know (rclone:// knows none); the message names s3.access_key_id and s3.secret_access_key. When url.Parse fails, only the inner cause is wrapped, not the whole *url.Error, so the raw URL is not echoed. file:// is left alone.

Definition of done

Tests: config set of a secret key does not print the value; a pre-existing 0644 config is tightened or warned about; both rejected URL forms, and the url.Parse failure text, in a new internal/storage/url_test.go.
No existing assertion weakened; make check green.

Line numbers are as of next at 6fcd8e1.

model: fable-5-1

Found by the security review https://git.eeqj.de/sneak/vaultik/issues/73. Severity: **low** (hardening; the same secrets are already in the config file on that host, so the added exposure is captured output, logs and pasted `vaultik info`). ## What is wrong 1. `config set` prints `key = value` for every key after writing (`internal/cli/config.go:407`), so setting `s3.secret_access_key` or `age_secret_key` this way prints the secret into any captured stdout. 2. `config set` leaves a group- or world-readable config as it is. The file always exists at that point and `os.WriteFile` does not change the mode of an existing file, so the stat-and-mode block at `config.go:395-400` has no effect. The warning about loose modes lives in `config.Load` (`internal/config/config.go:265-276`), which `config set` never calls. 3. `ParseStorageURL` (`internal/storage/url.go:63-111`) ignores the userinfo part and any query parameter other than `endpoint`, `region` and `ssl`. If credentials sit in the URL as well as in `s3.*`, no error is raised and the raw URL is logged at INFO and stored in the local index on first bind (`internal/vaultik/storage_bind.go:63-69`), printed by `vaultik info` (`internal/vaultik/info.go:52`) and repeated in the binding-mismatch error (`storage_bind.go:90-91`). Silently ignored parameters are also a correctness trap: a misspelt `endpoint=` sends the backup to the default AWS endpoint. ## Acceptable - `config set` prints only the key name after a successful write. - When the existing file has any of mode bits 0044 set, `config set` either calls `os.Chmod(path, 0o600)` after the write or prints the same warning `config.Load` prints. The ineffective mode block is deleted. Passing a different mode to `os.WriteFile` is not a fix. - `ParseStorageURL` returns an error, without repeating the URL, when an `s3://` or `rclone://` URL has a userinfo part or a query parameter it does not know (`rclone://` knows none); the message names `s3.access_key_id` and `s3.secret_access_key`. When `url.Parse` fails, only the inner cause is wrapped, not the whole `*url.Error`, so the raw URL is not echoed. `file://` is left alone. ## Definition of done 1. Tests: `config set` of a secret key does not print the value; a pre-existing 0644 config is tightened or warned about; both rejected URL forms, and the `url.Parse` failure text, in a new `internal/storage/url_test.go`. 2. No existing assertion weakened; `make check` green. Line numbers are as of `next` at `6fcd8e1`. model: fable-5-1

clawbot referenced this issue from a commit

2026-09-22 12:02:38 +02:00

Stop config set echoing secrets; reject credential-bearing storage URLs (closes #166)

clawbot referenced a pull request that will close this issue

2026-09-22 12:02:49 +02:00

Stop config set echoing secrets; reject credential-bearing storage URLs #184

clawbot commented

2026-09-22 12:03:11 +02:00

PR: #184

config set now prints only the key name (never the value), and tightens a pre-existing group- or world-readable config to 0600 after writing (the old mode-preserving block was dead code and is removed). ParseStorageURL rejects s3:// and rclone:// URLs carrying credentials in the userinfo or an unknown query parameter, naming s3.access_key_id and s3.secret_access_key; rclone:// accepts no parameters, so a misspelt one is caught. A url.Parse failure wraps only the inner cause, so the raw URL is not echoed. file:// is unchanged. Tests cover all of these; make check is green.

Minor deviation: the new URL tests went into the existing internal/storage/url_parse_test.go rather than a separate url_test.go.

Model: opus-4-8

PR: https://git.eeqj.de/sneak/vaultik/pulls/184 `config set` now prints only the key name (never the value), and tightens a pre-existing group- or world-readable config to 0600 after writing (the old mode-preserving block was dead code and is removed). `ParseStorageURL` rejects `s3://` and `rclone://` URLs carrying credentials in the userinfo or an unknown query parameter, naming `s3.access_key_id` and `s3.secret_access_key`; `rclone://` accepts no parameters, so a misspelt one is caught. A `url.Parse` failure wraps only the inner cause, so the raw URL is not echoed. `file://` is unchanged. Tests cover all of these; `make check` is green. Minor deviation: the new URL tests went into the existing `internal/storage/url_parse_test.go` rather than a separate `url_test.go`. Model: opus-4-8

clawbot closed this issue

2026-09-22 12:28:32 +02:00

clawbot referenced this issue from a commit

2026-09-22 12:28:34 +02:00

Stop config set echoing secrets; reject credential-bearing storage URLs (closes #166)

clawbot referenced this issue

2026-09-22 12:30:48 +02:00

next: accumulated work for the upcoming release #114

Sign in to join this conversation.