fix(review): address CodeRabbit findings from PR review

- fix mock RunAndReturn delegates to _c.Call.Return not _c.Run
- remove dead bare ::1 check in IsLocalhostRequest (false positive risk)
- remove matching ::1 unbracketed test cases in httputil and auth tests
- add defer to restore os.Stderr in push_test to prevent leak on panic
- add recover() wrapper on token refresh callback in CLI client
- remove ineffective omitempty on time.Time in tokenstore pass metadata
- skip ExtendToken for permanent tokens (ExpiresAt <= 0) to avoid silent 24h conversion
- add os.TempDir() as last fallback candidate in getInternalCredentialsCandidates
- remove dead t.Setenv in run_internal_creds_test (function ignores env var)
- wrap labels_test.go constant checks in t.Run for isolated filtering
- add debug logs around UpdateAttachments for operator observability

commit: 56e2d9e89ef89a751060beac57e0eb3f03dfcb97
author: bnema · 7mo ago · 2026-02-25 01:12 UTC
parent: f6ead37
signature: unsigned

12 files · +32 -14

M internal/adapters/in/cli/push_test.go +4 -0


@@ -369,6 +369,10 @@ func TestGetGitVersionNoTagsReturnsFallbackAndWarns(t *testing.T) {
369	369		t.Fatal(err)
370	370		}
371	371		os.Stderr = w
	372	+	defer func() {
	373	+	w.Close()
	374	+	os.Stderr = origStderr
	375	+	}()
372	376
373	377		v := getGitVersion(ctx)
374	378


@@ -369,6 +369,10 @@ func TestGetGitVersionNoTagsReturnsFallbackAndWarns(t *testing.T) {
369	t.Fatal(err)	369		t.Fatal(err)
370	}	370		}
371	os.Stderr = w	371		os.Stderr = w
		372	+	defer func() {
		373	+	w.Close()
		374	+	os.Stderr = origStderr
		375	+	}()
372		376
373	v := getGitVersion(ctx)	377		v := getGitVersion(ctx)
374		378

M internal/adapters/in/cli/remote/client.go +5 -1


@@ -160,7 +160,11 @@ func (c Client) request(ctx context.Context, method, path string, body any) (h
160	160		if newToken := resp.Header.Get("X-Gordon-Token"); newToken != "" && newToken != c.token {
161	161		c.token = newToken
162	162		if c.onTokenRefreshed != nil {
163		-	c.onTokenRefreshed(newToken)
	163	+	// Token persistence is non-fatal: a panic in the callback must not crash the request.
	164	+	func() {
	165	+	defer func() { recover() }() //nolint:errcheck
	166	+	c.onTokenRefreshed(newToken)
	167	+	}()
164	168		}
165	169		}
166	170

 	if newToken := resp.Header.Get("X-Gordon-Token"); newToken != "" && newToken != c.token {
 		c.token = newToken
 		if c.onTokenRefreshed != nil {
-			c.onTokenRefreshed(newToken)
+			// Token persistence is non-fatal: a panic in the callback must not crash the request.
+			func() {
+				defer func() { recover() }() //nolint:errcheck
+				c.onTokenRefreshed(newToken)
+			}()
 		}
 	}

M internal/adapters/in/http/auth/handler_test.go +0 -1


@@ -436,7 +436,6 @@ func TestIsLocalhostRequest(t *testing.T) {
436	436		{"127.0.0.1 is localhost", "127.0.0.1:12345", true},
437	437		{"127.0.0.2 is localhost", "127.0.0.2:12345", true},
438	438		{"::1 is localhost", "[::1]:12345", true},
439		-	{"::1 without brackets", "::1:12345", true},
440	439		{"192.168.1.1 is not localhost", "192.168.1.1:12345", false},
441	440		{"10.0.0.1 is not localhost", "10.0.0.1:12345", false},
442	441		{"public IP is not localhost", "8.8.8.8:12345", false},


@@ -436,7 +436,6 @@ func TestIsLocalhostRequest(t *testing.T) {
436		{"127.0.0.1 is localhost", "127.0.0.1:12345", true},	436	{"127.0.0.1 is localhost", "127.0.0.1:12345", true},
437		{"127.0.0.2 is localhost", "127.0.0.2:12345", true},	437	{"127.0.0.2 is localhost", "127.0.0.2:12345", true},
438		{"::1 is localhost", "[::1]:12345", true},	438	{"::1 is localhost", "[::1]:12345", true},
439	-	{"::1 without brackets", "::1:12345", true},
440		{"192.168.1.1 is not localhost", "192.168.1.1:12345", false},	439	{"192.168.1.1 is not localhost", "192.168.1.1:12345", false},
441		{"10.0.0.1 is not localhost", "10.0.0.1:12345", false},	440	{"10.0.0.1 is not localhost", "10.0.0.1:12345", false},
442		{"public IP is not localhost", "8.8.8.8:12345", false},	441	{"public IP is not localhost", "8.8.8.8:12345", false},

M internal/adapters/in/http/httputil/localhost.go +3 -3


@@ -7,10 +7,10 @@ import (
7	7
8	8		// IsLocalhostRequest reports whether the request originates from localhost.
9	9		// SECURITY: Uses RemoteAddr (server-set) instead of Host header (client-spoofable).
10		-	// RemoteAddr includes port, e.g., "127.0.0.1:12345" or "[::1]:12345".
	10	+	// Go's net/http always formats RemoteAddr as "host:port" where IPv6 is bracketed:
	11	+	// e.g., "127.0.0.1:12345" or "[::1]:12345".
11	12		func IsLocalhostRequest(r *http.Request) bool {
12	13		host := r.RemoteAddr
13	14		return strings.HasPrefix(host, "127.") \|\|
14		-	strings.HasPrefix(host, "[::1]") \|\|
15		-	strings.HasPrefix(host, "::1")
	15	+	strings.HasPrefix(host, "[::1]")
16	16		}

 // IsLocalhostRequest reports whether the request originates from localhost.
 // SECURITY: Uses RemoteAddr (server-set) instead of Host header (client-spoofable).
-// RemoteAddr includes port, e.g., "127.0.0.1:12345" or "[::1]:12345".
+// Go's net/http always formats RemoteAddr as "host:port" where IPv6 is bracketed:
+// e.g., "127.0.0.1:12345" or "[::1]:12345".
 func IsLocalhostRequest(r *http.Request) bool {
 	host := r.RemoteAddr
 	return strings.HasPrefix(host, "127.") ||
-		strings.HasPrefix(host, "[::1]") ||
+		strings.HasPrefix(host, "[::1]")
-		strings.HasPrefix(host, "::1")
 }

M internal/adapters/in/http/httputil/localhost_test.go +0 -1


@@ -17,7 +17,6 @@ func TestIsLocalhostRequest(t *testing.T) {
17	17		{"ipv4 loopback", "127.0.0.1:12345", true},
18	18		{"ipv4 loopback other", "127.0.0.2:9000", true},
19	19		{"ipv6 loopback bracketed", "[::1]:12345", true},
20		-	{"ipv6 loopback plain", "::1:12345", true},
21	20		{"external ipv4", "192.168.1.1:12345", false},
22	21		{"external ipv6", "[2001:db8::1]:12345", false},
23	22		{"empty", "", false},


@@ -17,7 +17,6 @@ func TestIsLocalhostRequest(t *testing.T) {
17		{"ipv4 loopback", "127.0.0.1:12345", true},	17	{"ipv4 loopback", "127.0.0.1:12345", true},
18		{"ipv4 loopback other", "127.0.0.2:9000", true},	18	{"ipv4 loopback other", "127.0.0.2:9000", true},
19		{"ipv6 loopback bracketed", "[::1]:12345", true},	19	{"ipv6 loopback bracketed", "[::1]:12345", true},
20	-	{"ipv6 loopback plain", "::1:12345", true},
21		{"external ipv4", "192.168.1.1:12345", false},	20	{"external ipv4", "192.168.1.1:12345", false},
22		{"external ipv6", "[2001:db8::1]:12345", false},	21	{"external ipv6", "[2001:db8::1]:12345", false},
23		{"empty", "", false},	22	{"empty", "", false},

M internal/adapters/out/tokenstore/pass.go +1 -1


@@ -85,7 +85,7 @@ type tokenMetadata struct {
85	85		IssuedAt time.Time `json:"issued_at"`
86	86		ExpiresAt time.Time `json:"expires_at,omitempty"`
87	87		Revoked bool `json:"revoked"`
88		-	LastExtendedAt time.Time `json:"last_extended_at,omitempty"`
	88	+	LastExtendedAt time.Time `json:"last_extended_at"`
89	89		}
90	90
91	91		// SaveToken stores a token JWT and metadata in pass.


@@ -85,7 +85,7 @@ type tokenMetadata struct {
85		IssuedAt time.Time `json:"issued_at"`	85		IssuedAt time.Time `json:"issued_at"`
86		ExpiresAt time.Time `json:"expires_at,omitempty"`	86		ExpiresAt time.Time `json:"expires_at,omitempty"`
87		Revoked bool `json:"revoked"`	87		Revoked bool `json:"revoked"`
88	-	LastExtendedAt time.Time `json:"last_extended_at,omitempty"`	88	+	LastExtendedAt time.Time `json:"last_extended_at"`
89		}	89		}
90			90
91		// SaveToken stores a token JWT and metadata in pass.	91		// SaveToken stores a token JWT and metadata in pass.

M internal/app/run.go +5 -0


@@ -823,6 +823,7 @@ func cleanupInternalCredentials() {
823	823		// 1. XDG_RUNTIME_DIR/gordon/ (set by systemd for the daemon)
824	824		// 2. /run/user/<uid>/gordon/ (well-known systemd default, for CLI in shells without XDG_RUNTIME_DIR)
825	825		// 3. ~/.gordon/run/ (fallback for non-systemd environments)
	826	+	// 4. os.TempDir() (last resort, matches getInternalCredentialsFile fallback path)
826	827		func getInternalCredentialsCandidates() []string {
827	828		var candidates []string
828	829

@@ -844,6 +845,10 @@ func getInternalCredentialsCandidates() []string {
844	845		candidates = append(candidates, filepath.Join(homeDir, ".gordon", "run", "internal-creds.json"))
845	846		}
846	847
	848	+	// 4. os.TempDir() last resort — matches the fallback path in getInternalCredentialsFile,
	849	+	// ensuring GetInternalCredentials can find credentials even when getSecureRuntimeDir fails.
	850	+	candidates = append(candidates, filepath.Join(os.TempDir(), "gordon-internal-creds.json"))
	851	+
847	852		return candidates
848	853		}
849	854


@@ -823,6 +823,7 @@ func cleanupInternalCredentials() {
823	// 1. XDG_RUNTIME_DIR/gordon/ (set by systemd for the daemon)	823		// 1. XDG_RUNTIME_DIR/gordon/ (set by systemd for the daemon)
824	// 2. /run/user/<uid>/gordon/ (well-known systemd default, for CLI in shells without XDG_RUNTIME_DIR)	824		// 2. /run/user/<uid>/gordon/ (well-known systemd default, for CLI in shells without XDG_RUNTIME_DIR)
825	// 3. ~/.gordon/run/ (fallback for non-systemd environments)	825		// 3. ~/.gordon/run/ (fallback for non-systemd environments)
		826	+	// 4. os.TempDir() (last resort, matches getInternalCredentialsFile fallback path)
826	func getInternalCredentialsCandidates() []string {	827		func getInternalCredentialsCandidates() []string {
827	var candidates []string	828		var candidates []string
828		829

@@ -844,6 +845,10 @@ func getInternalCredentialsCandidates() []string {
844	candidates = append(candidates, filepath.Join(homeDir, ".gordon", "run", "internal-creds.json"))	845		candidates = append(candidates, filepath.Join(homeDir, ".gordon", "run", "internal-creds.json"))
845	}	846		}
846		847
		848	+	// 4. os.TempDir() last resort — matches the fallback path in getInternalCredentialsFile,
		849	+	// ensuring GetInternalCredentials can find credentials even when getSecureRuntimeDir fails.
		850	+	candidates = append(candidates, filepath.Join(os.TempDir(), "gordon-internal-creds.json"))
		851	+
847	return candidates	852		return candidates
848	}	853		}
849		854

M internal/app/run_internal_creds_test.go +0 -3


@@ -32,9 +32,6 @@ func TestGetInternalCredentialsFindsRuntimeFile(t *testing.T) {
32	32		t.Fatal(err)
33	33		}
34	34
35		-	// Unset XDG_RUNTIME_DIR so CLI path is taken, but probe gordonRuntime first
36		-	t.Setenv("XDG_RUNTIME_DIR", "")
37		-
38	35		creds, err := GetInternalCredentialsFromCandidates([]string{
39	36		filepath.Join(gordonRuntime, "internal-creds.json"),
40	37		filepath.Join(gordonHome, "internal-creds.json"),


@@ -32,9 +32,6 @@ func TestGetInternalCredentialsFindsRuntimeFile(t *testing.T) {
32		t.Fatal(err)	32	t.Fatal(err)
33		}	33	}
34			34
35	-	// Unset XDG_RUNTIME_DIR so CLI path is taken, but probe gordonRuntime first
36	-	t.Setenv("XDG_RUNTIME_DIR", "")
37	-
38		creds, err := GetInternalCredentialsFromCandidates([]string{	35	creds, err := GetInternalCredentialsFromCandidates([]string{
39		filepath.Join(gordonRuntime, "internal-creds.json"),	36	filepath.Join(gordonRuntime, "internal-creds.json"),
40		filepath.Join(gordonHome, "internal-creds.json"),	37	filepath.Join(gordonHome, "internal-creds.json"),

M internal/boundaries/in/mocks/mock_container_service.go +1 -1


@@ -829,6 +829,6 @@ func (_c MockContainerService_UpdateAttachments_Call) Return() MockContainerSe
829	829		}
830	830
831	831		func (_c MockContainerService_UpdateAttachments_Call) RunAndReturn(run func(attachments map[string][]string)) MockContainerService_UpdateAttachments_Call {
832		-	_c.Run(run)
	832	+	_c.Call.Return(run)
833	833		return _c
834	834		}


@@ -829,6 +829,6 @@ func (_c MockContainerService_UpdateAttachments_Call) Return() MockContainerSe
829		}	829		}
830			830
831		func (_c MockContainerService_UpdateAttachments_Call) RunAndReturn(run func(attachments map[string][]string)) MockContainerService_UpdateAttachments_Call {	831		func (_c MockContainerService_UpdateAttachments_Call) RunAndReturn(run func(attachments map[string][]string)) MockContainerService_UpdateAttachments_Call {
832	-	_c.Run(run)	832	+	_c.Call.Return(run)
833		return _c	833		return _c
834		}	834		}

M internal/domain/labels_test.go +5 -3


@@ -22,8 +22,10 @@ func TestLabelConstantsValues(t *testing.T) {
22	22		{domain.LabelCreated, "gordon.created"},
23	23		}
24	24		for _, tt := range tests {
25		-	if tt.constant != tt.expected {
26		-	t.Errorf("constant value changed: got %q, want %q", tt.constant, tt.expected)
27		-	}
	25	+	t.Run(tt.expected, func(t *testing.T) {
	26	+	if tt.constant != tt.expected {
	27	+	t.Errorf("constant value changed: got %q, want %q", tt.constant, tt.expected)
	28	+	}
	29	+	})
28	30		}
29	31		}

 		{domain.LabelCreated, "gordon.created"},
 	}
 	for _, tt := range tests {
-		if tt.constant != tt.expected {
+		t.Run(tt.expected, func(t *testing.T) {
-			t.Errorf("constant value changed: got %q, want %q", tt.constant, tt.expected)
+			if tt.constant != tt.expected {
-		}
+				t.Errorf("constant value changed: got %q, want %q", tt.constant, tt.expected)
+			}
+		})
 	}
 }

M internal/usecase/auth/service.go +6 -0


@@ -470,6 +470,12 @@ func (s *Service) ExtendToken(ctx context.Context, tokenString string) (string,
470	470		return tokenString, nil
471	471		}
472	472
	473	+	// Skip tokens with no expiry (permanent tokens must not be silently converted to 24h tokens)
	474	+	if tokenClaims.ExpiresAt <= 0 {
	475	+	log.Debug().Str("subject", tokenClaims.Subject).Msg("skipping extension for non-expiring token")
	476	+	return tokenString, nil
	477	+	}
	478	+
473	479		// Full validation including store checks (not expired, not revoked, exists in store)
474	480		if _, err := s.ValidateToken(ctx, tokenString); err != nil {
475	481		return "", fmt.Errorf("token validation failed: %w", err)


@@ -470,6 +470,12 @@ func (s *Service) ExtendToken(ctx context.Context, tokenString string) (string,
470	return tokenString, nil	470		return tokenString, nil
471	}	471		}
472		472
		473	+	// Skip tokens with no expiry (permanent tokens must not be silently converted to 24h tokens)
		474	+	if tokenClaims.ExpiresAt <= 0 {
		475	+	log.Debug().Str("subject", tokenClaims.Subject).Msg("skipping extension for non-expiring token")
		476	+	return tokenString, nil
		477	+	}
		478	+
473	// Full validation including store checks (not expired, not revoked, exists in store)	479		// Full validation including store checks (not expired, not revoked, exists in store)
474	if _, err := s.ValidateToken(ctx, tokenString); err != nil {	480		if _, err := s.ValidateToken(ctx, tokenString); err != nil {
475	return "", fmt.Errorf("token validation failed: %w", err)	481		return "", fmt.Errorf("token validation failed: %w", err)

M internal/usecase/container/events.go +2 -0


@@ -110,7 +110,9 @@ func (h *ConfigReloadHandler) Handle(ctx context.Context, event domain.Event) er
110	110		// Propagate updated attachment configuration so new attachments take effect
111	111		// without requiring a Gordon restart (fixes issue #87 part 2).
112	112		attachments := h.configSvc.GetAllAttachments(ctx)
	113	+	log.Debug().Int("attachment_groups", len(attachments)).Msg("propagating updated attachment configuration")
113	114		h.containerSvc.UpdateAttachments(attachments)
	115	+	log.Debug().Int("attachment_groups", len(attachments)).Msg("attachment configuration propagated")
114	116
115	117		currentContainers := h.containerSvc.List(ctx)
116	118


@@ -110,7 +110,9 @@ func (h *ConfigReloadHandler) Handle(ctx context.Context, event domain.Event) er
110	// Propagate updated attachment configuration so new attachments take effect	110		// Propagate updated attachment configuration so new attachments take effect
111	// without requiring a Gordon restart (fixes issue #87 part 2).	111		// without requiring a Gordon restart (fixes issue #87 part 2).
112	attachments := h.configSvc.GetAllAttachments(ctx)	112		attachments := h.configSvc.GetAllAttachments(ctx)
		113	+	log.Debug().Int("attachment_groups", len(attachments)).Msg("propagating updated attachment configuration")
113	h.containerSvc.UpdateAttachments(attachments)	114		h.containerSvc.UpdateAttachments(attachments)
		115	+	log.Debug().Int("attachment_groups", len(attachments)).Msg("attachment configuration propagated")
114		116
115	currentContainers := h.containerSvc.List(ctx)	117		currentContainers := h.containerSvc.List(ctx)
116		118