[Detail Bug] Env providers treat empty values as not found, breaking precedence in MultiProvider

[detail-app]

Detail Bug Report

https://app.detail.dev/org_42bf1b6f-11b3-499a-ad6d-d5a056862990/bugs/bug_b9849bfe-28df-4eb9-bb2a-2b661f3fbc25

Summary

• Context: The EnvListProvider and EnvFilesProvider are used in a MultiProvider chain to provide environment variables from multiple sources, with higher-priority providers (like .env files) checked before lower-priority providers (like OS environment).
• Bug: EnvListProvider and EnvFilesProvider cannot distinguish between "variable explicitly set to empty string" and "variable not found", both returning empty string.
• Actual vs. expected: When a variable is explicitly set to empty in a higher-priority provider, MultiProvider incorrectly falls through to lower-priority providers instead of respecting the explicit empty value.
• Impact: Users cannot override/disable environment variables by setting them to empty in .env files, potentially causing production credentials to leak into development environments.

Code with bug

pkg/environment/env.go:

func (p *EnvListProvider) Get(_ context.Context, name string) string {
	for _, e := range p.env {
		n, v, ok := strings.Cut(e, "=")
		if ok && n == name {
			return v  // <-- BUG 🔴 Returns empty string for both "VAR=" and variable not found
		}
	}
	return ""  // <-- BUG 🔴 Also returns empty string, making them indistinguishable
}

pkg/environment/env.go:

func (p *EnvFilesProvider) Get(_ context.Context, name string) string {
	for _, kv := range p.values {
		if kv.Key == name {
			return kv.Value  // <-- BUG 🔴 Same issue as EnvListProvider
		}
	}

	return ""  // <-- BUG 🔴 Cannot distinguish from explicitly empty value
}

pkg/environment/multi.go (how the bug manifests):

func (p *MultiProvider) Get(ctx context.Context, name string) string {
	for _, provider := range p.providers {
		value := provider.Get(ctx, name)
		if value != "" {  // <-- BUG 🔴 Treats empty string as "not found"
			return value
		}
	}

	return ""
}

Failing test

package environment

import (
	"context"
	"testing"

	"github.com/stretchr/testify/assert"
)

// TestMultiProvider_EmptyValueFallthrough demonstrates the bug where
// an explicitly empty value in a higher-priority provider incorrectly
// falls through to a lower-priority provider.
func TestMultiProvider_EmptyValueFallthrough(t *testing.T) {
	t.Parallel()

	// Simulate real-world scenario:
	// - Higher priority: .env file with API_KEY= (explicitly disabled)
	// - Lower priority: OS environment with API_KEY=secret-key

	// First provider has VAR2 set to empty string
	firstProvider := NewEnvListProvider([]string{
		"VAR2=",
	})

	// Second provider has VAR2 set to a non-empty value
	secondProvider := NewEnvListProvider([]string{
		"VAR2=fallback_value",
	})

	multiProvider := NewMultiProvider(firstProvider, secondProvider)
	ctx := context.Background()

	val := multiProvider.Get(ctx, "VAR2")

	// EXPECTED: Empty string from first provider (higher priority)
	// ACTUAL: "fallback_value" from second provider (BUG!)
	assert.Equal(t, "", val, "MultiProvider should return empty string from first provider, not fall through to second")
}

Test output:

=== RUN   TestMultiProvider_EmptyValueFallthrough
=== PAUSE TestMultiProvider_EmptyValueFallthrough
=== CONT  TestMultiProvider_EmptyValueFallthrough
    env_empty_value_test.go:57:
        Error Trace:  /home/user/cagent/pkg/environment/env_empty_value_test.go:57
        Error:        Not equal:
                     expected: ""
                     actual  : "fallback_value"

                     Diff:
                     --- Expected
                     +++ Actual
                     @@ -1 +1 @@
                     -
                     +fallback_value
        Test:        TestMultiProvider_EmptyValueFallthrough
        Messages:    MultiProvider should return empty string from first provider, not fall through to second
--- FAIL: TestMultiProvider_EmptyValueFallthrough (0.00s)
FAIL
FAIL    github.com/docker/cagent/pkg/environment    0.003s
FAIL

Exploit scenario

• OS environment defines API_KEY=secret-production-key.
• A higher-priority .env file sets API_KEY= to disable it.
• MultiProvider falls through on empty, returning the OS value instead.
• Result: unintended use/exposure of the production key in development.

Recommended fix

Change the Provider interface to return (value string, found bool) and update implementations and MultiProvider to test found instead of value != "".

type Provider interface {
    Get(ctx context.Context, name string) (string, bool)
}

func (p *EnvListProvider) Get(_ context.Context, name string) (string, bool) {
    for _, e := range p.env {
        n, v, ok := strings.Cut(e, "=")
        if ok && n == name {
            return v, true  // <-- FIX 🟢 Indicate "found"
        }
    }
    return "", false  // <-- FIX 🟢 Indicate "not found"
}

func (p *MultiProvider) Get(ctx context.Context, name string) (string, bool) {
    for _, provider := range p.providers {
        value, found := provider.Get(ctx, name)
        if found {  // <-- FIX 🟢 Check presence, not non-empty value
            return value, true
        }
    }
    return "", false
}