feat(auth): derive auth_user.name from handle/username at a single databaseHooks choke point - #2297
Merged
Merged
Conversation
…tabaseHooks choke point auth_user.name has secretly duplicated the DJ's legal name since the djs-table collapse (a0cd197): dj-site provisioning fills it with realName || username, and every innocent .name read has been a potential PII leak to a public surface (BS#1286/BS#1288, BS#1393, BS#2281). Track 2b: databaseHooks.user.create.before/update.before in auth.definition.ts derive name from the on-air handle (else username, else the supplied value) via two pure helpers in the new derive-user-display-name.ts, mirroring capSessionUpdateAgainstDeviceFlow's { data: {...} } | undefined return contract that better-auth's with-hooks.mjs merge requires. update.before is payload-only by construction, since better-auth never hands an update hook the row it's updating. Track 2c: provisionUser() stops trusting a caller-supplied name and derives it itself (belt and suspenders with the hook); name is no longer a required field on ProvisionUserInput or the provision-user route. Track 2d: jobs/auth-user-name-backfill/ rewrites existing rows to the same derivation, computed in TypeScript via the canonical resolveDjDisplayName helper. It opens with a machine-enforced precondition gate that aborts non-zero if any row still holds its only copy of a legal name in name (the preserve-first copy to real_name hasn't run yet) — dry-run by default, --execute to write. Track 3b: tests/integration/dj-real-name-sentinel.spec.js seeds a real_name/name-duplicated auth_user row plus a show and flowsheet entries, then whole-body-scans GET /flowsheet, /flowsheet/latest, /flowsheet/range, and /flowsheet/search (including a dj: operator query) to prove the legal name never reaches a public response body. Verified against a live stack: passes cleanly, and demonstrably fails when a deliberate leak is introduced into the /flowsheet/range shows projection.
…l gate false-positive Six code-review findings against #2297's databaseHooks-based auth_user.name derivation: - deriveUserNameOnUpdate now rejects (returns false, aborting the write) any update payload that carries a bare `name` without a djName that resolves to a usable handle — closes the hole where better-auth's public POST /update-user let any signed-in session rewrite `name` verbatim, re-creating the hidden-legal-name-copy state this plan exists to close. Also rejects a name payload paired with an unusable (blank/'Anonymous') djName, closing the trivial bypass of attaching one to slip past the name-only rejection. - violatesPreserveFirstPrecondition (the backfill's preserve-first gate) now exempts rows whose name already equals the resolved on-air handle. Without this, a user provisioned after this program's hooks deploy (name=handle, real_name blank, name distinct from username) would false-positive the gate forever, and its remediation message would have an operator copy a handle into the real_name PII column. - user.additionalFields.realName/djName now set input:false, closing the public /update-user write path for those fields directly. Verified against the installed better-auth (v1.6.26) that this doesn't affect the admin plugin's updateUser route or internalAdapter-based writers (provisioning, onboarding), both of which bypass input filtering entirely. - fetchAllUsers now filters to non-anonymous rows at the query instead of loading (and discarding) every anonymous per-device row's real_name into memory. - job.test.ts's hand-rolled local renderSql is replaced with the canonical tests/utils/render-sql.ts helper. - create-auto-dj-user.ts's stale comment claiming `name` is the load-bearing carrier is rewritten to point at djName, which the derivation hook actually treats as invariant. Also registers @wxyc/auth-user-name-backfill in the jobs/package table.
Member
Author
|
Pushed a follow-up commit addressing review findings on this PR:
All six findings covered by new/updated unit tests (TDD: failing test first). Full local gate green (format, typecheck incl. the jobs workspace, lint 0 errors, full unit suite). CI: https://github.com/WXYC/Backend-Service/actions/runs/33099085511 |
…te output Consolidates the auth_user.name derivation chain (handle, else username) into deriveUserPublicName in dj-name.ts and reuses it at the three sites that had drifted into re-deriving it independently: provisionUser, the create hook, and the backfill job's rewrite-target computation. Renames deriveUserNameOnUpdate to deriveOrRejectUserNameOnUpdate so its signature advertises the write-abort veto, and moves the rejection policy and the databaseHooks merge-contract writeup to that function's docblock as their one canonical home, leaving pointers everywhere else they were previously restated. Adds the partition sentence clarifying that the input:false lock and the update-hook veto are complementary PII guards, not redundant ones. Rewrites the backfill gate's docblock to describe the preserve-first predicate in prose instead of a SQL quote it admitted diverging from, and changes the precondition-gate failure to emit the complete violating-id list (never row values) instead of a 10-row sample, so an operator can paste it straight into a remediation query. De-boilerplates the job/decide/pii-input/sentinel test suites with row factories, it.each, and a shared assertion helper without touching any assertion.
Member
Author
|
Simplify pass follow-up, pushed as
|
jakebromberg
added a commit
that referenced
this pull request
Aug 28, 2026
…l gate false-positive Six code-review findings against #2297's databaseHooks-based auth_user.name derivation: - deriveUserNameOnUpdate now rejects (returns false, aborting the write) any update payload that carries a bare `name` without a djName that resolves to a usable handle — closes the hole where better-auth's public POST /update-user let any signed-in session rewrite `name` verbatim, re-creating the hidden-legal-name-copy state this plan exists to close. Also rejects a name payload paired with an unusable (blank/'Anonymous') djName, closing the trivial bypass of attaching one to slip past the name-only rejection. - violatesPreserveFirstPrecondition (the backfill's preserve-first gate) now exempts rows whose name already equals the resolved on-air handle. Without this, a user provisioned after this program's hooks deploy (name=handle, real_name blank, name distinct from username) would false-positive the gate forever, and its remediation message would have an operator copy a handle into the real_name PII column. - user.additionalFields.realName/djName now set input:false, closing the public /update-user write path for those fields directly. Verified against the installed better-auth (v1.6.26) that this doesn't affect the admin plugin's updateUser route or internalAdapter-based writers (provisioning, onboarding), both of which bypass input filtering entirely. - fetchAllUsers now filters to non-anonymous rows at the query instead of loading (and discarding) every anonymous per-device row's real_name into memory. - job.test.ts's hand-rolled local renderSql is replaced with the canonical tests/utils/render-sql.ts helper. - create-auto-dj-user.ts's stale comment claiming `name` is the load-bearing carrier is rewritten to point at djName, which the derivation hook actually treats as invariant. Also registers @wxyc/auth-user-name-backfill in the jobs/package table.
This was referenced Aug 28, 2026
jakebromberg
added a commit
that referenced
this pull request
Aug 28, 2026
The registry still described the auth_user.name conversion as pending. The databaseHooks derivation hooks shipped in #2297 (veto fix #2301), and the one-shot backfill ran against production on 2026-08-28: 139 of 144 non-anonymous rows rewritten to handle-else-username, with the 2 rows whose only legal-name copy lived in name preserved into real_name first (Track 2a). Updates the history paragraph, the auth_user.name row's classification/read-sites/enforcement cells, and nothing else.
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.
Tracks 2b–2d + 3b of
plans/dj-name-pii-safeguards.md(the plan ships in the sibling PR #2294). This is the structural fix:auth_user.namestops being a hidden second copy of the DJ's legal name and becomes a derived, public-safe display value at a single choke point that every writer — including one that lives in another repo — must pass through.Closes #2296
What's here, piece by piece
1. The choke point —
databaseHooks.user.create.before/update.before(Track 2b)shared/authentication/src/derive-user-display-name.tsadds two pure functions;auth.definition.tswires them into thedatabaseHooks.userblock that already exists there.The single site covers
provision-user.ts,create-default-user.ts,create-auto-dj-user.ts,complete-onboarding.ts— and dj-site's directauthClient.admin.updateUserroster path, which no Backend-Service call-site helper can reach, because that request never passes through Backend-Service at all. Per-call-site derivation would have re-scattered the invariant across N writers, which is the disease this PR treats.create.beforederivesresolveDjDisplayName(djName) ?? username ?? <supplied name>. That last terminal is deliberate, not laziness: it is how'Anonymous'(better-auth's anonymous plugin) and'Auto DJ'(create-auto-dj-user.ts) pass through unclobbered, since neither has a resolvable handle or a username.update.beforeis payload-only by construction, and the docblock records why at length so nobody "fixes" it later. Verified against better-auth'sdb/with-hooks.mjsat the lockfile-resolved 1.6.26:internalAdapter.updateUser(userId, data)buildswhere: [{ field: 'id', value: userId }]itself and callsupdateWithHooks(data, where, 'user', …)— the hook only ever receivestoRun(data, context)and never seeswhere, so anupdate.beforehook cannot fetch the row it is updating.complete-onboarding.tsgoes through the identical path from outside any better-auth endpoint, so a request-context fallback is unavailable there either.The
{ data: … } | undefinedreturn contract is load-bearing rather than stylistic, and mirrorscapSessionUpdateAgainstDeviceFlowfor that reason:with-hooks.mjsmerges only a returned value satisfying'data' in result. A mutated argument or a bare object is silently discarded — the hook appears to run while the write proceeds with the original payload — andfalseaborts the write outright. For a PII guard, "fails as a silent no-op" is the worst available failure mode, so the shape is pinned and unit-tested.Two update shapes are left untouched on purpose: handle-clear, and username-only rename (a username-only payload cannot reveal whether the user currently has a handle, so deriving from it risks clobbering a live one). Post-backfill both leave a prior handle or username in
name— cosmetic staleness, structurally non-PII.2. Provision route stops trusting the client (Track 2c)
nameleaves the required-fields array inapps/auth/app.tsand becomes optional onProvisionUserInput;provisionUser()derives the stored value itself fromdjName/username.Deriving at the call site as well as in the hook is belt-and-suspenders on purpose: whether the hook merge runs before better-auth's adapter enforces the core schema's
required: trueonnameis unverified against the resolved version, and supplying the value here removes the ordering dependency entirely. The derived value is assigned directly rather than spread conditionally, so the key is never present-but-undefined— a distinction that adapter validation can treat differently from an absent key.A still-supplied
nameis accepted-and-ignored for the deploy overlap window, so dj-site can drop the field in its own PR without a lockstep deploy.3.
jobs/auth-user-name-backfill/— and the gate it opens with (Track 2d)A one-shot workspace (
package.jsonwith"job-type": "one-shot",tsconfig.json,tsup.config.ts, rootDockerfile.auth-user-name-backfill), dry-run by default,--executeto write, followingflowsheet-ghost-row-sweep's convention rather thanlegacy-dj-name-remediation's inverted one. ~139 rows; decisions computed in TypeScript through the canonicalresolveDjDisplayNameand never re-derived in SQL. Contradictory--execute --dry-runthrows rather than silently picking one.The job's first act is a machine-enforced precondition gate (
decide.ts'sviolatesPreserveFirstPrecondition) running the preserve-first predicateover every row, aborting non-zero in dry-run and execute mode alike if any row matches — with a count, sample ids, and an error message naming the manual step that has to run first. A matching row holds its only copy of a legal name in
auth_user.name; rewriting it before that value reachesreal_nameloses the legal name outright, the one unrecoverable failure in this program. The gate makes run order irrelevant rather than trusting an operator to sequence correctly. (IS DISTINCT FROMis reproduced exactly by JS!==on twostring | nullvalues; the docblock says why.)The precondition gate is also the real reason this is a full
jobs/workspace rather than ascripts/one-off — 139 rows alone would not justify it, but a gate standing between a rerun and unrecoverable data loss deserves a testable, reviewable, CI-typechecked entry point.Which is why
.github/workflows/test.ymlgains aType check: auth-user-name-backfillstep (the BS#2009 precedent immediately above it): the roottypecheckexcludesjobs/**andtsup --minifyis transpile-only, so without that step the one job carrying the gate could ship uncompiled-but-green.4.
tests/integration/dj-real-name-sentinel.spec.js(Track 3b)The mechanism that would have caught
a0cd1979. It seeds anauth_userrow shaped exactly like the conflation —real_nameandnameboth holdingSENTINEL-REAL-NAME-93aF— plus a show and flowsheet entries in a fixed 1995 window clear of the other integration fixtures, then whole-body string-matchesJSON.stringify(res.body)acrossGET /flowsheet,/flowsheet/latest,/flowsheet/rangeand/flowsheet/search(including adj:operator query) and asserts the sentinel appears in no response body.Note what it does not test: none of Tracks 2b–2d. It proves the public read surfaces never surface a legal name regardless of how a row got into that shape — which is the actual failure mode, since BS#1286/BS#1393/BS#2281 were every one of them a read path, not a write path. Whole-body matching (rather than a keys-only shape assertion) is what lets it catch fields that do not exist yet.
Every
itcarries a positive control asserting the seeded handle is present, so a passing "no sentinel" assertion cannot be quietly vacuous from the fixture having fallen out of query scope.The sentinel's mechanism was proven, not assumed
A green PII test proves nothing unless it can go red. During implementation the spec was run against a real local stack: 10/10 passing. Then a deliberate leak was injected into the
/flowsheet/rangeshows projection so that the legal name reached the response body — the spec failed, on exactly the assertions it should have — and the leak was reverted, returning the suite to 10/10.That closes the plan's named acceptance criterion: "Sentinel spec passes, and demonstrably fails against a deliberately-leaky mutation (proven once during implementation)." The spec was not re-run for this PR write-up; CI's integration-tests job exercises it against the Docker DB.
Deployment note — read before running anything
The backfill job must NOT be
--executed until the manual preserve-first SQL has run (plan step 2a: copyname → real_namefor rows whose only legal-name copy is inname, as reviewed SQL with the audit SELECT attached). That step is deliberately human-gated and is not automated by this PR — it is the one irreversible action in the program.The precondition gate enforces this mechanically, and will abort the job non-zero rather than destroy anything if the order is wrong. Say it out loud anyway: 2a first, then a dry-run, then
--execute.Merging this PR is safe on its own — the hooks and the provision change take effect on deploy and are forward-only; the job ships as an image that runs when someone runs it.
Relationships
mainprecisely becausenameis still a required provision field there — that failure is this PR's absence, observed live. Its E2E workflow can point itsbackend_refinput attask/auth-user-name-unscrewfor pre-merge verification.DJ_HANDLE. Disjoint files; either order.COMMENT ON COLUMNmigration, dead Cognito spec removal, and therestricted-real-namelint rule. It also carriesplans/dj-name-pii-safeguards.mditself. Note that the rule does not exist on this branch, so nothing here is lint-covered by it yet.flowsheet.dj_namestored-data scrub).Local gate
Run on the rebased branch (
origin/main@d8306903):The
flowsheet-etlsuites are run separately: all 197 tests pass, but the run exits 1 because a module under test setsprocess.exitCode = 1on its refuse-to-run path. Verified pre-existing — the identical command onorigin/main@d8306903, same install, produces the same 197-passing/exit-1 result. This branch touches noflowsheet-etlfile.Two full-suite runs also hit unrelated, non-reproducing route-test flakes (a
socket hang upininternal-slack-moderators.route.test.ts; a 401 inlibrary-artist-card-permissions.route.test.ts) under machine load. Each passes in isolation, and three subsequent full runs were clean at 8386/8386. Neither file is touched here.The integration tier was not stood up locally for this write-up — the sentinel spec was already executed and its failure mode proven during implementation, as described above. CI's integration-tests job runs it.