Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes update several core components. USB initialization now only runs once per execution. Firmware verification for the second part is now conditional based on firmware size. Some display calls in the bootloader UI are commented out. The SPI legacy handler now checks headers before processing data. The firmware update process clears the display and updates the status bar after cancellation. A new asynchronous function flushes the FIDO buffer when WebAuthn is disabled. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (3)
core/embed/extmod/modtrezorconfig/modtrezorconfig.c (2)
109-113:⚠️ Potential issueDocstring outdated
The text still promises a bool, but the function now returns a tuple.
118-124:⚠️ Potential issueUnused parameter
ext_saltis validated but never used.core/embed/trezorhal/se_thd89.c (1)
1125-1132:⚠️ Potential issueLength check mismatch
The check uses
PIN_MAX_LEN, but buffer size is hard‑coded to 64.Also applies to: 1166-1169
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
core/mocks/generated/trezorconfig.pyiis excluded by!**/generated/**core/mocks/generated/trezorcrypto/se_thd89.pyiis excluded by!**/generated/**
📒 Files selected for processing (4)
core/embed/extmod/modtrezorconfig/modtrezorconfig.c(2 hunks)core/embed/extmod/modtrezorcrypto/modtrezorcrypto-se-thd89.h(1 hunks)core/embed/trezorhal/se_thd89.c(9 hunks)core/embed/trezorhal/se_thd89.h(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
core/embed/extmod/modtrezorconfig/modtrezorconfig.c (4)
core/embed/trezorhal/se_thd89.h (1)
se_get_pin_type(102-102)core/embed/trezorhal/se_thd89.c (1)
se_get_pin_type(1347-1347)core/embed/fp_sensor_wrapper/fpsensor_platform.c (1)
fpsensor_data_init(399-449)core/embed/fp_sensor_wrapper/fpsensor_platform.h (1)
fpsensor_data_init(41-41)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Style check
- GitHub Check: Defs check
- GitHub Check: Gen check
core/embed/trezorhal/se_thd89.h
Outdated
| #define PIN_MAX_LENGTH (50) | ||
| #define PASSPHRASE_MAX_LENGTH (50) | ||
|
|
||
| typedef enum { | ||
| PIN_SUCCESS, | ||
| USER_PIN_ENTERED, | ||
| PASSPHRASE_PIN_ENTERED, | ||
| USER_PIN_NOT_ENTERED, | ||
| WIPE_CODE_ENTERED, | ||
| PIN_SAME_AS_USER_PIN, | ||
| PIN_SAME_AS_WIPE_CODE, | ||
| PIN_PASSPHRASE_MAX_ITEMS_REACHED, | ||
| PIN_PASSPHRASE_SAVE_FAILED, | ||
| PIN_PASSPHRASE_READ_FAILED, | ||
| PIN_FAILED | ||
| } pin_result_t; |
There was a problem hiding this comment.
Duplicate constant names
PIN_MAX_LENGTH here conflicts with PIN_MAX_LEN in the .c file.
core/embed/trezorhal/se_thd89.c
Outdated
| #define PIN_MAX_LEN (50) | ||
|
|
There was a problem hiding this comment.
Shadowed macro
Defines PIN_MAX_LEN while header uses PIN_MAX_LENGTH.
| /// def save_pin_passphrase(pin: str, passphrase: str) -> bool: | ||
| /// """ | ||
| /// Save the pin and passphrase to the list. | ||
| /// Returns True on success, False on failure. | ||
| /// """ | ||
| STATIC mp_obj_t mod_trezorcrypto_se_thd89_save_pin_passphrase( |
There was a problem hiding this comment.
Docstring inconsistent
Message says “Returns True on success, False on failure” but the function can raise ValueError.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/embed/trezorhal/se_thd89.c (2)
18-19: Shadowed macroDefines
PIN_MAX_LENwhile header usesPIN_MAX_LENGTH.
1317-1345:⚠️ Potential issueInconsistent constant usage
Using
PIN_MAX_LENGTHinstead ofPIN_MAX_LENhere.- if (strlen(pin) < 6 || strlen(pin) > PIN_MAX_LENGTH) { + if (strlen(pin) < 6 || strlen(pin) > PIN_MAX_LEN) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (2)
core/mocks/generated/trezorconfig.pyiis excluded by!**/generated/**core/mocks/generated/trezorcrypto/se_thd89.pyiis excluded by!**/generated/**
📒 Files selected for processing (4)
core/embed/extmod/modtrezorconfig/modtrezorconfig.c(2 hunks)core/embed/extmod/modtrezorcrypto/modtrezorcrypto-se-thd89.h(3 hunks)core/embed/trezorhal/se_thd89.c(9 hunks)core/embed/trezorhal/se_thd89.h(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- core/embed/extmod/modtrezorcrypto/modtrezorcrypto-se-thd89.h
- core/embed/trezorhal/se_thd89.h
- core/embed/extmod/modtrezorconfig/modtrezorconfig.c
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Defs check
- GitHub Check: Gen check
- GitHub Check: Style check
🔇 Additional comments (3)
core/embed/trezorhal/se_thd89.c (3)
255-258: Good defensive codingAvoids undefined behavior when no data is received.
1183-1186: Update pin_type correctlyProperly captures PIN verification result type.
1347-1348: Good accessor methodsClean implementation of getter functions.
| @@ -1147,12 +1160,18 @@ secbool se_setPin(const char *pin) { | |||
| static secbool se_verifyPin_ex(uint8_t addr, uint8_t *session_key, | |||
| const char *pin) { | |||
| uint8_t pin_buf[50 + 1] = {0}; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Buffer size concern
Fixed size pin_buf[50 + 1] but PIN_MAX_LEN is 50. Use PIN_MAX_LEN + 1.
- uint8_t pin_buf[50 + 1] = {0};
+ uint8_t pin_buf[PIN_MAX_LEN + 1] = {0};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint8_t pin_buf[50 + 1] = {0}; | |
| uint8_t pin_buf[PIN_MAX_LEN + 1] = {0}; |
1 participant
Summary by CodeRabbit
Bug Fixes
New Features
Style