feat(suite-native): block receiving when device compromised #16749
Conversation
WalkthroughThis pull request introduces functionality to block the receiving process when a firmware authenticity check fails. It includes the addition of a new translation key for notifying users about a compromised device. A dependency on the device module is established, and new selectors, specifically Assessment against linked issues
Suggested labels
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Lemonexe
force-pushed
the
feat/native-FW-revision-check-ReceiveBlocked
branch
from
3b847d2 to
90fff0c
Compare
Lemonexe
requested review from
martykan,
MiroslavProchazka,
szymonlesisz,
karliatto,
Nodonisko,
a team,
mroz22,
komret,
tomasklim,
matejkriz,
marekrjpolak and
vdovhanych
as code owners
Lemonexe
changed the base branch from
feat/native-FW-revision-check-DeviceCompromisedModal
to
develop
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
suite-native/receive/src/screens/ReceiveBlockedDeviceCompromisedScreen.tsx (1)
5-15: LGTM! Well-structured component with proper error handling UI.The component follows React best practices and provides a clear error message when the device is compromised.
A few suggestions to enhance user experience:
- Consider adding a descriptive message explaining why receiving is blocked.
- Add a call-to-action button to guide users on how to resolve the issue.
export const ReceiveBlockedDeviceCompromisedScreen = () => ( <Screen header={<ScreenHeader closeActionType="back" />}> <VStack justifyContent="center" flex={1}> <PictogramTitleHeader variant="critical" title={<Translation id="moduleReceive.deviceCompromisedScreen.title" />} titleVariant="titleSmall" /> + <Text color="textSubdued" textAlign="center"> + <Translation id="moduleReceive.deviceCompromisedScreen.description" /> + </Text> + <Button + onPress={handleLearnMore} + variant="tertiary" + size="small" + > + <Translation id="moduleReceive.deviceCompromisedScreen.learnMore" /> + </Button> </VStack> </Screen> );suite-native/transactions/src/components/TransactionsEmptyState.tsx (1)
21-24: Consider improving variable naming.The variable name
hasFirmwareAuthenticityCheckHardFailedis quite long and could be simplified for better readability.- const hasFirmwareAuthenticityCheckHardFailed = useSelector( - selectHasFirmwareAuthenticityCheckHardFailed, - ); + const isDeviceCompromised = useSelector(selectHasFirmwareAuthenticityCheckHardFailed);suite-native/module-home/src/screens/HomeScreen/components/PortfolioContent.tsx (1)
45-45: Fix typo in data-testID attribute.There's a typo in the test ID: "recieve" should be "receive".
- data-testID="@home/portolio/recieve-button" + data-testID="@home/portfolio/receive-button"suite-native/intl/src/en.ts (1)
570-572: Consider enhancing the error message.While the message is clear, it could be more informative by explaining why receiving is disabled and what actions users can take.
Consider expanding the message:
deviceCompromisedScreen: { - title: 'Receiving is disabled', + title: 'Receiving is disabled due to compromised device', + description: 'Your device failed the firmware authenticity check. Please contact Trezor support for assistance.', },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
suite-native/intl/src/en.ts(1 hunks)suite-native/module-accounts-management/package.json(1 hunks)suite-native/module-accounts-management/src/components/TransactionListHeader.tsx(4 hunks)suite-native/module-accounts-management/tsconfig.json(1 hunks)suite-native/module-home/src/screens/HomeScreen/components/PortfolioContent.tsx(2 hunks)suite-native/receive/src/screens/ReceiveAccountsScreen.tsx(3 hunks)suite-native/receive/src/screens/ReceiveAddressScreen.tsx(2 hunks)suite-native/receive/src/screens/ReceiveBlockedDeviceCompromisedScreen.tsx(1 hunks)suite-native/transactions/package.json(1 hunks)suite-native/transactions/src/components/TransactionsEmptyState.tsx(3 hunks)suite-native/transactions/tsconfig.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (12)
suite-native/transactions/package.json (1)
29-29: Dependency Addition: "@suite-native/device"The addition of
"@suite-native/device": "workspace:*"aligns with the overall PR objective of enforcing firmware revision checks and enhancing device security. This workspace dependency ensures the module remains in sync with local developments. Please verify that the device module's API and functionality are compatible with the revisions introduced in this PR.suite-native/module-accounts-management/package.json (1)
27-27: Dependency Addition: Validate Integration of@suite-native/deviceThe new dependency
"@suite-native/device": "workspace:*"has been added to support the firmware revision check. This addition correctly aligns with the PR objectives to enhance device security and to block receiving on compromised devices. Please verify that all references to device functionality in this module and others are updated to use the newly added dependency and that the workspace resolution satisfies version consistency across the codebase.suite-native/transactions/tsconfig.json (1)
31-31: Validate New Device Module ReferenceThe new reference
{ "path": "../device" }is added to support the firmware revision check functionality. Please verify that the relative path correctly points to the intended device module and that the module's types are properly resolved in this context.suite-native/receive/src/screens/ReceiveAccountsScreen.tsx (1)
26-29: LGTM! Clean implementation of firmware check.The early return pattern with the firmware check is clean and follows best practices.
suite-native/module-home/src/screens/HomeScreen/components/PortfolioContent.tsx (1)
26-30: LGTM! Clean implementation of firmware check.The logic for
showReceiveButtonis clear and follows best practices.suite-native/receive/src/screens/ReceiveAddressScreen.tsx (2)
13-16: LGTM! Clean import grouping.The imports are well organized, with the new device-related imports grouped together.
49-52: LGTM! Early check for compromised device.The firmware authenticity check is performed early in the component, before any other logic, which is the correct approach for security-critical checks.
suite-native/module-accounts-management/src/components/TransactionListHeader.tsx (4)
15-15: LGTM! Clean import.The new device selector import is properly placed with other imports.
100-102: LGTM! Selector hook usage.The firmware authenticity check selector is properly implemented using useSelector.
132-132: LGTM! Clear flag naming.The isReceiveButtonDisplayed flag clearly indicates its purpose and is correctly derived from the firmware check.
143-153: LGTM! Proper conditional rendering.The receive button is correctly wrapped in a conditional render block based on the firmware check.
suite-native/module-accounts-management/tsconfig.json (1)
25-25: LGTM! Proper module reference.The device module reference is correctly added and maintains alphabetical ordering.
Lemonexe
force-pushed
the
feat/native-FW-revision-check-ReceiveBlocked
branch
from
90fff0c to
f8d68ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
suite-native/transactions/src/components/TransactionsEmptyState.tsx (1)
49-53: Consider adding user feedback when receive button is hidden.While the conditional rendering is correct, users might benefit from understanding why the receive button is not available when the device is compromised.
Consider adding an error message or info text when
showReceiveButtonis false:- {showReceiveButton && ( + {showReceiveButton ? ( <Button viewLeft="arrowLineDown" onPress={handleReceive} size="large"> <Translation id="transactions.emptyState.button" /> </Button> + ) : ( + <Text color="textSubdued" textAlign="center"> + <Translation id="transactions.emptyState.receiveDisabled" /> + </Text> )}suite-native/module-accounts-management/src/components/TransactionListHeader.tsx (1)
143-153: Consider adding user feedback when receive button is hidden.While the conditional rendering is correct, users might benefit from understanding why the receive button is not available when the device is compromised.
Consider adding an error message or info text when
isReceiveButtonDisplayedis false:- {isReceiveButtonDisplayed && ( + {isReceiveButtonDisplayed ? ( <Box flex={1}> <Button viewLeft="arrowLineDown" onPress={handleReceive} testID="@account-detail/receive-button" > <Translation id="transactions.receive" /> </Button> </Box> + ) : ( + <Box flex={1}> + <Text color="textSubdued" textAlign="center"> + <Translation id="transactions.receiveDisabled" /> + </Text> + </Box> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
suite-native/intl/src/en.ts(1 hunks)suite-native/module-accounts-management/package.json(1 hunks)suite-native/module-accounts-management/src/components/TransactionListHeader.tsx(4 hunks)suite-native/module-accounts-management/tsconfig.json(1 hunks)suite-native/module-home/src/screens/HomeScreen/components/PortfolioContent.tsx(2 hunks)suite-native/receive/src/screens/ReceiveAccountsScreen.tsx(3 hunks)suite-native/receive/src/screens/ReceiveAddressScreen.tsx(2 hunks)suite-native/receive/src/screens/ReceiveBlockedDeviceCompromisedScreen.tsx(1 hunks)suite-native/transactions/package.json(1 hunks)suite-native/transactions/src/components/TransactionsEmptyState.tsx(3 hunks)suite-native/transactions/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- suite-native/module-accounts-management/package.json
- suite-native/transactions/tsconfig.json
- suite-native/module-accounts-management/tsconfig.json
- suite-native/transactions/package.json
- suite-native/receive/src/screens/ReceiveAccountsScreen.tsx
- suite-native/module-home/src/screens/HomeScreen/components/PortfolioContent.tsx
- suite-native/intl/src/en.ts
- suite-native/receive/src/screens/ReceiveBlockedDeviceCompromisedScreen.tsx
- suite-native/receive/src/screens/ReceiveAddressScreen.tsx
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: build
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: test
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: transport-e2e-test
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (5)
suite-native/transactions/src/components/TransactionsEmptyState.tsx (2)
1-1: LGTM!The new imports are correctly placed and necessary for implementing the firmware authenticity check.
Also applies to: 6-6
21-24: LGTM!The firmware authenticity check is correctly implemented using the selector, and the
showReceiveButtonflag is appropriately derived from the check result.suite-native/module-accounts-management/src/components/TransactionListHeader.tsx (3)
15-15: LGTM!The new import is correctly placed and necessary for implementing the firmware authenticity check.
100-102: LGTM!The firmware authenticity check is correctly implemented using the selector, and the
isReceiveButtonDisplayedflag is appropriately derived from the check result.Also applies to: 132-132
148-148: Verify test coverage for conditional rendering.The component uses test ID
@account-detail/receive-button. Please ensure that tests are updated to cover the new conditional rendering behavior.
|
🚀 Expo preview is ready!
|
ℹ️ If you have been automatically assigned to review, it's a mistake. I rebased this PR to develop, and changed base to
develop, but it caused this.. Sry!Description
FW revision check part III.
ℹ️ figma screen
Related Issue
Resolve #16451
Screenshots / QA instructions
2-mainor1-main) to continue.IsFwRevisionCheckEnabled: no visible changethis UX flow is IMHO very obscure, but you may try it: