Skip to content

fix: restrict membership management to org publishers#2285

Open
vyctorbrzezowski wants to merge 5 commits into
openclaw:mainfrom
vyctorbrzezowski:contrib/personal-publisher-membership-guard
Open

fix: restrict membership management to org publishers#2285
vyctorbrzezowski wants to merge 5 commits into
openclaw:mainfrom
vyctorbrzezowski:contrib/personal-publisher-membership-guard

Conversation

@vyctorbrzezowski
Copy link
Copy Markdown
Contributor

@vyctorbrzezowski vyctorbrzezowski commented May 16, 2026

Summary

Keeps personal publishers as identity aliases instead of collaborative org-like publishers.

What changed

  • Member add/update flows remain restricted to org publishers.
  • Personal publisher authority now comes only from linkedUserId === actorUserId.
  • Stale personal-publisher membership rows no longer authorize publish target resolution.
  • Skill transfer destinations no longer accept stale personal-publisher membership rows.
  • Package owner read/manage gates now treat user publishers as linked-user only.
  • Org publisher membership still authorizes org publishing normally.
  • Existing stale personal membership rows can remain for cleanup; they no longer grant authority.

Public behavior

Publishing, transferring, or managing through a personal publisher now requires that the personal publisher is linked to the acting user. A stale membership row cannot authorize a different user's personal publisher.

Org publisher membership remains valid for org-owned publishing.

Behavior proof

Live Convex runtime proof for the remaining stale personal-membership paths ClawSweeper called out:

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

✔ Convex functions ready!
{
  "directTransfer": {
    "error": "You do not have admin access for \"@proof2285-...-owner\". Ask an owner or admin to add you before transferring this skill.",
    "ok": false
  },
  "packageAppeal": {
    "error": "Unauthorized",
    "ok": false
  },
  "packageModerationStatus": {
    "error": "Unauthorized",
    "ok": false
  },
  "fixture": {
    "actorUserId": "<redacted>",
    "ownerHandle": "proof2285-...-owner",
    "packageName": "@proof2285-.../demo",
    "skillSlug": "proof2285-...-skill"
  }
}

This proof creates a stale personal-publisher membership for a non-linked actor and verifies that it cannot authorize direct skill transfer, package appeal submission, or owner-only package moderation status access.

Focused regression suite:

$ bun run test convex/publishers.test.ts convex/packages.public.test.ts convex/skills.ownership.test.ts
Test Files  3 passed (3)
Tests       167 passed (167)

Validation

$ bun run ci:unit
Test Files  204 passed (204)
Tests       1994 passed (1994)
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 unit, 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.

@openclaw-barnacle openclaw-barnacle Bot added the triage: refactor-only Candidate: refactor/cleanup-only PR without maintainer context. label May 16, 2026
@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
This PR changes Convex publisher, package, and skill authorization so personal publishers authorize through linkedUserId while org publishers continue to use membership roles.

Reproducibility: yes. Source inspection at PR head gives a high-confidence path: create a stale publisherMembers row for a kind: "user" publisher and call the package dashboard or skill owner/detail/dashboard/appeal paths as that non-linked user.

PR rating
Overall: 🧂 unranked krab
Proof: 🦞 diamond lobster
Patch quality: 🧂 unranked krab
Summary: The proof signal is strong for covered paths, but the patch is not quality-ready because high-severity authorization gaps remain.

Rank-up moves:

  • Harden the remaining package dashboard and skill owner/detail/dashboard/appeal gates.
  • Add stale-personal negative and org-positive regression tests for those remaining paths.
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 (live_output): The PR body includes redacted live Convex output showing stale personal memberships denied for direct transfer, package appeal, and package moderation status; it clears the proof gate for covered paths but not the source findings above.

Risk before merge
Why this matters: - Merging as-is would leave stale personal-publisher membership rows able to unlock owner-visible package dashboard listings and skill owner/detail/dashboard/appeal paths.

  • The live proof is useful for the paths it covers, but it does not prove the remaining source-visible owner gates are closed.

Maintainer options:

  1. Finish personal-publisher owner gates (recommended)
    Before merge, update the remaining package dashboard and skill owner/detail/dashboard/appeal checks so personal publishers ignore membership rows for non-linked users while org membership behavior stays intact.
  2. Move the sweep to a security branch
    If maintainers want a broader audit than this contributor branch should carry, pause this PR and land the same hardening through a maintainer-owned security branch with targeted proof.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Update PR head so `listDashboardPackagesForOwnerPublisher`, `getBySlug`, `list`, `listDashboardPaginated`, and `canUserAppealSkill` treat personal publishers as linked-user-only while preserving org membership authorization; add stale-personal negative and org-positive regression tests for those paths.

Next step before merge
A focused repair can update the remaining direct owner gates and add targeted regression tests without a product decision.

Security
Needs attention: The patch improves several authorization paths but leaves concrete stale-membership owner gates open.

Review findings

  • [P1] Ignore stale personal memberships in package dashboards — convex/packages.ts:915-917
  • [P1] Ignore stale personal memberships in skill owner gates — convex/skills.ts:1994
Review details

Best possible solution:

Finish the authorization sweep so every personal-publisher gate keys off linkedUserId only, while org publishers continue to authorize through membership roles with regression coverage for both cases.

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

Yes. Source inspection at PR head gives a high-confidence path: create a stale publisherMembers row for a kind: "user" publisher and call the package dashboard or skill owner/detail/dashboard/appeal paths as that non-linked user.

Is this the best way to solve the issue?

No. The linked-user-only direction is the right fix, but this patch is incomplete until the remaining direct owner gates use the same personal-versus-org authorization split.

Label justifications:

  • P1: The PR is repairing authorization behavior where stale publisher membership rows can still grant owner-only access.
  • merge-risk: 🚨 security-boundary: The diff changes publisher authorization semantics, and missed personal-publisher gates can preserve an authorization bypass.

Full review comments:

  • [P1] Ignore stale personal memberships in package dashboards — convex/packages.ts:915-917
    listDashboardPackagesForOwnerPublisher still treats any publisherMembers row as enough for isOwnDashboard before considering whether the publisher is personal. A stale member row on a kind: "user" publisher can still return owner dashboard package entries to a non-linked actor, so this path needs the same linked-user-only branch as the package owner helpers.
    Confidence: 0.92
  • [P1] Ignore stale personal memberships in skill owner gates — convex/skills.ts:1994
    The skill detail, list/dashboard, and appeal paths still use a raw membership row to decide owner access for publisher-owned skills. For personal publishers, that keeps stale rows authoritative for hidden/owner-only skill views and appeals; branch on publisher kind first and only honor membership rows for org publishers.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.91

Security concerns:

  • [high] Stale personal memberships still unlock package dashboards — convex/packages.ts:915
    A non-linked user with a stale personal-publisher membership can still satisfy isOwnDashboard in the package dashboard owner-publisher listing path.
    Confidence: 0.92
  • [high] Stale personal memberships still unlock skill owner paths — convex/skills.ts:1994
    Direct skill detail, dashboard listing, and appeal checks still treat personal-publisher membership rows as owner authority instead of requiring linkedUserId.
    Confidence: 0.9

Acceptance criteria:

  • bun run test convex/publishers.test.ts convex/packages.public.test.ts convex/skills.ownership.test.ts
  • bun run ci:unit

What I checked:

  • PR proof covers only some paths: The PR body includes redacted live Convex output showing stale personal memberships denied for direct skill transfer, package appeal submission, and package moderation status, plus focused Convex tests and unit/type-build validation; it does not prove package dashboard or direct skill owner/detail/dashboard paths. (16ece381443d)
  • Package dashboard still trusts any membership row: At PR head, listDashboardPackagesForOwnerPublisher still sets isOwnDashboard from membership || linkedUserId, so a stale member row on a personal publisher can still expose owner dashboard package entries to a non-linked user. (convex/packages.ts:915, 16ece381443d)
  • Skill owner gates still trust personal memberships: At PR head, getBySlug, list, listDashboardPaginated, and canUserAppealSkill still derive owner/dashboard/appeal authority from publisherMembers rows without first excluding kind: "user" publishers for non-linked actors. (convex/skills.ts:1985, 16ece381443d)
  • Patch direction is correct in shared helpers: The diff hardens assertCanManageOwnedResource, requirePublisherRole, resolvePublisherForActor, member management, package owner access helpers, and skill transfer destinations, but the remaining direct skill/package dashboard call sites bypass those helper semantics. (convex/lib/publishers.ts:129, 16ece381443d)
  • History points to publisher ownership feature area: git log -S found the publisher ownership feature in f6ce8f9e and dashboard pagination in 964fc0fa; current-main blame for the affected skill gates points through the v0.16 merge snapshot 1a00013c. (convex/skills.ts:1994, f6ce8f9e1efa)

Likely related people:

  • Peter Steinberger: git log -S ties publisher org ownership to f6ce8f9e, current blame for the affected owner checks points through 1a00013c, and shortlog shows the largest contribution share across the touched Convex ownership files. (role: publisher ownership feature owner and heavy area contributor; confidence: high; commits: f6ce8f9e1efa, 1a00013c21b1, 8287c494b20c; files: convex/skills.ts, convex/packages.ts, convex/publishers.ts)
  • Patrick Erichsen: Recent history touches package/skill moderation and owner-visible surfaces, including canUserAppealSkill provenance from c4d1fcdb and publisher/package follow-up work in nearby commits. (role: recent security and owner-surface contributor; confidence: medium; commits: c4d1fcdbc6be, c51cfe2459f3, 636750fbf826; files: convex/skills.ts, convex/packages.ts)
  • Vlad Ursul: git log -S for the skill dashboard ownership check identifies 964fc0fa as the dashboard pagination change relevant to listDashboardPaginated. (role: dashboard pagination contributor; confidence: medium; commits: 964fc0fa87f9; files: convex/skills.ts)

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

@vyctorbrzezowski vyctorbrzezowski force-pushed the contrib/personal-publisher-membership-guard branch from 7d3ffa9 to 33ffa7f Compare May 16, 2026 22:44
@clawsweeper clawsweeper Bot added P1 High-priority user-facing bug, regression, or broken workflow. 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. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. and removed impact:security Security boundary, credential, authz, sandbox, or sensitive-data risk. labels May 16, 2026
@vyctorbrzezowski vyctorbrzezowski force-pushed the contrib/personal-publisher-membership-guard branch from 33ffa7f to 5f97962 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:

@vyctorbrzezowski vyctorbrzezowski force-pushed the contrib/personal-publisher-membership-guard branch from 5f97962 to 8f08815 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. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed 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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 High-priority user-facing bug, regression, or broken workflow. 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. triage: refactor-only Candidate: refactor/cleanup-only PR without maintainer context.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant