Repository navigation
fix(github-config): refuse a managed team that names a parent team or directory group - #4519
Conversation
… directory group A nested team inherits its parent's repository access on GitHub, outside the grant rules that cap what the github-config release may hand out, and a directory link moves membership outside the member allow-list. The Team rules checked neither. Refuse all four fields the provider exposes, in forProvider and initProvider, with one literal JMESPath comparison. Fixes #4511 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e rule itself
Review finding: `x || ''` also reads false, [] and {} as empty, so only the
provider's schema stood between those values and admission. not_null() replaces
a missing value only. Adds fixtures for a boolean, an empty list, a blank
string and a Team with no spec, and words the message to match what is accepted.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Evaluation at
One behaviour to know about: the provider is not exempt. If a team were nested on GitHub by hand, the provider's attempt to record that on the object would be refused and the object would show as out of sync until the nesting is undone or the rule is changed in review. After deploy I will confirm the policy is Ready and both teams still reconcile, and record that on #4511. |
@coderabbitai review |
|
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 29 seconds. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
devantler
left a comment
devantler
left a comment
There was a problem hiding this comment.
🤖 Generated by the Agentic Engineer
Self-review (fallback — CodeRabbit, Codex and Cursor Bugbot unavailable)
Reviewed commit: 29976126292f2cbc5ae89cafa1dce5924c0d7399
- CodeRabbit: rate limited — it refused the request on this head ("Review rate limited", 10:08Z) and has reviewed nothing on this pull request.
- Codex: usage limit since 2026-10-05T08:40Z.
- Cursor Bugbot: usage limit since 2026-10-05T09:37Z; no review from it on record.
An independent reviewer read the change at this head, against the released provider source (terraform-provider-github v6.13.0, provider-upjet-github v0.20.0) and with kyverno 1.19.1.
No P0 or P1. It found no admitted Team that ends up nested or directory-linked, no refusal of admins or maintainers, and no way the rule blocks the provider's own writes: the provider omits empty values when it fills in defaults, so the empty parent fields never reach the spec.
What it checked: the Terraform schema has exactly the four arguments the rule covers and the CRD generates no reference or selector variants for Team; replacing each of the eight clauses with true in turn made the suite fail every time; numbers, booleans, empty objects, variable syntax and a quote-injection string are all refused; an explicit null is admitted and is the same as unset. The update path and background scans were reasoned from the rule, not run.
P3 notes, not applied on this head because none changes what is admitted or refused:
- The Team fixtures use
v1beta1, which the provider does not serve (it servesv1alpha1). This predates the change and the rule matches every version. - The comment says an empty object is refused, and no fixture row pins that or the typed values. The reviewer's probes confirm they are refused.
- "The provider reports all four empty" is exact for the three parent fields;
ldapDnis absent rather than empty. - If someone nests a team by hand on GitHub, the provider's attempt to record it is refused, so the nesting stays and other drift on that Team stops being corrected until someone intervenes. The comment could say so more plainly.
Outside this change: other kinds in the same API group could link a team to a directory group, but they are neither activated nor granted to the tenant.
Verdict: no P0/P1 findings
Resolved the merge conflict at The full 48-file Kyverno fixture evaluation, the 44-rule enforced-action baseline, race-tested baseline validator, and all ten manifest-layer builds pass locally. The conflict reproduced a YAML parse failure before the resolution. Fresh native CI and current-head review are still required; the PR is back in draft until they pass. |
There was a problem hiding this comment.
🤖 Generated by the Agentic Engineer
Self-review (fallback — CodeRabbit, Codex and Cursor Bugbot unavailable)
Reviewed commit: 665c78fdd85566b3ee898035986d89285bfdb552
- CodeRabbit: included public-repository review quota refused the request on this repository at 2026-10-05T18:32:29Z; the edited provider summary at 18:32:38Z says the next included review is in 39 minutes (19:11:38Z). Verified again this round; the old 10:08 refusal on this PR is not used as an unexpired timer. No current-head substantive artifact is present.
- Codex: account code-review usage limit in the direct provider response at 2026-10-05T16:59:30Z, freshly read this round. Recovery requires account credits/limit restoration; no paid overage is authorized.
- Cursor Bugbot: user/team usage or spend limit in the direct provider response at 2026-10-03T22:35:03Z, freshly read this round. Recovery requires the account administrator; no successful newer servicing evidence was found.
Fresh direct reads covered this PR's reviews, conversation, threads and current-head check runs, including the absence of a Bugbot review. There are no current-head reviewer findings or unresolved review threads.
Reviewed all five changed paths against current main 8cb909aeb70f3927a0522f44d4b10ccc2f7a090d: the merge preserves main's external-identity policy and all existing Enforce actions. The new rule checks each of four parent/directory fields in both provider blocks using literal JMESPath equality; missing/null/empty strings remain admitted, while wildcard text, whitespace and non-text values are denied. It does not exempt the provider, weaken the existing team/member/grant rules, expand a credential reader, or change rollout configuration. Existing-violation updates remain denied until the offending fields are cleared.
The conflict was reproduced as a YAML parse failure before resolution. After resolution, all 48 Kyverno fixture files actually evaluate their named rules; the 44-rule action baseline passes, its Go tests pass under the race detector three times, and all ten manifest layers build. The Team and external-identity suites both pass together. The previously noted Team-fixture API-version mismatch remains a test-fidelity limitation: these CLI assertions evaluate the all-version policy match and expressions, not API-server admission or production deployment. It is not attributed to #4518/#4524, which address external grant identity. Live eligibility and deployment readback remain separate from these offline results.
Verdict: no P0/P1 findings
1 participant
Why
The rules that limit what our GitHub configuration release may do check a team's name, members and repository access, but not whether the team is placed under another team or tied to an outside directory group. On GitHub a team placed under another inherits that team's repository access, so a faulty or compromised release could widen access without any of the existing limits noticing.
What
A managed team can no longer be placed under another team or linked to a directory group unless the platform's rules are changed in a reviewed pull request. Our two existing teams use neither and are unaffected.
Fixes #4511