Repository navigation
fix(soroban): render signed SCMap entry on the signing screens - #3017
Conversation
|
PR Preview build is ready: https://github.com/stellar/freighter/releases/tag/untagged-81d86ceefb5d4089aa00 |
aristidesstaffieri
changed the title
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Some distinct signed values remain ambiguous or incomplete in the approval UI.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 5
Open (6)
What changed in this PR
Improves Soroban signing-screen rendering while preserving signed XDR value types.
Changes:
- Adds byte-safe rendering for Soroban strings, symbols, and tags.
- Renders maps and vectors without lossy native conversion.
- Adds helper and UI regression tests.
| File | Description |
|---|---|
| extension/src/popup/helpers/soroban.ts | Adds type-aware Soroban value rendering. |
| extension/src/popup/helpers/__tests__/soroban.test.js | Tests invocation fidelity and binary names. |
| extension/src/popup/helpers/__tests__/scValByType.test.js | Tests container and binary-value rendering. |
| extension/src/popup/components/signTransaction/Operations/KeyVal/index.tsx | Passes original SCVals to the renderer. |
| extension/src/popup/components/signTransaction/Operations/index.tsx | Uses safer function-name rendering. |
| extension/src/popup/components/signTransaction/Operations/__tests__/index.test.tsx | Tests rendered contract-call parameters. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…oNative scValToNative decodes SCMaps through Object.fromEntries, which coerces every key via ToPropertyKey and applies last-write-wins, so a map with N signed entries could render as one on the approval screen — with no glyph and no warning. Render maps and vecs from the signed entry list instead, as a value literal where quoting carries the key type. Decode SCString/SCSymbol-backed fields (arguments, function names, CAP-85 executable tags) strictly rather than leniently, so invalid UTF-8 renders as labelled hex instead of collapsing distinct payloads onto one U+FFFD string. Carry auth-tree args through as ScVal rather than round tripping them through nativeToScVal, which silently re-typed map keys.
Hovering or clicking a contract-call argument now shows the SCVal arm it was signed as. A symbol-keyed struct renders the same as before -- naming the type inline would bury the data the signer is there to read -- so the type is one hover away instead. One tooltip is shared by every value on the screen and re-anchored as the pointer moves, rather than one per value being torn down and rebuilt, with the usual open/close/skip delays so moving along a map reads as continuous. Both the rendered text and the clipboard string are now built from one token stream, so they cannot drift apart. Verified byte-identical to the previous rendering across every arm, except four that returned a non-string React drops -- a signed `false`, `void`, a ledger-key instance and a non-wasm contract instance each reached the screen as an empty row. Also sets `white-space: pre-wrap` on `.ParameterValue`, so a rendered SCMap actually shows one signed entry per line. The rule that was meant to do this sat on `.Parameter`, a class no component renders.
…lete the instance arm Labelled hex (`string(0x616c696365ff)`) could be spelled out by valid text that happened to read the same, so two distinct signed payloads still shared one screen string. Control and bidi codepoints also reached the screen raw, letting one signed string reorder into another. Escape the wire bytes instead: backslash, C0/C1, DEL, bidi and zero-width marks become escapes, invalid bytes become `\xNN`, and everything else -- including ordinary non-ASCII -- passes through untouched. The backslash is itself escaped, so the escapes are prefix-free and no two byte strings can render alike. This is the SDK's SEP-0051 `toJson()` alphabet widened to keep legible text legible; `toJson()` hex-escapes every byte above ASCII and would render `café` as `caf\xc3\xa9`. The displayed name is no longer a lookup key. `KeyValueInvokeHostFnArgs` takes `specFnName`, the raw signed name, and skips the contract-spec lookup when the bytes are not text. SCV_CONTRACT_INSTANCE now renders its storage map and the CAP-85 external reference's owner and tag, so two instances sharing an executable are told apart, and stays on one line as a map key.
b91cb12 to
df48eb4
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…is displayed Escape Unicode's `Default_Ignorable_Code_Point` class rather than a hand-picked list of ranges, keeping the C1 controls and adding U+2028/U+2029, which draw as nothing but insert a line break. Copy contract-call arguments with `scValToDisplayValue` so the clipboard and the approval text come from one token stream. Delete `scValByType`, which had no other caller, and move its `default:` arm behaviour onto `scValToDisplayTokens`.
`ScValDisplay` built its tokens from the context of the render it was in, so wrapping them in a provider afterwards handed a value to children that never read it. Render the tokens directly and state the provider requirement on the component. Say in the Soroban guide what `scValToNative` does to an SCMap and to non-UTF-8 text, rather than that it "may not work for all SC vals".
…l type Ports the remaining two commits of stellar/freighter#3017. - Escape XdrString-backed fields to one prefix-free byte string: valid text containing `\xff` rendered like the byte 0xFF, and bidi overrides reached the screen raw, letting one signed string reorder into another. - Decode strict UTF-8 by hand -- RN's TextDecoder polyfill throws on the `fatal` option, which Jest never sees because Node implements it. - Render arguments from a token stream, so the drawn and copied text cannot drift, and let the signer tap a scalar to see the arm it was signed as. - scValByType returns a string on every arm; `false`, `void`, a ledger-key instance and a non-wasm instance each reached the screen as an empty row. Instances now render their storage map and the CAP-85 owner and tag. - Key the contract-spec lookup off the raw signed function name, not the escaped display form.
…l type Ports the remaining two commits of stellar/freighter#3017. - Escape XdrString-backed fields to one prefix-free byte string: valid text containing `\xff` rendered like the byte 0xFF, and bidi overrides reached the screen raw, letting one signed string reorder into another. - Decode strict UTF-8 by hand -- RN's TextDecoder polyfill throws on the `fatal` option, which Jest never sees because Node implements it. - Render arguments from a token stream, so the drawn and copied text cannot drift, and let the signer tap a scalar to see the arm it was signed as. - scValByType returns a string on every arm; `false`, `void`, a ledger-key instance and a non-wasm instance each reached the screen as an empty row. Instances now render their storage map and the CAP-85 owner and tag. - Key the contract-spec lookup off the raw signed function name, not the escaped display form.
| * Unicode's own class for codepoints that are meant to render as nothing: | ||
| * soft hyphens, zero-width marks, bidi controls, variation selectors and the | ||
| * tag block. Named rather than hand-listed so the boundary is one the standard | ||
| * maintains — the previous enumeration missed 4159 codepoints, U+2060 WORD |
There was a problem hiding this comment.
Looks like this comment is maybe adding something about the previous Claude prompts?
There was a problem hiding this comment.
Agreed, that sentence was comparing against a hand-listed range set that only ever existed earlier on this branch, so it has no referent on master. Trimmed it to the rationale alone. I also reworded two test comments on the branch that had the same problem.
| punct("owner: "), | ||
| value(scvAddressToString(ref.executableOwner), executable.type), | ||
| ], | ||
| [punct("tag: "), value(xdrStringToDisplay(ref.tag), executable.type)], |
There was a problem hiding this comment.
An executable tag can fake extra fields, including a second owner
Merge blocker. The signing screen can show an owner address that was never signed, and a tag can look exactly like a string.
TL;DR: Executable tags are shown on the signing screen without quotes or escaping of quotes, commas, colons or brackets. A contract-instance tag of x, owner: G…ATTACKER therefore renders as a second owner: field. A tag whose bytes are "hello" renders the same as the string "hello". The mobile PR (stellar/freighter-mobile#1012) has the same code and the same problem.
Steps to reproduce:
- Build a contract-call argument containing a contract instance whose executable is an external reference with tag
x, owner: <any G address>. - Open it on the signing screen. It shows:
contractInstance {
executable: externalRef {
owner: GCZY5…YWOP,
tag: x, owner: GCHOM…GWZQ
}
}
A map keyed by executable tag "hello" and by string "hello" shows two identical "hello": keys.
Detailed explanation (for agents)
Root cause: in a value literal, both sites render the tag with xdrStringToDisplay, which escapes backslashes and control characters but not " , : { } [ ]. Strings and symbols already go through the quoting xdrStringToLiteral, but these two sites don't.
executableToTokens, external-reftag:scValToDisplayTokens,scvExecutableTag:freighter/extension/src/popup/helpers/soroban.ts
Lines 967 to 969 in 1033063
Deterministic repro: call scValToDisplayValue on scvContractInstance with contractExecutableExternalRef({ executableOwner, tag: "x, owner: G…" }), and on scvMap with keys scvExecutableTag('"hello"') and scvString("hello"). Checked at 1033063932a0a4a2722dea3e71221010dbea7c28.
Suggested fix: render both sites with xdrStringToLiteral(…, "string") so the tag is quoted, with " escaped. If the tag needs to look different from a string, use a wrapper such as tag("…"). Make the same change on mobile.
| return quoted; | ||
| }; | ||
|
|
||
| const scvAddressToString = (address: xdr.ScAddress) => { |
There was a problem hiding this comment.
scvAddressToString duplicates addressToString
Nit, not a merge blocker. The output is identical either way.
TL;DR: The new helper does exactly what the existing one in this file already does, so there are now two address helpers to keep in step. Calling the existing one directly matches what mobile #1012 does at the same two places.
Detailed explanation (for agents)
The account branch is the same line as in addressToString. The fallback calls addressToString, which re-checks the account case and then uses Address.fromScAddress(address).toString(). addressToString is unchanged from the base branch.
- new:
freighter/extension/src/popup/helpers/soroban.ts
Lines 797 to 802 in 1033063
- existing:
freighter/extension/src/popup/helpers/soroban.ts
Lines 369 to 375 in 1033063
Suggested fix: delete scvAddressToString and call addressToString at the two places that use it (external-ref owner and the scvAddress arm).
There was a problem hiding this comment.
Agreed, it was a straight duplicate. Deleted it and both sites call addressToString now.
piyalbasu
left a comment
piyalbasu
left a comment
There was a problem hiding this comment.
Similar 2 comments as on mobile and a couple of nits, but otherwise lgtm
* fix(soroban): render signing-screen values from the SCVal, not scValToNative Decoding an SCMap to a JS object coerced every key through ToPropertyKey and applied last-write-wins, so a map with N signed entries could render as one line on the approval screen. Render maps and vecs from the signed entry list instead, as a value literal where quoting carries the key type. Decode SCString, SCSymbol, executable tags and function names strictly, falling back to the reversible SEP-51 escape form only when the bytes are not valid UTF-8. Two distinct binary payloads can no longer collapse onto one screen string, and valid non-ASCII text stays readable instead of being escaped byte by byte. * fix(soroban): escape signing-screen text and reveal each value's SCVal type Ports the remaining two commits of stellar/freighter#3017. - Escape XdrString-backed fields to one prefix-free byte string: valid text containing `\xff` rendered like the byte 0xFF, and bidi overrides reached the screen raw, letting one signed string reorder into another. - Decode strict UTF-8 by hand -- RN's TextDecoder polyfill throws on the `fatal` option, which Jest never sees because Node implements it. - Render arguments from a token stream, so the drawn and copied text cannot drift, and let the signer tap a scalar to see the arm it was signed as. - scValByType returns a string on every arm; `false`, `void`, a ledger-key instance and a non-wasm instance each reached the screen as an empty row. Instances now render their storage map and the CAP-85 owner and tag. - Key the contract-spec lookup off the raw signed function name, not the escaped display form. * fix(soroban): quote executable tags on the signing screen Port of stellar/freighter@78c6a04. An external reference's tag is now a quoted string literal and a standalone SCVal tag renders as tag("…"), so a deployer-chosen tag can neither forge an `owner:` field nor read as a string with the same characters.
Summary
Renders Soroban contract-call arguments on the signing screens from the signed
SCValrather than from a decoded native value, so the screen matches what is being signed. Three defects are fixed:SCMapwith N signed entries could render as one line — with no glyph and no warning. Maps now render from the signed entry list, so entry count is preserved by construction.u64(1)andstring("1"), orsymbol("a")andstring("a"), previously rendered as identical keys. Quoting now carries the type: strings quoted, symbols and numbers bare, structured keys inline.SCString,SCSymbol, function names and CAP-85 executable tags are decoded strictly instead of leniently, so two distinct non-UTF-8 payloads can no longer collapse onto the sameU+FFFDstring.All three hold at every nesting depth. Only a top-level argument was handled before: one level down a map collapsed, a non-UTF-8 string rendered as
{"0":97,"1":108,…}, and bytes as{"type":"Buffer","data":[…]}.Auth-entry invocation trees now carry
xdr.ScValargs through to the renderer instead of decoding to natives and re-encoding, a round trip that silently re-typed map keys.Affected surfaces:
SignTransaction(Parameters and Auth Entries),SignAuthEntry, and theReviewTransactionauth block.Display format change — map and vector parameters render as a value literal rather than JSON:
Test plan
yarn test:ci— 1929 passing, including 24 new cases: entry-count preservation for structured and mixed-type map keys, address- andu32-keyed controls, nested containers, and strict decoding of non-UTF-8 strings, symbols, executable tags and function namesyarn build:extension