Skip to content

Improve RBAC policy precedence checks#1654

Open
modelsbridgeaicom-ship-it wants to merge 1 commit into
UnitOneAI:mainfrom
modelsbridgeaicom-ship-it:improve/rbac-policy-precedence
Open

Improve RBAC policy precedence checks#1654
modelsbridgeaicom-ship-it wants to merge 1 commit into
UnitOneAI:mainfrom
modelsbridgeaicom-ship-it:improve/rbac-policy-precedence

Conversation

@modelsbridgeaicom-ship-it

Copy link
Copy Markdown

Skill Improvement ($50-150 Bounty)

Skill Modified

Skill name: rbac-design
Skill path: skills/identity/rbac-design/

What Was Wrong

The skill already warned that ABAC policy conflicts should be detected, but it did not require reviewers to document the policy combining algorithm or prove deny precedence with negative tests. In a hybrid RBAC/ABAC design, broad role permits can silently override high-risk deny rules when precedence is implicit or implemented differently across the PDP, gateway, and application layer.

This implements the review issue in #1651.

What This PR Fixes

  • Adds RBAC-ABAC-09 through RBAC-ABAC-12 checks for combining algorithm, broad permit override, fail-open missing attributes, and missing negative tests.
  • Adds a dedicated Policy Precedence and Conflict Resolution section covering default deny, deny precedence, fail-closed behavior, and conflict tests.
  • Adds RBAC-PREC-* finding IDs for auditable review output.
  • Extends the report template with policy conflict test evidence.
  • Adds a common pitfall for implicit precedence across PDP/gateway/application code.

Evidence

Before (skill misses this / false positive on this):

allow {
  input.subject.role == finance-reader
  input.action == read
}

deny {
  input.subject.status == suspended
}

The previous skill could identify that conflicts exist, but it did not require documenting whether deny, permit, first-match, or priority-based logic wins.

After (now correctly handled):

RBAC-PREC-01: No documented policy combining algorithm
RBAC-PREC-02: Broad role permits override tenant, suspension, or data-classification denies
RBAC-PREC-03: Missing attributes produce permit decisions for sensitive resources
RBAC-PREC-05: Conflict tests cover only happy-path permits, not deny precedence

The skill now requires conflict-resolution evidence and negative tests showing the expected deny decision.

Test Cases Added/Updated

  • Added vulnerable test cases (tests/vulnerable/)
  • Added benign test cases (tests/benign/)
  • Existing tests still pass / not applicable for markdown-only skill update

Validation performed:

  • git diff --check
  • frontmatter required-field check for rbac-design
  • changed-file prompt-injection pattern scan; only the pre-existing safety notice matched
  • marker check for RBAC-ABAC-09, RBAC-PREC-01, and Policy Conflict Test Evidence

Bounty Tier

  • Minor ($50) - Doc update, small logic tweak, typo fix
  • Moderate ($100) - New edge case coverage, FP reduction with evidence
  • Substantial ($150) - Rewritten detection logic, major coverage expansion

Pull Request Checklist

  • Skill follows the format specification in CONTRIBUTING.md
  • At least one real framework is cited with correct control IDs
  • All framework references verified against primary sources already cited by the skill
  • Prompt Injection Safety Notice section included
  • injection-hardened: true set in frontmatter
  • allowed-tools scoped to minimum necessary permissions
  • Tested with at least one AI coding agent: Codex
  • No prohibited patterns per SECURITY.md
  • index.yaml updated with new skill entry (not applicable; existing skill only)

Bounty Info

  • I have read and agree to the bounty terms in CONTRIBUTING.md
  • Preferred payment method: Payment details can be provided privately after maintainer acceptance.

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.

1 participant