Skip to content

fix(policies): reject material policy attachments that can never apply - #3775

Merged
migmartri merged 8 commits into
chainloop-dev:mainfrom
migmartri:fix/3774-reject-never-applying-policy-attachments
Nov 5, 2026
Merged

migmartri merged 8 commits into
chainloop-dev:mainfrom
migmartri:fix/3774-reject-never-applying-policy-attachments

Conversation

@migmartri

@migmartri migmartri commented Nov 4, 2026 •

Copy link
Copy Markdown
Member

Closes #3774

A policy without material types (kind-less) applies only to the material named by the attachment selector. A contract that attached such a policy without selector.name was accepted, but the policy never ran and nothing reported it. For a gating policy, this looked like a passing check.

Changes:

  • Control plane: contract create, update and apply now reject a material policy attachment that can never apply:

    • a kind-less policy without selector.name, for example: policy "upstream-workflow-attested" applies to any material type and requires a material selector (selector.name)
    • a typed policy whose exact selector names a contract material of a type that the policy does not support.

    A selector that names a material not declared in the contract, or a selector that is not an exact match (such as PREFIX), is still accepted, because it can match materials added at attestation time.

  • Attestation init: the same check runs when attestation init --contract <file> sends the contract, because that path also creates or updates the contract. A pipeline whose contract attaches a policy that can never apply now fails at init with the same error.

  • Shared rule: the applicability rule of shouldApplyPolicy moves into pure functions (attachmentApplies, canMatchAnyMaterial). Policy evaluation and the new validation both use them, so the two can't drift.

Not in scope:

  • Policy groups: a kind-less policy in an unnamed group material does not silently do nothing. The group verifier applies it to every material. This PR leaves that behavior unchanged.
  • policy develop eval --material-name: not part of this PR.
  • Embedded policies in contracts: they go through the provider lookup with an empty name in findAndValidatePolicy, which fails. This is a separate, existing issue.

This PR was written with AI assistance (Claude Code).

View guided diff

A policy without material types applies only to the material named by the
attachment selector. Attaching it without selector.name was accepted, but
the policy never ran and nothing reported it. The control plane now rejects
such attachments on contract create and update, and also typed policies
whose exact selector names a contract material of an unsupported type.
attestation init warns about them for contracts created before this check.

The applicability rule is shared between evaluation and validation.

Closes chainloop-dev#3774

Assisted-by: Claude Code
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>

Chainloop-Trace-Sessions: f6a62db2-f397-4a52-bc1d-ab2c15ef422c
@migmartri
migmartri requested a review from jiparis November 4, 2026 09:46
@chainloop-platform

chainloop-platform Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

PR validation — ✅ 3 passing

Status Policy Material Messages
✅ Passed pr-min-approvals pr-info -
✅ Passed pr-description-required pr-info -
✅ Passed pr-user-story-linked pr-info -

View attestation ↗

AI Session Checks — 🟡 71% · ✅ 0 failing

Avg score Sessions Failing policies Attribution Files Lines Total Duration
🟡 71% 2 ✅ 0 96% AI / 4% Human 9 +578 / -48 3h57m51s

🟡 60% — 90% AI — ✅ All policies passing

Nov 5, 2026 02:13 UTC · 13m41s · $3.12 · 170 in / 45.0k out · claude-code 2.1.318 (claude-sonnet-5-5)

View session details ↗

Change Summary

Adds raw batch policy specs to dry-run apply requests and parses them with schema validation. Routes resolved batch policies through shared input and material-applicability checks, retaining name-only compatibility. Updates generated API artifacts and adds applicability and parser regression tests.

AI Session Overall Score

🟡 60% — Autonomous review-fix run leaves name-only dry runs unchecked despite passing focused tests.

AI Session Analysis Breakdown

🟢 93% · scope-discipline

No notes.

🟡 70% · context-and-planning

🟠 The optional API extension and caller-migration dependency were not surfaced before editing. · Medium Severity

💡 Before changing request contracts, explicitly plan caller migration and the behavior of existing clients.

🟡 70% · solution-quality

🟢 Resolved batch policies reuse required-input and material-applicability checks rather than duplicating approximations. · High Impact

🟡 68% · verification

🟢 The parser mutation exposes the metadata-only failure; restored validation passes the focused suite and lint. · High Impact

🟠 The original bypass lacked a pre-fix behavioral reproduction; the first observed runner failure was a signature compilation error. · Medium Severity

💡 Run a baseline-compatible regression test before changing signatures, and verify it fails for the reported behavior.

🟠 Separate parser and business tests do not establish request-level dry-run wiring or parity with real apply. · Medium Severity

💡 Add a request-level regression that compares dry-run and real-apply outcomes for the same invalid attachment.

🔴 35% · alignment

🟢 The completion report explicitly discloses the surviving name-only bypass and external caller dependency. · High Impact

🔴 Name-only batch references still skip applicability validation, leaving the requested parity conditional on callers supplying the new optional specs. · High Severity

💡 Close the original request path before claiming completion, or obtain approval for an explicit incomplete-validation fallback.

abstained · user-trust-signal

🟡 user-trust-signal not assessed — reviewer should confirm coverage · Low Severity


File Attribution

██████████████████░░ 90% AI / 10% Human

Status Attribution File Lines
modified ai app/controlplane/internal/service/workflowcontract_test.go +56 / -0
modified ai app/controlplane/pkg/biz/workflowcontract_policies_test.go +52 / -1
modified ai app/controlplane/pkg/biz/workflowcontract.go +28 / -24
modified ai app/controlplane/internal/service/workflowcontract.go +37 / -4
modified ai app/controlplane/api/controlplane/v1/workflow_contract.proto +5 / -0
modified ai app/controlplane/internal/service/attestation.go +1 / -1

Policies (4)

Status Policy Material Messages
✅ Passed ai-config-ai-agents-allowed ai-coding-session-2df578 -
✅ Passed ai-config-no-dangerous-commands ai-coding-session-2df578 -
✅ Passed ai-config-no-secrets ai-coding-session-2df578 -
✅ Passed ai-config-mcp-servers-allowed ai-coding-session-2df578 -

🟡 82% — 99% AI — ✅ All policies passing

Nov 4, 2026 09:35 UTC · 3h44m10s · $28.45 · 886 in / 255.5k out · claude-code 2.1.318 (claude-opus-5-5)

View session details ↗

Change Summary

Adds shared material-policy attachment validation and wires rejection into both contract schema versions. Reuses pure applicability helpers between validation and evaluation while preserving selector matching semantics. Adds focused policy and contract-validator tests, including missing selectors and unsupported material types.

AI Session Overall Score

🟡 82% — Focused, tested validation needs review of omitted group checks and unconfirmed legacy CLI coverage.

AI Session Analysis Breakdown

🟢 94% · scope-discipline

🟢 Production changes stay in contract validation and policy applicability; no unrelated modules or dependencies change. · High Impact

🟢 92% · solution-quality

🟢 Validation and evaluation share pure applicability helpers, avoiding duplicated rules and preserving runtime matching behavior. · High Impact

🟢 88% · user-trust-signal

No notes.

🟡 78% · verification

🟢 Post-cleanup policy tests and filtered contract tests return observed passes; final touched-area lint reports zero issues. · High Impact

🟠 The user remains active through review and cleanup requests but never explicitly confirms the new behavior works. · Medium Severity

💡 After validation changes, confirm one representative failing and passing contract at the actual user-facing entrypoint.

🟡 76% · context-and-planning

🟠 The initial shared-validator design leaves unresolved references and divergent group behavior unsettled before implementation begins. · Medium Severity

💡 Before cross-entrypoint validation work, outline handling for direct/group attachments, resolved/local references, and legacy/contract-free paths.

🟡 68% · alignment

🟠 The agent explicitly omits requested group-entry rejection because group semantics differ, without a captured user instruction dropping it. · Medium Severity

💡 When inspection disproves a ticket assumption, confirm the resulting scope change with the user before dropping that requirement.


File Attribution

███████████████████░ 99% AI / 1% Human

Status Attribution File Lines
created ai app/controlplane/pkg/biz/workflowcontract_policies_test.go +151 / -0
created ai pkg/policies/attachment_validation_test.go +133 / -0
created ai pkg/policies/attachment_validation.go +78 / -0
modified ai pkg/policies/policies.go +16 / -13
modified ai app/controlplane/pkg/biz/workflowcontract.go +19 / -3
modified ai app/controlplane/internal/service/attestation.go +2 / -2

Policies (4)

Status Policy Material Messages
✅ Passed ai-config-ai-agents-allowed ai-coding-session-f6a62d -
✅ Passed ai-config-no-dangerous-commands ai-coding-session-f6a62d -
✅ Passed ai-config-no-secrets ai-coding-session-f6a62d -
✅ Passed ai-config-mcp-servers-allowed ai-coding-session-f6a62d -

Security Checks — ✅ 6 passing

✅ Reviewed this change against the project's security context (39 concerns hunted) — no introduced risk found.

View in Chainloop ↗ · How this works ↗
✅ Reviewed this change against the project's security context (39 concerns hunted) — no introduced risk found.

View in Chainloop ↗ · How this works ↗

✅ secret-scan

Status Policy Messages
✅ Passed secrets-detection -

✅ sast-scan

Status Policy Messages
✅ Passed owasp-top10-2025 -
✅ Passed sast -
✅ Passed cwe-top25 -
✅ Passed cwe-top26-40-cusp -

✅ iac-scan

Status Policy Messages
✅ Passed iac-misconfiguration -
Scans not applied (2)
Scan Reason
vulnerability-scan no manifest/lockfile changed
github-actions-scan no workflow files changed

View attestation ↗


Powered by Chainloop and Chainloop Trace

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 8 files

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

…tion init

attestation init re-sends the contract on every run, so rejecting material
policy attachments that can never apply there would break pipelines whose
contract was stored before the check. Skip the check in FindOrCreateWorkflow
and keep it on contract create and update. The CLI warns at init instead.

Refs chainloop-dev#3774

Assisted-by: Claude Code
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>

Chainloop-Trace-Sessions: f6a62db2-f397-4a52-bc1d-ab2c15ef422c
Fixes the goconst lint findings in the new attachment validation tests.

Assisted-by: Claude Code
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>

Chainloop-Trace-Sessions: f6a62db2-f397-4a52-bc1d-ab2c15ef422c
jiparis
jiparis previously approved these changes Nov 4, 2026
Comment thread app/cli/pkg/action/attestation_init.go Resolved
The contract can't be fixed at attestation init, so the check only runs
when the contract is created or updated in the control plane. Remove the
init warning and the verifier helper that only it used.

Refs chainloop-dev#3774

Assisted-by: Claude Code
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>

Chainloop-Trace-Sessions: f6a62db2-f397-4a52-bc1d-ab2c15ef422c
FindOrCreateWorkflow also creates or updates the contract when contract
bytes are sent, so validate it there too, and drop the
WithoutMaterialApplicabilityCheck option.

Refs chainloop-dev#3774

Assisted-by: Claude Code
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>

Chainloop-Trace-Sessions: f6a62db2-f397-4a52-bc1d-ab2c15ef422c
Validation now uses canMatchAnyMaterial and nameMatches, the same helpers
as evaluation, instead of repeating their conditions. It only checks
contract materials for exact selectors, so a new match mode is never
treated as an exact match. Drop duplicate test cases.

Refs chainloop-dev#3774

Assisted-by: Claude Code
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>

Chainloop-Trace-Sessions: f6a62db2-f397-4a52-bc1d-ab2c15ef422c
jiparis
jiparis previously approved these changes Nov 4, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 3 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

Comment thread pkg/policies/attachment_validation.go Resolved

@migmartri migmartri left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — Batch-local dry runs bypass applicability validation

findAndValidatePolicy returns nil for references in batchPolicyNames, and findAndValidateMaterialPolicy then returns without calling ValidateMaterialAttachment (app/controlplane/pkg/biz/workflowcontract.go:636-639). WorkflowContractService.Apply supplies that list for dry runs, so a dry-run containing a new kind-less policy attached without selector.name succeeds, while the real apply rejects the same contract after resolving the policy. This makes dry-run validation disagree with the write path. Please resolve the batch policy specs for this check (and add a batch-local dry-run test) rather than skipping applicability validation.

I reproduced this with ValidateContractPolicies(..., []string{"kindless"}, nil), which returned nil for the invalid attachment.

Reviewed the change against the posted security context; I found no new bypass of those guards.

…ocal dry runs

Accept the raw specs of batch policies in the apply request and use them to run the same applicability and input checks as a real apply.

Assisted-by: Claude Code
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>

Chainloop-Trace-Sessions: 2df578c9-f7ba-441c-8f53-410c372821f2
@migmartri

Copy link
Copy Markdown
Member Author

Done in 48e466a: dry-run apply now accepts the raw batch policy specs (new batch_policies field) and runs the same input and material applicability checks as a real apply, so a batch-local kind-less policy attached without selector.name is rejected; added a batch-local test. Callers that send only batch_policy_names keep the previous behavior (name is known, no spec checks) until they also send the specs.

🤖 Posted by Maximus bot (Claude Code) on behalf of @migmartri using the eng-autopilot skill

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 9 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

View guided diff | Re-trigger cubic

Validate the raw batch policies while parsing them, and document that batch_policies is only used on dry runs.

Assisted-by: Claude Code
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>

Chainloop-Trace-Sessions: 2df578c9-f7ba-441c-8f53-410c372821f2
@migmartri
migmartri merged commit 87a4c17 into chainloop-dev:main Nov 5, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reject material policy attachments that can never apply (kind-less policy without a selector)

3 participants