amadan
console: normalize the address in `member`, and name a stranded account — a case-typo'd row is unreachable authority
Review fixes.
REQUIRED: `member` now lowercases and trims the address before ValidAddress and
before every Role/AddMember/RemoveMember call. Every other address path in the
package already normalizes (sign-in, invite), and `memberships` has a
literal-text primary key with no COLLATE NOCASE, so case is the difference
between a row and no row. Two silent failures, both now covered by tests that
fail without this change:
1. `member -add James@Keymail.dev bab -role owner` reported "added as owner"
while writing a row the real session — signing in as james@keymail.dev —
could never match. The operator walks away believing access was restored.
That is the worst failure available to a tool whose whole job is repairing
authorization, and it is exactly the command about to be run for
james@keymail.dev.
2. A case-typo'd duplicate counted as a second owner, so lastOwnerGuard would
let the only functionally reachable owner be removed while reporting that
another remained. The test proves the pre-fix DB really held two owner
rows for one person.
Recommended: when CreateAccount fails after MintAccount has already claimed the
id, log the sqid. The manifest exists with no console row and no owner —
unreachable rather than dangerous, and the next mint just takes the following
id — but the only other trace was a 500 in somebody's browser. Now there is a
line naming accounts/<sqid>/account.json so it can be adopted with
`console member -add … -role owner` or deleted.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 files changed,
+132
−3
cmd/console/member.go+22 −3cmd/console/member_test.go+100 −0internal/console/server.go+10 −0
diff --git a/cmd/console/member.go b/cmd/console/member.go| index bbf513893..3ab9fdec3 100644 |
| --- a/cmd/console/member.go |
| +++ b/cmd/console/member.go |
| @@ -6,6 +6,7 @@ import ( |
| "fmt" |
| "log" |
| "os" |
| + "strings" |
| "time" |
| "github.com/carlosframework/platform/internal/console" |
| @@ -62,17 +63,35 @@ func runMember(args []string) { |
| if len(pos) != 1 { |
| log.Fatal(memberUsage) |
| } |
| - sqid = pos[0] |
| + sqid = strings.TrimSpace(pos[0]) |
| default: |
| if len(pos) != 2 { |
| log.Fatal(memberUsage) |
| } |
| - address, sqid = pos[0], pos[1] |
| + typed := pos[0] |
| + sqid = strings.TrimSpace(pos[1]) |
| + // Lowercased and trimmed before ANY store call — every other address |
| + // path in the console does the same (sign-in, invite), and the |
| + // memberships primary key is literal text with no COLLATE NOCASE, so |
| + // case is the difference between a row and no row. Two ways an |
| + // unnormalized address here goes wrong, both silent: |
| + // |
| + // 1. `member -add James@Keymail.dev bab` would report "added as |
| + // owner" while writing a row the real session — which signs in as |
| + // james@keymail.dev — can never match. The operator walks away |
| + // believing access was restored when it was not, which is the |
| + // worst possible failure for a tool whose whole job is fixing |
| + // authorization. |
| + // 2. A case-typo'd second owner row would inflate the store's |
| + // last-owner count, letting `member -remove` take away the only |
| + // owner anybody can actually sign in as while the guard reports |
| + // another still exists. |
| + address = strings.ToLower(strings.TrimSpace(typed)) |
| // Same shape check the sign-in form makes: a membership for an address |
| // keymail could never authenticate is a grant nobody can claim, and a |
| // silent typo here is a member who "was added" and cannot get in. |
| if !console.ValidAddress(address) { |
| - log.Fatalf("%q is not a keymail address — expected <user>@<host>, e.g. paul@keymail.dev", address) |
| + log.Fatalf("%q is not a keymail address — expected <user>@<host>, e.g. paul@keymail.dev", typed) |
| } |
| } |
diff --git a/cmd/console/member_test.go b/cmd/console/member_test.go| index b786ee3c2..31c9f8663 100644 |
| --- a/cmd/console/member_test.go |
| +++ b/cmd/console/member_test.go |
| @@ -137,6 +137,92 @@ func TestMemberVerbRefusesToStrandAnAccount(t *testing.T) { |
| } |
| } |
| +// Addresses are case-insensitive identities, but the memberships primary key |
| +// is literal text with no COLLATE NOCASE — so an unnormalized address here |
| +// writes a row nobody's session can ever match, while the verb cheerfully |
| +// reports success. That is the worst failure available to a tool whose entire |
| +// job is repairing authorization: the operator walks away believing access was |
| +// restored. |
| +func TestMemberVerbNormalizesTheAddress(t *testing.T) { |
| + bin := buildConsole(t) |
| + tmp := t.TempDir() |
| + dbPath := filepath.Join(tmp, "console.db") |
| + run := runner(t, bin, "CONSOLE_DB="+dbPath) |
| + |
| + st, err := console.Open(dbPath) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := st.CreateAccount(console.OpsAccountID, "bab", "ops", "paul@keymail.dev", time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + st.Close() |
| + |
| + if out, err := run("member", "-add", " James@Keymail.dev ", "bab", "-role", "owner"); err != nil { |
| + t.Fatalf("member -add: %v\n%s", err, out) |
| + } |
| + // The address the real session presents is the lowercase one. |
| + if role := roleOf(t, dbPath, console.OpsAccountID, "james@keymail.dev"); role != console.RoleOwner { |
| + t.Fatalf("role for the lowercase address = %q, want owner — the row is unreachable", role) |
| + } |
| + // And it is ONE row, not two: the mixed-case form resolves to the same |
| + // membership, so re-adding it is the no-op it looks like. |
| + out, err := run("member", "-add", "JAMES@keymail.dev", "bab", "-role", "owner") |
| + if err != nil { |
| + t.Fatalf("re-add in another case: %v\n%s", err, out) |
| + } |
| + if !strings.Contains(out, "already") { |
| + t.Errorf("a case variant created a second membership: %s", out) |
| + } |
| + if out, _ := run("member", "-list", "bab"); strings.Contains(out, "James@Keymail.dev") || |
| + strings.Contains(out, "JAMES@keymail.dev") { |
| + t.Errorf("member list shows an unnormalized address: %s", out) |
| + } |
| + // Removal normalizes too, or the tool cannot undo what it just did. |
| + if out, err := run("member", "-remove", "James@Keymail.dev", "bab"); err != nil { |
| + t.Fatalf("member -remove: %v\n%s", err, out) |
| + } |
| + if role := roleOf(t, dbPath, console.OpsAccountID, "james@keymail.dev"); role != "" { |
| + t.Errorf("role after case-variant removal = %q, want none", role) |
| + } |
| +} |
| + |
| +// The second way an unnormalized address breaks things: a case-typo'd |
| +// duplicate would count as a second owner, so the last-owner guard would let |
| +// the only owner anybody can sign in as be removed while reporting that |
| +// another remains. Normalizing collapses the duplicate and the invariant holds. |
| +func TestMemberVerbCaseVariantCannotFakeASecondOwner(t *testing.T) { |
| + bin := buildConsole(t) |
| + tmp := t.TempDir() |
| + dbPath := filepath.Join(tmp, "console.db") |
| + run := runner(t, bin, "CONSOLE_DB="+dbPath) |
| + |
| + st, err := console.Open(dbPath) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := st.CreateAccount(42, "cdef", "Kass", "solo@keymail.dev", time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + st.Close() |
| + |
| + if out, err := run("member", "-add", "Solo@Keymail.dev", "cdef", "-role", "owner"); err != nil { |
| + t.Fatalf("member -add: %v\n%s", err, out) |
| + } |
| + members := membersOf(t, dbPath, 42) |
| + if len(members) != 1 { |
| + t.Fatalf("account has %d members, want 1 — a case variant became a second owner: %+v", |
| + len(members), members) |
| + } |
| + out, err := run("member", "-remove", "solo@keymail.dev", "cdef") |
| + if err == nil { |
| + t.Fatalf("removed the only functionally reachable owner: %s", out) |
| + } |
| + if !strings.Contains(out, "last owner") { |
| + t.Errorf("refusal does not explain itself: %s", out) |
| + } |
| +} |
| + |
| // A typo must name what it looked for, not just fail. These commands get run |
| // at 2am against a production box. |
| func TestMemberVerbRejectsBadArguments(t *testing.T) { |
| @@ -221,6 +307,20 @@ func TestMemberVerbAdministersABucketOnlyAccount(t *testing.T) { |
| } |
| } |
| +func membersOf(t *testing.T, dbPath string, accountID int64) []console.Account { |
| + t.Helper() |
| + st, err := console.Open(dbPath) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + defer st.Close() |
| + members, err := st.Members(accountID) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + return members |
| +} |
| + |
| func roleOf(t *testing.T, dbPath string, accountID int64, address string) string { |
| t.Helper() |
| st, err := console.Open(dbPath) |
diff --git a/internal/console/server.go b/internal/console/server.go| index 5b90a3b64..011662a43 100644 |
| --- a/internal/console/server.go |
| +++ b/internal/console/server.go |
| @@ -383,6 +383,16 @@ func (s *Server) mintAccount(ctx context.Context, name, ownerAddr string) (*Acco |
| return nil, err |
| } |
| if err := s.store.CreateAccount(id, sqid, name, ownerAddr, now); err != nil { |
| + // The manifest already landed, so the id is claimed and there is now an |
| + // account in the bucket with no console row and no owner. It is |
| + // unreachable rather than dangerous (no membership means no access, and |
| + // materialization gives it no members either), and the next mint simply |
| + // takes the id after it. But without this line the only trace is a 500 |
| + // in the user's browser: name the sqid so the stranded account can be |
| + // found and either adopted with `console member -add … -role owner` or |
| + // deleted from the bucket. |
| + log.Printf("console: minted account %s (id %d, %q) into the bucket but could not record it "+ |
| + "locally: %v — accounts/%s/account.json is stranded with no owner", sqid, id, name, err, sqid) |
| return nil, err |
| } |
| if err := s.store.SetLastAccount(ownerAddr, id); err != nil { |