From 3f550bd7c11ea4b2618130fe64fc9d832c8828e7 Mon Sep 17 00:00:00 2001 From: Sylwester Piskozub Date: Tue, 7 Apr 2026 12:20:30 +0200 Subject: [PATCH] fix: surface proper parsing errors for malformed v2 contracts Signed-off-by: Sylwester Piskozub --- .../internal/service/workflowcontract.go | 33 +++- .../internal/service/workflowcontract_test.go | 173 ++++++++++++++++++ 2 files changed, 198 insertions(+), 8 deletions(-) create mode 100644 app/controlplane/internal/service/workflowcontract_test.go diff --git a/app/controlplane/internal/service/workflowcontract.go b/app/controlplane/internal/service/workflowcontract.go index b0f9f9596..047079923 100644 --- a/app/controlplane/internal/service/workflowcontract.go +++ b/app/controlplane/internal/service/workflowcontract.go @@ -17,6 +17,7 @@ package service import ( "context" + "fmt" pb "github.com/chainloop-dev/chainloop/app/controlplane/api/controlplane/v1" schemav1 "github.com/chainloop-dev/chainloop/app/controlplane/api/workflowcontract/v1" @@ -465,7 +466,7 @@ func validateAndExtractMetadata(rawContract []byte, explicitName, explicitDesc s return name, description, nil } -// extractNameFromMetadata attempts to extract the name from metadata.name in v2 contracts +// extractMetadata attempts to extract the name from metadata.name in v2 contracts func extractMetadata(rawContract []byte) (*schemav1.Metadata, error) { if len(rawContract) == 0 { return nil, nil @@ -474,17 +475,33 @@ func extractMetadata(rawContract []byte) (*schemav1.Metadata, error) { // Identify the format format, err := unmarshal.IdentifyFormat(rawContract) if err != nil { - return nil, errors.BadRequest("invalid", "failed to identify contract format") + return nil, nil + } + + // Do a lenient parse (no validation) to detect v2 markers. + v2Probe := &schemav1.CraftingSchemaV2{} + lenientErr := unmarshal.FromRaw(rawContract, format, v2Probe, false) + + isV2 := v2Probe.GetApiVersion() == "chainloop.dev/v1" && v2Probe.GetKind() == "Contract" + if !isV2 { + // Not a v2 contract + return nil, nil } - // Try parsing as v2 Contract + // It's a v2 contract. If the lenient parse already failed, surface that error + if lenientErr != nil { + return nil, errors.BadRequest("invalid", fmt.Sprintf("invalid contract: %s", lenientErr)) + } + + // Lenient parse succeeded, validate v2Contract := &schemav1.CraftingSchemaV2{} - if err := unmarshal.FromRaw(rawContract, format, v2Contract, true); err == nil { - if v2Contract.GetMetadata() != nil { - return v2Contract.GetMetadata(), nil - } + if err := unmarshal.FromRaw(rawContract, format, v2Contract, true); err != nil { + return nil, errors.BadRequest("invalid", fmt.Sprintf("invalid contract: %s", err)) + } + + if v2Contract.GetMetadata() != nil { + return v2Contract.GetMetadata(), nil } - // If v2 parsing failed or no metadata, return nothing return nil, nil } diff --git a/app/controlplane/internal/service/workflowcontract_test.go b/app/controlplane/internal/service/workflowcontract_test.go new file mode 100644 index 000000000..6fcb1ea47 --- /dev/null +++ b/app/controlplane/internal/service/workflowcontract_test.go @@ -0,0 +1,173 @@ +// +// Copyright 2026 The Chainloop Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package service + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestExtractMetadata(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + rawContract []byte + wantName string + wantErr bool + errContains string + }{ + { + name: "empty content returns nil", + rawContract: []byte{}, + wantName: "", + wantErr: false, + }, + { + name: "valid v2 contract returns metadata", + rawContract: []byte(` +apiVersion: chainloop.dev/v1 +kind: Contract +metadata: + name: my-contract + description: a test contract +spec: + materials: + - type: ARTIFACT + name: my-artifact +`), + wantName: "my-contract", + wantErr: false, + }, + { + name: "v2 contract with structural error returns parsing error", + rawContract: []byte(` +apiVersion: chainloop.dev/v1 +kind: Contract +metadata: + name: my-contract +spec: + materials: + ref: vulnerabilities +`), + wantErr: true, + errContains: "invalid contract", + }, + { + name: "non-v2 content returns nil", + rawContract: []byte(` +schemaVersion: v1 +materials: + - type: ARTIFACT + name: my-artifact +`), + wantName: "", + wantErr: false, + }, + { + name: "unparseable content returns nil", + rawContract: []byte("\x00\x01\x02"), + wantName: "", + wantErr: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + metadata, err := extractMetadata(tc.rawContract) + if tc.wantErr { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.errContains) + return + } + + require.NoError(t, err) + if tc.wantName == "" { + assert.Nil(t, metadata) + } else { + require.NotNil(t, metadata) + assert.Equal(t, tc.wantName, metadata.GetName()) + } + }) + } +} + +func TestValidateAndExtractMetadata(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + rawContract []byte + explicitName string + explicitDesc string + wantName string + wantDesc *string + wantErr bool + errContains string + }{ + { + name: "explicit name with no contract", + rawContract: nil, + explicitName: "my-workflow", + wantName: "my-workflow", + wantErr: false, + }, + { + name: "no name and no contract returns error", + rawContract: nil, + wantErr: true, + errContains: "name is required", + }, + { + name: "v2 contract with structural error surfaces parsing error", + rawContract: []byte(` +apiVersion: chainloop.dev/v1 +kind: Contract +metadata: + name: my-contract +spec: + materials: + ref: vulnerabilities +`), + wantErr: true, + errContains: "invalid contract", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + name, desc, err := validateAndExtractMetadata(tc.rawContract, tc.explicitName, tc.explicitDesc) + if tc.wantErr { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.errContains) + return + } + + require.NoError(t, err) + assert.Equal(t, tc.wantName, name) + if tc.wantDesc != nil { + require.NotNil(t, desc) + assert.Equal(t, *tc.wantDesc, *desc) + } + }) + } +}