Release: develop -> main - #4583
Merged
Merged
Conversation
* chore(ci): pin the files that reached full coverage The ratchet run reports files that reach 100 percent on all four metrics but are not guarded yet, so a later change can erode them without turning the gate red. Fourteen had accumulated: ten carrying logic, four declarative. Only the fiat controller comes from recent work on the specification cache; the other thirteen were completed by other pull requests and reported since. * docs(ci): re-derive the pinned counts in the coverage gate document The document quoted the pinned count in five places and split it into logic-carrying and declarative files. Those numbers had drifted before this change already: the document said 422 while the config held 425. They are now counted from the config rather than incremented. The measured class counts under "Current state" stay as they are — they name the commit they were measured at, and deriving a new Partial figure by subtraction would invent a number nobody measured. That table's Complete cell now says so explicitly.
github-actions
Bot
requested review from
TaprootFreak and
davidleomay
as code owners
August 1, 2026 16:01
…4582) * Fall back to the collection account only on infrastructure failures The collection-account fallback swallowed every issuance error with `.catch(() => null)`, including the authoritative KYC rejection that issuance raises under its own lock against the freshly loaded, merge-resolved owner. The fallback then re-checked only the request's own, possibly stale KYC-50 snapshot, so a customer the locked check had just rejected could still receive a usable collection account with a reference. Only degrade to the fallback on an infrastructure failure; re-throw business rejections (BadRequestException, above all KYC_REQUIRED) so they reach the caller. Applies to both the explicit and the implicit Frick issuance paths. A test covers the discrepancy case. * Test the explicit path and reorder the helper Cover the explicit personal-IBAN path with the same business-rejection test the implicit path already has: a KYC_REQUIRED raised by issuance for a level-50 request must reach the caller and never reach the collection-account fallback. Move the infrastructure-vs-business helper above the fallback method so each comment sits directly over the method it describes.
…4584) * fix(asset): write decimals through the service that owns the cache The hourly decimals job wrote through the repository instance built by RepositoryFactory, while every reader goes through AssetService and its own instance. invalidateCache() only clears the instance it is called on, so the freshly written decimals stayed invisible to readers until the entry expired. The write now goes through AssetService, which invalidates the cache the readers are served from. updatePrices did exactly that already and was the only caller of its kind, so it is renamed to updateAssets and shared instead of copied. The job also collects its updates and writes once: it runs hourly and usually finds nothing, so invalidating unconditionally would drop the whole asset cache every hour for no reason. * fix(asset): invalidate the cache even when an update fails Batching the writes had given up the failure isolation the per-asset loop used to have: if one update threw, invalidateCache() was never reached and the rows written before it stayed out of the cache — the exact staleness this change exists to remove. The invalidation now sits in a finally block, so it also runs when an update fails. The empty case is guarded in the service instead of at the call site, which makes it hold for every caller: the price job passed an empty list every five minutes and dropped the whole asset cache for nothing. * fix(asset): attempt every update instead of stopping at the first failure The finally block covered the cache but not the loop: the first failing update still aborted the batch, so every asset queued behind it was skipped. Before this change each asset was written on its own, and a row that keeps failing was harmless — as a batch it would block all following assets on every run. Every update is attempted now, failures are collected, the cache is invalidated either way, and the collected ids are reported afterwards. * fix(asset): keep the original errors when several updates fail Collecting the failures had reduced them to text: the caller lost the type, the stack and the code of the database error, and every cause after the first was dropped. Whoever reads the log needs the error itself. AggregateError carries all of them and names the affected ids in its message. No dedicated error class with a failedIds field: neither caller inspects the error, both hand it to the cron, so that would be structure without a consumer.
…ting each other (#4571) * Bound the masking work, and stop the passes from eating each other Four findings against what was just merged, all in the same log line. The reason was masked whole before it was capped, twice over since the masking became two passes. An exception message can be as large as the body it interpolated a value from: 1 MiB held the thread for 663 ms, 20 MiB for 14.5 s. Only the front is masked now, wide enough past the cap that a pattern starting inside what stays visible is still seen whole - and a pattern length is dropped from the end of what came back, because a pattern crossing the point where the masking stopped arrives halved and its head would otherwise be what the shortening pulls into view. The tail goes after the masking, not before it; before it, the head moves into view with everything else. The first of the two passes wrote the placeholders the second one reads. `***` is a valid local part as far as the address pattern is concerned, so `192.0.2.123<sep>@error.code` collapsed to `***` and took the diagnostic text with it. The first pass writes markers the patterns cannot match instead, and they are turned into the placeholders once both passes are done. `format` collapsed the serialized section but never masked it, and `redact` walks the values rather than the keys - so a body key that is an address reached the log as one. The section goes through the same rendering as any other value now. The status was read twice and could answer differently, which put a body under a status it does not name; it is read once. A status in the accepted range does not have to have a name, so the message falls back past it. * Cover the plain key too, not only the one with a character in it A body key reached the trace whether or not anything was placed inside it - the serialization was never masked at all. The test said so only for the harder case. * Close what a request could still forge, decode, or lose Six findings across both reviews of this branch, four of them in what it added. A request could send the markers the first masking pass writes. They survived both passes untouched and were turned into `***` and `0x…` on the far side, a redaction that never redacted anything. Each marker is now grown until it does not occur in the value it is used on. The body an exception carries names a status of its own, and that was never compared to the one being sent: `new HttpException({ statusCode: 418 }, 400)` shipped `{"statusCode":418}` under HTTP 400. The body is kept only when it either names the status being sent or names none. Masking the whole serialized section masked the keys but masked the values twice, and the second time took their surroundings with them - a wallet address in front of a domain became the whole string. The keys are rendered where the values already are, in `redact`, and the section itself only needs collapsing again. The tail dropped from a shortened reason was dropped even where it could not have reached the visible part. It only goes where the shortening could bring it into view. Two more are older than this branch and unchanged by it, but they are in the same lines and cheap to close. An address stayed readable whenever a letter or an underscore stood next to it, because the boundary was against word characters rather than digits - `ip192.0.2.123` is how one usually arrives. And a request target carries its values percent-encoded, so `victim%40example.com` was an address to everyone reading the line and to no pattern matching it; the segments are decoded before they are masked. * Take the second masking pass back out The pass was added because removing what breaks a line can join two halves and hide a pattern from a masking that runs after it. Closing that needed markers the patterns could not match, and the markers needed to be chosen per value so a request could not send them - which grew quadratically on a value full of them, and could still be forged with a character that only disappears later. The threat it defends against is a request hiding what it sent about itself. That is not what this masking is for: it is here so a value nobody meant to log does not end up in one, and it is best effort by construction. What a request must not be able to do is put a second line in the log, and the removal alone does that. So the machinery goes and the limit is written down instead: a wallet address with a character inside it comes out as the longer hex run it becomes, which is left alone on purpose because that is what a transaction hash looks like. Two keys can render the same once they are masked. The second one kept the entry and the first one disappeared; it gets its own now. * Give an object key the guard a value already had Masking the keys reopened the thing this branch is about. A value longer than is reported by length instead of masked, because the masking is regex work over whatever it is handed; a key had no such guard and a request chooses one as freely and as long as the body allows. A 20 MB key held the thread for 20 seconds - the same shape as the reason, in the one place that had just been given more to do. It goes through the guard now, and is capped like the name it is. The comment about the serialized section still said the keys were not reached. * Count collisions instead of searching for them, and keep the path as it came Renaming a key that rendered the same as an earlier one searched the object it was building, once per candidate suffix: a thousand keys that all mask to the same thing cost a hundred milliseconds and wrote past the section budget. They are counted now, and what the rename adds is charged to the budget like anything else. The object has no prototype either, so `constructor` and `toString` are keys like any other rather than collisions with something it never held. Decoding a path segment is how a pattern is recognized in it, not how the path is rendered. `%2F` became a separator and `%3F` the start of a query, and the line named a route that was never called. The decoded form is only what gets rendered when something in it was masked. A body carrying an accessor for `statusCode` is read again when it is serialized and can answer differently then; only a plain value counts as naming a status. The failure to send a response is reported inside the same guard as the rest of the line, since it is as much a description of what happened and as unable to travel back to the caller. The previous commit's message lost the name `MAX_STRING` to a shell expansion: the sentence should read that a value longer than `MAX_STRING` is reported by length instead of masked. * Render the section instead of its keys, and settle what is sent Masking the keys where they are walked needed a length guard, then a collision counter, then budget accounting for what the counter added - three rounds, and the last one still let a real key named like a generated one take another entry. The section is cut to its budget and rendered whole instead, which reaches the keys in two lines and in an affordable size. A value is masked a second time that way and the second time can take its surroundings with it; that is the direction to be wrong in. A path segment is read decoded and rendered as it arrived, unless something in it was masked - and if the decoding would have put a separator or a query into the path, nothing of it is rendered at all, because the line would otherwise name a route with a segment more than the one that was called. Each escape is decoded on its own, so one that is not a valid sequence no longer costs the whole segment. The target itself is capped before any of that: reading it is regex work like everything else here. The body an exception carries is settled once before it is checked, so what is sent is the thing that was checked - a body that answers differently the second time it is asked cannot put a status in the response that the status line does not have. The error from a failed send is kept as what it is rather than asserted to be an `Error`, and one that is falsy is still an error. * Put back what develop already had, and stop widening the masking Two of these are corrections of this branch rather than of what it set out to fix. The masking on develop runs before and after the removal. This branch had taken the second pass out on the grounds that it defended against a request hiding its own data - which is true of one direction and not of the other: removing a character also joins what stood on either side, and a wallet address followed by one more hex character stops being one. Against develop that was a weakening, not a simplification. It is back as develop has it, and what it costs - the second pass folding `***` into an address of its own - is written down and tested. Masking the object keys is out too, in every form it was tried: in `redact` with a length guard, then a collision counter, then budget accounting for the counter; and over the serialized section, which masks each value a second time. Each round of review found the next thing wrong with it. A body key that is an address still reaches the trace, exactly as it does today, and what to do about that is a question about what this trace is for - the same question the encoded values in a path raise, and the same one raised by tracing bodies that carry free customer text at all. It does not belong in a change about bounding work. The response body is checked as it is rather than settled first: making a copy of it before checking added a full serialization of a body that has no bound, which is the thing this branch is about. What is left is what it set out to be: the reason is masked only as far as it is shown, the request target is capped after being read rather than before, the status is read once and the body has to name it, a failure to log cannot reach the caller, and an address stays an address when a letter stands next to it. * Take the replacement message out of the body, not out of the exception Dropping a body that names another status put `exception.message` on the wire in its place. That message is what an exception says to us; the body is what its thrower wrote for the caller, and the two can differ - a thrower can set an internal one on the exception and a public one in the body. Against develop and against the framework's own filter, this branch was the only thing that would have sent the first. The replacement message comes out of the body now, as a plain string or a plain array of them, and out of the status when the body has none. Six comments and tests were behind the code. The two masking docstrings had the passes the wrong way round or missing; the guard around the logging said the response was already out, which is not true of the path where sending it failed; and four properties had no test that would fail without them - the status being read once, the accessor checks on the body, the cap on the request target coming after the masking, and a failure to report a failed send staying inside the guard. * Send only what the body would have sent itself A property that is not enumerable is left out when the response is serialized, so reading one to build the replacement message put something on the wire that would never otherwise have been on it. Both the message and the status are now read the way the serialization would read them: the body's own property, enumerable, and a value rather than something asked again. The timing bound in the reason test is looser. What it shows is the bound - the same message costs seconds without it - and a limit that tight only measured how busy the machine was. * Give a plain error the same footing as a body Taking the message helper out left the branch for everything that is not an HttpException reading the exception directly and unguarded. An error that says nothing about itself put an empty message on the wire in place of the name of what is being sent, and one whose message cannot be read at all took the response with it - the thing this branch exists to prevent. It goes through a reading that can fail and falls back to the status, like every other message here. * Read a body only where reading it says anything A body that answers toJSON is serialized from what that returns, not from what it holds. Judging it by its own properties therefore judged something that was never going to be sent - and worse, a body holding an internal message and serializing a public one was dropped for naming the wrong status and then rebuilt out of the internal one. That is more than develop sends, which is the one thing this branch must not be. Such a body is not read at all now: neither for the status it names nor for the message it carries, and what goes out in its place is the name of the status being sent. An exception message that is not text is not a message either. The other path already checked; this one now does the same. * Judge a body only where judging it means anything Five ways the replacement body read something the wire would never have carried, or replaced one the wire would have carried unchanged. A status that had to be replaced now takes the body with it. What a thrower wrote belongs to the status it wrote it for; reading its message for a different status reads it for something it was not, and the message it wrote for the caller and the one it kept for itself are not always the same. The generic body goes out instead. A body that serializes itself is passed on rather than replaced. What it holds says nothing about what it sends, so there is nothing here to judge - and building a replacement out of what it holds was how the one thing this branch must not do happened again. `JSON.stringify` leaves out more than an accessor: a function, a symbol, and any name put on an array alongside its elements. None of those name a status, so a body carrying one is no longer replaced for contradicting a status it never carried. A sparse array is sent with its holes as `null`, which is not an array of strings. `every` skips the holes; the check no longer does. And asking what an exception is can throw - a proxy is enough - which left the caller without a response at all. That question is inside the guard now. * Read a body once, and send what was read A status carried by an accessor is serialized, so the body does carry one - and it answers again when it is, so what it would carry cannot be read here. That is not agreement, and such a body is replaced rather than passed on. What the serialization leaves out entirely - a name that is not enumerable, a function, a symbol, a name put on an array alongside its elements - carries nothing and is left alone, which is what the round before this one asked for. `toJSON` is asked for as a descriptor rather than read. Reading it would be one read and the serialization another, and something that answers differently between them would be sent as whatever it holds. A message array is copied out rather than passed on. Checking it with `Array.from` ran whatever iterator it brought along, and what that runs can leave different elements behind than the ones that were checked; `JSON.stringify` reads an array by index, so that is how it is read here. Deliberately not done: confirming the status a second time before the body is passed on. It would close the case of a `getStatus` that answers differently on the second call - but reading the status twice was itself a finding two rounds ago, and what it defends against is an exception this code base builds against itself. * Take nothing out of a body that could not be passed on Four rounds of review have all found the same shape: a field read off a body was not the field the serialization would have sent. A message array with a `toJSON` of its own, a `statusCode` that answers when asked, a hole in an array, a name a prototype supplies. Each one was closed and the next one appeared, because the premise was wrong - a body that cannot be passed on is a body whose contents say nothing about what it would have sent, and reading it at all is reading something else. So it is not read. What goes out in its place is the status being sent and the name of that status, and the reading of messages, arrays and descriptors that served no other purpose goes with it - thirty-nine lines of it. A body agrees with the status it is sent under unless it carries one of its own that differs, and a value of `undefined` is not carried at all: the serialization leaves it out like a function, a symbol, or a name put on an array. What was thrown while sending a response is only turned into text if it already is text. Anything else could hold what the body held, and the line that reports a failed send is not the place to find that out. * Name the question the way it is answered The check returns true when the body agrees with the status being sent and also when it says nothing about one, which is not what `names` reads as at the call site. * Say what the code does with what was thrown Only a thrown `Error` is reported as what it is; a thrown string is replaced by a fixed one like anything else. The comment said the opposite, and the fixed one started lower case. * Trust a toJSON that is simply there, not one that might answer An accessor might answer with a function and might not. Taking it for one let a body past the check that the status it names is the status being sent, and the serialization then ignored what the accessor returned and sent what the body holds - a status in the body that the status line does not have. Only a function that is simply there serializes a body; a body that only might is judged like any other. Names: the effective status is called that rather than `sent`, which read like the response had already gone out and collided with the flag that says whether it had. The methods are verbs. * Drop two tests that describe a mechanism there no longer is Both said they were about what is read out of a message, and both used a body whose status already differed from the one being sent - so the status decided and the message never came into it. There is no path left that reads a message out of a body at all: what cannot be passed on is replaced whole. What they were meant to show is covered by the test that says exactly that. * Use the nullish default the repository asks for Five fallbacks fell back on falsiness where the name of a status is either there or not there at all. * Give the two questions the prefix the repository asks of them Both answer yes or no, and the rule for that is the same for both.
* fix: omit Content-Type from Frick vIBAN GET requests * fix: keep Frick transport comment parseable
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automatic Release PR
This PR was automatically created after changes were pushed to develop.
Commits: 1 new commit(s)
Checklist