Fix shallow copy mutation in normalize payload functions#112
Fix shallow copy mutation in normalize payload functions#112JustWalters merged 3 commits intomasterfrom
Conversation
…ePayload Both functions were doing a shallow spread of the payload, leaving the `attributes` object as a shared reference. Mutations to `attributes` in the normalized copy would silently affect the original payload on `req.body`, causing double-normalization to corrupt timestamp fields (DateTime.fromSQL on an ISO string returns null). Fix: spread `attributes` into the new object so mutations are isolated. Add tests covering SQL→ISO conversion, mutation safety, and idempotency. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes a mutation bug in payload normalization where shallow-copying payload caused payload.attributes to be shared and unintentionally mutated across accesses, leading to invalid Luxon parsing on subsequent reads.
Changes:
- Deepens the copy in
normalizeEntryPayload/normalizeInvitePayloadto also cloneattributes. - Adds Jest coverage for timestamp normalization, mutation safety, and repeat-call stability for entry/invite payloads.
- Bumps package version
2.5.0→2.5.1.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/payloads/EntryPayload.ts |
Clones attributes to prevent req.body.payload.attributes mutation during normalization. |
src/payloads/InvitePayload.ts |
Clones attributes to prevent mutation when rewriting legal-docs. |
test/payloads/EntryPayload.test.ts |
Adds tests for SQL→ISO conversion and verifies original payload is not mutated. |
test/payloads/InvitePayload.test.ts |
Adds tests for legal-doc normalization and repeat-call behavior (but one mutation-safety assertion is currently ineffective). |
package.json |
Patch version bump. |
package-lock.json |
Lockfile version bump alignment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Import payload types as return type for makePayload, enabling direct access to optional attributes like legal-docs without type errors - InvitePayload mutation test now asserts that payload.attributes['legal-docs'] still references the original array (the previous assertion on legalDocs elements would have passed even with the old shallow-copy bug) - Add blank lines between arrange/act/assert steps throughout Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Would love to hear more about this bug |
|
@ryanflynndev What do you want to know? I was testing https://github.com/envoy/pacs-integration-service/pull/63 and when I signed an invite in, I saw we were creating the credential to start at the current time, even though I changed the signed in at time. I had ngrok running so it was easy to confirm the request had the value set, and I traced it down to this function. |
Summary
normalizeEntryPayloadandnormalizeInvitePayloadwere spreading the payload shallowly ({ ...payload }), leavingattributesas a shared reference with the original request bodyattributessilently corruptedreq.body.payload, so any second access toreq.envoy.payloadwould pass already-ISO timestamps toDateTime.fromSQL(), which returnsnullattributesinto the new object —{ ...payload, attributes: { ...payload.attributes } }Tests
Added
test/payloads/EntryPayload.test.tsandtest/payloads/InvitePayload.test.tscovering:Version
Patch bump:
2.5.0→2.5.1