Bind a role to an invite code, and make the binding survive the world changing (GRYT-893) - #129
Merged
Merged
Conversation
… changing (GRYT-893) A secret link handed to trusted people should grant the right role on arrival rather than somebody awarding it by hand afterwards. The storage is a column. The work is that an invite is a stored capability, and every other role grant on this server is checked with the actor in the room. `resolveRoleChange` asks for manage_roles, that the actor outranks the target, and that the role is strictly below the actor's own rank. An invite defers the grant to a moment nobody is present for, which breaks two of those three: - The creator can lose standing. An admin binds moderator to a link and is demoted a week later. Nothing revisits the invite, so a former admin holds a link that still mints moderators. - The role can move under the invite. Somebody with manage_roles binds a rank-10 role, then edits that role to rank 90 with everything ticked. The link now grants near-owner and no check was ever failed. The second needs no demotion and no second account, which is why a creation-time check on its own is decoration. So the rules are applied twice — once when the invite is made, once when it is redeemed, against the world as it is then. Four gates, in services/inviteRoles.ts so they can be read in one place: - A `grantable_by_invite` flag, off for every role until somebody ticks it. Rank alone would make every role below the creator silently grantable the moment it existed. - Never the owner role, and never `admin` — by id, because admin holds none of the escalation permissions (those are owner-only) and would pass a permission-only test. Admin is the role people mean when they say it has to be given by hand. - Never a role carrying manage_roles, manage_server, replace_identity or manage_bots. Checked again at redemption, so widening a bound role's permissions later cannot smuggle it through. - A rank snapshot taken when the role was bound. A role that has climbed since is not the role that was agreed to. Falling is fine — nobody needs protecting from getting less than was agreed. Redemption deliberately does not consult the creator's standing today. They may have left, and a link that dies when somebody leaves is a different bug; the snapshot and the rules already bound what the link can do. Two things fall out of the existing code rather than being added. The invite block in join.ts sits behind `!isActiveMember`, so a bound role can only ever land on a genuine first join and never on a reconnect carrying the same stored code. And the grant is an add, not an assign, so an invite can raise somebody above the tier default and never below it — the line autoRoles already takes. A refusal at redemption writes an audit row. Somebody arriving and not getting the role they were promised is indistinguishable from the feature being broken unless it is written down where an operator can find it. 14 tests on the rules, including both exploits above. 679 pass, build clean. Server half only. Nothing sets the flag or binds a role from the UI yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The first commit added the flag and the rules but no way to see or set it, so the feature was unreachable even once a client existed. `roleEditorState` now carries `grantableByInvite`, and the save handler accepts it. Setting it runs the same `mayBindRoleToInvite` check the invite creation path runs, against the rank and permissions being saved rather than the ones on disk — so ticking the box on a role that is about to become an admin role is refused at the same moment, rather than saving a tick that would then fail at binding time. Clearing it is always allowed. Taking a permission away needs no permission. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The gap named in the first PR body. The rules were tested and pure; everything between them and SQLite was typechecked only — three columns added by migration, a rank snapshot written at creation and read back at redemption, and an UPDATE whose argument list has to line up with its placeholders. That last one is not hypothetical. It was wrong once during this change and would have written a role id into the flag column, which typechecks perfectly. There is now a test that renames a role and asserts the flag and the rank survived, which is what a misaligned argument list destroys. Eight cases, including the exploit run for real rather than in the abstract: bind a role at rank 20, edit it to rank 90, redeem, and assert both that the role was not granted and that an `invite_role_refused` row explains why. Also checked that the tests would catch a regression rather than merely passing. Replacing the rank comparison with `if (false)` fails two tests, one from each file; doing the same to the admin-by-id rule fails two more. A test that cannot fail is a comment. 687 pass, build clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Member
Author
Migration verified against a populated databaseThe thing a real server does on upgrade, rather than only the fresh-database case the tests cover. Built a pre-change database by hand —
Also booted three times against the same database with no duplicate-column error, so the Scratch files used for this were deleted; the worktree is clean. |
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
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.
A secret link handed to trusted people should grant the right role on arrival, rather than somebody awarding it by hand afterwards.
Review-required path (
src/db/**), so this is yours to merge.Why this isn't just a column
Every other role grant here is checked with the actor in the room.
resolveRoleChangeasks formanage_roles, that the actor outranks the target, and that the role is strictly below the actor's own rank. An invite defers the grant to a moment nobody is present for, which breaks two of those three:manage_rolesbinds a role near the bottom of the list, then drags that role to the top and ticks every permission. The link now grants near-owner, and no check ever failed.The second needs no demotion and no second account. That is why a creation-time check alone is decoration, and why the rules run twice — at creation, and again at redemption against the world as it is then.
The four gates
All in
src/services/inviteRoles.ts, pure and in one place:grantable_by_invite, off by default. Rank alone would make every role below the creator silently grantable the moment it existed.admin— by id.adminholds none of the escalation permissions (those are owner-only), so a permission-only test would let an invite hand it out. Admin is the role people mean when they say it has to be given by hand.manage_roles,manage_server,replace_identityormanage_bots. Re-checked at redemption, so widening a bound role later cannot smuggle it through.What to look at
Redemption deliberately ignores the creator's current standing. They may have left, and a link that dies when somebody leaves is a different bug. The snapshot plus the rules already bound what the link can do. It is the one judgement call here and it is the one I would most like a second opinion on.
The UPDATE in
roleDefinitions.tsgained a placeholder; check the argument list still lines up. I got that wrong first time and it would have written the role id into the flag column.Two properties fall out of existing code rather than being added, so they are worth confirming rather than trusting my reading:
join.tsis behind!isActiveMember, so a bound role can only land on a genuine first join, never on a reconnect carrying the same stored code. (That also answers the question of what happens when an existing member uses the link: they never reach the invite path at all, and never spend a use.)addMemberRole, notsetServerRole, so an invite can raise somebody above the tier default and never below it.Tests
14 on the rules, including both exploits above, named so a future reader knows what they are protecting. 679 pass,
yarn buildclean.There is no integration test on the database path in
applyInviteRole— the rules are covered, the wiring is only typechecked.Not in this PR
The client half. Nothing sets the flag or binds a role from the UI yet, so this ships inert until that lands.
🤖 Generated with Claude Code