fix(realunit): bound the tax-residence payload and stop writing user_data.tin on a rejected request#4210
Open
TaprootFreak wants to merge 3 commits into
Open
fix(realunit): bound the tax-residence payload and stop writing user_data.tin on a rejected request#4210TaprootFreak wants to merge 3 commits into
TaprootFreak wants to merge 3 commits into
Conversation
…data.tin on a rejected request Three defects in the tax-residence contract: - user_data.tin is varchar(256), but neither CountryAndTin.tin nor countryAndTINs was length-bounded and the client allows an unbounded number of tax residences. Seven entries with an 11-digit TIN already serialize to 260 characters, so the UPDATE threw 'value too long for type character varying(256)' — AFTER forwardRegistration had already persisted the registration, turning a completed registration into a 500 that every retry reproduced. Bound the input (10 entries, 64-character TIN), widen the column to varchar(1024) to cover that bounded maximum, and reject an oversized payload up front in validateRegistrationDto so it can never fail once the registration is durable. - The idempotent branch of completeRegistration wrote user_data.tin BEFORE idempotentRegistrationResult rejects a mismatching signature, so a request answered with a 400 had already mutated the column. countryAndTINs is not part of the EIP-712 envelope, so the body cannot describe what was actually registered either. Check the signature first and sync the snapshot from the durable registration row instead of the request body. - @ValidateIf(!swissTaxResidence) disabled every validator on countryAndTINs — including @isarray — whenever swissTaxResidence was true, so a non-array body reached the service and crashed it with a TypeError (500 instead of 400). Validate whenever the field is required or present, and keep a defensive Array.isArray guard in the service. Also replace the unguarded JSON.parse(user_data.tin) in the registration prefill: a malformed legacy value took down the only endpoint through which the user could submit a corrected declaration. It now degrades loudly (PII-free error log) to 'no known tax residences'.
The @Transform(Util.trim) added to CountryAndTin.tin runs in class-transformer before class-validator, so a non-string tin (e.g. a number) made value.trim() throw a TypeError => HTTP 500 before @IsString could reject it as a 400 — the exact 500-instead-of-400 regression this change set exists to remove. Trim only actual strings and let any other type fall through to @IsString.
Collaborator
Author
|
Ran the mandatory pre-review process: two full review passes (conformance + logic). The first pass surfaced one issue (the newly added Verified on a build host: |
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
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.
Follow-up to #4198. A full review of #4198 together with its client-side counterpart surfaced three defects; this PR fixes all three plus one hardening item.
1.
user_data.tinoverflows its column (blocker)user_data.tinischaracter varying(256), but neitherCountryAndTin.tin(no@MaxLength) norcountryAndTINs(no@ArrayMaxSize) was bounded, and the client allows an unbounded number of tax residences. Seven entries with an 11-digit TIN already serialize to 260 characters. Verified against a real Postgres 16:Postgres errors rather than truncating, and the write happens after
forwardRegistrationhas already POSTed to Aktionariat and persisted the registration row — so a completed registration answered with a 500, and every retry hit the idempotent path, re-ran the same write and reproduced it.Fixed on three levels:
@MaxLength(64)onCountryAndTin.tin,@ArrayMaxSize(10)oncountryAndTINs, mirrored by service-level guards.user_data.tintovarchar(1024)(migration) — the bounded worst case serializes to ~890 characters. Widening a varchar length in Postgres is a catalog-only change, no table rewrite.validateRegistrationDtonow rejects an oversized payload with a 400 before anything is forwarded, so the write can never fail on a registration that is already durable.@Transform(Util.trim)ontinalso closes a smaller gap: the TIN was validated as non-empty after trimming but stored and forwarded untrimmed.2. A rejected request had already mutated the database (blocker)
In the idempotent branch of
completeRegistration,persistUserDataTinAfterRegistrationran one line beforeidempotentRegistrationResult— which rejects a mismatching signature with a 400.countryAndTINsis not part of the EIP-712 envelope, so the request body cannot describe what was actually registered either: an already-registered user could sign a fresh payload carrying arbitrary TINs, haveuser_data.tinoverwritten, and then receive the 400. The snapshot drifted away from the Aktionariat record of truth with no new registration event — which is exactly what the "Event before snapshot" rule added to CONTRIBUTING.md in #4198 forbids.Now the signature is checked first, and the snapshot is synced from the durable registration row (
toRegistrationDto), never from the request body.3.
@ValidateIfdisabled@IsArray→ 500 instead of 400 (major)@ValidateIf((o) => !o.swissTaxResidence)skipped every validator oncountryAndTINs— including@IsArray— as soon asswissTaxResidencewas true. Verified against the realValidationPipe:{ swissTaxResidence: true, countryAndTINs: "pwned" }passed validation, andentries.some(...)then threw aTypeError→ HTTP 500. The service comment claimed to enforce the nested shape, but only checked the elements, never thatentrieswas an array at all.The condition now also fires whenever the field is present, and the service keeps a defensive
Array.isArrayguard.4. Unguarded
JSON.parse(user_data.tin)in the prefill (minor)A malformed legacy value took down
GETregistration-info with a 500 — the only channel through which the user could submit a corrected declaration. It now degrades loudly (PII-free error log) to "no known tax residences", and also drops contract-violating entries (e.g. a legacyCHentry) so the prefill can never hand back a payload the validator would reject.Test plan
format:check,lint,type-check@ValidateIfgap reproduced against the real NestJSValidationPipe