fix(hub): grant chat staging write by role, and take it back on demotion - #222
Merged
Merged
Conversation
The three call sites that give a new member access to the hidden chat staging folder granted a fixed value of 4. That value meant write before the permission bits were renumbered and means download now, so a member whose role carries no write bit could never upload an attachment and got a bare 403 with nothing in the UI to explain it. Grant write instead, but only to a role that may chat. The grant was previously unconditional, so a view-only member already carried it; left unconditional it would have handed them an upload path once the value became meaningful. set_privilege also has to move the grant in both directions. It only ever granted, so demoting a member from chat to view left the staging access behind and the demoted member kept uploading. Revoke it there instead.
… the package Privilege.WRITE resolves to 15 under server-essentials 1.3.1 and to 7 under 1.3.6, which republished the pre-1.3.0 bit layout. package.json asks for ^1.3.1, so a bare npm install picks up 1.3.6 and the grant would carry no write bit at all -- the exact 403 this change exists to fix, reintroduced by a dependency bump nobody would connect to chat. Pin the value alongside the other capability bits, which are spelled out for the same reason and say so in the file header.
…at value Both properties the attachment fix rests on can be undone by an edit that reads like a simplification, and neither shows up as a test failure today. The gate must refuse a view-only member. The grant went out regardless of role before, so rows for view-only members already exist in the wild; loosening the check to a partial bit overlap would admit exactly the role it exists to exclude, because the chat constant in the package overlaps read. The granted value must carry the write bit and must not be read from the package. server-essentials 1.3.6 republished the pre-1.3.0 layout, where Privilege.WRITE is 7 and carries no write bit in this schema, and package.json asks for ^1.3.1 — so a dependency bump nobody connects to chat would silently restore the 403. Reads the call sites as text rather than loading them, and imports only member-capability, which requires nothing. That keeps it in the same install-free workflow as the other regressions here. Checked against three mutations: restoring the package value, dropping the gate from one call site, and removing the revoke on demotion. Each fails the suite.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Server half of the chat-attachment fix. Pairs with drumee/schemas#182 and needs that one deployed first.
Replaces #221, which targeted
previewfrom a feature branch — this repository only accepts a PR intopreviewfromtest. Same three commits, rebased ontotest.Cause
A member whose workspace role is Chat could not attach a file in chat. The upload answered
403and the chat showed nothing — no attachment chip, no error.An attachment stages in the hidden folder
/__chat__/__upload__before it becomes a message. A chat member carries privilege7and has no write bit for the workspace, which is intended, so the invite path grants write on that one folder withassign_via 'no_traversal'to keep the raised access from reaching anything inside it.All three call sites granted a fixed value of
4. That meant write before the permission bits were renumbered (download 2 -> 4,write 4 -> 8) and means download now, so the grant no longer satisfied the upload ACL.Changes
_grantMembership,invite_with_roles, andset_privilegenow grant write, and only to a role that may chat.The role gate is not a precaution. The grant was previously handed out regardless of role, so view-only members already carry the row — 22 of 197 on stage. Changing the value without gating would have handed an upload path to members entitled only to read. It is written at the call site rather than a layer above, using
privilegeAllows(privilege, CAN_CHAT)fromservice/lib/member-capability.js, whose header explains why the bit is spelled out there rather than read fromConstants.permission.set_privilegealso revokes on the way down. It only ever granted, so demoting a member from chat to view-only left the staging access in place and the demoted member kept uploading. A role change has to move the grant in both directions.The granted value is pinned, not read from the package.
Privilege.WRITEresolves to15under server-essentials 1.3.1 and to7under 1.3.6, which republished the pre-1.3.0 layout.package.jsonasks for^1.3.1and only the lockfile holds 1.3.1, so a barenpm installwould carry no write bit at all — the exact 403 this fixes, reintroduced by a dependency bump nobody would connect to chat.Deploy precondition: install with
npm ci. Under 1.3.6 the ACL wordsrc: "write"becomes the chat bit and the role model is wrong well beyond chat.Verification
Deployed to the
aaronstage endpoint and exercised with real accounts, reading thepermissiontable to confirm:no_traversalResolved permission afterwards: the chat member reads 15 on the staging folder and 7 on the workspace root; the view-only member reads 3 on both; the owner is unchanged at 63.
offline/test/chat-upload-grant.test.jslocks the two properties an edit could quietly undo — that the gate refuses a view-only member even against a mask that overlaps read, and that the granted value carries the write bit and is not the value 1.3.6 would give. It reads the call sites as text and imports onlymember-capability, which requires nothing, so it runs in the existing install-free workflow. Checked against three mutations; each fails the suite.Noted, not fixed here
permission_grantrefuses to write in a workspace where no member holds63on'*', and five such workspaces exist on stage — their owners carry31or7. Every grant there rolls back as "New granting would create orphaned hub", so invitations and role changes fail in those workspaces too, silently. Separate issue.