diff --git a/Server/CLAUDE.md b/Server/CLAUDE.md index 8659323e..ded59782 100644 --- a/Server/CLAUDE.md +++ b/Server/CLAUDE.md @@ -40,3 +40,14 @@ prometheus. imports it needs a row in `invariants/db_import_boundary.go` (`DBImportAllow`) with a disposition and reason — the B3 inventory, which only shrinks. New persistence goes behind a service, not into a handler. +- Only `permissions/` calls the raw permission bit helpers (`HasPerm`, + `HasAnyPerm`, `HasServerPerm`, `HasAdmin`, `EffectivePerms`, + `EffectiveChannelPerms`). Everywhere else resolves a `permissions.Subject` + and asks the predicate that owns the property (`CanViewChannel`, + `CanAdmitSession`, `CanSendMessage`, `CanType`, `CanJoinVoice`, + `CanModerateVoice`) — one predicate per security property, so a call site + cannot re-derive half a rule. The residue that predates B2-5 is listed by + symbol in `invariants/authz_chokepoint.go` (`AuthzResidueAllow`) with a + class, a reason, and the exact helper calls it is frozen at — a row is an + inventory, not a licence for the function, so a second raw call inside a + listed one still fails. That list only shrinks too. diff --git a/Server/invariants/authz_chokepoint.go b/Server/invariants/authz_chokepoint.go new file mode 100644 index 00000000..c4153b6e --- /dev/null +++ b/Server/invariants/authz_chokepoint.go @@ -0,0 +1,283 @@ +package invariants + +import ( + "fmt" + "go/ast" + "go/token" + "path" +) + +// authzChokepointID is the rule's stable id (a const for the same +// initialization-cycle reason as syncutilLocksID). +const authzChokepointID = "authz-chokepoint" + +// permissionsImportPath is the package that owns every authorization decision. +const permissionsImportPath = "github.com/J3vb/OwnCord/Server/permissions" + +// rawPermChecks are the raw bit helpers exported by permissions.go: the six +// call targets HP-2 question 5's residue grep treats as a raw check +// (HasPerm, HasAnyPerm, HasServerPerm, HasAdmin, EffectivePerms, +// EffectiveChannelPerms). They are the whole of that file's exported surface +// apart from Name, which formats a bit rather than deciding on one. +// +// Everything else in the package is a chokepoint rather than a raw check and +// is deliberately absent: the B2-5 predicates (CanViewChannel, +// CanAdmitSession, CanSendMessage, CanType, CanJoinVoice, CanModerateVoice), +// Subject.Has, and Checker — those are what a call site is supposed to use. +var rawPermChecks = map[string]bool{ + "HasPerm": true, + "HasAnyPerm": true, + "HasServerPerm": true, + "HasAdmin": true, + "EffectivePerms": true, + "EffectiveChannelPerms": true, +} + +// Residue classes, from HP-2 question 5's table. Each says why the sites in it +// are not a channel predicate, so B3-8 can retire a whole class at once. +const ( + // classServerScoped: a server-wide permission with no channel to resolve + // a permissions.Subject for. These are the canonical server-wide check. + classServerScoped = "server-scoped" + // classAdminShortCircuit: HasAdmin used to skip the override query before + // the predicate runs. An optimisation, not a decision. + classAdminShortCircuit = "admin-short-circuit" + // classAdminPerimeter: HasAdmin as an authorization input for role + // hierarchy and the admin perimeter — the 2026-08-18 measurement's + // "no Outranks" class. + classAdminPerimeter = "admin-perimeter" + // classBulkReaderWalk: the bulk @everyone reader walk, a per-role layer + // walk whose mechanical conversion the owner declined on 2026-08-18. + classBulkReaderWalk = "bulk-reader-walk" + // classBaseBitRejection: a base-bit early rejection ahead of + // CanModerateVoice. It never admits; it keeps FORBIDDEN ahead of the + // voice-state lookup (a B2-5 decision). + classBaseBitRejection = "base-bit-rejection" +) + +// authzResidueClasses is the closed set of classes a row may carry; +// TestAuthzResidueAllowIsLive rejects any other value. A row cannot invent a +// class, and that includes the "unclassified" escape valve the B3-6 brief +// sketched: a genuinely new kind of residue means adding a constant above as a +// deliberate, reviewable edit, so nothing reaches the allowlist without a +// classification someone chose. authzClassList must name it too. +var authzResidueClasses = map[string]bool{ + classServerScoped: true, + classAdminShortCircuit: true, + classAdminPerimeter: true, + classBulkReaderWalk: true, + classBaseBitRejection: true, +} + +// authzClassList names the legal classes in the violation message. Built from +// the constants, so it cannot drift from them; TestAuthzResidueAllowIsLive +// checks it covers authzResidueClasses. +const authzClassList = classServerScoped + ", " + classAdminShortCircuit + ", " + + classAdminPerimeter + ", " + classBulkReaderWalk + ", " + classBaseBitRejection + +// calls is a helper-name → call-count multiset, aliased purely to keep the +// residue table's literals short. An alias rather than a defined type, so the +// exported field below stays a plain map[string]int. +type calls = map[string]int + +// AuthzResidueEntry is one row of HP-2 question 5's residue table: why a +// production symbol outside Server/permissions still calls a raw bit helper +// instead of a predicate, and exactly which raw calls it is frozen at. +type AuthzResidueEntry struct { + Class string // one of the classes above; the set is closed + Note string // what this particular site does + // Calls is the exact multiset of raw helper calls the symbol may contain: + // helper name → count. It is what stops a row from being a licence for the + // whole function. A symbol with an extra call, a call of a different + // helper, or one more of the same helper fails authz-chokepoint at the + // offending line; one with fewer fails TestAuthzResidueAllowIsLive, which + // compares the multiset exactly rather than asking for at least one hit. + Calls calls +} + +// AuthzResidueAllow is the residue. Rows are keyed by symbol — the file's +// directory relative to the Server tree, then the enclosing function or +// method, e.g. "ws.(*Hub).readyVisibleChannels" — never by file:line, because +// lines move on every edit and a stale line number would silently stop +// matching. The directory rather than the package clause keeps the four +// `package main` files at different paths from colliding. +// +// A row binds three things, not one: the symbol, which raw helpers it calls, +// and how many times each. Allowlisting a symbol alone would exempt the whole +// function — a second raw call added inside it, or a switch to a different +// helper, would then pass silently and the residue could grow without review. +// +// The list only shrinks and never widens: a symbol that stops calling a raw +// helper, or whose counts no longer match, fails TestAuthzResidueAllowIsLive; +// an extra or different call fails authz-chokepoint at the offending line; a +// new raw call anywhere else fails it too. B3-8 deletes rows as it moves each +// family behind a service. Raising a count is a reviewable edit here, never a +// side effect of editing the function. +// +// 19 symbols, 21 bound calls, matching HP-2 question 5 at dev 75d64dd4. +var AuthzResidueAllow = map[string]AuthzResidueEntry{ + // ── server-scoped: no channel exists to resolve a Subject for ────────── + "admin.adminAuthMiddleware": {classServerScoped, "HasAnyPerm over AdminPerimeter gates the admin panel as a whole", calls{"HasAnyPerm": 1}}, + "admin.requirePerm": {classServerScoped, "per-route server permission for the admin mux", calls{"HasServerPerm": 1}}, + "api.RequirePermission": {classServerScoped, "per-route server permission for the REST mux", calls{"HasServerPerm": 1}}, + "service.(*EmojiService).RequireManage": {classServerScoped, "MANAGE_SERVER is server-wide; emoji have no channel", calls{"HasServerPerm": 1}}, + "service.(*ModerationService).requirePerm": {classServerScoped, "ban/kick/timeout are server-wide", calls{"HasServerPerm": 1}}, + "service.(*RoleService).actorRole": {classServerScoped, "MANAGE_ROLES is server-wide", calls{"HasServerPerm": 1}}, + + // ── HasAdmin as a fetch short-circuit, ahead of the predicate ────────── + "service.(*ChannelService).ListVisibleChannels": {classAdminShortCircuit, "an administrator sees every channel; skips the override query", calls{"HasAdmin": 1}}, + "service.(*MessageService).GetAccessibleChannelIDs": {classAdminShortCircuit, "an administrator searches every channel; skips the override query", calls{"HasAdmin": 1}}, + "service.(*PermissionService).getOrPopulate": {classAdminShortCircuit, "cache fill skips the override query for an administrator", calls{"HasAdmin": 1}}, + "ws.(*Hub).computeAllowedChannels": {classAdminShortCircuit, "broadcast audience skips the override query for an administrator", calls{"HasAdmin": 1}}, + "ws.(*Hub).readyVisibleChannels": {classAdminShortCircuit, "ready snapshot skips the override query for an administrator", calls{"HasAdmin": 1}}, + "ws.(*Hub).voiceJoinPublishPerms": {classAdminShortCircuit, "publish/video/screenshare bits skip the override query for an administrator", calls{"HasAdmin": 1}}, + + // ── HasAdmin as an authorization input: role hierarchy and perimeter ─── + "admin.requireGrantableOverride": {classAdminPerimeter, "refuses an override that grants past the actor's own role", calls{"HasAdmin": 1}}, + "admin.requireManageableUser": {classAdminPerimeter, "role-hierarchy check on the target user", calls{"HasAdmin": 1}}, + "admin.logStreamAuthorize": {classAdminPerimeter, "the log stream is administrator-only, re-checked per tick", calls{"HasAdmin": 1}}, + "api.serveFileAuthorize": {classAdminPerimeter, "administrator bypass for attachment access", calls{"HasAdmin": 1}}, + "service.requireGrantable": {classAdminPerimeter, "refuses a role edit that grants past the actor's own bits", calls{"HasAdmin": 1}}, + + // ── bulk @everyone reader walk ───────────────────────────────────────── + // The one multi-call row: one EffectivePerms for the layer's mask, then + // HasAdmin twice — once to keep an administrator role in, once in the + // read test. + "service.(*MessageService).mentionReaders": {classBulkReaderWalk, "per-role layer walk over every role that can read the channel", calls{"EffectivePerms": 1, "HasAdmin": 2}}, + + // ── base-bit early rejection ahead of CanModerateVoice ───────────────── + "ws.voiceModTarget": {classBaseBitRejection, "rejects on MUTE_MEMBERS before the voice-state lookup; never admits", calls{"HasServerPerm": 1}}, +} + +// authzChokepoint fails on any production symbol outside Server/permissions +// that calls a raw permission bit helper without a residue row. B2-5 gave +// every channel-scoped security property exactly one predicate; this rule +// keeps the next call site from re-deriving one of them by hand, which is how +// the thirteen hand-rolled decision sites B2-5 collapsed came to exist. +// +// Test files are out of scope — Run never parses them — so a parity table may +// call the helpers freely. +var authzChokepoint = Rule{ + ID: authzChokepointID, + Scope: nil, // every directory; permissions/ itself is excluded in Check + Check: checkAuthzChokepoint, +} + +func checkAuthzChokepoint(f *ast.File, fset *token.FileSet, rel string) []Violation { + var out []Violation + flag := func(h authzHit, msg string) { + out = append(out, Violation{Rule: authzChokepointID, File: rel, Line: h.Line, Msg: msg}) + } + + // Per symbol, how many calls of each helper have been seen so far in this + // file. A symbol's hits are always in one file — Go forbids two functions + // of the same name in a package — so one pass sees the whole multiset, and + // counting as we go means the violation lands on the extra call itself. + seen := make(map[string]calls) + + for _, h := range authzHits(f, fset, rel) { + if h.Helper == "" { + // A dot-import binds no call to count, and an allowlisted symbol + // is no excuse for one. + flag(h, h.message()) + continue + } + row, listed := AuthzResidueAllow[h.Symbol] + if !listed { + flag(h, h.message()) + continue + } + if seen[h.Symbol] == nil { + seen[h.Symbol] = make(calls) + } + seen[h.Symbol][h.Helper]++ + if got, want := seen[h.Symbol][h.Helper], row.Calls[h.Helper]; got > want { + flag(h, h.excessMessage(want, got)) + } + } + return out +} + +// authzHit is one raw permission check at one source location, tagged with the +// symbol an AuthzResidueAllow row would name. +type authzHit struct { + Symbol string // e.g. "ws.(*Hub).readyVisibleChannels" + Helper string // e.g. "HasAdmin", or "" for a dot-import + Line int +} + +func (h authzHit) message() string { + if h.Helper == "" { + return `dot-import of the permissions package defeats authz-chokepoint (a bare HasAdmin/HasPerm call can no longer be matched); import permissions normally` + } + return "raw permissions." + h.Helper + " resolves permission bits outside Server/permissions " + + "(the Has* helpers decide, the Effective* ones compute the mask a decision then reads); " + + "resolve a permissions.Subject and ask the predicate that owns the property " + + "(" + authzPredicateList + "), " + + "or add an AuthzResidueAllow entry for " + h.Symbol + " with a reason, the calls it binds, " + + "and one of the classes " + authzClassList +} + +// excessMessage is the message for a call inside an allowlisted symbol that the +// symbol's row does not account for: a helper the row never listed (want 0), or +// one more of a helper than it binds. +func (h authzHit) excessMessage(want, got int) string { + return fmt.Sprintf("raw permissions.%s at %s: the residue row binds %d call(s) of %s here, found %d. "+ + "A row freezes an inventory, it is not a licence for the function — resolve a permissions.Subject "+ + "and ask the predicate that owns the property (%s), or have the row's count raised under review. "+ + "B3-8 removes rows, it never widens them.", + h.Helper, h.Symbol, want, h.Helper, got, authzPredicateList) +} + +// authzPredicateList names the B2-5 predicates a call site should be using. +const authzPredicateList = "CanViewChannel, CanAdmitSession, CanSendMessage, CanType, CanJoinVoice, CanModerateVoice" + +// authzHits reports every raw permission check in one file, whether or not it +// is allowlisted, so the rule and TestAuthzResidueAllowIsLive read the same +// scan from opposite directions. +// +// It matches the selector, not the call: `f := permissions.HasAdmin` followed +// by `f(bits)` is the same decision made at the same place, and a rule that +// only looked at CallExpr would miss it. +func authzHits(f *ast.File, fset *token.FileSet, rel string) []authzHit { + dir := path.Dir(rel) + if dir == "." { + dir = "" + } + // permissions/ needs no exemption and deliberately does not get one: a file + // in that package cannot import itself, so it binds no "permissions" + // identifier and matches nothing here. A directory-keyed exemption would be + // worse than redundant — it would silently exempt a future + // permissions/, which is a different package that does import + // permissions and must be checked like any other. + names, dotImports := importNames(f, permissionsImportPath) + + var out []authzHit + for _, imp := range dotImports { + out = append(out, authzHit{ + Symbol: dir + "." + enclosingSymbol(f, imp.Pos()), + Line: fset.Position(imp.Pos()).Line, + }) + } + if len(names) == 0 { + return out + } + + ast.Inspect(f, func(n ast.Node) bool { + sel, ok := n.(*ast.SelectorExpr) + if !ok { + return true + } + pkg, ok := sel.X.(*ast.Ident) + if !ok || !names[pkg.Name] || !rawPermChecks[sel.Sel.Name] { + return true + } + out = append(out, authzHit{ + Symbol: dir + "." + enclosingSymbol(f, sel.Pos()), + Helper: sel.Sel.Name, + Line: fset.Position(sel.Pos()).Line, + }) + return true + }) + return out +} diff --git a/Server/invariants/authz_chokepoint_test.go b/Server/invariants/authz_chokepoint_test.go new file mode 100644 index 00000000..b54e8d56 --- /dev/null +++ b/Server/invariants/authz_chokepoint_test.go @@ -0,0 +1,427 @@ +package invariants + +import ( + "go/ast" + "go/token" + "maps" + "strings" + "testing" +) + +func TestAuthzChokepoint(t *testing.T) { + const importPerms = `import "github.com/J3vb/OwnCord/Server/permissions"` + tests := []struct { + name string + path string + src string + want int + }{ + { + name: "unlisted api function calling HasPerm is flagged", + path: "api/brand_new_handler.go", + src: "package api\n" + importPerms + ` +func brandNew(bits int64) bool { return permissions.HasPerm(bits, 1) } +`, + want: 1, + }, + { + name: "every raw bit helper is flagged", + path: "api/brand_new_handler.go", + src: "package api\n" + importPerms + ` +func brandNew(bits, a, d int64) bool { + _ = permissions.HasAnyPerm(bits, 1) + _ = permissions.HasServerPerm(bits, 1) + _ = permissions.HasAdmin(bits) + _ = permissions.EffectivePerms(bits, a, d) + _ = permissions.EffectiveChannelPerms(bits, permissions.ChannelOverride{}) + return permissions.HasPerm(bits, 1) +} +`, + want: 6, + }, + { + name: "a predicate call is the point of the rule and is clean", + path: "api/brand_new_handler.go", + src: "package api\n" + importPerms + ` +func brandNew(s permissions.Subject) error { return permissions.CanSendMessage(s) } +`, + want: 0, + }, + { + name: "aliased import is still flagged", + path: "ws/brand_new.go", + src: `package ws +import perms "github.com/J3vb/OwnCord/Server/permissions" +func brandNew(bits int64) bool { return perms.HasAdmin(bits) } +`, + want: 1, + }, + { + name: "dot-import is flagged as its own violation", + path: "ws/brand_new.go", + src: `package ws +import . "github.com/J3vb/OwnCord/Server/permissions" +func brandNew(bits int64) bool { return HasAdmin(bits) } +`, + want: 1, + }, + { + name: "taking the helper as a value, not calling it, is still flagged", + path: "ws/brand_new.go", + src: "package ws\n" + importPerms + ` +func brandNew(bits int64) bool { + f := permissions.HasAdmin + return f(bits) +} +`, + want: 1, + }, + { + name: "listed method symbol is allowed", + path: "ws/serve_ready.go", + src: `package ws +` + importPerms + ` +type Hub struct{} +func (h *Hub) readyVisibleChannels(bits int64) bool { return permissions.HasAdmin(bits) } +`, + want: 0, + }, + { + name: "listed plain function symbol is allowed at the calls its row binds", + path: "api/upload_handler.go", + src: "package api\n" + importPerms + ` +func serveFileAuthorize(bits int64) bool { return permissions.HasAdmin(bits) } +`, + want: 0, + }, + { + // The Codex P2: a row must not exempt the whole function. + name: "a second call of the bound helper inside a listed symbol is flagged", + path: "api/upload_handler.go", + src: "package api\n" + importPerms + ` +func serveFileAuthorize(bits, other int64) bool { + return permissions.HasAdmin(bits) || permissions.HasAdmin(other) +} +`, + want: 1, + }, + { + name: "a different helper inside a listed symbol is flagged", + path: "api/upload_handler.go", + src: "package api\n" + importPerms + ` +func serveFileAuthorize(bits int64) bool { return permissions.HasPerm(bits, 1) } +`, + want: 1, + }, + { + // Fewer calls than the row binds is the liveness test's direction, + // not the rule's: the rule never flags a shrinking residue. + name: "fewer calls than the row binds is not the rule's business", + path: "service/mentions.go", + src: "package service\n" + importPerms + ` +type MessageService struct{} +func (s *MessageService) mentionReaders(bits int64) bool { return permissions.HasAdmin(bits) } +`, + want: 0, + }, + { + name: "the multi-call row is satisfied only by its exact multiset", + path: "service/mentions.go", + src: "package service\n" + importPerms + ` +type MessageService struct{} +func (s *MessageService) mentionReaders(bits, a, d int64) bool { + _ = permissions.EffectivePerms(bits, a, d) + return permissions.HasAdmin(bits) || permissions.HasAdmin(a) +} +`, + want: 0, + }, + { + name: "a dot-import inside a listed symbol is still flagged", + path: "api/upload_handler.go", + src: `package api +import . "github.com/J3vb/OwnCord/Server/permissions" +func serveFileAuthorize(bits int64) bool { return HasAdmin(bits) } +`, + want: 1, + }, + { + name: "a row is keyed by symbol, not by file: the same symbol in another file of the package is allowed", + path: "api/moved_somewhere_else.go", + src: "package api\n" + importPerms + ` +func serveFileAuthorize(bits int64) bool { return permissions.HasAdmin(bits) } +`, + want: 0, + }, + { + name: "a row does not cover a different symbol in the same file", + path: "api/upload_handler.go", + src: "package api\n" + importPerms + ` +func someOtherHelper(bits int64) bool { return permissions.HasAdmin(bits) } +`, + want: 1, + }, + { + name: "a row does not cover the same function name on a different receiver", + path: "ws/serve_ready.go", + src: `package ws +` + importPerms + ` +type Client struct{} +func (c *Client) readyVisibleChannels(bits int64) bool { return permissions.HasAdmin(bits) } +`, + want: 1, + }, + { + name: "a row does not carry across packages", + path: "plugin/brand_new.go", + src: "package plugin\n" + importPerms + ` +func serveFileAuthorize(bits int64) bool { return permissions.HasAdmin(bits) } +`, + want: 1, + }, + { + // Not an exemption: a file in package permissions cannot import + // itself, so it binds no "permissions" identifier and the bare + // call matches nothing. + name: "a file inside permissions binds no name and is not flagged", + path: "permissions/checker.go", + src: `package permissions +func f(bits int64) bool { return HasAdmin(bits) } +`, + want: 0, + }, + { + // A directory-keyed exemption for permissions/ would silently let + // this through, which is why the rule has none. Re-adding one must + // fail here. + name: "a permissions subpackage does import permissions and is checked like any other", + path: "permissions/policy/x.go", + src: "package policy\n" + importPerms + ` +func decide(bits int64) bool { return permissions.HasAdmin(bits) } +`, + want: 1, + }, + { + name: "a call at package scope has no enclosing symbol and is flagged", + path: "api/brand_new_handler.go", + src: "package api\n" + importPerms + ` +var adminOnly = permissions.HasAdmin(0) +`, + want: 1, + }, + { + name: "a same-named helper on another package is not a permission check", + path: "api/brand_new_handler.go", + src: `package api +import "github.com/J3vb/OwnCord/Server/plugin" +func brandNew(bits int64) bool { return plugin.HasAdmin(bits) } +`, + want: 0, + }, + { + name: "permissions.Name is not a check", + path: "api/brand_new_handler.go", + src: "package api\n" + importPerms + ` +func brandNew(bit int64) string { return permissions.Name(bit) } +`, + want: 0, + }, + { + name: "allow comment with a reason suppresses", + path: "api/brand_new_handler.go", + src: "package api\n" + importPerms + ` +func brandNew(bits int64) bool { + return permissions.HasAdmin(bits) //invariant:allow authz-chokepoint — synthetic fixture +} +`, + want: 0, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := checkSourceWith([]Rule{authzChokepoint}, token.NewFileSet(), tt.path, []byte(tt.src)) + if len(got) != tt.want { + t.Fatalf("want %d violation(s), got %d: %v", tt.want, len(got), got) + } + for _, v := range got { + if v.Rule != authzChokepointID { + t.Errorf("rule id = %q, want %q", v.Rule, authzChokepointID) + } + } + }) + } +} + +func TestAuthzChokepointMessageNamesTheFix(t *testing.T) { + src := `package api +import "github.com/J3vb/OwnCord/Server/permissions" + +type h struct{} + +func (x *h) decide(bits int64) bool { return permissions.HasPerm(bits, 1) } +` + got := checkSourceWith([]Rule{authzChokepoint}, token.NewFileSet(), "api/x.go", []byte(src)) + if len(got) != 1 { + t.Fatalf("got %d violation(s), want 1: %v", len(got), got) + } + v := got[0] + if v.Line != 6 { + t.Errorf("Line = %d, want 6", v.Line) + } + if !strings.Contains(v.Msg, "permissions.HasPerm") { + t.Errorf("message must name the helper that was called, got %q", v.Msg) + } + if !strings.Contains(v.Msg, "CanSendMessage") { + t.Errorf("message must name the predicates to use instead, got %q", v.Msg) + } + // The symbol is the allowlist key, so the message doubles as the row to + // paste when a site is genuinely residue. + if !strings.Contains(v.Msg, "api.(*h).decide") { + t.Errorf("message must name the symbol an AuthzResidueAllow row would use, got %q", v.Msg) + } + // A row is only writable if the caller is told which classes are legal. + for _, class := range []string{ + classServerScoped, classAdminShortCircuit, classAdminPerimeter, + classBulkReaderWalk, classBaseBitRejection, + } { + if !strings.Contains(v.Msg, class) { + t.Errorf("message must name the legal class %q, got %q", class, v.Msg) + } + } +} + +// TestAuthzChokepointMessageFitsAllSixTargets guards the wording: two of the +// six helpers compute a mask rather than deciding, so the message cannot claim +// the call "decides authorization". +func TestAuthzChokepointMessageFitsAllSixTargets(t *testing.T) { + src := `package api +import "github.com/J3vb/OwnCord/Server/permissions" + +func compute(bits, a, d int64) int64 { return permissions.EffectivePerms(bits, a, d) } +` + got := checkSourceWith([]Rule{authzChokepoint}, token.NewFileSet(), "api/x.go", []byte(src)) + if len(got) != 1 { + t.Fatalf("got %d violation(s), want 1: %v", len(got), got) + } + if strings.Contains(got[0].Msg, "decides authorization") { + t.Errorf("EffectivePerms computes a mask, it does not decide: %q", got[0].Msg) + } + if !strings.Contains(got[0].Msg, "resolves permission bits") { + t.Errorf("message must describe all six targets accurately, got %q", got[0].Msg) + } +} + +// TestAuthzChokepointExcessMessageNamesHelperAndCount pins what a maintainer +// needs to act on an over-count: which helper, how many the row binds, and how +// many are actually there. +func TestAuthzChokepointExcessMessageNamesHelperAndCount(t *testing.T) { + src := `package api +import "github.com/J3vb/OwnCord/Server/permissions" + +func serveFileAuthorize(bits, other int64) bool { + return permissions.HasAdmin(bits) || permissions.HasAdmin(other) +} +` + got := checkSourceWith([]Rule{authzChokepoint}, token.NewFileSet(), "api/upload_handler.go", []byte(src)) + if len(got) != 1 { + t.Fatalf("got %d violation(s), want 1: %v", len(got), got) + } + v := got[0] + // The second call, not the first: the row's one bound call is spent. + if v.Line != 5 { + t.Errorf("Line = %d, want 5 (the extra call, not the bound one)", v.Line) + } + for _, want := range []string{ + "HasAdmin", // which helper + "binds 1 call(s) of HasAdmin", // how many the row allows + "found 2", // how many are there + "api.serveFileAuthorize", // where + "it never widens them", // and that raising it is a review, not an edit + } { + if !strings.Contains(v.Msg, want) { + t.Errorf("message must contain %q, got %q", want, v.Msg) + } + } +} + +// TestAuthzResidueAllowIsLive keeps the residue honest in the other direction: +// every allowlisted symbol must still exist and still call a raw bit helper. +// A row for a site that moved behind a predicate is stale and must be deleted +// — the list only shrinks. TestServerInvariants covers the opposite direction +// (no unlisted raw check anywhere in the tree), so together they pin the +// residue to exactly this set of symbols. +func TestAuthzResidueAllowIsLive(t *testing.T) { + live := make(map[string]calls) + collect := Rule{ + ID: authzChokepointID, + Check: func(f *ast.File, fset *token.FileSet, rel string) []Violation { + for _, h := range authzHits(f, fset, rel) { + if live[h.Symbol] == nil { + live[h.Symbol] = make(calls) + } + live[h.Symbol][h.Helper]++ + } + return nil + }, + } + if _, err := runWith([]Rule{collect}, ".."); err != nil { + t.Fatalf("walking the server tree: %v", err) + } + if len(live) == 0 { + t.Fatal("no raw permission check found anywhere in the tree; the scan is vacuous") + } + + for sym, entry := range AuthzResidueAllow { + switch { + case live[sym] == nil: + t.Errorf("AuthzResidueAllow[%q] no longer performs a raw permission check — delete the row", sym) + case !maps.Equal(entry.Calls, live[sym]): + // Exact, not "at least one": a row that over-counts would leave + // headroom for a raw call nobody reviewed, and one that under-counts + // is caught by the rule instead. + t.Errorf("AuthzResidueAllow[%q] binds %v but the tree has %v — "+ + "correct the row under review, or delete it if the calls have moved behind a predicate", + sym, entry.Calls, live[sym]) + } + if len(entry.Calls) == 0 { + t.Errorf("AuthzResidueAllow[%q]: a row must bind the calls it allows, otherwise it exempts the whole symbol", sym) + } + if fileScopeRow(sym) { + t.Errorf("AuthzResidueAllow[%q] would exempt every package-scope raw call and dot-import "+ + "in that directory at once; move the call into a named function and key the row on it", sym) + } + if !authzResidueClasses[entry.Class] { + t.Errorf("AuthzResidueAllow[%q]: unknown class %q", sym, entry.Class) + } + if entry.Note == "" { + t.Errorf("AuthzResidueAllow[%q]: the reason is mandatory", sym) + } + } + + // The message can only tell a caller which classes are legal if it names + // every one of them. + for class := range authzResidueClasses { + if !strings.Contains(authzClassList, class) { + t.Errorf("class %q is missing from authzClassList, so the violation message never names it", class) + } + } +} + +// fileScopeRow reports whether an allowlist key names a whole directory's file +// scope rather than one function. Such a row is never residue: it would match +// every package-scope raw call and every dot-import in that directory at once. +func fileScopeRow(sym string) bool { return strings.HasSuffix(sym, "."+fileScopeSymbol) } + +func TestFileScopeRowsAreRejected(t *testing.T) { + for sym, want := range map[string]bool{ + "api.": true, + ".": true, // a root-level file + "api.serveFileAuthorize": false, + "ws.(*Hub).readyVisibleChannels": false, + "service.(*MessageService).mentionReaders": false, + } { + if got := fileScopeRow(sym); got != want { + t.Errorf("fileScopeRow(%q) = %v, want %v", sym, got, want) + } + } +} diff --git a/Server/invariants/invariants.go b/Server/invariants/invariants.go index 64b94614..dedf4912 100644 --- a/Server/invariants/invariants.go +++ b/Server/invariants/invariants.go @@ -19,6 +19,7 @@ import ( "os" "path" "sort" + "strconv" "strings" ) @@ -61,7 +62,81 @@ func (r Rule) inScope(dir string) bool { } // Rules is the registry every gate runs. -var Rules = []Rule{syncutilLocks, dbImportBoundary} +var Rules = []Rule{syncutilLocks, dbImportBoundary, authzChokepoint} + +// importNames returns the local identifiers this file binds to the import +// path, and separately every dot-import of it (import . "p"), which binds no +// identifier at all. A rule that matches pkg.Sym selectors needs both: the +// names to match against, and the dot-imports to report, since a dot-import +// lets the symbol be spelled bare and evade the selector match entirely. +// +// An unaliased import binds the package's own clause name, which is not in the +// file being parsed; the last path element stands in for it. That holds for +// every path these rules care about (all first-party, all named after their +// directory) and would need the package's own source for one where it does not +// (gopkg.in/yaml.v3 binds yaml, not yaml.v3). +func importNames(f *ast.File, importPath string) (names map[string]bool, dotImports []*ast.ImportSpec) { + names = make(map[string]bool) + for _, imp := range f.Imports { + p, err := strconv.Unquote(imp.Path.Value) + if err != nil || p != importPath { + continue + } + switch { + case imp.Name == nil: + names[path.Base(importPath)] = true + case imp.Name.Name == "_": + // Blank import: no identifier is bound, so pkg.Sym cannot be + // spelled at all. + case imp.Name.Name == ".": + dotImports = append(dotImports, imp) + default: + names[imp.Name.Name] = true + } + } + return names, dotImports +} + +// enclosingSymbol names the function or method containing pos, in the form +// rules use for a symbol-keyed allowlist: "requirePerm" for a function, +// "(*Hub).canSee" for a method. An expression outside every function body (a +// package-level var initializer) has no enclosing function and gets +// fileScopeSymbol, which no allowlist row is expected to name. +func enclosingSymbol(f *ast.File, pos token.Pos) string { + for _, d := range f.Decls { + fd, ok := d.(*ast.FuncDecl) + if !ok || pos < fd.Pos() || pos >= fd.End() { + continue + } + if fd.Recv == nil || len(fd.Recv.List) == 0 { + return fd.Name.Name + } + return "(" + receiverTypeName(fd.Recv.List[0].Type) + ")." + fd.Name.Name + } + return fileScopeSymbol +} + +// fileScopeSymbol stands in for "not inside any function". +const fileScopeSymbol = "" + +// receiverTypeName renders a method receiver's type syntactically: Hub, +// *Hub, or the generic forms *Hub[T] / *Hub[K, V] reduced to their base name, +// which is what identifies the method. Written by hand rather than with +// go/types.ExprString so the package keeps its go/ast-only promise. +func receiverTypeName(e ast.Expr) string { + switch t := e.(type) { + case *ast.StarExpr: + return "*" + receiverTypeName(t.X) + case *ast.Ident: + return t.Name + case *ast.IndexExpr: // generic receiver: Hub[T] + return receiverTypeName(t.X) + case *ast.IndexListExpr: // generic receiver: Hub[K, V] + return receiverTypeName(t.X) + default: + return "?" + } +} // allowPrefix introduces a line-scoped suppression: // @@ -172,6 +247,13 @@ var skipDirs = map[string]bool{ // separated and already relative to root, which is exactly the form // CheckSource and Violation.File want. func Run(root string) ([]Violation, error) { + return runWith(Rules, root) +} + +// runWith is Run against an explicit rule set rather than the global registry +// — the walker counterpart of checkSourceWith, so one rule's tests can sweep +// the real tree in isolation. +func runWith(rules []Rule, root string) ([]Violation, error) { r, err := os.OpenRoot(root) if err != nil { return nil, err @@ -206,7 +288,7 @@ func Run(root string) ([]Violation, error) { if err != nil { return err } - out = append(out, CheckSource(fset, p, src)...) + out = append(out, checkSourceWith(rules, fset, p, src)...) return nil }) if err != nil { diff --git a/Server/invariants/syncutil_locks.go b/Server/invariants/syncutil_locks.go index 1d6b40a5..2f25f5b9 100644 --- a/Server/invariants/syncutil_locks.go +++ b/Server/invariants/syncutil_locks.go @@ -3,7 +3,6 @@ package invariants import ( "go/ast" "go/token" - "strconv" ) // syncutilLocksID is the rule's stable id. It is a const, not a field read @@ -37,7 +36,7 @@ var syncutilLocks = Rule{ func checkSyncutilLocks(f *ast.File, fset *token.FileSet, rel string) []Violation { var out []Violation - names, dotImports := syncImportNames(f) + names, dotImports := importNames(f, "sync") for _, imp := range dotImports { out = append(out, Violation{ @@ -78,28 +77,3 @@ func checkSyncutilLocks(f *ast.File, fset *token.FileSet, rel string) []Violatio return out } - -// syncImportNames returns the local identifiers this file binds to the -// "sync" import path (its name, or an alias), and separately every dot-import -// of "sync" (import . "sync"), which binds no identifier at all. -func syncImportNames(f *ast.File) (names map[string]bool, dotImports []*ast.ImportSpec) { - names = make(map[string]bool) - for _, imp := range f.Imports { - p, err := strconv.Unquote(imp.Path.Value) - if err != nil || p != "sync" { - continue - } - switch { - case imp.Name == nil: - names["sync"] = true - case imp.Name.Name == "_": - // Blank import: no identifier is bound, so sync.Mutex cannot be - // spelled at all. - case imp.Name.Name == ".": - dotImports = append(dotImports, imp) - default: - names[imp.Name.Name] = true - } - } - return names, dotImports -} diff --git a/docs/plans/b3-server-architecture-guardrails-2026-08-29.md b/docs/plans/b3-server-architecture-guardrails-2026-08-29.md index 3b565028..d65e8d60 100644 --- a/docs/plans/b3-server-architecture-guardrails-2026-08-29.md +++ b/docs/plans/b3-server-architecture-guardrails-2026-08-29.md @@ -660,6 +660,47 @@ each item is its own PR so none blocks another. Nothing here edits Exit: each item green in CI on its own PR; the numbers (floor, seeds, bench baseline) in this section's evidence block. +#### Evidence — item 7 (`authz-chokepoint`) + +- Branch `feat/b3-6-authz-chokepoint`; commits: `2f4d18fa` `feat(b3-6): authz-chokepoint rule — raw permission checks route through the B2-5 predicates`, plus this evidence commit +- RED: a temporary `permissions.HasPerm(0, permissions.ReadMessages)` inside + `api/diagnostics_handler.go`'s `isPrivateIP`, then + `cd Server && go test -count=1 ./invariants/` → + `api/diagnostics_handler.go:89: [authz-chokepoint] raw permissions.HasPerm decides authorization at the call site; … or add an AuthzResidueAllow entry for api.isPrivateIP with a class and a reason`. + Probe reverted (`git checkout -- api/diagnostics_handler.go`, `git status` clean). +- RED (allowlist, both directions): a bogus row → + `TestAuthzResidueAllowIsLive … AuthzResidueAllow["ws.(*Hub).movedBehindAPredicate"] no longer performs a raw permission check — delete the row`; + deleting the real `ws.(*Hub).readyVisibleChannels` row → + `TestServerInvariants … ws/serve_ready.go:169: [authz-chokepoint] raw permissions.HasAdmin …`. + Every row is load-bearing, and `TestAuthzResidueAllowIsLive` proves all 19 at + once on every run. +- GREEN: `cd Server && go test -count=1 ./invariants/` → `ok github.com/J3vb/OwnCord/Server/invariants` +- RED (Codex P2 on #1451 — a row must not exempt the whole function): a second + `permissions.HasAdmin` inside `api/serveFileAuthorize` → + `api/upload_handler.go:405: … the residue row binds 1 call(s) of HasAdmin here, found 2`; + swapping that call to `permissions.HasPerm` → + `api/upload_handler.go:404: … binds 0 call(s) of HasPerm here, found 1`; + setting the row to `HasAdmin: 2` → + `TestAuthzResidueAllowIsLive … binds map[HasAdmin:2] but the tree has map[HasAdmin:1]`. + All three restored. +- Numbers: allowlist **19 rows / 21 bound calls**, which is HP-2 question 5's + 21 code hits exactly — re-measured at `dev` `75d64dd4`, zero hits outside its + five classes and zero unclassified. A row binds symbol **and** helper **and** + count, so the residue cannot grow inside an allowlisted function; 18 rows bind + one call, `service.(*MessageService).mentionReaders` binds three + (`EffectivePerms` 1, `HasAdmin` 2). By helper: `HasAdmin` 13, + `HasServerPerm` 6, `HasAnyPerm` 1, `EffectivePerms` 1. Classes: + `server-scoped` 6, `admin-short-circuit` 6, `admin-perimeter` 5, + `bulk-reader-walk` 3 (one symbol), `base-bit-rejection` 1. Six flagged call + targets (`HasPerm`, `HasAnyPerm`, `HasServerPerm`, `HasAdmin`, + `EffectivePerms`, `EffectiveChannelPerms`) — the whole exported surface of + `permissions.go` except `Name`. Registry is now three rules. +- Verified against HEAD: the plan's "`Rules = []Rule{syncutilLocks}` … one + rule" predates B3-0; `authz-chokepoint` is the third, not the second. The + residue table's line numbers had moved (B3-2 shifted `api/middleware.go` + 200→213), which is why rows are keyed by `.` and never + by `file:line`; the hit count is unchanged at 21. No production code changed. + ## B3-7 — Alpha-shaped test dataset Roadmap workstream 12. Beside the slice.