fix(admin): add image validation to PUT endpoint and improve error handling
- commit
- 5d5d9c2da5e2d355cd5c124edf6e849da5be3d36
- author
- bnema · · 2026-01-28 20:58 UTC
- parent
- 0df4d83
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
3 files · +30 -18
M internal/adapters/in/http/admin/handler.go +22 -2
| @@ -282,8 +282,13 @@ func (h *Handler) handleRoutesPost(w http.ResponseWriter, r *http.Request) { | |||
| 282 | 282 | if h.registrySvc != nil { | |
| 283 | 283 | imageName, imageRef := validation.ParseImageReference(route.Image) | |
| 284 | 284 | if _, err := h.registrySvc.GetManifest(ctx, imageName, imageRef); err != nil { | |
| 285 | - | log.Error().Err(err).Str("image", route.Image).Msg("image not found in registry") | |
| 286 | - | h.sendError(w, http.StatusBadRequest, fmt.Sprintf("image '%s' not found in Gordon's registry. Please push it first using: gordon push %s", route.Image, route.Image)) | |
| 285 | + | if errors.Is(err, domain.ErrManifestNotFound) { | |
| 286 | + | log.Error().Err(err).Str("image", route.Image).Msg("image not found in registry") | |
| 287 | + | h.sendError(w, http.StatusBadRequest, fmt.Sprintf("image '%s' not found in Gordon's registry. Please push it first using: gordon push %s", route.Image, route.Image)) | |
| 288 | + | } else { | |
| 289 | + | log.Error().Err(err).Str("image", route.Image).Msg("failed to check image in registry") | |
| 290 | + | h.sendError(w, http.StatusServiceUnavailable, "failed to verify image in registry") | |
| 291 | + | } | |
| 287 | 292 | return | |
| 288 | 293 | } | |
| 289 | 294 | } | |
| @@ -330,6 +335,21 @@ func (h *Handler) handleRoutesPut(w http.ResponseWriter, r *http.Request, routeD | |||
| 330 | 335 | ||
| 331 | 336 | route.Domain = routeDomain | |
| 332 | 337 | ||
| 338 | + | // Validate image exists in registry before updating route | |
| 339 | + | if h.registrySvc != nil && route.Image != "" { | |
| 340 | + | imageName, imageRef := validation.ParseImageReference(route.Image) | |
| 341 | + | if _, err := h.registrySvc.GetManifest(ctx, imageName, imageRef); err != nil { | |
| 342 | + | if errors.Is(err, domain.ErrManifestNotFound) { | |
| 343 | + | log.Error().Err(err).Str("image", route.Image).Msg("image not found in registry") | |
| 344 | + | h.sendError(w, http.StatusBadRequest, fmt.Sprintf("image '%s' not found in Gordon's registry. Please push it first using: gordon push %s", route.Image, route.Image)) | |
| 345 | + | } else { | |
| 346 | + | log.Error().Err(err).Str("image", route.Image).Msg("failed to check image in registry") | |
| 347 | + | h.sendError(w, http.StatusServiceUnavailable, "failed to verify image in registry") | |
| 348 | + | } | |
| 349 | + | return | |
| 350 | + | } | |
| 351 | + | } | |
| 352 | + | ||
| 333 | 353 | if err := h.configSvc.UpdateRoute(ctx, route); err != nil { | |
| 334 | 354 | log.Error().Err(err).Str("domain", routeDomain).Msg("failed to update route") | |
| 335 | 355 | switch { | |
| @@ -282,8 +282,13 @@ func (h *Handler) handleRoutesPost(w http.ResponseWriter, r *http.Request) { | |||||
| 282 | if h.registrySvc != nil { | 282 | if h.registrySvc != nil { | ||
| 283 | imageName, imageRef := validation.ParseImageReference(route.Image) | 283 | imageName, imageRef := validation.ParseImageReference(route.Image) | ||
| 284 | if _, err := h.registrySvc.GetManifest(ctx, imageName, imageRef); err != nil { | 284 | if _, err := h.registrySvc.GetManifest(ctx, imageName, imageRef); err != nil { | ||
| 285 | - | log.Error().Err(err).Str("image", route.Image).Msg("image not found in registry") | 285 | + | if errors.Is(err, domain.ErrManifestNotFound) { |
| 286 | - | h.sendError(w, http.StatusBadRequest, fmt.Sprintf("image '%s' not found in Gordon's registry. Please push it first using: gordon push %s", route.Image, route.Image)) | 286 | + | log.Error().Err(err).Str("image", route.Image).Msg("image not found in registry") |
| 287 | + | h.sendError(w, http.StatusBadRequest, fmt.Sprintf("image '%s' not found in Gordon's registry. Please push it first using: gordon push %s", route.Image, route.Image)) | |||
| 288 | + | } else { | |||
| 289 | + | log.Error().Err(err).Str("image", route.Image).Msg("failed to check image in registry") | |||
| 290 | + | h.sendError(w, http.StatusServiceUnavailable, "failed to verify image in registry") | |||
| 291 | + | } | |||
| 287 | return | 292 | return | ||
| 288 | } | 293 | } | ||
| 289 | } | 294 | } | ||
| @@ -330,6 +335,21 @@ func (h *Handler) handleRoutesPut(w http.ResponseWriter, r *http.Request, routeD | |||||
| 330 | 335 | ||||
| 331 | route.Domain = routeDomain | 336 | route.Domain = routeDomain | ||
| 332 | 337 | ||||
| 338 | + | // Validate image exists in registry before updating route | |||
| 339 | + | if h.registrySvc != nil && route.Image != "" { | |||
| 340 | + | imageName, imageRef := validation.ParseImageReference(route.Image) | |||
| 341 | + | if _, err := h.registrySvc.GetManifest(ctx, imageName, imageRef); err != nil { | |||
| 342 | + | if errors.Is(err, domain.ErrManifestNotFound) { | |||
| 343 | + | log.Error().Err(err).Str("image", route.Image).Msg("image not found in registry") | |||
| 344 | + | h.sendError(w, http.StatusBadRequest, fmt.Sprintf("image '%s' not found in Gordon's registry. Please push it first using: gordon push %s", route.Image, route.Image)) | |||
| 345 | + | } else { | |||
| 346 | + | log.Error().Err(err).Str("image", route.Image).Msg("failed to check image in registry") | |||
| 347 | + | h.sendError(w, http.StatusServiceUnavailable, "failed to verify image in registry") | |||
| 348 | + | } | |||
| 349 | + | return | |||
| 350 | + | } | |||
| 351 | + | } | |||
| 352 | + | ||||
| 333 | if err := h.configSvc.UpdateRoute(ctx, route); err != nil { | 353 | if err := h.configSvc.UpdateRoute(ctx, route); err != nil { | ||
| 334 | log.Error().Err(err).Str("domain", routeDomain).Msg("failed to update route") | 354 | log.Error().Err(err).Str("domain", routeDomain).Msg("failed to update route") | ||
| 335 | switch { | 355 | switch { | ||
M internal/adapters/in/http/admin/handler_test.go +2 -1
| @@ -183,6 +183,7 @@ func TestHandler_RoutesPut_RequiresWriteScope(t *testing.T) { | |||
| 183 | 183 | t.Run(tt.name, func(t *testing.T) { | |
| 184 | 184 | if tt.wantStatus == http.StatusOK { | |
| 185 | 185 | configSvc.EXPECT().UpdateRoute(mock.Anything, mock.AnythingOfType("domain.Route")).Return(nil).Maybe() | |
| 186 | + | registrySvc.EXPECT().GetManifest(mock.Anything, "myapp", "v2").Return(nil, nil).Maybe() | |
| 186 | 187 | } | |
| 187 | 188 | ||
| 188 | 189 | req := httptest.NewRequest("PUT", "/admin/routes/app.example.com", bytes.NewBufferString(routeJSON)) | |
| @@ -728,7 +729,7 @@ func TestHandler_RoutesPost_ImageNotFound(t *testing.T) { | |||
| 728 | 729 | ||
| 729 | 730 | for _, tt := range tests { | |
| 730 | 731 | t.Run(tt.name, func(t *testing.T) { | |
| 731 | - | registrySvc.EXPECT().GetManifest(mock.Anything, tt.manifestName, tt.manifestRef).Return(nil, fmt.Errorf("manifest not found")) | |
| 732 | + | registrySvc.EXPECT().GetManifest(mock.Anything, tt.manifestName, tt.manifestRef).Return(nil, domain.ErrManifestNotFound) | |
| 732 | 733 | ||
| 733 | 734 | req := httptest.NewRequest("POST", "/admin/routes", bytes.NewBufferString(tt.body)) | |
| 734 | 735 | req = req.WithContext(ctxWithScopes("admin:routes:write")) | |
| @@ -183,6 +183,7 @@ func TestHandler_RoutesPut_RequiresWriteScope(t *testing.T) { | |||||
| 183 | t.Run(tt.name, func(t *testing.T) { | 183 | t.Run(tt.name, func(t *testing.T) { | ||
| 184 | if tt.wantStatus == http.StatusOK { | 184 | if tt.wantStatus == http.StatusOK { | ||
| 185 | configSvc.EXPECT().UpdateRoute(mock.Anything, mock.AnythingOfType("domain.Route")).Return(nil).Maybe() | 185 | configSvc.EXPECT().UpdateRoute(mock.Anything, mock.AnythingOfType("domain.Route")).Return(nil).Maybe() | ||
| 186 | + | registrySvc.EXPECT().GetManifest(mock.Anything, "myapp", "v2").Return(nil, nil).Maybe() | |||
| 186 | } | 187 | } | ||
| 187 | 188 | ||||
| 188 | req := httptest.NewRequest("PUT", "/admin/routes/app.example.com", bytes.NewBufferString(routeJSON)) | 189 | req := httptest.NewRequest("PUT", "/admin/routes/app.example.com", bytes.NewBufferString(routeJSON)) | ||
| @@ -728,7 +729,7 @@ func TestHandler_RoutesPost_ImageNotFound(t *testing.T) { | |||||
| 728 | 729 | ||||
| 729 | for _, tt := range tests { | 730 | for _, tt := range tests { | ||
| 730 | t.Run(tt.name, func(t *testing.T) { | 731 | t.Run(tt.name, func(t *testing.T) { | ||
| 731 | - | registrySvc.EXPECT().GetManifest(mock.Anything, tt.manifestName, tt.manifestRef).Return(nil, fmt.Errorf("manifest not found")) | 732 | + | registrySvc.EXPECT().GetManifest(mock.Anything, tt.manifestName, tt.manifestRef).Return(nil, domain.ErrManifestNotFound) |
| 732 | 733 | ||||
| 733 | req := httptest.NewRequest("POST", "/admin/routes", bytes.NewBufferString(tt.body)) | 734 | req := httptest.NewRequest("POST", "/admin/routes", bytes.NewBufferString(tt.body)) | ||
| 734 | req = req.WithContext(ctxWithScopes("admin:routes:write")) | 735 | req = req.WithContext(ctxWithScopes("admin:routes:write")) | ||
M pkg/validation/image.go +6 -15
| @@ -1,5 +1,7 @@ | |||
| 1 | 1 | package validation | |
| 2 | 2 | ||
| 3 | + | import "strings" | |
| 4 | + | ||
| 3 | 5 | // ParseImageReference parses an image reference into name and tag/digest. | |
| 4 | 6 | // Supports formats: | |
| 5 | 7 | // - image:tag (default tag is "latest") | |
| @@ -13,18 +15,18 @@ package validation | |||
| 13 | 15 | // - reference: the tag (e.g., "latest") or digest (e.g., "sha256:...") | |
| 14 | 16 | func ParseImageReference(imageRef string) (string, string) { | |
| 15 | 17 | // Check for digest reference (@sha256:...) | |
| 16 | - | if idx := indexString(imageRef, "@sha256:"); idx != -1 { | |
| 18 | + | if idx := strings.Index(imageRef, "@sha256:"); idx != -1 { | |
| 17 | 19 | return imageRef[:idx], imageRef[idx+1:] // Return name and full digest | |
| 18 | 20 | } | |
| 19 | 21 | ||
| 20 | 22 | // Check for tag reference (:tag) | |
| 21 | - | if idx := indexString(imageRef, ":"); idx != -1 { | |
| 23 | + | if idx := strings.Index(imageRef, ":"); idx != -1 { | |
| 22 | 24 | // Check if this is actually a port number (e.g., registry:5000/image) | |
| 23 | 25 | // by looking for a slash before the colon | |
| 24 | - | if slashIdx := indexString(imageRef, "/"); slashIdx != -1 && slashIdx > idx { | |
| 26 | + | if slashIdx := strings.Index(imageRef, "/"); slashIdx != -1 && slashIdx > idx { | |
| 25 | 27 | // Slash comes after colon, so colon is part of registry port | |
| 26 | 28 | // Look for another colon for the tag | |
| 27 | - | if tagIdx := indexString(imageRef[slashIdx:], ":"); tagIdx != -1 { | |
| 29 | + | if tagIdx := strings.Index(imageRef[slashIdx:], ":"); tagIdx != -1 { | |
| 28 | 30 | actualTagIdx := slashIdx + tagIdx | |
| 29 | 31 | return imageRef[:actualTagIdx], imageRef[actualTagIdx+1:] | |
| 30 | 32 | } | |
| @@ -36,14 +38,3 @@ func ParseImageReference(imageRef string) (string, string) { | |||
| 36 | 38 | // Default to "latest" tag | |
| 37 | 39 | return imageRef, "latest" | |
| 38 | 40 | } | |
| 39 | - | ||
| 40 | - | // indexString returns the index of the first occurrence of substr in s, or -1 if not found. | |
| 41 | - | // This is a helper function that mirrors strings.Index for testing purposes. | |
| 42 | - | func indexString(s, substr string) int { | |
| 43 | - | for i := 0; i <= len(s)-len(substr); i++ { | |
| 44 | - | if s[i:i+len(substr)] == substr { | |
| 45 | - | return i | |
| 46 | - | } | |
| 47 | - | } | |
| 48 | - | return -1 | |
| 49 | - | } | |
| @@ -1,5 +1,7 @@ | |||||
| 1 | package validation | 1 | package validation | ||
| 2 | 2 | ||||
| 3 | + | import "strings" | |||
| 4 | + | ||||
| 3 | // ParseImageReference parses an image reference into name and tag/digest. | 5 | // ParseImageReference parses an image reference into name and tag/digest. | ||
| 4 | // Supports formats: | 6 | // Supports formats: | ||
| 5 | // - image:tag (default tag is "latest") | 7 | // - image:tag (default tag is "latest") | ||
| @@ -13,18 +15,18 @@ package validation | |||||
| 13 | // - reference: the tag (e.g., "latest") or digest (e.g., "sha256:...") | 15 | // - reference: the tag (e.g., "latest") or digest (e.g., "sha256:...") | ||
| 14 | func ParseImageReference(imageRef string) (string, string) { | 16 | func ParseImageReference(imageRef string) (string, string) { | ||
| 15 | // Check for digest reference (@sha256:...) | 17 | // Check for digest reference (@sha256:...) | ||
| 16 | - | if idx := indexString(imageRef, "@sha256:"); idx != -1 { | 18 | + | if idx := strings.Index(imageRef, "@sha256:"); idx != -1 { |
| 17 | return imageRef[:idx], imageRef[idx+1:] // Return name and full digest | 19 | return imageRef[:idx], imageRef[idx+1:] // Return name and full digest | ||
| 18 | } | 20 | } | ||
| 19 | 21 | ||||
| 20 | // Check for tag reference (:tag) | 22 | // Check for tag reference (:tag) | ||
| 21 | - | if idx := indexString(imageRef, ":"); idx != -1 { | 23 | + | if idx := strings.Index(imageRef, ":"); idx != -1 { |
| 22 | // Check if this is actually a port number (e.g., registry:5000/image) | 24 | // Check if this is actually a port number (e.g., registry:5000/image) | ||
| 23 | // by looking for a slash before the colon | 25 | // by looking for a slash before the colon | ||
| 24 | - | if slashIdx := indexString(imageRef, "/"); slashIdx != -1 && slashIdx > idx { | 26 | + | if slashIdx := strings.Index(imageRef, "/"); slashIdx != -1 && slashIdx > idx { |
| 25 | // Slash comes after colon, so colon is part of registry port | 27 | // Slash comes after colon, so colon is part of registry port | ||
| 26 | // Look for another colon for the tag | 28 | // Look for another colon for the tag | ||
| 27 | - | if tagIdx := indexString(imageRef[slashIdx:], ":"); tagIdx != -1 { | 29 | + | if tagIdx := strings.Index(imageRef[slashIdx:], ":"); tagIdx != -1 { |
| 28 | actualTagIdx := slashIdx + tagIdx | 30 | actualTagIdx := slashIdx + tagIdx | ||
| 29 | return imageRef[:actualTagIdx], imageRef[actualTagIdx+1:] | 31 | return imageRef[:actualTagIdx], imageRef[actualTagIdx+1:] | ||
| 30 | } | 32 | } | ||
| @@ -36,14 +38,3 @@ func ParseImageReference(imageRef string) (string, string) { | |||||
| 36 | // Default to "latest" tag | 38 | // Default to "latest" tag | ||
| 37 | return imageRef, "latest" | 39 | return imageRef, "latest" | ||
| 38 | } | 40 | } | ||
| 39 | - | ||||
| 40 | - | // indexString returns the index of the first occurrence of substr in s, or -1 if not found. | |||
| 41 | - | // This is a helper function that mirrors strings.Index for testing purposes. | |||
| 42 | - | func indexString(s, substr string) int { | |||
| 43 | - | for i := 0; i <= len(s)-len(substr); i++ { | |||
| 44 | - | if s[i:i+len(substr)] == substr { | |||
| 45 | - | return i | |||
| 46 | - | } | |||
| 47 | - | } | |||
| 48 | - | return -1 | |||
| 49 | - | } | |||