valuesource: the env provider does not trim, so a trailing newline reaches the auth header and Go refuses the request #304

New issue

Open

opened 2026-08-17 23:22:32 +00:00 by coilyco-ops · 0 comments

coilyco-ops commented

2026-08-17 23:22:32 +00:00

Member

Found while debugging a live Discord MCP wrap that had stopped making outbound calls. The failure is silent in an unhelpful way, and the fix is one call.

The asymmetry

pkg/valuesource/valuesource.go, Builtins():

"env": func(_ context.Context, name string) (string, error) {
    v, ok := os.LookupEnv(name)
    if !ok {
        return "", fmt.Errorf("env var %q is not set", name)
    }
    return v, nil                              // no trim
},
"file": func(_ context.Context, path string) (string, error) {
    b, err := os.ReadFile(path)
    if err != nil {
        return "", err
    }
    return strings.TrimSpace(string(b)), nil   // trims
},

file trims because files carry trailing newlines. env does not, even though an env var is very often a file's contents one hop removed.

Why one hop removed is the common case

The path that produced this:

An operator stores a credential with aws ssm put-parameter --value file://..., which is the recommended form precisely because it keeps the value out of argv. The file has a trailing newline, because editors add one.
SSM stores the value byte for byte, newline included.
External Secrets syncs it into a Kubernetes Secret, unchanged.
The Secret is mounted as an env var via secretKeyRef, unchanged.
value env "TOKEN" in a guardfile resolves it, unchanged.

Every layer is faithful, which is correct behavior for all of them, and the newline arrives intact.

What it does downstream

http/opcore/runtime.go:149:

req.Header.Set(rt.Auth.Header, rt.Auth.Prefix+secret)

Go's transport validates header values on write, so the request never leaves the process:

token "cleantoken"          -> err: <nil>
token "tokenwithnewline\n"  -> err: net/http: invalid header field value for "Authorization"

This is the part that makes it expensive to diagnose. There is no 401 and no upstream log line, because the upstream is never contacted. From the operator's side a tool simply fails, and the natural first guess is a bad credential or a permissions problem rather than a whitespace problem. The credential is correct. It is also unprintable, so the usual instinct of echoing it to compare is both unsafe and unavailable.

Suggested fix

return strings.TrimSpace(v), nil in the env provider, matching file.

Worth considering for literal too, though the argument is weaker: a literal is author-supplied in the guardfile, so trailing whitespace there is more plausibly deliberate and more visible in review.

A credential with meaningful leading or trailing whitespace is not a real case for any of the three, and every consumer that has thought about it has landed on trimming independently. sirens-echo does strings.TrimSpace(os.Getenv("DISCORD_TOKEN")) at internal/community/config.go:903, which is exactly why the same malformed value worked fine through its gateway connection while failing through the guardfile.

Alternative worth weighing

Reject rather than trim: fail closed at resolve time with an error naming the variable and the offending character class, never the value. That surfaces the operator's mistake instead of silently correcting it, and a credential is the wrong place to be quietly lenient.

Trimming matches file's existing behavior and fixes every deployment already carrying the newline without an operator round trip. Rejecting is more honest but strands anyone whose stored value has one. Either beats the current state, where the value is neither trimmed nor refused. Filing with a recommendation of trim, for consistency with file, but the call is not mine.

Repro

os.Setenv("TOKEN", "abc\n")
v, _ := valuesource.Builtins()["env"](context.Background(), "TOKEN")
req, _ := http.NewRequest("GET", srv.URL, nil)
req.Header.Set("Authorization", "Bot "+v)
_, err := http.DefaultClient.Do(req)   // net/http: invalid header field value

Observed against umbra v0.154.0, the version mcp-beaver pins.

**Found while debugging a live Discord MCP wrap that had stopped making outbound calls.** The failure is silent in an unhelpful way, and the fix is one call. ## The asymmetry `pkg/valuesource/valuesource.go`, `Builtins()`: ```go "env": func(_ context.Context, name string) (string, error) { v, ok := os.LookupEnv(name) if !ok { return "", fmt.Errorf("env var %q is not set", name) } return v, nil // no trim }, "file": func(_ context.Context, path string) (string, error) { b, err := os.ReadFile(path) if err != nil { return "", err } return strings.TrimSpace(string(b)), nil // trims }, ``` `file` trims because files carry trailing newlines. `env` does not, even though an env var is very often a file's contents one hop removed. ## Why one hop removed is the common case The path that produced this: 1. An operator stores a credential with `aws ssm put-parameter --value file://...`, which is the recommended form precisely because it keeps the value out of argv. The file has a trailing newline, because editors add one. 2. SSM stores the value byte for byte, newline included. 3. External Secrets syncs it into a Kubernetes Secret, unchanged. 4. The Secret is mounted as an env var via `secretKeyRef`, unchanged. 5. `value env "TOKEN"` in a guardfile resolves it, unchanged. Every layer is faithful, which is correct behavior for all of them, and the newline arrives intact. ## What it does downstream `http/opcore/runtime.go:149`: ```go req.Header.Set(rt.Auth.Header, rt.Auth.Prefix+secret) ``` Go's transport validates header values on write, so the request never leaves the process: ``` token "cleantoken" -> err: <nil> token "tokenwithnewline\n" -> err: net/http: invalid header field value for "Authorization" ``` **This is the part that makes it expensive to diagnose.** There is no 401 and no upstream log line, because the upstream is never contacted. From the operator's side a tool simply fails, and the natural first guess is a bad credential or a permissions problem rather than a whitespace problem. The credential is correct. It is also unprintable, so the usual instinct of echoing it to compare is both unsafe and unavailable. ## Suggested fix `return strings.TrimSpace(v), nil` in the `env` provider, matching `file`. Worth considering for `literal` too, though the argument is weaker: a literal is author-supplied in the guardfile, so trailing whitespace there is more plausibly deliberate and more visible in review. A credential with meaningful leading or trailing whitespace is not a real case for any of the three, and every consumer that has thought about it has landed on trimming independently. `sirens-echo` does `strings.TrimSpace(os.Getenv("DISCORD_TOKEN"))` at `internal/community/config.go:903`, which is exactly why the same malformed value worked fine through its gateway connection while failing through the guardfile. ## Alternative worth weighing Reject rather than trim: fail closed at resolve time with an error naming the variable and the offending character class, never the value. That surfaces the operator's mistake instead of silently correcting it, and a credential is the wrong place to be quietly lenient. Trimming matches `file`'s existing behavior and fixes every deployment already carrying the newline without an operator round trip. Rejecting is more honest but strands anyone whose stored value has one. Either beats the current state, where the value is neither trimmed nor refused. Filing with a recommendation of trim, for consistency with `file`, but the call is not mine. ## Repro ```go os.Setenv("TOKEN", "abc\n") v, _ := valuesource.Builtins()["env"](context.Background(), "TOKEN") req, _ := http.NewRequest("GET", srv.URL, nil) req.Header.Set("Authorization", "Bot "+v) _, err := http.DefaultClient.Do(req) // net/http: invalid header field value ``` Observed against umbra `v0.154.0`, the version `mcp-beaver` pins.

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

coilyco-flight-deck/umbra#304

No description provided.

Rows
Columns