Skip to content

fix: revalidate package publish owners before insert#2281

Open
vyctorbrzezowski wants to merge 1 commit into
openclaw:mainfrom
vyctorbrzezowski:contrib/package-publish-final-owner-check
Open

fix: revalidate package publish owners before insert#2281
vyctorbrzezowski wants to merge 1 commit into
openclaw:mainfrom
vyctorbrzezowski:contrib/package-publish-final-owner-check

Conversation

@vyctorbrzezowski
Copy link
Copy Markdown
Contributor

@vyctorbrzezowski vyctorbrzezowski commented May 16, 2026

Summary

Revalidates package publish authority immediately before inserting a package release.

What changed

  • Final release insert rechecks actor state.
  • Final release insert rechecks owner user state.
  • Final release insert rechecks owner publisher active state.
  • A publish that started while valid cannot complete if the actor/owner/publisher is banned, deleted, or deactivated before the final write.

Public behavior

Package publish rejects the final insert when the actor, requested owner, or owner publisher becomes invalid during the publish window. It does not create a package or release row under stale authority.

Behavior proof

Live Convex runtime proof from the final package publish insert boundary:

$ bunx convex run --push --typecheck=disable --codegen disable proof2281:run
- Preparing Convex functions...

✔ Convex functions ready!
{
  "actorRevoked": {
    "publish": {
      "error": "Unauthorized",
      "ok": false
    },
    "revokedPrincipal": "actor",
    "snapshot": {
      "packageInserted": false,
      "releaseCount": 0
    }
  },
  "ownerRevoked": {
    "publish": {
      "error": "Package owner is unavailable",
      "ok": false
    },
    "revokedPrincipal": "owner",
    "snapshot": {
      "packageInserted": false,
      "releaseCount": 0
    }
  },
  "publisherRevoked": {
    "publish": {
      "error": "Package owner publisher is unavailable",
      "ok": false
    },
    "revokedPrincipal": "publisher",
    "snapshot": {
      "packageInserted": false,
      "releaseCount": 0
    }
  }
}

This proof starts from valid publish principals, revokes one principal before the final insert, and confirms the release insert aborts without creating package/release data.

Focused regression suite:

$ bun run test convex/packages.public.test.ts
Test Files  1 passed (1)
Tests       132 passed (132)

Validation

$ bun run ci:unit
Test Files  204 passed (204)
Tests       1988 passed (1988)
Statements  85.98%
Branches    75.17%
Functions   86.37%
Lines        89.48%
$ bun run ci:types-build
tsc, package typechecks, clawhub-mod typecheck, Vite build, and Nitro build passed.

Current GitHub CI for this head also has packages, types-build, e2e-http, playwright-smoke, and playwright-local-auth passing. The static job currently stops at bun audit on the existing transitive ws advisory GHSA-58qx-3vcg-4xpx.

@vyctorbrzezowski vyctorbrzezowski requested review from a team and Patrick-Erichsen as code owners May 16, 2026 04:00
@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented May 16, 2026

@vyctorbrzezowski is attempting to deploy a commit to the Amantus Machina Team on Vercel.

A member of the Team first needs to authorize it.

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 16, 2026

Codex review: needs changes before merge.

Workflow note: Future ClawSweeper reviews update this same comment in place.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Summary
The branch adds final insert active-state rechecks for package publish actor, owner user, and owner publisher, plus regression tests, a security spec note, and a changelog entry.

Reproducibility: yes. Source inspection shows user org membership is checked before scan/hash work, while the final insert receives ids and still does not re-query publisherMembers before writing package data.

PR rating
Overall: 🧂 unranked krab
Proof: 🦞 diamond lobster
Patch quality: 🧂 unranked krab
Summary: The runtime proof is strong for the covered cases, but the patch is not quality-ready until the stale org-membership authorization gap is fixed.

Rank-up moves:

  • Add the final user org-membership recheck and a regression test for removed publisher membership before insert.
  • Update the security spec note if final publish authority is intended to include publisher membership as well as active principal state.
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

PR egg
🔥 Warming up: proof, findings, or rank-up moves are still in progress.

       .-.
    .-'   '-.
   /  _   _  \
  |  (_) (_)  |
   \   ___   /
    '.___._.'
What is this egg doing here?
  • Eggs appear after the PR passes real-behavior proof. It is here for vibes, not verdicts: it does not change labels, ratings, merge decisions, or automation.
  • The shell reacts to review momentum: open follow-up work warms it up, re-review makes it wobble, and a clean final review lets it hatch.
  • How to hatch it: reach status: 👀 ready for maintainer look or status: 🚀 automerge armed; that usually means sufficient real-behavior proof, no blocking P0/P1/P2 findings, no security attention needed, and clean correctness.
  • The hatch is seeded from this repository and PR number, so the same PR keeps the same creature; the reviewed head SHA can only change safe visual details.
  • Rarity is just collectible sparkle: 🥚 common, 🌱 uncommon, 💎 rare, ✨ glimmer, and 🌈 legendary.

Real behavior proof
Sufficient (terminal): The PR body includes live Convex terminal output showing actor, owner, and publisher revocation each rejects final insertion without creating package or release data.

Risk before merge
Why this matters: - Merging intentionally makes in-flight publishes fail closed if actor, owner, or owner publisher active state changes before the final write; maintainers should accept that user-visible compatibility behavior.

  • The patch still allows a user org publish to complete after the actor's publisher membership is revoked during validation/scanning, leaving the final authorization boundary incomplete.

Maintainer options:

  1. Complete final authority revalidation (recommended)
    Add a final user-publish org membership recheck before package/release inserts, with regression coverage for membership removal during the publish window.
  2. Accept active-state-only hardening
    Maintainers may land the active-principal checks as a narrower hardening step only if they intentionally track stale publisher membership as a separate security follow-up.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Add a final user-publish org membership recheck in `insertReleaseInternal` before package/release inserts, with a regression test that removes the actor's publisher membership after initial publish target resolution. Preserve trusted GitHub Actions publish behavior and the new actor/owner/publisher active-state checks.

Next step before merge
There is a narrow, actionable security-boundary repair: recheck publisher membership for user org publishes at the final insert and add focused regression coverage.

Security
Needs attention: The diff strengthens package publish authorization but leaves a concrete stale-membership gap at the final write boundary.

Review findings

  • [P1] Recheck org membership before inserting user publishes — convex/packages.ts:5197-5203
Review details

Best possible solution:

Land final insert authority revalidation that covers active principals and user org membership, with regression tests for membership removal and no change to trusted GitHub Actions semantics.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection shows user org membership is checked before scan/hash work, while the final insert receives ids and still does not re-query publisherMembers before writing package data.

Is this the best way to solve the issue?

No. The active-state checks are the right direction, but final publish authority is not fully revalidated until user org membership revocation is covered too.

Label justifications:

  • P2: This is a focused package publish authorization hardening PR with limited blast radius but security-sensitive behavior.
  • merge-risk: 🚨 compatibility: The PR deliberately turns some in-flight publishes into final-write failures when authority changes during the publish window.

Full review comments:

  • [P1] Recheck org membership before inserting user publishes — convex/packages.ts:5197-5203
    For user publishes to an org owner, publishPackageImpl resolves membership before the scan/hash window. The new final insert checks active actor, owner, and publisher rows, but it still accepts the release if the actor's publisherMembers row was removed before this mutation, so a revoked org member can complete an in-flight publish. Re-read membership for user publishes with ownerPublisherId before inserting rows, while preserving the trusted GitHub Actions path.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.86

Security concerns:

  • [medium] Org membership is not revalidated at final insert — convex/packages.ts:5197
    A user publish to an org owner can pass the early membership check, lose publisher membership during validation/scanning, and still insert the package release because insertReleaseInternal does not re-query publisherMembers.
    Confidence: 0.86

Acceptance criteria:

  • bun run test convex/packages.public.test.ts
  • bun run ci:unit
  • bun run ci:types-build

What I checked:

  • PR proof: The PR body includes live Convex terminal output showing actor, owner, and publisher revocation each rejects final insertion without package or release data. (d188894d9ab7)
  • Patch rechecks active principals only: The PR head re-reads actor, owner, and ownerPublisher at insertReleaseInternal, but the added block does not re-query publisherMembers for user org publishes. (convex/packages.ts:5197, d188894d9ab7)
  • Initial org publish authorization is early: Current main resolves the owner publisher before scanning and hashing, using resolvePublishTargetForUserInternal with minimumRole publisher. (convex/packages.ts:4377, 2aa2a449e5fc)
  • Membership check happens before final insert: resolvePublishTargetForUserInternal checks getPublisherMembership and role before returning the owner publisher id, but that result is only an early authorization snapshot. (convex/publishers.ts:816, 2aa2a449e5fc)
  • Final insert receives ids after the long publish window: publishPackageImpl passes actorUserId, ownerUserId, ownerPublisherId, and publishActor into insertReleaseInternal after scan/hash work, so final membership must be revalidated there if membership can change mid-publish. (convex/packages.ts:4571, 2aa2a449e5fc)
  • Existing membership helper is indexed: getPublisherMembership already queries publisherMembers by the by_publisher_user index, giving a clear low-blast-radius repair path. (convex/lib/publishers.ts:462, 2aa2a449e5fc)

Likely related people:

  • Peter Steinberger: Current package publish and insert-release lines blame to his v0.16.0 package publish snapshot, and git history shows he introduced the native package registry and publisher org ownership foundations. (role: recent area contributor; confidence: high; commits: 1a00013c21b1, fc7d4dadca25, f6ce8f9e1efa; files: convex/packages.ts, convex/publishers.ts, convex/lib/publishers.ts)
  • Patrick Erichsen: Recent merged work touched package ownership and publisher membership behavior near the same package/publisher authorization surface. (role: adjacent package ownership contributor; confidence: medium; commits: 1aab139775a4, c51cfe2459f3; files: convex/packages.ts, convex/packages.public.test.ts, convex/publishers.ts)
  • Onur: Introduced the GitHub OIDC trusted package publishing path that shares insertReleaseInternal and must be preserved when adding final user-membership checks. (role: trusted publishing feature author; confidence: medium; commits: 8592272720fe; files: convex/packages.ts, convex/packagePublishTokens.ts, convex/httpApiV1/packagesV1.ts)

Codex review notes: model gpt-5.5, reasoning high; reviewed against 2aa2a449e5fc.

@clawsweeper clawsweeper Bot added P2 Normal backlog priority with limited blast radius. impact:security Security boundary, credential, authz, sandbox, or sensitive-data risk. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed impact:security Security boundary, credential, authz, sandbox, or sensitive-data risk. labels May 17, 2026
@vyctorbrzezowski vyctorbrzezowski force-pushed the contrib/package-publish-final-owner-check branch from ce25b8f to 64bf59c Compare May 18, 2026 15:01
@vyctorbrzezowski
Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 18, 2026

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. label May 18, 2026
@vyctorbrzezowski vyctorbrzezowski force-pushed the contrib/package-publish-final-owner-check branch from 64bf59c to d188894 Compare May 18, 2026 20:13
@vyctorbrzezowski
Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels May 18, 2026
@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 18, 2026

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels May 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal backlog priority with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant