Release: develop -> main - #4404
Merged
Merged
Conversation
…4385) * Guard every already-covered file with a repo-wide coverage ratchet The only coverage gate so far was the Frick one: seven files, 0.4% of production code. Nothing stopped a change from silently dropping coverage anywhere else. Measure the whole suite once and pin every production file that already reaches 100% on branches, functions, lines and statements - 401 of 1593 files. Dropping below 100 on any of them now fails CI. This is a regression gate, not a claim about test quality. Overall coverage is 57.7% of statements and 39.5% of branches; the pinned set is simply what is complete today. Of the 401, 218 carry real logic and 183 are declarative (modules, constants) - kept as separate blocks in the config so the count is not mistaken for test depth. The declarative ones are pinned on purpose: adding logic to one of them now requires tests. Two properties of the measurement matter and are documented so the list stays reproducible: - Full compilation via tsconfig.coverage.json, as the Frick gate already does. Transpile-only emit reports phantom uncovered branches on injected constructors, which would understate dozens of files. - The whole suite runs, because files are frequently covered by specs other than their own. The Frick gate stays untouched and separate: running only its seven specs, it proves those specs alone reach 100%, which a whole-suite run cannot assert. Verified locally: the gate passes with no threshold violation (299/306 suites, 5170 tests), and parallel execution yields byte-identical coverage totals to a --runInBand reference run, which is why the CI script does not serialise. docs/coverage-gate.md records the measured gaps, including the 29 files already at >=90% on all four metrics as the next batch. * Exclude test scaffolding from the ratchet, correct the inventory Two files under shared/utils are test support, not production code: test.util.ts (60 importers) and test.shared.module.ts (28), every one of them a *.spec.ts. Both were collected and pinned, which made two things wrong. It coupled a production gate to test scaffolding: extending a test helper without testing the helper itself would have turned CI red although no production coverage moved. And it made the documented inventory inaccurate, since both were counted as production files. Exclude them from collectCoverageFrom and drop their threshold entries. Pinned files 401 -> 399 (217 with logic, 182 declarative). Correct the remaining numbers against a fresh verified run: 1,591 measured files, statements 57.66%, branches 39.46%, functions 31.84%. The collection glob matches 1,643 files; the 52 that never appear in the report are type-only files that compile to no executable statements, so they cannot be pinned. Also document what happens when a pinned file is deleted or renamed - jest exits 1 with "Coverage data ... was not found", so the gate cannot silently stop protecting a file - and fix the stale "three job groups" comment in the workflow, which this change turns into four. * Make the documented inventory and timing claims verifiable Two documentation defects, both about claims a reader cannot check. The inventory said all 52 uncollected files are type-only. Two of them are not: p2b.service.ts and payin/enums/index.ts consist entirely of commented-out code. The counts and the conclusion were right, but lumping dead code in with type declarations hides a second, different reason a file cannot be measured. Both are pre-existing and left untouched; removing them is separate cleanup. The parallelism claim rested on a one-off measurement ("a --runInBand run produced identical totals"), which nothing in the repo lets a maintainer reproduce. Replace it with the structural reason it holds: istanbul merges per-worker counters additively, so scheduling cannot turn a covered statement into an uncovered one. The measurement stays as corroboration, not as the argument. Likewise drop the precise "~16 min" from the workflow comment and date the approximation in the doc, so neither goes stale silently. * Keep only repo-verifiable claims in the coverage documentation Dating a measurement records when someone asserted it, not whether it holds. The doc still carried two claims a maintainer cannot check from the repository: that a serial reference run once produced identical totals, and that this job is the slowest of the four at roughly a quarter hour. Drop both. What remains is derivable from the repository itself: coverage merging is additive across workers, so parallelism cannot lower a per-file percentage, and the gate runs the whole suite under full compilation while the test job shards three ways and the Frick gate runs seven specs. That explains the runtime without asserting a number nobody can reproduce. Measured values stay in the pull request, where they belong - a PR reports what its author ran, with the CI run alongside it. Durable documentation is read when nobody remembers who measured what. * Assert parallel fee fetching directly instead of by the clock The test named "should execute in parallel" measured wall-clock duration and required it to stay under 100 ms while five mocked calls each slept 10 ms. That assertion answered the wrong question in both directions. It was too strict under load: scheduling delay alone pushed it to 181 ms on a busy machine, failing a test whose subject was correct. It was also too weak to catch what it existed for. Sequential execution of five 10 ms calls totals about 50 ms, comfortably under the limit. Verified by replacing Promise.all in BitcoinBasedFeeService.getTxFeeRates with a sequential loop: the old test stayed green, 27 passed. It could only ever fail because of machine speed, never because of the defect it was written to detect. Assert the property itself instead. The mock now counts how many calls are in flight simultaneously and the test requires that maximum to equal the number of txids. Concurrent execution reaches five; a sequential implementation never exceeds one. Against the same sequential mutation the new test fails with "Expected: 5, Received: 1", so it detects the regression the old one missed, and it does not depend on timing at all. Coverage is unchanged: the pinned BitcoinBasedFeeService still reports 100% on all four metrics under the full gate run. * Follow the section-comment convention, drop an unverifiable provenance claim CONTRIBUTING requires the `// --- NAME --- //` form for major sections; the two principal blocks of the gate config used a different decorative style. The doc also claimed the pinned list "was derived from a run without" Postgres. That is a statement about a past measurement, and nothing checked in records it. State the current, checkable behaviour instead: no pinned file belongs to the migration suites MIGRATION_TEST_PG enables, and enabling further suites can only raise coverage. * Drop the provenance claim from the workflow comment too The same unverifiable statement removed from docs/coverage-gate.md survived in the workflow: "matching the measurement the pinned list was derived from". Nothing checked in records that historical environment, and the workflow and the documentation should not make different-strength claims about the same fact. State only what a reader can check: no pinned file belongs to the migration suites that need Postgres. * Generate the pinned thresholds from two named lists The threshold block spelled out the same four-metric object for each of the 399 pinned files, which made the config 2037 lines and turned every rename into a five-line diff. The paths now live in PINNED_LOGIC and PINNED_DECLARATIVE and coverageThreshold is built from them, so the two groups are data rather than a comment, and adding a file is one line. The pinned set itself is unchanged: same 399 paths, same order, same grouping. Also emit an lcov report, which the workflow uploads when the gate fails. * Bound the ratchet job and upload its report on failure The job had no timeout and would have held a runner for the default six hours if it hung. It is also the most expensive job in the workflow: an unsharded whole-suite run under full compilation. When the gate fails, jest names the file and the metric but not the uncovered lines, which left a local rerun of roughly fifteen minutes as the only way to find out what to fix. The lcov report is now uploaded as an artifact instead. Also run the gate with --silent, matching the test script. An unsilenced whole-suite run buries the threshold message under the console output of every suite. * Release the mocked calls when the parallelism assertion fails The assertion sits between the point where the mocked getMempoolEntry calls are parked and the point where they are released. On failure the release never ran, leaving five pending promises and a getTxFeeRates call that could not settle. Moving the release into a finally block keeps the failure clean. * Document what the ratchet does not protect Three gaps a reader would otherwise find the hard way: - A newly added production file with no coverage passes the gate. The list only protects what is already on it, and it grows by hand. - When a purely declarative file first gains executable logic, the function metric moves from 0/0 to 0/N and the gate turns red. Tests stay the preferred fix, but unpinning that one file is an allowed outcome when the pull request says so. For the logic-carrying files the rule is unchanged. - The lcov report is uploaded as an artifact when the gate fails. Also describe the two pinned-path arrays, since that is where a file is added now. * Run the coverage ratchet on a self-hosted runner The gate executes the whole suite under full TypeScript compilation and is CPU-bound. A hosted runner gives a public repository four vCPUs, so Jest defaults to three workers: the gate took 13.8 minutes and was the sole reason a PR run took 15.7 minutes instead of 4.8. The ceiling for a full run is 5. Sharding it across hosted runners was not a way out. At 11-12 jobs per push the repository already reaches the account's 20-job concurrency limit whenever two runs overlap - measured at 37-52 seconds of queueing against 2-3 seconds otherwise - so more parallelism there buys queueing rather than speed. The self-hosted pool has far more cores and does not count against that limit. maxWorkers is set on the workflow step rather than in the npm script, so the script keeps Jest's own default for anyone running it locally. * Drop the npm cache from the self-hosted gate job Measured on the first self-hosted run: the gate itself dropped from 13.8 to 6.8 minutes, but the post-run cache upload from setup-node took 5.3 minutes and ate most of that gain. The cache is pointless on a self-hosted runner. Its ~/.npm survives between jobs, so restoring adds nothing, and saving pays to upload a cache that will never be read. The hosted jobs keep it - there the runner is thrown away after every job. * Give the ratchet more workers At 16 workers the gate ran 3.8 minutes and the whole PR run landed at 4.9 - six seconds under the five-minute ceiling. That is not a margin, it is a coincidence: one more test case would break it. The runner has considerably more cores than 16. Tracking its performance cores leaves the rest of the machine to whatever else shares it, and should turn those six seconds into something that survives the suite growing. * Go back to 16 workers, which measured best Raising the worker count backfired. At 20 the gate took 8.4 minutes against 4.8 at 16, and the whole run missed the five-minute ceiling. Host CPU tells the story: it stayed flat at roughly 53% mean and 74% peak in both runs. The extra workers did not draw more compute, they contended - under full compilation each worker holds its own TypeScript program. Nothing else ran on the machine during either measurement. The number is now documented as measured, with the counter-evidence, so the next person tempted to raise it re-measures first. * Document where the gate runs and what was measured there The write-up still described the gate as an ordinary hosted job. It now runs on a self-hosted runner, and three of its settings only make sense with the measurements that produced them: - maxWorkers is 16 because 20 was slower - 8.4 min against 4.8, with host CPU flat at ~53% either way. The workers contend rather than parallelise. - That job alone omits the npm cache. On a persistent runner the restore gains nothing and the save uploads a cache nobody reads, at 5.3 min per run. - A cold runner reports roughly double the steady-state runtime, so a first measurement is not a result. Also records why sharding across hosted runners was rejected, and that the margin against the five-minute ceiling is thin - the sharded test job at ~4.4 min is close behind. * Serialise the ratchet so two runs cannot collide Two gate jobs running at once on the same machine do not share it, they block each other. Measured: two runs started four seconds apart both took 16.6 min, against 1.5 min for a single run on the same configuration. Nothing else was on the machine. Waiting is the cheaper outcome by a wide margin, so the job now takes a concurrency group. queue: max keeps additional runs pending rather than cancelling all but the newest - without it a third PR would turn an already-waiting run into a cancelled check, which reads as a failure. This also makes the runner-side slot count a capacity decision rather than a correctness one: however many slots exist, at most one gate runs at a time. * Try fewer workers, since cores were never the limit At 16 workers the gate swung between 1.5 and 5.4 min across otherwise identical runs, which is too unstable to sit under a five-minute ceiling. Host CPU explains why more workers did not help and points the other way: it never went above roughly 70% in any run - not even when two jobs put 32 workers on 28 cores and both took 16.6 min. The workers were never waiting for cores. Under full compilation each holds its own TypeScript program, and how much memory is free on that machine varies with what else is running. So this tries the opposite direction. Note the target is not the fastest possible gate: the sharded test job takes about 4.4 min, so anything below that stops being the bottleneck. Stability matters more than the best case. * Record the measurements the gate's settings rest on Every number in that section was wrong at some point during this work, so it now says what was measured and what it replaced. The counter-intuitive one is the worker count. Fewer is faster: 8 workers run the gate in 1.5 min, three consecutive runs with no variation, where 16 swung between 1.5 and 5.4 and 20 took 8.4. Host CPU never exceeded ~70% in any run, so cores were never the constraint and adding workers could not help. Also recorded: why the job carries a concurrency group (two gates at once take 16.6 min each against 1.5 alone), why it alone omits the npm cache (the post-run upload cost more than the whole move saved), and that a freshly registered runner reports roughly double until its cache is warm. The last part matters most for reading a slow run later: the gate is no longer the bottleneck. Full runs sit at 4.9-5.0 min and are decided by the sharded test job at ~4.2 min. Tightening the gate further buys nothing, and a run that waits behind another PR's gate carries that wait in its total - three queued runs measured 5.0, 4.9 and 7.6 min with an identical 1.5 min gate. * Keep fork pull requests off the self-hosted runner A self-hosted runner executes both the workflow and the code of the PR head, and this is a public repository - anyone able to open a fork PR could otherwise run arbitrary code on it. The repository also requires approval for all outside contributors, but that is a settings- level control someone can change; this guard sits in the reviewed diff. Fork PRs fall back to a hosted runner rather than being skipped. A skipped check counts as passing, so an `if:` on the job would have let a fork PR bypass the gate altogether - the opposite of what a ratchet is for. They run the same gate, just slower, with the worker count lowered to match a four-vCPU runner. * Follow the repo's script and section-comment conventions Two conventions the new files did not follow: The only comparable script is `test:frick:cov`, so the scoped coverage script is `test:gate:cov`, not `test:cov:gate`. CONTRIBUTING asks for `// --- NAME --- //` on major sections. The two pinned-path arrays are the file's main sections and had plain comments - an earlier commit had introduced the right style, and the refactor into arrays lost it again. * Correct two claims that do not survive checking The inventory called two files entirely commented-out. One of them, subdomains/supporting/payin/enums/index.ts, is empty - zero bytes, no code at all, commented or otherwise. The conclusion holds either way, but a document whose own stated goal is to keep only repo-verifiable claims should not carry one that fails the check. The test comment promised the pending getTxFeeRates call cannot outlive the test. That holds for the two implementations in question, not in general: a partially parallel version would park calls the finally block never sees. Narrowed to what is actually guaranteed. * Scope the concurrency group to the runner it protects The group existed because two gates on the same self-hosted machine block each other. Since fork pull requests moved to a throwaway hosted runner, they share nothing with an internal run - but they still sat in the same queue, so either could wait on the other for no physical reason. The group now carries the same condition as runs-on; fork runs get one of their own. * Describe the gate's runner as it actually resolves The document still quoted a fixed runs-on value and said nothing about fork pull requests falling back to a hosted runner with fewer workers. The workflow comments explain it, the document did not - and this document exists precisely to be the checkable account of how the gate runs.
github-actions
Bot
requested review from
TaprootFreak and
davidleomay
as code owners
July 27, 2026 11:48
…s empty (#3874) * fix(auth): reject custodial Lightning sign-in when stored signature is empty verifySignature returned `!dbSignature || signature === dbSignature` for the custodial-Lightning comparison branch. On sign-in, an account whose stored signature is empty/null (legacy accounts — see the 'temporary code to update empty signatures' self-heal in doSignIn) would authenticate with ANY signature matching the 140-146 alnum shape, then doSignIn persists the attacker's signature → permanent account takeover. Reachable via the public /auth/signIn (isCustodial defaults to false there, the regex is trivially satisfiable). A stored empty signature must never authenticate. Thread an isSignUp flag so sign-up still establishes the first signature (nothing stored yet) while sign-in requires an exact match against a non-empty stored signature — aligning Lightning with the DeFiChain branch right below, which already fails closed on an empty stored signature. * fix(auth): drop dead sign-in signature self-heal that would resurrect the Lightning empty-signature bypass The `else if (!user.signature)` branch persisted the incoming signature whenever the stored one was empty. That was the exact vector the previous commit closes: before the fix it let an attacker's signature become the account credential; after the fix, `verifySignature` returns false for empty stored signatures on Lightning and DeFiChain sign-ins, so the branch is unreachable for those chains. For crypto-verified chains it still ran, but `user.signature` is not read anywhere else in the codebase — leaving it null has no downstream effect. Removing keeps the codebase honest about the new invariant "an empty stored signature must never authenticate": no re-hydration path, no way for the takeover vector to reappear if `verifySignature` is later refactored. * chore(auth): drop the now-unused UserRepository injection from AuthService The prior commit removed the only call site (this.userRepo.update in doSignIn). The constructor param and its import were only there to serve that self-heal, so this trims the DI surface without functional change. --------- Co-authored-by: David May <david.leo.may@gmail.com>
* feat(custody): grant, change and revoke access to a Safe Until now there was no way to give another person access to a Safe. The data model for it existed - custody_account_access with a Read/Write level, and an access guard that evaluates it - but nothing ever wrote a grant except the owner's own entry created alongside a new account. Across the whole production data set every single grant is the account owner's own; the sharing feature was never usable. Adds three endpoints on the account resource: create a grant, change its level and revoke it. The target person is resolved by e-mail, matched case-insensitively because signed authorisation documents are inconsistent in capitalisation. Granting is restricted to the account owner: holding a write grant lets you operate the Safe, but handing out rights is an act of ownership, so that is enforced separately rather than relying on the write guard. Accounts that still run in legacy mode cannot hold a grant at all, because a legacy Safe has no account row for one to point at. Rather than migrating every existing custody customer up front, the account is materialised at the moment its owner first grants access: the account is created, the existing balances and orders are attached to it, and the grant is written - all in one transaction, so either the whole step took effect or nothing did. A repeated call cannot produce a second account. Verified against a local instance: granting on a legacy Safe creates exactly one account, attaches all balances and orders to it, and writes the grant; a repeated grant for the same person is rejected with a conflict and leaves the account count unchanged; an e-mail in different capitalisation resolves to the same person. * fix(custody): keep access-grant changes reconstructible and close review gaps The binding rule in CONTRIBUTING is that no mutation may destroy a previous value: an investigator has to be able to reconstruct what a row looked like before, and when it changed, from the database alone. Changing an access level overwrote it in place and revoking deleted the row outright. For access rights that is the worst place to lose history, because who held which access and when is exactly what an audit asks about. Grants now supersede instead of overwrite. A level change deactivates the existing row and inserts a new active one; a revocation deactivates. Every read that decides whether somebody has access considers active rows only. The migration adds the two columns and replaces the unique index on (accountId, userDataId) with a partial one that applies to active rows only - without that, an access revoked once could never be granted again. Further defects found in review and fixed here: - The legacy path accepted the pseudo id even when the owner already had real accounts, and then granted access on an arbitrary one while the legacy holdings stayed unmaterialised. A legacy Safe is the absence of an owned account, so that case is now rejected. - A user's own legacy Safe disappeared from the account list as soon as somebody granted them access to a different Safe, because the fallback only applied to an entirely empty result. - Materialisation reparented balances and orders unconditionally; it now only claims rows that are not yet assigned, and it associates the custody users themselves. - The recipient was resolved before the caller's ownership was checked, which let an outsider tell registered e-mail addresses from unknown ones by the error. - The owner's own grant could be edited or removed, leaving the access list claiming something different from what authorisation actually does. - A concurrent duplicate grant surfaced as a 500 instead of a conflict, a malformed route parameter reached the database as NaN, and a non-string e-mail threw before validation could reject it. Verified against a local instance: changing a level keeps the previous row intact and deactivated with a timestamp, revoking deactivates rather than deletes, granting again afterwards succeeds, editing the owner's grant is refused, and malformed input is answered with 400 instead of 500. * fix(custody): serialise grant transitions and make the rollback possible Two defects introduced by the history model itself. A revocation could report success while the access survived. Update and revoke both read the active grant before mutating, without a lock, so a concurrent pair interleaved: the update deactivated the row and inserted its replacement, the revoke then deactivated its own stale copy and returned success - leaving the replacement active. The caller was told access was withdrawn while the grantee still had it, which for access rights is the worst way to fail. Both operations now run in a transaction and take a pessimistic lock on the grant, so exactly one transition wins and the loser gets an explicit error instead of a false success. The migration could not be rolled back. After any level change or revoke-and-regrant there are several rows per (accountId, userDataId), so recreating the original full unique index failed. The down path now consolidates first - keeping the active row per pair, or the most recent one where none is active - before restoring the old schema. That is lossy by nature and says so. Also: grants are filtered to active rows in SQL rather than loading the full history and filtering in memory, on paths that run on every authorisation; the deactivation transition moved onto the entity instead of being written out twice; and the migration is Prettier-formatted. Verified against a local instance: a concurrent update and revoke on the same grant end with the revoke succeeding, the update answering 404, and no active grant left - before the fix the grantee would have kept access. * fix(custody): let new rows inherit the account of their user Materialising a legacy Safe attached the balances, orders and custody users that existed at that moment, but everything created afterwards still carried no account. The Safe therefore started drifting apart again right after it was materialised - the very split the step was meant to end. Orders and balances now take the account from the user they belong to, and a new custody user inherits it from an existing custody sibling. Users still in legacy mode carry no account, so those rows keep a null reference exactly as before and nothing changes for anyone who never materialised a Safe. Also replaces parseInt with the unary conversion the repository requires. * fix(custody): resolve the account through a dependency-light service Putting the account resolution on CustodyAccountService closed a dependency cycle and the application stopped booting: Nest could no longer resolve UserDataService for that service once CustodyService and CustodyOrderService injected it. The resolution only needs to read a foreign-key column, so it moves into its own provider that depends on nothing but the repository it queries. That removes the cycle structurally instead of hiding it behind forwardRef. The guarantee from the previous round is unchanged: the account is read from the database by user id, never from a possibly unloaded user.custodyAccount relation, and all three creation paths - order, balance and new custody user - go through the same helper, so a later caller cannot silently reintroduce null accounts. A non-numeric foreign key fails closed rather than being treated as legacy. * fix(custody): serialise account resolution with legacy materialisation Materialisation attaches the rows that exist at that moment, once. Resolving the account for a new row ran outside that lock, so a creation could read a null account, let materialisation finish its sweep, and then insert a row that stays outside the Safe forever - the sweep does not run again and the legacy entry point is gone afterwards. The window is narrow but the damage does not heal. Order, balance and custody-user creation now resolve and insert inside a transaction holding the same owner-scoped advisory lock materialisation uses, with the account read after the lock is taken. Materialisation selects the custody users to attach inside its own transaction instead of from a collection loaded earlier, so it also sees rows created just before it. Verified against a local instance: materialisation attaches everything as before, orders created afterwards carry the account, and two concurrent order creations both succeed without deadlocking. * fix(custody): keep the user insert out of the advisory lock Wrapping custody-user creation in the lock transaction meant calling createUser() inside it, and that service persists through its own global repositories. The outer transaction held one pooled connection while the inner call asked for a second, so ten concurrent signups could exhaust the default pool and hang; the insert also escaped a rollback of the lock transaction because it ran on another connection. Preparation and the user insert now happen before the lock. Immediately afterwards a short locked step resolves the account and assigns it, using only the manager it owns and touching the row only while its account is still null. Ancillary signup work stays outside. No HTTP, blockchain or mail call runs under the lock. This narrows the invariant rather than eliminating it: if the process dies between the insert and the assignment, and materialisation has already run its one-time sweep, that user can remain without an account. The sweep still picks up users created before it, and the assignment still picks up an account materialised before it, so only that crash window remains. Verified against a local instance: the app boots, a custody signup assigns the account, and concurrent signups no longer exhaust the pool. * refactor(custody): drop the write-side account inheritance Four consecutive review rounds each found a new defect in one mechanism: making newly created orders, balances and custody users inherit an account. Fixing one edge produced the next - an unloaded relation, then a dependency cycle that stopped the app from booting, then a race with materialisation, then connection-pool exhaustion, then a race between signup and order creation. The mechanism was never needed for what this change is about. Those foreign keys are null for every row in production and nothing reads them, and an access grant only needs the account row itself: owner and grants. Which balance or order carries a reference to it has no bearing on authorisation, and the account-scoped read endpoints resolve data through the account owner instead. So the inheritance is gone, together with the resolver, the advisory locks in the creation paths, and the re-parenting of existing rows during materialisation. The order, balance and signup flows are byte-identical to develop again; this change no longer touches them at all. Materialisation now does exactly what grants require: create the account and the owner's grant. Also rejects an out-of-range account id: 309 digits converted to Infinity, passed a NaN check, and failed in the integer query with a 500 instead of a 400. Verified against a local instance: granting on a legacy Safe creates the account and both grants, a level change keeps the previous row deactivated with its timestamp, the owner's own grant cannot be edited, an oversized id is refused with 400, and custody order creation still works unchanged. * refactor(custody): use the UpdateResult pattern for the grant transition CONTRIBUTING prescribes the UpdateResult pattern for entity state transitions, and it is the dominant one in this codebase (107 occurrences against 77 returning this). deactivate() now returns [id, update] and both callers apply it through a targeted manager.update() inside their transaction instead of saving the whole entity. Verified against a local instance: changing an access level still deactivates the previous row with its timestamp and inserts the new active one. * fix(custody): do not resurrect revoked grants on rollback The down path kept, for every (account, grantee) pair, the active row or - if none was active - the most recent one. That reinstates access the customer revoked: grant READ, raise it to WRITE, then revoke, and the pair is left with history only, the newest of it carrying WRITE. After the active column is dropped, the rolled-back application cannot tell live from historical and reads that row as a live WRITE grant. Rollback now keeps only active rows and deletes all history. The partial index guarantees at most one active row per pair, so the original full unique index can still be restored, and a pair whose access was revoked disappears entirely instead of coming back with the level it had before. * fix(custody): inject the account access guards and reject the legacy alias without entitlement The two access guards extend an abstract base that holds the constructor dependency but declared no constructor of their own, so no design:paramtypes metadata was emitted for them. Nest instantiated both with zero arguments and every guarded route answered 403 with a TypeError message. The catch in canActivate translated that into a ForbiddenException, which is why it read as a plain access denial. Give each guard its own constructor and let canActivate rethrow anything that is not an HttpException, so a defect surfaces as 500 instead of looking like a denied request. checkAccess also accepted the legacy pseudo id from any authenticated caller. It now requires the same entitlement the rest of the service uses - a custody user and no owned account rows - and both paths share one helper. * fix(custody): refuse a lossy rollback and restrict the access list to the owner The migration's down() deleted every inactive grant, which erases exactly the history this change exists to keep. Keeping the newest inactive row instead would be worse - a pair with no active row would come back as a live grant once the active column is gone. Both consolidations lose something, so down() now counts the inactive rows first and refuses with a specific message instead of silently picking one, leaving the schema untouched when it refuses. Listing the access grants required only READ, so any grantee could enumerate the other grants on the account. Third-party grants only become possible with this change, so this is where that disclosure appears. Listing now requires the owner, like granting, changing and revoking, and the route drops the guard the same way its siblings do. * fix(custody): honour account status, hide account existence, serialise account creation Account resolution for authorisation ignored the status column, so a Blocked or Closed account stayed fully usable by direct id while disappearing from the listing. It now resolves only ACTIVE accounts, which covers the read guard, the write guard and grant management in one place. The grant routes carry no custody guard and reached requireOwner directly, so a missing account answered 404 while a foreign one answered 403 - enough to enumerate which account ids exist. Both now answer the same Forbidden the routes already use. Ordinary account creation did not take the owner-scoped advisory lock, so it could insert between legacy materialisation's zero-account check and its own insert. It now runs under the same lock through the transaction manager. * fix(custody): lock the access table before deciding whether a rollback is safe down() refuses when historical rows exist, but it counted them without a lock. A revoke committing between the count and dropping the active column would leave a revoked grant indistinguishable from a live one - exactly what the refusal is meant to prevent. Taking ACCESS EXCLUSIVE first makes the decision and the schema change see the same table; the migration transaction holds it.
TaprootFreak
approved these changes
Jul 27, 2026
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
1 participant
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.
Automatic Release PR
This PR was automatically created after changes were pushed to develop.
Commits: 1 new commit(s)
Checklist