e0e9ee06 - fix(auth): strip the login signature for callers below admin - #4505
e0e9ee06 - fix(auth): strip the login signature for callers below admin#4505TaprootFreak wants to merge 9 commits into
Conversation
user.signature is a static login credential: for DeFiChain and custodial Lightning the stored value IS what authenticates a sign-in, so whoever reads it can sign in as that user. The column carried no select: false and no @exclude, and there is no global ClassSerializerInterceptor. It therefore travelled out through every admin endpoint that returns an entity reaching it via a relation - reachable from the Support role upwards. Transaction.user is eager, so it also arrived where no relation had been requested, which is why filtering the known endpoints would have been structurally incomplete: the next endpoint returning a transaction would reopen it. Marking the column select: false removes it from every query by default, so it cannot leave through an entity at all. The single remaining read path is UserService.getSignature(), which fetches it explicitly for the comparison in doSignIn. Writing is unaffected. Covered by a metadata assertion that the column is select: false (verified by mutation: removing the option turns that test red) and, against a real Postgres, that findOne omits such a column, that addSelect returns it - the pattern getSignature() relies on - and that raw SQL still sees the value, so this is an ORM-level change and not a data migration.
Prettier collapses the split test title and the query chains. No behavioural change - the five cases assert exactly the same things and pass unchanged.
Both comments overclaimed: they said the value can never leave through an entity, while the same change proves otherwise - an explicit addSelect still loads it, which is exactly what getSignature() does. They now say what holds: select: false keeps the column out of every default selection, including entities returned through relations and eager ones, and explicit widening remains possible and should go through that one method. Adds unit coverage for getSignature() itself. The Postgres cases so far proved the ORM mechanism on a probe entity, so a removed addSelect or a mistyped alias in the real method would have gone unnoticed - and would have made it return undefined for every caller. The new cases pin the addSelect argument and the where clause, the returned value, and that a missing user yields undefined rather than throwing. Also switches the spec to an absolute import as CONTRIBUTING.md requires.
TaprootFreak
changed the title
Cosmetic only; both orderings occur in this repo and no lint rule governs it.
The previous assertion only checked that addSelect had been called with user.signature. That misses a real regression: select() replaces prior selections in TypeORM, so reordering the chain to addSelect(...).select(...) would drop the column, make getSignature() return undefined for everyone, and break the DeFiChain and custodial Lightning sign-ins - while leaving all three tests green. The query-builder mock now models that replacement semantics: select() resets the tracked selection, addSelect() appends. The test asserts the resulting selection contains the column. Verified by mutation: reordering the chain in getSignature() turns exactly this test red.
|
Three review passes across two independent lenses — conformance against The substantive ones all concerned the tests claiming more than they proved:
Two rejections, both measured rather than argued:
Verification on the final commit: |
Replaces the earlier approach. Marking the column select: false kept it out of every response, admins included - but admins are meant to load it normally, so the guarantee has to be role-dependent rather than absolute. The column is loaded as before. A globally registered interceptor removes it from responses whose caller does not satisfy the ADMIN requirement, reusing the hierarchy helper from role.guard.ts so ADMIN and SUPER_ADMIN pass untouched. Solving it centrally matters because the column reaches responses through relations across many admin endpoints, and Transaction.user is eager, so it arrives where no relation was requested - per-endpoint filtering would be incomplete and would be forgotten on the next endpoint. Filtering keys off the entity type, never the property name: other entities carry a field called signature and must not be touched. That property is pinned by its own test and verified by mutation - switching the check to the field name turns exactly that test red. The walk is cycle-safe via a WeakSet, since the entity graph is bidirectional, and a missing request.user counts as non-admin. getSignature() and its call site are gone again; doSignIn reads user.signature as it did before.
TaprootFreak
changed the title
Every existing case constructs the interceptor directly, so none of them would notice if the APP_INTERCEPTOR entry were dropped from AppModule - the protection would be gone with all tests still green. This asserts the wiring itself. Verified by mutation: removing the provider entry turns exactly this test red.
APP_INTERCEPTOR is a string token, not a symbol, so the previous annotation made the comparison provably false and tsc rejected it. The test passed regardless, because Jest transpiles without type checking - the type-check gate caught it.
❌ TypeScript: 1 errors |
Replaces the two any-typed cycle fixtures with a self-referencing interface, and reads the provider list through MODULE_METADATA.PROVIDERS instead of the raw string - the pattern accounting.module.spec.ts already uses. A hardcoded key would break silently if Nest renamed it, while the app stayed correct.
|
The approach in this PR changed midway, so a note on how it got here. It first made the column Five review passes across two independent lenses. Findings that shaped the result:
Three rejections, each measured rather than argued: an import-order claim (the rule governs sorting within a statement, not between statements), a Two known limits, deliberately not addressed here: the global exception filter and Verification: Full disclosure on process: the last commit only changes test typing and swaps a hardcoded metadata key for the constant the repo already uses. I did not run another full double review for it, since the remaining findings had shrunk to that class and another pass would not have added confidence. |
|
The The review job on the current head reports |
1 participant
What changed
user.signatureis loaded as before. A globally registeredSignatureVisibilityInterceptorremoves it from responses whose caller does not satisfy theADMINrequirement, reusing the hierarchy helper fromrole.guard.ts— soADMINandSUPER_ADMINsee the column exactly as they always did, and everyone below no longer sees it at all.Why
The column is a static login credential, not a record of one: for DeFiChain (
auth.service.ts:554-556) and custodial Lightning (:546-551) the stored value is what authenticates a sign-in, and it never expires. Whoever reads it can sign in as that user.It left the system through every endpoint returning an entity that reaches it through a relation — reachable from the Support role upwards.
Filtering those endpoints one by one would be structurally incomplete:
Transaction.useris declaredeager: true(transaction.entity.ts:136), so the relation loads on everyfindOneagainstTransaction, regardless of therelationspassed in. Endpoints carry the column without ever asking for it, and the next endpoint returning a transaction would reopen the path. Solving it in one place closes the current paths and any future one.Design notes
instanceof User), never the property name. Other entities carry a field calledsignature— RealUnit registrations among them — and must not be touched. This is pinned by its own test and verified by mutation: switching the check to a property-name test turns exactly that test red and nothing else.WeakSet. The entity graph is bidirectional, so a naive recursion would not terminate.request.usercounts as non-admin — unauthenticated routes are filtered, not exempted.request.useris populated — the same reasonTfaEnforcementInterceptoris built this way.Impact
Checks
format:check,lint,type-checkcleanuserData.users[]is covered, a non-Userobject keeps its ownsignature, and a cyclic graph terminatesinstanceof Userwith a property-name check turns exactly the non-Usercase red; restored, all seven passScope note
This restricts who sees the column in responses. It does not change who can sign in, and it adds no new endpoint for reading it.