feat(EC-1881): add except_when support to disallowed_attributes rule data#1739
feat(EC-1881): add except_when support to disallowed_attributes rule data#1739robnester-rh wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds an except_when clause to disallowed SBOM attribute data, validates regex patterns, implements disallowed_attribute_excepted(purl) in library, suppresses matching CycloneDX/SPDX violations, adds comprehensive tests, and updates documentation source line links. ChangesSBOM disallowed attribute exceptions
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@policy/lib/sbom/sbom.rego`:
- Around line 304-310: In disallowed_attribute_excepted, avoid reparsing
purl_string inside the loop: parse the PURL once at the top of the rule (assign
parsed := ec.purl.parse(purl_string) after confirming purl_string != ""), then
iterate over disallowed.except_when and over parsed.qualifiers to compare
qualifier.key to exception.purl_qualifier and call
url_matches_any_pattern(qualifier.value, exception.patterns); this removes the
repeated parse per exception and keeps the rest of the logic intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 8fded4f4-8737-45b1-8c7f-d2f80e6b57db
📒 Files selected for processing (9)
antora/docs/modules/ROOT/pages/packages/release_sbom_cyclonedx.adocantora/docs/modules/ROOT/pages/packages/release_sbom_spdx.adocpolicy/lib/sbom/sbom.regopolicy/lib/sbom/sbom_test.regopolicy/release/sbom/sbom_test.regopolicy/release/sbom_cyclonedx/sbom_cyclonedx.regopolicy/release/sbom_cyclonedx/sbom_cyclonedx_test.regopolicy/release/sbom_spdx/sbom_spdx.regopolicy/release/sbom_spdx/sbom_spdx_test.rego
ReviewFindingsLow
Info
Previous runReviewFindingsMedium
Low
|
fullsend-ai-review
Bot
added
the
requires-manual-review
Review Summary by QodoAdd except_when support to disallowed_attributes rule with PURL qualifier matching WalkthroughsDescription• Adds except_when field to disallowed_attributes rule schema • Enables suppressing violations when PURL qualifiers match regex patterns • Implements fail-closed semantics for missing PURLs or non-matching patterns • Validates regex patterns and PURL qualifier matching in both SPDX and CycloneDX • Updates documentation links to reflect code line number changes Diagramflowchart LR
A["disallowed_attributes rule data"] -->|contains except_when field| B["PURL qualifier patterns"]
B -->|regex match against| C["Package PURL qualifiers"]
C -->|match found| D["Suppress violation"]
C -->|no match or missing| E["Produce violation"]
F["Schema validation"] -->|validates regex format| B
G["CycloneDX/SPDX processors"] -->|check exception| H["disallowed_attribute_excepted"]
H -->|returns true/false| D
H -->|returns true/false| E
File Changes1. policy/lib/sbom/sbom.rego
|
Code Review by QodoSorry, something went wrongWe weren't able to complete the code review on our side. Please try again |
…data Extend the disallowed_attributes rule data schema with an optional except_when field that suppresses violations when a package's PURL qualifier matches a set of regex patterns. This enables teams to allow specific disallowed attributes (e.g., hermeto:pip:package:binary) for packages from trusted indexes while still blocking untrusted sources. The filtering is fail-closed: if no purl-type ref exists, the qualifier is absent, or no pattern matches, the violation is still produced. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Move PURL parsing outside except_when loop to avoid reparsing per exception entry (CodeRabbit) - Use array comprehension in _package_purl to safely handle SPDX packages with multiple PURL refs (Fullsend) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressed both findings from Fullsend review in 5d1b802:
|
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Move _package_purl and disallowed_attribute_excepted after all incremental rule definitions to avoid splitting rule_data_errors and deny rule groups. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| ) | ||
| } | ||
|
|
||
| _package_purl(pkg) := purl if { |
There was a problem hiding this comment.
[low] edge-case
The _package_purl helper selects the first PURL-type externalRef via purls[0]. If an SPDX package has multiple PURL-type external references, only the first is evaluated for except_when exception matching. Consider adding a test case with two purl-type externalRefs to document the first-wins semantics.
fullsend-ai-review
Bot
added
ready-for-merge
| some exception in disallowed.except_when | ||
| some qualifier in parsed.qualifiers | ||
| qualifier.key == exception.purl_qualifier | ||
| url_matches_any_pattern(qualifier.value, exception.patterns) |
There was a problem hiding this comment.
regex.match does substring matching, not full-string matching. A pattern like console\.redhat\.com also matches evil-console.redhat.com. This is inherited from url_matches_any_pattern, but it now extends to a security-sensitive bypass path.
Worth either wrapping patterns with ^/$ in the matching function, or at minimum documenting the anchoring requirement so operators don't accidentally widen exception scope.
| property.name == disallowed.name | ||
| object.get(property, "value", "") == object.get(disallowed, "value", "") | ||
|
|
||
| not sbom.disallowed_attribute_excepted(disallowed, object.get(component, "purl", "")) |
There was a problem hiding this comment.
The METADATA description block for this rule (above) doesn't mention except_when. These annotations generate the public docs on conforma.dev and are the primary way operators discover rule data configuration. EC-1881 AC requires documentation updates.
The SPDX counterpart has the same gap. Worth describing the new field, its schema, and matching semantics in both METADATA blocks.
| with ec.oci.image_tag_refs as [] | ||
| with data.rule_data as {sbom.rule_data_attributes_key: disallowed_attributes} | ||
|
|
||
| count({r | some r in results; r.code == "sbom_spdx.disallowed_package_attributes"}) == 0 |
There was a problem hiding this comment.
CycloneDX has test_attributes_except_when_without_except_when_unchanged covering mixed entries with/without except_when. Missing the SPDX equivalent. The two formats have structurally different iteration patterns, so the backward-compat path is worth testing independently.
| disallowed_attribute_excepted(disallowed, purl_string) if { | ||
| purl_string != "" | ||
| parsed := ec.purl.parse(purl_string) | ||
| some exception in disallowed.except_when | ||
| some qualifier in parsed.qualifiers | ||
| qualifier.key == exception.purl_qualifier | ||
| url_matches_any_pattern(qualifier.value, exception.patterns) | ||
| } |
There was a problem hiding this comment.
Sibling predicates like component_found_by_hermeto and package_found_by_hermeto use else := false. This one relies on implicit undefined-is-falsy. Adding else := false would make the fail-closed intent explicit.
| with data.rule_data as {sbom.rule_data_attributes_key: [{"name": "syft:distro:id", "value": "rhel"}]} | ||
| } | ||
|
|
||
| test_attributes_except_when_match_suppresses_violation if { |
There was a problem hiding this comment.
SPDX tests extract _spdx_excepted_package to reduce boilerplate. These tests inline identical component structures across ~7 tests. A similar helper would cut the duplication.
| # regal ignore:line-length | ||
| "msg": "Item at index 8 in disallowed_attributes has an invalid regular expression in except_when: \"[\"", | ||
| "severity": "failure", | ||
| }, |
There was a problem hiding this comment.
Schema validation tests cover the invalid regex case but don't include a well-formed except_when entry among the valid inputs. The positive path is only implicitly tested via integration tests.
|
I didn't see this and created a similar PR (#1744) 🫣 Also, I have an image that can be used to verify this, feel free to use it. Here's the snapshot: {
"application": "lucarval-test",
"artifacts": {},
"componentGroup": "",
"components": [
{
"containerImage": "quay.io/redhat-user-workloads/rhtap-shared-team-tenant/awesome-fermi-c9ff1@sha256:27e73129cb102cff44c47b31cd5beef2b0a8cdd337ba42bac88613cdb3f9a326",
"name": "awesome-fermi-c9ff1",
"source": {
"git": {
"revision": "d764b86481955715da6a34ec6d15d068e65a25dc",
"url": "https://github.com/lucarval-rhtap/awesome-fermi"
}
},
"version": ""
}
]
} |
4 participants
Summary
disallowed_attributesrule data schema with an optionalexcept_whenfield that suppresses violations when a package's PURL qualifier matches regex patternshermeto:pip:package:binary) for packages from trusted indexes while still blocking untrusted sourcesTest plan
except_whenentries and invalid regex patternsdisallowed_package_attributessuppresses violations when qualifier matchesdisallowed_package_attributessuppresses violations when qualifier matchesexcept_whenentries work correctlyexcept_whenbehave exactly as before (backwards compatible)🤖 Generated with Claude Code