Skip to content

feat(gs): grant staff clearance to the Debug account - #4590

Merged
TaprootFreak merged 5 commits into
developfrom
feat/gs-staff-clearance-debug-account
Aug 2, 2026
Merged

feat(gs): grant staff clearance to the Debug account#4590
TaprootFreak merged 5 commits into
developfrom
feat/gs-staff-clearance-debug-account

Conversation

@Danswar

@Danswar Danswar commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Problem

The staff-clearance rule (#4395#4572) requires, on top of the role, a non-empty user_data.verifiedName for every elevated RoleGuard — every guard whose entry roles all sit in KycGatedRoles (Admin, Debug, Compliance, Support, RealUnit). POST /gs/debug is RoleGuard(Debug) and therefore gated.

The backfill in #4574 covered user data 375162 and the GSheet service account. User data 403938 — the Debug account — was not among them and still answers every elevated endpoint with 403 STAFF_KYC_REQUIRED. Sign-in is unaffected and still reports role=Debug, so the tooling looks healthy right up to the first query; only the query itself is refused.

That 403938 is the right key is verifiable without DB access: the prod-minted JWT for that wallet carries account=403938, user=410490, role=Debug; AuthService.generateUserToken sets account: user.userData.id, and RoleGuard reads exactly that claim. The prod DB itself is unreadable here — the only structured read path is the endpoint this PR unblocks.

Change

One new migration, 1785635000000-BackfillDebugStaffVerifiedName.js, plus its spec:

  • PRD-guarded, no-op elsewhere.
  • The personal name is read from the deployment variable STAFF_VERIFIED_NAME_403938 and never appears in this repository. The variable is mandatory on PRD, so a missing value fails loudly instead of recording a silent no-op migration.
  • The update precondition is the exact negation of the closing assertion — both use the same BlankChars-aware BTRIM predicate. That matters: a precondition of verifiedName IS NULL against a non-blank postcondition leaves a present-but-blank name (a lone tab, a non-breaking space) as a state the migration refuses to repair and then refuses to accept.
  • Coupled to a durable audit entry in log (system=User, subsystem=StaffVerifiedNameBackfill), written in the same statement — the audit cannot go missing while the update lands. The entry carries an action of backfilled or keptExistingName.
  • Idempotent: a re-run after a successful backfill updates nothing and appends no audit row.
  • down() is a deliberate no-op, matching 019fbcf7 - Backfill staff verified names on PRD #4574 and 1785500000000-ClearDevUserSignatures.js: a granted clearance is not auto-revoked by an unrelated rollback.

Deliberate deviation from #4574. The closing assertion checks the clearance predicate itself rather than equality with the supplied value. #4574 asserts verifiedName = $1, which would throw — and therefore crash the boot — if an identity-verified path had written a different but perfectly valid name in the meantime. Asserting the goal state keeps the fail-loud behaviour for the case that matters (the account is still not cleared) without letting a spelling difference take production down. That divergence is not silent: it is recorded as a keptExistingName audit entry, so the deployed state cannot differ from the reviewed one without a trace.

The BlankChars set is duplicated rather than imported because migrations are plain JS executed by TypeORM and cannot pull in application sources. The spec pins the copy behaviourally: it derives the authoritative set from the runtime — every character String.prototype.trim() strips, which is what BlankChars is defined as — and requires the migration to repair a name built from all of them. A copy that lost a code point fails that test.

Deployment order — blocking

STAFF_VERIFIED_NAME_403938 must exist in the production environment before this migration reaches main. The variable is prepared separately and has to be released to production first.

Two things sharpen this beyond "the migration throws":

  • migrationsTransactionMode is unset in src/config/config.ts, so TypeORM's default all applies. Either boot-fatal throw — the missing variable or a failed assertion — rolls back the whole release's migration batch and fails DataSource.initialize(). The blast radius is the API, not this migration.
  • auto-release-pr.yaml keeps a developmain release PR open continuously: it checks for an open one first and creates it only when none exists, so every develop push either opens the release PR or lands on the one already open. The practical gate is therefore the merge to develop, not the merge to main.

ENVIRONMENT !== 'prd' returns before the variable is read, so no other environment needs it.

Scope note for the release

verifiedName is not a clearance-only field. It feeds AML (aml-helper.service.ts NO_VERIFIED_NAME, name matching in aml.service.ts), the SEPA beneficiary name on sell payouts, duplicate-account signals and payment-link display; the identity path writes it as `${firstname} ${surname}`, and kyc.service.ts raises FIRST_NAME_NOT_MATCHING_VERIFIED_NAME / LAST_NAME_NOT_MATCHING_VERIFIED_NAME on the next ident step unless a token of each matches. Util.includesSameName matches per token, so the check is tolerant, but the deployed value should still be the account's actual legal first and last name. This reach is inherent to the mechanism #4572 chose, not introduced here — worth stating explicitly because the reviewer approving the value is approving an identity field, not just a feature flag.

For this account that reach is inert, and the account cannot be inspected to confirm it either way — both facts follow from the same property. Debug sits outside the account hierarchy: it appears in neither additionalRoles[UserRole.ACCOUNT] nor additionalRoles[UserRole.USER], so hasRoleAccess(ACCOUNT, DEBUG) is false and the role satisfies Debug-gated endpoints only. Confirmed against prod: GET /v2/user, /v2/user/profile, /v1/user and /v1/kyc all answer 403 Forbidden resource for this wallet — the generic role refusal, not STAFF_KYC_REQUIRED, so it is the role check failing rather than the clearance gate.

An account that cannot reach any KYC or profile endpoint also never runs the ident step whose FIRST_NAME_NOT_MATCHING_VERIFIED_NAME / LAST_NAME_NOT_MATCHING_VERIFIED_NAME checks are the reason the value's exactness would matter, and never initiates the sell payout that would use it as a beneficiary name. The value therefore serves the same role here as the existing entry does for the account #4574 covered: an operator-reviewed identification token, reviewed in the PR rather than derived from an identity record. Whether firstname/surname are populated on the row cannot be read with this wallet by design.

Verification

backfill-debug-staff-verified-name.migration.spec.ts, in the shape of the #4574 spec: mocked-queryRunner cases plus a MIGRATION_TEST_PG-gated real-Postgres suite, so this runs in CI on the postgres:16 service the three test shards already provide — not in a throwaway container. 20/20 pass locally against postgres:16.

Covered:

Case Expectation
ENVIRONMENT dev/loc/staging/unset no query issued at all
variable missing / empty / whitespace throws before any SQL
PRD, fresh NULL one update, one backfilled audit row, name never inlined in the SQL
second run no update, no second audit row
present-but-blank name (tab, spaces, NBSP, BOM) repaired, previous value recorded in the audit
name built from every character trim() strips repaired — pins the duplicated BlankChars copy
pre-existing divergent name left untouched, keptExistingName audit row, migration succeeds
target row absent throws, transaction rolls back, no audit row
audit insert suppressed by a trigger nothing changes, throws

Two of those rows are mutation-verified rather than merely green:

  • dropping "id" = 403938 AND from the closing assertion — which in production would count every cleared account and throw — takes the suite to 10 failures. (Before this was pinned, that mutant passed all 20 tests; the fixture now gives the non-target account a cleared name.)
  • reverting the precondition to verifiedName IS NULL fails the blank-name cases.

Repository checks: migration-psql-check.spec.ts passes (Array.of keeps the parameter list out of the bracket-quoting pattern); BLANK_CHARS compared programmatically against BlankChars — identical, 25 code points; node --check, eslint, prettier --check and tsc --noEmit clean.

PR completeness

  1. Migration — this is the migration; no entity or column change.
  2. Environment/Infrastructure — one new deployment variable, prepared separately, see the deployment-order section. Its live presence cannot be confirmed from this repo and is the gate before merging to develop.
  3. Service updates — none; no DTO or interface change.
  4. Frontend synchronization — none; no API contract change.
  5. Cron job inventory — no @DfxCron change.

Doc correction included here

docs/staff-kyc-clearance.md is the canonical instruction for this exact task, and it was pointing at the bug:

  • Step 1 prescribed WHERE id = … AND "verifiedName" IS NULL — the precondition this branch removes, because it is not the negation of the clearance predicate. The next staff-clearance migration written from that recipe would reproduce the boot-fatal gap.
  • Step 1 said nothing about the assertion, so an author following it would copy the equality assertion from 019fbcf7 - Backfill staff verified names on PRD #4574 — which throws over a spelling difference. It now mandates the clearance-predicate assertion and gives both failure modes of the equality form.
  • The negation rule is now NULL-total: taken literally against the printed assertion it yielded BTRIM("verifiedName", <BlankChars>) = '' without the COALESCE, under which a NULL name is neither repaired nor accepted — the same boot-fatal shape, reintroduced by the fix for it.
  • Step 1 never required the audit coupling, though both shipped backfills gate the write on EXISTS (SELECT 1 FROM "audit") and CONTRIBUTING treats unaudited mutation of a PII column as blocking.
  • Step 2 said the migration "applies it only when present", while both this migration and 019fbcf7 - Backfill staff verified names on PRD #4574 hard-throw on PRD when the variable is absent — a reader would misjudge the deploy risk. It now also names the merge to develop, not to main, as the point by which the variable must be live.

Doc-only edit; it touches no migration, so the immutability gate is unaffected.

Danswar added 5 commits August 1, 2026 22:43
The staff-clearance rule (#4395 -> #4572) requires a non-empty user_data.verifiedName
on top of the role for every elevated RoleGuard, including the Debug gate on
POST /gs/debug. The backfill in #4574 covered two accounts; user data 403938 was not
among them and still answers every elevated endpoint with STAFF_KYC_REQUIRED.

Adds a PRD-only, idempotent, audited migration in the shape of #4574. The personal name
is read from the deployment variable STAFF_VERIFIED_NAME_403938 and never appears in
this repository.

The closing assertion checks the clearance predicate itself — the BlankChars-aware BTRIM
that StaffKycClearanceService uses — instead of equality with the supplied value. Should
an identity-verified path have written a different, correct name in the meantime, that
account is cleared and the deploy must not fail over the spelling.
…ertion

The update repaired only a NULL verifiedName while the closing assertion demanded a
non-blank one, so a present-but-blank name (a lone tab, a non-breaking space) was a state
the migration refused to repair and then refused to accept. Because
migrationsTransactionMode defaults to 'all', that throw rolls back the whole release's
migration batch and fails DataSource.initialize() — a PRD crash-loop, not a failed tool.

The precondition now uses the same BTRIM predicate as the assertion, so the two are
complementary by construction and the blank case becomes a repair.

Keeping a divergent existing name is no longer silent either: the audit entry records the
outcome, so the deployed state cannot differ from the reviewed one without a trace. A
re-run after a successful backfill stays a true no-op.

Adds the migration spec the two preceding data migrations ship, including the blank-name
cases that were the defect, so the verification runs in CI on the postgres:16 service
instead of living in a throwaway container.
…hars

The spec left the non-target account's verifiedName NULL in every database case, so
`count(*) = 1` held whether or not the closing assertion was scoped to the account.
Dropping `"id" = 403938 AND` from the postcondition passed all 20 tests while in
production it would count every cleared account and throw. The fixture now gives that
account a cleared name, which kills the mutant with 10 failures.

The blank-name cases covered four hand-picked characters, so the migration's copy of
BlankChars could lose any of the other 21 code points unnoticed. The set is now derived
from the runtime — every character String.prototype.trim() strips, which is what
BlankChars is defined as — and the migration must repair a name built from all of them.

Also drops a vacuous assertion: the log count was read after the rollback, where it holds
regardless of what the migration wrote.

docs/staff-kyc-clearance.md still prescribed the `verifiedName IS NULL` precondition this
branch removed, so the next author following it would reproduce the boot-fatal gap. The
recipe now states the exact-negation requirement, and step 2 no longer says the value is
applied "only when present" when PRD in fact throws without it.
…recipe requires

The migration's JSDoc still said the update "only touches a still-null verifiedName",
carried over from the migration this one is modelled on and false since the precondition
became the negation of the closing assertion. This has to be fixed before the merge:
api-migration-check.yaml allows comment-only edits to an existing migration by stripping
whole lines starting with `//`, and JSDoc lines start with `*`, so afterwards the sentence
could never be corrected.

The runbook demanded the precondition be the exact negation of the closing assertion
without saying what that assertion must be. The only precedent in the repo asserts
equality with the supplied name, so an author following both would write an equality
assertion — which throws when an identity-verified path wrote a different but valid name,
and under the 'all' transaction mode takes the release's whole migration batch and the
boot with it. The recipe now states the clearance-predicate assertion explicitly, and
names `develop` rather than `main` as the last point where the deploy order can still be
arranged.

Two spec corrections: the BlankChars pin claimed a drifted copy would fail the migration's
own assertion, when in fact the postcondition shares the drifted constant and reports
success — the drift is silent, which is precisely why the test asserts the repaired state.
And a test name still promised a post-rollback assertion that no longer exists. Adds the
sanity floor on the derived character set that the sibling spec already carries.
… coupling

Taken literally, "the exact negation of that assertion" produced
`BTRIM("verifiedName", <BlankChars>) = ''` — without the COALESCE that the printed
precondition one paragraph earlier does carry. That is not the negation over NULL: a NULL
name yields NULL rather than true, so the ordinary un-backfilled account is neither
repaired nor accepted, which is the same boot-fatal shape the paragraph exists to prevent.
Verified as a mutant against the shipped SQL: three tests fail with the postcondition
error. The instruction now spells out both forms and says which is wrong.

The ban on equality assertions was justified with only one of its two failure modes, and
that one is unreachable once the negation rule is applied — inviting the conclusion that
the ban is redundant. Both branches are now stated: against a blankness precondition an
equality assertion throws over a spelling difference, against its own negation it silently
overwrites an identity-verified name.

The recipe also never mentioned the audit coupling, though both shipped backfills gate the
write on EXISTS (SELECT 1 FROM "audit") and CONTRIBUTING treats unaudited mutation of a PII
column as blocking. An author following only the runbook produced an unaudited migration.

Corrects the release-PR mechanism too: auto-release-pr.yaml checks for an open PR first and
creates one only when none exists, so it keeps a release PR open continuously rather than
opening one per push. The conclusion — develop is the last point where the deploy order can
be arranged — is unchanged.
@Danswar

Danswar commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator Author

Four review passes on this branch before it settled. What each one caught, since the later fixes are only legible against them:

  1. The update repaired only a NULL verifiedName while the closing assertion demanded a non-blank one, leaving a present-but-blank name as a state the migration would neither repair nor accept — boot-fatal, since migrationsTransactionMode defaults to all and the throw takes the whole release's batch. Also: no committed spec, so the verification could not be re-run by a reviewer.
  2. The spec's fixture left the non-target account's name NULL in every database case, so the closing assertion's account scoping was entirely unpinned — dropping "id" = 403938 AND passed all 20 tests while in production it would count every cleared account and throw. Fixed and mutation-confirmed at 10 failures. The BlankChars copy was likewise pinned only against four hand-picked characters; it is now pinned behaviourally against the runtime-derived set.
  3. The migration's JSDoc still described the old precondition, which had to be corrected before merge: the immutability gate permits comment-only edits by stripping // lines, and JSDoc lines start with *, so the sentence would have been frozen wrong. A spec comment also claimed the migration self-detects BlankChars drift; it does not — the postcondition shares the drifted constant and reports success.
  4. The runbook recipe, read literally, reproduced the original defect: "the exact negation of that assertion" yields the predicate without COALESCE, under which NULL is neither repaired nor accepted. It also never required the audit coupling that CONTRIBUTING treats as blocking for a PII column.

Being straight about where this stopped: pass 4's findings are fixed, but the commit fixing them has not itself been through a fifth pass. The code and SQL have been unchanged and mutation-verified since the pass-1 fix — passes 3 and 4 found only comment and documentation defects — so I stopped rather than iterate on prose indefinitely. Reviewers should read the final commit as unreviewed.

This stays a draft deliberately. The deployment variable it depends on is not yet live in production, and merging to develop puts a boot-fatal migration one release-PR merge from production. It should go ready only once that config change is merged and released.

@TaprootFreak
TaprootFreak marked this pull request as ready for review August 2, 2026 13:38
@TaprootFreak
TaprootFreak merged commit 674acbc into develop Aug 2, 2026
13 checks passed
@TaprootFreak
TaprootFreak deleted the feat/gs-staff-clearance-debug-account branch August 2, 2026 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants