builds: address build-system review follow-ups

cmd/api/api/builds.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"mime/multipart"
 	"net/http"
 	"strconv"
 
@@ -17,6 +18,28 @@ import (
 	"github.com/kernel/hypeman/lib/tags"
 )
 
+var (
+	// maxBuildSourceSize bounds the source tarball accepted by POST /builds.
+	// A var so tests can exercise the limit without large uploads.
+	maxBuildSourceSize int64 = 512 << 20 // 512 MiB
+	// maxBuildFormFieldSize bounds each small non-source multipart field
+	// (dockerfile, secrets, tags, and friends).
+	maxBuildFormFieldSize int64 = 1 << 20 // 1 MiB
+)
+
+// readLimitedPart reads a multipart part fully, erroring when its contents
+// exceed limit bytes.
+func readLimitedPart(part *multipart.Part, limit int64) ([]byte, error) {
+	data, err := io.ReadAll(io.LimitReader(part, limit+1))
+	if err != nil {
+		return nil, fmt.Errorf("failed to read %s field: %w", part.FormName(), err)
+	}
+	if int64(len(data)) > limit {
+		return nil, fmt.Errorf("%s exceeds the maximum size of %d bytes", part.FormName(), limit)
+	}
+	return data, nil
+}
+
 // ListBuilds returns all builds
 func (s *ApiService) ListBuilds(ctx context.Context, request oapi.ListBuildsRequestObject) (oapi.ListBuildsResponseObject, error) {
 	log := logger.FromContext(ctx)
@@ -65,133 +88,61 @@ func (s *ApiService) CreateBuild(ctx context.Context, request oapi.CreateBuildRe
 			}, nil
 		}
 
-		switch part.FormName() {
-		case "source":
-			sourceData, err = io.ReadAll(part)
-			if err != nil {
-				return oapi.CreateBuild400JSONResponse{
-					Code:    "invalid_source",
-					Message: "failed to read source data",
-				}, nil
+		name := part.FormName()
+		limit := maxBuildFormFieldSize
+		if name == "source" {
+			limit = maxBuildSourceSize
+		}
+		data, err := readLimitedPart(part, limit)
+		part.Close()
+		if err != nil {
+			code := "invalid_request"
+			if name == "source" {
+				code = "invalid_source"
 			}
+			return oapi.CreateBuild400JSONResponse{
+				Code:    code,
+				Message: err.Error(),
+			}, nil
+		}
+
+		switch name {
+		case "source":
+			sourceData = data
 		case "base_image_digest":
-			data, err := io.ReadAll(part)
-			if err != nil {
-				return oapi.CreateBuild400JSONResponse{
-					Code:    "invalid_request",
-					Message: "failed to read base_image_digest field",
-				}, nil
-			}
 			baseImageDigest = string(data)
 		case "builder_id":
-			data, err := io.ReadAll(part)
-			if err != nil {
-				return oapi.CreateBuild400JSONResponse{
-					Code:    "invalid_request",
-					Message: "failed to read builder_id field",
-				}, nil
-			}
 			builderID = string(data)
 		case "cache_scope":
-			data, err := io.ReadAll(part)
-			if err != nil {
-				return oapi.CreateBuild400JSONResponse{
-					Code:    "invalid_request",
-					Message: "failed to read cache_scope field",
-				}, nil
-			}
 			cacheScope = string(data)
 		case "dockerfile":
-			data, err := io.ReadAll(part)
-			if err != nil {
-				return oapi.CreateBuild400JSONResponse{
-					Code:    "invalid_request",
-					Message: "failed to read dockerfile field",
-				}, nil
-			}
 			dockerfile = string(data)
 		case "timeout_seconds":
-			data, err := io.ReadAll(part)
-			if err != nil {
-				return oapi.CreateBuild400JSONResponse{
-					Code:    "invalid_request",
-					Message: "failed to read timeout_seconds field",
-				}, nil
-			}
 			if v, err := strconv.Atoi(string(data)); err == nil {
 				timeoutSeconds = v
 			}
 		case "memory_mb":
-			data, err := io.ReadAll(part)
-			if err != nil {
-				return oapi.CreateBuild400JSONResponse{
-					Code:    "invalid_request",
-					Message: "failed to read memory_mb field",
-				}, nil
-			}
 			if v, err := strconv.Atoi(string(data)); err == nil {
 				memoryMB = v
 			}
 		case "cpus":
-			data, err := io.ReadAll(part)
-			if err != nil {
-				return oapi.CreateBuild400JSONResponse{
-					Code:    "invalid_request",
-					Message: "failed to read cpus field",
-				}, nil
-			}
 			if v, err := strconv.Atoi(string(data)); err == nil {
 				cpus = v
 			}
 		case "secrets":
-			data, err := io.ReadAll(part)
-			if err != nil {
-				return oapi.CreateBuild400JSONResponse{
-					Code:    "invalid_request",
-					Message: "failed to read secrets field",
-				}, nil
-			}
 			if err := json.Unmarshal(data, &secrets); err != nil {
 				return oapi.CreateBuild400JSONResponse{
 					Code:    "invalid_request",
 					Message: "secrets must be a JSON array of {\"id\": \"...\", \"env_var\": \"...\"} objects",
 				}, nil
 			}
 		case "is_admin_build":
-			data, err := io.ReadAll(part)
-			if err != nil {
-				return oapi.CreateBuild400JSONResponse{
-					Code:    "invalid_request",
-					Message: "failed to read is_admin_build field",
-				}, nil
-			}
 			isAdminBuild = string(data) == "true" || string(data) == "1"
 		case "global_cache_key":
-			data, err := io.ReadAll(part)
-			if err != nil {
-				return oapi.CreateBuild400JSONResponse{
-					Code:    "invalid_request",
-					Message: "failed to read global_cache_key field",
-				}, nil
-			}
 			globalCacheKey = string(data)
 		case "image_name":
-			data, err := io.ReadAll(part)
-			if err != nil {
-				return oapi.CreateBuild400JSONResponse{
-					Code:    "invalid_request",
-					Message: "failed to read image_name field",
-				}, nil
-			}
 			imageName = string(data)
 		case "tags":
-			data, err := io.ReadAll(part)
-			if err != nil {
-				return oapi.CreateBuild400JSONResponse{
-					Code:    "invalid_request",
-					Message: "failed to read tags field",
-				}, nil
-			}
 			parsed, err := parseTagsJSON(string(data))
 			if err != nil {
 				return oapi.CreateBuild400JSONResponse{
@@ -201,7 +152,6 @@ func (s *ApiService) CreateBuild(ctx context.Context, request oapi.CreateBuildRe
 			}
 			resourceTags = parsed
 		}
-		part.Close()
 	}
 
 	if len(sourceData) == 0 {
@@ -211,6 +161,17 @@ func (s *ApiService) CreateBuild(ctx context.Context, request oapi.CreateBuildRe
 		}, nil
 	}
 
+	// Reject malformed secret IDs at the boundary so a bad reference fails
+	// the request instead of the build.
+	for _, secret := range secrets {
+		if err := builds.ValidateSecretID(secret.ID); err != nil {
+			return oapi.CreateBuild400JSONResponse{
+				Code:    "invalid_request",
+				Message: err.Error(),
+			}, nil
+		}
+	}
+
 	// Validate image_name early so the user gets a fast 400 instead of
 	// a successful build that silently falls back to builds/{id}.
 	if imageName != "" {

cmd/api/api/builds_test.go

@@ -0,0 +1,128 @@
+package api
+
+import (
+	"bytes"
+	"mime/multipart"
+	"testing"
+
+	"github.com/kernel/hypeman/lib/oapi"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// buildMultipartBody builds a POST /builds multipart body from small form
+// fields plus the source tarball part.
+func buildMultipartBody(t *testing.T, fields map[string]string, source []byte) *oapi.CreateBuildRequestObject {
+	t.Helper()
+
+	var body bytes.Buffer
+	writer := multipart.NewWriter(&body)
+	for k, v := range fields {
+		require.NoError(t, writer.WriteField(k, v))
+	}
+	if source != nil {
+		part, err := writer.CreateFormFile("source", "source.tar.gz")
+		require.NoError(t, err)
+		_, err = part.Write(source)
+		require.NoError(t, err)
+	}
+	require.NoError(t, writer.Close())
+
+	return &oapi.CreateBuildRequestObject{
+		Body: multipart.NewReader(&body, writer.Boundary()),
+	}
+}
+
+// overrideBuildUploadLimits shrinks the upload limits for the test and
+// restores them on cleanup. Tests using it must not run in parallel.
+func overrideBuildUploadLimits(t *testing.T, sourceSize, fieldSize int64) {
+	t.Helper()
+	origSource, origField := maxBuildSourceSize, maxBuildFormFieldSize
+	maxBuildSourceSize, maxBuildFormFieldSize = sourceSize, fieldSize
+	t.Cleanup(func() {
+		maxBuildSourceSize, maxBuildFormFieldSize = origSource, origField
+	})
+}
+
+func TestCreateBuild_SourceAtLimitAccepted(t *testing.T) {
+	svc := newTestService(t)
+	overrideBuildUploadLimits(t, 1024, 1024)
+
+	resp, err := svc.CreateBuild(ctx(), *buildMultipartBody(t, nil, bytes.Repeat([]byte("a"), 1024)))
+	require.NoError(t, err)
+	_, ok := resp.(oapi.CreateBuild202JSONResponse)
+	assert.True(t, ok, "expected 202 for source exactly at the limit, got %T", resp)
+
+	// Wait for the build goroutine to finish writing before TempDir cleanup.
+	require.NoError(t, svc.BuildManager.Shutdown(ctx()))
+}
+
+func TestCreateBuild_SourceOverLimitRejected(t *testing.T) {
+	svc := newTestService(t)
+	overrideBuildUploadLimits(t, 1024, 1024)
+
+	resp, err := svc.CreateBuild(ctx(), *buildMultipartBody(t, nil, bytes.Repeat([]byte("a"), 1025)))
+	require.NoError(t, err)
+	r, ok := resp.(oapi.CreateBuild400JSONResponse)
+	require.True(t, ok, "expected 400 for oversized source, got %T", resp)
+	assert.Equal(t, "invalid_source", r.Code)
+	assert.Contains(t, r.Message, "source exceeds the maximum size")
+}
+
+func TestCreateBuild_FormFieldAtLimitAccepted(t *testing.T) {
+	svc := newTestService(t)
+	overrideBuildUploadLimits(t, 1024, 1024)
+
+	fields := map[string]string{"dockerfile": string(bytes.Repeat([]byte("a"), 1024))}
+	resp, err := svc.CreateBuild(ctx(), *buildMultipartBody(t, fields, []byte("source")))
+	require.NoError(t, err)
+	_, ok := resp.(oapi.CreateBuild202JSONResponse)
+	assert.True(t, ok, "expected 202 for field exactly at the limit, got %T", resp)
+
+	// Wait for the build goroutine to finish writing before TempDir cleanup.
+	require.NoError(t, svc.BuildManager.Shutdown(ctx()))
+}
+
+func TestCreateBuild_FormFieldOverLimitRejected(t *testing.T) {
+	svc := newTestService(t)
+	overrideBuildUploadLimits(t, 1024, 1024)
+
+	fields := map[string]string{"dockerfile": string(bytes.Repeat([]byte("a"), 1025))}
+	resp, err := svc.CreateBuild(ctx(), *buildMultipartBody(t, fields, []byte("source")))
+	require.NoError(t, err)
+	r, ok := resp.(oapi.CreateBuild400JSONResponse)
+	require.True(t, ok, "expected 400 for oversized field, got %T", resp)
+	assert.Equal(t, "invalid_request", r.Code)
+	assert.Contains(t, r.Message, "dockerfile exceeds the maximum size")
+}
+
+func TestCreateBuild_InvalidSecretIDRejected(t *testing.T) {
+	svc := newTestService(t)
+
+	for _, secrets := range []string{
+		`[{"id": "../escape"}]`,
+		`[{"id": "a/b"}]`,
+		`[{"id": ""}]`,
+	} {
+		fields := map[string]string{"secrets": secrets}
+		resp, err := svc.CreateBuild(ctx(), *buildMultipartBody(t, fields, []byte("source")))
+		require.NoError(t, err)
+		r, ok := resp.(oapi.CreateBuild400JSONResponse)
+		require.True(t, ok, "expected 400 for secrets %s, got %T", secrets, resp)
+		assert.Equal(t, "invalid_request", r.Code)
+		assert.Contains(t, r.Message, "invalid secret id")
+	}
+}
+
+func TestCreateBuild_ValidSecretsAccepted(t *testing.T) {
+	svc := newTestService(t)
+
+	fields := map[string]string{"secrets": `[{"id": "npm_token", "env_var": "NPM_TOKEN"}]`}
+	resp, err := svc.CreateBuild(ctx(), *buildMultipartBody(t, fields, []byte("source")))
+	require.NoError(t, err)
+	_, ok := resp.(oapi.CreateBuild202JSONResponse)
+	assert.True(t, ok, "expected 202 for valid secrets, got %T", resp)
+
+	// Wait for the build goroutine to finish writing before TempDir cleanup.
+	require.NoError(t, svc.BuildManager.Shutdown(ctx()))
+}

cmd/api/main.go

@@ -695,6 +695,16 @@ func run() error {
 			logger.Info("ingress manager shutdown complete")
 		}
 
+		// Cancel in-flight builds and wait for their goroutines to return.
+		// Interrupted builds keep their status and pending builds stay
+		// queued on disk; both are recovered on next start.
+		if err := app.BuildManager.Shutdown(shutdownCtx); err != nil {
+			logger.Error("failed to shutdown build manager", "error", err)
+			// Don't return error - continue with shutdown
+		} else {
+			logger.Info("build manager shutdown complete")
+		}
+
 		return errors.Join(shutdownErrs...)
 	})
 

lib/builds/README.md

@@ -242,7 +242,7 @@ queued → building → pushing → ready
 
 1. **Isolation**: Each build runs in a fresh microVM (Cloud Hypervisor)
 2. **Rootless**: BuildKit runs without root privileges
-3. **Network Control**: `network_mode: isolated` or `egress` with optional domain allowlist
+3. **Network Control**: `network_mode: isolated` or `egress` (outbound allowed)
 4. **Secret Handling**: Secrets fetched via vsock, never written to disk in guest
 5. **Cache Isolation**: Per-tenant cache scopes prevent cross-tenant cache poisoning
 6. **Registry Auth**: Short-lived JWT tokens scoped to specific repositories (builds/{id}, cache/{scope})