feat: server-side normalization, rejection reporting and capture options - #21
Merged
Conversation
Phase 1 of the server-normalisation plan. Three additive options, each
defaulting to current behaviour so an upgrading consumer sees no change.
* `CaptureOptions.lowercaseValues` (default false) folds captured values.
Placed on CaptureOptions rather than SanitizeConfig, mirroring the
existing `BuildUtmUrlOptions.lowercaseValues` on the outbound side. The
plan argued for SanitizeConfig on the grounds that folding must precede
the value gates, but that is satisfied more directly here: folding runs
in the capture loop, before sanitisation and PII filtering both, so
every downstream pattern sees folded input. It also avoids forcing a
consumer who wants only lowercasing to enable sanitisation and switch
off two unrelated flags.
* `SanitizeConfig.onMaxLength` ('truncate' | 'drop', default 'truncate').
Truncation invents a value nobody sent and merges two campaigns sharing
a long prefix; 'drop' is the honest option when values key a datastore.
* `SanitizeConfig.valuePattern` — a positive allowlist gate. Unlike
`customPattern` (subtractive) it accepts or drops the value whole.
Both new SanitizeConfig fields are optional rather than required, unlike
the plan's proposal for a required field. Nothing needs them to be
required, and optional keeps the change non-breaking for anyone
constructing a bare SanitizeConfig literal.
Ordering note, a deliberate deviation: valuePattern is tested AFTER trim,
not before as the plan specified. Testing first would reject values whose
only offence is surrounding whitespace that sanitizeValue was about to
remove anyway — `' linkedin '` under /^[a-z]+$/ should not be a rejection.
Moving lowercaseValues to CaptureOptions opened a gap the plan's design
did not have: the React path maps UtmConfig onto CaptureOptions, so the
option also had to be threaded through UtmConfig, DEFAULT_CONFIG,
createConfig and mergeConfig to stay reachable from UtmProvider. Covered
by tests through the provider.
Tests: 476 passing, up from 455.
🤖 Generated with [Nori](https://noriagentic.com)
Co-Authored-By: Nori <contact@tilework.tech>
Phase 2. An empty capture result means two different things today:
genuine direct traffic, and a campaign link whose every parameter was
filtered. Collapsing them inflates the direct-traffic denominator that
every campaign share is measured against.
captureUtmParametersWithReport returns { params, rejected, invalidUrl }.
captureUtmParameters now delegates to it and returns .params, so there is
exactly one capture pipeline rather than two that can drift.
The rejected value is never included — key, reason and (for PII) the
matching pattern name only. onPiiDetected already warns that raw values
must not be logged; a report struct carrying one would be handed straight
to a logger by most consumers. Asserted by checking the serialised report
does not contain the fixture email.
Two additions beyond the plan's design:
* `allowedParameters` is a rejection reason. A utm_ key dropped by the
allowlist is probably the most common rejection there is, and the
plan's union had no case for it.
* `invalidUrl` distinguishes an unparseable URL from an empty capture.
That path currently console.warns and returns {}, which is exactly the
absence/rejection ambiguity this feature exists to remove — an
unparseable URL is neither direct traffic nor a rejected campaign.
Rejections are recorded per parameter, so one bad parameter never costs
the whole campaign. Reporting variants (sanitizeValueWithReport,
filterValueWithReport) carry the reasons; the existing sanitizeValue and
filterValue are now thin wrappers with unchanged signatures.
A value reduced to '' by ordinary stripping is deliberately NOT reported
as a rejection — that predates this feature, and reporting it would give
every consumer spurious rejections from existing behaviour.
Tests: 501 passing, up from 476.
🤖 Generated with [Nori](https://noriagentic.com)
Co-Authored-By: Nori <contact@tilework.tech>
Phase 3. normalizeUtmParams and normalizeUtmUrl apply the same folding rules server-side, so a public ingest endpoint does not have to reimplement them. The client-side pass cannot be trusted for a public endpoint — anyone can POST to it directly. Three properties are the point: * Total output. Every allowed key is always present, absent ones carrying `absentValue` (default ''). A consumer keying a composite primary key needs absence to be a value that groups; NULL does not deduplicate, so a nullable column fragments one campaign across as many rows as it has absent parameters. * Never throws. The input is an untrusted HTTP body, so a throw is a 500 on somebody's first page load. Covered by a 16-case table: null, 42, strings, arrays, nested arrays, functions, symbol keys and a prototype-polluting body all produce a usable total result. Non-string values are rejected rather than coerced — String(['a','b']) is 'a,b', a value nobody sent. * Server defaults differ deliberately: lowercasing on, over-length dropped rather than truncated, PII filtering on. Two of the plan's open questions, decided: * allowedParameters defaults to all six STANDARD_UTM_PARAMETERS, including utm_id, matching the browser default. Documented on the option, since a consumer keying five columns against a library producing six gets a mystery extra row. * redact mode is NOT offered server-side; `mode` is omitted from the options type entirely. '[REDACTED]' persisted as a campaign value is a campaign nobody ran, which is worse than dropping it. On the justification, stated honestly because the plan asked for it: the root entry does NOT crash in Node — verified by importing the built artifact in a bare Node process. The real case for /server is a documented DOM-free surface, server defaults, totality, and a structural guarantee that it cannot reach storage. The bundle is 8K against the root entry's 52K. That structural guarantee is enforced rather than asserted: __tests__/server/isolation.test.ts walks the transitive RUNTIME import graph from the entry and fails if it reaches storage, form, decorator, debug or react, or if any reachable module touches window/document/web storage. Type-only imports are not followed — they are erased at build and cannot pull runtime code in. Verified against the built artifact, not the source: both dist/server formats import and run in bare Node, and neither bundle contains a single reference to sessionStorage, localStorage or document. Tests: 573 passing, up from 501. normalize.ts at 98.95% statements. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <contact@tilework.tech>
Phase 4, Tasks 9 and 10. The sendBeacon subsection is the highest-value part. It is a live footgun that produces no error and wrong data: navigator.sendBeacon(url, JSON.stringify(params)) sends Content-Type: text/plain;charset=UTF-8. A server parsing only application/json gets the body as a raw string, JSON parsing fails, and a defensively-written server answers 204 with an empty campaign. No error anywhere, and every campaign is attributed to direct traffic. Wrapping the body in an application/json Blob does not fix it — that makes the request non-simple, sendBeacon cannot preflight, and the browser drops it silently instead. Documented as a tradeoff rather than a recommendation, with both workable options (fetch + keepalive, or sendBeacon plus a server that accepts text/plain) and the Fastify/Express snippets for the second. Also adds: * lowercaseValues, onMaxLength and valuePattern to the sanitization section, with a table distinguishing the three regex options that are otherwise easy to confuse — customPattern subtracts, valuePattern gates, piiFiltering.allowlistPattern gates within PII decisions. * A section on captureUtmParametersWithReport, including the warning that the report deliberately omits the rejected value. * A Server-Side Usage section covering totality, the never-throws contract, and a table of the four defaults that differ from the browser on purpose. States the utm_id decision explicitly, per the plan. * A SanitizeConfig table with the full rule ordering. * lowercaseValues in the main configuration table. The /server justification is stated honestly: the root entry does not crash in Node, so the reasons given are the DOM-free surface, server defaults, totality, enforced isolation and size (8KB vs 52KB) — not a crash that does not happen. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <contact@tilework.tech>
Adds src/server/docs.md and brings the inbound, config, types, react,
root and test docs in line with the change.
The load-bearing entries are the invariants rather than the API surface:
the single-pipeline rule (capture semantics belong in capture-report.ts;
captureUtmParameters cannot drift because it delegates rather than
copies), the enforced isolation of src/server and why it must be enforced
rather than documented, and the "a flat capture option costs five edits
in config/" chain that lowercaseValues had to be threaded through.
Also fixes two pre-existing staleness issues found on the way: the react
doc described the capture call as taking {sanitize, piiFiltering}, which
no longer matches, and the inbound doc gave the old sanitizer rule
ordering without the valuePattern gate.
🤖 Generated with [Nori](https://noriagentic.com)
Co-Authored-By: Nori <contact@tilework.tech>
Three reviewers on the same diff; the two most serious findings each came
from a single reviewer, not consensus.
HIGH — captureUtmParameters smuggled a value past the PII filter.
Reproduced against main:
?utm_source=good&utm_source=<pii> with piiFiltering enabled
main -> {} this branch (before fix) -> { utm_source: 'good' }
The old pipeline collected duplicates last-wins and filtered afterwards,
so rejecting the last occurrence took the key with it. Processing each
occurrence in turn left the earlier accepted duplicate standing. Fixed by
restructuring into two phases — collect last-wins, then gate the surviving
value per key — which restores parity exactly and, unlike the delete-on-
reject patch it replaces, also fixes the related onPiiDetected drift: the
callback receives the RAW value and was firing for occurrences that never
reached the output, handing consumers more raw PII than before.
HIGH — the refactor narrowed the pipeline's try/catch to the URL parse.
The old implementation wrapped everything and returned {} on any throw,
with a comment saying why. Verified drift:
patterns: [{ name: 'x', enabled: true }] (no regex)
main -> {} branch -> THREW "Cannot set properties of undefined"
Capture runs on a page-load path, so a consumer's malformed config must
not become a broken page. Outer guard restored; the server gained the
equivalent per-key guard so one bad pattern degrades to a rejection
rather than a 500.
HIGH — mergeSanitizeConfig silently dropped onMaxLength and valuePattern.
It rebuilds the object field by field, so two of the three new options
were unreachable through createConfig, UtmProvider and loadConfigFromJson.
Only direct captureUtmParameters options were tested, which is why this
went unseen. A loader test also passed through the wrong path:
createConfig() with no argument early-returns the defaults without
merging, so it could never have caught this.
MEDIUM — the delegation-equivalence test asserted x === x.
captureUtmParameters IS captureUtmParametersWithReport(...).params, so
comparing them passes against any pipeline however wrong — which is
exactly how the duplicate bug slipped through. Replaced with fourteen
golden cases taken from main's behaviour, including the duplicate ones.
MEDIUM — the report's `key` is attacker-controlled. Any utm_-prefixed
parameter is captured, so `?utm_someone@example.com=1` puts an email in
the rejection the docs tell people to log. `rejected` is also unbounded.
Both now documented on the types and in the README.
MEDIUM — validateConfig ignored all three new fields, so JSON config with
onMaxLength: 'trunkate' or lowercaseValues: 'yes' validated clean.
MEDIUM — normalizeUtmParams could throw despite the never-throws promise:
throwing getters, hostile Proxies and revoked Proxies all propagated, as
did a non-array allowedParameters (a string was iterated per character,
producing one column per letter). Made the promise true rather than
weakening the claim.
MEDIUM — the isolation walk missed side-effect and dynamic imports, the
two most likely ways someone reintroduces storage later. Both now
followed, bare specifiers assert empty (catching `import 'react'`, which
the relative-path checks cannot see), an unresolvable relative specifier
fails loudly rather than under-reporting, and the positive control now
pins modules two hops out. Verified by deliberately introducing each
violation and confirming the test fails.
LOW — getDiagnostics forwarded only keyFormat and allowedParameters, so
the debug output disagreed with the real pipeline whenever sanitize, PII
filtering or folding was configured. A diagnostics tool that lies causes
the confusion it exists to resolve. There were no debug tests at all,
which is why this was invisible; added some.
LOW — README: an `await` inside a non-async handler (a syntax error as
copy-pasted), and a bundle-size figure of 8KB that was `du` block
rounding. The real number is ~5KB. Corrected, with some irony given the
plan's instruction to report measured results rather than assumptions.
Tests: 635 passing, up from 573.
🤖 Generated with [Nori](https://noriagentic.com)
Co-Authored-By: Nori <contact@tilework.tech>
Amends the docs written in 9af5425 for the changes in b77dcf0. The substantive one is the inbound pipeline: the diagram showed a single per-parameter chain, which is now wrong and, more importantly, was the shape that caused the duplicate-parameter bug. It now shows collect last-wins then gate the survivor, and records WHY that order is load-bearing rather than incidental. Also corrects a bullet in the config docs that listed the plumbing sites a new flat capture option touches — it omitted validateConfig, and that omission is exactly what let the validation gap through. The list now names validateConfig and the nested merge functions, and explains that a field mergeSanitizeConfig forgets works via direct capture options while silently vanishing on every route through config. Adds the attacker-controlled-key caveat next to the folder's existing "reports never carry the rejected value" claim, since the two read as contradictory otherwise. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <contact@tilework.tech>
7 tasks
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.
Implements
temp/utm-toolkit-implementation-plan.mdin full — all 10 tasks across its four phases. Suggested version bump: 0.3.0 (additive; see Backwards compatibility). Release script deliberately not run.Why
This came out of building a real Fastify ingest endpoint against the toolkit, counting campaign traffic into a Postgres table keyed on the UTM parameters. That consumer had to hand-write ~80 lines of server-side normalization duplicating what the toolkit already does client-side, because the client-side pass can't be trusted for a public endpoint — anyone can POST to it directly.
What's in it
Phase 1 — three additive capture options, each defaulting to today's behaviour:
CaptureOptions.lowercaseValuesfalseLinkedInandlinkedinare one campaignSanitizeConfig.onMaxLength'truncate''drop'— a truncated value is one nobody sentSanitizeConfig.valuePatternundefinedcustomPattern, which subtracts)Phase 2 —
captureUtmParametersWithReportreturning{ params, rejected, invalidUrl }, so "no campaign" is distinguishable from "campaign arrived and every parameter was filtered".captureUtmParametersdelegates to it, so there is exactly one pipeline.Phase 3 —
@jackmisner/utm-toolkit/server:normalizeUtmParams(input: unknown, options)andnormalizeUtmUrl(url, options). Total output, never throws, server-appropriate defaults.Phase 4 — README, including the
sendBeaconcontent-type trap.Four deliberate deviations from the plan
lowercaseValuesonCaptureOptions, notSanitizeConfig. The plan arguedSanitizeConfigbecause "the ordering requirement puts it in that stage" — but folding in the capture loop runs strictly earlier than anything inSanitizeConfig, so it satisfies the ordering better while keeping symmetry withbuildUtmUrl. It also avoids the plan's own admitted cost: needingsanitize: { enabled: true, stripHtml: false, stripControlChars: false, ... }just to fold case.valuePatterntested aftertrim, not before. Gating first would reject' linkedin 'under/^[a-z]+$/— punishing a value for whitespacesanitizeValuewas about to remove.SanitizeConfigfields optional, not required. The plan accepted a breaking type change for this; nothing needed it, and optional keeps the change non-breaking.allowedParameters(probably the most common rejection there is) and aninvalidUrlflag — a malformed URL returned{}, which is precisely the absence/rejection ambiguity this feature exists to remove.The plan's open questions, answered
lowercaseValueslives) — see deviation 1.utm_id) — server defaults to all six standard params, matching the browser. Documented prominently, since keying five columns against a library producing six is a mystery extra row./serverjustification is stated honestly throughout as: documented DOM-free surface, server defaults, totality, enforced isolation, and size (~5KB vs 52KB) — not a crash that doesn't happen.modeis omitted fromServerNormalizeOptionsentirely;'[REDACTED]'persisted as a campaign value is a campaign nobody ran.sendBeacontrap is documented instead, with both workable options and server snippets.Isolation is enforced, not documented
/servermust not reach storage, form, decorator, debug or react.__tests__/server/isolation.test.tswalks the transitive runtime import graph and fails on any forbidden module, any bare third-party specifier, or any reachable module touchingwindow/document/web storage. Type-only imports aren't followed (erased at build). Verified by deliberately introducing each violation form — forbidden import, side-effect import, dynamic import, bare specifier — and confirming the test fails on each.Review findings fixed before this PR
A three-model review panel ran over the diff. It found three HIGH issues, and the two most serious each came from a single reviewer rather than consensus:
?utm_source=good&utm_source=<pii>returned{utm_source:"good"}wheremainreturned{}.maincollected duplicates last-wins then filtered; my per-occurrence pipeline left the earlier accepted value standing. Fixed by restructuring into collect-then-gate, which also fixesonPiiDetected(which receives the raw value) firing for superseded occurrences.try/catchmeant a consumer's malformed PII pattern threw into a page-load path wheremainreturned{}.mergeSanitizeConfigdroppedonMaxLengthandvaluePattern, making two of three new options unreachable throughcreateConfig/UtmProvider/loadConfigFromJson.x === x—captureUtmParametersiscaptureUtmParametersWithReport(...).params, so it passed against any pipeline however wrong. That's how the duplicate bug got through. Replaced with 14 golden cases taken frommain's behaviour.Also:
normalizeUtmParamsnow genuinely never throws (throwing getters, hostile/revoked Proxies, non-arrayallowedParameters);validateConfigcovers the new fields;getDiagnosticsforwards the full capture config so debug output stops disagreeing with the real pipeline.Backwards compatibility
main.captureUtmParameterskeeps its exact signature and results../serveris purely additive. Node >= 16 and dual ESM/CJS unchanged.Test Plan
npm test— 635 passing, up from 573 onmainnpm run type-check/lint/format:check— cleannpm run build— succeeds;./serverentry emitted in both formats with declarationsnpm run coverage— 94.45% statements / 90.76% branches, above the 80% gatesessionStorage/localStorage/documentreferences in the server bundlemaindirectly, not just self-comparedKnown follow-ups
config.defaultParamsare not folded bylowercaseValues. A consumer enabling folding for one-row-per-campaign still gets{ utm_source: 'Direct' }unfolded from defaults. Arguably should fold; left alone as out of scope.'notAString'lives in the sharedUtmRejectionReasonunion though only the server can produce it. A browser consumer switching exhaustively must handle an unreachable case. Splitting the union would be tidier.