diff --git a/internal/auth/auth.go b/internal/auth/auth.go index cf7a3ba..0517e19 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -45,7 +45,10 @@ type Authenticator struct { } func NewAuthenticator() (*Authenticator, error) { - cfgDir, _ := os.UserConfigDir() + cfgDir, err := os.UserConfigDir() + if err != nil { + return nil, fmt.Errorf("cannot determine user config directory: %w", err) + } authPath := filepath.Join(cfgDir, "tusk", "auth.json") cfg, err := config.Get() @@ -279,7 +282,10 @@ func (a *Authenticator) RequestDeviceCode(ctx context.Context) (DeviceCodeRespon } defer func() { _ = resp.Body.Close() }() - body, _ := io.ReadAll(resp.Body) + body, err := io.ReadAll(resp.Body) + if err != nil { + return dcr, fmt.Errorf("error reading device code response body: %w", err) + } if resp.StatusCode != http.StatusOK { return dcr, fmt.Errorf( "error returned by device code endpoint %d: %s", @@ -318,8 +324,11 @@ func (a *Authenticator) PollForToken(ctx context.Context, dcr DeviceCodeResponse if err != nil { return fmt.Errorf("error making request for token: %w", err) } - body, _ := io.ReadAll(resp.Body) + body, err := io.ReadAll(resp.Body) _ = resp.Body.Close() + if err != nil { + return fmt.Errorf("error reading token response body: %w", err) + } var tr tokenResp if err := json.Unmarshal(body, &tr); err != nil { @@ -372,7 +381,10 @@ func (a *Authenticator) FetchUserEmail(ctx context.Context) error { return err } defer func() { _ = resp.Body.Close() }() - b, _ := io.ReadAll(resp.Body) + b, err := io.ReadAll(resp.Body) + if err != nil { + return fmt.Errorf("error reading userinfo response body: %w", err) + } if resp.StatusCode != http.StatusOK { return fmt.Errorf("userinfo http %d: %s", resp.StatusCode, string(b)) @@ -408,8 +420,11 @@ func (a *Authenticator) refreshAccessToken(ctx context.Context) error { if err != nil { return fmt.Errorf("error requesting refresh token endpoint %w", err) } - body, _ := io.ReadAll(resp.Body) + body, err := io.ReadAll(resp.Body) _ = resp.Body.Close() + if err != nil { + return fmt.Errorf("error reading refresh token response body: %w", err) + } if resp.StatusCode != http.StatusOK { return fmt.Errorf("refresh http %d: %s", resp.StatusCode, string(body)) diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go new file mode 100644 index 0000000..e077fd7 --- /dev/null +++ b/internal/auth/auth_test.go @@ -0,0 +1,218 @@ +package auth + +// Regression tests for the three bugs fixed in commit 3d1d0c8: +// +// 1. Token file misplacement: os.UserConfigDir() error was silently ignored, +// so a failure produced an empty cfgDir and the token was written to a +// relative path ("tusk/auth.json") in the current working directory instead +// of the proper user-config location. +// +// 2. Silent io.ReadAll errors: all four Auth0 HTTP methods (RequestDeviceCode, +// PollForToken, FetchUserEmail, refreshAccessToken) discarded io.ReadAll +// errors with `body, _ := io.ReadAll(...)`, hiding network failures. + +import ( + "context" + "errors" + "io" + "net/http" + "runtime" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// ── helpers ────────────────────────────────────────────────────────────────── + +// errorReader returns a fixed error on the first Read call, mimicking a +// mid-stream network failure that causes io.ReadAll to return an error. +type errorReader struct{} + +func (e *errorReader) Read(_ []byte) (int, error) { + return 0, errors.New("simulated network read error") +} + +// errorBodyTransport is an http.RoundTripper that returns a well-formed +// HTTP 200 response whose body always errors on Read. This is the minimal +// setup needed to trigger the io.ReadAll error paths that were previously +// swallowed with `body, _ := io.ReadAll(resp.Body)`. +type errorBodyTransport struct{} + +func (t *errorBodyTransport) RoundTrip(_ *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Header: make(http.Header), + Body: io.NopCloser(&errorReader{}), + }, nil +} + +// newTestAuthenticator builds a minimal Authenticator for unit tests. +// It bypasses NewAuthenticator (which calls os.UserConfigDir and config.Get) +// so tests remain hermetic and fast. +func newTestAuthenticator(transport http.RoundTripper) *Authenticator { + return &Authenticator{ + authFilePath: "/tmp/tusk-test-auth.json", + httpClient: &http.Client{Timeout: 5 * time.Second, Transport: transport}, + domain: "test.example.auth0.com", + clientID: "test-client-id", + scope: "openid email offline_access", + audience: "drift-cli", + } +} + +// ── Bug 1: os.UserConfigDir() failure ──────────────────────────────────────── + +// TestNewAuthenticator_UserConfigDirFailure is a minimal repro for the token +// file misplacement bug. +// +// Before the fix, NewAuthenticator used: +// +// cfgDir, _ := os.UserConfigDir() // error silently dropped +// authPath := filepath.Join(cfgDir, ...) // cfgDir == "" → relative path +// +// With cfgDir empty, filepath.Join("", "tusk", "auth.json") returned the +// relative path "tusk/auth.json", so the token was written to the current +// working directory – not the user's config dir. +// +// After the fix the error is propagated and NewAuthenticator returns an error, +// preventing the misplaced write entirely. +// +// Reproduce: +// +// HOME="" XDG_CONFIG_HOME="" go test ./internal/auth/ -run TestNewAuthenticator_UserConfigDirFailure +func TestNewAuthenticator_UserConfigDirFailure(t *testing.T) { + // Unset every env var that os.UserConfigDir reads on any supported OS. + t.Setenv("HOME", "") + t.Setenv("XDG_CONFIG_HOME", "") // Linux fallback + if runtime.GOOS == "windows" { + t.Setenv("AppData", "") + } + + auth, err := NewAuthenticator() + + require.Error(t, err, "NewAuthenticator must fail when the config dir cannot be determined") + assert.Nil(t, auth) + assert.Contains(t, err.Error(), "cannot determine user config directory", + "error message should identify the root cause") + + // Before the fix this test would PASS (no error returned) but the + // resulting Authenticator would have an authFilePath starting with + // "tusk/" – a relative path. The assertion below is included as + // documentation of the old buggy behaviour: + // + // assert.True(t, filepath.IsAbs(auth.authFilePath)) // would FAIL +} + +// TestNewAuthenticator_AuthPathIsAbsolute verifies that, when the environment +// is intact, the resolved auth file path is always absolute. This would have +// caught the misplacement silently introduced when cfgDir was empty. +func TestNewAuthenticator_AuthPathIsAbsolute(t *testing.T) { + // NewAuthenticator also needs a minimal config (clientID etc.). + // Set the env vars expected by config.Get / the Authenticator constructor + // so the call succeeds in CI without a real config file. + t.Setenv("TUSK_AUTH0_CLIENT_ID", "test-client-id") + t.Setenv("TUSK_AUTH0_AUDIENCE", "drift-cli") + + auth, err := NewAuthenticator() + if err != nil { + // If the env/config still isn't complete enough (e.g. missing YAML), + // skip rather than fail – the important assertion is the path check. + t.Skipf("NewAuthenticator setup incomplete in test env: %v", err) + } + + assert.True(t, strings.HasPrefix(auth.authFilePath, "/") || (len(auth.authFilePath) > 1 && auth.authFilePath[1] == ':'), + "authFilePath %q must be an absolute path, not a relative one", auth.authFilePath) + assert.Contains(t, auth.authFilePath, "tusk", + "authFilePath should be inside a 'tusk' sub-directory") + assert.Contains(t, auth.authFilePath, "auth.json", + "authFilePath should point at auth.json") +} + +// ── Bug 2: silent io.ReadAll errors ────────────────────────────────────────── + +// TestRequestDeviceCode_PropagatesReadError is a minimal repro for the +// silent-error bug in RequestDeviceCode. +// +// Before the fix: +// +// body, _ := io.ReadAll(resp.Body) // network error silently dropped +// json.Unmarshal(body, &dcr) // body is empty/partial → silent failure +// +// After the fix: +// +// body, err := io.ReadAll(resp.Body) +// if err != nil { return dcr, fmt.Errorf("error reading device code response body: %w", err) } +func TestRequestDeviceCode_PropagatesReadError(t *testing.T) { + a := newTestAuthenticator(&errorBodyTransport{}) + + _, err := a.RequestDeviceCode(context.Background()) + + require.Error(t, err) + assert.Contains(t, err.Error(), "error reading device code response body", + "ReadAll error must be surfaced with context") + assert.Contains(t, err.Error(), "simulated network read error") +} + +// TestPollForToken_PropagatesReadError is a minimal repro for the +// silent-error bug in PollForToken. +// +// Before the fix: +// +// body, _ := io.ReadAll(resp.Body) // error silently dropped +func TestPollForToken_PropagatesReadError(t *testing.T) { + a := newTestAuthenticator(&errorBodyTransport{}) + + dcr := DeviceCodeResponse{ + DeviceCode: "test-device-code", + Interval: 0, // zero → 5 s default, but ctx is cancelled immediately + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + err := a.PollForToken(ctx, dcr) + + require.Error(t, err) + assert.Contains(t, err.Error(), "error reading token response body", + "ReadAll error must be surfaced with context") + assert.Contains(t, err.Error(), "simulated network read error") +} + +// TestFetchUserEmail_PropagatesReadError is a minimal repro for the +// silent-error bug in FetchUserEmail. +// +// Before the fix: +// +// b, _ := io.ReadAll(resp.Body) // error silently dropped +func TestFetchUserEmail_PropagatesReadError(t *testing.T) { + a := newTestAuthenticator(&errorBodyTransport{}) + a.AccessToken = "fake-access-token" + + err := a.FetchUserEmail(context.Background()) + + require.Error(t, err) + assert.Contains(t, err.Error(), "error reading userinfo response body", + "ReadAll error must be surfaced with context") + assert.Contains(t, err.Error(), "simulated network read error") +} + +// TestRefreshAccessToken_PropagatesReadError is a minimal repro for the +// silent-error bug in refreshAccessToken. +// +// Before the fix: +// +// body, _ := io.ReadAll(resp.Body) // error silently dropped +func TestRefreshAccessToken_PropagatesReadError(t *testing.T) { + a := newTestAuthenticator(&errorBodyTransport{}) + a.RefreshToken = "fake-refresh-token" + + err := a.refreshAccessToken(context.Background()) + + require.Error(t, err) + assert.Contains(t, err.Error(), "error reading refresh token response body", + "ReadAll error must be surfaced with context") + assert.Contains(t, err.Error(), "simulated network read error") +} diff --git a/internal/runner/executor.go b/internal/runner/executor.go index 8385e55..8ac12d6 100644 --- a/internal/runner/executor.go +++ b/internal/runner/executor.go @@ -12,6 +12,7 @@ import ( "os/exec" "path/filepath" "strings" + "sync" "time" "github.com/Use-Tusk/fence/pkg/fence" @@ -37,6 +38,7 @@ type Executor struct { globalSpans []*core.Span // Explicitly marked global spans for cross-trace matching allowSuiteWideMatching bool // When true, allows cross-trace matching from any suite span cancelTests context.CancelFunc + cancelTestsMu sync.Mutex disableSandbox bool debug bool fenceManager *fence.Manager @@ -162,7 +164,9 @@ func (e *Executor) RunTestsConcurrently(tests []Test, maxConcurrency int) ([]Tes defer cancel() // Store cancel function so signal handler can call it + e.cancelTestsMu.Lock() e.cancelTests = cancel + e.cancelTestsMu.Unlock() testChan := make(chan Test, len(tests)) resultChan := make(chan TestResult, len(tests)) @@ -387,8 +391,11 @@ func (e *Executor) SetAllowSuiteWideMatching(enabled bool) { } func (e *Executor) CancelTests() { - if e.cancelTests != nil { - e.cancelTests() + e.cancelTestsMu.Lock() + cancel := e.cancelTests + e.cancelTestsMu.Unlock() + if cancel != nil { + cancel() } } diff --git a/internal/runner/executor_test.go b/internal/runner/executor_test.go index 067b3ce..187a74c 100644 --- a/internal/runner/executor_test.go +++ b/internal/runner/executor_test.go @@ -877,6 +877,68 @@ func BenchmarkExecutor_RunTestsConcurrently(b *testing.B) { } } +// TestExecutor_CancelTests_NoDataRace is a minimal repro for the data race that +// existed between RunTestsConcurrently (which writes e.cancelTests) and CancelTests +// (which reads and invokes e.cancelTests). The two operations ran in separate +// goroutines without synchronisation, triggering the Go race detector. +// +// Run with: go test -race ./internal/runner/ -run TestExecutor_CancelTests_NoDataRace +// +// Before the fix (cancelTestsMu added in commit 3d1d0c8) this test would be +// flagged as a DATA RACE by the race detector. After the fix it passes cleanly. +func TestExecutor_CancelTests_NoDataRace(t *testing.T) { + // A fast server so the writer goroutine actually enters RunTestsConcurrently + // and stores the cancel function before the test finishes. + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Tiny sleep so the two goroutines overlap in time. + time.Sleep(5 * time.Millisecond) + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + // Run many iterations: each one races a write (RunTestsConcurrently stores + // cancelTests) against a read (CancelTests loads cancelTests). + const iterations = 50 + for i := 0; i < iterations; i++ { + executor := NewExecutor() + executor.serviceURL = server.URL + executor.SetTestTimeout(500 * time.Millisecond) + + tests := []Test{ + {TraceID: "race-t1", Request: Request{Method: "GET", Path: "/"}}, + {TraceID: "race-t2", Request: Request{Method: "GET", Path: "/"}}, + } + + var wg sync.WaitGroup + wg.Add(2) + + // Goroutine A: writes e.cancelTests inside RunTestsConcurrently. + go func() { + defer wg.Done() + _, _ = executor.RunTestsConcurrently(tests, 2) + }() + + // Goroutine B: reads and invokes e.cancelTests inside CancelTests. + // Without the mutex these two goroutines race on the same field. + go func() { + defer wg.Done() + executor.CancelTests() + }() + + wg.Wait() + } +} + +// TestExecutor_CancelTests_BeforeRunIsNoop verifies that CancelTests is safe to +// call before any test run has started (e.cancelTests is nil). +func TestExecutor_CancelTests_BeforeRunIsNoop(t *testing.T) { + executor := NewExecutor() + // Must not panic when cancelTests has never been set. + assert.NotPanics(t, func() { + executor.CancelTests() + }) +} + func BenchmarkCountPassedTests(b *testing.B) { // Create test data results := make([]TestResult, 1000)