From a6a4e425ed05655ae9a813c3d8fa5d4b56fe5439 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Mon, 27 Jul 2026 13:48:02 +0200 Subject: [PATCH 1/4] Guard every already-covered file with a repo-wide coverage ratchet (#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/workflows/api-pr.yaml | 87 +++- .gitignore | 1 + docs/coverage-gate.md | 202 ++++++++ jest.coverage-gate.config.js | 460 ++++++++++++++++++ package.json | 1 + .../__tests__/bitcoin-fee.service.spec.ts | 38 +- 6 files changed, 776 insertions(+), 13 deletions(-) create mode 100644 docs/coverage-gate.md create mode 100644 jest.coverage-gate.config.js diff --git a/.github/workflows/api-pr.yaml b/.github/workflows/api-pr.yaml index 03d7f0f888..deb69cf348 100644 --- a/.github/workflows/api-pr.yaml +++ b/.github/workflows/api-pr.yaml @@ -13,8 +13,10 @@ permissions: env: NODE_VERSION: '20.x' -# Three job groups run in parallel, so PR feedback is ~max(test-shard, checks, coverage) -# instead of the sum of every step. `test` is additionally split into shards. +# Four job groups run in parallel, so PR feedback is ~max(test-shard, checks, coverage, +# coverage-gate) instead of the sum of every step. `test` is additionally split into shards. +# The ratchet runs the whole suite under full TypeScript compilation, which is what makes its +# per-file numbers exact. jobs: checks: name: Build and checks @@ -81,6 +83,87 @@ jobs: - name: Run coverage run: npm run test:frick:cov + # Runs on a self-hosted runner for branches of this repository. The gate executes the whole suite + # under full compilation and is CPU-bound; a hosted runner gives a public repo four vCPUs, so Jest + # defaults to three workers and the gate alone decided how long a PR run took. The self-hosted + # pool has more cores and does not count against the account's concurrent-job limit. + # + # Pull requests from forks stay on a hosted runner. A self-hosted runner executes the workflow and + # the code of the PR head, so anyone able to open a fork PR would otherwise run arbitrary code on + # it. The repository additionally requires approval for all outside contributors, but that is a + # settings-level control someone can change; this guard lives in the reviewed diff. + # + # Deliberately NOT an `if:` on the job - a skipped check counts as passing, which would let a fork + # PR bypass the gate entirely. Forks run the same gate, just slower. + coverage-gate: + name: Coverage ratchet + runs-on: >- + ${{ (github.event_name != 'pull_request' + || github.event.pull_request.head.repo.full_name == github.repository) + && fromJSON('["self-hosted","dfx-api"]') || 'ubuntu-latest' }} + timeout-minutes: 30 + # Serialise the gate, but only where that is physically necessary. Two of these on the same + # self-hosted machine do not split it, they block each other: measured 16.6 min each against + # 1.5 min for a single run, so waiting is the cheaper outcome. A fork PR runs on a throwaway + # hosted runner instead and shares nothing, so it gets a group of its own and never queues + # behind - or ahead of - an internal run. `queue: max` keeps pending runs queued rather than + # cancelling all but the newest, so a third PR does not turn a waiting run into a red check. + concurrency: + group: >- + ${{ (github.event_name != 'pull_request' + || github.event.pull_request.head.repo.full_name == github.repository) + && 'coverage-ratchet-self-hosted' + || format('coverage-ratchet-hosted-{0}', github.run_id) }} + cancel-in-progress: false + queue: max + steps: + - name: Checkout + uses: actions/checkout@v5 + + # No `cache: 'npm'` here, unlike the hosted jobs. The runner's ~/.npm survives between jobs, + # so the cache would only be redundant - and its post-run upload cost 5.3 min, more than the + # gate itself saves by running here. + - name: Use Node.js ${{ env.NODE_VERSION }} + uses: actions/setup-node@v5 + with: + node-version: ${{ env.NODE_VERSION }} + + - name: Install packages + uses: nick-fields/retry@v4 + with: + timeout_minutes: 10 + max_attempts: 3 + retry_on: error + command: npm ci + + # Repo-wide ratchet: every file already at 100% is pinned, so coverage cannot regress. + # Runs the whole suite (a file is often covered by specs other than its own) with the same + # full compilation as the Frick gate. Deliberately without the Postgres service: no pinned + # file belongs to the migration suites that need it, and enabling those suites can only + # raise coverage, never lower it. + # + # maxWorkers is set here rather than in the npm script so the script stays machine-agnostic: + # a contributor running it locally keeps Jest's own default. + # + # 8 is measured, and the direction is counter-intuitive: fewer workers, not more. At 20 the + # gate took 8.4 min, at 16 it swung between 1.5 and 5.4 min. Host CPU never exceeded ~70% + # in any of those runs - not even with two jobs and 32 workers on 28 cores - so the workers + # were never short of cores. Under full compilation each holds its own TypeScript program, + # and the machine's free memory varies with what else runs on it. Re-measure before changing. + - name: Run coverage ratchet + run: npm run test:gate:cov -- --maxWorkers=${{ (github.event_name != 'pull_request' + || github.event.pull_request.head.repo.full_name == github.repository) && '8' || '3' }} + + # A red gate names the file and the metric but not the uncovered lines. Uploading the lcov + # report turns diagnosis into a download instead of a 15-minute local rerun. + - name: Upload coverage report + if: failure() + uses: actions/upload-artifact@v7 + with: + name: coverage-gate + path: coverage-gate/ + retention-days: 7 + test: name: Test (shard ${{ matrix.shard }}/3) runs-on: ubuntu-latest diff --git a/.gitignore b/.gitignore index 749e21eb8d..4325b38d02 100644 --- a/.gitignore +++ b/.gitignore @@ -18,6 +18,7 @@ lerna-debug.log* # Tests /coverage +/coverage-gate /.nyc_output # IDEs and editors diff --git a/docs/coverage-gate.md b/docs/coverage-gate.md new file mode 100644 index 0000000000..c34b1965a9 --- /dev/null +++ b/docs/coverage-gate.md @@ -0,0 +1,202 @@ +# Coverage gates + +This repo runs two coverage gates in CI. They answer different questions, and neither replaces +the other. + +| Gate | Config | Scope | Question it answers | +| ---------------- | ------------------------------ | ---------------------------------------- | -------------------------------------------------------- | +| Frick gate | `jest.frick.config.js` | 7 Frick files, run by 7 Frick specs only | Do _these specs alone_ fully cover _these files_? | +| Coverage ratchet | `jest.coverage-gate.config.js` | 399 files, whole suite | Has coverage regressed anywhere it was already complete? | + +## What the ratchet is, and what it is not + +The ratchet pins every production file that **already** reaches 100% on all four metrics +(branches, functions, lines, statements). If a change drops any of them below 100 on a pinned +file, CI fails. + +It is a **regression gate**, not a statement about test quality: + +- It does not claim the repo is well tested. Overall coverage is 57.66% of statements and 39.46% + of branches; the pinned files are the subset that happens to be complete today. +- It does not verify that a file's _own_ spec covers it. Under a whole-suite run, coverage may + come from any spec. The Frick gate is the one that makes the stronger per-spec claim, which is + why it stays separate. +- A newly added production file with no coverage at all passes this gate without complaint. The + ratchet only protects files already on the list, and that list grows by hand (see "How the list + grows"). That is the price of the threshold approach. + +Of the 399 pinned files, **217 carry real logic** (they have functions and/or branches) and +**182 are purely declarative today** (NestJS modules, constant files with neither). The two groups +are kept visibly separate in the config so the count is not mistaken for test depth. + +Pinning the declarative ones is deliberate and not vacuous. Istanbul reports a metric with a total +of 0 as 100%, but adding an unexecuted function or conditional moves that metric from 0/0 to 0/N +and fails the threshold. Statements and lines are pinned as well, so even top-level executable +code that no test reaches turns the gate red. + +Test scaffolding is excluded. `shared/utils/test.util.ts` and `shared/utils/test.shared.module.ts` +live outside a `__tests__` directory but are imported only by specs (60 and 28 importers, all +`*.spec.ts`). They are filtered out of `collectCoverageFrom`, so an untested change to a test +helper cannot fail a production gate. + +## How the list was measured + +Reproduce with: + +```bash +npm ci +npm run test:gate:cov +``` + +Two properties of that run matter, and changing either invalidates the numbers: + +1. **Full compilation.** The transform uses `tsconfig.coverage.json` (`isolatedModules: false`), + the same as the Frick gate. The main suite runs ts-jest in transpile-only mode, which emits + the `emitDecoratorMetadata` helpers differently and reports phantom uncovered branches on + dependency-injected constructors. Measured transpile-only, dozens of files would look + incomplete when they are not. +2. **Whole suite.** Files are frequently covered by specs other than their own, so a narrower + run would understate coverage and shrink the list for no reason. + +The gate job deliberately runs **without** the Postgres service that the sharded `test` job uses. +No pinned file belongs to the migration suites that `MIGRATION_TEST_PG` enables, and enabling +further suites can only raise coverage, never lower it. + +Parallelism does not affect the result: istanbul merges per-worker counters additively, so a +statement executed by a suite counts as executed no matter which worker ran it. Worker scheduling +cannot turn a covered file into an uncovered one, which is why the CI script does not serialise. + +The gate runs the whole suite under full compilation, unlike the sharded `test` job that splits +the suite three ways and the Frick gate that runs seven specs. Exact per-file numbers are what +that costs in run time. + +## Where the gate runs + +On a **self-hosted runner**, unlike every other job in the workflow, and serialised across pull +requests by a `concurrency` group. + +Both are conditional. `runs-on` resolves to the self-hosted pool for branches of this repository +and to `ubuntu-latest` for pull requests from forks — a self-hosted runner executes the workflow +and the code of the PR head, and this repository is public. Fork runs get `--maxWorkers=3` to match +a four-vCPU runner, and their own concurrency group, since they share no machine with anything and +have no reason to queue. Deliberately not an `if:` on the job: a skipped check counts as passing, +which would let a fork pull request bypass the gate entirely. Forks run the same gate, slower. + +A hosted runner gives a public repository four vCPUs, so Jest defaults to three workers. Measured +there the gate took 13.8 min and single-handedly pushed a PR run from 4.8 to 15.7 min. The team's +ceiling for a full run is 5 min. On the self-hosted runner the same work takes **1.5 min**. + +Sharding it across hosted runners was considered and rejected: at 11-12 jobs per push the +repository already reaches the account's 20-concurrent-job limit whenever two runs overlap +(measured: 37-52 s of queueing against 2-3 s otherwise), so more jobs there buy queueing, not +speed. Self-hosted jobs do not count against that limit. + +### What was measured, and what it cost to learn + +Everything below is measured. Each line replaced an assumption that turned out wrong, so +re-measure before changing any of it. + +- **`--maxWorkers=8` — fewer, not more.** At 20 the gate took 8.4 min; at 16 it swung between 1.5 + and 5.4 min across otherwise identical runs; at 8 it took 1.5 min in three consecutive runs with + no variation. Host CPU never exceeded ~70 % in any of them — not even when two jobs ran side by + side with twice that many workers and both took 16.6 min. The workers were never short of cores, + so adding more could not help. Under full compilation + each holds its own TypeScript program, and how much memory is free on that host varies with what + else runs there. +- **A `concurrency` group, because two gates at once cripple both.** Two runs started four seconds + apart each took 16.6 min, against 1.5 min alone. They do not split the machine, they block each + other. `queue: max` matters: without it a third pull request cancels the already-waiting run, and + a cancelled check reads as a failure. +- **No `cache: 'npm'` on that job**, unlike the hosted ones. A persistent runner keeps `~/.npm` + between jobs, so restoring gains nothing while saving uploads a cache nobody reads. It cost + 5.3 min per run — more than moving off hosted runners saved in the first place. +- **A cold runner reports roughly double.** The first run on a freshly registered slot took 6.8 min + for work that later took 1.5. Each slot warms separately. A first measurement is not a result. + +### The gate is no longer the bottleneck — but the margin is not the gate's + +At 1.5 min the gate is well clear of the sharded `test` job at ~4.2 min, which now decides how long +a run takes. Full runs measured at **4.9-5.0 min**: the ceiling is met, but by seconds, and +tightening the gate further buys nothing. + +Two caveats worth knowing before reading a slow run as a regression: + +- **Serialisation is not free.** A run that waits for another pull request's gate carries that wait + in its total. Three runs queued back to back measured 5.0, 4.9 and 7.6 min — all with a 1.5 min + gate. Waiting is still much cheaper than colliding (7.6 against 16.6 min), but it can breach the + ceiling when several pull requests land together. +- **The remaining margin belongs to the `test` shards.** If runs need to get reliably faster, that + is where to look, not here. + +## What happens when a pinned file changes + +Both failure modes are loud, verified against jest 29.7 rather than assumed: + +| Situation | Result | Exit | +| -------------------------------------- | ------------------------------------------- | ---- | +| Pinned file drops below 100% | `coverage threshold for ... not met: ` | 1 | +| Pinned file deleted, renamed, excluded | `Coverage data for ... was not found` | 1 | + +The second row is the important one: the gate cannot silently stop protecting a file. Threshold +keys are resolved with `path.resolve` against the working directory, and both `npm run +test:gate:cov` and the workflow run from the repo root, so the `src/...` keys match the coverage +map. + +The run also writes an `lcov` report under `coverage-gate/`. On failure the CI job uploads that +directory as the `coverage-gate` artifact (7-day retention, via `actions/upload-artifact@v7`, +step "Upload coverage report" on the "Coverage ratchet" job). That shows which lines are missing +without re-running the whole gate locally — which on a developer machine, without the CI runner's +warm caches, is a good deal slower than the 1.5 min it takes in CI. + +## Current state + +The collection glob matches 1,643 files under `src/`. 1,591 of them contain instrumentable code +and appear in the report. The remaining 52 compile to no executable statements and therefore +cannot be measured or pinned: 50 are type-only (interfaces, type aliases, response shapes), one +consists entirely of commented-out code (`integration/exchange/services/p2b.service.ts`) and one +is empty (`subdomains/supporting/payin/enums/index.ts`, 0 bytes). Those two are pre-existing and untouched here; +deleting them would be a separate cleanup. + +| Class | Files | Meaning | +| -------- | ----- | ----------------------------------------------- | +| Complete | 399 | Pinned by the ratchet | +| Partial | 1,062 | Some coverage, below 100 on at least one metric | +| None | 130 | No coverage at all | + +Totals: statements 57.66%, branches 39.46%, functions 31.84%, lines 57.98%. + +Coverage is very unevenly distributed. `subdomains/supporting/payout` has 69 of 102 files +complete; `subdomains/supporting/dex` has 6 of 170, `subdomains/supporting/payin` 6 of 102, and +`subdomains/core/liquidity-management` 7 of 62. Nine files under `subdomains/supporting/mros` and +six under `subdomains/generic/admin` have no coverage at all. + +## How the list grows + +Any PR may add files to `coverageThreshold` once they reach 100%. +`jest.coverage-gate.config.js` holds the 399 paths in two arrays, `PINNED_LOGIC` (logic-carrying +files) and `PINNED_DECLARATIVE` (purely declarative files), from which `coverageThreshold` is +generated. Adding a file means appending its path to the matching array, not writing out a +`coverageThreshold` object entry by hand. + +The intended next step is the set already within reach: **29 files sit at ≥90% on all four +metrics**, several of them one or two uncovered branches away. Examples: + +| File | branches | functions | lines | statements | +| --------------------------------------------------------------------------- | -------- | --------- | ----- | ---------- | +| `src/subdomains/core/accounting/services/consumers/exchange-tx.consumer.ts` | 97.03 | 100 | 100 | 99.54 | +| `src/subdomains/core/accounting/services/ledger-reconciliation.service.ts` | 95.52 | 100 | 99.41 | 99.48 | +| `src/subdomains/core/accounting/services/ledger-mark.service.ts` | 95.23 | 100 | 100 | 98.95 | +| `src/integration/infrastructure/storage/s3-storage.service.ts` | 95 | 100 | 100 | 100 | + +To regenerate the full picture, run the gate and read `coverage-gate/coverage-summary.json`. + +**Removing a file from the list is not a normal fix.** If a change makes a pinned file drop +below 100, the expected response is to extend the tests. Unpinning is an explicit decision that +belongs in the PR description, not a silent edit. + +That rule stays hard for the 217 logic-carrying files. A foreseeable friction case is different: +when one of the 182 purely declarative files (a NestJS module, a constants file) first gains +executable logic — for example a `useFactory` on a module — the function metric jumps from 0/0 to +0/N and the gate turns red. Tests remain the preferred fix, but unpinning that one file is an +allowed outcome if the PR description names and justifies it (not as a silent edit). For +logic-carrying files the rule is unchanged: extend the tests. diff --git a/jest.coverage-gate.config.js b/jest.coverage-gate.config.js new file mode 100644 index 0000000000..161525cee1 --- /dev/null +++ b/jest.coverage-gate.config.js @@ -0,0 +1,460 @@ +// Repo-wide coverage ratchet. Every production file that already reaches 100% on all four +// metrics is pinned here, so a change that drops coverage on any of them fails CI. +// +// This is a REGRESSION gate, not a claim of exhaustive testing: it freezes the coverage that +// exists today. Files not listed here are still uncovered or only partially covered - see +// docs/coverage-gate.md for the measured gaps and for how the list is meant to grow. +// +// Two rules keep the numbers honest: +// 1. Full compilation (tsconfig.coverage.json, isolatedModules: false), same as the Frick gate. +// The main suite runs ts-jest transpile-only, which emits the emitDecoratorMetadata helpers +// differently and reports phantom uncovered branches on dependency-injected constructors. +// 2. The whole suite runs, because a file is frequently covered by specs other than its own. +// +// The dedicated Frick gate (jest.frick.config.js) stays separate on purpose: it runs ONLY the +// seven Frick specs and therefore proves that those specs alone reach 100% - an assertion this +// repo-wide run cannot make, because here any spec may contribute the coverage. +const base = require('./package.json').jest; + +// Every pinned file is held to all four metrics at 100%. +const FULL_COVERAGE = { branches: 100, functions: 100, lines: 100, statements: 100 }; + +// --- PINNED LOGIC --- // +// Files carrying real logic: they have functions and/or branches, so the threshold asserts +// that executable code stays covered. +const PINNED_LOGIC = [ + 'src/config/frick.config.ts', + 'src/integration/bank/dto/frick-vban.dto.ts', + 'src/integration/bank/dto/frick.dto.ts', + 'src/integration/bank/dto/olkypay.dto.ts', + 'src/integration/bank/dto/yapeal.dto.ts', + 'src/integration/bank/services/frick.service.ts', + 'src/integration/bank/services/iso20022.service.ts', + 'src/integration/binance-pay/dto/binance.dto.ts', + 'src/integration/blockchain/bitcoin/node/bitcoin-client.ts', + 'src/integration/blockchain/bitcoin/node/rpc/node-not-ready.error.ts', + 'src/integration/blockchain/bitcoin/services/bitcoin-based-fee.service.ts', + 'src/integration/blockchain/boltz/dto/boltz.dto.ts', + 'src/integration/blockchain/monero/dto/monero.dto.ts', + 'src/integration/blockchain/monero/monero-helper.ts', + 'src/integration/blockchain/realunit/dto/realunit-broker.dto.ts', + 'src/integration/blockchain/shared/enums/blockchain.enum.ts', + 'src/integration/blockchain/shared/errors/tx-broadcast.error.ts', + 'src/integration/blockchain/shared/evm/paymaster/pimlico-paymaster.service.ts', + 'src/integration/blockchain/zano/zano-helper.ts', + 'src/integration/checkout/dto/checkout.dto.ts', + 'src/integration/exchange/dto/mexc.dto.ts', + 'src/integration/exchange/dto/scrypt.dto.ts', + 'src/integration/exchange/dto/trade-result.dto.ts', + 'src/integration/exchange/enums/exchange.enum.ts', + 'src/integration/infrastructure/storage/azure-storage.service.ts', + 'src/integration/infrastructure/storage/storage.factory.ts', + 'src/integration/infrastructure/storage/storage.service.ts', + 'src/integration/kucoin-pay/kucoin-pay.dto.ts', + 'src/integration/lightning/dto/lnd.dto.ts', + 'src/integration/scorechain/dto/scorechain-screening-dto.mapper.ts', + 'src/integration/scorechain/exceptions/scorechain-object-not-found.exception.ts', + 'src/integration/sift/dto/sift.dto.ts', + 'src/polyfills.ts', + 'src/shared/auth/allow-tfa-pending.decorator.ts', + 'src/shared/auth/user-role.enum.ts', + 'src/shared/services/typeorm-logger.ts', + 'src/shared/utils/bitbox-ascii.util.ts', + 'src/shared/utils/cron.ts', + 'src/shared/utils/custom-cron-expression.ts', + 'src/shared/utils/request-client.ts', + 'src/shared/validators/is-ssrf-safe-url.validator.ts', + 'src/subdomains/core/accounting/controllers/ledger.controller.ts', + 'src/subdomains/core/accounting/dto/ledger-account.dto.ts', + 'src/subdomains/core/accounting/dto/ledger-dto.mapper.ts', + 'src/subdomains/core/accounting/dto/ledger-query.dto.ts', + 'src/subdomains/core/accounting/dto/ledger-reconciliation.dto.ts', + 'src/subdomains/core/accounting/entities/ledger-account.entity.ts', + 'src/subdomains/core/accounting/entities/ledger-leg.entity.ts', + 'src/subdomains/core/accounting/entities/ledger-tx.entity.ts', + 'src/subdomains/core/accounting/repositories/ledger-account.repository.ts', + 'src/subdomains/core/accounting/repositories/ledger-leg.repository.ts', + 'src/subdomains/core/accounting/repositories/ledger-tx.repository.ts', + 'src/subdomains/core/accounting/services/consumers/ledger-gate-blocked.exception.ts', + 'src/subdomains/core/accounting/services/consumers/ledger-mark-bridge.helper.ts', + 'src/subdomains/core/accounting/services/ledger-account.service.ts', + 'src/subdomains/core/accounting/services/ledger-booking-job.service.ts', + 'src/subdomains/core/accounting/services/ledger-bootstrap.service.ts', + 'src/subdomains/core/aml/enums/aml-list-status.enum.ts', + 'src/subdomains/core/aml/enums/aml-reason.enum.ts', + 'src/subdomains/core/aml/enums/aml-rule.enum.ts', + 'src/subdomains/core/aml/enums/check-status.enum.ts', + 'src/subdomains/core/aml/enums/scorechain-outcome.enum.ts', + 'src/subdomains/core/aml/services/transaction-aml-check.service.ts', + 'src/subdomains/core/buy-crypto/process/exceptions/abort-batch-creation.exception.ts', + 'src/subdomains/core/custody/dto/output/custody-order-history.dto.ts', + 'src/subdomains/core/custody/enums/custody.ts', + 'src/subdomains/core/faucet-request/enums/faucet-request.ts', + 'src/subdomains/core/history/dto/history.dto.ts', + 'src/subdomains/core/history/dto/output/chain-report-history.dto.ts', + 'src/subdomains/core/history/dto/output/coin-tracking-history.dto.ts', + 'src/subdomains/core/liquidity-management/enums/index.ts', + 'src/subdomains/core/liquidity-management/exceptions/order-failed.exception.ts', + 'src/subdomains/core/monitoring/observers/external-services.observer.ts', + 'src/subdomains/core/monitoring/observers/realunit-w2w-gas.observer.ts', + 'src/subdomains/core/payment-link/entities/payment-link.config.ts', + 'src/subdomains/core/payment-link/enums/index.ts', + 'src/subdomains/core/payment-link/enums/merchant.enum.ts', + 'src/subdomains/core/trading/enums/index.ts', + 'src/subdomains/generic/forwarding/controllers/lnurld-forward.controller.ts', + 'src/subdomains/generic/forwarding/controllers/lnurlw-forward.controller.ts', + 'src/subdomains/generic/gs/middleware/debug-query-tree-size.middleware.ts', + 'src/subdomains/generic/kyc/dto/ident-result-data.dto.ts', + 'src/subdomains/generic/kyc/dto/kyc-error.enum.ts', + 'src/subdomains/generic/kyc/dto/kyc-file.dto.ts', + 'src/subdomains/generic/kyc/dto/manual-ident-result.dto.ts', + 'src/subdomains/generic/kyc/dto/mapper/kyc-file.mapper.ts', + 'src/subdomains/generic/kyc/dto/output/kyc-info.dto.ts', + 'src/subdomains/generic/kyc/dto/output/setup-2fa.dto.ts', + 'src/subdomains/generic/kyc/enums/content-type.enum.ts', + 'src/subdomains/generic/kyc/enums/file-category.enum.ts', + 'src/subdomains/generic/kyc/enums/kyc-step-name.enum.ts', + 'src/subdomains/generic/kyc/enums/review-status.enum.ts', + 'src/subdomains/generic/support/dto/onboarding-pdf.dto.ts', + 'src/subdomains/generic/support/dto/user-data-support.dto.ts', + 'src/subdomains/generic/user/models/auth/dto/auth-lnurl.dto.ts', + 'src/subdomains/generic/user/models/bank-data/dto/create-bank-data.dto.ts', + 'src/subdomains/generic/user/models/bank-data/dto/update-bank-data.dto.ts', + 'src/subdomains/generic/user/models/kyc/dto/kyc-file.dto.ts', + 'src/subdomains/generic/user/models/recommendation/dto/recommendation.dto.ts', + 'src/subdomains/generic/user/models/user-data-relation/dto/user-data-relation.enum.ts', + 'src/subdomains/generic/user/models/user-data/account-type.enum.ts', + 'src/subdomains/generic/user/models/user-data/jwt-revocation-sync.service.ts', + 'src/subdomains/generic/user/models/user-data/kyc-identification-type.enum.ts', + 'src/subdomains/generic/user/models/user-data/user-data.enum.ts', + 'src/subdomains/generic/user/models/user/dto/verify-mail.dto.ts', + 'src/subdomains/generic/user/models/user/user.enum.ts', + 'src/subdomains/generic/user/services/webhook/dto/webhook.dto.ts', + 'src/subdomains/supporting/bank-tx/bank-tx/dto/sepa.dto.ts', + 'src/subdomains/supporting/bank-tx/bank-tx/services/bank-tx-frick.service.ts', + 'src/subdomains/supporting/bank-tx/bank-tx/services/bank-tx-outgoing-match.service.ts', + 'src/subdomains/supporting/bank/bank/dto/bank.dto.ts', + 'src/subdomains/supporting/bank/virtual-iban/providers/frick-viban.provider.ts', + 'src/subdomains/supporting/bank/virtual-iban/providers/yapeal-viban.provider.ts', + 'src/subdomains/supporting/dex/exceptions/price-slippage.exception.ts', + 'src/subdomains/supporting/dex/strategies/check-liquidity/impl/base/check-liquidity.strategy-registry.ts', + 'src/subdomains/supporting/dex/strategies/purchase-liquidity/impl/base/purchase-liquidity.strategy-registry.ts', + 'src/subdomains/supporting/fiat-output/dto/create-fiat-output.dto.ts', + 'src/subdomains/supporting/fiat-output/dto/update-fiat-output.dto.ts', + 'src/subdomains/supporting/fiat-output/fiat-output-frick.service.ts', + 'src/subdomains/supporting/log/log.entity.ts', + 'src/subdomains/supporting/notification/enums/index.ts', + 'src/subdomains/supporting/payin/strategies/register/impl/base/polling.strategy.ts', + 'src/subdomains/supporting/payin/strategies/register/impl/base/register.strategy-registry.ts', + 'src/subdomains/supporting/payin/strategies/send/impl/base/send.strategy-registry.ts', + 'src/subdomains/supporting/payment/dto/payment-method.enum.ts', + 'src/subdomains/supporting/payment/dto/transaction-helper/quote-error.enum.ts', + 'src/subdomains/supporting/payment/dto/transaction-helper/tx-statement-details.dto.ts', + 'src/subdomains/supporting/payment/dto/transaction.dto.ts', + 'src/subdomains/supporting/payout/entities/payout-order.entity.ts', + 'src/subdomains/supporting/payout/exceptions/invalid-payout-amount.exception.ts', + 'src/subdomains/supporting/payout/exceptions/payout-broadcast.exception.ts', + 'src/subdomains/supporting/payout/factories/payout-order.factory.ts', + 'src/subdomains/supporting/payout/repositories/payout-order.repository.ts', + 'src/subdomains/supporting/payout/services/payout-arbitrum.service.ts', + 'src/subdomains/supporting/payout/services/payout-arkade.service.ts', + 'src/subdomains/supporting/payout/services/payout-base.service.ts', + 'src/subdomains/supporting/payout/services/payout-bitcoin-testnet4.service.ts', + 'src/subdomains/supporting/payout/services/payout-bsc.service.ts', + 'src/subdomains/supporting/payout/services/payout-citrea.service.ts', + 'src/subdomains/supporting/payout/services/payout-ethereum.service.ts', + 'src/subdomains/supporting/payout/services/payout-evm.service.ts', + 'src/subdomains/supporting/payout/services/payout-firo.service.ts', + 'src/subdomains/supporting/payout/services/payout-gnosis.service.ts', + 'src/subdomains/supporting/payout/services/payout-icp.service.ts', + 'src/subdomains/supporting/payout/services/payout-lightning.service.ts', + 'src/subdomains/supporting/payout/services/payout-monero.service.ts', + 'src/subdomains/supporting/payout/services/payout-optimism.service.ts', + 'src/subdomains/supporting/payout/services/payout-polygon.service.ts', + 'src/subdomains/supporting/payout/services/payout-sepolia.service.ts', + 'src/subdomains/supporting/payout/services/payout-spark.service.ts', + 'src/subdomains/supporting/payout/services/payout-tron.service.ts', + 'src/subdomains/supporting/payout/services/payout-zano.service.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/arbitrum-coin.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/arbitrum-token.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/arkade.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/base-coin.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/base-token.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/base/bitcoin-based.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/base/icp.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/base/payout.strategy-registry.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/base/payout.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/base/tron.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/base/zano.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/bitcoin-testnet4.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/bitcoin.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/bsc-coin.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/bsc-token.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/cardano-coin.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/cardano-token.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/citrea-coin.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/citrea-token.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/ethereum-coin.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/ethereum-token.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/firo.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/gnosis-coin.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/gnosis-token.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/icp-coin.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/icp-token.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/lightning.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/monero.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/optimism-coin.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/optimism-token.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/polygon-coin.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/polygon-token.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/sepolia-coin.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/sepolia-token.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/solana-coin.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/solana-token.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/spark.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/tron-coin.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/tron-token.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/zano-coin.strategy.ts', + 'src/subdomains/supporting/payout/strategies/payout/impl/zano-token.strategy.ts', + 'src/subdomains/supporting/payout/strategies/prepare/impl/base/evm.strategy.ts', + 'src/subdomains/supporting/payout/strategies/prepare/impl/base/prepare.strategy-registry.ts', + 'src/subdomains/supporting/pricing/domain/exceptions/price-invalid.exception.ts', + 'src/subdomains/supporting/pricing/domain/exceptions/price-unavailable.exception.ts', + 'src/subdomains/supporting/pricing/dto/price-request.ts', + 'src/subdomains/supporting/realunit/controllers/realunit-legal.controller.ts', + 'src/subdomains/supporting/realunit/dto/client.dto.ts', + 'src/subdomains/supporting/realunit/dto/realunit-confirm-aktionariat.dto.ts', + 'src/subdomains/supporting/realunit/dto/realunit-legal-dto.mapper.ts', + 'src/subdomains/supporting/realunit/dto/realunit-registration.dto.ts', + 'src/subdomains/supporting/realunit/entities/aktionariat-registration.entity.ts', + 'src/subdomains/supporting/realunit/entities/realunit-legal-acceptance.entity.ts', + 'src/subdomains/supporting/realunit/enums/realunit-legal-agreement.enum.ts', + 'src/subdomains/supporting/realunit/exceptions/buy-exceptions.ts', + 'src/subdomains/supporting/realunit/exceptions/price-source-unavailable.exception.ts', + 'src/subdomains/supporting/realunit/realunit-dev.service.ts', + 'src/subdomains/supporting/realunit/realunit-legal.service.ts', + 'src/subdomains/supporting/realunit/repositories/aktionariat-registration.repository.ts', + 'src/subdomains/supporting/realunit/repositories/realunit-legal-acceptance.repository.ts', + 'src/subdomains/supporting/recall/recall-reason.enum.ts', + 'src/subdomains/supporting/support-issue/dto/support-issue.dto.ts', + 'src/subdomains/supporting/support-issue/enums/department.enum.ts', + 'src/subdomains/supporting/support-issue/enums/support-issue.enum.ts', + 'src/subdomains/supporting/support-issue/enums/support-log.enum.ts', +]; + +// --- PINNED DECLARATIVE --- // +// Purely declarative files: NestJS modules and constant files with neither functions nor +// branches today. Istanbul reports a metric with a total of 0 as 100%, so pinning them is not +// vacuous - adding an unexecuted function moves that metric from 0/0 to 0/N and fails the gate. +const PINNED_DECLARATIVE = [ + 'src/config/chains.config.ts', + 'src/config/config.module.ts', + 'src/integration/alchemy/alchemy.module.ts', + 'src/integration/bank/bank.module.ts', + 'src/integration/binance-pay/binance-pay.module.ts', + 'src/integration/binance-pay/dto/binance-enum.mapper.ts', + 'src/integration/blockchain/api/blockchain-api.module.ts', + 'src/integration/blockchain/api/dto/create-transaction.dto.ts', + 'src/integration/blockchain/api/dto/get-balances.dto.ts', + 'src/integration/blockchain/arbitrum/arbitrum.module.ts', + 'src/integration/blockchain/arkade/arkade.module.ts', + 'src/integration/blockchain/arweave/arweave.module.ts', + 'src/integration/blockchain/base/base.module.ts', + 'src/integration/blockchain/bitcoin-testnet4/bitcoin-testnet4.module.ts', + 'src/integration/blockchain/bitcoin/bitcoin.module.ts', + 'src/integration/blockchain/bitcoin/node/dto/command.dto.ts', + 'src/integration/blockchain/bitcoin/node/rpc/index.ts', + 'src/integration/blockchain/blockchain.module.ts', + 'src/integration/blockchain/boltz/boltz.module.ts', + 'src/integration/blockchain/bsc/bsc.module.ts', + 'src/integration/blockchain/cardano/cardano.module.ts', + 'src/integration/blockchain/citrea/citrea.module.ts', + 'src/integration/blockchain/clementine/clementine.module.ts', + 'src/integration/blockchain/deuro/deuro.module.ts', + 'src/integration/blockchain/ebel2x/ebel2x.module.ts', + 'src/integration/blockchain/ethereum/ethereum.module.ts', + 'src/integration/blockchain/firo/firo.module.ts', + 'src/integration/blockchain/frankencoin/frankencoin.module.ts', + 'src/integration/blockchain/gnosis/gnosis.module.ts', + 'src/integration/blockchain/icp/icp.module.ts', + 'src/integration/blockchain/juice/juice.module.ts', + 'src/integration/blockchain/monero/monero.module.ts', + 'src/integration/blockchain/optimism/optimism.module.ts', + 'src/integration/blockchain/polygon/polygon.module.ts', + 'src/integration/blockchain/realunit/realunit-blockchain.module.ts', + 'src/integration/blockchain/sepolia/sepolia.module.ts', + 'src/integration/blockchain/shared/blockscout/blockscout.module.ts', + 'src/integration/blockchain/shared/evm/delegation/eip7702-delegation.module.ts', + 'src/integration/blockchain/shared/evm/paymaster/pimlico-paymaster.module.ts', + 'src/integration/blockchain/shared/util/blockchain.service.ts', + 'src/integration/blockchain/solana/solana.module.ts', + 'src/integration/blockchain/spark/spark.module.ts', + 'src/integration/blockchain/tron/tron.module.ts', + 'src/integration/blockchain/zano/zano.module.ts', + 'src/integration/checkout/checkout.module.ts', + 'src/integration/exchange/dto/trade-order.dto.ts', + 'src/integration/exchange/dto/withdrawal-order.dto.ts', + 'src/integration/geolocation/geo-location.module.ts', + 'src/integration/ikna/dto/ikna-query.dto.ts', + 'src/integration/ikna/ikna.module.ts', + 'src/integration/infrastructure/storage/worm-retention.const.ts', + 'src/integration/integration.module.ts', + 'src/integration/kucoin-pay/kucoin-pay.module.ts', + 'src/integration/letter/letter.module.ts', + 'src/integration/lightning/lightning.module.ts', + 'src/integration/railgun/railgun.module.ts', + 'src/integration/scorechain/dto/scorechain-query.dto.ts', + 'src/integration/scorechain/dto/scorechain-screening.dto.ts', + 'src/integration/scorechain/scorechain.module.ts', + 'src/integration/sift/sift.module.ts', + 'src/integration/tatum/tatum.module.ts', + 'src/shared/dto/entity.dto.ts', + 'src/shared/dto/error.dto.ts', + 'src/shared/dto/route.dto.ts', + 'src/shared/models/asset/dto/asset-query.dto.ts', + 'src/shared/models/asset/dto/update-asset.dto.ts', + 'src/shared/models/country/dto/country.dto.ts', + 'src/shared/models/language/dto/language.dto.ts', + 'src/shared/models/language/language.entity.ts', + 'src/shared/models/setting/dto/custom-sign-up-fees.dto.ts', + 'src/shared/models/setting/dto/info-banner.dto.ts', + 'src/shared/models/setting/dto/ip-blacklist.dto.ts', + 'src/shared/models/setting/dto/manual-log-position.dto.ts', + 'src/shared/models/setting/dto/support-clerk-account.dto.ts', + 'src/shared/models/setting/dto/update-process.dto.ts', + 'src/shared/models/setting/setting.entity.ts', + 'src/shared/shared.module.ts', + 'src/shared/utils/logos/dfx-logo.ts', + 'src/shared/utils/logos/realunit-logo-full.ts', + 'src/subdomains/core/accounting/accounting.module.ts', + 'src/subdomains/core/accounting/dto/ledger-margin.dto.ts', + 'src/subdomains/core/aml/dto/manual-aml-check.dto.ts', + 'src/subdomains/core/aml/entities/sanction.entity.ts', + 'src/subdomains/core/buy-crypto/routes/buy/dto/buy-payment-info.dto.ts', + 'src/subdomains/core/buy-crypto/routes/buy/dto/buy-quote.dto.ts', + 'src/subdomains/core/buy-crypto/routes/buy/dto/buy.dto.ts', + 'src/subdomains/core/buy-crypto/routes/buy/dto/pdf.dto.ts', + 'src/subdomains/core/buy-crypto/routes/buy/dto/update-buy.dto.ts', + 'src/subdomains/core/buy-crypto/routes/swap/dto/swap-payment-info.dto.ts', + 'src/subdomains/core/buy-crypto/routes/swap/dto/swap-quote.dto.ts', + 'src/subdomains/core/buy-crypto/routes/swap/dto/swap.dto.ts', + 'src/subdomains/core/buy-crypto/routes/swap/dto/update-swap.dto.ts', + 'src/subdomains/core/custody/config/order-config.ts', + 'src/subdomains/core/custody/dto/input/create-custody-account.dto.ts', + 'src/subdomains/core/custody/dto/input/custody-signup.dto.ts', + 'src/subdomains/core/custody/dto/input/update-custody-account.dto.ts', + 'src/subdomains/core/custody/dto/output/custody-account.dto.ts', + 'src/subdomains/core/custody/dto/output/custody-auth.dto.ts', + 'src/subdomains/core/custody/dto/output/custody-balance.dto.ts', + 'src/subdomains/core/custody/dto/output/custody-order-response.dto.ts', + 'src/subdomains/core/custody/dto/output/custody-order.dto.ts', + 'src/subdomains/core/history/dto/history-filter.dto.ts', + 'src/subdomains/core/history/dto/refund-data.dto.ts', + 'src/subdomains/core/liquidity-management/dto/input/liquidity-management-action.dto.ts', + 'src/subdomains/core/liquidity-management/dto/input/liquidity-management-request.dto.ts', + 'src/subdomains/core/liquidity-management/dto/input/liquidity-management-settings.dto.ts', + 'src/subdomains/core/liquidity-management/dto/input/liquidity-management-update.dto.ts', + 'src/subdomains/core/liquidity-management/dto/output/liquidity-management-rule-output.dto.ts', + 'src/subdomains/core/monitoring/system-state-snapshot.entity.ts', + 'src/subdomains/core/payment-link/dto/payment-link-recipient-address.dto.ts', + 'src/subdomains/core/payment-link/dto/payment-link.dto.ts', + 'src/subdomains/core/payment-link/payment-link-payment.module.ts', + 'src/subdomains/core/referral/process/ref.entity.ts', + 'src/subdomains/core/referral/reward/dto/update-ref-reward.dto.ts', + 'src/subdomains/core/route/dto/update-route.dto.ts', + 'src/subdomains/core/sell-crypto/route/dto/gasless-transfer.dto.ts', + 'src/subdomains/core/sell-crypto/route/dto/sell-payment-info.dto.ts', + 'src/subdomains/core/sell-crypto/route/dto/sell-quote.dto.ts', + 'src/subdomains/core/sell-crypto/route/dto/sell.dto.ts', + 'src/subdomains/core/sell-crypto/route/dto/unsigned-tx.dto.ts', + 'src/subdomains/core/sell-crypto/route/dto/update-sell.dto.ts', + 'src/subdomains/generic/kyc/dto/input/kyc-query.dto.ts', + 'src/subdomains/generic/kyc/dto/input/update-kyc-step.dto.ts', + 'src/subdomains/generic/kyc/dto/input/update-name-check-log.dto.ts', + 'src/subdomains/generic/kyc/dto/input/verify-2fa.dto.ts', + 'src/subdomains/generic/kyc/dto/output/kyc-financial-out.dto.ts', + 'src/subdomains/generic/kyc/dto/output/kyc-merged.dto.ts', + 'src/subdomains/generic/kyc/entities/mail-change-log.entity.ts', + 'src/subdomains/generic/kyc/entities/manual-log.entity.ts', + 'src/subdomains/generic/kyc/entities/merge-log.entity.ts', + 'src/subdomains/generic/kyc/entities/risk-status-log.entity.ts', + 'src/subdomains/generic/kyc/entities/totp-auth-log.entity.ts', + 'src/subdomains/generic/support/dto/transaction-list-query.dto.ts', + 'src/subdomains/generic/support/entities/support-issue-template.entity.ts', + 'src/subdomains/generic/user/models/auth/dto/auth-response.dto.ts', + 'src/subdomains/generic/user/models/auth/dto/challenge.dto.ts', + 'src/subdomains/generic/user/models/auth/dto/merge-response.dto.ts', + 'src/subdomains/generic/user/models/auth/dto/sign-message.dto.ts', + 'src/subdomains/generic/user/models/custody-provider/custody-provider.entity.ts', + 'src/subdomains/generic/user/models/custody-provider/dto/custody-provider.dto.ts', + 'src/subdomains/generic/user/models/kyc/dto/kyc-data-transfer.dto.ts', + 'src/subdomains/generic/user/models/kyc/dto/kyc-data.dto.ts', + 'src/subdomains/generic/user/models/kyc/dto/kyc-info.dto.ts', + 'src/subdomains/generic/user/models/user-data-relation/dto/update-user-data-relation.dto.ts', + 'src/subdomains/generic/user/models/user/dto/alby.dto.ts', + 'src/subdomains/generic/user/models/user/dto/api-key.dto.ts', + 'src/subdomains/generic/user/models/user/dto/download-user-data.dto.ts', + 'src/subdomains/generic/user/models/user/dto/linked-user.dto.ts', + 'src/subdomains/generic/user/models/user/dto/update-address.dto.ts', + 'src/subdomains/generic/user/models/user/dto/user-name.dto.ts', + 'src/subdomains/generic/user/models/user/dto/user-profile.dto.ts', + 'src/subdomains/generic/user/models/user/dto/user.dto.ts', + 'src/subdomains/generic/user/models/wallet/dto/wallet.dto.ts', + 'src/subdomains/supporting/address-pool/deposit/dto/create-deposit.dto.ts', + 'src/subdomains/supporting/address-pool/deposit/dto/deposit.dto.ts', + 'src/subdomains/supporting/bank-tx/bank-tx-return/dto/update-bank-tx-return.dto.ts', + 'src/subdomains/supporting/bank/bank-account/bank-account.entity.ts', + 'src/subdomains/supporting/bank/bank-account/dto/bank-account.dto.ts', + 'src/subdomains/supporting/bank/bank-account/dto/create-bank-account.dto.ts', + 'src/subdomains/supporting/bank/virtual-iban/dto/create-virtual-iban.dto.ts', + 'src/subdomains/supporting/bank/virtual-iban/dto/virtual-iban.dto.ts', + 'src/subdomains/supporting/dex/dex.module.ts', + 'src/subdomains/supporting/dex/strategies/sell-liquidity/impl/base/evm-coin.strategy.ts', + 'src/subdomains/supporting/dex/strategies/sell-liquidity/impl/base/evm-token.strategy.ts', + 'src/subdomains/supporting/log/dto/create-log.dto.ts', + 'src/subdomains/supporting/log/log.module.ts', + 'src/subdomains/supporting/notification/notification.module.ts', + 'src/subdomains/supporting/notification/realunit-mail-rules.ts', + 'src/subdomains/supporting/payin/interfaces/index.ts', + 'src/subdomains/supporting/payin/payin-webhook.module.ts', + 'src/subdomains/supporting/payin/services/base/payin-bitcoin-based.service.ts', + 'src/subdomains/supporting/payment/dto/transaction-helper/min-amount.dto.ts', + 'src/subdomains/supporting/payment/dto/transaction-helper/structured-error.dto.ts', + 'src/subdomains/supporting/payout/payout.module.ts', + 'src/subdomains/supporting/payout/services/base/payout-bitcoin-based.service.ts', + 'src/subdomains/supporting/pricing/dto/price-request-raw.ts', + 'src/subdomains/supporting/realunit/dto/realunit-legal.dto.ts', + 'src/subdomains/supporting/realunit/utils/queries.ts', + 'src/subdomains/supporting/support-issue/dto/bind-escalation-chat.dto.ts', + 'src/subdomains/supporting/support-issue/dto/limit-request.dto.ts', + 'src/subdomains/supporting/support-issue/dto/support-issue-label.ts', + 'src/subdomains/supporting/support-issue/dto/update-support-issue.dto.ts', +]; + +module.exports = { + ...base, + transform: { '^.+\\.(t|j)s$': ['ts-jest', { tsconfig: 'tsconfig.coverage.json' }] }, + collectCoverageFrom: [ + '**/*.ts', + '!**/*.spec.ts', + '!**/__tests__/**', + '!**/__test__/**', + '!**/tests/**', + '!**/__mocks__/**', + '!**/*.mock.ts', + '!**/*.d.ts', + '!jest-env.setup.ts', + // Test scaffolding that lives outside a __tests__ directory: imported only by specs + // (60 and 28 importers respectively, all of them *.spec.ts). Pinning them would make an + // untested change to a test helper fail the production gate. + '!shared/utils/test.util.ts', + '!shared/utils/test.shared.module.ts', + ], + // json-summary is what docs/coverage-gate.md tells you to read when extending the pinned list; + // lcov is uploaded as a CI artifact so a failing gate can be diagnosed without a local rerun. + coverageReporters: ['text-summary', 'json-summary', 'lcov'], + coverageDirectory: '../coverage-gate', + coverageThreshold: Object.fromEntries( + [...PINNED_LOGIC, ...PINNED_DECLARATIVE].map((file) => [file, { ...FULL_COVERAGE }]), + ), +}; diff --git a/package.json b/package.json index eb87e8421b..e4d80203e7 100644 --- a/package.json +++ b/package.json @@ -21,6 +21,7 @@ "test:watch": "jest --watch", "test:cov": "jest --coverage", "test:frick:cov": "jest --config jest.frick.config.js integration/bank/services/__tests__/frick.service.spec.ts integration/bank/services/__tests__/iso20022.service.spec.ts config/__tests__/frick.config.spec.ts config/__tests__/bank-frick-config.spec.ts subdomains/supporting/bank-tx/bank-tx/services/__tests__/bank-tx-frick.service.spec.ts subdomains/supporting/bank-tx/bank-tx/services/__tests__/bank-tx-outgoing-match.service.spec.ts subdomains/supporting/fiat-output/__tests__/fiat-output-frick.service.spec.ts --coverage --runInBand --collectCoverageFrom=integration/bank/dto/frick.dto.ts --collectCoverageFrom=integration/bank/services/frick.service.ts --collectCoverageFrom=integration/bank/services/iso20022.service.ts --collectCoverageFrom=config/frick.config.ts --collectCoverageFrom=subdomains/supporting/bank-tx/bank-tx/services/bank-tx-frick.service.ts --collectCoverageFrom=subdomains/supporting/bank-tx/bank-tx/services/bank-tx-outgoing-match.service.ts --collectCoverageFrom=subdomains/supporting/fiat-output/fiat-output-frick.service.ts", + "test:gate:cov": "jest --config jest.coverage-gate.config.js --coverage --silent", "type-check": "tsc --noEmit", "format:check": "prettier --check \"src/**/*.ts\" \"test/**/*.ts\"", "check": "npm run lint && npm run test", diff --git a/src/integration/blockchain/bitcoin/services/__tests__/bitcoin-fee.service.spec.ts b/src/integration/blockchain/bitcoin/services/__tests__/bitcoin-fee.service.spec.ts index 2ef1755f0d..0809ee1479 100644 --- a/src/integration/blockchain/bitcoin/services/__tests__/bitcoin-fee.service.spec.ts +++ b/src/integration/blockchain/bitcoin/services/__tests__/bitcoin-fee.service.spec.ts @@ -291,22 +291,38 @@ describe('BitcoinFeeService', () => { }); it('should execute in parallel', async () => { - let callCount = 0; + let inFlight = 0; + let maxInFlight = 0; + const resolvers: Array<() => void> = []; + mockClient.getMempoolEntry.mockImplementation(async () => { - callCount++; - // Simulate delay - await new Promise((resolve) => setTimeout(resolve, 10)); - return { feeRate: callCount * 10, vsize: 100 }; + inFlight++; + maxInFlight = Math.max(maxInFlight, inFlight); + await new Promise((resolve) => { + resolvers.push(resolve); + }); + inFlight--; + return { feeRate: 10, vsize: 100 }; }); - const startTime = Date.now(); const txids = ['tx1', 'tx2', 'tx3', 'tx4', 'tx5']; - await service.getTxFeeRates(txids); - const duration = Date.now() - startTime; + const resultPromise = service.getTxFeeRates(txids); + + await new Promise((resolve) => setImmediate(resolve)); + + // Sequential awaits would keep maxInFlight at 1; concurrent calls reach txids.length + try { + expect(maxInFlight).toBe(txids.length); + } finally { + // Release the calls captured so far even when the assertion fails, so a failing run + // does not leave them parked. getTxFeeRates either resolves or stays pending + // harmlessly - the assertion throws before it is awaited. + resolvers.forEach((resolve) => resolve()); + } + + const result = await resultPromise; - // If executed in parallel, total time should be ~10ms, not ~50ms - // Allow some margin for test execution overhead - expect(duration).toBeLessThan(100); + expect(result.size).toBe(5); }); it('should handle empty txid array', async () => { From 6691d8302689ed2e80eea7ff574fe514546bc3d7 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Mon, 27 Jul 2026 15:49:11 +0200 Subject: [PATCH 2/4] fix(auth): reject custodial Lightning sign-in when stored signature is empty (#3874) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- .../auth-lightning-signature.spec.ts | 54 +++++++++++++++++++ .../generic/user/models/auth/auth.service.ts | 15 +++--- 2 files changed, 61 insertions(+), 8 deletions(-) create mode 100644 src/subdomains/generic/user/models/auth/__tests__/auth-lightning-signature.spec.ts diff --git a/src/subdomains/generic/user/models/auth/__tests__/auth-lightning-signature.spec.ts b/src/subdomains/generic/user/models/auth/__tests__/auth-lightning-signature.spec.ts new file mode 100644 index 0000000000..b53c53a488 --- /dev/null +++ b/src/subdomains/generic/user/models/auth/__tests__/auth-lightning-signature.spec.ts @@ -0,0 +1,54 @@ +import { ConfigService } from 'src/config/config'; +import { AuthService } from '../auth.service'; + +// Regression guard for the custodial-Lightning sign-in bypass: an empty stored signature must never +// authenticate. verifySignature is private, so we exercise it via bracket access with a bare instance +// (the Lightning branch only uses getSignMessages + the static CryptoService.getBlockchainsBasedOn). +describe('AuthService custodial Lightning signature check', () => { + let service: AuthService; + + // LNNID + 66 alnum → recognised as a Lightning address by CryptoService + const lightningAddress = `LNNID${'A'.repeat(66)}`; + // 140 lowercase alnum → matches the custodial-Lightning signature shape + const validShapeSignature = 'a'.repeat(140); + + const verify = (signature: string, dbSignature: string | undefined, isSignUp = false): Promise => + ( + service as unknown as { + verifySignature: ( + address: string, + signature: string, + isCustodial: boolean, + key: string | undefined, + dbSignature: string | undefined, + blockchain: undefined, + isSignUp: boolean, + ) => Promise; + } + ).verifySignature(lightningAddress, signature, false, undefined, dbSignature, undefined, isSignUp); + + beforeAll(() => { + new ConfigService(); + }); + + beforeEach(() => { + service = Object.create(AuthService.prototype); + }); + + it('rejects sign-in when the stored signature is empty (account takeover guard)', async () => { + await expect(verify(validShapeSignature, '')).resolves.toBe(false); + await expect(verify(validShapeSignature, undefined)).resolves.toBe(false); + }); + + it('rejects sign-in when the signature does not match the stored one', async () => { + await expect(verify(validShapeSignature, 'b'.repeat(140))).resolves.toBe(false); + }); + + it('accepts sign-in when the signature matches a non-empty stored signature', async () => { + await expect(verify(validShapeSignature, validShapeSignature)).resolves.toBe(true); + }); + + it('accepts sign-up (establishes the first signature) even without a stored signature', async () => { + await expect(verify(validShapeSignature, undefined, true)).resolves.toBe(true); + }); +}); diff --git a/src/subdomains/generic/user/models/auth/auth.service.ts b/src/subdomains/generic/user/models/auth/auth.service.ts index 24cb2d4907..258aa075b0 100644 --- a/src/subdomains/generic/user/models/auth/auth.service.ts +++ b/src/subdomains/generic/user/models/auth/auth.service.ts @@ -40,7 +40,6 @@ import { getServiceProviderForWallet, KycType, TradeApprovalReason, UserDataStat import { UserDataService } from '../user-data/user-data.service'; import { LinkedUserInDto } from '../user/dto/linked-user.dto'; import { User } from '../user/user.entity'; -import { UserRepository } from '../user/user.repository'; import { UserService } from '../user/user.service'; import { Wallet } from '../wallet/wallet.entity'; import { WalletService } from '../wallet/wallet.service'; @@ -81,7 +80,6 @@ export class AuthService { constructor( private readonly userService: UserService, - private readonly userRepo: UserRepository, private readonly walletService: WalletService, private readonly custodyProviderService: CustodyProviderService, private readonly jwtService: JwtService, @@ -155,7 +153,7 @@ export class AuthService { const custodyProvider = await this.custodyProviderService.getWithMasterKey(dto.signature).catch(() => undefined); if ( !custodyProvider && - !(await this.verifySignature(dto.address, dto.signature, isCustodial, dto.key, undefined, dto.blockchain)) + !(await this.verifySignature(dto.address, dto.signature, isCustodial, dto.key, undefined, dto.blockchain, true)) ) { throw new BadRequestException('Invalid signature'); } @@ -249,9 +247,6 @@ export class AuthService { !(await this.verifySignature(dto.address, dto.signature, isCustodial, dto.key, user.signature, dto.blockchain)) ) { throw new UnauthorizedException('Invalid credentials'); - } else if (!user.signature) { - // TODO: temporary code to update empty signatures (remove?) - await this.userRepo.update({ address: dto.address }, { signature: dto.signature }); } } @@ -542,14 +537,18 @@ export class AuthService { key?: string, dbSignature?: string, blockchain?: Blockchain, + isSignUp = false, ): Promise { const { defaultMessage, fallbackMessage } = this.getSignMessages(address); const blockchains = CryptoService.getBlockchainsBasedOn(address); if (blockchains.includes(Blockchain.LIGHTNING) && (isCustodial || /^[a-z0-9]{140,146}$/.test(signature))) { - // custodial Lightning wallet, only comparison check - return !dbSignature || signature === dbSignature; + // custodial Lightning wallet: no cryptographic check is possible, so the signature acts as a + // shared secret. On sign-up nothing is stored yet, so the first signature establishes it; on + // sign-in it must match a NON-EMPTY stored signature. An empty stored signature must never + // authenticate — otherwise any signature passes for an account whose credential was never set. + return isSignUp || (!!dbSignature && signature === dbSignature); } if (blockchains.includes(Blockchain.DEFICHAIN)) { From 2c489d9011913f6b3b7a1d9d304501d3cba1423b Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:13:09 +0200 Subject: [PATCH 3/4] fix(storage): stabilize reconciliation scan race (#4407) --- scripts/storage/reconcile-stores.ts | 147 +++++++++++++- .../__tests__/reconcile-stores.spec.ts | 183 ++++++++++++++++++ 2 files changed, 324 insertions(+), 6 deletions(-) diff --git a/scripts/storage/reconcile-stores.ts b/scripts/storage/reconcile-stores.ts index 3d0aa222b5..433a6f0756 100644 --- a/scripts/storage/reconcile-stores.ts +++ b/scripts/storage/reconcile-stores.ts @@ -21,6 +21,12 @@ * and require a separate, not-yet-built byte-level Azure↔S3 verification before Azure * teardown (that check is not part of this tool). * + * Azure and S3 listings are not atomic. After each pair of listings, additive candidates are + * therefore re-checked directly on the allegedly missing target. A same-size target that has + * appeared meanwhile is removed as a concurrent dual-write race; a different-size target is + * promoted to the hard size-mismatch gate. This keeps live uploads enabled without weakening + * the candidate digest or allowing a write, overwrite, or delete in REPORT mode. + * * Overwrite direction is one-sided on purpose: only azure.lastModified >= s3.lastModified + * tolerance is flagged. The reverse (s3 newer than azure) is the normal backfill / dual-write * ordering case and must not be treated as an overwrite — Azure is the authoritative source. @@ -80,6 +86,7 @@ import { GetObjectCommand, GetObjectLockConfigurationCommand, + HeadObjectCommand, ListBucketsCommand, ListObjectsV2Command, PutObjectCommand, @@ -133,6 +140,11 @@ export interface DiffResult { suspectedOverwrite: string[]; } +export interface StabilizedDiffResult { + diff: DiffResult; + resolvedConcurrentCandidates: number; +} + export type HealDirection = 'azureToS3' | 's3ToAzure'; export interface DirectionSummary { @@ -693,7 +705,8 @@ export function formatReconcileReportJsonLine(report: MachineReadableReconcileRe return `RECONCILE_REPORT_JSON=${JSON.stringify(report)}`; } -// size/lastModified from list pages only — no HeadObject per key. +// Base inventory uses size/lastModified from list pages. Only keys that initially appear on +// one side are target-HEAD re-checked after listing to close the concurrent-upload race. // --- LISTING --- // export async function listS3Objects(client: S3Client, bucket: string): Promise { @@ -747,6 +760,109 @@ export async function listAzureObjects(containerClient: ContainerClient): Promis return objects; } +export function isS3NotFound(err: unknown): boolean { + const e = err as { $metadata?: { httpStatusCode?: number }; name?: string }; + return e?.$metadata?.httpStatusCode === 404 || e?.name === 'NoSuchKey' || e?.name === 'NotFound'; +} + +export function isAzureNotFound(err: unknown): boolean { + const e = err as { statusCode?: number; details?: { errorCode?: string } }; + return e?.statusCode === 404 || e?.details?.errorCode === 'BlobNotFound'; +} + +/** + * Close the open-inventory race without pausing uploads. Azure is listed before S3, so a + * successful dual write completed during the scan can look S3-only even though Azure already + * contains the object by the time the diff is built. Re-check only additive candidates on the + * allegedly missing target: + * - same size now present → concurrent appearance, remove from the additive candidate set; + * - different size now present → promote to the existing hard size-mismatch gate; + * - still absent → keep the real additive candidate. + * + * Successful target re-check metadata is added to the in-memory inventory maps so promoted + * size mismatches remain printable in detail mode. This deliberately performs no content + * download, write, overwrite, or delete. Raw keys stay private and are used only as SDK inputs / + * privacy-safe hashed error references. + */ +export async function stabilizeAdditiveCandidates( + diff: DiffResult, + azureByKey: Map, + s3ByKey: Map, + azureContainer: ContainerClient, + s3: S3Client, + container: string, +): Promise { + // Preserve the cheap one-sided-empty outage/misconfiguration guard before doing candidate + // I/O. The only bounded exception is a new container's first concurrent object (0↔1), which + // can legitimately appear between the two non-atomic listings and is safe to HEAD once. + const oneSideEmpty = (azureByKey.size === 0) !== (s3ByKey.size === 0); + if (oneSideEmpty && Math.max(azureByKey.size, s3ByKey.size) > 1) { + assertNotOneSidedEmpty(azureByKey.size, s3ByKey.size, container); + } + + const onlyOnAzure: string[] = []; + const onlyOnS3: string[] = []; + const sizeMismatch = [...diff.sizeMismatch]; + const suspectedOverwrite = [...diff.suspectedOverwrite]; + let resolvedConcurrentCandidates = 0; + + for (const key of diff.onlyOnAzure) { + const source = azureByKey.get(key); + if (!source) throw new Error(`Missing Azure inventory object for ${safeObjectReference(container, key)}`); + + try { + const target = await s3.send(new HeadObjectCommand({ Bucket: container, Key: key })); + if (target.ContentLength == null || target.LastModified == null) { + throw new Error( + `Incomplete S3 HEAD response for ${safeObjectReference(container, key)}: ` + + `ContentLength and LastModified are required`, + ); + } + s3ByKey.set(key, { key, size: target.ContentLength, lastModified: target.LastModified }); + if (target.ContentLength === source.size) { + resolvedConcurrentCandidates++; + if (source.lastModified.getTime() >= target.LastModified.getTime() + OVERWRITE_SKEW_TOLERANCE_MS) { + suspectedOverwrite.push(key); + } + } else sizeMismatch.push(key); + } catch (err) { + if (isS3NotFound(err)) onlyOnAzure.push(key); + else throw new Error(`S3 candidate re-check failed for ${safeObjectReference(container, key)}`, { cause: err }); + } + } + + for (const key of diff.onlyOnS3) { + const source = s3ByKey.get(key); + if (!source) throw new Error(`Missing S3 inventory object for ${safeObjectReference(container, key)}`); + + try { + const target = await azureContainer.getBlockBlobClient(key).getProperties(); + if (target.contentLength == null || target.lastModified == null) { + throw new Error( + `Incomplete Azure properties response for ${safeObjectReference(container, key)}: ` + + `contentLength and lastModified are required`, + ); + } + azureByKey.set(key, { key, size: target.contentLength, lastModified: target.lastModified }); + if (target.contentLength === source.size) { + resolvedConcurrentCandidates++; + if (target.lastModified.getTime() >= source.lastModified.getTime() + OVERWRITE_SKEW_TOLERANCE_MS) { + suspectedOverwrite.push(key); + } + } else sizeMismatch.push(key); + } catch (err) { + if (isAzureNotFound(err)) onlyOnS3.push(key); + else + throw new Error(`Azure candidate re-check failed for ${safeObjectReference(container, key)}`, { cause: err }); + } + } + + return { + diff: { onlyOnAzure, onlyOnS3, sizeMismatch, suspectedOverwrite }, + resolvedConcurrentCandidates, + }; +} + // --- CLIENTS / CONFIG --- // function buildS3Client(): S3Client { @@ -1289,11 +1405,18 @@ async function main(): Promise { const azureObjs = await listAzureObjects(azureContainer); const s3Objs = await listS3Objects(s3, container); - assertNotOneSidedEmpty(azureObjs.length, s3Objs.length, container); - - const diff = diffStores(azureObjs, s3Objs, OVERWRITE_SKEW_TOLERANCE_MS); const azureByKey = indexStoredObjectsByKey(azureObjs); const s3ByKey = indexStoredObjectsByKey(s3Objs); + const initialDiff = diffStores(azureObjs, s3Objs, OVERWRITE_SKEW_TOLERANCE_MS); + const { diff, resolvedConcurrentCandidates } = await stabilizeAdditiveCandidates( + initialDiff, + azureByKey, + s3ByKey, + azureContainer, + s3, + container, + ); + assertNotOneSidedEmpty(azureByKey.size, s3ByKey.size, container); reports.push({ container, diff, @@ -1309,6 +1432,7 @@ async function main(): Promise { const s3ToAzure = buildDirectionSummary(diff.onlyOnS3, s3ByKey); console.log(`\n[${container}] azure=${azureObjs.length} s3=${s3Objs.length}`); + console.log(` candidateRecheckResolved: ${resolvedConcurrentCandidates}`); printCategory(container, 'onlyOnAzure', diff.onlyOnAzure, verbose, { bytes: azureToS3.bytes, azureByKey, @@ -1434,7 +1558,17 @@ async function main(): Promise { const azureContainer = azure.getContainerClient(container); const azureObjs = await listAzureObjects(azureContainer); const s3Objs = await listS3Objects(s3, container); - const diff = diffStores(azureObjs, s3Objs, OVERWRITE_SKEW_TOLERANCE_MS); + const azureByKey = indexStoredObjectsByKey(azureObjs); + const s3ByKey = indexStoredObjectsByKey(s3Objs); + const initialDiff = diffStores(azureObjs, s3Objs, OVERWRITE_SKEW_TOLERANCE_MS); + const { diff, resolvedConcurrentCandidates } = await stabilizeAdditiveCandidates( + initialDiff, + azureByKey, + s3ByKey, + azureContainer, + s3, + container, + ); if (diff.onlyOnAzure.length > 0 || diff.onlyOnS3.length > 0) { throw new Error( @@ -1448,7 +1582,8 @@ async function main(): Promise { console.log( `\n[post-heal ${container}] sizeMismatch=${diff.sizeMismatch.length} ` + - `suspectedOverwrite=${diff.suspectedOverwrite.length}`, + `suspectedOverwrite=${diff.suspectedOverwrite.length} ` + + `candidateRecheckResolved=${resolvedConcurrentCandidates}`, ); } catch (e) { throw new Error(`[container="${container}"] ${e?.message ?? e}`, { cause: e }); diff --git a/src/integration/infrastructure/storage/__tests__/reconcile-stores.spec.ts b/src/integration/infrastructure/storage/__tests__/reconcile-stores.spec.ts index cf7cde908c..a9b22c2f25 100644 --- a/src/integration/infrastructure/storage/__tests__/reconcile-stores.spec.ts +++ b/src/integration/infrastructure/storage/__tests__/reconcile-stores.spec.ts @@ -1,6 +1,7 @@ import { GetObjectCommand, GetObjectLockConfigurationCommand, + HeadObjectCommand, ListObjectsV2Command, PutObjectCommand, S3Client, @@ -45,6 +46,7 @@ import { RECONCILER_PRIVACY_LOG_VERSION, runAdditiveHealOrchestration, safeObjectReference, + stabilizeAdditiveCandidates, StoredObject, } from '../../../../../scripts/storage/reconcile-stores'; import { GEBUEV_RETENTION_FLOOR_DAYS, GEBUEV_RETENTION_FLOOR_YEARS } from '../worm-retention.const'; @@ -302,6 +304,187 @@ describe('diffStores', () => { }); }); +describe('stabilizeAdditiveCandidates', () => { + beforeEach(() => { + s3Mock.reset(); + }); + + it('removes an Azure-only candidate that appeared concurrently on S3 with the same size', async () => { + const key = SENTINEL_KEY_A; + const azureByKey = indexStoredObjectsByKey([storedObject(key, 42, t0)]); + s3Mock.on(HeadObjectCommand, { Bucket: 'kyc', Key: key }).resolves({ ContentLength: 42, LastModified: t0 }); + const s3ByKey = new Map(); + + const result = await stabilizeAdditiveCandidates( + { onlyOnAzure: [key], onlyOnS3: [], sizeMismatch: [], suspectedOverwrite: [] }, + azureByKey, + s3ByKey, + {} as never, + makeS3Client(), + 'kyc', + ); + + expect(result.diff.onlyOnAzure).toEqual([]); + expect(result.diff.sizeMismatch).toEqual([]); + expect(result.resolvedConcurrentCandidates).toBe(1); + expect(s3ByKey.get(key)).toEqual(storedObject(key, 42, t0)); + expect(() => assertNotOneSidedEmpty(azureByKey.size, s3ByKey.size, 'kyc')).not.toThrow(); + }); + + it('removes an S3-only candidate that appeared concurrently on Azure with the same size', async () => { + const key = SENTINEL_KEY_B; + const s3ByKey = indexStoredObjectsByKey([storedObject(key, 43, t0)]); + const getProperties = jest.fn().mockResolvedValue({ contentLength: 43, lastModified: t0 }); + const azureContainer = { getBlockBlobClient: jest.fn().mockReturnValue({ getProperties }) } as never; + const azureByKey = new Map(); + + const result = await stabilizeAdditiveCandidates( + { onlyOnAzure: [], onlyOnS3: [key], sizeMismatch: [], suspectedOverwrite: [] }, + azureByKey, + s3ByKey, + azureContainer, + makeS3Client(), + 'kyc', + ); + + expect(result.diff.onlyOnS3).toEqual([]); + expect(result.diff.sizeMismatch).toEqual([]); + expect(result.resolvedConcurrentCandidates).toBe(1); + expect(azureByKey.get(key)).toEqual(storedObject(key, 43, t0)); + expect(() => assertNotOneSidedEmpty(azureByKey.size, s3ByKey.size, 'kyc')).not.toThrow(); + }); + + it('preserves the overwrite timestamp heuristic for concurrently appeared targets', async () => { + const azureKey = SENTINEL_KEY_A; + const s3Key = SENTINEL_KEY_B; + const oldTime = t0; + const newTime = new Date(t0.getTime() + OVERWRITE_SKEW_TOLERANCE_MS); + const azureByKey = indexStoredObjectsByKey([storedObject(azureKey, 42, newTime)]); + const s3ByKey = indexStoredObjectsByKey([storedObject(s3Key, 43, oldTime)]); + s3Mock + .on(HeadObjectCommand, { Bucket: 'kyc', Key: azureKey }) + .resolves({ ContentLength: 42, LastModified: oldTime }); + const azureContainer = { + getBlockBlobClient: jest.fn().mockReturnValue({ + getProperties: jest.fn().mockResolvedValue({ contentLength: 43, lastModified: newTime }), + }), + } as never; + + const result = await stabilizeAdditiveCandidates( + { onlyOnAzure: [azureKey], onlyOnS3: [s3Key], sizeMismatch: [], suspectedOverwrite: ['existing'] }, + azureByKey, + s3ByKey, + azureContainer, + makeS3Client(), + 'kyc', + ); + + expect(result.diff.onlyOnAzure).toEqual([]); + expect(result.diff.onlyOnS3).toEqual([]); + expect(result.diff.suspectedOverwrite).toEqual(['existing', azureKey, s3Key]); + expect(result.resolvedConcurrentCandidates).toBe(2); + }); + + it('keeps candidates whose target is still absent', async () => { + const azureKey = SENTINEL_KEY_A; + const s3Key = SENTINEL_KEY_B; + const azureByKey = indexStoredObjectsByKey([storedObject(azureKey, 42, t0)]); + const s3ByKey = indexStoredObjectsByKey([storedObject(s3Key, 43, t0)]); + s3Mock.on(HeadObjectCommand).rejects(Object.assign(new Error('not found'), { name: 'NotFound' })); + const azureContainer = { + getBlockBlobClient: jest.fn().mockReturnValue({ + getProperties: jest.fn().mockRejectedValue({ statusCode: 404, details: { errorCode: 'BlobNotFound' } }), + }), + } as never; + + const result = await stabilizeAdditiveCandidates( + { onlyOnAzure: [azureKey], onlyOnS3: [s3Key], sizeMismatch: [], suspectedOverwrite: [] }, + azureByKey, + s3ByKey, + azureContainer, + makeS3Client(), + 'kyc', + ); + + expect(result.diff.onlyOnAzure).toEqual([azureKey]); + expect(result.diff.onlyOnS3).toEqual([s3Key]); + expect(result.resolvedConcurrentCandidates).toBe(0); + }); + + it('rejects a large one-sided-empty inventory before any target re-check request', async () => { + const azureKeys = [SENTINEL_KEY_A, SENTINEL_KEY_B]; + const s3Keys = [SENTINEL_KEY_B, SENTINEL_KEY_C]; + const getProperties = jest.fn(); + const getBlockBlobClient = jest.fn().mockReturnValue({ getProperties }); + const azureContainer = { getBlockBlobClient } as never; + + await expect( + stabilizeAdditiveCandidates( + { onlyOnAzure: azureKeys, onlyOnS3: [], sizeMismatch: [], suspectedOverwrite: [] }, + indexStoredObjectsByKey(azureKeys.map((key) => storedObject(key, 42, t0))), + new Map(), + azureContainer, + makeS3Client(), + 'kyc', + ), + ).rejects.toThrow(/One-sided empty inventory/); + expect(s3Mock.commandCalls(HeadObjectCommand)).toHaveLength(0); + + await expect( + stabilizeAdditiveCandidates( + { onlyOnAzure: [], onlyOnS3: s3Keys, sizeMismatch: [], suspectedOverwrite: [] }, + new Map(), + indexStoredObjectsByKey(s3Keys.map((key) => storedObject(key, 43, t0))), + azureContainer, + makeS3Client(), + 'kyc', + ), + ).rejects.toThrow(/One-sided empty inventory/); + expect(getBlockBlobClient).not.toHaveBeenCalled(); + expect(getProperties).not.toHaveBeenCalled(); + }); + + it('promotes a concurrently appeared target with a different size to sizeMismatch', async () => { + const azureKey = SENTINEL_KEY_A; + const s3Key = SENTINEL_KEY_B; + const azureByKey = indexStoredObjectsByKey([storedObject(azureKey, 42, t0)]); + const s3ByKey = indexStoredObjectsByKey([storedObject(s3Key, 43, t0)]); + s3Mock.on(HeadObjectCommand, { Bucket: 'kyc', Key: azureKey }).resolves({ ContentLength: 99, LastModified: t0 }); + const azureContainer = { + getBlockBlobClient: jest + .fn() + .mockReturnValue({ getProperties: jest.fn().mockResolvedValue({ contentLength: 98, lastModified: t0 }) }), + } as never; + + const result = await stabilizeAdditiveCandidates( + { onlyOnAzure: [azureKey], onlyOnS3: [s3Key], sizeMismatch: ['existing'], suspectedOverwrite: [] }, + azureByKey, + s3ByKey, + azureContainer, + makeS3Client(), + 'kyc', + ); + + expect(result.diff.onlyOnAzure).toEqual([]); + expect(result.diff.onlyOnS3).toEqual([]); + expect(result.diff.sizeMismatch).toEqual(['existing', azureKey, s3Key]); + expect(result.resolvedConcurrentCandidates).toBe(0); + expect(s3ByKey.get(azureKey)).toEqual(storedObject(azureKey, 99, t0)); + expect(azureByKey.get(s3Key)).toEqual(storedObject(s3Key, 98, t0)); + + const consoleSpy = jest.spyOn(console, 'log').mockImplementation(); + expect(() => + printCategory('kyc', 'sizeMismatch', [azureKey, s3Key], true, { + azureByKey, + s3ByKey, + dualSide: true, + }), + ).not.toThrow(); + const output = consoleSpy.mock.calls.flat().join('\n'); + assertNoSentinelLeak(output); + }); +}); + describe('isGateBlocking', () => { it('returns false for an empty diff', () => { const diff: DiffResult = { From fd37a03d38af4e9b3559b65c29ffce4f86ecf4e6 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:43:49 +0200 Subject: [PATCH 4/4] feat(custody): grant, change and revoke access to a Safe (#4401) * 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. --- ...00000000-AddCustodyAccountAccessHistory.js | 73 ++++ .../controllers/custody-account.controller.ts | 119 +++++- .../create-custody-account-access.dto.ts | 18 + .../update-custody-account-access.dto.ts | 9 + .../entities/custody-account-access.entity.ts | 23 +- .../guards/custody-account-access.guard.ts | 38 +- .../mappers/custody-account-dto.mapper.ts | 11 +- .../services/custody-account.service.ts | 386 ++++++++++++++++-- src/subdomains/generic/gs/dto/gs.dto.ts | 2 +- 9 files changed, 616 insertions(+), 63 deletions(-) create mode 100644 migration/1785100000000-AddCustodyAccountAccessHistory.js create mode 100644 src/subdomains/core/custody/dto/input/create-custody-account-access.dto.ts create mode 100644 src/subdomains/core/custody/dto/input/update-custody-account-access.dto.ts diff --git a/migration/1785100000000-AddCustodyAccountAccessHistory.js b/migration/1785100000000-AddCustodyAccountAccessHistory.js new file mode 100644 index 0000000000..b6f7d1332f --- /dev/null +++ b/migration/1785100000000-AddCustodyAccountAccessHistory.js @@ -0,0 +1,73 @@ +/** + * @typedef {import('typeorm').MigrationInterface} MigrationInterface + * @typedef {import('typeorm').QueryRunner} QueryRunner + */ + +/** + * Adds supersede-with-history columns to `custody_account_access` so grant changes stay + * reconstructible (active + deactivatedAt). Replaces the full unique index on + * (accountId, userDataId) with a partial unique index that only applies to active rows, + * so a second grant after revocation (or a level change that inserts a new active row) + * is possible while inactive history is kept. + * + * Constraint/index names are TypeORM's deterministic DefaultNamingStrategy values — + * ` + sha1(table + '_' + columns.sort().join('_') [+ '_' + where])` truncated to + * 26 hex chars for IDX_ — so a future `migration:generate` detects no drift against the + * entity's @Index decorator. + * + * Existing rows are backfilled as active via the column DEFAULT (no separate UPDATE). + * + * @class + * @implements {MigrationInterface} + */ +module.exports = class AddCustodyAccountAccessHistory1785100000000 { + name = 'AddCustodyAccountAccessHistory1785100000000'; + + /** + * @param {QueryRunner} queryRunner + */ + async up(queryRunner) { + await queryRunner.query(`ALTER TABLE "custody_account_access" ADD "active" boolean NOT NULL DEFAULT true`); + await queryRunner.query(`ALTER TABLE "custody_account_access" ADD "deactivatedAt" TIMESTAMP`); + // Drop the full unique index so historical (inactive) rows may share (accountId, userDataId). + await queryRunner.query(`DROP INDEX "public"."IDX_380e225bfd7707fff0e4f98035"`); + // Partial unique: at most one active grant per (account, grantee). + await queryRunner.query( + `CREATE UNIQUE INDEX "IDX_aab22f509e4cf0a1856adefa45" ON "custody_account_access" ("accountId", "userDataId") WHERE "active" = true`, + ); + } + + /** + * @param {QueryRunner} queryRunner + */ + async down(queryRunner) { + // ACCESS EXCLUSIVE first: the refusal decision and the schema change must see the same + // table. Without the lock a concurrent revoke can deactivate a grant after the count + // returns 0 and before we drop `active`, resurrecting that grant as if it were live. + // TypeORM runs this migration in a transaction, so the lock is held until commit/rollback. + await queryRunner.query(`LOCK TABLE "custody_account_access" IN ACCESS EXCLUSIVE MODE`); + + // Refuse when inactive history exists: both consolidations are lossy (delete history, or + // resurrect revoked grants after dropping `active`). Count first so a refused rollback + // leaves the schema completely untouched. + const inactiveCount = ( + await queryRunner.query(`SELECT COUNT(*)::int AS count FROM "custody_account_access" WHERE "active" = false`) + ).at(0).count; + + if (inactiveCount > 0) { + throw new Error( + `Cannot roll back AddCustodyAccountAccessHistory: ${inactiveCount} inactive custody_account_access row(s) exist. ` + + 'Both consolidations are lossy — deleting inactive rows destroys grant history; keeping them and ' + + 'recreating the full unique index would resurrect revoked grants once the active column is gone. ' + + 'Archive or consolidate custody_account_access deliberately, then run the rollback again.', + ); + } + + await queryRunner.query(`DROP INDEX "public"."IDX_aab22f509e4cf0a1856adefa45"`); + await queryRunner.query( + `CREATE UNIQUE INDEX "IDX_380e225bfd7707fff0e4f98035" ON "custody_account_access" ("accountId", "userDataId")`, + ); + await queryRunner.query(`ALTER TABLE "custody_account_access" DROP COLUMN "deactivatedAt"`); + await queryRunner.query(`ALTER TABLE "custody_account_access" DROP COLUMN "active"`); + } +}; diff --git a/src/subdomains/core/custody/controllers/custody-account.controller.ts b/src/subdomains/core/custody/controllers/custody-account.controller.ts index d15699d560..45cc87ff8d 100644 --- a/src/subdomains/core/custody/controllers/custody-account.controller.ts +++ b/src/subdomains/core/custody/controllers/custody-account.controller.ts @@ -1,4 +1,15 @@ -import { Body, Controller, Get, NotFoundException, Param, Post, Put, UseGuards } from '@nestjs/common'; +import { + BadRequestException, + Body, + Controller, + Delete, + Get, + NotFoundException, + Param, + Post, + Put, + UseGuards, +} from '@nestjs/common'; import { AuthGuard } from '@nestjs/passport'; import { ApiBearerAuth, ApiCreatedResponse, ApiOkResponse, ApiTags } from '@nestjs/swagger'; import { GetJwt } from 'src/shared/auth/get-jwt.decorator'; @@ -6,13 +17,20 @@ import { JwtPayload } from 'src/shared/auth/jwt-payload.interface'; import { RoleGuard } from 'src/shared/auth/role.guard'; import { UserActiveGuard } from 'src/shared/auth/user-active.guard'; import { UserRole } from 'src/shared/auth/user-role.enum'; +import { CreateCustodyAccountAccessDto } from '../dto/input/create-custody-account-access.dto'; import { CreateCustodyAccountDto } from '../dto/input/create-custody-account.dto'; +import { UpdateCustodyAccountAccessDto } from '../dto/input/update-custody-account-access.dto'; import { UpdateCustodyAccountDto } from '../dto/input/update-custody-account.dto'; import { CustodyAccountAccessDto, CustodyAccountDto } from '../dto/output/custody-account.dto'; import { CustodyAccessLevel } from '../enums/custody'; import { CustodyAccountReadGuard, CustodyAccountWriteGuard } from '../guards/custody-account-access.guard'; import { CustodyAccountDtoMapper } from '../mappers/custody-account-dto.mapper'; -import { CustodyAccountService, LegacyAccountId } from '../services/custody-account.service'; +import { + CustodyAccountId, + CustodyAccountService, + LegacyAccountId, + PG_INTEGER_MAX, +} from '../services/custody-account.service'; @ApiTags('Custody') @Controller('custody/account') @@ -35,7 +53,9 @@ export class CustodyAccountController { const custodyAccounts = await this.custodyAccountService.getCustodyAccountsForUser(jwt.account); const isLegacy = id === LegacyAccountId; - const account = isLegacy ? custodyAccounts.find((ca) => ca.isLegacy) : custodyAccounts.find((ca) => ca.id === +id); + const account = isLegacy + ? custodyAccounts.find((ca) => ca.isLegacy) + : custodyAccounts.find((ca) => ca.id === this.parsePositiveIntParam(id, 'custody account ID')); if (!account) throw new NotFoundException(`${isLegacy ? 'Legacy' : 'Custody'} account not found`); return account; @@ -68,7 +88,7 @@ export class CustodyAccountController { @Body() dto: UpdateCustodyAccountDto, ): Promise { const custodyAccount = await this.custodyAccountService.updateCustodyAccount( - +id, + this.parsePositiveIntParam(id, 'custody account ID'), jwt.account, dto.title, dto.description, @@ -79,15 +99,92 @@ export class CustodyAccountController { @Get(':id/access') @ApiBearerAuth() - @UseGuards(AuthGuard(), RoleGuard(UserRole.ACCOUNT), UserActiveGuard(), CustodyAccountReadGuard) + @UseGuards(AuthGuard(), RoleGuard(UserRole.ACCOUNT), UserActiveGuard()) @ApiOkResponse({ type: [CustodyAccountAccessDto], description: 'List of users with access' }) async getAccessList(@GetJwt() jwt: JwtPayload, @Param('id') id: string): Promise { - const accessList = await this.custodyAccountService.getAccessList(+id, jwt.account); + const accessList = await this.custodyAccountService.getAccessList( + this.parsePositiveIntParam(id, 'custody account ID'), + jwt.account, + ); + + return accessList.map(CustodyAccountDtoMapper.toAccessDto); + } + + @Post(':id/access') + @ApiBearerAuth() + @UseGuards(AuthGuard(), RoleGuard(UserRole.ACCOUNT), UserActiveGuard()) + @ApiCreatedResponse({ type: CustodyAccountAccessDto, description: 'Create access grant' }) + async grantAccess( + @GetJwt() jwt: JwtPayload, + @Param('id') id: string, + @Body() dto: CreateCustodyAccountAccessDto, + ): Promise { + const custodyAccountId = this.parseCustodyAccountId(id); + const access = await this.custodyAccountService.grantAccess( + custodyAccountId, + jwt.account, + dto.mail, + dto.accessLevel, + ); + + return CustodyAccountDtoMapper.toAccessDto(access); + } + + @Put(':id/access/:accessId') + @ApiBearerAuth() + @UseGuards(AuthGuard(), RoleGuard(UserRole.ACCOUNT), UserActiveGuard()) + @ApiOkResponse({ type: CustodyAccountAccessDto, description: 'Update access grant' }) + async updateAccess( + @GetJwt() jwt: JwtPayload, + @Param('id') id: string, + @Param('accessId') accessId: string, + @Body() dto: UpdateCustodyAccountAccessDto, + ): Promise { + const access = await this.custodyAccountService.updateAccess( + this.parsePositiveIntParam(id, 'custody account ID'), + this.parsePositiveIntParam(accessId, 'access ID'), + jwt.account, + dto.accessLevel, + ); + + return CustodyAccountDtoMapper.toAccessDto(access); + } + + @Delete(':id/access/:accessId') + @ApiBearerAuth() + @UseGuards(AuthGuard(), RoleGuard(UserRole.ACCOUNT), UserActiveGuard()) + @ApiOkResponse({ description: 'Revoke access grant' }) + async revokeAccess( + @GetJwt() jwt: JwtPayload, + @Param('id') id: string, + @Param('accessId') accessId: string, + ): Promise { + await this.custodyAccountService.revokeAccess( + this.parsePositiveIntParam(id, 'custody account ID'), + this.parsePositiveIntParam(accessId, 'access ID'), + jwt.account, + ); + } + + private parseCustodyAccountId(id: string): CustodyAccountId { + if (id === LegacyAccountId) return LegacyAccountId; + return this.parsePositiveIntParam(id, 'custody account ID'); + } + + /** + * Finite, safe, positive integer within the Postgres INTEGER/SERIAL range. + * Rejects non-digits, Infinity (e.g. 309 nines), zero, and values outside column range → 400. + */ + private parsePositiveIntParam(value: string, name: string): number { + if (!/^\d+$/.test(value)) { + throw new BadRequestException(`Invalid ${name}`); + } + + const n = Number(value); + if (!Number.isSafeInteger(n) || n < 1 || n > PG_INTEGER_MAX) { + throw new BadRequestException(`Invalid ${name}`); + } - return accessList.map((access) => ({ - id: access.id, - user: { id: access.userData.id }, - accessLevel: access.accessLevel, - })); + return n; } } diff --git a/src/subdomains/core/custody/dto/input/create-custody-account-access.dto.ts b/src/subdomains/core/custody/dto/input/create-custody-account-access.dto.ts new file mode 100644 index 0000000000..cdf8ed4339 --- /dev/null +++ b/src/subdomains/core/custody/dto/input/create-custody-account-access.dto.ts @@ -0,0 +1,18 @@ +import { ApiProperty } from '@nestjs/swagger'; +import { Transform } from 'class-transformer'; +import { IsEmail, IsEnum, IsNotEmpty, IsString } from 'class-validator'; +import { CustodyAccessLevel } from '../../enums/custody'; + +export class CreateCustodyAccountAccessDto { + @ApiProperty({ description: 'E-mail of the user to grant access to' }) + @IsNotEmpty() + @IsString() + @IsEmail() + // Leave non-strings untouched so IsString/IsEmail can reject with 400 instead of a 500 TypeError. + @Transform(({ value }) => (typeof value === 'string' ? value.trim().toLowerCase() : value)) + mail: string; + + @ApiProperty({ enum: CustodyAccessLevel, description: 'Access level to grant' }) + @IsEnum(CustodyAccessLevel) + accessLevel: CustodyAccessLevel; +} diff --git a/src/subdomains/core/custody/dto/input/update-custody-account-access.dto.ts b/src/subdomains/core/custody/dto/input/update-custody-account-access.dto.ts new file mode 100644 index 0000000000..20d8a515eb --- /dev/null +++ b/src/subdomains/core/custody/dto/input/update-custody-account-access.dto.ts @@ -0,0 +1,9 @@ +import { ApiProperty } from '@nestjs/swagger'; +import { IsEnum } from 'class-validator'; +import { CustodyAccessLevel } from '../../enums/custody'; + +export class UpdateCustodyAccountAccessDto { + @ApiProperty({ enum: CustodyAccessLevel, description: 'New access level' }) + @IsEnum(CustodyAccessLevel) + accessLevel: CustodyAccessLevel; +} diff --git a/src/subdomains/core/custody/entities/custody-account-access.entity.ts b/src/subdomains/core/custody/entities/custody-account-access.entity.ts index 63b912e051..85b9bd5f95 100644 --- a/src/subdomains/core/custody/entities/custody-account-access.entity.ts +++ b/src/subdomains/core/custody/entities/custody-account-access.entity.ts @@ -1,11 +1,12 @@ -import { IEntity } from 'src/shared/models/entity'; +import { IEntity, UpdateResult } from 'src/shared/models/entity'; import { UserData } from 'src/subdomains/generic/user/models/user-data/user-data.entity'; import { Column, Entity, Index, ManyToOne } from 'typeorm'; import { CustodyAccessLevel } from '../enums/custody'; import { CustodyAccount } from './custody-account.entity'; @Entity() -@Index((a: CustodyAccountAccess) => [a.account, a.userData], { unique: true }) +// One active grant per (account, userData); historical rows stay with active = false. +@Index((a: CustodyAccountAccess) => [a.account, a.userData], { unique: true, where: '"active" = true' }) export class CustodyAccountAccess extends IEntity { @Index() @ManyToOne(() => CustodyAccount, (custodyAccount) => custodyAccount.accessGrants, { nullable: false }) @@ -17,4 +18,22 @@ export class CustodyAccountAccess extends IEntity { @Column() accessLevel: CustodyAccessLevel; + + @Column({ default: true }) + active: boolean; + + @Column({ type: 'timestamp', nullable: true }) + deactivatedAt?: Date; + + /** Marks this grant as historical so a new active row can supersede it. */ + deactivate(): UpdateResult { + const update: Partial = { + active: false, + deactivatedAt: new Date(), + }; + + Object.assign(this, update); + + return [this.id, update]; + } } diff --git a/src/subdomains/core/custody/guards/custody-account-access.guard.ts b/src/subdomains/core/custody/guards/custody-account-access.guard.ts index d8ead9b53f..aa76ea5848 100644 --- a/src/subdomains/core/custody/guards/custody-account-access.guard.ts +++ b/src/subdomains/core/custody/guards/custody-account-access.guard.ts @@ -1,6 +1,11 @@ -import { CanActivate, ExecutionContext, ForbiddenException, Injectable } from '@nestjs/common'; +import { CanActivate, ExecutionContext, ForbiddenException, HttpException, Injectable } from '@nestjs/common'; import { CustodyAccessLevel } from '../enums/custody'; -import { CustodyAccountId, CustodyAccountService, LegacyAccountId } from '../services/custody-account.service'; +import { + CustodyAccountId, + CustodyAccountService, + LegacyAccountId, + PG_INTEGER_MAX, +} from '../services/custody-account.service'; abstract class CustodyAccountAccessGuard implements CanActivate { protected abstract readonly requiredLevel: CustodyAccessLevel; @@ -20,18 +25,31 @@ abstract class CustodyAccountAccessGuard implements CanActivate { await this.custodyAccountService.checkAccess(custodyAccountId, accountId, this.requiredLevel); return true; } catch (error) { - throw new ForbiddenException(error.message || 'Access denied'); + // Only translate HTTP errors (e.g. 404 from checkAccess) into 403 to prevent account + // enumeration. Programming errors must surface as 500, not look like access denied. + if (error instanceof HttpException) { + throw new ForbiddenException(error.message || 'Access denied'); + } + throw error; } } - private getCustodyAccountId(request: any): CustodyAccountId { + private getCustodyAccountId(request: { params?: Record }): CustodyAccountId { const id = request.params?.custodyAccountId || request.params?.id; if (id == null) throw new ForbiddenException('Custody account ID required'); if (id === LegacyAccountId) return id; - const parsed = +id; - if (isNaN(parsed)) throw new ForbiddenException('Invalid custody account ID'); + // Same constraints as the controller: digits only, finite safe positive int in SERIAL range. + // (Guards map validation failures to 403; the controller answers 400 on grant routes.) + if (!/^\d+$/.test(id)) { + throw new ForbiddenException('Invalid custody account ID'); + } + + const parsed = Number(id); + if (!Number.isSafeInteger(parsed) || parsed < 1 || parsed > PG_INTEGER_MAX) { + throw new ForbiddenException('Invalid custody account ID'); + } return parsed; } @@ -40,9 +58,17 @@ abstract class CustodyAccountAccessGuard implements CanActivate { @Injectable() export class CustodyAccountReadGuard extends CustodyAccountAccessGuard { protected readonly requiredLevel = CustodyAccessLevel.READ; + + constructor(custodyAccountService: CustodyAccountService) { + super(custodyAccountService); + } } @Injectable() export class CustodyAccountWriteGuard extends CustodyAccountAccessGuard { protected readonly requiredLevel = CustodyAccessLevel.WRITE; + + constructor(custodyAccountService: CustodyAccountService) { + super(custodyAccountService); + } } diff --git a/src/subdomains/core/custody/mappers/custody-account-dto.mapper.ts b/src/subdomains/core/custody/mappers/custody-account-dto.mapper.ts index 092f3a8042..225203f1b3 100644 --- a/src/subdomains/core/custody/mappers/custody-account-dto.mapper.ts +++ b/src/subdomains/core/custody/mappers/custody-account-dto.mapper.ts @@ -1,5 +1,6 @@ import { UserData } from 'src/subdomains/generic/user/models/user-data/user-data.entity'; -import { CustodyAccountDto } from '../dto/output/custody-account.dto'; +import { CustodyAccountAccessDto, CustodyAccountDto } from '../dto/output/custody-account.dto'; +import { CustodyAccountAccess } from '../entities/custody-account-access.entity'; import { CustodyAccount } from '../entities/custody-account.entity'; import { CustodyAccessLevel } from '../enums/custody'; @@ -25,4 +26,12 @@ export class CustodyAccountDtoMapper { owner: { id: userData.id }, }; } + + static toAccessDto(access: CustodyAccountAccess): CustodyAccountAccessDto { + return { + id: access.id, + user: { id: access.userData.id }, + accessLevel: access.accessLevel, + }; + } } diff --git a/src/subdomains/core/custody/services/custody-account.service.ts b/src/subdomains/core/custody/services/custody-account.service.ts index 2185838be4..e7087673cd 100644 --- a/src/subdomains/core/custody/services/custody-account.service.ts +++ b/src/subdomains/core/custody/services/custody-account.service.ts @@ -1,6 +1,14 @@ -import { ForbiddenException, Injectable, NotFoundException } from '@nestjs/common'; +import { + BadRequestException, + ConflictException, + ForbiddenException, + Injectable, + NotFoundException, +} from '@nestjs/common'; import { UserRole } from 'src/shared/auth/user-role.enum'; +import { UserData } from 'src/subdomains/generic/user/models/user-data/user-data.entity'; import { UserDataService } from 'src/subdomains/generic/user/models/user-data/user-data.service'; +import { EntityManager } from 'typeorm'; import { CustodyAccountDto } from '../dto/output/custody-account.dto'; import { CustodyAccountAccess } from '../entities/custody-account-access.entity'; import { CustodyAccount } from '../entities/custody-account.entity'; @@ -12,6 +20,22 @@ import { CustodyAccountRepository } from '../repositories/custody-account.reposi export const LegacyAccountId = 'legacy'; export type CustodyAccountId = number | typeof LegacyAccountId; +/** Postgres INTEGER / SERIAL upper bound (positive ids only). */ +export const PG_INTEGER_MAX = 2_147_483_647; + +/** + * Owner-scoped advisory lock key for ordinary creation vs legacy materialisation. + * Must stay identical everywhere — a second key scheme would re-open races. + */ +function custodyLegacyMaterializeLockKey(ownerAccountId: number): string { + return `custody-legacy-materialize:${ownerAccountId}`; +} + +/** Transaction-scoped advisory lock serialising concurrent creation and legacy materialisations. */ +async function acquireCustodyLegacyMaterializeLock(manager: EntityManager, ownerAccountId: number): Promise { + await manager.query('SELECT pg_advisory_xact_lock(hashtext($1))', [custodyLegacyMaterializeLockKey(ownerAccountId)]); +} + @Injectable() export class CustodyAccountService { constructor( @@ -25,40 +49,49 @@ export class CustodyAccountService { const account = await this.userDataService.getUserData(accountId, { users: true, custodyAccounts: true, - custodyAccountAccesses: { account: { owner: true } }, }); if (!account) throw new NotFoundException('User not found'); - // owned accounts - const ownedAccounts = (account.custodyAccounts ?? []).filter((ca) => ca.status === CustodyAccountStatus.ACTIVE); + // owned accounts (active only for the list) + const allOwnedAccounts = account.custodyAccounts ?? []; + const ownedAccounts = allOwnedAccounts.filter((ca) => ca.status === CustodyAccountStatus.ACTIVE); - // shared accounts (via access grants, excluding owned) - const sharedAccounts = (account.custodyAccountAccesses ?? []) - .filter((a) => a.account.status === CustodyAccountStatus.ACTIVE) - .filter((a) => a.account.owner.id !== accountId); + // shared accounts via active grants only (history filtered in SQL, not JS) + const activeSharedGrants = await this.custodyAccountAccessRepo.find({ + where: { + userData: { id: accountId }, + active: true, + account: { status: CustodyAccountStatus.ACTIVE }, + }, + relations: { account: { owner: true } }, + }); + const sharedAccounts = activeSharedGrants.filter((a) => a.account.owner.id !== accountId); const custodyAccounts: CustodyAccountDto[] = [ ...ownedAccounts.map((ca) => CustodyAccountDtoMapper.toDto(ca, CustodyAccessLevel.WRITE)), ...sharedAccounts.map((a) => CustodyAccountDtoMapper.toDto(a.account, a.accessLevel)), ]; - if (custodyAccounts.length > 0) { - return custodyAccounts; - } - - // fallback to legacy custody account - const hasCustody = account.users.some((u) => u.role === UserRole.CUSTODY); - if (hasCustody) { - return [CustodyAccountDtoMapper.toLegacyDto(account)]; + // Legacy Safe = absence of any owned account row; independent of shared grants. + if (allOwnedAccounts.length === 0) { + const hasCustody = account.users.some((u) => u.role === UserRole.CUSTODY); + if (hasCustody) { + custodyAccounts.push(CustodyAccountDtoMapper.toLegacyDto(account)); + } } - return []; + return custodyAccounts; } + /** + * Resolves an account for authorisation. Only ACTIVE accounts are visible — + * Blocked/Closed are treated as missing so status cannot be bypassed via id. + * Shared by checkAccess and requireOwner so every auth path is covered once. + */ async getCustodyAccountById(custodyAccountId: number): Promise { const custodyAccount = await this.custodyAccountRepo.findOne({ - where: { id: custodyAccountId }, - relations: { owner: true, accessGrants: { userData: true } }, + where: { id: custodyAccountId, status: CustodyAccountStatus.ACTIVE }, + relations: { owner: true }, }); if (!custodyAccount) throw new NotFoundException('Custody account not found'); @@ -72,8 +105,13 @@ export class CustodyAccountService { accountId: number, requiredLevel: CustodyAccessLevel, ): Promise<{ custodyAccount: CustodyAccount | null; isLegacy: boolean }> { - // Legacy mode + // Legacy mode — same entitlement as listing / grantAccessForLegacy (CUSTODY user + no owned rows). + // Both failures map to NotFound so the alias cannot be used for enumeration or after materialisation. if (custodyAccountId === LegacyAccountId) { + const { hasCustody, ownedCount } = await this.loadLegacyOwner(accountId); + if (!hasCustody || ownedCount > 0) { + throw new NotFoundException('Legacy account not found'); + } if (requiredLevel === CustodyAccessLevel.WRITE) { throw new ForbiddenException('Cannot modify legacy account'); } @@ -87,8 +125,14 @@ export class CustodyAccountService { return { custodyAccount, isLegacy: false }; } - // Check access grants - const access = custodyAccount.accessGrants.find((a) => a.userData.id === accountId); + // Active grant only — inactive history must not participate in authorisation + const access = await this.custodyAccountAccessRepo.findOne({ + where: { + account: { id: custodyAccountId }, + userData: { id: accountId }, + active: true, + }, + }); if (!access) { throw new ForbiddenException('No access to this custody account'); } @@ -105,25 +149,12 @@ export class CustodyAccountService { async createCustodyAccount(accountId: number, title: string, description?: string): Promise { const owner = await this.userDataService.getActiveUserData(accountId); - const custodyAccount = this.custodyAccountRepo.create({ - title, - description, - owner, - status: CustodyAccountStatus.ACTIVE, - requiredSignatures: 1, - }); - - const saved = await this.custodyAccountRepo.save(custodyAccount); - - // Create WRITE access for owner - const ownerAccess = this.custodyAccountAccessRepo.create({ - account: saved, - userData: owner, - accessLevel: CustodyAccessLevel.WRITE, + // Same owner-scoped lock as grantAccessForLegacy so ordinary create cannot race + // materialisation (check-zero → insert) and leave two accounts + a legacy grant. + return this.custodyAccountRepo.manager.transaction(async (manager) => { + await acquireCustodyLegacyMaterializeLock(manager, accountId); + return this.persistCustodyAccount(manager, owner, title, description); }); - await this.custodyAccountAccessRepo.save(ownerAccess); - - return saved; } // --- UPDATE --- // @@ -142,11 +173,282 @@ export class CustodyAccountService { // --- GET ACCESS LIST --- // async getAccessList(custodyAccountId: number, accountId: number): Promise { - await this.checkAccess(custodyAccountId, accountId, CustodyAccessLevel.READ); + await this.requireOwner(custodyAccountId, accountId); return this.custodyAccountAccessRepo.find({ - where: { account: { id: custodyAccountId } }, + where: { account: { id: custodyAccountId }, active: true }, relations: { userData: true }, }); } + + // --- GRANT ACCESS --- // + async grantAccess( + custodyAccountId: CustodyAccountId, + ownerAccountId: number, + mail: string, + accessLevel: CustodyAccessLevel, + ): Promise { + const isInvalidNumericId = + custodyAccountId !== LegacyAccountId && + (typeof custodyAccountId !== 'number' || + !Number.isSafeInteger(custodyAccountId) || + custodyAccountId < 1 || + custodyAccountId > PG_INTEGER_MAX); + if (isInvalidNumericId) { + throw new BadRequestException('Invalid custody account ID'); + } + + // Authorise the Safe first (ownership / legacy entitlement) so e-mail resolution cannot + // leak whether an address is registered to callers without access. + if (custodyAccountId === LegacyAccountId) { + return this.grantAccessForLegacy(ownerAccountId, mail, accessLevel); + } + + const account = await this.requireOwner(custodyAccountId, ownerAccountId); + + const target = await this.resolveUserByMail(mail); + if (target.id === ownerAccountId) { + throw new BadRequestException('Cannot grant access to yourself'); + } + + return this.createGrant(this.custodyAccountAccessRepo.manager, account, target, accessLevel); + } + + async updateAccess( + custodyAccountId: number, + accessId: number, + ownerAccountId: number, + accessLevel: CustodyAccessLevel, + ): Promise { + const account = await this.requireOwner(custodyAccountId, ownerAccountId); + + // Read + deactivate + insert under one transaction with a row lock so concurrent + // update/revoke cannot leave a superseded active grant behind a false revoke success. + return this.custodyAccountAccessRepo.manager.transaction(async (manager) => { + const access = await this.lockActiveAccessGrant(manager, custodyAccountId, accessId); + this.rejectOwnerGrantMutation(access, account, 'modify'); + + if (access.accessLevel === accessLevel) { + return access; + } + + await manager.update(CustodyAccountAccess, ...access.deactivate()); + + const grant = manager.create(CustodyAccountAccess, { + account: access.account, + userData: access.userData, + accessLevel, + active: true, + }); + + return this.saveGrant(manager, grant); + }); + } + + async revokeAccess(custodyAccountId: number, accessId: number, ownerAccountId: number): Promise { + const account = await this.requireOwner(custodyAccountId, ownerAccountId); + + await this.custodyAccountAccessRepo.manager.transaction(async (manager) => { + const access = await this.lockActiveAccessGrant(manager, custodyAccountId, accessId); + this.rejectOwnerGrantMutation(access, account, 'revoke'); + + await manager.update(CustodyAccountAccess, ...access.deactivate()); + }); + } + + // --- HELPER METHODS --- // + private async resolveUserByMail(mail: string): Promise { + const users = await this.userDataService.getUsersByMail(mail, true, {}); + if (users.length === 0) { + throw new NotFoundException('User with this e-mail not found'); + } + if (users.length > 1) { + throw new ConflictException('Multiple users found for this e-mail'); + } + + return users[0]; + } + + /** + * Owner-only authorisation for grant management. Missing, non-active and foreign + * accounts all yield the same Forbidden so callers cannot probe existence (403 vs 404). + * NotFound for missing grant rows stays downstream after ownership is established. + */ + private async requireOwner(custodyAccountId: number, accountId: number): Promise { + let custodyAccount: CustodyAccount; + try { + custodyAccount = await this.getCustodyAccountById(custodyAccountId); + } catch (e) { + if (e instanceof NotFoundException) { + throw new ForbiddenException('Only the account owner can manage access grants'); + } + throw e; + } + + if (custodyAccount.owner.id !== accountId) { + throw new ForbiddenException('Only the account owner can manage access grants'); + } + + return custodyAccount; + } + + private rejectOwnerGrantMutation( + access: CustodyAccountAccess, + account: CustodyAccount, + action: 'modify' | 'revoke', + ): void { + if (access.userData.id === account.owner.id) { + throw new BadRequestException( + action === 'revoke' + ? "Cannot revoke the account owner's access grant" + : "Cannot modify the account owner's access grant", + ); + } + } + + /** + * Locks the active grant row (SELECT … FOR UPDATE OF access) so concurrent update/revoke + * serialise on the same row. A lost race (row already inactive / missing) yields NotFound — + * never a false success. + */ + private async lockActiveAccessGrant( + manager: EntityManager, + custodyAccountId: number, + accessId: number, + ): Promise { + const access = await manager + .createQueryBuilder(CustodyAccountAccess, 'access') + .innerJoinAndSelect('access.userData', 'userData') + .innerJoinAndSelect('access.account', 'account') + .where('access.id = :accessId', { accessId }) + .andWhere('access.active = :active', { active: true }) + .andWhere('account.id = :custodyAccountId', { custodyAccountId }) + .setLock('pessimistic_write', undefined, ['access']) + .getOne(); + + if (!access) { + throw new NotFoundException('Access grant not found'); + } + + return access; + } + + private async createGrant( + manager: EntityManager, + account: CustodyAccount, + target: UserData, + accessLevel: CustodyAccessLevel, + ): Promise { + const existing = await manager.findOne(CustodyAccountAccess, { + where: { account: { id: account.id }, userData: { id: target.id }, active: true }, + }); + if (existing) { + throw new ConflictException('Access grant already exists for this user'); + } + + const grant = manager.create(CustodyAccountAccess, { + account, + userData: target, + accessLevel, + active: true, + }); + + return this.saveGrant(manager, grant); + } + + private async saveGrant(manager: EntityManager, grant: CustodyAccountAccess): Promise { + try { + return await manager.save(grant); + } catch (e) { + // Concurrent insert lost the unique race (SQLSTATE 23505) → same 409 as the pre-check. + if ((e as { code?: string }).code === '23505') { + throw new ConflictException('Access grant already exists for this user'); + } + throw e; + } + } + + private async persistCustodyAccount( + manager: EntityManager, + owner: UserData, + title: string, + description?: string, + ): Promise { + const custodyAccount = manager.create(CustodyAccount, { + title, + description, + owner, + status: CustodyAccountStatus.ACTIVE, + requiredSignatures: 1, + }); + + const saved = await manager.save(custodyAccount); + + const ownerAccess = manager.create(CustodyAccountAccess, { + account: saved, + userData: owner, + accessLevel: CustodyAccessLevel.WRITE, + active: true, + }); + await manager.save(ownerAccess); + + return saved; + } + + /** + * Loads active owner user_data (+ users) and counts owned custody_account rows. + * Shared by checkAccess (legacy alias) and grantAccessForLegacy so entitlement stays consistent. + */ + private async loadLegacyOwner( + ownerAccountId: number, + ): Promise<{ owner: UserData; hasCustody: boolean; ownedCount: number }> { + const owner = await this.userDataService.getActiveUserData(ownerAccountId, { users: true }); + const hasCustody = owner.users.some((u) => u.role === UserRole.CUSTODY); + const ownedCount = await this.custodyAccountRepo.count({ where: { owner: { id: ownerAccountId } } }); + return { owner, hasCustody, ownedCount }; + } + + private async grantAccessForLegacy( + ownerAccountId: number, + mail: string, + accessLevel: CustodyAccessLevel, + ): Promise { + // Authorise legacy entitlement before resolving the e-mail (no enumeration for outsiders). + const { owner, hasCustody, ownedCount } = await this.loadLegacyOwner(ownerAccountId); + if (!hasCustody) { + throw new NotFoundException('Legacy account not found'); + } + + // Legacy Safe = absence of any owned account row (pre-check; re-checked under lock). + if (ownedCount > 0) { + throw new BadRequestException('Legacy account not available because custody accounts already exist'); + } + + const target = await this.resolveUserByMail(mail); + if (target.id === ownerAccountId) { + throw new BadRequestException('Cannot grant access to yourself'); + } + + return this.custodyAccountRepo.manager.transaction(async (manager) => { + // Serialize concurrent materialisations for the same owner + await acquireCustodyLegacyMaterializeLock(manager, ownerAccountId); + + const existingAccounts = await manager.find(CustodyAccount, { + where: { owner: { id: ownerAccountId } }, + relations: { owner: true }, + order: { id: 'ASC' }, + }); + + if (existingAccounts.length > 0) { + throw new BadRequestException('Legacy account not available because custody accounts already exist'); + } + + // Creates the account + owner grant only. Data rows (balances, orders, users) are + // deliberately not re-parented: nothing reads accountId/custodyAccountId for + // authorisation, and account-scoped read paths resolve data through the account owner. + const account = await this.persistCustodyAccount(manager, owner, 'Custody'); + + return this.createGrant(manager, account, target, accessLevel); + }); + } } diff --git a/src/subdomains/generic/gs/dto/gs.dto.ts b/src/subdomains/generic/gs/dto/gs.dto.ts index 701440b35f..660f1052a6 100644 --- a/src/subdomains/generic/gs/dto/gs.dto.ts +++ b/src/subdomains/generic/gs/dto/gs.dto.ts @@ -558,7 +558,7 @@ export const DebugAllowedColumns: Record = { columns: ['id', 'created', 'updated', 'ownerId', 'requiredSignatures', 'status'], }, custody_account_access: { - columns: ['id', 'created', 'updated', 'accessLevel', 'accountId', 'userDataId'], + columns: ['id', 'created', 'updated', 'accessLevel', 'accountId', 'userDataId', 'active', 'deactivatedAt'], }, custody_balance: { columns: ['id', 'created', 'updated', 'accountId', 'assetId', 'balance', 'userId'],