fix(secrets): address all Copilot review comments

- Add 'gordon-' prefix to resolveContainerName to match actual container naming
- Update test expectations to include 'gordon-' prefix in container names
- Fix documentation: use 'gordon deploy' instead of 'gordon routes deploy' (3 locations)
- Validate attachment path segments strictly: reject paths with extra parts
- Update ErrInvalidServiceName message to mention all validation rules
- Improve error handling for invalid attachment DELETE paths

This completes all 7 Copilot review comments.

commit: 71f548904c144fb56adf1d444ca6cbff095cce7a
author: brice · 8mo ago · 2026-02-06 12:51 UTC
parent: bb9656b
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

5 files · +34 -17

M docs/cli/secrets.md +2 -2


@@ -183,7 +183,7 @@ gordon secrets set myapp.example.com DATABASE_URL "$DATABASE_URL"
183	183		gordon secrets set myapp.example.com API_KEY "$API_KEY"
184	184
185	185		# Deploy
186		-	gordon routes deploy myapp.example.com
	186	+	gordon deploy myapp.example.com
187	187		```
188	188
189	189		### Rotating Secrets

@@ -196,7 +196,7 @@ NEW_JWT_SECRET=$(openssl rand -base64 32)
196	196		gordon secrets set myapp.local JWT_SECRET "$NEW_JWT_SECRET"
197	197
198	198		# Redeploy to pick up new secret
199		-	gordon routes deploy myapp.local
	199	+	gordon deploy myapp.local
200	200		```
201	201
202	202		### Attachment Secrets


@@ -183,7 +183,7 @@ gordon secrets set myapp.example.com DATABASE_URL "$DATABASE_URL"
183		gordon secrets set myapp.example.com API_KEY "$API_KEY"	183		gordon secrets set myapp.example.com API_KEY "$API_KEY"
184			184
185		# Deploy	185		# Deploy
186	-	gordon routes deploy myapp.example.com	186	+	gordon deploy myapp.example.com
187		```	187		```
188			188
189		### Rotating Secrets	189		### Rotating Secrets

@@ -196,7 +196,7 @@ NEW_JWT_SECRET=$(openssl rand -base64 32)
196		gordon secrets set myapp.local JWT_SECRET "$NEW_JWT_SECRET"	196		gordon secrets set myapp.local JWT_SECRET "$NEW_JWT_SECRET"
197			197
198		# Redeploy to pick up new secret	198		# Redeploy to pick up new secret
199	-	gordon routes deploy myapp.local	199	+	gordon deploy myapp.local
200		```	200		```
201			201
202		### Attachment Secrets	202		### Attachment Secrets

M internal/adapters/in/http/admin/handler.go +24 -7


@@ -455,14 +455,31 @@ func (h Handler) handleSecrets(w http.ResponseWriter, r http.Request, path str
455	455		secretDomain := parts[0]
456	456
457	457		// Check for attachment sub-path: /secrets/{domain}/attachments/{service}[/{key}]
458		-	if len(parts) >= 3 && parts[1] == "attachments" {
459		-	service := parts[2]
460		-	attachmentKey := ""
461		-	if len(parts) > 3 {
462		-	attachmentKey = parts[3]
	458	+	if len(parts) >= 2 && parts[1] == "attachments" {
	459	+	switch r.Method {
	460	+	case http.MethodPost:
	461	+	// Expected path: /secrets/{domain}/attachments/{service}
	462	+	if len(parts) != 3 {
	463	+	h.sendError(w, http.StatusBadRequest, "invalid attachment path: expected /secrets/{domain}/attachments/{service}")
	464	+	return
	465	+	}
	466	+	service := parts[2]
	467	+	h.handleAttachmentSecrets(w, r, secretDomain, service, "")
	468	+	return
	469	+	case http.MethodDelete:
	470	+	// Expected path: /secrets/{domain}/attachments/{service}/{key}
	471	+	if len(parts) != 4 {
	472	+	h.sendError(w, http.StatusBadRequest, "invalid attachment path: expected /secrets/{domain}/attachments/{service}/{key}")
	473	+	return
	474	+	}
	475	+	service := parts[2]
	476	+	attachmentKey := parts[3]
	477	+	h.handleAttachmentSecrets(w, r, secretDomain, service, attachmentKey)
	478	+	return
	479	+	default:
	480	+	h.sendError(w, http.StatusMethodNotAllowed, "method not allowed")
	481	+	return
463	482		}
464		-	h.handleAttachmentSecrets(w, r, secretDomain, service, attachmentKey)
465		-	return
466	483		}
467	484
468	485		secretKey := ""


@@ -455,14 +455,31 @@ func (h Handler) handleSecrets(w http.ResponseWriter, r http.Request, path str
455		secretDomain := parts[0]	455		secretDomain := parts[0]
456			456
457		// Check for attachment sub-path: /secrets/{domain}/attachments/{service}[/{key}]	457		// Check for attachment sub-path: /secrets/{domain}/attachments/{service}[/{key}]
458	-	if len(parts) >= 3 && parts[1] == "attachments" {	458	+	if len(parts) >= 2 && parts[1] == "attachments" {
459	-	service := parts[2]	459	+	switch r.Method {
460	-	attachmentKey := ""	460	+	case http.MethodPost:
461	-	if len(parts) > 3 {	461	+	// Expected path: /secrets/{domain}/attachments/{service}
462	-	attachmentKey = parts[3]	462	+	if len(parts) != 3 {
			463	+	h.sendError(w, http.StatusBadRequest, "invalid attachment path: expected /secrets/{domain}/attachments/{service}")
			464	+	return
			465	+	}
			466	+	service := parts[2]
			467	+	h.handleAttachmentSecrets(w, r, secretDomain, service, "")
			468	+	return
			469	+	case http.MethodDelete:
			470	+	// Expected path: /secrets/{domain}/attachments/{service}/{key}
			471	+	if len(parts) != 4 {
			472	+	h.sendError(w, http.StatusBadRequest, "invalid attachment path: expected /secrets/{domain}/attachments/{service}/{key}")
			473	+	return
			474	+	}
			475	+	service := parts[2]
			476	+	attachmentKey := parts[3]
			477	+	h.handleAttachmentSecrets(w, r, secretDomain, service, attachmentKey)
			478	+	return
			479	+	default:
			480	+	h.sendError(w, http.StatusMethodNotAllowed, "method not allowed")
			481	+	return
463		}	482		}
464	-	h.handleAttachmentSecrets(w, r, secretDomain, service, attachmentKey)
465	-	return
466		}	483		}
467			484
468		secretKey := ""	485		secretKey := ""

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


@@ -1143,7 +1143,7 @@ func TestHandler_AttachmentSecretsDelete_RequiresKey(t *testing.T) {
1143	1143		handler.ServeHTTP(rec, req)
1144	1144
1145	1145		assert.Equal(t, http.StatusBadRequest, rec.Code)
1146		-	assert.Contains(t, rec.Body.String(), "key required")
	1146	+	assert.Contains(t, rec.Body.String(), "invalid attachment path")
1147	1147		}
1148	1148
1149	1149		func TestHandler_AttachmentSecretsPost_InvalidJSON(t *testing.T) {


@@ -1143,7 +1143,7 @@ func TestHandler_AttachmentSecretsDelete_RequiresKey(t *testing.T) {
1143		handler.ServeHTTP(rec, req)	1143		handler.ServeHTTP(rec, req)
1144			1144
1145		assert.Equal(t, http.StatusBadRequest, rec.Code)	1145		assert.Equal(t, http.StatusBadRequest, rec.Code)
1146	-	assert.Contains(t, rec.Body.String(), "key required")	1146	+	assert.Contains(t, rec.Body.String(), "invalid attachment path")
1147		}	1147		}
1148			1148
1149		func TestHandler_AttachmentSecretsPost_InvalidJSON(t *testing.T) {	1149		func TestHandler_AttachmentSecretsPost_InvalidJSON(t *testing.T) {

M internal/usecase/secrets/service.go +5 -5


@@ -19,7 +19,7 @@ var (
19	19		ErrDomainPathTraversal = errors.New("domain contains path traversal sequence")
20	20		ErrDomainInvalidChars = errors.New("domain contains invalid characters")
21	21		ErrServiceEmpty = errors.New("service name cannot be empty")
22		-	ErrInvalidServiceName = errors.New("invalid service name: must start with a letter and contain only lowercase letters, numbers, and hyphens")
	22	+	ErrInvalidServiceName = errors.New("invalid service name: must start with a letter, contain only lowercase letters, numbers, and hyphens, be at most 63 characters, and not end with a hyphen")
23	23		)
24	24
25	25		// Service implements the SecretService interface.

@@ -263,14 +263,14 @@ func validateServiceName(service string) error {
263	263		// Ensures the total name does not exceed Docker's 255-character limit.
264	264		func resolveContainerName(domainName, service string) string {
265	265		sanitized := domain.SanitizeDomainForContainer(domainName)
266		-	name := sanitized + "-" + service
	266	+	name := "gordon-" + sanitized + "-" + service
267	267
268	268		// Docker container names are limited to 255 characters
269	269		if len(name) > 255 {
270		-	// Truncate the domain part to fit, keeping room for "-" and service name
271		-	maxDomainLen := 255 - 1 - len(service)
	270	+	// Truncate the domain part to fit, keeping room for "gordon-", "-", and service name
	271	+	maxDomainLen := 255 - 7 - 1 - len(service) // 7 for "gordon-", 1 for "-"
272	272		if maxDomainLen > 0 {
273		-	name = sanitized[:maxDomainLen] + "-" + service
	273	+	name = "gordon-" + sanitized[:maxDomainLen] + "-" + service
274	274		}
275	275		}
276	276		return name


@@ -19,7 +19,7 @@ var (
19		ErrDomainPathTraversal = errors.New("domain contains path traversal sequence")	19		ErrDomainPathTraversal = errors.New("domain contains path traversal sequence")
20		ErrDomainInvalidChars = errors.New("domain contains invalid characters")	20		ErrDomainInvalidChars = errors.New("domain contains invalid characters")
21		ErrServiceEmpty = errors.New("service name cannot be empty")	21		ErrServiceEmpty = errors.New("service name cannot be empty")
22	-	ErrInvalidServiceName = errors.New("invalid service name: must start with a letter and contain only lowercase letters, numbers, and hyphens")	22	+	ErrInvalidServiceName = errors.New("invalid service name: must start with a letter, contain only lowercase letters, numbers, and hyphens, be at most 63 characters, and not end with a hyphen")
23		)	23		)
24			24
25		// Service implements the SecretService interface.	25		// Service implements the SecretService interface.

@@ -263,14 +263,14 @@ func validateServiceName(service string) error {
263		// Ensures the total name does not exceed Docker's 255-character limit.	263		// Ensures the total name does not exceed Docker's 255-character limit.
264		func resolveContainerName(domainName, service string) string {	264		func resolveContainerName(domainName, service string) string {
265		sanitized := domain.SanitizeDomainForContainer(domainName)	265		sanitized := domain.SanitizeDomainForContainer(domainName)
266	-	name := sanitized + "-" + service	266	+	name := "gordon-" + sanitized + "-" + service
267			267
268		// Docker container names are limited to 255 characters	268		// Docker container names are limited to 255 characters
269		if len(name) > 255 {	269		if len(name) > 255 {
270	-	// Truncate the domain part to fit, keeping room for "-" and service name	270	+	// Truncate the domain part to fit, keeping room for "gordon-", "-", and service name
271	-	maxDomainLen := 255 - 1 - len(service)	271	+	maxDomainLen := 255 - 7 - 1 - len(service) // 7 for "gordon-", 1 for "-"
272		if maxDomainLen > 0 {	272		if maxDomainLen > 0 {
273	-	name = sanitized[:maxDomainLen] + "-" + service	273	+	name = "gordon-" + sanitized[:maxDomainLen] + "-" + service
274		}	274		}
275		}	275		}
276		return name	276		return name

M internal/usecase/secrets/service_test.go +2 -2


@@ -465,7 +465,7 @@ func TestService_SetAttachment(t *testing.T) {
465	465		svc := NewService(store, testLogger())
466	466
467	467		secrets := map[string]string{"POSTGRES_PASSWORD": "secret123"}
468		-	store.EXPECT().SetAttachment("app__example__com-postgres", secrets).Return(nil)
	468	+	store.EXPECT().SetAttachment("gordon-app__example__com-postgres", secrets).Return(nil)
469	469
470	470		err := svc.SetAttachment(context.Background(), "app.example.com", "postgres", secrets)
471	471		assert.NoError(t, err)

@@ -475,7 +475,7 @@ func TestService_DeleteAttachment(t *testing.T) {
475	475		store := outmocks.NewMockDomainSecretStore(t)
476	476		svc := NewService(store, testLogger())
477	477
478		-	store.EXPECT().DeleteAttachment("app__example__com-postgres", "OLD_KEY").Return(nil)
	478	+	store.EXPECT().DeleteAttachment("gordon-app__example__com-postgres", "OLD_KEY").Return(nil)
479	479
480	480		err := svc.DeleteAttachment(context.Background(), "app.example.com", "postgres", "OLD_KEY")
481	481		assert.NoError(t, err)


@@ -465,7 +465,7 @@ func TestService_SetAttachment(t *testing.T) {
465		svc := NewService(store, testLogger())	465		svc := NewService(store, testLogger())
466			466
467		secrets := map[string]string{"POSTGRES_PASSWORD": "secret123"}	467		secrets := map[string]string{"POSTGRES_PASSWORD": "secret123"}
468	-	store.EXPECT().SetAttachment("app__example__com-postgres", secrets).Return(nil)	468	+	store.EXPECT().SetAttachment("gordon-app__example__com-postgres", secrets).Return(nil)
469			469
470		err := svc.SetAttachment(context.Background(), "app.example.com", "postgres", secrets)	470		err := svc.SetAttachment(context.Background(), "app.example.com", "postgres", secrets)
471		assert.NoError(t, err)	471		assert.NoError(t, err)

@@ -475,7 +475,7 @@ func TestService_DeleteAttachment(t *testing.T) {
475		store := outmocks.NewMockDomainSecretStore(t)	475		store := outmocks.NewMockDomainSecretStore(t)
476		svc := NewService(store, testLogger())	476		svc := NewService(store, testLogger())
477			477
478	-	store.EXPECT().DeleteAttachment("app__example__com-postgres", "OLD_KEY").Return(nil)	478	+	store.EXPECT().DeleteAttachment("gordon-app__example__com-postgres", "OLD_KEY").Return(nil)
479			479
480		err := svc.DeleteAttachment(context.Background(), "app.example.com", "postgres", "OLD_KEY")	480		err := svc.DeleteAttachment(context.Background(), "app.example.com", "postgres", "OLD_KEY")
481		assert.NoError(t, err)	481		assert.NoError(t, err)