From 2c1b24bc4cf69bf062470b43c92aaa4b5d55d0ab Mon Sep 17 00:00:00 2001 From: Vibhav Bobade Date: Fri, 3 Apr 2026 08:33:51 +0530 Subject: [PATCH] fix(crafter): surface DNS-1123 validation errors instead of masking them The material auto-discovery loop in AddMaterialContactFreeWithAutoDetectedKind silently swallowed DNS-1123 validation errors because: 1. Variable shadowing: `m, err :=` inside the for loop created a new `err` that never propagated to the outer scope, so the final error message was always nil when all kinds failed. 2. No early validation: invalid material names (uppercase, underscores, dots) were passed through the entire kind-probing loop before failing deep in proto validation, with the real cause buried. 3. Proto-validation errors not detected: the loop treated schema-level validation failures the same as kind mismatches, continuing to probe instead of stopping. Fixes: - Replace `:=` with `=` to avoid shadowing the outer `err` - Add DNS-1123 regex check at the top of AddMaterialContractFree - Detect protovalidate.ValidationError in the auto-discovery loop and break immediately instead of masking the real error Fixes #2394 Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Vibhav Bobade --- pkg/attestation/crafter/crafter.go | 27 +++++++++++++-- pkg/attestation/crafter/crafter_test.go | 44 ++++++++++++++++++++++--- 2 files changed, 64 insertions(+), 7 deletions(-) diff --git a/pkg/attestation/crafter/crafter.go b/pkg/attestation/crafter/crafter.go index f8f6bb8b3..cc4568138 100644 --- a/pkg/attestation/crafter/crafter.go +++ b/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 diff --git a/pkg/attestation/crafter/crafter_test.go b/pkg/attestation/crafter/crafter_test.go index 20ac3101e..ca172cb93 100644 --- a/pkg/attestation/crafter/crafter_test.go +++ b/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 != "" {