Skip to content

Commit e76e251

Browse files
committed
fix(controlplane): tighten batch-local policy ref exemption
Address review feedback on contract apply batch exemption: - Only honor the client-supplied batch name lists on a dry-run. A real apply persists batch resources before the contract, so it must always validate fully and never trust the client lists to skip validation. - Only exempt bare references (no provider/org). References that explicitly target a remote provider or org are always validated, so a bare-name collision with a batch-local resource can no longer bypass validation. Assisted-by: Claude Code Signed-off-by: Javier Rodriguez <javier@chainloop.dev> Chainloop-Trace-Sessions: f56037fd-1000-4118-9de7-d532c82f30a2
1 parent 33f6101 commit e76e251

3 files changed

Lines changed: 40 additions & 15 deletions

File tree

app/controlplane/internal/service/workflowcontract.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -253,7 +253,16 @@ func (s *WorkflowContractService) Apply(ctx context.Context, req *pb.WorkflowCon
253253
return nil, err
254254
}
255255

256-
if err = s.contractUseCase.ValidateContractPolicies(ctx, req.RawSchema, token, req.GetBatchPolicyNames(), req.GetBatchPolicyGroupNames()); err != nil {
256+
// Batch-local references are only exempted from registry resolution on a dry-run, where
257+
// the resources may not be persisted yet. A real apply persists batch resources before the
258+
// contract, so it must always validate fully and never trust the client-supplied batch lists.
259+
var batchPolicyNames, batchPolicyGroupNames []string
260+
if dryRun {
261+
batchPolicyNames = req.GetBatchPolicyNames()
262+
batchPolicyGroupNames = req.GetBatchPolicyGroupNames()
263+
}
264+
265+
if err = s.contractUseCase.ValidateContractPolicies(ctx, req.RawSchema, token, batchPolicyNames, batchPolicyGroupNames); err != nil {
257266
return nil, handleUseCaseErr(err, s.log)
258267
}
259268

app/controlplane/internal/service/workflowcontract_integration_test.go

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -171,11 +171,11 @@ spec:
171171
`
172172
}
173173

174-
// TestApplyBatchExemption verifies that references to resources declared as part of the same
175-
// batch apply are treated as known (not resolved against the registry), while references to
176-
// resources not in the batch are still validated - in dry-run as well as on a real apply.
177-
// No policy provider is configured in this harness, so any non-exempt provider-scheme reference
178-
// fails resolution, which is exactly what proves remote references are still validated.
174+
// TestApplyBatchExemption verifies that, on a dry-run, bare references to resources declared as
175+
// part of the same batch apply are treated as known (not resolved against the registry), while
176+
// remote references, explicitly scoped references, and any reference on a real apply are still
177+
// validated. No policy provider is configured in this harness, so any non-exempt provider-scheme
178+
// reference fails resolution, which is exactly what proves those references are still validated.
179179
func (s *workflowContractApplyIntegrationTestSuite) TestApplyBatchExemption() {
180180
testCases := []struct {
181181
name string
@@ -192,16 +192,28 @@ func (s *workflowContractApplyIntegrationTestSuite) TestApplyBatchExemption() {
192192
dryRun: true,
193193
},
194194
{
195-
name: "batch-local policy exempted on real apply",
195+
name: "batch-local policy group exempted in dry-run",
196+
rawSchema: contractWithPolicyGroupRef("svc-apply-batch-grp-dry", "chainloop://batch-grp"),
197+
batchPolicyGroupNames: []string{"batch-grp"},
198+
dryRun: true,
199+
},
200+
{
201+
// A real apply persists batch resources before the contract, so it must validate
202+
// fully and never trust the client-supplied batch list.
203+
name: "batch list not trusted on real apply",
196204
rawSchema: contractWithPolicyRef("svc-apply-batch-pol-real", "chainloop://batch-pol"),
197205
batchPolicyNames: []string{"batch-pol"},
198206
dryRun: false,
207+
wantErr: true,
199208
},
200209
{
201-
name: "batch-local policy group exempted in dry-run",
202-
rawSchema: contractWithPolicyGroupRef("svc-apply-batch-grp-dry", "chainloop://batch-grp"),
203-
batchPolicyGroupNames: []string{"batch-grp"},
204-
dryRun: true,
210+
// An org-scoped reference is unambiguously remote and must not be exempted by a
211+
// bare-name collision with a batch-local policy.
212+
name: "org-scoped ref not exempted by name collision",
213+
rawSchema: contractWithPolicyRef("svc-apply-scoped-pol-dry", "chainloop://acme/batch-pol"),
214+
batchPolicyNames: []string{"batch-pol"},
215+
dryRun: true,
216+
wantErr: true,
205217
},
206218
{
207219
name: "non-batch policy still validated in dry-run",

app/controlplane/pkg/biz/workflowcontract.go

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -588,8 +588,10 @@ func (uc *WorkflowContractUseCase) findAndValidatePolicy(ctx context.Context, at
588588
if loader.IsProviderScheme(att.GetRef()) {
589589
pr := loader.ProviderParts(att.GetRef())
590590
// Policies created/updated in the same batch apply may not be persisted yet, so
591-
// treat references to them as known instead of resolving against the registry.
592-
if slices.Contains(batchPolicyNames, pr.Name) {
591+
// treat references to them as known instead of resolving against the registry. Only
592+
// bare references (no provider/org) can name a batch-local policy; references that
593+
// explicitly target a remote provider or org are always validated.
594+
if pr.Provider == "" && pr.OrgName == "" && slices.Contains(batchPolicyNames, pr.Name) {
593595
return nil, nil
594596
}
595597
// Validate attachment
@@ -629,8 +631,10 @@ func (uc *WorkflowContractUseCase) findAndValidatePolicyGroup(ctx context.Contex
629631
// [chainloop://][provider/]name
630632
pr := loader.ProviderParts(att.GetRef())
631633
// Policy groups created/updated in the same batch apply may not be persisted yet, so
632-
// treat references to them as known instead of resolving against the registry.
633-
if slices.Contains(batchPolicyGroupNames, pr.Name) {
634+
// treat references to them as known instead of resolving against the registry. Only bare
635+
// references (no provider/org) can name a batch-local group; references that explicitly
636+
// target a remote provider or org are always validated.
637+
if pr.Provider == "" && pr.OrgName == "" && slices.Contains(batchPolicyGroupNames, pr.Name) {
634638
return nil, nil
635639
}
636640
remoteGroup, err := uc.GetPolicyGroup(ctx, pr.Provider, pr.Name, pr.OrgName, "", token)

0 commit comments

Comments
 (0)