fix(frontend): stop a payload string masquerading as undefined in raw view - #8045
Open
yhabib wants to merge 4 commits into
Open
fix(frontend): stop a payload string masquerading as undefined in raw view#8045yhabib wants to merge 4 commits into
yhabib wants to merge 4 commits into
Conversation
stringifyJson wrote the sentinel "__UNDEFINED__" for an undefined value, then replaced every occurrence of that quoted token in the serialized text with the bare word undefined. The replacement did not tell the sentinel from payload data. A proposal payload string that read __UNDEFINED__ showed as undefined, and a quote in front of it garbled the rest of the output. stringifyJson now walks the value itself. It writes the word undefined only for a value that is undefined, and it hands every leaf and every key to JSON.stringify. The layout, the escaping and the mapped values (Principal, bigint, hash, Promise, ArrayBuffer, function, symbol) stay the same.
The spec submits a proposal whose Motion text is the string __UNDEFINED__, then reads the three surfaces that show a payload: the tree view, the raw view and the copy button. Each surface must show the plain quoted string. The spec is NOT RUN on this machine. The local replica comes up empty with no NNS canisters, and no snsdemo snapshot archive is available to reinstall.
Array.prototype.map skips an array hole, so stringifyJson wrote [1,,3] for a sparse array. That text is not valid JSON, and the absent value did not show. Array.from visits every index and reads a hole as undefined. That is the text stringifyJson wrote before this branch. Two readability points from the review are in this commit too. The JSDoc now reads as one block, and the space after a key colon is a const.
|
✅ No security or compliance issues detected. Reviewed everything up to f909c2c. Security Overview
Detected Code Changes
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
stringifyJson’s new jsonGap helper can throw for non-finite indentation values and one updated JSDoc comment is inaccurate, so the implementation/docs should be corrected before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes stringifyJson so proposal payload strings that equal the previous undefined sentinel ("__UNDEFINED__") render as ordinary strings (instead of being rewritten to undefined) across the JSON tree view, raw view, and copy-to-clipboard text.
Changes:
- Replaced the sentinel+regex-replace strategy in
stringifyJsonwith a recursive serializer that writesundefinedonly for actualundefinedvalues and delegates all other leaf serialization toJSON.stringify. - Added unit and e2e coverage for the
"__UNDEFINED__"payload case (including copy button behavior) and for sparse-array holes. - Added a security changelog entry describing the rendering fix.
File summaries
| File | Description |
|---|---|
| frontend/src/lib/utils/utils.ts | Reworks stringifyJson to avoid sentinel collisions and preserve undefined/array holes explicitly. |
| frontend/src/tests/lib/utils/utils.spec.ts | Adds unit tests covering sentinel-string rendering, layout parity, array holes, and circulars. |
| frontend/src/tests/lib/components/common/JsonPreview.spec.ts | Adds raw/tree view assertions for the "__UNDEFINED__" string case. |
| frontend/src/tests/lib/components/proposal-detail/ProposalProposerActionsEntry.spec.ts | Adds a unit test verifying copy-to-clipboard preserves "__UNDEFINED__". |
| frontend/src/tests/page-objects/ProposalProposerActionsEntry.page-object.ts | Adds a page-object accessor for the copy button. |
| frontend/src/tests/e2e/proposal-payload-undefined-token.spec.ts | Adds an e2e regression test validating all three surfaces (tree/raw/copy). |
| CHANGELOG-Nns-Dapp-unreleased.md | Documents the fix under Security. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Math.floor(NaN) fed a RangeError into " ".repeat. Clamp NaN to 0, the same value JSON.stringify uses. Also fix the BigInt devMode JSDoc, which named the wrong output format. Copilot review on PR 8045.
Contributor
There was a problem hiding this comment.
🟢 Approved
The serialization fix is well-scoped and is backed by targeted unit/component/e2e regression tests, with only a minor documentation nit noted.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
frontend/src/tests/e2e/proposal-payload-undefined-token.spec.ts:138
- The comment says the raw view is valid JSON, but RawJson renders via stringifyJson which can output bare
undefined(not valid JSON) for actual undefined values (see JsonPreview.spec.ts expecting{"test": undefined}). This test only guarantees JSON validity for this specific payload (noundefinedvalues), so the comment should be scoped to that to avoid misleading future readers.
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
3 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
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.
Motivation
stringifyJsonmarked a missing value by writing the sentinel string"__UNDEFINED__", then replacing every occurrence of that exact text with the bare wordundefinedafter serialization. A proposal payload string equal to__UNDEFINED__matched the same replace, so it rendered asundefinedin the raw view, the tree view, and the copy text, even though the value was set. A payload string that ended in__UNDEFINED__right after a quote garbled the rest of the display.Changes
undefineddirectly when a value isundefined, and delegates every other value toJSON.stringify. A payload string that reads__UNDEFINED__now shows as that plain quoted string, like any other string.stringifyJsonto serialize an array hole asundefinedinstead of skipping it: the array branch usedArray.fromin place of.map, which skips holes and left them as holes in the output.__UNDEFINED__as the payload text and checks all three surfaces (tree view, raw view, copy button).JSON.stringify, and the array-hole case.Tests
npm run check: PASS.CI=true npm run test: PASS, 672 test files, 5917 passed, 9 skipped../scripts/check-relative-imports: PASS../scripts/e2e-tests): not run here, no local replica available in this environment. The new spec was reviewed and approved without a live run; someone with a working replica should run it before merge.Todos
CHANGELOG-Nns-Dapp-unreleased.mdin the same Security section as fix(frontend): sanitize rendered markdown and sns descriptions to block html injection #8036 (fix/proposal-html-injection). Merge fix(frontend): sanitize rendered markdown and sns descriptions to block html injection #8036 first, then rebase this branch to resolve the changelog conflict.