fix(crafter): surface DNS-1123 validation errors instead of masking them

pkg/attestation/crafter/crafter.go

@@ -22,6 +22,7 @@ import (
 	"net/url"
 	"os"
 	"reflect"
+	"regexp"
 	"slices"
 	"strings"
 	"time"
@@ -87,6 +88,9 @@ type VersionedCraftingState struct {
 
 var ErrAttestationStateNotLoaded = errors.New("crafting state not loaded")
 
+// dns1123Regex matches valid DNS-1123 label names (same rule as the proto CEL constraint).
+var dns1123Regex = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`)
+
 type NewOpt func(c *Crafter) error
 
 func WithAuthRawToken(token string) NewOpt {
@@ -578,6 +582,13 @@ func (c *Crafter) AddMaterialContractFree(ctx context.Context, attestationID, ki
 		m.Name = fmt.Sprintf("material-%d", time.Now().UnixNano())
 	}
 
+	// 2.2 - Validate the material name is DNS-1123 compliant before proceeding.
+	// This catches invalid names early instead of letting them be masked by the
+	// auto-discovery loop which swallows validation errors while probing kinds.
+	if !dns1123Regex.MatchString(m.Name) {
+		return nil, fmt.Errorf("invalid material name %q: must be DNS-1123 compliant (lowercase alphanumeric and hyphens only)", m.Name)
+	}
+
 	// 3 - Craft resulting material
 	return c.addMaterial(ctx, &m, attestationID, value, casBackend, runtimeAnnotations)
 }
@@ -628,9 +639,12 @@ func (c *Crafter) IsMaterialInContract(key string) bool {
 // AddMaterialContactFreeWithAutoDetectedKind adds a material to the crafting state checking the incoming material matches any of the
 // supported types in validation order. If the material is not found it will return an error.
 func (c *Crafter) AddMaterialContactFreeWithAutoDetectedKind(ctx context.Context, attestationID, name, value string, casBackend *casclient.CASBackend, runtimeAnnotations map[string]string) (*api.Attestation_Material, error) {
-	var err error
+	var (
+		err error
+		m   *api.Attestation_Material
+	)
 	for _, kind := range schemaapi.CraftingMaterialInValidationOrder {
-		m, err := c.AddMaterialContractFree(ctx, attestationID, kind.String(), name, value, casBackend, runtimeAnnotations)
+		m, err = c.AddMaterialContractFree(ctx, attestationID, kind.String(), name, value, casBackend, runtimeAnnotations)
 		if err == nil {
 			// Successfully added material, return the kind
 			return m, nil
@@ -639,7 +653,6 @@ func (c *Crafter) AddMaterialContactFreeWithAutoDetectedKind(ctx context.Context
 		c.Logger.Debug().Err(err).Str("kind", kind.String()).Msg("failed to add material")
 
 		// Handle base error for upload and craft errors except the opening file error
-		// TODO: have an error to detect validation error instead
 		var policyError *policies.PolicyError
 		if errors.Is(err, materials.ErrBaseUploadAndCraft) || errors.As(err, &policyError) {
 			return nil, err
@@ -649,6 +662,14 @@ func (c *Crafter) AddMaterialContactFreeWithAutoDetectedKind(ctx context.Context
 		if v1.IsAttestationStateErrorConflict(err) {
 			return nil, err
 		}
+
+		// Proto-validation errors (e.g. invalid material name) are schema-level
+		// failures, not kind mismatches. Stop probing immediately so the real
+		// error is surfaced to the user instead of being masked by the loop.
+		var valErr *protovalidate.ValidationError
+		if errors.As(err, &valErr) {
+			return nil, err
+		}
 	}
 
 	// Return an error if no material could be added

pkg/attestation/crafter/crafter_test.go

@@ -539,6 +539,7 @@ func (s *crafterSuite) TestAddMaterialsAutomatic() {
 		expectedType   schemaapi.CraftingSchema_Material_MaterialType
 		uploadArtifact bool
 		wantErr        bool
+		wantErrMsg     string
 	}{
 		{
 			name:         "sarif",
@@ -593,6 +594,36 @@ func (s *crafterSuite) TestAddMaterialsAutomatic() {
 			wantErr:        true,
 			uploadArtifact: true,
 		},
+		{
+			name:           "non-dns-1123 name surfaces clear error",
+			materialName:   "My_Invalid.Name",
+			materialPath:   "./materials/testdata/report.sarif",
+			wantErr:        true,
+			wantErrMsg:     "must be DNS-1123 compliant",
+			uploadArtifact: true, // no uploader interaction expected
+		},
+		{
+			name:           "uppercase name surfaces dns-1123 error",
+			materialName:   "MyMaterial",
+			materialPath:   "random-string",
+			wantErr:        true,
+			wantErrMsg:     "must be DNS-1123 compliant",
+			uploadArtifact: true,
+		},
+		{
+			// This test exercises the auto-discovery loop with a valid DNS-1123 name
+			// but content (empty string) that every material kind rejects during Craft.
+			// File-based kinds fail because empty string is not a file path. The STRING kind
+			// crafts successfully but fails proto validation (KeyVal.Value min_len=1).
+			// This verifies the shadowing fix (= instead of :=) works: the error from
+			// the loop is properly propagated instead of being nil.
+			name:           "valid dns name with empty value surfaces crafting error",
+			materialName:   "my-material",
+			materialPath:   "",
+			wantErr:        true,
+			wantErrMsg:     "validation error",
+			uploadArtifact: true,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -613,7 +644,7 @@ func (s *crafterSuite) TestAddMaterialsAutomatic() {
 
 			// Establishing a maximum size for the artifact to be uploaded to the CAS causes an error
 			// if the value is exceeded
-			if tc.wantErr {
+			if tc.wantErr && tc.wantErrMsg == "" {
 				backend.MaxSize = 1
 			}
 
@@ -622,10 +653,15 @@ func (s *crafterSuite) TestAddMaterialsAutomatic() {
 
 			m, err := c.AddMaterialContactFreeWithAutoDetectedKind(context.Background(), "random-id", tc.materialName, tc.materialPath, backend, nil)
 			if tc.wantErr {
-				assert.ErrorIs(s.T(), err, materials.ErrBaseUploadAndCraft)
-			} else {
-				require.NoError(s.T(), err)
+				require.Error(s.T(), err)
+				if tc.wantErrMsg != "" {
+					assert.Contains(s.T(), err.Error(), tc.wantErrMsg)
+				} else {
+					assert.ErrorIs(s.T(), err, materials.ErrBaseUploadAndCraft)
+				}
+				return
 			}
+			require.NoError(s.T(), err)
 			kind := m.GetMaterialType()
 			assert.Equal(s.T(), tc.expectedType.String(), kind.String())
 			if tc.materialName != "" {