-
Notifications
You must be signed in to change notification settings - Fork 54
Warn when regex patterns in rule data lack anchoring #1781
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -182,4 +182,8 @@ _rule_data_errors contains error if { | |
| } | ||
| } | ||
|
|
||
| _rule_data_errors contains error if { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] correctness Anchoring check fires on non-string values (e.g., integer 1) because startswith is undefined for non-strings in Rego and not-undefined evaluates to true. This produces a confusing warning message alongside the existing type-error failure from schema validation. Suggested fix: Add an is_string(r) guard before the not startswith(r, "^") check to avoid noisy duplicate warnings on already-invalid data. |
||
| some error in rule_data.anchoring_errors(_plat_patterns_rule_data_key) | ||
| } | ||
|
|
||
| _plat_patterns_rule_data_key := "disallowed_platform_patterns" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,7 @@ single_test_case(branch, expected_results) if { | |
| "^rhel-10\\.[0-9]+$", | ||
| "^rhel-[0-9]+\\.[0-9]\\.0$", | ||
| "^rhel-[0-9]+-main$", | ||
| "branch[0-9]+-rhel-[0-9]+.[0-9]+.[0-9]+$", | ||
| "^.*branch[0-9]+-rhel-[0-9]+.[0-9]+.[0-9]+$", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] correctness Test mock data pattern changed from unanchored to ^.* prefix to suppress the new anchoring warning. While ^.* is semantically equivalent to no anchor (matches same strings), this is test infrastructure only with no production impact. |
||
| ] | ||
|
|
||
| mock_tasks := mock_input.attestations[0].statement.predicate.buildConfig.tasks | ||
|
|
@@ -271,3 +271,17 @@ test_hotfix_branch_with_extra_suffix_denied if { | |
| }} | ||
| single_test_case("kernel-5.14.0-570.42.1.el9_6-branch1-rhel-9.6.0-extra", expected) | ||
| } | ||
|
|
||
| test_anchoring_warning_unanchored_pattern if { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] test-integrity Tests assert directly on git_branch.rule_data_errors instead of asserting through the deny interface. This masks the fact that rule_data_errors is not wired into deny and would never surface during policy evaluation. |
||
| expected := {{ | ||
| "code": "git_branch.allowed_target_branch_patterns_format", | ||
| # regal ignore:line-length | ||
| "msg": "Pattern \"main\" in allowed_target_branch_patterns is not anchored with ^", | ||
| "severity": "warning", | ||
| }} | ||
| assertions.assert_equal_results(expected, git_branch.deny) with data.rule_data.allowed_target_branch_patterns as ["main"] | ||
| } | ||
|
|
||
| test_anchoring_warning_anchored_pattern if { | ||
| assertions.assert_empty(git_branch.deny) with data.rule_data.allowed_target_branch_patterns as ["^main$"] | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,3 +36,23 @@ matches_any(branch, valid_locations) if { | |
| some pattern in valid_locations | ||
| regex.match(pattern, branch) | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [high] correctness The rule_data_errors rule is defined but never consumed by any deny or warn rule in this package. Warnings about unanchored patterns in allowed_rpm_build_dependency_sources will never surface during policy evaluation. Tests pass only because they directly assert on rule_data_errors, not through warn. Suggested fix: Add a deny or warn rule with METADATA annotations that consumes rule_data_errors, consistent with how other packages wire up rule data validation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] style/conventions Uses public rule_data_errors (no underscore prefix) contrary to codebase convention. All other packages use _rule_data_errors (private) consumed by a local deny rule. Suggested fix: Rename to _rule_data_errors and add a local deny/warn consumer with METADATA annotations. |
||
| # METADATA | ||
| # title: allowed_rpm_build_dependency_sources format | ||
| # description: >- | ||
| # Confirm the `allowed_rpm_build_dependency_sources` rule data uses anchored regex patterns. | ||
| # custom: | ||
| # short_name: allowed_rpm_build_dependency_sources_format | ||
| # failure_msg: "%s" | ||
| # collections: | ||
| # - redhat_rpms | ||
| # - policy_data | ||
| # | ||
| deny contains result if { | ||
| some error in _rule_data_errors | ||
| result := metadata.result_helper_with_severity(rego.metadata.chain(), [error.message], error.severity) | ||
| } | ||
|
|
||
| _rule_data_errors contains error if { | ||
| some error in rule_data.anchoring_errors("allowed_rpm_build_dependency_sources") | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -160,6 +160,22 @@ test_matches_any_multiple_patterns if { | |
| rpm_build_deps.matches_any("https://codeload.github.com/user/repo/tar.gz/v1.0", patterns) | ||
| } | ||
|
|
||
| test_anchoring_warning_unanchored_pattern if { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] test-integrity Tests assert directly on rpm_build_deps.rule_data_errors instead of asserting through the warn interface. This masks the fact that rule_data_errors is not wired into warn and would never surface during policy evaluation. |
||
| expected := {{ | ||
| "code": "rpm_build_deps.allowed_rpm_build_dependency_sources_format", | ||
| # regal ignore:line-length | ||
| "msg": "Pattern \"download.example.com\" in allowed_rpm_build_dependency_sources is not anchored with ^", | ||
| "severity": "warning", | ||
| }} | ||
| assertions.assert_equal_results(expected, rpm_build_deps.deny) with input.attestations as [_sbom_attestation_with_download_location("NOASSERTION")] | ||
| with data.rule_data.allowed_rpm_build_dependency_sources as ["download.example.com"] | ||
| } | ||
|
|
||
| test_anchoring_warning_anchored_pattern if { | ||
| assertions.assert_empty(rpm_build_deps.deny) with input.attestations as [_sbom_attestation_with_download_location("NOASSERTION")] | ||
| with data.rule_data.allowed_rpm_build_dependency_sources as ["^https://download\\.example\\.com/.*"] | ||
| } | ||
|
|
||
| # Helper function to create SBOM attestation with specific download location | ||
| _sbom_attestation_with_download_location(location) := {"statement": { | ||
| "predicateType": "https://spdx.dev/Document", | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[low] correctness
The anchoring check only verifies
^at the start of the pattern. Users can silence the warning by prepending^.*without improving security, since^.*Xis equivalent to unanchoredXfor substring matching. This is a known design trade-off — stricter heuristics would produce false positives for legitimate patterns.