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(core): increase maximum passphrase length to 128 bytes #4520

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions core/.changelog.d/4084.changed
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Increase maximum passphrase length to 128 bytes.
8 changes: 5 additions & 3 deletions core/embed/rust/src/ui/component/base.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use heapless::Vec;
use heapless::{String, Vec};

use crate::{
strutil::{ShortString, TString},
strutil::TString,
time::Duration,
ui::{
button_request::{ButtonRequest, ButtonRequestCode},
Expand Down Expand Up @@ -592,6 +592,8 @@ impl EventCtx {
}
}

pub type FlowMsgText = String<128>;

/// Component::Msg for component parts of a swipe flow. Converting results of
/// different screens to a shared type makes things easier to work with.
///
Expand All @@ -603,7 +605,7 @@ pub enum FlowMsg {
Cancelled,
Info,
Choice(usize),
Text(ShortString),
Text(FlowMsgText),
}

#[cfg(feature = "micropython")]
Expand Down
10 changes: 5 additions & 5 deletions core/embed/rust/src/ui/component/text/common.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::{
strutil::ShortString,
ui::{component::EventCtx, util::ResultExt},
use crate::ui::{
component::{base::FlowMsgText, EventCtx},
util::ResultExt,
};

/// Reified editing operations of `TextBox`.
Expand All @@ -16,13 +16,13 @@ pub enum TextEdit {
/// operations over it. Text ops usually take a `EventCtx` to request a paint
/// pass in case of any state modification.
pub struct TextBox {
text: ShortString,
text: FlowMsgText,
}

impl TextBox {
/// Create a new `TextBox` with content `text`.
pub fn new(text: &str, max_len: usize) -> Self {
let text = unwrap!(ShortString::try_from(text));
let text = unwrap!(FlowMsgText::try_from(text));
debug_assert!(text.capacity() >= max_len);
Self { text }
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ const KEYBOARD: [[&str; KEY_COUNT]; PAGE_COUNT] = [
["_<>", ".:@", "/|\\", "!()", "+%&", "-[]", "?{}", ",'`", ";\"~", "$^="],
];

const MAX_LENGTH: usize = 50;
const MAX_LENGTH: usize = 128;
const INPUT_AREA_HEIGHT: i16 = ScrollBar::DOT_SIZE + 9;

impl PassphraseKeyboard {
Expand Down
19 changes: 8 additions & 11 deletions core/embed/rust/src/ui/layout_caesar/component/changing_text.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
use crate::{
strutil::ShortString,
ui::{
component::{Component, Event, EventCtx, Never, Pad},
display::Font,
geometry::{Alignment, Point, Rect},
shape::{self, Renderer},
util::long_line_content_with_ellipsis,
},
use crate::ui::{
component::{base::FlowMsgText, Component, Event, EventCtx, Never, Pad},
display::Font,
geometry::{Alignment, Point, Rect},
shape::{self, Renderer},
util::long_line_content_with_ellipsis,
};

use super::theme;
Expand All @@ -16,7 +13,7 @@ use super::theme;
/// and without being affected by other components.
pub struct ChangingTextLine {
pad: Pad,
text: ShortString,
text: FlowMsgText,
font: Font,
/// Whether to show the text. Can be disabled.
show_content: bool,
Expand All @@ -29,7 +26,7 @@ pub struct ChangingTextLine {

impl ChangingTextLine {
pub fn new(text: &str, font: Font, alignment: Alignment, max_len: usize) -> Self {
let text = unwrap!(ShortString::try_from(text));
let text = unwrap!(FlowMsgText::try_from(text));
debug_assert!(text.capacity() >= max_len);
Self {
pad: Pad::with_background(theme::BG),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
use crate::{
strutil::{ShortString, TString},
strutil::TString,
translations::TR,
trezorhal::random,
ui::{
component::{text::common::TextBox, Child, Component, ComponentExt, Event, EventCtx},
component::{
base::FlowMsgText, text::common::TextBox, Child, Component, ComponentExt, Event,
EventCtx,
},
display::Icon,
geometry::Rect,
shape::Renderer,
Expand All @@ -26,7 +29,7 @@ enum ChoiceCategory {
SpecialSymbol,
}

const MAX_PASSPHRASE_LENGTH: usize = 50;
const MAX_PASSPHRASE_LENGTH: usize = 128;

const DIGITS: &str = "0123456789";
const LOWERCASE_LETTERS: &str = "abcdefghijklmnopqrstuvwxyz";
Expand Down Expand Up @@ -291,17 +294,17 @@ impl PassphraseEntry {

fn update_passphrase_dots(&mut self, ctx: &mut EventCtx) {
debug_assert!({
let s = ShortString::new();
let s = FlowMsgText::new();
s.capacity() >= MAX_PASSPHRASE_LENGTH
});

let text_to_show = if self.show_plain_passphrase {
unwrap!(ShortString::try_from(self.passphrase()))
unwrap!(FlowMsgText::try_from(self.passphrase()))
} else if self.is_empty() {
unwrap!(ShortString::try_from(""))
unwrap!(FlowMsgText::try_from(""))
} else {
// Showing asterisks and possibly the last digit.
let mut dots = ShortString::new();
let mut dots = FlowMsgText::new();
for _ in 0..self.textbox.len() - 1 {
unwrap!(dots.push('*'));
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
use crate::{
strutil::{ShortString, TString},
strutil::TString,
translations::TR,
ui::{
component::{
base::ComponentExt, swipe_detect::SwipeConfig, text::common::TextBox, Component, Event,
EventCtx, Label, Maybe, Never, Swipe,
base::{ComponentExt, FlowMsgText},
swipe_detect::SwipeConfig,
text::common::TextBox,
Component, Event, EventCtx, Label, Maybe, Never, Swipe,
},
display,
geometry::{Alignment, Direction, Grid, Insets, Offset, Rect},
Expand All @@ -27,7 +29,7 @@ use core::cell::Cell;
use num_traits::ToPrimitive;

pub enum PassphraseKeyboardMsg {
Confirmed(ShortString),
Confirmed(FlowMsgText),
Cancelled,
}

Expand Down Expand Up @@ -101,7 +103,7 @@ const KEYBOARD: [[&str; KEY_COUNT]; PAGE_COUNT] = [
["_<>", ".:@", "/|\\", "!()", "+%&", "-[]", "?{}", ",'`", ";\"~", "$^="],
];

const MAX_LENGTH: usize = 50;
const MAX_LENGTH: usize = 128;

const CONFIRM_BTN_INSETS: Insets = Insets::new(5, 0, 5, 0);
const CONFIRM_EMPTY_BTN_MARGIN_RIGHT: i16 = 7;
Expand Down Expand Up @@ -338,12 +340,12 @@ impl Component for PassphraseKeyboard {
// Confirm button was clicked, we're done.
if let Some(ButtonMsg::Clicked) = self.confirm_empty_btn.event(ctx, event) {
return Some(PassphraseKeyboardMsg::Confirmed(unwrap!(
ShortString::try_from(self.passphrase())
FlowMsgText::try_from(self.passphrase())
)));
}
if let Some(ButtonMsg::Clicked) = self.confirm_btn.event(ctx, event) {
return Some(PassphraseKeyboardMsg::Confirmed(unwrap!(
ShortString::try_from(self.passphrase())
FlowMsgText::try_from(self.passphrase())
)));
}
if let Some(ButtonMsg::Clicked) = self.cancel_btn.event(ctx, event) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
use crate::{
error,
strutil::ShortString,
translations::TR,
ui::{
component::ComponentExt,
component::{base::FlowMsgText, ComponentExt},
flow::{
base::{Decision, DecisionBuilder as _},
FlowController, FlowMsg, SwipeFlow,
Expand Down Expand Up @@ -44,7 +43,7 @@ impl FlowController for RequestPassphrase {
(Self::Keypad, FlowMsg::Cancelled) => self.return_msg(FlowMsg::Cancelled),
(Self::ConfirmEmpty, FlowMsg::Cancelled) => Self::Keypad.goto(),
(Self::ConfirmEmpty, FlowMsg::Confirmed) => {
self.return_msg(FlowMsg::Text(ShortString::new()))
self.return_msg(FlowMsg::Text(FlowMsgText::new()))
}
_ => self.do_nothing(),
}
Expand Down
2 changes: 1 addition & 1 deletion core/src/apps/common/passphrase.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import storage.device as storage_device
from trezor.wire import DataError

_MAX_PASSPHRASE_LEN = const(50)
_MAX_PASSPHRASE_LEN = const(128)


def is_enabled() -> bool:
Expand Down
2 changes: 1 addition & 1 deletion python/src/trezorlib/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@

LOG = logging.getLogger(__name__)

MAX_PASSPHRASE_LENGTH = 50
MAX_PASSPHRASE_LENGTH = 128
MAX_PIN_LENGTH = 50

PASSPHRASE_ON_DEVICE = object()
Expand Down
27 changes: 27 additions & 0 deletions tests/click_tests/common.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

import itertools
import typing as t
from enum import Enum

Expand Down Expand Up @@ -27,6 +28,32 @@ class CommonPass:

EMPTY_ADDRESS = "mvbu1Gdy8SUjTenqerxUaZyYjmveZvt33q"

LONGEST = "".join(itertools.islice(itertools.cycle("1234567890"), 128))
LONGEST_ADDRESS = "msHs5d865WetSd7Uai8roAu2eCaZgx3oqg"

ALMOST_LONGEST = LONGEST[:-1]
ALMOST_LONGEST_ADDRESS = "mkRvtfidQoPhNQx1eesDNMXCRiZeUK5kLB"

TOO_LONG = LONGEST + "x"
TOO_LONG_ADDRESS = LONGEST_ADDRESS

VECTORS = ( # passphrase, address
(SHORT, SHORT_ADDRESS),
(WITH_SPACE, WITH_SPACE_ADDRESS),
(RANDOM_25, RANDOM_25_ADDRESS),
(ALMOST_LONGEST, ALMOST_LONGEST_ADDRESS),
(LONGEST, LONGEST_ADDRESS),
)


assert len(CommonPass.LONGEST) == 128
assert len(CommonPass.ALMOST_LONGEST) == 127
assert len(CommonPass.TOO_LONG) == 129
assert CommonPass.LONGEST_ADDRESS != CommonPass.ALMOST_LONGEST_ADDRESS
assert CommonPass.LONGEST_ADDRESS == CommonPass.TOO_LONG_ADDRESS
# TODO: show some UI message when length reaches the limit?
# (it currently disabled typing and greys out the buttons)


class PassphraseCategory(Enum):
MENU = "MENU"
Expand Down
42 changes: 8 additions & 34 deletions tests/click_tests/test_passphrase_bolt.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,24 +45,6 @@
TT_CATEGORY = PassphraseCategory.LOWERCASE
TT_COORDS_PREV: buttons.Coords = (0, 0)

# Testing the maximum length is really 50
# TODO: show some UI message when length reaches 50?
# (it currently disabled typing and greys out the buttons)

DA_50 = 25 * "da"
DA_50_ADDRESS = "mg5L2i8HZKUvceK1sfmGHhE4gichFSsdvm"
assert len(DA_50) == 50

DA_49 = DA_50[:-1]
DA_49_ADDRESS = "mxrB75ydMS3ZzqmYKK28fj4bNMEx7dDw6e"
assert len(DA_49) == 49
assert DA_49_ADDRESS != DA_50_ADDRESS

DA_51 = DA_50 + "d"
DA_51_ADDRESS = DA_50_ADDRESS
assert len(DA_51) == 51
assert DA_51_ADDRESS == DA_50_ADDRESS


@contextmanager
def prepare_passphrase_dialogue(
Expand Down Expand Up @@ -151,16 +133,7 @@ def delete_char(debug: "DebugLink") -> None:
debug.click(coords)


VECTORS = ( # passphrase, address
(CommonPass.SHORT, CommonPass.SHORT_ADDRESS),
(CommonPass.WITH_SPACE, CommonPass.WITH_SPACE_ADDRESS),
(CommonPass.RANDOM_25, CommonPass.RANDOM_25_ADDRESS),
(DA_49, DA_49_ADDRESS),
(DA_50, DA_50_ADDRESS),
)


@pytest.mark.parametrize("passphrase, address", VECTORS)
@pytest.mark.parametrize("passphrase, address", CommonPass.VECTORS)
@pytest.mark.setup_client(passphrase=True)
def test_passphrase_input(
device_handler: "BackgroundDeviceHandler", passphrase: str, address: str
Expand All @@ -171,10 +144,10 @@ def test_passphrase_input(


@pytest.mark.setup_client(passphrase=True)
def test_passphrase_input_over_50_chars(device_handler: "BackgroundDeviceHandler"):
with prepare_passphrase_dialogue(device_handler, DA_51_ADDRESS) as debug: # type: ignore
input_passphrase(debug, DA_51, check=False)
assert debug.read_layout().passphrase() == DA_50
def test_passphrase_input_too_long(device_handler: "BackgroundDeviceHandler"):
with prepare_passphrase_dialogue(device_handler, CommonPass.TOO_LONG_ADDRESS) as debug: # type: ignore
input_passphrase(debug, CommonPass.TOO_LONG, check=False)
assert debug.read_layout().passphrase() == CommonPass.LONGEST
enter_passphrase(debug)


Expand Down Expand Up @@ -290,9 +263,10 @@ def test_passphrase_dollar_sign_deletion(
def test_cycle_through_last_character(
device_handler: "BackgroundDeviceHandler",
):
# Checks that we can cycle through the last (50th) passphrase character
# Checks that we can cycle through the last (128th) passphrase character
# (was a bug previously)
with prepare_passphrase_dialogue(device_handler) as debug:
passphrase = DA_49 + "i" # for i we need to cycle through "ghi" three times
# for "i" we need to cycle through "ghi" three times
passphrase = CommonPass.ALMOST_LONGEST + "i"
input_passphrase(debug, passphrase)
enter_passphrase(debug)
Loading
Loading