Skip to content

Commit edfb39c

Browse files
author
matthew
committed
fix(docs): the numeric-id role narrows grants, it does not split buckets
An audit of the previous commit cleared it — role matching in core is exact map lookup with no wildcard, no consumer parses roles expecting a `github:org/` shape, the numeric id arrives on the same authenticated `/user` body as the login so it is no more forgeable, and a hostile org literally named `id/5` yields `github:org/id/5`, not `github:id/5`. But it caught my doc comment overclaiming. Binding to `github:id/<id>` protects the GRANT side: an impostor who re-registers a released handle carries a different numeric role, so an exact-match binding does not follow them. It does NOT separate the BUCKETS. The enforcement subject is still `principal.id`, so the re-registered handle keeps landing on the same `user:github:<login>` group, usage ledger and budget as the previous owner. Splitting those means changing the principal id, which is the breaking change this deliberately avoids — so the limit is now stated where an operator reading the comment will see it, rather than implied fixed. Also drops a test I added that was a strict subset of an existing one covering the same two cases plus a positive control. And gates this repo's release workflow on its test suite: `ci.yml` is armed on branch pushes and PRs only, so a `v*` tag ran release.yml with no test job in the graph at all, and the build-provenance attestation would have certified the provenance of an untested artifact. The gate runs the repo's own suite against the core SHA pinned in `.busbar-ref` — the commit the release actually builds against, not a moving branch.
1 parent bf11020 commit edfb39c

3 files changed

Lines changed: 49 additions & 12 deletions

File tree

.github/workflows/release.yml

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,9 +28,44 @@ permissions:
2828
attestations: write # record the build-provenance attestation
2929

3030
jobs:
31+
# ── TEST GATE ────────────────────────────────────────────────────────────────────────────────
32+
# Nothing is built, signed or attested until this repo's own suite passes AGAINST THE EXACT CORE
33+
# COMMIT THIS RELEASE WILL BUILD AGAINST.
34+
#
35+
# This was missing, and the gap was not theoretical: `ci.yml` is armed on branch pushes and PRs
36+
# only, so a `v*` tag event did not trigger it AT ALL, while `release-on-upstream.yml`'s cron cuts
37+
# those tags automatically. A signed, provenance-attested plugin could therefore ship from a
38+
# commit whose suite never ran in that run -- the attestation would faithfully certify the
39+
# provenance of an untested artifact.
40+
#
41+
# `busbar_ref` is resolved from `.busbar-ref`, NOT from the branch: the release builds against a
42+
# pinned core SHA, so the gate has to test against that same SHA or it is not testing what ships.
43+
# Testing against a moving branch instead is the other half of the same defect.
44+
resolve-busbar-ref:
45+
runs-on: ubuntu-latest
46+
outputs:
47+
sha: ${{ steps.ref.outputs.sha }}
48+
steps:
49+
- uses: actions/checkout@v7
50+
- name: Resolve busbar ref from .busbar-ref
51+
id: ref
52+
run: echo "sha=$(cut -d' ' -f1 .busbar-ref)" >> "$GITHUB_OUTPUT"
53+
shell: bash
54+
55+
gate:
56+
needs: resolve-busbar-ref
57+
uses: GetBusbar/busbar/.github/workflows/plugin-ci.yml@dev
58+
with:
59+
plugin_crate: busbar-auth-github-plugin
60+
plugin_kind: auth
61+
plugin_alias: github
62+
service: wiremock
63+
busbar_ref: ${{ needs.resolve-busbar-ref.outputs.sha }}
64+
3165
# Create the Release first so the parallel per-target upload jobs have something to attach to
3266
# (uploading from a matrix without a pre-existing release races -> "release not found").
3367
create-release:
68+
needs: gate
3469
runs-on: ubuntu-latest
3570
steps:
3671
- uses: actions/checkout@v7

busbar-auth-github/src/lib.rs

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -405,11 +405,20 @@ pub fn parse_org_groups(body: &str) -> Option<Vec<String>> {
405405
/// anywhere reporting a change of person.
406406
///
407407
/// So the stable numeric account id is emitted ALONGSIDE, as the `github:id/<id>` role. That is
408-
/// purely additive: every existing login binding keeps working untouched, and an operator who wants
409-
/// an identifier that cannot be transferred can bind to `github:id/12345` and migrate at their own
410-
/// pace. New deployments should prefer it. The same reasoning applies to `github:org/<org>`, which is
411-
/// likewise a renameable slug, but GitHub's `/user/orgs` entries are reduced to their login here and
412-
/// closing that one needs a payload change rather than a one-line addition.
408+
/// purely additive: every existing login binding keeps working untouched, and an operator can bind to
409+
/// `github:id/12345` and migrate at their own pace. New deployments should prefer it.
410+
///
411+
/// WHAT THIS DOES AND DOES NOT FIX, because the difference is easy to overread. It narrows WHO GETS
412+
/// GRANTED: a binding on `github:id/<id>` cannot be inherited by whoever re-registers the handle,
413+
/// because core matches roles by exact lookup and the impostor carries a different numeric role. It
414+
/// does NOT separate the BUCKETS: the enforcement subject is still `principal.id`, so the
415+
/// re-registered handle continues to land on the same `user:github:<login>` group, usage ledger and
416+
/// budget as its previous owner. Separating those means changing the principal id, which is the
417+
/// breaking change this deliberately avoids.
418+
///
419+
/// `github:org/<org>` has the same weakness, being likewise a renameable slug. Closing that one needs
420+
/// the org's numeric id threaded through `parse_org_groups`, a payload change rather than a one-line
421+
/// addition.
413422
pub fn build_principal(user: &GhUser, org_groups: Vec<String>) -> Principal {
414423
let mut p = Principal::from_id(format!("github:{}", user.login));
415424
p.name = Some(user.name.clone().unwrap_or_else(|| user.login.clone()));

busbar-auth-github/src/tests.rs

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -749,10 +749,3 @@ fn the_stable_numeric_id_distinguishes_two_accounts_sharing_one_handle() {
749749
b.roles
750750
);
751751
}
752-
753-
/// A `/user` body with no numeric `id` still fails closed, as it always did.
754-
#[test]
755-
fn a_user_body_without_a_numeric_id_is_rejected() {
756-
assert!(parse_user(r#"{"login":"octocat"}"#).is_none());
757-
assert!(parse_user(r#"{"login":"octocat","id":"not-a-number"}"#).is_none());
758-
}

0 commit comments

Comments
 (0)