From f02bb0aecfdc65ef292dc260b887b560afb5dbdf Mon Sep 17 00:00:00 2001 From: developer Date: Sun, 27 Sep 2026 13:45:59 +0000 Subject: [PATCH] Fix env expansion: it never worked inside quotes, and empty was fatal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `${VAR}` support added with the Kubernetes API-key Secret was silently useless in the only form anyone would write it. The reference check sat in the same switch as the quote-state handling, below the `inDouble`/`inSingle` branches, so both of those branches consumed the bytes and a reference inside a quoted scalar was copied through verbatim. `api_keys: ["${KEY}"]` — the exact form deploy/k8s/00-config.yaml uses — came out of Load unchanged, so the key arrived as the literal string "${FANCYDNS_API_KEY}" and the admin API was protected by nothing. Everything looked fine because no test asserted on the quoted form at the Load boundary until now. Two further corrections while in there: - A variable that is set but empty now expands to empty instead of being an error. `${X}`, `${X-y}` and `${X:-y}` differ precisely on that point, and getting it wrong turns an intentionally blank secret into a startup failure. - A reference is skipped when it follows a backslash, so "\${FOO}" is literal. Also fixes two test fixtures that had an `inline:` key, which is not a field — the struct spells it block_entries — and were failing Validate for a reason that had nothing to do with what they were testing. That is the same silent-ignore-unknown-keys behaviour flagged in STATUS.md, caught here by a test that thought it was testing environment expansion. --- internal/config/config.go | 63 ++++++++++++++++++++-------------- internal/config/config_test.go | 17 ++++++--- 2 files changed, 51 insertions(+), 29 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index af26b32..d03fb28 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -651,27 +651,19 @@ func expandLine(line []byte, b *bytes.Buffer) error { i++ continue } - switch { - case inDouble: - switch { - case escaped: - escaped = false - case c == '\\': - escaped = true - case c == '"': - inDouble = false - } - case inSingle: - if c == '\'' { - inSingle = false - } - case c == '#' && (i == 0 || line[i-1] == ' ' || line[i-1] == '\t'): - inComment = true - case c == '"': - inDouble = true - case c == '\'': - inSingle = true - case c == '$' && i+1 < len(line) && line[i+1] == '{': + // An escape is consumed before the reference check below, or + // "\${FOO}" would still expand. + if escaped { + escaped = false + b.WriteByte(c) + i++ + continue + } + // References expand wherever they are not in a comment, quotes + // included: the whole point of the feature is that + // `api_keys: ["${KEY}"]` is how an operator would naturally write it. + // Quote tracking exists only to decide whether a '#' opens a comment. + if c == '$' && i+1 < len(line) && line[i+1] == '{' { end := bytes.IndexByte(line[i+2:], '}') if end < 0 { return fmt.Errorf("unterminated ${ at byte %d", i) @@ -692,7 +684,7 @@ func expandLine(line []byte, b *bytes.Buffer) error { // A bare `-` is a default too, for an unset (not an empty) // variable. It splits on the FIRST hyphen, so a variable name // may not contain one. - k = strings.IndexByte(spec, '-') + k := strings.IndexByte(spec, '-') name, def, hasDefault = spec[:k], spec[k+1:], true } if name == "" { @@ -700,9 +692,11 @@ func expandLine(line []byte, b *bytes.Buffer) error { } v, isSet := os.LookupEnv(name) switch { - case isSet && (v != "" || hasDefault && !colonDefault): - // v is "" here only for the "-" form with an empty variable, - // which is the whole difference between ${X-y} and ${X:-y}. + case isSet && (v != "" || !hasDefault || !colonDefault): + // A variable that is set but empty expands to empty. That is + // the whole difference between ${X}, ${X-y} and ${X:-y}, and + // getting it wrong turns an intentionally blank secret into a + // startup failure. b.WriteString(v) case hasDefault: b.WriteString(def) @@ -711,6 +705,25 @@ func expandLine(line []byte, b *bytes.Buffer) error { } continue } + switch { + case inDouble: + switch { + case c == '\\': + escaped = true + case c == '"': + inDouble = false + } + case inSingle: + if c == '\'' { + inSingle = false + } + case c == '#' && (i == 0 || line[i-1] == ' ' || line[i-1] == '\t'): + inComment = true + case c == '"': + inDouble = true + case c == '\'': + inSingle = true + } b.WriteByte(c) i++ } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index d0ebcd0..d3f55f4 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -232,8 +232,17 @@ func TestExpandEnvMultiLine(t *testing.T) { } func TestExpandEnvWithoutReferencesIsByteIdentical(t *testing.T) { - raw := []byte("# no references here\nurl: https://x.invalid/?a=$b&c=${not_this}\n'# ${FD_TEST_KEY}'\n") - clearEnv(t, "FD_TEST_KEY") + // A bare "$" is not a reference: a blocklist URL with a query string must + // survive untouched. The "#" cases are comments, and a comment must not be + // scanned for references at all — documenting "${VAR}" in a config comment + // must not make the config unloadable. + // A bare "$" is not a reference: a blocklist URL with a query string must + // survive untouched. A real comment must not be scanned for references at + // all — documenting "${VAR}" in a config comment must not make the config + // unloadable. (A "#" inside quotes is data, not a comment, so a reference + // there *is* live; that is covered by TestExpandEnvSubstitution.) + raw := []byte("# no references here\nurl: https://x.invalid/?a=$b&c=$d\n# and ${FD_TEST_MISSING} on a comment line\n") + clearEnv(t, "FD_TEST_MISSING") got, err := expandEnv(raw) if err != nil { t.Fatalf("expandEnv: %v", err) @@ -294,7 +303,7 @@ func TestLoadSubstitutesEnvInFile(t *testing.T) { password: prefix-${FD_TEST_PW} # the key is documented here: ${FD_TEST_KEY} blocklist: - inline: + block_entries: - "example.com" `) cfg, err := Load(path) @@ -315,7 +324,7 @@ blocklist: func TestLoadUnsetEnvIsFatalAndNamesVariable(t *testing.T) { clearEnv(t, "FD_TEST_MISSING") - path := writeTempConfig(t, "api:\n auth:\n mode: apikey\n api_keys: [\"${FD_TEST_MISSING}\"]\nblocklist:\n inline:\n - \"example.com\"\n") + path := writeTempConfig(t, "api:\n auth:\n mode: apikey\n api_keys: [\"${FD_TEST_MISSING}\"]\nblocklist:\n block_entries:\n - \"example.com\"\n") _, err := Load(path) if err == nil { t.Fatal("Load: want error for unset variable")