Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: solve hash check on install false positives #15772

Merged
merged 2 commits into from
Dec 6, 2024

Conversation

Lemonexe
Copy link
Contributor

@Lemonexe Lemonexe commented Dec 4, 2024

Description

  1. after FW update, if hash check threw an error, don't persist the device as hard failure FW
  2. when device connects and succeeds automatic FW hash check (even when turned off), exonerate it from having failed FW hash check after update
  3. persist that in storage

Checkout testing branch to simulate hash mismatch, or other error (separate commits for each)

Related issue

😞 Reopens #5868

Screenshots

  1. simulated other error: only modal is displayed, but banner is not shown other error is now soft.webm
  2. simulated hash mismatch: banner is displayed at first, but reloading the suite clears it
    hash mismatch is exonerated.webm

Copy link

github-actions bot commented Dec 4, 2024

🚀 Expo preview is ready!

  • Project → trezor-suite-preview
  • Platforms → android, ios
  • Scheme → trezorsuitelite
  • Runtime Version → 17
  • More info

Learn more about 𝝠 Expo Github Action

@Lemonexe Lemonexe marked this pull request as ready for review December 4, 2024 15:38
@@ -86,6 +87,17 @@ export const prepareFirmwareReducer = createReducerWithExtraDeps(initialState, (
state.cachedDevice = payload;
})
.addCase(deviceActions.addButtonRequest, extra.reducers.addButtonRequestFirmware)
.addCase(deviceActions.connectDevice, (state, { payload: { device } }) => {
Copy link
Contributor Author

@Lemonexe Lemonexe Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 Similar condition in deviceReducer, but this is simpler because we don't care about unacquired devices. Then it's just one action we need to match, so we can use the simpler addCase instead of addMatcher

@Lemonexe Lemonexe marked this pull request as draft December 5, 2024 08:56
@Lemonexe Lemonexe marked this pull request as ready for review December 5, 2024 11:05
@Lemonexe Lemonexe force-pushed the feat/hash-check-on-install-false-positives branch from e5cb614 to 509ff96 Compare December 5, 2024 11:13
@@ -311,6 +311,8 @@ const storageMiddleware = (api: MiddlewareAPI<Dispatch, AppState>) => {
case FORM_DRAFT.REMOVE_DRAFT:
storageActions.removeFormDraft(action.key);
break;

case deviceActions.connectDevice.type: // so that firmwareReducer.addCase for the same action is persisted
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this works because next(action) is called at beginning.
So the firmwareReducer.addCase does its thing first, and only then this case block is executed to persist the newly updated state.firmware.firmwareHashInvalid.

@@ -164,7 +161,6 @@ export const firmwareUpdate = createThunk<
// device failed to respond to the hash check, consider the firmware counterfeit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please edit the comment, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Lemonexe
Copy link
Contributor Author

Lemonexe commented Dec 5, 2024

/rebase

Copy link

github-actions bot commented Dec 5, 2024

@trezor-ci trezor-ci force-pushed the feat/hash-check-on-install-false-positives branch from b051dd0 to 726fed6 Compare December 5, 2024 15:22
@Lemonexe Lemonexe merged commit e8fbc88 into develop Dec 6, 2024
28 checks passed
@Lemonexe Lemonexe deleted the feat/hash-check-on-install-false-positives branch December 6, 2024 10:10
@bosomt
Copy link
Contributor

bosomt commented Dec 6, 2024

QA OK

webUSB

  • T2T1 Universal 2.1.4 > Universal 2.8.1
  • T3T1 Universal 2.8.3 > Universal 2.8.3
  • T2B1 Universal 2.8.0 > Universal 2.8.0
  • T3B1 Universal 2.8.3 > Universal 2.8.3

Info:

  • Suite version: web 25.1.0 (0fc5924)
  • Browser: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/131.0.0.0 Safari/537.36
  • OS: MacIntel
  • Transport: WebUsbTransport

dev app

  • T2T1 Universal 2.1.8 > Universal 2.1.8
  • T3T1 Universal 2.8.3 > Universal 2.8.3
  • T2B1 Universal 2.8.0 > Universal 2.8.0
  • T3B1 Universal 2.8.3 > Universal 2.8.3

Info:

  • Suite version: desktop 25.1.0 (5557f53)
  • Browser: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) TrezorSuiteDev/25.1.0 Chrome/128.0.6613.186 Electron/32.2.6 Safari/537.36
  • OS: MacIntel
  • Transport: BridgeTransport 2.0.33

@bosomt
Copy link
Contributor

bosomt commented Dec 10, 2024

QA OK

APP

  • T3T1 Universal 2.8.6 > Universal 2.8.6
  • T3B1 Universal 2.8.3 > Universal 2.8.3
  • T1B1
  • T2T1 Universal 2.8.1 > Bitcoin-only 2.8.1
  • T2B1 Universal 2.8.0 > Universal 2.8.0

WEB

  • T3T1 Universal 2.8.6 > Universal 2.8.6
  • T2T1 Universal 2.8.1 > Bitcoin-only 2.8.1
  • T2B1 Universal 2.8.0 > Universal 2.8.0
  • T3B1 Universal 2.8.3 > Universal 2.8.3

Info:

  • Suite version: desktop 24.12.1 (0d4070d)
  • Browser: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) TrezorSuite/24.12.1 Chrome/128.0.6613.186 Electron/32.2.6 Safari/537.36
  • OS: MacIntel
  • Screen: 1512x982

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants