fix(moderation): write the address ranges the ModCP lookup reads

apps/community/src/server/auth-actions.test.ts

@@ -113,6 +113,7 @@ const {
 } = await import('./auth-actions')
 const { EMPTY_STATE } = await import('./auth-form-state')
 const { SESSION_COOKIE } = await import('./cookies')
+const { getContainer } = await import('./container')
 
 const CONTAINER_KEY = Symbol.for('@meith/forum.container')
 
@@ -406,6 +407,43 @@ describe('loginAction', () => {
   })
 })
 
+describe('the address range an account is recorded against', () => {
+  const ADDRESS = '203.0.113.7'
+  const RANGE = '203.0.113.0/24'
+
+  it('stores the range a registration came from, never the address', async () => {
+    const create = vi.spyOn(getContainer().accountStore.accounts, 'create')
+
+    await registerUser()
+
+    expect(create).toHaveBeenCalledWith(
+      expect.objectContaining({ registrationIpPrefix: RANGE }),
+    )
+    expect(JSON.stringify(create.mock.calls)).not.toContain(ADDRESS)
+  })
+
+  it('records the range a sign-in came from, never the address', async () => {
+    await registerUser()
+    const record = vi.spyOn(getContainer().accountStore.accounts, 'recordLastIpPrefix')
+
+    await redirectOf(
+      loginAction(EMPTY_STATE, form({ identifier: CREDS.username, password: CREDS.password })),
+    )
+
+    expect(record).toHaveBeenCalledWith(expect.any(Number), RANGE)
+    expect(JSON.stringify(record.mock.calls)).not.toContain(ADDRESS)
+  })
+
+  it('records nothing when a sign-in is refused', async () => {
+    await registerUser()
+    const record = vi.spyOn(getContainer().accountStore.accounts, 'recordLastIpPrefix')
+
+    await loginAction(EMPTY_STATE, form({ identifier: CREDS.username, password: 'wrong' }))
+
+    expect(record).not.toHaveBeenCalled()
+  })
+})
+
 describe('password reset', () => {
   it('gives the same answer for a known and an unknown address', async () => {
     await registerUser()

apps/community/src/server/auth-actions.ts

@@ -42,6 +42,10 @@ function field(form: FormData, name: string): string {
 
 const toFormState = formStateReporter('auth-actions', 'unexpected error in auth action')
 
+async function addressContext(): Promise<{ readonly ipPrefix: string | null }> {
+  return { ipPrefix: truncateIp(await remoteAddress()) ?? null }
+}
+
 async function loginBuckets(
   identifier: string,
   config: AuthConfig,
@@ -104,7 +108,10 @@ export async function registerAction(
         ? []
         : await fields.validateRegistration({ submitted: submittedFields(form), context })
 
-    const result = await identity.register({ username, email, password })
+    const result = await identity.register(
+      { username, email, password },
+      await addressContext(),
+    )
 
     if (fields !== null) await fields.applyRegistration(result.account.id, fieldValues)
 
@@ -190,6 +197,7 @@ export async function loginAction(
       identifier,
       password,
       await loginBuckets(identifier, config),
+      await addressContext(),
     )
     await setSessionCookie(result.sessionToken, result.expiresAt)
 

docs/mybb-parity.md

@@ -629,6 +629,24 @@ choice made here. It is stated on the screen because the difference matters to
 what a moderator does next: "shares an address" reads as proof, "shares a range"
 reads as something to check, and only the second is what the data supports.
 
+**Two ranges are on record per account, and they are written at two moments.**
+`registration_ip_prefix` is written once, by the registration that created the
+account; `last_ip_prefix` is rewritten by every successful sign-in. MyBB also
+stamps `lastip` on ordinary page views; here the presence write is left alone,
+because the sign-in is the moment the board learns an account is being used from
+somewhere and it costs one update per session rather than one per member per
+minute. The consequence is worth knowing at the screen: a member who is still
+signed in from before this shipped shows no last-visit range until they sign in
+again, and one who never signs in again keeps the range of their last sign-in
+rather than of their last visit.
+
+**Cost.** Both columns are null for every account the board already had, and for
+every account a MyBB import creates — the importer does not carry `regip` or
+`lastip` across, so an imported board's lookups stay empty until its members
+register or sign in here. `posts.ip_prefix` exists in the schema and nothing
+writes it: the lookup does not read it, and a per-post range would be a second
+address trail to keep rather than a second thing to search.
+
 ### Copying a thread credits its authors twice
 
 **MyBB:** copying a thread duplicates its posts, and each copy counts towards

docs/operating.md

@@ -58,11 +58,14 @@ not the same as encrypted, and is worth knowing before choosing.
 
 ### Who the board thinks you are
 
-Four things key off a visitor's address: the control panel allowlist, the login
-lockout counters, the hourly limits a guest gets, and the truncated address
-written to the moderator log. Behind a proxy the board cannot see the connection
-— it sees `X-Forwarded-For`, a header each proxy **appends** its view of the
-caller to, and which the caller may send some of themselves.
+Five things key off a visitor's address: the control panel allowlist, the login
+lockout counters, the hourly limits a guest gets, the truncated address written
+to the moderator log, and the truncated range recorded against an account when
+it registers and each time it signs in — which is what the ModCP's address
+lookup and the member search's **IP** filter read. Behind a proxy the board
+cannot see the connection — it sees `X-Forwarded-For`, a header each proxy
+**appends** its view of the caller to, and which the caller may send some of
+themselves.
 
 `TRUSTED_PROXY_HOPS` is how many proxies are in front of the board, and the
 board reads that many entries back from the **right-hand** end of the chain.
@@ -83,7 +86,8 @@ audit trail. When in doubt, count the proxies and use that number; it is safer
 to be one too low than one too high.
 
 At `0` the board resolves no address at all: the allowlist refuses everybody,
-guest limits fall back to a single shared bucket, and the log records nothing.
+guest limits fall back to a single shared bucket, and neither the log nor an
+account's ranges record anything.
 It warns once per process when a request arrives with a forwarding header it has
 been told to ignore.
 

packages/accounts/src/member-settings.test.ts

@@ -103,6 +103,7 @@ class MemoryAccounts implements AccountRepository {
   async markEmailVerified(): Promise<null> {
     return null
   }
+  async recordLastIpPrefix() {}
 }
 
 class MemorySessions {

packages/accounts/src/memory-repos.ts

@@ -16,6 +16,10 @@ import type {
 
 class MemoryAccounts implements AccountRepository {
   private readonly byId = new Map<number, AccountRecord>()
+  private readonly ipPrefixes = new Map<
+    number,
+    { registration: string | null; lastVisit: string | null }
+  >()
   private seq = 0
 
   async findById(id: number): Promise<AccountRecord | null> {
@@ -51,9 +55,19 @@ class MemoryAccounts implements AccountRepository {
       primaryGroupId: input.primaryGroupId,
     }
     this.byId.set(record.id, record)
+    this.ipPrefixes.set(record.id, {
+      registration: input.registrationIpPrefix ?? null,
+      lastVisit: null,
+    })
     return record
   }
 
+  async recordLastIpPrefix(userId: number, prefix: string): Promise<void> {
+    const current = this.ipPrefixes.get(userId)
+    if (current === undefined) return
+    this.ipPrefixes.set(userId, { ...current, lastVisit: prefix })
+  }
+
   async updatePassword(userId: number, passwordHash: string, passwordAlgo: string): Promise<void> {
     const cur = this.byId.get(userId)
     if (cur) this.byId.set(userId, { ...cur, passwordHash, passwordAlgo })

packages/accounts/src/ports.ts

@@ -24,6 +24,7 @@ export interface NewAccount {
   readonly passwordAlgo: string
   readonly state: AccountState
   readonly primaryGroupId: number
+  readonly registrationIpPrefix?: string | null
 }
 
 export interface AccountRepository {
@@ -35,6 +36,7 @@ export interface AccountRepository {
   setState(userId: number, state: AccountState): Promise<void>
   markEmailVerified(userId: number, at: Date, activate: boolean): Promise<AccountState | null>
   touchLastActive(userId: number, now: Date, windowSeconds: number): Promise<boolean>
+  recordLastIpPrefix(userId: number, prefix: string): Promise<void>
 }
 
 export interface MemberProfileRecord {

packages/accounts/src/service.test.ts

@@ -1,6 +1,6 @@
 import { ForbiddenError, ValidationError } from '@meith/core'
 import { argon2id } from 'hash-wasm'
-import { beforeEach, describe, expect, it } from 'vitest'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
 
 import { rejectionMessage } from './test-support.fixture'
 
@@ -752,3 +752,66 @@ describe('ban filters block registration and login', () => {
     await expect(service.register(CREDS)).resolves.toBeDefined()
   })
 })
+
+describe('the address ranges an account is recorded against', () => {
+  const CREDS = {
+    username: 'Ivan',
+    email: 'ivan@example.com',
+    password: 'correct horse battery',
+  }
+  const REGISTERED_FROM = '198.51.100.0/24'
+  const SIGNED_IN_FROM = '203.0.113.0/24'
+
+  let store: AccountStore
+
+  beforeEach(() => {
+    store = createMemoryStore()
+  })
+
+  it('keeps the registration range the caller resolved', async () => {
+    const create = vi.spyOn(store.accounts, 'create')
+    const { service } = makeService(store)
+
+    await service.register(CREDS, { ipPrefix: REGISTERED_FROM })
+
+    expect(create).toHaveBeenCalledWith(
+      expect.objectContaining({ registrationIpPrefix: REGISTERED_FROM }),
+    )
+  })
+
+  it('records the range a sign-in came from', async () => {
+    const { service } = makeService(store)
+    const { account } = await service.register(CREDS, { ipPrefix: REGISTERED_FROM })
+
+    const record = vi.spyOn(store.accounts, 'recordLastIpPrefix')
+    await service.login('ivan', CREDS.password, 'ivan', { ipPrefix: SIGNED_IN_FROM })
+
+    expect(record).toHaveBeenCalledWith(account.id, SIGNED_IN_FROM)
+  })
+
+  it('records nothing when the board resolved no address', async () => {
+    const { service } = makeService(store)
+    const create = vi.spyOn(store.accounts, 'create')
+    const record = vi.spyOn(store.accounts, 'recordLastIpPrefix')
+
+    await service.register(CREDS)
+    await service.login('ivan', CREDS.password, 'ivan')
+
+    expect(create).toHaveBeenCalledWith(
+      expect.objectContaining({ registrationIpPrefix: null }),
+    )
+    expect(record).not.toHaveBeenCalled()
+  })
+
+  it('records nothing for a sign-in that failed', async () => {
+    const { service } = makeService(store)
+    await service.register(CREDS, { ipPrefix: REGISTERED_FROM })
+
+    const record = vi.spyOn(store.accounts, 'recordLastIpPrefix')
+    await expect(
+      service.login('ivan', 'wrong', 'ivan', { ipPrefix: SIGNED_IN_FROM }),
+    ).rejects.toThrow(ValidationError)
+
+    expect(record).not.toHaveBeenCalled()
+  })
+})

packages/accounts/src/service.ts

@@ -37,6 +37,7 @@ export interface BanLookup {
 
 export interface RequestContext {
   readonly ip?: string | undefined
+  readonly ipPrefix?: string | null | undefined
 }
 
 export interface RegisterInput {
@@ -140,6 +141,7 @@ export class IdentityService {
       passwordAlgo: PASSWORD_ALGO,
       state,
       primaryGroupId: this.config.defaultMemberGroupId,
+      registrationIpPrefix: prefixOf(context),
     })
 
     if (this.verifiesEmail()) {
@@ -271,6 +273,11 @@ export class IdentityService {
       await this.store.accounts.updatePassword(account.id, upgraded, PASSWORD_ALGO)
     }
 
+    const prefix = prefixOf(context)
+    if (prefix !== null) {
+      await this.store.accounts.recordLastIpPrefix(account.id, prefix)
+    }
+
     return this.startSession(account, at)
   }
 
@@ -390,6 +397,13 @@ export class IdentityService {
   }
 }
 
+function prefixOf(context: RequestContext): string | null {
+  const prefix = context.ipPrefix
+  if (prefix === undefined || prefix === null) return null
+  const value = prefix.trim()
+  return value === '' ? null : value
+}
+
 let dummyHashPromise: Promise<string> | null = null
 function dummyHash(): Promise<string> {
   if (dummyHashPromise === null) {

packages/db/src/account-repos.test.ts

@@ -1,9 +1,12 @@
 import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'
 
+import { truncateIp } from '@meith/core'
+
 import { createTestDb, type TestDb } from './pglite.fixture'
 import {
   createPostgresAccountStore,
 } from './account-repos'
+import { PostgresModCpRepository } from './modcp-repo'
 import { credentialTokens, loginAttempts, rememberTokens, sessions, usergroups } from './schema'
 import type { AccountStore } from '@meith/accounts'
 
@@ -304,4 +307,67 @@ describe('Postgres account repositories', () => {
       expect(await store.loginAttempts.countFailuresSince('alice', new Date(0))).toBe(0)
     })
   })
+
+  describe('the address ranges the moderator panel looks up', () => {
+    let modcp: PostgresModCpRepository
+
+    beforeAll(() => {
+      modcp = new PostgresModCpRepository(h.db)
+    })
+
+    async function joined(name: string, address: string | null): Promise<number> {
+      const created = await store.accounts.create({
+        username: name,
+        usernameLower: name,
+        email: `${name}@example.test`,
+        emailLower: `${name}@example.test`,
+        passwordHash: 'x',
+        passwordAlgo: 'argon2id',
+        state: 'active',
+        primaryGroupId: 2,
+        registrationIpPrefix: address === null ? null : (truncateIp(address) ?? null),
+      })
+      return created.id
+    }
+
+    it('keeps the range a registration came from, never the address', async () => {
+      const id = await joined('ida', '192.0.2.14')
+
+      expect(await modcp.ipPrefixesFor(id)).toEqual({
+        registration: '192.0.2.0/24',
+        lastVisit: null,
+      })
+    })
+
+    it('records a sign-in range without disturbing the registration range', async () => {
+      const id = await joined('ines', '192.0.2.14')
+
+      await store.accounts.recordLastIpPrefix(id, truncateIp('198.18.51.9')!)
+
+      expect(await modcp.ipPrefixesFor(id)).toEqual({
+        registration: '192.0.2.0/24',
+        lastVisit: '198.18.51.0/24',
+      })
+    })
+
+    it('finds the account sharing a range and leaves an unrelated one out', async () => {
+      const iris = await joined('iris', '198.51.100.14')
+      const ivo = await joined('ivo', '198.51.100.200')
+      const ilse = await joined('ilse', '203.0.113.9')
+
+      const matches = await modcp.ipMatches(iris, 10)
+
+      expect(matches.map((m) => m.userId)).toEqual([ivo])
+      expect(matches[0]).toMatchObject({ username: 'ivo', matchedOn: 'registration' })
+      expect(matches.map((m) => m.userId)).not.toContain(ilse)
+    })
+
+    it('leaves an account with no recorded range out of every lookup', async () => {
+      const inge = await joined('inge', null)
+      const ingrid = await joined('ingrid', null)
+
+      expect(await modcp.ipMatches(inge, 10)).toEqual([])
+      expect((await modcp.ipMatches(ingrid, 10)).map((m) => m.userId)).not.toContain(inge)
+    })
+  })
 })