Harden the additionalFields write-protection guard: OIDC rationale is inaccurate and the lock assertion is substring-based

[jakebromberg]

Problem

Two gaps in the additionalFields write-protection guard that BS#2358 introduced, both found by that PR's review and deliberately deferred rather than expanding an already twice-bounced schema PR.

1. The returned: false rationale claims OIDC coverage the flag does not provide

shared/authentication/src/auth.definition.ts (the selfSignupReviewedBy comment) states that returned: false keeps the field out of the OIDC additional_user_info claim, citing buildOidcUserInfoClaim as spreading the whole user object. Both halves are wrong:

• buildOidcUserInfoClaim (shared/authentication/src/jwt-payload.ts) returns exactly { role, capabilities } and spreads nothing. The two cited line numbers both belong to buildJwtPayload, which does spread.
• better-auth hands getAdditionalUserInfoClaim the raw internalAdapter.findUserById row (better-auth/dist/plugins/oidc-provider/index.mjs, for both the id_token and /oauth2/userinfo), never a parseUserOutput result — so returned: false is inert on the OIDC path in both directions.

The field genuinely is protected there, but by explicit allowlisting in buildOidcUserInfoClaim, which is the stricter mechanism. The problem is that the comment, the docs/authentication.md paragraph, and the test name all assert the flag is what covers it. The natural refactor — harmonising buildOidcUserInfoClaim with buildJwtPayload as return { ...userRecord, role, capabilities } — would ship the reviewing manager's auth_user.id to every relying party (wxyc-crm, wxyc-canary) with the comment still reading as coverage and the test still green, since it only greps the declaration for returned:\s*false and never exercises a payload.

The other three surfaces the comment claims (/get-session, the JWT, /admin/list-users) were verified correct against better-auth 1.6.26.

2. The lock assertion matches input: false anywhere in a field's brace body

tests/unit/authentication/pii-additional-fields-input.test.ts asserts /input:\s*false/ against the whole brace body of each field, so any construct rendering that substring satisfies it. Verified surviving mutants, all of which remain writable via public POST /update-user because parseInputData reads only fields[key].input:

• capabilities: { ..., validator: { input: false } }
• { transform: { input: false } }
• { label: 'input: false' }
• a regex literal /input: false/

Pre-existing rather than introduced by BS#2358 — it survives identically against that PR's parent — but it is the same class of silent hole the file was rewritten to close, on the object holding the capabilities privilege grant.

Desired end state

• The returned: false rationale describes the mechanism that actually protects each surface, and the OIDC allowlist in buildOidcUserInfoClaim is documented as load-bearing so a future refactor does not quietly remove it. A test that exercises an actual payload — rather than grepping a declaration — would be worth more than the comment fix.
• The lock assertion binds to the field's own input key rather than to a substring anywhere in its body, so a nested object, a string default, or a regex literal cannot satisfy it.

Where

• shared/authentication/src/auth.definition.ts — the additionalFields block and the selfSignupReviewedBy rationale comment
• shared/authentication/src/jwt-payload.ts — buildJwtPayload and buildOidcUserInfoClaim
• tests/unit/authentication/pii-additional-fields-input.test.ts — the lock assertion and the stripComments scanner
• docs/authentication.md — the input/returned paragraph

Also worth folding in

• The vacuity guard is fields.length >= 8 against 9 declared fields, so exactly one silently-dropped field would not trip it. An exact count removes the slack.
• shared/database/src/schema.ts: the ip_hash canonicalization rule (trim, lowercase, reduce IPv4-mapped IPv6 to dotted quad) normalises only a subset of IPv6 textual forms, while the "equal client IPs produce equal hashes" claim below it is unconditional. It holds in practice because the input is pinned to nginx's X-Real-IP ($remote_addr), which renders one form per peer. One scoping clause makes the claim exact.

Acceptance criteria

• A mutation that harmonises buildOidcUserInfoClaim into spreading the user record turns a test red.
• capabilities: { ..., validator: { input: false } } turns the lock assertion red.
• { label: 'input: false' } turns the lock assertion red.
• Comment, docs/authentication.md, and test naming agree with the verified mechanism per surface.
• Existing mutation controls still pass: unlocking capabilities, unlocking isAnonymous, adding an unflagged field, locking the allowlisted appSkin.

Related

Follow-up from #2358 (PR #2366). Part of the surface described in the epic #2365.