fix(sign): show all operation fields on the transaction signing screen - #968
Open
JakeUrban wants to merge 3 commits into
Open
fix(sign): show all operation fields on the transaction signing screen#968JakeUrban wants to merge 3 commits into
JakeUrban wants to merge 3 commits into
Conversation
Port stellar/freighter#2829 to mobile. The signing approval screen used truthiness guards that dropped legitimately-present but falsy fields: setOptions masterWeight/thresholds set to 0 (which can permanently lock a user out of their account), a cleared home domain, combined flag bitmasks, a manageData deletion, and setTrustLineFlags being turned off. It also never showed the asset issuer for value-bearing assets, so a counterfeit asset using a real asset's code was indistinguishable from the genuine one. - setOptions/setTrustLineFlags/manageData: presence checks instead of truthy guards; render Enabled/Disabled, Cleared/Deleted badges - add a master-key-disable warning when masterWeight is set to 0 - decode combined account-flag bitmasks (with an Unknown fallback) - show the asset issuer for every value-bearing non-native asset Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SneTW8QhowLxdA7E8q8XCd
Contributor
|
iOS Simulator preview build is ready: https://github.com/stellar/freighter-mobile/releases/tag/untagged-fb5c4bd4f37bff613356 (SDF collaborators only — install instructions in the release description) |
Code review noted the revoked trustline's asset was still rendered by code alone, unlike every other asset row on the signing screen. Show the issuer there as well so the affected trustline's asset is unambiguous. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SneTW8QhowLxdA7E8q8XCd
…rows The signing screen's List rows put the value in a non-shrinking column, so a long value (combined account-flag bitmasks, a manageData value) collapsed the label off screen. Render those long values with the stacked KeyValueListItem already used elsewhere on this screen, so the label and the full untruncated value are both shown. Also switch the master-key warning from a gray Notification to the red error Banner used for other urgent warnings in the dapp-signing flow. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SneTW8QhowLxdA7E8q8XCd
Contributor
There was a problem hiding this comment.
Pull request overview
Ports the browser-extension signing security fix to mobile, ensuring operation details accurately disclose falsy values and asset issuers.
Changes:
- Displays zero, empty, deleted, and disabled operation fields.
- Adds master-key warnings and flag decoding.
- Discloses non-native asset issuers with regression tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
Operations.tsx |
Improves signing-operation disclosure and warnings. |
Operations.test.tsx |
Adds regression coverage for disclosure behavior. |
en/translations.json |
Adds English UI strings. |
pt/translations.json |
Adds Portuguese UI strings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* eslint-disable no-bitwise */ | ||
| if ((bits & value) !== 0) { | ||
| labels.push(label); | ||
| remaining &= ~value; |
| }), | ||
| ); | ||
| } | ||
| return labels.join(", "); |
Comment on lines
+85
to
+89
| <Icon.Copy01 | ||
| size={16} | ||
| themeColor="gray" | ||
| onPress={() => copyToClipboard(issuer)} | ||
| /> |
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.
What
Ports the security fix from the Freighter browser extension (stellar/freighter#2829, merged 2026-07-23) to mobile. The transaction-signing approval screen (
Operations.tsx) is the only place a user sees what a connected dApp is asking them to sign, and it was silently dropping legitimately-present operation fields whose value happened to be falsy.Changes to
src/components/screens/SignTransactionDetails/components/Operations.tsx:setOptionspresence checks.masterWeight/lowThreshold/medThreshold/highThreshold/setFlags/clearFlags/homeDomainwere gated by truthiness guards, so a value of0(weights/thresholds) or""(home domain) rendered nothing. AsetOptions{masterWeight: 0}op previously produced a completely empty operation body. These now use presence checks and render the real value.masterWeight === 0, a redNotificationbanner warns that the account's master key is being disabled and access may be permanently lost.homeDomain === ""(clearing the domain) now shows a "Cleared" badge instead of being hidden.authorizationMaponly had keys for single bits (1,2,4,8), so any combinedsetFlags/clearFlagsvalue (e.g.5) rendered blank. A new decoder bitwise-splits the mask into named flags, with anUnknown ({{bits}})fallback for unrecognized bits.manageDatadeletion. A data-entry delete (valueabsent) was indistinguishable from a set. The Value row now always renders, showing a "Deleted" badge for a deletion.setTrustLineFlags. A flag being turned off (false) was hidden by the truthy guard, and the raw boolean was passed as the value (not rendered by React). Flags now use presence checks and renderEnabled/Disabled.(code, issuer), not code alone, so a counterfeitUSDC:<attacker>was indistinguishable from genuine USDC. The signing screen now shows a "Token Issuer" row (copyable, truncated) for every non-native asset it renders —payment, bothpathPayment*,createPassiveSellOffer,manageSellOffer,manageBuyOffer,createClaimableBalance,clawback,setTrustLineFlags, andrevokeTrustlineSponsorship. Native XLM (no issuer) renders no row.New i18n keys added to
enandpttranslations.Why
A malicious or compromised connected dApp could request a
stellar_signXDR/stellar_signAndSubmitXDRwhose confirmation screen showed an empty operation body — or, in the takeover variant, only a benign-looking "add a co-signer" row — while the transaction actually handed the attacker exclusive, permanent control of the account:setOptions{masterWeight: 0}— with the master key at weight 0 and no other signer, the account's signer set becomes empty and no key can ever sign for it again; the account and its assets are frozen permanently.setOptions{signer:<attacker>, masterWeight:0, thresholds:0}— the UI showed only "Signer / weight 1" (the standard add-a-co-signer pattern) while the owner's key was removed and the attacker's signer took full control.Because
SET_OPTIONSmoves no assets, the Blockaid simulation row also reads "No balance changes detected", reinforcing the false impression. This class was already fixed in the sibling browser extension; mobile never received the port (grep -rni masterkey src/returned nothing).Known limitations
authorizationMapremain hardcoded English (pre-existing; unchanged by this PR).yarn@4.10.0requires network egress that is blocked here); the equivalent checks were run directly — see Testing.Checklist
PR structure
Testing
Automated checks run directly (pre-commit hook unavailable in-sandbox):
jeston the operations test — 20 passed (6 pre-existing + 14 new regression tests covering the empty-operation/takeover cases, cleared home domain, combined flag bitmasks,manageDatadeletion,setTrustLineFlagsEnabled/Disabled, and issuer disclosure/non-disclosure).eslinton the touched source and test — clean.tsc --noEmit— clean.Release
🤖 Generated with Claude Code