Fix env expansion: it never worked inside quotes, and empty was fatal
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.
This commit is contained in:
1 parent
b0e6b3bb92
commit
f02bb0aecf
2 files changed
+51
-29
No files matched your search
+38
-25
@@ -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++
|
||||
}
|
||||
|
||||
@@ -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")
|
||||
|
||||
Reference in new issue
Block a user