fix: remediate trivial security audit findings (#124)

spinifex/gateway/auth.go

@@ -308,10 +308,7 @@ func canonicalHeaderName(header string) string {
 
 // writeSigV4Error writes an EC2-compatible XML error response for authentication failures.
 func (gw *GatewayConfig) writeSigV4Error(w http.ResponseWriter, r *http.Request, errorCode string) {
-	requestID := r.Header.Get("X-Amz-Request-Id")
-	if requestID == "" {
-		requestID = uuid.NewString()
-	}
+	requestID := uuid.NewString()
 
 	errorMsg, exists := awserrors.ErrorLookup[errorCode]
 	if !exists {

spinifex/gateway/auth_test.go

@@ -1012,7 +1012,7 @@ func TestWriteSigV4Error_ResponseFormat(t *testing.T) {
 	}
 }
 
-func TestWriteSigV4Error_PreservesRequestID(t *testing.T) {
+func TestWriteSigV4Error_IgnoresClientRequestID(t *testing.T) {
 	gw := &GatewayConfig{DisableLogging: true, Region: testRegion}
 
 	w := httptest.NewRecorder()
@@ -1022,8 +1022,11 @@ func TestWriteSigV4Error_PreservesRequestID(t *testing.T) {
 	gw.writeSigV4Error(w, req, awserrors.ErrorIncompleteSignature)
 
 	body, _ := io.ReadAll(w.Result().Body)
-	if !strings.Contains(string(body), "<RequestID>test-request-id-123</RequestID>") {
-		t.Errorf("Expected preserved request ID, got: %s", string(body))
+	if strings.Contains(string(body), "test-request-id-123") {
+		t.Errorf("Expected server-generated request ID, but client ID was used: %s", string(body))
+	}
+	if !strings.Contains(string(body), "<RequestID>") {
+		t.Errorf("Expected RequestID in response, got: %s", string(body))
 	}
 }
 

spinifex/gateway/gateway.go

@@ -231,11 +231,8 @@ func (gw *GatewayConfig) ErrorHandler(w http.ResponseWriter, r *http.Request, er
 	svc, _ := gw.GetService(r)
 	slog.Debug("ErrorHandler", "service", svc, "error", err.Error())
 
-	// Get the request ID
+	// Generate a server-side request ID — never trust client-provided values
 	var requestId = uuid.NewString()
-	if rid := r.Header.Get("X-Amz-Request-Id"); rid != "" {
-		requestId = rid
-	}
 
 	var errorMsg = awserrors.ErrorMessage{}
 

spinifex/gateway/gateway_test.go

@@ -230,7 +230,7 @@ func TestErrorHandler_EC2Service(t *testing.T) {
 	assert.NotContains(t, xmlStr, "<ErrorResponse>")
 }
 
-func TestErrorHandler_UsesRequestIDHeader(t *testing.T) {
+func TestErrorHandler_IgnoresClientRequestID(t *testing.T) {
 	gw := &GatewayConfig{DisableLogging: true}
 
 	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -244,7 +244,8 @@ func TestErrorHandler_UsesRequestIDHeader(t *testing.T) {
 	resp := doRequest(handler, req)
 
 	body, _ := io.ReadAll(resp.Body)
-	assert.Contains(t, string(body), "custom-req-id-123")
+	assert.NotContains(t, string(body), "custom-req-id-123")
+	assert.Contains(t, string(body), "<RequestID>")
 }
 
 func TestErrorHandler_ContentTypeXML(t *testing.T) {

spinifex/handlers/ec2/vpc/security_group.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
+	"slices"
 	"strings"
 	"time"
 
@@ -325,6 +326,11 @@ func (s *VPCServiceImpl) AuthorizeSecurityGroupIngress(input *ec2.AuthorizeSecur
 
 	// Convert AWS IpPermissions to SGRules
 	newRules := ipPermissionsToSGRules(input.IpPermissions)
+	for _, nr := range newRules {
+		if slices.Contains(record.IngressRules, nr) {
+			return nil, errors.New(awserrors.ErrorInvalidPermissionDuplicate)
+		}
+	}
 	record.IngressRules = append(record.IngressRules, newRules...)
 
 	data, err := json.Marshal(record)
@@ -370,6 +376,11 @@ func (s *VPCServiceImpl) AuthorizeSecurityGroupEgress(input *ec2.AuthorizeSecuri
 	}
 
 	newRules := ipPermissionsToSGRules(input.IpPermissions)
+	for _, nr := range newRules {
+		if slices.Contains(record.EgressRules, nr) {
+			return nil, errors.New(awserrors.ErrorInvalidPermissionDuplicate)
+		}
+	}
 	record.EgressRules = append(record.EgressRules, newRules...)
 
 	data, err := json.Marshal(record)

spinifex/handlers/iam/service_impl.go

@@ -463,10 +463,8 @@ func (s *IAMServiceImpl) DeleteAccessKey(accountID string, input *iam.DeleteAcce
 		return nil, errors.New(awserrors.ErrorIAMNoSuchEntity)
 	}
 
-	if err := s.accessKeysBucket.Delete(accessKeyID); err != nil {
-		return nil, fmt.Errorf("delete access key: %w", err)
-	}
-
+	// Update user record first, then delete the key. This avoids orphaning the
+	// reference if a crash occurs between the two operations.
 	user.AccessKeys = remaining
 	userData, err := json.Marshal(user)
 	if err != nil {
@@ -477,6 +475,10 @@ func (s *IAMServiceImpl) DeleteAccessKey(accountID string, input *iam.DeleteAcce
 		return nil, fmt.Errorf("update user: %w", err)
 	}
 
+	if err := s.accessKeysBucket.Delete(accessKeyID); err != nil {
+		return nil, fmt.Errorf("delete access key: %w", err)
+	}
+
 	slog.Info("IAM access key deleted", "accountID", accountID, "userName", userName, "accessKeyID", accessKeyID)
 	return &iam.DeleteAccessKeyOutput{}, nil
 }

spinifex/utils/nats.go

@@ -77,6 +77,17 @@ func NATSRequest[Out any](conn *nats.Conn, subject string, input any, timeout ti
 	return &output, nil
 }
 
+const (
+	// maxScatterGatherResponseSize is the maximum allowed size for a single
+	// scatter-gather response payload (10 MB). Responses exceeding this are
+	// skipped to prevent OOM from buggy nodes.
+	maxScatterGatherResponseSize = 10 * 1024 * 1024
+
+	// maxScatterGatherUnboundedResponses is the hard cap on responses collected
+	// when expectedNodes is 0 (unbounded fan-out).
+	maxScatterGatherUnboundedResponses = 256
+)
+
 // NATSScatterGather publishes a fan-out request and collects responses from
 // multiple nodes. It returns the first successful (non-error) response. Error
 // payloads from individual nodes are skipped. If all responses are errors, the
@@ -108,8 +119,13 @@ func NATSScatterGather[Out any](conn *nats.Conn, subject string, input any, time
 	responsesReceived := 0
 	var lastErr error
 
+	maxResponses := maxScatterGatherUnboundedResponses
+	if expectedNodes > 0 {
+		maxResponses = expectedNodes
+	}
+
 	for time.Now().Before(deadline) {
-		if expectedNodes > 0 && responsesReceived >= expectedNodes {
+		if responsesReceived >= maxResponses {
 			break
 		}
 
@@ -126,6 +142,11 @@ func NATSScatterGather[Out any](conn *nats.Conn, subject string, input any, time
 			return nil, fmt.Errorf("scatter-gather receive error: %w", err)
 		}
 
+		if len(msg.Data) > maxScatterGatherResponseSize {
+			slog.Warn("ScatterGather: skipping oversized response", "subject", subject, "size", len(msg.Data))
+			continue
+		}
+
 		responsesReceived++
 
 		responseError, err := ValidateErrorPayload(msg.Data)