-
Notifications
You must be signed in to change notification settings - Fork 0
fix(security): restack atomic product publisher lease #378
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
Closed
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
8f91601
test(security): require atomic product publisher lease
seonghobae fdfebdb
fix(security): publish proposal refs atomically
seonghobae 874e1eb
docs(doctoring): record atomic publisher authority
seonghobae 4af180c
fix(security): revalidate PR identity before cleanup
seonghobae 1dacdb2
test(security): bind publisher cleanup to recovered PR identity
seonghobae 6c8083f
docs(security): document exact PR cleanup revalidation
seonghobae File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| # Atomic product-publisher ref and pull-request lease | ||
|
|
||
| ## Status and scope | ||
|
|
||
| Reviewed on 2026-08-15 against protected `main` `2db716d7252603689b2cc18b700bac25e872b28f`. This record applies only to the credential-bearing `publish_product_increment` stage in `.github/workflows/hourly-product-development.yml`. It does not grant review, merge, release, deployment, or licensing authority, and it does not change the NVIDIA NIM proposer/verifier trust split protected by #366. | ||
|
|
||
| This clean successor restacks only the unique atomic publisher behavior from stale Draft #80. The stale branch and its old checks are historical evidence; no predecessor CI, review, or scanner result transfers to this successor. | ||
|
|
||
| ## Problem and RED condition | ||
|
|
||
| Protected `main` first inventories a generated branch with `git ls-remote`, then performs an unconditional branch push. On later failure it deletes the branch by name without proving that the remote ref still equals the proposal commit. That is a check-then-act race: another actor may create the same ref after inventory, or may advance/recreate a publisher-created ref before cleanup. | ||
|
|
||
| `test/hourly-product-development-publisher-lease.test.ts` is the executable RED contract. Against the protected-main implementation it rejects the unguarded push/delete sequence and requires: | ||
|
|
||
| - expected-absence branch creation in the same Git ref update; | ||
| - exact-proposal-head cleanup rather than deletion by name; | ||
| - machine-readable REST pull-request creation; | ||
| - cleanup armed before a possibly successful create request can lose its response; | ||
| - server-side head/base identity revalidation; and | ||
| - fully paginated post-create open-PR inventory before publication is accepted. | ||
|
|
||
| ## Decision | ||
|
|
||
| ### Explicit expected-absence ref creation | ||
|
|
||
| The publisher captures the exact local proposal commit and uses Git's explicit lease form: | ||
|
|
||
| ```sh | ||
| proposal_head="$(git rev-parse HEAD)" | ||
| git push --force-with-lease="refs/heads/${branch}:" \ | ||
| origin "HEAD:refs/heads/${branch}" | ||
| ``` | ||
|
|
||
| An empty explicit expectation means the mutation is accepted only while the destination ref is absent. The preliminary `git ls-remote` inventory is removed because it is observation, not write authority. | ||
|
|
||
| ### Exact-head cleanup | ||
|
|
||
| Cleanup is conditioned on the branch still identifying the exact proposal commit created by this run: | ||
|
|
||
| ```sh | ||
| cleanup_remote_branch() { | ||
| git push --force-with-lease="refs/heads/${branch}:${proposal_head}" \ | ||
| origin ":refs/heads/${branch}" >/dev/null 2>&1 || true | ||
| } | ||
| ``` | ||
|
|
||
| If another actor advances or replaces the ref, cleanup fails harmlessly rather than deleting foreign state. | ||
|
|
||
| ### Recoverable pull-request identity | ||
|
|
||
| The publisher creates a pull request through GitHub's REST endpoint with a 256-bit correlation marker in the bounded body. A cleanup trap is armed before `POST /pulls`. If the client response is lost or malformed after possible server-side success, the publisher performs a paginated head-scoped query and accepts exactly one candidate only when its `head.sha`, `base.sha`, and hidden marker all match this run. Cleanup does not trust a previously returned numeric PR identifier: immediately before any close it discards that identifier, repeats the marker/head/base recovery, and closes only the single recovered positive integer pull-request number. If recovery is unavailable, ambiguous, or no longer matches the exact proposal identity, the PR is left open for manual investigation rather than being closed by stale identity. | ||
|
|
||
| After creation, the publisher re-reads that exact pull request and requires `head.sha == proposal_head` and `base.sha == expected_base`. It then paginates the complete open-PR queue and accepts publication only when the created pull request is the sole open PR. Missing, malformed, ambiguous, or unavailable evidence remains failure. | ||
|
|
||
| ## Authority and rollback boundaries | ||
|
|
||
| The proposer may use `NVIDIA_NIM_API_KEY` but has no shell execution authority. A separate uncredentialed job executes `npm run release:verify`; the credential-bearing publisher reconstructs and publishes only the already verified immutable proposal. The Maintainer App token never becomes model/verifier authority. This change does not weaken the central Security Scan, configured coverage, package, SBOM/provenance, review, protected-base, or release gates. | ||
|
|
||
| Rollback is source rollback of this bounded publisher change. No force-push, destructive rebase, branch-protection bypass, self-approval, repair workflow, or alternate credential is required. | ||
|
|
||
| ## Source-supported rationale | ||
|
|
||
| Git 2.55.0 documents `--force-with-lease=<refname>:<expect>` as conditioning a ref update on the named ref still having the explicitly supplied current value. This is the relevant compare-and-swap primitive for both expected-absence creation and exact-head cleanup. | ||
|
|
||
| GitHub's current pull-request REST API provides structured creation, retrieval, head filtering, and pagination. These primitives avoid parsing human-oriented CLI output and allow the publisher to rebind publication to exact server-observed commit identities. | ||
|
|
||
| NIST SSDF 1.1 requires organizations to protect software and development environments from unauthorized access and tampering and to maintain provenance/integrity controls across the software lifecycle. The repository therefore keeps model generation, executable verification, publication credentials, checks, review, merge, release, and deployment as separate authorities. | ||
|
|
||
| ## References (APA 7th) | ||
|
|
||
| Git Project. (2026). *git-push documentation (Version 2.55.0).* https://git-scm.com/docs/git-push | ||
|
|
||
| GitHub, Inc. (2026). *REST API endpoints for pull requests.* https://docs.github.com/en/rest/pulls/pulls | ||
|
|
||
| Souppaya, M., Scarfone, K., & Dodson, D. (2022). *Secure software development framework (SSDF) version 1.1: Recommendations for mitigating the risk of software vulnerabilities* (NIST Special Publication 800-218). National Institute of Standards and Technology. https://doi.org/10.6028/NIST.SP.800-218 |
118 changes: 118 additions & 0 deletions
118
test/hourly-product-development-publisher-lease.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| import { readFileSync } from "node:fs"; | ||
| import { describe, expect, it } from "vitest"; | ||
|
|
||
| const workflowPath = ".github/workflows/hourly-product-development.yml"; | ||
|
|
||
| /** Return the trusted publication job so lease assertions cannot match an untrusted stage. */ | ||
| function publisherJob(): string { | ||
| const workflow = readFileSync(workflowPath, "utf8"); | ||
| const start = workflow.indexOf(" publish_product_increment:"); | ||
| expect(start).toBeGreaterThan(-1); | ||
| return workflow.slice(start); | ||
| } | ||
|
|
||
| describe("hourly product-development publisher ref lease", () => { | ||
| it("atomically creates a proposal ref only when the remote ref is absent", () => { | ||
| const publisher = publisherJob(); | ||
| const push = 'git push --force-with-lease="refs/heads/${branch}:" origin "HEAD:refs/heads/${branch}"'; | ||
|
|
||
| expect(publisher).toContain(push); | ||
| expect(publisher).not.toContain('git push origin "HEAD:refs/heads/${branch}"'); | ||
| expect(publisher).not.toContain("git ls-remote --exit-code --heads origin"); | ||
| }); | ||
|
|
||
| it("never deletes a raced or subsequently changed proposal ref during cleanup", () => { | ||
| const publisher = publisherJob(); | ||
| const headCapture = 'proposal_head="$(git rev-parse HEAD)"'; | ||
| const createPush = 'git push --force-with-lease="refs/heads/${branch}:" origin "HEAD:refs/heads/${branch}"'; | ||
| const cleanupLease = 'git push --force-with-lease="refs/heads/${branch}:${proposal_head}" origin ":refs/heads/${branch}"'; | ||
| const trap = "trap cleanup_remote_branch ERR"; | ||
|
|
||
| const headIndex = publisher.indexOf(headCapture); | ||
| const pushIndex = publisher.indexOf(createPush); | ||
| const trapIndex = publisher.indexOf(trap); | ||
|
|
||
| expect(headIndex).toBeGreaterThan(-1); | ||
| expect(pushIndex).toBeGreaterThan(headIndex); | ||
| expect(trapIndex).toBeGreaterThan(pushIndex); | ||
| expect(publisher).toContain(cleanupLease); | ||
| expect(publisher).not.toContain('git push origin --delete "$branch"'); | ||
| }); | ||
|
|
||
| it("arms recoverable numeric cleanup before machine-readable PR creation", () => { | ||
| const publisher = publisherJob(); | ||
| const marker = "publication_marker="; | ||
| const createPr = 'gh api --method POST "repos/${GITHUB_REPOSITORY}/pulls" --input "$pr_request_file"'; | ||
| const parseNumber = 'pr_number="$(jq -r'; | ||
| const recoverNumber = "recover_created_pr_number"; | ||
| const installCreatedPrCleanup = "trap cleanup_created_pr ERR"; | ||
| const closeCreatedPr = 'gh api --method PATCH "repos/${GITHUB_REPOSITORY}/pulls/${pr_number}" -f state=closed'; | ||
| const readCreatedPr = 'gh api "repos/${GITHUB_REPOSITORY}/pulls/${pr_number}"'; | ||
| const headGuard = '[ "$live_pr_head" != "$proposal_head" ]'; | ||
| const baseGuard = '[ "$live_pr_base" != "$expected_base" ]'; | ||
| const clearTrap = "trap - ERR"; | ||
|
|
||
| const markerIndex = publisher.indexOf(marker); | ||
| const cleanupTrapIndex = publisher.indexOf(installCreatedPrCleanup, markerIndex); | ||
| const createPrIndex = publisher.indexOf(createPr, cleanupTrapIndex); | ||
| const parseNumberIndex = publisher.indexOf(parseNumber, createPrIndex); | ||
| const readCreatedPrIndex = publisher.indexOf(readCreatedPr, parseNumberIndex); | ||
| const headGuardIndex = publisher.indexOf(headGuard, readCreatedPrIndex); | ||
| const baseGuardIndex = publisher.indexOf(baseGuard, readCreatedPrIndex); | ||
| const clearTrapIndex = publisher.indexOf( | ||
| clearTrap, | ||
| Math.max(headGuardIndex, baseGuardIndex), | ||
| ); | ||
|
|
||
| expect(markerIndex).toBeGreaterThan(-1); | ||
| expect(cleanupTrapIndex).toBeGreaterThan(markerIndex); | ||
| expect(createPrIndex).toBeGreaterThan(cleanupTrapIndex); | ||
| expect(parseNumberIndex).toBeGreaterThan(createPrIndex); | ||
| expect(readCreatedPrIndex).toBeGreaterThan(parseNumberIndex); | ||
| expect(headGuardIndex).toBeGreaterThan(readCreatedPrIndex); | ||
| expect(baseGuardIndex).toBeGreaterThan(readCreatedPrIndex); | ||
| expect(clearTrapIndex).toBeGreaterThan(Math.max(headGuardIndex, baseGuardIndex)); | ||
| expect(publisher).toContain(recoverNumber); | ||
| expect(publisher).toContain("pulls?state=open&head="); | ||
| expect(publisher).toContain(closeCreatedPr); | ||
| expect(publisher).not.toContain("gh pr create"); | ||
| expect(publisher).not.toContain('gh pr close "$pr_url"'); | ||
| }); | ||
|
|
||
| it("revalidates a known PR number before cleanup can close it", () => { | ||
| const publisher = publisherJob(); | ||
| const cleanupStart = publisher.indexOf("cleanup_created_pr() {"); | ||
| const cleanupEnd = publisher.indexOf("trap cleanup_created_pr ERR", cleanupStart); | ||
| const cleanup = publisher.slice(cleanupStart, cleanupEnd); | ||
| const recover = 'pr_number="$(recover_created_pr_number 2>/dev/null || true)"'; | ||
| const closeCreatedPr = 'gh api --method PATCH "repos/${GITHUB_REPOSITORY}/pulls/${pr_number}" -f state=closed'; | ||
|
|
||
| expect(cleanupStart).toBeGreaterThan(-1); | ||
| expect(cleanupEnd).toBeGreaterThan(cleanupStart); | ||
| expect(cleanup.indexOf(recover)).toBeGreaterThan(-1); | ||
| expect(cleanup.indexOf(closeCreatedPr)).toBeGreaterThan(cleanup.indexOf(recover)); | ||
| expect(cleanup).not.toContain('if ! [[ "${pr_number:-}" =~ ^[1-9][0-9]*$ ]]'); | ||
| }); | ||
|
|
||
| it("rechecks the fully paginated open-PR queue after creation before accepting publication", () => { | ||
| const publisher = publisherJob(); | ||
| const baseGuard = '[ "$live_pr_base" != "$expected_base" ]'; | ||
| const paginatedInventory = "gh api --paginate"; | ||
| const openPullsEndpoint = 'repos/${GITHUB_REPOSITORY}/pulls?state=open&per_page=100'; | ||
| const numberProjection = "--jq '.[].number'"; | ||
| const queueConflict = "created_pull_request_queue_conflict"; | ||
| const clearTrap = "trap - ERR"; | ||
|
|
||
| const baseGuardIndex = publisher.indexOf(baseGuard); | ||
| const inventoryIndex = publisher.indexOf(paginatedInventory, baseGuardIndex); | ||
| const queueConflictIndex = publisher.indexOf(queueConflict, inventoryIndex); | ||
| const clearTrapIndex = publisher.indexOf(clearTrap, queueConflictIndex); | ||
|
|
||
| expect(baseGuardIndex).toBeGreaterThan(-1); | ||
| expect(inventoryIndex).toBeGreaterThan(baseGuardIndex); | ||
| expect(publisher).toContain(openPullsEndpoint); | ||
| expect(publisher).toContain(numberProjection); | ||
| expect(queueConflictIndex).toBeGreaterThan(inventoryIndex); | ||
| expect(clearTrapIndex).toBeGreaterThan(queueConflictIndex); | ||
| }); | ||
| }); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.