3094000 fix: address CodeRabbit review — dedup validation, testify, docs clarity · gordon

M docs/upgrading.md +3 -1


@@ -42,6 +42,7 @@ Gordon v2.30.0 removes password-based authentication entirely. Only token-based
42	42		3. Update your config to remove password fields:
43	43
44	44		Before (v2.16.0):
	45	+
45	46		```toml
46	47		[auth]
47	48		enabled = true

@@ -52,6 +53,7 @@ Gordon v2.30.0 removes password-based authentication entirely. Only token-based
52	53		```
53	54
54	55		After (v2.30.0):
	56	+
55	57		```toml
56	58		[auth]
57	59		enabled = true

@@ -79,7 +81,7 @@ Gordon v2.30.0 removes password-based authentication entirely. Only token-based
79	81
80	82		Ephemeral access tokens issued by `/auth/token` now have a configurable lifetime via `auth.access_token_ttl` (default `"15m"`, maximum `"1h"`).
81	83
82		-	Tokens with a lifetime at or below `MaxAccessTokenLifetime` (1 hour) are treated as ephemeral: they skip token store validation for performance, which means they cannot be individually revoked. They become invalid only when they naturally expire. If you need tighter revocation control, use a shorter TTL.
	84	+	Tokens with a lifetime at or below `MaxAccessTokenLifetime` (1 hour) are treated as ephemeral: they skip token store validation for performance, which means they cannot be individually revoked. They become invalid only when they naturally expire. Shortening `auth.access_token_ttl` only reduces the exposure window; it does not enable per-token revocation. If you need explicit revocation, use a stored long-lived token instead of `/auth/token`.
83	85
84	86		```toml
85	87		[auth]


@@ -42,6 +42,7 @@ Gordon v2.30.0 removes password-based authentication entirely. Only token-based
42		3. Update your config to remove password fields:	42		3. Update your config to remove password fields:
43			43
44		Before (v2.16.0):	44		Before (v2.16.0):
			45	+
45		```toml	46		```toml
46		[auth]	47		[auth]
47		enabled = true	48		enabled = true

@@ -52,6 +53,7 @@ Gordon v2.30.0 removes password-based authentication entirely. Only token-based
52		```	53		```
53			54
54		After (v2.30.0):	55		After (v2.30.0):
			56	+
55		```toml	57		```toml
56		[auth]	58		[auth]
57		enabled = true	59		enabled = true

@@ -79,7 +81,7 @@ Gordon v2.30.0 removes password-based authentication entirely. Only token-based
79			81
80		Ephemeral access tokens issued by `/auth/token` now have a configurable lifetime via `auth.access_token_ttl` (default `"15m"`, maximum `"1h"`).	82		Ephemeral access tokens issued by `/auth/token` now have a configurable lifetime via `auth.access_token_ttl` (default `"15m"`, maximum `"1h"`).
81			83
82	-	Tokens with a lifetime at or below `MaxAccessTokenLifetime` (1 hour) are treated as ephemeral: they skip token store validation for performance, which means they cannot be individually revoked. They become invalid only when they naturally expire. If you need tighter revocation control, use a shorter TTL.	84	+	Tokens with a lifetime at or below `MaxAccessTokenLifetime` (1 hour) are treated as ephemeral: they skip token store validation for performance, which means they cannot be individually revoked. They become invalid only when they naturally expire. Shortening `auth.access_token_ttl` only reduces the exposure window; it does not enable per-token revocation. If you need explicit revocation, use a stored long-lived token instead of `/auth/token`.
83			85
84		```toml	86		```toml
85		[auth]	87		[auth]

M internal/adapters/out/domainsecrets/store.go +13 -9


@@ -219,15 +219,22 @@ func (s *FileStore) GetAllAttachment(containerName string) (map[string]string, e
219	219		return secrets, nil
220	220		}
221	221
222		-	// writeSecretsAtomic writes all secrets to the domain's env file atomically.
223		-	// It writes to a temporary file first, syncs it, then renames to the final path.
224		-	func (s *FileStore) writeSecretsAtomic(domainName string, secrets map[string]string) error {
225		-	// Validate secret values before any file operations
	222	+	// validateSecretValues checks that no secret value contains newline characters.
	223	+	func validateSecretValues(secrets map[string]string) error {
226	224		for key, value := range secrets {
227	225		if strings.ContainsAny(value, "\n\r") {
228	226		return fmt.Errorf("secret value for %q contains newline characters", key)
229	227		}
230	228		}
	229	+	return nil
	230	+	}
	231	+
	232	+	// writeSecretsAtomic writes all secrets to the domain's env file atomically.
	233	+	// It writes to a temporary file first, syncs it, then renames to the final path.
	234	+	func (s *FileStore) writeSecretsAtomic(domainName string, secrets map[string]string) error {
	235	+	if err := validateSecretValues(secrets); err != nil {
	236	+	return err
	237	+	}
231	238
232	239		envFile, err := s.validateEnvFilePath(domainName)
233	240		if err != nil {

@@ -370,11 +377,8 @@ func (s *FileStore) validateAttachmentEnvFilePath(containerName string) (string,
370	377		// writeAttachmentSecretsAtomic writes all secrets to the attachment's env file atomically.
371	378		// It writes to a temporary file first, syncs it, then renames to the final path.
372	379		func (s *FileStore) writeAttachmentSecretsAtomic(containerName string, secrets map[string]string) error {
373		-	// Validate secret values before any file operations
374		-	for key, value := range secrets {
375		-	if strings.ContainsAny(value, "\n\r") {
376		-	return fmt.Errorf("secret value for %q contains newline characters", key)
377		-	}
	380	+	if err := validateSecretValues(secrets); err != nil {
	381	+	return err
378	382		}
379	383
380	384		envFile, err := s.validateAttachmentEnvFilePath(containerName)

 	return secrets, nil
 }
-// writeSecretsAtomic writes all secrets to the domain's env file atomically.
+// validateSecretValues checks that no secret value contains newline characters.
-// It writes to a temporary file first, syncs it, then renames to the final path.
+func validateSecretValues(secrets map[string]string) error {
-func (s *FileStore) writeSecretsAtomic(domainName string, secrets map[string]string) error {
-	// Validate secret values before any file operations
 	for key, value := range secrets {
 		if strings.ContainsAny(value, "\n\r") {
 			return fmt.Errorf("secret value for %q contains newline characters", key)
 		}
 	}
+	return nil
+}
+// writeSecretsAtomic writes all secrets to the domain's env file atomically.
+// It writes to a temporary file first, syncs it, then renames to the final path.
+func (s *FileStore) writeSecretsAtomic(domainName string, secrets map[string]string) error {
+	if err := validateSecretValues(secrets); err != nil {
+		return err
+	}
 	envFile, err := s.validateEnvFilePath(domainName)
 	if err != nil {
 // writeAttachmentSecretsAtomic writes all secrets to the attachment's env file atomically.
 // It writes to a temporary file first, syncs it, then renames to the final path.
 func (s *FileStore) writeAttachmentSecretsAtomic(containerName string, secrets map[string]string) error {
-	// Validate secret values before any file operations
+	if err := validateSecretValues(secrets); err != nil {
-	for key, value := range secrets {
+		return err
-		if strings.ContainsAny(value, "\n\r") {
-			return fmt.Errorf("secret value for %q contains newline characters", key)
-		}
 	}
 	envFile, err := s.validateAttachmentEnvFilePath(containerName)

M internal/app/run_auth_config_test.go +16 -44


@@ -2,12 +2,13 @@ package app
2	2
3	3		import (
4	4		"context"
5		-	"strings"
6	5		"testing"
7	6
8	7		"github.com/bnema/gordon/internal/domain"
9	8		"github.com/bnema/zerowrap"
10	9		"github.com/spf13/viper"
	10	+	"github.com/stretchr/testify/assert"
	11	+	"github.com/stretchr/testify/require"
11	12		)
12	13
13	14		func TestBuildAuthConfig_UsesConfigEnabledFlag(t *testing.T) {

@@ -18,78 +19,49 @@ func TestBuildAuthConfig_UsesConfigEnabledFlag(t *testing.T) {
18	19		cfg.Auth.Type = "token"
19	20
20	21		authCfg, err := buildAuthConfig(context.Background(), cfg, domain.AuthTypeToken, domain.SecretsBackendUnsafe, t.TempDir(), zerowrap.Default())
21		-	if err != nil {
22		-	t.Fatalf("expected no error, got %v", err)
23		-	}
24		-	if authCfg.Enabled {
25		-	t.Fatalf("expected auth config enabled=false, got true")
26		-	}
	22	+	require.NoError(t, err)
	23	+	assert.False(t, authCfg.Enabled, "expected auth config enabled=false")
27	24		}
28	25
29	26		func TestResolveSecretsBackend_RejectsMissingBackend(t *testing.T) {
30	27		_, err := resolveSecretsBackend("")
31		-	if err == nil {
32		-	t.Fatal("expected error for missing secrets backend")
33		-	}
34		-	if !strings.Contains(err.Error(), "auth.secrets_backend is required") {
35		-	t.Fatalf("unexpected error: %v", err)
36		-	}
	28	+	require.Error(t, err)
	29	+	assert.Contains(t, err.Error(), "auth.secrets_backend is required")
37	30		}
38	31
39	32		func TestResolveSecretsBackend_RejectsUnknownBackend(t *testing.T) {
40	33		_, err := resolveSecretsBackend("vault")
41		-	if err == nil {
42		-	t.Fatal("expected error for unsupported secrets backend")
43		-	}
44		-	if !strings.Contains(err.Error(), `unsupported auth.secrets_backend "vault"`) {
45		-	t.Fatalf("unexpected error: %v", err)
46		-	}
	34	+	require.Error(t, err)
	35	+	assert.Contains(t, err.Error(), `unsupported auth.secrets_backend "vault"`)
47	36		}
48	37
49	38		func TestResolveAuthType_RejectsPassword(t *testing.T) {
50	39		cfg := Config{}
51	40		cfg.Auth.Type = "password"
52	41		_, err := resolveAuthType(cfg)
53		-	if err == nil {
54		-	t.Fatal("expected error for auth.type=password")
55		-	}
56		-	if !strings.Contains(err.Error(), "unsupported auth.type") {
57		-	t.Fatalf("unexpected error: %v", err)
58		-	}
	42	+	require.Error(t, err)
	43	+	assert.Contains(t, err.Error(), "unsupported auth.type")
59	44		}
60	45
61	46		func TestResolveAuthType_AcceptsToken(t *testing.T) {
62	47		cfg := Config{}
63	48		cfg.Auth.Type = "token"
64	49		authType, err := resolveAuthType(cfg)
65		-	if err != nil {
66		-	t.Fatalf("unexpected error: %v", err)
67		-	}
68		-	if authType != domain.AuthTypeToken {
69		-	t.Fatalf("expected AuthTypeToken, got %v", authType)
70		-	}
	50	+	require.NoError(t, err)
	51	+	assert.Equal(t, domain.AuthTypeToken, authType)
71	52		}
72	53
73	54		func TestResolveAuthType_AcceptsEmpty(t *testing.T) {
74	55		cfg := Config{}
75	56		authType, err := resolveAuthType(cfg)
76		-	if err != nil {
77		-	t.Fatalf("unexpected error: %v", err)
78		-	}
79		-	if authType != domain.AuthTypeToken {
80		-	t.Fatalf("expected AuthTypeToken, got %v", authType)
81		-	}
	57	+	require.NoError(t, err)
	58	+	assert.Equal(t, domain.AuthTypeToken, authType)
82	59		}
83	60
84	61		func TestLoadConfig_DoesNotDefaultSecretsBackendToUnsafe(t *testing.T) {
85	62		v := viper.New()
86	63
87	64		err := loadConfig(v, "")
88		-	if err != nil {
89		-	t.Fatalf("expected no error, got %v", err)
90		-	}
91		-
92		-	if got := v.GetString("auth.secrets_backend"); got != "" {
93		-	t.Fatalf("expected empty default for auth.secrets_backend, got %q", got)
94		-	}
	65	+	require.NoError(t, err)
	66	+	assert.Empty(t, v.GetString("auth.secrets_backend"), "expected empty default for auth.secrets_backend")
95	67		}

 import (
 	"context"
-	"strings"
 	"testing"
 	"github.com/bnema/gordon/internal/domain"
 	"github.com/bnema/zerowrap"
 	"github.com/spf13/viper"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 func TestBuildAuthConfig_UsesConfigEnabledFlag(t *testing.T) {
 	cfg.Auth.Type = "token"
 	authCfg, err := buildAuthConfig(context.Background(), cfg, domain.AuthTypeToken, domain.SecretsBackendUnsafe, t.TempDir(), zerowrap.Default())
-	if err != nil {
+	require.NoError(t, err)
-		t.Fatalf("expected no error, got %v", err)
+	assert.False(t, authCfg.Enabled, "expected auth config enabled=false")
-	}
-	if authCfg.Enabled {
-		t.Fatalf("expected auth config enabled=false, got true")
-	}
 }
 func TestResolveSecretsBackend_RejectsMissingBackend(t *testing.T) {
 	_, err := resolveSecretsBackend("")
-	if err == nil {
+	require.Error(t, err)
-		t.Fatal("expected error for missing secrets backend")
+	assert.Contains(t, err.Error(), "auth.secrets_backend is required")
-	}
-	if !strings.Contains(err.Error(), "auth.secrets_backend is required") {
-		t.Fatalf("unexpected error: %v", err)
-	}
 }
 func TestResolveSecretsBackend_RejectsUnknownBackend(t *testing.T) {
 	_, err := resolveSecretsBackend("vault")
-	if err == nil {
+	require.Error(t, err)
-		t.Fatal("expected error for unsupported secrets backend")
+	assert.Contains(t, err.Error(), `unsupported auth.secrets_backend "vault"`)
-	}
-	if !strings.Contains(err.Error(), `unsupported auth.secrets_backend "vault"`) {
-		t.Fatalf("unexpected error: %v", err)
-	}
 }
 func TestResolveAuthType_RejectsPassword(t *testing.T) {
 	cfg := Config{}
 	cfg.Auth.Type = "password"
 	_, err := resolveAuthType(cfg)
-	if err == nil {
+	require.Error(t, err)
-		t.Fatal("expected error for auth.type=password")
+	assert.Contains(t, err.Error(), "unsupported auth.type")
-	}
-	if !strings.Contains(err.Error(), "unsupported auth.type") {
-		t.Fatalf("unexpected error: %v", err)
-	}
 }
 func TestResolveAuthType_AcceptsToken(t *testing.T) {
 	cfg := Config{}
 	cfg.Auth.Type = "token"
 	authType, err := resolveAuthType(cfg)
-	if err != nil {
+	require.NoError(t, err)
-		t.Fatalf("unexpected error: %v", err)
+	assert.Equal(t, domain.AuthTypeToken, authType)
-	}
-	if authType != domain.AuthTypeToken {
-		t.Fatalf("expected AuthTypeToken, got %v", authType)
-	}
 }
 func TestResolveAuthType_AcceptsEmpty(t *testing.T) {
 	cfg := Config{}
 	authType, err := resolveAuthType(cfg)
-	if err != nil {
+	require.NoError(t, err)
-		t.Fatalf("unexpected error: %v", err)
+	assert.Equal(t, domain.AuthTypeToken, authType)
-	}
-	if authType != domain.AuthTypeToken {
-		t.Fatalf("expected AuthTypeToken, got %v", authType)
-	}
 }
 func TestLoadConfig_DoesNotDefaultSecretsBackendToUnsafe(t *testing.T) {
 	v := viper.New()
 	err := loadConfig(v, "")
-	if err != nil {
+	require.NoError(t, err)
-		t.Fatalf("expected no error, got %v", err)
+	assert.Empty(t, v.GetString("auth.secrets_backend"), "expected empty default for auth.secrets_backend")
-	}
-	if got := v.GetString("auth.secrets_backend"); got != "" {
-		t.Fatalf("expected empty default for auth.secrets_backend, got %q", got)
-	}
 }

M pkg/validation/path.go +1 -1


@@ -136,7 +136,7 @@ func ValidateDomainParam(domain string) error {
136	136		if strings.Contains(domain, "..") {
137	137		return fmt.Errorf("domain contains path traversal sequence")
138	138		}
139		-	if strings.ContainsAny(domain, "\x00\n\r") {
	139	+	if strings.ContainsAny(domain, "\x00\n\r/") {
140	140		return fmt.Errorf("domain contains invalid characters")
141	141		}
142	142		return nil


@@ -136,7 +136,7 @@ func ValidateDomainParam(domain string) error {
136		if strings.Contains(domain, "..") {	136		if strings.Contains(domain, "..") {
137		return fmt.Errorf("domain contains path traversal sequence")	137		return fmt.Errorf("domain contains path traversal sequence")
138		}	138		}
139	-	if strings.ContainsAny(domain, "\x00\n\r") {	139	+	if strings.ContainsAny(domain, "\x00\n\r/") {
140		return fmt.Errorf("domain contains invalid characters")	140		return fmt.Errorf("domain contains invalid characters")
141		}	141		}
142		return nil	142		return nil

M pkg/validation/path_test.go +7 -5


@@ -3,6 +3,8 @@ package validation
3	3		import (
4	4		"strings"
5	5		"testing"
	6	+
	7	+	"github.com/stretchr/testify/require"
6	8		)
7	9
8	10		func TestValidateRepositoryName(t *testing.T) {

@@ -215,15 +217,15 @@ func TestValidateDomainParam(t *testing.T) {
215	217		{"carriage return", "example\r.com", false},
216	218		{"empty", "", false},
217	219		{"double dot in middle", "foo..bar", false},
	220	+	{"forward slash", "example/com", false},
218	221		}
219	222		for _, tt := range tests {
220	223		t.Run(tt.name, func(t *testing.T) {
221	224		err := ValidateDomainParam(tt.input)
222		-	if tt.valid && err != nil {
223		-	t.Errorf("expected valid, got error: %v", err)
224		-	}
225		-	if !tt.valid && err == nil {
226		-	t.Errorf("expected error for input %q", tt.input)
	225	+	if tt.valid {
	226	+	require.NoError(t, err, "input: %q", tt.input)
	227	+	} else {
	228	+	require.Error(t, err, "input: %q", tt.input)
227	229		}
228	230		})
229	231		}

 import (
 	"strings"
 	"testing"
+	"github.com/stretchr/testify/require"
 )
 func TestValidateRepositoryName(t *testing.T) {
 		{"carriage return", "example\r.com", false},
 		{"empty", "", false},
 		{"double dot in middle", "foo..bar", false},
+		{"forward slash", "example/com", false},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			err := ValidateDomainParam(tt.input)
-			if tt.valid && err != nil {
+			if tt.valid {
-				t.Errorf("expected valid, got error: %v", err)
+				require.NoError(t, err, "input: %q", tt.input)
-			}
+			} else {
-			if !tt.valid && err == nil {
+				require.Error(t, err, "input: %q", tt.input)
-				t.Errorf("expected error for input %q", tt.input)
 			}
 		})
 	}