Feat/entropy check UI #16711
Feat/entropy check UI #16711
Conversation
|
🚀 Expo preview is ready!
|
komret
force-pushed
the
feat/entropy-check-ui
branch
2 times, most recently
from
bad609b to
b1af14f
Compare
WalkthroughThis pull request standardizes the approach to enabling and disabling various device security checks by transitioning from “opt-out” actions with object payloads to “toggle” actions with boolean payloads. The changes include renaming action types and constants (e.g., replacing Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
🪧 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 (
|
komret
force-pushed
the
feat/entropy-check-ui
branch
3 times, most recently
from
7c79f2b to
7701eea
Compare
packages/suite/src/views/settings/SettingsDebug/CheckFirmwareAuthenticity.tsx
Show resolved
Hide resolved
packages/suite/src/views/settings/SettingsDebug/CheckFirmwareAuthenticity.tsx
Outdated
Show resolved
Hide resolved
packages/suite/src/views/settings/SettingsDebug/CheckFirmwareAuthenticity.tsx
Outdated
Show resolved
Hide resolved
komret
force-pushed
the
feat/entropy-check-ui
branch
from
7701eea to
500d7ff
Compare
komret
requested review from
MiroslavProchazka,
tomasklim and
matejkriz
as code owners
![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 (12)
packages/suite/src/reducers/suite/suiteReducer.ts (2)
518-519: Avoid double negation in variable naming.Renaming or using the existing
selectIsFirmwareRevisionCheckEnabledselector would reduce confusion and improve clarity.Here’s a suggested refactor:
- const isFirmwareRevisionCheckDisabled = - !state.suite.settings.enabledSecurityChecks.firmwareRevision; + const isFirmwareRevisionEnabled = + state.suite.settings.enabledSecurityChecks.firmwareRevision; if (!isFirmwareRevisionEnabled) { // ... }
552-552: Use positive naming for readability.Applying the same approach here (using an "enabled" variable) provides a cleaner structure and consistency.
- const isFirmwareHashCheckDisabled = !state.suite.settings.enabledSecurityChecks.firmwareHash; + const isFirmwareHashCheckEnabled = state.suite.settings.enabledSecurityChecks.firmwareHash; if (!isFirmwareHashCheckEnabled) { return null; }packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/FirmwareRevisionOptOutModal.tsx (1)
1-69: Consider sharing common modal logic.Both
FirmwareRevisionOptOutModalandDeviceAuthenticityOptOutModalshare very similar structure and behavior. Consider extracting common logic into a reusable component.packages/suite/src/views/settings/SettingsDevice/FirmwareAuthenticityChecks.tsx (1)
23-26: Consider extracting the check logic into a selector.The comment explains the behavior well, but the logic for determining if all checks are enabled could be moved to a selector for better reusability and testing.
+// In suiteReducer.ts +export const selectAreAllFirmwareChecksEnabled = createSelector( + [selectIsFirmwareHashCheckEnabled, selectIsFirmwareRevisionCheckEnabled], + (hashEnabled, revisionEnabled) => hashEnabled && revisionEnabled +); // In this file -const areAllFirmwareChecksEnabled = - isFirmwareHashCheckEnabled && isFirmwareRevisionCheckEnabled; +const areAllFirmwareChecksEnabled = useSelector(selectAreAllFirmwareChecksEnabled);packages/suite/src/components/suite/SecurityCheck/DeviceCompromised.tsx (1)
78-84: Consider adding a more descriptive comment.The comment could be more explicit about the security implications of allowing wallet access.
- // Only let user access the wallet if it may have been initiated before so that they can access the funds and send them to safety. + // Security measure: Allow wallet access only for previously initiated wallets to enable users to secure their funds by transferring them to safety.packages/suite/src/components/suite/Preloader/Preloader.tsx (1)
107-114: Consider extracting the condition to a named function.The complex condition combining firmware and entropy checks could be more readable if extracted to a named function.
+const shouldShowDeviceCompromised = ( + isFirmwareAuthenticityCheckDismissed: boolean, + isFirmwareCheckFailed: boolean, + isEntropyCheckEnabledAndFailed: boolean, + route?: { app?: string } +) => + (route?.app === undefined || !ROUTES_TO_SKIP_FIRMWARE_CHECK.includes(route?.app)) && + ((!isFirmwareAuthenticityCheckDismissed && isFirmwareCheckFailed) || + isEntropyCheckEnabledAndFailed); + if ( - (router.route?.app === undefined || - !ROUTES_TO_SKIP_FIRMWARE_CHECK.includes(router.route?.app)) && - ((!isFirmwareAuthenticityCheckDismissed && isFirmwareCheckFailed) || - isEntropyCheckEnabledAndFailed) + shouldShowDeviceCompromised( + isFirmwareAuthenticityCheckDismissed, + isFirmwareCheckFailed, + isEntropyCheckEnabledAndFailed, + router.route + ) ) { return <DeviceCompromised isEntropyCheckFailed={isEntropyCheckEnabledAndFailed} />; }packages/suite/src/actions/settings/deviceSettingsActions.ts (1)
189-196: Consider documenting the temporary error blacklist.The blacklist is marked as temporary but lacks documentation about when/how it will be updated.
Add a more detailed TODO comment explaining the criteria for updating the blacklist:
- const hardErrors: string[] = [ - 'Invalid session', // returned from firmware when entropy check is not supported - 'Missing verifyEntropy data', // entropy check fail - 'verifyEntropy xpub mismatch', // entropy check fail - ]; // TODO: This is a temporary blacklist to prevent false positives. + // TODO: This is a temporary blacklist to prevent false positives during the entropy check rollout. + // It will be refined based on user feedback and error patterns. + // Criteria for updating: + // 1. New firmware versions may introduce different error messages + // 2. User reports of legitimate failures being flagged + // 3. Analysis of error frequency and patterns + const hardErrors: string[] = [ + 'Invalid session', // returned from firmware when entropy check is not supported + 'Missing verifyEntropy data', // entropy check fail + 'verifyEntropy xpub mismatch', // entropy check fail + ];packages/suite/src/actions/suite/storageActions.ts (1)
466-472: Add error handling for database operations.While the implementation is correct, it would be beneficial to add error handling for the database operation to ensure robustness.
export const saveEntropyCheckFail = () => async (_dispatch: Dispatch, getState: GetState) => { if (!(await db.isAccessible())) return; const { devicesWithFailedEntropyCheck } = getState().device; if (!devicesWithFailedEntropyCheck) return; - db.addItem('security', { devicesWithFailedEntropyCheck }, 'security', true); + try { + await db.addItem('security', { devicesWithFailedEntropyCheck }, 'security', true); + } catch (error) { + console.error('Failed to save entropy check failure:', error); + } };suite-common/wallet-core/src/device/deviceReducer.ts (2)
40-41: Consider making devicesWithFailedEntropyCheck non-optional.Since the reducer initializes the array when needed, consider making this property non-optional to improve type safety.
- devicesWithFailedEntropyCheck?: string[]; + devicesWithFailedEntropyCheck: string[];
642-647: Consider deduplicating device IDs in the array.To prevent duplicate entries, consider using a Set or checking if the device ID already exists.
.addCase(deviceActions.setEntropyCheckFail, (state, { payload }) => { if (!state.devicesWithFailedEntropyCheck) { state.devicesWithFailedEntropyCheck = []; } - state.devicesWithFailedEntropyCheck.push(payload); + if (!state.devicesWithFailedEntropyCheck.includes(payload)) { + state.devicesWithFailedEntropyCheck.push(payload); + } })packages/suite/src/support/messages.ts (1)
6893-6896: Message entry looks good but could be more informativeThe message entry for entropy check failure follows the correct format and conventions. However, the message could be more informative by providing guidance on what the user should do next.
Consider expanding the message to include next steps:
- defaultMessage: 'Security check verifying entropy failed during wallet creation.', + defaultMessage: 'Security check verifying entropy failed during wallet creation. Please contact Trezor Support for assistance.',packages/suite/src/storage/CHANGELOG.md (1)
8-8: Clarify and Enhance Changelog Entry for Security Devices TrackingThe new bullet point on line 8 documents the addition of the
security.devicesWithFailedEntropyCheckproperty. To improve maintainability and clarity, consider expanding this entry with a brief description of its purpose (i.e., tracking devices that have failed the entropy check) and its linkage with the corresponding action (setEntropyCheckFail) and persistence logic. This added context can help developers quickly understand the impact of this change when reviewing the changelog in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts(1 hunks)packages/suite-web/e2e/support/utils/shortcuts.ts(1 hunks)packages/suite/src/actions/settings/deviceSettingsActions.ts(4 hunks)packages/suite/src/actions/suite/constants/suiteConstants.ts(1 hunks)packages/suite/src/actions/suite/storageActions.ts(1 hunks)packages/suite/src/actions/suite/suiteActions.ts(2 hunks)packages/suite/src/components/suite/Preloader/Preloader.tsx(4 hunks)packages/suite/src/components/suite/Preloader/__tests__/Preloader.test.tsx(2 hunks)packages/suite/src/components/suite/SecurityCheck/DeviceCompromised.tsx(4 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/DeviceAuthenticityOptOutModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/FirmwareRevisionOptOutModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx(1 hunks)packages/suite/src/middlewares/wallet/storageMiddleware.ts(2 hunks)packages/suite/src/reducers/suite/suiteReducer.ts(6 hunks)packages/suite/src/storage/CHANGELOG.md(1 hunks)packages/suite/src/storage/definitions.ts(1 hunks)packages/suite/src/storage/index.ts(1 hunks)packages/suite/src/storage/migrations/index.ts(1 hunks)packages/suite/src/support/extraDependencies.ts(1 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/support/suite/preloadStore.ts(2 hunks)packages/suite/src/utils/onboarding/steps.ts(1 hunks)packages/suite/src/views/onboarding/steps/ResetDevice.tsx(1 hunks)packages/suite/src/views/onboarding/steps/SecurityCheck/SecurityCheck.tsx(2 hunks)packages/suite/src/views/settings/SettingsDebug/CheckFirmwareAuthenticity.tsx(1 hunks)packages/suite/src/views/settings/SettingsDevice/DeviceAuthenticityOptOut.tsx(4 hunks)packages/suite/src/views/settings/SettingsDevice/FirmwareAuthenticityChecks.tsx(4 hunks)packages/suite/src/views/settings/SettingsDevice/SettingsDevice.tsx(2 hunks)suite-common/message-system/src/messageSystemTypes.ts(1 hunks)suite-common/suite-types/src/modal.ts(1 hunks)suite-common/wallet-core/src/device/deviceActions.ts(2 hunks)suite-common/wallet-core/src/device/deviceReducer.ts(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/suite/src/storage/index.ts
- suite-common/suite-types/src/modal.ts
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: e2e-test-suite-web (@group_other, trezor-user-env-unix, 1)
- GitHub Check: e2e-test-suite-web (@group_passphrase, trezor-user-env-unix, 1)
- GitHub Check: Linting and formatting
- GitHub Check: Type Checking
- GitHub Check: Unit Tests
- 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: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
🔇 Additional comments (45)
packages/suite/src/reducers/suite/suiteReducer.ts (4)
105-110: Good consolidation of security checks.Encapsulating all security checks under
enabledSecurityChecksimproves clarity and reduces scattered boolean flags.
179-184: Sensible default values.All checks defaulting to
truealigns with secure-by-default principles.
491-499: Selectors are well-defined.Each selector cleanly retrieves its respective check. This improves code readability and reusability.
351-362: Validate toggles receive a boolean payload.The toggle actions cleanly set each check to the payload value. However, ensure the dispatched payloads are always booleans.
Please run the following script to confirm all dispatch calls align with a boolean payload:
✅ Verification successful
Toggle actions are correctly dispatched with boolean payloads.
- The action type definitions in
packages/suite/src/actions/suite/suiteActions.tsexplicitly require a boolean payload.- In components like
packages/suite/src/views/settings/SettingsDebug/CheckFirmwareAuthenticity.tsx, the payload is coerced to boolean (using!!isChecked) or set to a literal boolean.- All instances from the search output confirm that the dispatched payloads are strictly boolean.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash rg -A 3 "TOGGLE_DEVICE_AUTHENTICITY_CHECK" rg -A 3 "TOGGLE_FIRMWARE_REVISION_CHECK" rg -A 3 "TOGGLE_FIRMWARE_HASH_CHECK" rg -A 3 "TOGGLE_ENTROPY_CHECK"Length of output: 10830
suite-common/message-system/src/messageSystemTypes.ts (1)
26-28: Consistent naming for firmware and entropy checks.Renaming
firmwareRevisionCheckand addingentropyCheckkeeps feature labels aligned with the new toggling approach.packages/suite/src/actions/suite/constants/suiteConstants.ts (1)
24-27: Clear toggling constants for security checks.Introducing toggle-based constants enhances the manageability of these checks and corresponds well with the reducer changes.
packages/suite/src/views/settings/SettingsDevice/DeviceAuthenticityOptOut.tsx (3)
4-4: LGTM! Clean transition to toggle-based state management.The changes consistently implement the new toggle pattern, with appropriate selector and action naming.
Also applies to: 13-13, 17-17
19-24: LGTM! Logical handler implementation.The
handleClickfunction correctly manages the modal flow:
- Opens modal when check is enabled (requiring confirmation)
- Directly toggles when disabled (no confirmation needed)
32-34: LGTM! Consistent UI state handling.The UI elements appropriately reflect the check's state:
- Translation keys match enabled/disabled states
- Button variant changes to 'destructive' when disabling the check
- Data-testid follows the new pattern
Also applies to: 41-43, 52-53, 57-59
packages/suite/src/utils/onboarding/steps.ts (1)
36-44: LGTM! Clean refactor of authentication step logic.The changes maintain the same security level while simplifying the code:
- Cleaner state access through
enabledSecurityChecks.deviceAuthenticity- Logical condition remains secure with bootloader check
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/DeviceAuthenticityOptOutModal.tsx (1)
6-6: LGTM! Consistent implementation of toggle pattern.The modal correctly implements the new toggle pattern:
- Clean import of new action
- Simplified dispatch with boolean parameter
Also applies to: 19-19
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/FirmwareRevisionOptOutModal.tsx (1)
6-6: LGTM! Consistent implementation of toggle pattern.The changes mirror those in DeviceAuthenticityOptOutModal:
- Clean import of new toggle action
- Simplified dispatch with boolean parameter
Also applies to: 19-19
packages/suite/src/views/settings/SettingsDebug/CheckFirmwareAuthenticity.tsx (3)
18-22: Consider renaming toggle actions to better reflect their behavior.The toggle functions are setting specific values rather than just toggling state. Based on past feedback, consider renaming these to use
SETinstead ofTOGGLEto better reflect their behavior.
36-44: LGTM! Well-structured UI component for entropy check.The new entropy check section follows the established pattern and provides clear, descriptive text.
18-32: 🛠️ Refactor suggestionRemove unnecessary type casting and optional parameters.
The
isCheckedparameter is marked as optional but is always provided by the Switch component. Remove the optional parameter and type casting:-const toggleEntropyCheck = (isChecked?: boolean) => +const toggleEntropyCheck = (isChecked: boolean) => dispatch({ type: SUITE.TOGGLE_ENTROPY_CHECK, - payload: !!isChecked, + payload: isChecked, });Apply the same changes to
toggleFirmwareHashCheckandtoggleFirmwareRevisionCheck.Likely invalid or redundant comment.
packages/suite/src/views/settings/SettingsDevice/FirmwareAuthenticityChecks.tsx (1)
28-33: LGTM! Clear conditional dispatch logic.The
handleClickfunction clearly handles both enable and disable cases.packages/suite/src/support/suite/preloadStore.ts (1)
47-47: LGTM! Clean addition of security preload.The security item is loaded and included in the payload following the established pattern.
Also applies to: 71-71
suite-common/wallet-core/src/device/deviceActions.ts (1)
90-93: LGTM! Clean implementation of entropy check failure action.The new action creator follows the established pattern and is properly exported.
Also applies to: 110-110
packages/suite/src/storage/definitions.ts (1)
134-139: LGTM! Well-structured schema addition for security data.The new
securityproperty follows the established schema patterns and correctly defines the storage structure for tracking devices that fail entropy checks.packages/suite/src/components/suite/SecurityCheck/DeviceCompromised.tsx (2)
15-55: LGTM! Well-structured hook implementation.The
useSecurityCheckFailPropshook effectively handles multiple security check scenarios, including the new entropy check, with clear precedence rules and proper error state management.
57-59: LGTM! Clear type definition.The
DeviceCompromisedPropstype is well-defined and properly documents the required props.packages/suite/src/components/suite/Preloader/Preloader.tsx (1)
104-106: LGTM! Clear and concise condition.The
isEntropyCheckEnabledAndFailedvariable effectively combines multiple conditions into a readable boolean expression.packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts (1)
121-122: LGTM! Consistent with new toggle-based approach.The action dispatch has been correctly updated to use the new toggle-based approach with a boolean payload.
packages/suite/src/views/onboarding/steps/ResetDevice.tsx (1)
78-78: LGTM! Clean removal of entropy check bypass.The removal of
isEntropyErrorstate and its associated logic aligns with the PR objectives to remove temporary code that bypassed entropy checks during rollout. The dependency array update is correct.packages/suite/src/views/settings/SettingsDevice/SettingsDevice.tsx (2)
25-25: LGTM! Clean import update.The import statement is correctly updated to use the new
FirmwareAuthenticityCheckscomponent.
177-177: LGTM! Appropriate placement of firmware checks.The
FirmwareAuthenticityCheckscomponent is correctly placed in the advanced settings section, which is an appropriate location for this type of functionality.packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx (2)
95-96: LGTM! Consistent naming for device authenticity check.The case label update maintains functionality while using more consistent terminology.
97-98: LGTM! Consistent naming for firmware authenticity checks.The case label update aligns with the broader refactor to standardize device checks across the application.
packages/suite-web/e2e/support/utils/shortcuts.ts (1)
101-102: LGTM! Simplified action structure.The update to use
TOGGLE_FIRMWARE_HASH_CHECKwith a boolean payload simplifies the action structure while maintaining the same functionality.packages/suite/src/support/extraDependencies.ts (1)
208-208: LGTM! Property added for tracking failed entropy checks.The addition of
devicesWithFailedEntropyCheckproperty with optional chaining is well-implemented.packages/suite/src/actions/settings/deviceSettingsActions.ts (2)
156-156: LGTM! Entropy check state retrieval added.The selector is correctly used to retrieve the entropy check enabled state.
172-172: LGTM! Entropy check parameter added to resetDevice.The entropy check state is correctly passed to the TrezorConnect.resetDevice call.
packages/suite/src/actions/suite/suiteActions.ts (2)
41-44: LGTM! Consistent toggle action types added.The new toggle actions follow a consistent naming pattern and use boolean payloads.
296-303: LGTM! Functions updated to use toggle pattern.The functions have been renamed and simplified to use the new toggle pattern consistently.
Also applies to: 305-314
packages/suite/src/views/onboarding/steps/SecurityCheck/SecurityCheck.tsx (2)
277-282: LGTM! Improved state access pattern.The state access has been updated to use a more direct and type-safe path to security check settings.
295-295: LGTM! Clearer authenticity check condition.The condition now uses a positive check (
isEnabledvs!isDisabled), making the intent clearer.packages/suite/src/middlewares/wallet/storageMiddleware.ts (2)
237-239: LGTM! Security check toggles are properly handled.The middleware correctly saves suite settings when security check toggles are modified.
321-323: LGTM! Entropy check failures are properly persisted.The middleware correctly dispatches the action to save entropy check failures to storage.
packages/suite/src/components/suite/Preloader/__tests__/Preloader.test.tsx (4)
46-55: LGTM! Initial state setup is comprehensive.The test setup correctly includes all security check flags in the initial state.
494-516: LGTM! Entropy check failure test is well-structured.The test properly verifies the UI behavior when a device fails the entropy check.
518-543: LGTM! Firmware hash check failure test is well-structured.The test properly verifies the UI behavior when a device fails the firmware hash check.
545-570: LGTM! Firmware revision check failure test is well-structured.The test properly verifies the UI behavior when a device fails the firmware revision check.
suite-common/wallet-core/src/device/deviceReducer.ts (1)
855-859: LGTM! Selector implementation is correct.The selector properly checks if the selected device's ID is in the list of devices that failed the entropy check.
packages/suite/src/storage/migrations/index.ts (2)
1255-1255: LGTM! The security store creation aligns with the entropy check feature.The addition of the
securityobject store will enable persistence of entropy check results, which is a key requirement for this feature.
1253-1253: Verify idempotency of migrationCoinmarketToTrading.The version check for
migrationCoinmarketToTradinghas been removed, making it run unconditionally. Please ensure that this migration is idempotent and can safely run multiple times without corrupting data.Run the following script to verify the migration's idempotency:
packages/suite/src/components/suite/Preloader/__tests__/Preloader.test.tsx
Show resolved
Hide resolved
| }, | ||
| }; | ||
|
|
||
| const store = initStore( |
There was a problem hiding this comment.
maybe nit but it can be inline very easily and AFAIK it adds to the readability const store = initStore(getInitialState({ device }));
There was a problem hiding this comment.
It is used twice so needs to be stored in a variable.
| unmount(); | ||
| }); | ||
|
|
||
| it('Failed firmware revision check', () => { |
There was a problem hiding this comment.
Those 3 tests are basically copy-paste and could be easily replaced by data-provider
| @@ -70,6 +70,9 @@ export const Preloader = ({ children }: PropsWithChildren) => { | |||
| selectIsFirmwareAuthenticityCheckDismissed, | |||
| ); | |||
| const isEntropyCheckEnabled = useSelector(selectIsEntropyCheckEnabled); | |||
| const isEntropyCheckDisabledByMessageSystem = useSelector(state => | |||
There was a problem hiding this comment.
I think this shall be handled in selectFirmwareRevisionCheckErrorIfEnabled together with other errors of this kind
There was a problem hiding this comment.
The entropy check is handled differently than revision/hash check so I did it separately in selectIsEntropyCheckEnabledAndFailed. See c88d495.
|
Added 6fb41cb to actually disable the check, not just the UI. This is necessary in case there is a bug in the check itself which might block wallet creation.n UI must also be disabled in case the state is persisted. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/suite/src/actions/settings/deviceSettingsActions.ts (1)
193-197: Consider defining error messages as constants.The hardcoded error messages in the temporary blacklist could be moved to constants for better maintainability.
+const ENTROPY_CHECK_HARD_ERRORS = { + INVALID_SESSION: 'Invalid session', + MISSING_VERIFY_DATA: 'Missing verifyEntropy data', + XPUB_MISMATCH: 'verifyEntropy xpub mismatch', +} as const; const hardErrors: string[] = [ - 'Invalid session', - 'Missing verifyEntropy data', - 'verifyEntropy xpub mismatch', + ENTROPY_CHECK_HARD_ERRORS.INVALID_SESSION, + ENTROPY_CHECK_HARD_ERRORS.MISSING_VERIFY_DATA, + ENTROPY_CHECK_HARD_ERRORS.XPUB_MISMATCH, ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/suite/src/actions/settings/deviceSettingsActions.ts(5 hunks)packages/suite/src/components/suite/Preloader/Preloader.tsx(4 hunks)packages/suite/src/components/suite/Preloader/__tests__/Preloader.test.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/suite/src/components/suite/Preloader/Preloader.tsx
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: e2e-test-suite-web (@group=other, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group=metadata2, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group=metadata1, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group=settings, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group=suite, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group_wallet, trezor-user-env-unix bitcoin-regtest, 1)
- GitHub Check: e2e-test-suite-web (@group_other, trezor-user-env-unix, 1)
- GitHub Check: e2e-test-suite-web (@group_passphrase, trezor-user-env-unix, 1)
- GitHub Check: e2e-test-suite-web (@group_device-management, trezor-user-env-unix, 1)
- GitHub Check: e2e-test-suite-web (@group_suite, trezor-user-env-unix, 1)
- GitHub Check: Unit Tests
- GitHub Check: Type Checking
- 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: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (4)
packages/suite/src/actions/settings/deviceSettingsActions.ts (2)
157-161: LGTM! Well-structured feature flag implementation.The entropy check can be controlled via both user preference and message system override, providing flexibility in managing the feature.
Also applies to: 176-176
181-192: LGTM! Comprehensive error reporting implementation.The error reporting includes all necessary device details and handles specific error codes appropriately.
packages/suite/src/components/suite/Preloader/__tests__/Preloader.test.tsx (2)
46-55: LGTM! Well-structured initial state setup.The settings object properly includes all security check flags with clear naming.
110-149: LGTM! Excellent data-driven test implementation.The test cases are well-structured using fixtures, making them maintainable and easy to extend. The coverage includes all critical security check scenarios.
Also applies to: 535-551
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/suite/src/views/settings/SettingsDebug/DeviceAuthenticity.tsx (1)
21-21: Add data-testid to the Switch component.While the parent SectionItem has a data-testid, adding one to the Switch component itself would improve testability by allowing direct interaction testing.
- <Switch onChange={handleChange} isChecked={debug.isUnlockedBootloaderAllowed} /> + <Switch + data-testid="@settings/debug/device-authenticity/toggle" + onChange={handleChange} + isChecked={debug.isUnlockedBootloaderAllowed} + />packages/suite/src/components/suite/SecurityCheck/DeviceCompromised.tsx (2)
16-20: LGTM with a minor suggestion.Good change to return a complete
SecurityCheckFailPropsobject instead of a partial one for better type safety.Consider extracting the selector into a custom hook for better reusability:
-const isEntropyCheckFailed = useSelector(selectIsEntropyCheckEnabledAndFailed); +const useIsEntropyCheckFailed = () => useSelector(selectIsEntropyCheckEnabledAndFailed); +const isEntropyCheckFailed = useIsEntropyCheckFailed();
31-39: Track the TODO comment for the support URL.The entropy check failure handling looks good, with appropriate error messages and safety measures like disabling back navigation.
Would you like me to create an issue to track the TODO for adding a specific support URL for entropy check failures?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/suite/src/components/suite/Preloader/Preloader.tsx(3 hunks)packages/suite/src/components/suite/SecurityCheck/DeviceCompromised.tsx(2 hunks)packages/suite/src/reducers/suite/suiteReducer.ts(8 hunks)packages/suite/src/views/settings/SettingsDebug/DeviceAuthenticity.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/suite/src/reducers/suite/suiteReducer.ts
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Releases revision Checks
- GitHub Check: Other Checks
- GitHub Check: Unit Tests
- GitHub Check: Build libs for publishing
- GitHub Check: Type Checking
- GitHub Check: Linting and formatting
- 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: EAS Update
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
🔇 Additional comments (6)
packages/suite/src/views/settings/SettingsDebug/DeviceAuthenticity.tsx (1)
11-12: LGTM! Type safety improvement.The change from optional to required boolean parameter improves type safety and better reflects the actual usage, as the Switch component always provides a boolean value.
packages/suite/src/components/suite/SecurityCheck/DeviceCompromised.tsx (2)
3-3: LGTM!The new imports are correctly added to support the entropy check functionality.
Also applies to: 10-10
81-81: LGTM!The component correctly spreads all props from
securityCheckFailProps, aligning with the updated hook that returns a complete props object.packages/suite/src/components/suite/Preloader/Preloader.tsx (3)
13-13: LGTM!The new selector import is correctly added to support the entropy check functionality.
64-72: Consider consolidating error handling.As suggested in past reviews, consider handling all security check failures (firmware and entropy) in a single selector for better maintainability.
Create a new selector that combines all security checks:
const selectIsAnySecurityCheckFailed = createSelector( [ selectIsFirmwareAuthenticityCheckEnabledAndHardFailed, selectIsFirmwareAuthenticityCheckDismissed, selectIsEntropyCheckEnabledAndFailed ], (isFirmwareCheckFailed, isFirmwareCheckDismissed, isEntropyCheckFailed) => ({ isCompromised: (!isFirmwareCheckDismissed && isFirmwareCheckFailed) || isEntropyCheckFailed, reason: isEntropyCheckFailed ? 'entropy' : 'firmware' }) );
99-106: LGTM!The device compromise detection correctly handles both firmware and entropy check failures, with a clear comment explaining the need to check if entropy check is enabled.
| @@ -118,6 +107,47 @@ const initStore = (state: State) => { | |||
|
|
|||
| const Index = ({ app }: any) => <Preloader>{app || 'foo'}</Preloader>; | |||
|
|
|||
| const deviceCompromisedFixtures = [ | |||
There was a problem hiding this comment.
note: adding a declarative typing to the fixture helps a lot when working with it in IDE as it gives you auto-completetion
|
/rebase |
|
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/13133721797 |
… API with other checks
trezor-ci
force-pushed
the
feat/entropy-check-ui
branch
from
394458d to
9f6eeeb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/suite/src/views/settings/SettingsDevice/DeviceAuthenticityOptOut.tsx (1)
53-53: Consider updating the test ID to reflect the toggle approach.The current test ID still references "opt-out" which doesn't align with the new toggle approach.
- data-testid="@settings/device/open-device-authenticity-check-opt-out-modal-button" + data-testid="@settings/device/toggle-device-authenticity-check-button"packages/suite/src/components/suite/SecurityCheck/DeviceCompromised.tsx (2)
25-28: Improve TypeScript-related comment.The current comment could be more descriptive about why device.id is always defined at this point.
- // Condition to satisfy TypeScript, device.id is always defined at this point. + // TypeScript check: device.id is always defined here as this function is only called + // when a device is connected and has failed a security check.
71-72: Make the default case more explicit.The current default case could be more informative about why it returns only the support URL.
- // should not happen, but default props will be used with no problem - return { supportUrl: TREZOR_SUPPORT_FW_REVISION_CHECK_FAILED_URL }; + // Default case: no security check failures detected + // Only supportUrl is required by SecurityCheckFailProps type + return { + supportUrl: TREZOR_SUPPORT_FW_REVISION_CHECK_FAILED_URL, + };packages/suite/src/actions/settings/deviceSettingsActions.ts (1)
193-197: Consider moving the hardcoded error list to constants.The temporary blacklist of hard errors should be moved to a constants file for better maintainability and reusability.
+// In constants file +export const ENTROPY_CHECK_HARD_ERRORS = [ + 'Invalid session', + 'Missing verifyEntropy data', + 'verifyEntropy xpub mismatch', +] as const; -const hardErrors: string[] = [ - 'Invalid session', // returned from firmware when entropy check is not supported - 'Missing verifyEntropy data', // entropy check fail - 'verifyEntropy xpub mismatch', // entropy check fail -]; // TODO: This is a temporary blacklist to prevent false positives. +const hardErrors = ENTROPY_CHECK_HARD_ERRORS;packages/suite/src/reducers/suite/suiteReducer.ts (1)
496-503: Consider memoizing the selectors for performance optimization.These selectors are simple but might be called frequently. Consider using
createSelectorfromreselectto memoize them.+import { createSelector } from 'reselect'; -export const selectIsDeviceAuthenticityCheckEnabled = (state: SuiteRootState) => - state.suite.settings.enabledSecurityChecks.deviceAuthenticity; +export const selectIsDeviceAuthenticityCheckEnabled = createSelector( + (state: SuiteRootState) => state.suite.settings.enabledSecurityChecks.deviceAuthenticity, + (enabled) => enabled +);packages/suite/src/storage/migrations/index.ts (1)
1255-1255: Consider adding indexes to the security object store.The
securityobject store is created without any indexes. If you plan to query devices that have failed the entropy check, consider adding an index for better query performance.Apply this diff to add an index:
- db.createObjectStore('security'); + const securityStore = db.createObjectStore('security'); + securityStore.createIndex('deviceId', 'deviceId', { unique: false });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
packages/components/src/components/form/Switch/Switch.tsx(1 hunks)packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts(1 hunks)packages/suite-web/e2e/support/utils/shortcuts.ts(1 hunks)packages/suite/src/actions/settings/deviceSettingsActions.ts(5 hunks)packages/suite/src/actions/suite/constants/suiteConstants.ts(1 hunks)packages/suite/src/actions/suite/storageActions.ts(1 hunks)packages/suite/src/actions/suite/suiteActions.ts(2 hunks)packages/suite/src/components/suite/Preloader/Preloader.tsx(3 hunks)packages/suite/src/components/suite/Preloader/__tests__/Preloader.test.tsx(3 hunks)packages/suite/src/components/suite/SecurityCheck/DeviceCompromised.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/DeviceAuthenticityOptOutModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/FirmwareRevisionOptOutModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx(1 hunks)packages/suite/src/middlewares/wallet/storageMiddleware.ts(2 hunks)packages/suite/src/reducers/suite/suiteReducer.ts(8 hunks)packages/suite/src/storage/CHANGELOG.md(1 hunks)packages/suite/src/storage/definitions.ts(1 hunks)packages/suite/src/storage/index.ts(1 hunks)packages/suite/src/storage/migrations/index.ts(1 hunks)packages/suite/src/support/extraDependencies.ts(1 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/support/suite/preloadStore.ts(2 hunks)packages/suite/src/utils/onboarding/steps.ts(1 hunks)packages/suite/src/views/onboarding/steps/ResetDevice.tsx(1 hunks)packages/suite/src/views/onboarding/steps/SecurityCheck/SecurityCheck.tsx(2 hunks)packages/suite/src/views/settings/SettingsDebug/CheckFirmwareAuthenticity.tsx(1 hunks)packages/suite/src/views/settings/SettingsDebug/DeviceAuthenticity.tsx(1 hunks)packages/suite/src/views/settings/SettingsDevice/DeviceAuthenticityOptOut.tsx(4 hunks)packages/suite/src/views/settings/SettingsDevice/FirmwareAuthenticityChecks.tsx(4 hunks)packages/suite/src/views/settings/SettingsDevice/SettingsDevice.tsx(2 hunks)suite-common/message-system/src/messageSystemTypes.ts(1 hunks)suite-common/suite-types/src/modal.ts(1 hunks)suite-common/wallet-core/src/device/deviceActions.ts(2 hunks)suite-common/wallet-core/src/device/deviceReducer.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (24)
- packages/suite/src/storage/index.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx
- packages/components/src/components/form/Switch/Switch.tsx
- packages/suite/src/views/settings/SettingsDevice/SettingsDevice.tsx
- packages/suite/src/storage/CHANGELOG.md
- packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/FirmwareRevisionOptOutModal.tsx
- packages/suite-web/e2e/support/utils/shortcuts.ts
- packages/suite/src/actions/suite/storageActions.ts
- packages/suite/src/storage/definitions.ts
- packages/suite/src/support/extraDependencies.ts
- packages/suite/src/views/settings/SettingsDebug/DeviceAuthenticity.tsx
- suite-common/wallet-core/src/device/deviceActions.ts
- suite-common/suite-types/src/modal.ts
- suite-common/message-system/src/messageSystemTypes.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/DeviceAuthenticityOptOutModal.tsx
- packages/suite/src/views/onboarding/steps/ResetDevice.tsx
- packages/suite/src/views/settings/SettingsDebug/CheckFirmwareAuthenticity.tsx
- packages/suite/src/support/suite/preloadStore.ts
- packages/suite/src/components/suite/Preloader/Preloader.tsx
- packages/suite/src/views/settings/SettingsDevice/FirmwareAuthenticityChecks.tsx
- packages/suite/src/support/messages.ts
- packages/suite/src/components/suite/Preloader/tests/Preloader.test.tsx
- suite-common/wallet-core/src/device/deviceReducer.ts
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Type Checking
- GitHub Check: Releases revision Checks
- GitHub Check: Other Checks
- GitHub Check: Build libs for publishing
- GitHub Check: Linting and formatting
- GitHub Check: Unit Tests
- 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: EAS Update
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (18)
packages/suite/src/actions/suite/constants/suiteConstants.ts (1)
24-27: LGTM! The new toggle constants align with the standardized approach.The changes effectively transition from opt-out to toggle actions, providing a more consistent way to manage security checks.
packages/suite/src/views/settings/SettingsDevice/DeviceAuthenticityOptOut.tsx (1)
19-24: LGTM! The toggle logic is well-implemented.The component correctly handles both enabled and disabled states, with appropriate modal interaction.
packages/suite/src/utils/onboarding/steps.ts (1)
36-38: LGTM! Clean transition to the new security checks structure.The code maintains its functionality while being more direct in accessing the device authenticity check state.
packages/suite/src/components/suite/SecurityCheck/DeviceCompromised.tsx (1)
31-39: LGTM! Well-implemented entropy check failure handling.The implementation correctly handles entropy check failures with appropriate UI messaging and support URL.
packages/suite/src/actions/settings/deviceSettingsActions.ts (3)
157-161: LGTM! Entropy check state management is well-implemented.The implementation correctly retrieves the entropy check state and message system override, providing a robust way to control the feature.
176-176: LGTM! Entropy check parameter is correctly applied.The entropy check is enabled only when both conditions are met: the check is enabled in settings and not disabled by the message system.
198-200: LGTM! Error handling and state update.The implementation correctly updates the device state when a hard error is encountered and the device ID is available.
packages/suite/src/actions/suite/suiteActions.ts (1)
41-44: LGTM! Consistent toggle actions implementation.The new toggle actions follow a consistent pattern using boolean payloads, making the code more maintainable and easier to understand.
packages/suite/src/views/onboarding/steps/SecurityCheck/SecurityCheck.tsx (2)
290-295: LGTM! Clean state management implementation.The selectors are well-organized and follow the established patterns for accessing state.
304-309: LGTM! Comprehensive device authentication checks.The implementation correctly checks all required conditions before authenticating a device:
- Device model support
- Initial run state
- Enabled security check
- Debug device status
packages/suite/src/middlewares/wallet/storageMiddleware.ts (2)
237-239: LGTM! Consistent storage handling for toggle actions.The toggle action cases are well-integrated with the existing storage middleware pattern.
321-323: LGTM! Entropy check failure persistence.The implementation correctly persists entropy check failures, ensuring the state is maintained across sessions.
packages/suite/src/reducers/suite/suiteReducer.ts (5)
110-115: LGTM! Well-structured security checks configuration.The new
enabledSecurityChecksobject provides a clean and maintainable way to manage multiple security checks in one place.
184-189: LGTM! Sensible default values for security checks.All security checks are enabled by default, which is the correct approach for maintaining security.
356-367: LGTM! Clear and consistent action handlers.The toggle action handlers follow a consistent pattern and correctly update the state using immer's draft.
585-597: LGTM! Comprehensive entropy check selector.The
selectIsEntropyCheckEnabledAndFailedselector correctly considers all three conditions:
- Check is enabled in settings
- Check is not disabled by message system
- Check has failed
523-524: LGTM! Clean integration with existing firmware check logic.The firmware check selectors have been updated to use the new
enabledSecurityChecksstructure while maintaining the existing behavior.Also applies to: 557-557
packages/suite/src/storage/migrations/index.ts (1)
1253-1253: Verify that the migration function is idempotent.The
migrationCoinmarketToTradingfunction is now called unconditionally without version checks. Please ensure that this function is idempotent to prevent data corruption if executed multiple times.Run the following script to check if the function has any safeguards against multiple executions:
✅ Verification successful
I'll run an additional script to locate the complete definition of
migrationCoinmarketToTradingand inspect its implementation for any idempotency safeguards.
Migration Idempotency and Security Store Index Suggestion
- The
migrationCoinmarketToTradingfunction checks for the existence of the old store (coinmarketTrades) withif (db.objectStoreNames.contains(oldStoreName)) { ... }. This conditional guard prevents re-running the migration if it has already been executed, ensuring idempotency.- The new
securityobject store is created unconditionally without defined indexes. Consider adding relevant indexes to facilitate efficient queries for the persisted entropy check data.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if migrationCoinmarketToTrading has idempotency checks. # Search for idempotency checks in the migration function ast-grep --pattern $'function migrationCoinmarketToTrading($_, $_, $_, $_) { $$$ if ($$$) { return; } $$$ }'Length of output: 121
Script:
#!/bin/bash # Description: Retrieve the full definition and surrounding context of migrationCoinmarketToTrading to check for idempotency measures. rg -n "migrationCoinmarketToTrading" -C 50 .Length of output: 17615
|
QA OK
Info: |
As checks are being introduced gradually, refactoring is necessary to keep the API reasonable. I added a new
enabledSecurityChecksproperty tosuite.settingswhich is a dictionary of checks with boolean values for enabled/disabled state. I did not create a migration because users shouldn't generally have any of the checks disabled. I only added the entropy check toggle to debug settings because there's probably no use case for disabling it other than playing with fake devices.We must use app state here because we need to persist the check result in case user disconnects and reconnects the device. This commit is temporary and will be expanded upon in the future. We must be careful during the roll-out to avoid false positives. In the future, we should improve error filtering and avoid relying on device id when detecting fake devices.
We reuse the "ghost modal" but omit the "back" button because in this case, the user's wallet should have no history and they won't need to access it to save their funds.
We must somehow persist the information about failed check in case user restarts Suite because in that case, they shouldn't repeat the check. This is because 1) the check is probabilistic and 2) wallet may have already been created despite the check failing. Again, we should avoid relying on device id in the future. Note that I decreased the DB version number to 52 because it is not necessary to increase it multiple times between releases.
Resolves https://github.com/trezor/trezor-suite-private/issues/123