Repository navigation
CODEOWNERS: pkgscan-allow.toml entries need the security owner - #1113
bryan-minimal wants to merge 1 commit into
Conversation
packages/<name>/pkgscan-allow.toml clears findings at the gate that pkgmgr-rs applies before it mirrors and opens an update unattended (gominimal/pkgmgr-rs#1105). An entry is a security decision, so the security owner reviews every one. Agents may draft entries but never merge them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 1 billable file and costs up to $0.25.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 5 minutes for your next included review. View limit details
Comment |
There was a problem hiding this comment.
The pattern is right. packages/*/pkgscan-allow.toml matches the path pkgmgr-rs builds (pkg_dir.join("pkgscan-allow.toml"), one level under packages/). It comes after *, so it takes precedence for these files. The CODEOWNERS errors API reports no errors at head, and bryan-minimal has admin on the repo. I'm leaving this as a comment, not an approval, because the owner deadlock described in the PR body is still open, and there is a second gap below.
| # (packages/<name>/pkgscan-allow.toml, read by pkgmgr-rs). The security owner | ||
| # approves every entry; agents may draft one but never merge it. Listed after | ||
| # `*`, so it replaces that rule for these files: CODEOWNERS uses the last match. | ||
| packages/*/pkgscan-allow.toml @bryan-minimal |
There was a problem hiding this comment.
The deadlock in the description is real at head. The main ruleset (id 14240756) has bypass_actors: [] and require_code_owner_review: true. Agent sessions open pkgs PRs as bryan-minimal, and an author cannot approve their own PR. So every allowlist PR an agent drafts would sit unmergeable, and that is the exact flow pkgmgr-rs#1105 sets up. Pick an owner set before merging. Option 1 (a security team with at least two humans) keeps the security-owner intent. Option 2 (@bryan-minimal @gominimal/minimalists) means any minimalist can approve, the same gate as today, so it would make this rule advisory.
| @@ -1,2 +1,8 @@ | |||
| # Every path requires review from a code owner. | |||
| * @gominimal/minimalists | |||
There was a problem hiding this comment.
.github/CODEOWNERS itself still falls under *. Any one minimalist can approve a PR that deletes or rewrites line 8, and that PR doesn't need the security owner. Add a .github/CODEOWNERS line owned by the same owner set as the pkgscan rule, after *, so nobody can drop the gate without the security owner's approval.
There was a problem hiding this comment.
The pattern is right. packages/*/pkgscan-allow.toml matches both allowlists on main (async-profiler, atuin) and the three in #1263, #1264 and #1265. Every package on main sits at packages/<name>/build.ncl, and pkgmgr-rs only reads <pkg_dir>/pkgscan-allow.toml, so there is no nested path to miss. Because * does not cross /, the pattern matches nothing else. The CODEOWNERS errors API reports no errors at head. Two things block this before merge:
- .github/CODEOWNERS:8, the owner set. The
mainruleset has no bypass actors, and #1263, #1264 and #1265 are all authored bybryan-minimal. With@bryan-minimalas the only owner, none of them could be approved once this lands. The org has no security team today (onlyminimalistsandwebapp-minimalists). So either create one with at least two humans and name it here, or list@bryan-minimalalongside a second named human owner. Don't fall back to@gominimal/minimalists, because that makes the rule advisory. - .github/CODEOWNERS:2. The file itself still falls under
*, so any minimalist can approve a PR that removes line 8. Add/.github/CODEOWNERSafter th 8000 e new rule with the same owner set.
There was a problem hiding this comment.
Scope matches the title. The diff adds one CODEOWNERS rule and its comment. Clobber none: git log shows only #713 before this commit, and the diff removes no line. Correctness the pattern is valid. It matches every package directory, because all packages sit one level under packages/. Last-match behavior is as the comment says. No workflow in this repo auto-merges or auto-approves. Two defects remain at this head. Both repeat the open threads r4421165650 and r4421165639, which have no reply and no new commit. They are filed again so that this review does not read as an approval. One owner set fixes both. Not checked: the gh api calls were denied. So the ruleset, the write access of bryan-minimal, the CODEOWNERS errors endpoint and the path that pkgmgr-rs reads are unverified. gitleaks is not installed. I read the six added lines by hand and found no secret.
| # (packages/<name>/pkgscan-allow.toml, read by pkgmgr-rs). The security owner | ||
| # approves every entry; agents may draft one but never merge it. Listed after | ||
| # `*`, so it replaces that rule for these files: CODEOWNERS uses the last match. | ||
| packages/*/pkgscan-allow.toml @bryan-minimal |
There was a problem hiding this comment.
high · The new gate does not protect itself. .github/CODEOWNERS still falls under *, so one minimalist can approve a PR that deletes line 8 or changes its owner. The next allowlist entry then needs no security owner. Add a .github/CODEOWNERS rule after * with the same owner set as line 8. This repeats thread r4421165650, which has no reply. CODEOWNERS has only these two rules, so the fix is local.
| # (packages/<name>/pkgscan-allow.toml, read by pkgmgr-rs). The security owner | ||
| # approves every entry; agents may draft one but never merge it. Listed after | ||
| # `*`, so it replaces that rule for these files: CODEOWNERS uses the last match. | ||
| packages/*/pkgscan-allow.toml @bryan-minimal |
There was a problem hiding this comment.
medium · @bryan-minimal is the only owner, and the PR body says agent sessions open pkgs PRs from that account. An author cannot approve their own PR, so an allowlist PR from that account cannot merge. The PR body says the ruleset has no bypass actors. The triaged finding then keeps gating the update until someone opens the PR from another account. Choose the owner set before merge. A team with at least two humans keeps the security intent. This repeats thread r4421165639, which has no reply. I could not read the ruleset myself.
There was a problem hiding this comment.
Follow-up to my Nov 4 review on the same commit: one new high finding inline. The earlier findings still stand.
| # (packages/<name>/pkgscan-allow.toml, read by pkgmgr-rs). The security owner | ||
| # approves every entry; agents may draft one but never merge it. Listed after | ||
| # `*`, so it replaces that rule for these files: CODEOWNERS uses the last match. | ||
| packages/*/pkgscan-allow.toml @bryan-minimal |
There was a problem hiding this comment.
high · The comment says that the security owner approves every entry and that agents only draft. The rule cannot prove either claim. The PR body says agent sessions act as bryan-minimal, and that is the only account that can give the owner approval. An agent session with that credential can approve an allowlist PR that another account opened. Option 3 in the PR body creates exactly this flow. The entry then clears the unattended supply-chain gate with no human security review. Make the owner a team that excludes every account that agents use. Option 1 fixes this only if bryan-minimal is not the agent account on that team. I did not read the token scope of the agent sessions, so confidence is limited. CODEOWNERS has one such rule, so the fix is local.
Adds
packages/*/pkgscan-allow.toml @bryan-minimalto.github/CODEOWNERS.Why. gominimal/pkgmgr-rs#1105 made
packages/<name>/pkgscan-allow.tomlthe place where a triaged pkgscan finding stops gating the unattended update, which mirrors bytes and opens PRs. An entry is a security decision, so the security owner reviews each one. pkgmgr-rs#1121 adds the matching agent norm: agents may draft an entry but never merge one.The
mainruleset already hasrequire_code_owner_review: true, so this takes effect on merge. CODEOWNERS uses the last matching pattern, so for these files the line replaces* @gominimal/minimalistsinstead of adding to it.Decision for the reviewer: possible deadlock. Agent sessions open pkgs PRs as
bryan-minimal, and GitHub doesn't let an author approve their own PR. The ruleset has no bypass actors. So with@bryan-minimalas the only owner, nobody can approve an allowlist PR opened from Bryan's account, whether an agent or Bryan opened it. Options:@gominimal/securityteam with at least two humans as the owner (my suggestion).@bryan-minimal @gominimal/minimalists: any one of them approves. That is the same gate as today, plus a review request to Bryan.The line is left exactly as asked, so this is your call. Please pick one before merging. The agent did not merge this; pkgs PRs are merged by humans.
🤖 Generated with Claude Code