fix(hub): grant chat staging write by role, and take it back on demotion - #221
aaron-tsar wants to merge 4 commits into
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.
|
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 1s —— View job I'll analyze this and get back to you. |
… 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.
|
Amended: the grant value is no longer read from the package.
The value is pinned as Deploy precondition: install with |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
…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.
|
Checked the bot replies on this PR. One is real, one is not. Not a finding: the SonarCloud quality gate. It fails on Also not findings: the Codex reviewer hit its usage limit and produced nothing, and the Claude action errored after 0s and 1s — infrastructure, no review either time. Added anyway, because the coverage gate had a point even if the number is noise. This is a permission gate and it had no test.
It also reads the three call sites as text and asserts each grant sits behind a chat-bit check, and that Dependency-free, so it runs in the existing install-free workflow, and it is registered in |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Closing in favour of #222, which targets This one was opened against Same three commits, rebased onto |
|
Reopened on request, targeting The branch carries all three commits — the role-gated grant, the pinned grant value, and the regression test — and sits exactly three commits ahead of
#222 carries the same three commits into Either way this needs drumee/schemas#182 deployed first: the evaluator has to know how to read a node-scoped grant before the values this code writes mean anything. |
…ed package The note claimed a bare `npm install` would pick up server-essentials 1.3.6. It would not: package-lock.json pins 1.3.1 and is consistent with the range in package.json, so npm installs the locked version. 1.3.6 has been on the registry since 2026-06-15 and has never been installed here. The hazard is narrower and worth stating as it is — anything that re-resolves the range rather than reading the lock. Pinning the value is still right; the reason given for it was overstated.
|
Correction to something I wrote on this PR and in the code comment. I said a bare No change is needed to the deploy pipeline. The stage path ( The hazard is narrower than I described: anything that re-resolves the range instead of reading the lock — a regenerated or deleted lockfile, or a bump that puts The code comment is corrected in the commit above. Note that the copy now on |
|
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
1 participant
Server half of the chat-attachment fix. Pairs with drumee/schemas#182 and needs that one deployed first.
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 grantPrivilege.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.Verification
user_permissionwas probed in isolation on stage: a node grant of 15 against an account-wide 7 yields 15 on the staging folder, while the root folder and plain files stay at 7, and a node grant lower than the account-wide value does not lower it.Still to run against this change, once schemas#182 is deployed: invite a chat member and a view-only member and check both, and demote a chat member and confirm the upload stops.
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.