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")