fix(self-hosted): tighten tenant creation and config publication - #1309
Conversation
…ance Creating a community tenant verified that the community existed, that its id matched the subdomain, and that a separate Hive account was named as owner. It never checked that the named owner had any standing in the community, so the instance for any community could be claimed by an account with no part in it. Ownership is not recoverable afterwards: every later mutation authorises against tenants.owner, and the create upsert only re-enters an abandoned row or a same-owner inactive one. verifyCommunity is replaced by verifyCommunityControlledBy, which reads the community's team from the same bridge.get_community call and requires the claimed owner to hold the owner or admin role, matching who can change the community's own settings on chain. Mods are excluded: they moderate content but do not control the community's identity. The check fails closed, so a node error refuses the claim rather than granting it.
POST /v1/tenants deliberately does not write the served config file, with a comment explaining why: nginx serves any file that exists with no subscription check, so writing one for a tenant that has not paid puts a blog live. PATCH /v1/tenants/:username broke that invariant by regenerating the file unconditionally, so reserving a name and saving a config once was enough to be served until the periodic sweep removed the file, and re-saving restored it. Publication is now gated on the same rule syncAllConfigs already applies (status 'active'), shared as isPublishableTenant so the two cannot drift. The config is still persisted for any status, so an edit is never lost; the response reports whether it went live, which also gives the editor something to say when a saved change is not visible yet.
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughTenant creation now verifies that the owner controls the community. Tenant configuration updates persist for inactive tenants without generating served config files. Publication is limited to active subscriptions and is reported in the PATCH response. ChangesTenant validation and publication
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TenantsRoute as tenants.ts route
participant TenantService
participant HiveRPC as Hive RPC
TenantsRoute->>TenantService: verifyCommunityControlledBy(communityId, owner)
TenantService->>HiveRPC: fetch community and team data
HiveRPC-->>TenantService: return community and team
TenantService-->>TenantsRoute: return authorization result
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e30c94dd70
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| if (!(await TenantService.verifyCommunity(communityId))) { | ||
| return { ok: false, status: 400, error: 'Community not found' }; | ||
| if (!(await TenantService.verifyCommunityControlledBy(communityId, owner))) { |
There was a problem hiding this comment.
This ownership check is selected by the caller-controlled config.type, so an attacker can submit { "username": "hive-125125" } without declaring type: "community", pass the personal-blog branch with owner === username, and then fund blog:hive-125125; the payment listener activates that reservation while preserving the unmanageable community account as owner. The listener can also create the same owner=username row directly for a payment when no reservation exists (payment-listener.ts lines 341-351). Thus an arbitrary payer can still permanently reserve a community subdomain despite this check; hive-* community identifiers should require a prevalidated admin-owned reservation and must not be auto-created as personal tenants.
Useful? React with 👍 / 👎.
… body The ownership check added in the previous commit was selected by body.config.type, which the caller controls. Omitting it routed a request for a hive-NNNN subdomain through the personal blog branch, which only requires owner === username, so the community's subdomain could still be captured with no ownership check. A community account is a real Hive account, so the account existence check does not stop it, and funding the reservation activates a tenant the community's team can never reclaim. isCommunity is now derived from the username being claimed, with the declared type kept only as an additional trigger. communityId defaults to that username instead of being required in the body. resolveAndValidateTenant is exported and covered directly: the two tests for an undeclared and a blog-declared community name both fail against the previous logic.
|
Good find, and it defeated the whole point of the first commit. Fixed in 5c6de8c. The check was selected by
140 hosting API tests pass, typecheck clean. |
The ownership check added here covers POST /v1/tenants, but the payment listener creates a tenant of its own when a payment arrives for a name that has no row, with owner = username. A blog:hive-NNNN transfer therefore reached that path and took the community's subdomain without any community-control verification at all: the same squat, through the other door. A community account is a real Hive account, so the existence check that follows does not stop it, and the row is not recoverable afterwards. The create upsert only revives an abandoned row or a same-owner inactive one, so the real team could never claim it back. The listener now refuses to auto-create a community-shaped name and requires it to have been reserved through the validated route first. The name pattern moved to tenant-service so the route and the listener cannot drift on what counts as a community.
|
Correct, and it is the same squat through the other door. Fixed in 3aadfe4. I gated Not recoverable afterwards, which is what makes it a squat rather than a nuisance: the create upsert only revives an The listener now refuses to auto-create a community-shaped name and requires the reservation to have gone through the validated route first, rejected as permanent so the payment fails rather than retrying forever. The pattern moved to This is the second time this fix has been bypassed by a path I did not enumerate — first 143 tests pass, typecheck clean, and I confirmed this merges cleanly with #1306, which also touches |
1 participant
Two authorization gaps in the hosting API tenant lifecycle. Grouped because they touch the same routes and both concern which tenant state a caller can bring about.
Community instance creation
Creating a community tenant verified that the community existed, that its id matched the subdomain, and that a separate Hive account was named as owner. It never checked that the named owner had any standing in the community.
verifyCommunityis replaced byverifyCommunityControlledBy, which reads the communityteamfrom the samebridge.get_communitycall and requires the claimed owner to hold theowneroradminrole. That matches who can change the community’s own settings on chain. Mods are excluded: they moderate content but do not control the community’s identity.The check fails closed, so a node error refuses the claim rather than granting it. This matters because ownership is not recoverable after the fact: every later mutation authorizes against
tenants.owner, and the create upsert only re-enters anabandonedrow or a same-ownerinactiveone.Note
teamcarries[account, role, title]tuples, not objects, and the tests cover the malformed shapes.Config publication
POST /v1/tenantsdeliberately does not write the served config file, with a comment explaining why: nginx serves any file that exists with no subscription check.PATCH /v1/tenants/:usernamebroke that invariant by regenerating the file unconditionally, so a tenant that had never been activated could be served, and the periodic sweep that removes such files could be undone by saving again.Publication is now gated on the same rule
syncAllConfigsalready applies (statusactive), extracted asisPublishableTenantso the two paths cannot drift.The config is still persisted for any status, so an edit is never lost. The response reports whether it went live, which also gives the config editor something to say when a saved change is not yet visible.
Verification
apps/web’s signup already sendsowner, and a community owner who administers the community is unaffected.Summary by CodeRabbit
New Features
Bug Fixes