fix: resolve all findings from full-codebase review (security, correctness, perf, binary size)#675
Open
replydev wants to merge 55 commits into
Open
fix: resolve all findings from full-codebase review (security, correctness, perf, binary size)#675replydev wants to merge 55 commits into
replydev wants to merge 55 commits into
Conversation
The FreeOTP+ importer decided whether to keep the counter by testing the hash algorithm field (token.algo, e.g. "SHA1") against "HOTP" instead of the token type field (token.type). The comparison was never true, so every imported HOTP token ended up with counter: None and failed code generation with "Missing counter value". Trigger: import any FreeOTP+ backup containing an HOTP token. Fix: compare token.type against "HOTP" when extracting the counter. Adds a fixture with an HOTP entry (counter 4) and a test asserting the counter survives the conversion.
…panicking on I/O errors All export formats (full database JSON, andOTP, FreeOTP+, otpauth URIs) are written unencrypted. The file was previously created with File::create, which honors the default umask and typically yields world-readable 0644 permissions, letting any local user read every exported OTP secret. The file is now opened via OpenOptions with mode(0o600) on Unix (write + create + truncate), so only the owner can read it; on non-Unix platforms the behavior is unchanged. A warning is also printed to stderr reminding the user that the exported file contains secrets in plain text. Additionally: - The two .expect() calls on file creation and writing panicked on any I/O failure (e.g. 'cotp export -p /nonexistent/dir/x.json') even though the function already returns Result; errors are now propagated as a readable message including the target path. - The serialized plaintext String was only zeroized on the success path; it is now zeroized on every path (empty export, write failure, success) before returning. Unit tests cover the error propagation, the empty-export short-circuit, and (on Unix) the 0600 permission bits of the created file.
The main window key handler matched KeyCode::Char('d' | 'D' | 'c') in a
single arm and only distinguished the intended Ctrl modifier inside it,
so pressing bare 'c' (a natural guess for "copy") fell into the else
branch and opened the delete-OTP confirmation popup.
Split the arm: 'c'/'C' now only acts with KeyModifiers::CONTROL (the
intended Ctrl-C exit path, matching the search-bar handler), while
'd'/'D' keeps its dual role of Ctrl-D exit and plain-d delete prompt.
Bare 'c' now does nothing.
…ted database decrypt_string() used .expect()/.unwrap() when Base64-decoding the nonce, cipher text and salt fields of the on-disk encrypted database envelope. A hand-edited or bit-rotted db.cotp therefore crashed cotp at startup with a panic and backtrace instead of a useful diagnostic, even though the function already returns a color_eyre Result. The three decode calls are now map_err'd into the Result with messages of the form 'database file is corrupted: cannot decode Base64 <field>: <cause>', so the user gets an actionable error message. Unit tests feed envelopes with invalid Base64 in each of the three fields and assert that decrypt_string returns Err (mentioning the corrupted field) rather than panicking.
handle_counter_switch called element.counter.unwrap() with a comment claiming the unwrap was safe because the element type is HOTP. That assumption is false: HOTP elements can exist with counter == None, for example when imported from an otpauth:// URI that omits the counter parameter. Pressing '+' or '-' on such an element crashed the TUI while the terminal was in raw mode. Treat a missing counter as 0 via unwrap_or(0), so '+' initializes it to 1 and '-' saturates at 0; the code is regenerated afterwards as before via mark_modified() and tick(true).
…e digest slice Yandex code generation dispatched on the element's algorithm field, falling back to HMAC-SHA1 for anything that was not SHA256/SHA512. Since SHA1 is the default algorithm when adding an element, the default configuration used HMAC-SHA1, whose output is only 20 bytes. The dynamic offset (last byte & 0xf) ranges over 0..=15, so for offsets 13..=15 the slice period_hash[offset..offset + 8] read out of bounds and panicked. That is roughly 3 out of 16 time windows (~19%), and inside the TUI the panic fired mid raw-mode, corrupting the terminal. Yandex.Key and Aegis' reference implementation (which this port is based on) exclusively use HMAC-SHA256, so honoring the stored algorithm field was wrong as well as unsound: codes produced with SHA1/SHA512 never matched Yandex's servers. Fix: calculate_yandex_code is no longer generic over the hash and always uses HMAC-SHA256 (32-byte output, offset+8 <= 23 always in bounds). As defense in depth, the digest indexing now goes through get()/get_mut() and returns OtpError instead of panicking. The existing test vector already exercised SHA256 and still passes.
AegisInfo had no pin field and the AegisElement -> OTPElement conversion hardcoded pin: None, so Yandex/MOTP entries imported from Aegis backups lost their pin and could never generate codes. Trigger: import a plain Aegis backup containing a Yandex or MOTP entry whose info object carries a pin. Fix: add pin: Option<String> (serde default) to AegisInfo and map it through to OTPElement. Adds a unit test covering entries with and without a pin. Also removes the dead AegisHeader empty struct (kept alive only by an allow(dead_code)) and the unused Serialize derives on these import-only types.
handlers/popup.rs matched PopupAction::EditOtp with todo!(), and App::new initialized popup.action to exactly that variant. Nothing in the codebase ever implemented editing via popup, so the variant was a landmine: any future code path that focuses the popup without going through show_popup() would hit the todo!() and panic while the terminal is in raw mode. Delete the EditOtp variant entirely and derive Default on PopupAction with GeneralInfo as the default, which is harmless if it is ever reached: its handler just returns focus to the main page on i/Esc/Enter. All real popups keep setting their action explicitly via show_popup().
After dashboard() returns, the mpsc receiver inside EventHandler is dropped, but the polling thread keeps running. Its next sender.send(...).expect(...) then panicked with "failed to send terminal event" / "failed to send tick event", race-dependently spraying a panic message onto the just-restored terminal. Treat a send error (receiver gone) as the shutdown signal and break out of the loop instead of panicking, for both terminal events and tick events. Note: the Event enum still carries dead unit payloads (Mouse(()), Resize((), ()), FocusGained(), Paste(())); they cannot be collapsed without editing the exact-shape match in main.rs, which is out of scope for this change.
… relative path
init_app decided whether this was a first run by checking the existence of
the database file's parent directory. For a bare relative filename (e.g.
`cotp -d db.cotp`), Path::parent() returns the empty path and
Path::new("").exists() is always false, so an existing, populated database
was treated as a first run: cotp prompted for a new password and overwrote
the file with an empty database, destroying all stored secrets.
The first-run decision is now taken from the database file itself
(db_path.exists()); the parent directory is only created when the file does
not exist yet, treating an empty parent as the current directory. When
directory creation fails, the underlying io::Error is now printed to stderr
instead of being silently discarded (the Result<bool, ()> signature is kept
because the caller in src/main.rs matches on Err(()) and is out of scope
for this change).
Adds an integration test that runs the binary twice from a temporary
working directory with a bare relative --database-path and asserts the
database and its content survive the second run.
KEY_DERIVATION_CONFIG declared lanes: 4 but ThreadMode::Sequential, so every database unlock computed the four 32 MiB lanes one after another on a single core. Switching to ThreadMode::Parallel lets rust-argon2 compute the lanes on separate threads, giving an expected 2-4x speedup of key derivation (and thus of every cotp startup) on multi-core machines. This is safe for existing databases: in Argon2 the lane count is part of the hash parameters, while the thread mode is purely an execution strategy of the implementation, so the derived key is bit-for-bit identical. This is verified by a new unit test whose expected key for a fixed password+salt was captured from the previous Sequential implementation before the change; the test still passes with Parallel. Deliberately not migrating to the RustCrypto argon2 crate here: it is single-threaded, which would forfeit exactly this multi-lane parallelism win.
The main page layout computed Constraint::Length(height - 8) from the raw frame height; on a terminal shorter than 8 rows the u16 subtraction underflowed and panicked in debug builds while the terminal was in raw mode. Use height.saturating_sub(8) instead, and harden the same subtraction pattern in centered_rect() (100 - percent_x/percent_y) with saturating_sub as defense in depth against out-of-range percentages.
A period of 0 was accepted everywhere an element can enter the database (add/edit flags, otpauth URIs with period=0, imports), but every time-based code generator divides the current Unix time by the period: totp_maker, yandex_otp_maker and motp_maker all computed seconds/period and panicked with a division by zero. In the TUI the panic fired on the very first render tick, crashing the dashboard while the terminal was in raw mode. Fix at both levels: - OTPElementBuilder::validate now rejects period == 0 (and digits == 0), so new elements built through the builder can never carry the invalid value. - The generators are hardened for elements that already exist in a database or bypass the builder: totp, yandex and motp return the new OtpError::InvalidPeriod instead of dividing by zero. motp's signature changes from String to Result<String, OtpError>; its only caller is OTPElement::get_otp_code which already returns that Result type.
…cking The encrypted Aegis importer used unwrap()/expect() on untrusted backup contents: slot.n/p/r and slot.salt could be absent on a type-1 slot, and the nonce, tag, key and salt hex strings could be non-hex. Any such malformed (or maliciously crafted) file crashed cotp with a panic instead of reporting an invalid backup. In addition, the scrypt cost parameter was converted with (n as f32).log2() as u8, which silently truncates a non-power-of-two n, derives a key with the wrong cost and surfaces as a misleading "wrong password" decryption failure. Trigger: run cotp import --aegis-encrypted with a backup whose slot is missing n/r/p/salt or contains non-hex nonce/tag/key/salt fields. Fix: convert every unwrap/expect on backup data into a descriptive Err through the existing String error path, validate n.is_power_of_two() and derive log2(n) with n.trailing_zeros(). Adds unit tests feeding malformed slot JSON and asserting an Err is returned instead of a panic.
The SSH clipboard path wrote the OSC52 escape sequence (with the base64-encoded secret) to io::stdout(). The TUI dashboard deliberately runs on io::stderr() so that stdout stays clean for piping; writing the escape to stdout polluted piped output with the raw escape bytes and broke that contract. Write the sequence to io::stderr() instead - the terminal the TUI owns - which is where terminals interpret the OSC52 sequence anyway when the alternate screen is active.
… parallel Argon2)
…tick render_qrcode_page called element.get_qrcode() on every render, i.e. on every 250ms tick and every key event while the QR page was open. Each call rebuilds the otpauth URI, recomputes the QR matrix and re-renders it to a unicode string, four times per second for a static image. Cache the rendered string in App keyed by the selected element index and reuse it while the same element stays selected. The cache is invalidated when the selection changes (different index), when the table is refreshed (App::tick force/period refresh, which covers HOTP counter changes and deletions) and when leaving the page via App::reset(). get_qrcode() itself is unchanged.
…actually fire
All conditional rules on the add subcommand were dead:
- required_if_eq("otp_type", "HOTP") (and YANDEX/MOTP) compared against
uppercase values, but ValueEnum possible values are lowercase ("hotp",
"yandex", "motp"), so the predicates never matched.
- default_value_if("type", "STEAM", "5") referenced a nonexistent arg id
(the field id is otp_type; "type" is only the long flag name) on top of
the same case mismatch.
Consequences: `add -t hotp` succeeded without --counter (the element then
renders ERROR and the TUI +/- counter keys panic), `add -t steam` produced
6-digit codes while Steam codes are 5 digits (every generated code
invalid), and yandex/motp entries added without --pin were permanently
broken.
Fix the arg ids and value casing so hotp requires --counter, yandex and
motp require --pin, and steam defaults digits to 5. Additionally add a
post-parse backstop (validate_type_invariants) in the execution path so
the invariants still hold even if the declarative clap rules silently rot
again, plus unit tests covering both the clap rules and the backstop.
from_otp_uri ran urlencoding::decode over the entire URI and only then handed it to Url::parse. Any percent-encoded structural character in a label or query value therefore corrupted parsing: - otpauth://totp/C%23Corp?secret=... failed entirely, because "%23" became "#" and the whole query turned into a URL fragment - "%3F" in a label became "?" and truncated the label / shifted the query, "%26" in a value became "&" and split the query pair - every value was effectively decoded twice (the pre-decode plus Url::query_pairs, which decodes again), so labels containing a literal "%25" decoded to "%" instead of "%25" Fix: parse the RAW URI with Url::parse and percent-decode only the individual components. Query values are decoded exactly once by Url::query_pairs; the first path segment is decoded once before being split on ':' into issuer and label (decoding before splitting preserves the GH-548 behavior of treating an encoded %3A as the separator). Also renames the meaningless helper fn get() to issuer_label_segments() and adds regression tests for labels containing %23, %3F, %26, %25 and a double-encoded literal %25 (%2525).
The TryFrom<AegisEncryptedDatabase> for Vec<OTPElement> impl called the blocking terminal prompt utils::password() inside a pure conversion trait. That coupled decryption to an interactive terminal, making the whole encrypted-Aegis import path impossible to unit test and inconsistent with the rest of the codebase, where interactive decisions live in the argument layer. Fix: replace the TryFrom impl with an explicit AegisEncryptedDatabase::decrypt(self, password) -> Result<Vec<OTPElement>, String> method, and move the prompting into a dedicated import_aegis_encrypted() branch in arguments/import.rs, which reads the file, deserializes it, asks for the password (zeroizing it after use, as before) and delegates to decrypt(). Adds a unit test invoking decrypt() on a backup with no usable key slot and asserting a graceful error. No happy-path decryption test is added because no encrypted Aegis fixture exists in test_samples/.
extract compared the user-supplied --index directly against the 0-based enumerate() position, while list, edit, delete and the TUI all present and accept 1-based indexes. As a result `extract --index 1` silently returned the SECOND element's OTP code — off by one from what the user sees in `list` — and `extract --index 0` returned the first element instead of being rejected. The index filter now subtracts 1 before comparing against the array position (i.checked_sub(1) == Some(index)), and --index 0 is rejected with an explicit error explaining that indexes are 1-based. Unit tests cover 1-based selection, out-of-range indexes and the index-0 rejection.
After decrypting an encrypted Aegis backup, map_results() converted the plaintext into a String containing every OTP secret in the backup and dropped it without zeroization, leaving the secrets recoverable from freed memory. The raw bytes kept inside the FromUtf8Error on the invalid-utf8 path leaked the same way. Trigger: any successful (or utf8-invalid) cotp import --aegis-encrypted run. Fix: zeroize the plaintext JSON String once parsing is done, and zeroize the error's byte buffer on the invalid-utf8 path, consistent with the zeroization already applied to the password and master key.
The dashboard hardcoded a 30 second cycle everywhere: App::tick only regenerated codes when the global 30s progress (utils::percentage) wrapped, and the gauge always showed that same 30s cycle. Elements with a different period - 60s TOTP, 10s MOTP default - kept showing expired codes for up to 30 seconds and a gauge unrelated to their actual validity window. Track each element's own RFC 6238 time step (unix_seconds / period) across ticks and regenerate the table as soon as any element crosses its own period boundary. The progress gauge now shows the elapsed fraction of the SELECTED element's period, computed at render time so it also reacts immediately to selection changes; when nothing is selected it falls back to the global 30s utils::percentage cycle. The period-aware helpers live in src/interface/app.rs (current_step, period_percentage, element_steps); src/utils.rs is intentionally left untouched. The 250ms tick rate is unchanged, and a period of 0 is clamped to 1 to avoid division by zero.
search_and_select was four near-identical scan loops over the table rows - issuer prefix, label prefix, issuer substring, label substring - each re-lowercasing the search query and the inspected cell on every iteration, and each reaching into the row through positional values.get(1)/get(2).unwrap() magic indices. Replace it with a single ranked pass: the query is lowercased once, each row is assigned the rank of the best predicate it satisfies (issuer prefix < label prefix < issuer substring < label substring) and the minimum (rank, index) wins, preserving the exact selection semantics of the four sequential loops including tie-breaking by row order. To remove the magic column indices, Row now has named fields (id, issuer, label, otp_code) instead of values: Vec<String>; height() and cells() iterate an ordered columns() array. This also lets copy_selected_code_to_clipboard read row.otp_code directly, dropping the unreachable "Cannot get OTP Code column" branch. All Row users live in src/interface/, so the change is fully contained.
Three related defects made exported MOTP and Yandex entries unusable after re-import: - from_otp_uri force-uppercased every secret. MOTP secrets are lowercase hex strings fed as TEXT into MD5 (see motp_maker), so an imported MOTP entry silently generated wrong codes. - get_otpauth_uri never emitted the pin parameter, while Yandex and MOTP codes cannot be generated without it: exporting and re-importing such an entry produced an element that could only ever return "Missing pin value". - from_otp_uri constructed OTPElement literally, bypassing OTPElementBuilder validation, so invalid (e.g. non-BASE32) secrets were accepted at import time and only failed later, at code generation. Fix: from_otp_uri now builds the element through OTPElementBuilder, which validates the secret encoding, period and digits, and normalizes secret case per type (uppercase for the base32 types TOTP/HOTP/Steam/ Yandex, lowercase hex for MOTP). get_otpauth_uri emits a pin=<value> query parameter when the element has a pin, and from_otp_uri parses the same "pin" parameter back. Adds round-trip tests (element -> get_otpauth_uri -> from_otp_uri) for TOTP, MOTP with a lowercase hex secret, and Yandex with a pin, plus a test that invalid BASE32 secrets are rejected at URI import.
The extract subcommand used GlobBuilder/GlobMatcher solely for case-insensitive wildcard matching of the --issuer/--label filters. That single use dragged globset and the full regex engine (regex-automata, regex-syntax, bstr) into the release binary, measured at about 512 KB of binary size for functionality that boils down to matching `*` and `?`. Replace it with a small iterative two-pointer wildcard matcher (wildcard_match) that supports `*` (any possibly-empty sequence) and `?` (exactly one character), performs whole-string matching, and is case-insensitive via Unicode to_lowercase - preserving the previous observable behavior of the extract filters. All pre-existing extract filter tests keep passing unchanged, and new unit tests cover the matcher itself (literal, empty pattern, `*` collapse, `?`, Unicode case folding). globset is removed from Cargo.toml; it only remains in Cargo.lock as a transitive dev-dependency of assert_fs, which does not affect the shipped binary.
OTPDatabase::overwrite_database_key serialized the whole database — every secret and pin included — into a plaintext JSON String that was simply dropped at the end of the function, leaving its contents in freed heap memory on every save. The read path (reading.rs) already zeroizes the decrypted plaintext, so the save path was the only place the full plaintext lingered. Fix: bind the serialized JSON as mutable and zeroize() it immediately after encrypt_string_with_key has consumed it, before the encryption result is unwrapped or anything is written to disk.
…ths, QR cache, search refactor)
Two issues in the delete subcommand:
1. `delete --index 0` silently targeted the FIRST element. The
index.and_then(|i| i.checked_sub(1)) chain mapped 0 to None, which fell
through to the issuer/label matcher with both filters defaulting to the
empty string - and `contains("")` matches every element, so element 0
was proposed for deletion even though the user never referred to it.
Out-of-range indexes fell through the same way. Now, when --index is
supplied, 0 and out-of-range values are rejected with explicit errors
and the issuer/label matcher is only consulted when no index was given.
2. Answering "N" to the confirmation prompt returned
Err("Operation interrupt by the user"), which main.rs surfaces as
"An error occurred: ..." with a failure exit code. Declining a
confirmation prompt is a normal outcome, not an error: it now prints a
neutral message and returns Ok with the unchanged database (which is
not marked modified, so nothing is re-encrypted or rewritten).
The database file was written via File::create, which creates new files with mode 0644 (subject to umask): any local user could read the encrypted database. The contents are encrypted with a key derived from the user's password, so this is defense in depth rather than a direct secret leak, but there is no reason for the file to be world-readable — it also exposes the encrypted blob to offline brute-forcing by other local users. Fix: on unix the file is now opened through OpenOptions with write/create/truncate and .mode(0o600) (std::os::unix::fs:: OpenOptionsExt), so newly created databases are only accessible by their owner. Other platforms keep the plain File::create behavior, which already truncates.
OTPElement derived Debug, so any debug formatting of an element — eyre error reports, dbg!/log statements, assertion failure output in tests — printed the raw secret and pin to the terminal or logs in plaintext. Fix: replace the derive with a hand-written Debug implementation that prints "***" for the secret and for the pin (when present) while keeping all non-sensitive fields. The type still implements Debug, so existing assert_eq! usage in tests keeps working.
OTPElement::get_qrcode unwrapped QrCode::new. A QR code has a maximum data capacity, so an element with an oversized label, issuer or secret made the otpauth URI too long to encode and the unwrap panicked. The method is called from the TUI QR-code view, so the panic fired while the terminal was in raw mode, crashing the dashboard and leaving the terminal corrupted. Fix: handle the QrCode::new error and return the printable string "Cannot render QR code: data too long" instead, which the TUI simply displays in place of the QR code. The String return type is kept so the caller in src/interface/ is untouched.
Three issues in the list subcommand:
1. The table view hand-rolled its padding with
" ".repeat(issuer_width - 6) where issuer_width is the longest issuer
length + 3. With only short issuers (2 characters or fewer, e.g. issuer
"ab"), the subtraction underflows usize and the command panics.
The manual string concatenation is replaced with {:<width$} format
specifiers, and both column widths are clamped to at least their header
lengths, so no subtraction can underflow and columns stay aligned.
2. `list --json` printed the JSON document with print!, leaving no
trailing newline (the next shell prompt glued to the output and piped
consumers received an unterminated last line). It now uses println!.
3. `list --json` was all-or-nothing: a single uncomputable element (e.g.
an HOTP entry missing its counter) failed the entire command, while the
table view degrades per-row with "ERROR". The JSON view now degrades
the same way, emitting the OTP error string as that element's otp_code
value instead of aborting the whole listing.
read_decrypted_text deleted the database file as a side effect of the read path whenever the file existed but was empty. A destructive, irreversible file removal buried inside a read function is surprising: an empty file can be the leftover of an interrupted or failed write, and the user may want to restore a backup over that exact path rather than have cotp silently discard it and start over. Treating the empty file as a first run here was not an option either: first-run detection (utils::init_app) happens before the password is read and uses a verified "choose a password" prompt, while this path has already consumed a plain "Password:" prompt (or stdin), so it cannot correctly initialize a new database. Fix: keep the file untouched and return a clear error stating the database path, that the file is empty or corrupted, and what to do (restore a backup over it, or remove it manually and restart cotp to initialize a new database). The now-unused delete_db helper is removed.
migrate() copied the const MIGRATIONS_LIST into a local binding and sorted it on every database save, allocating and sorting work for a list whose order is known at compile time. Fix: iterate the const directly by reference. The list is kept sorted by ascending to_version as a documented convention, enforced with a debug_assert!(is_sorted_by_key(...)) so any future out-of-order entry fails fast in debug/test builds without adding release overhead.
… no-op edits Two issues in the edit subcommand: 1. `edit --change-secret` stored the raw prompted string directly into the element, bypassing the base32/hex validation and case normalization that `add` performs through OTPElementBuilder. An invalid secret (e.g. not valid base32 for a TOTP entry) was silently persisted into the encrypted database and only surfaced later as a code-generation error. The new secret is now routed through OTPElementBuilder with the element's own type/fields, so it gets exactly the same validation and normalization as `add` and an invalid secret aborts the edit with a clear error before anything is saved. 2. run_command called mark_modified() unconditionally, so even a no-op `edit -i N` with no field arguments re-encrypted and rewrote the whole database file. The element is now compared against a snapshot taken before applying the changes and the database is only marked modified when at least one field actually changed value.
…round-trip, zeroization)
The add integration test mutates test_samples/cli_integration_test/ empty_database in place when run; a test run's side effect was accidentally swept into a previous commit. Restore the original fixture bytes. The underlying test is being fixed separately to copy the fixture to a temporary directory instead of writing to it.
…prompt errors Password prompt hygiene fixes: - password(): a typed attempt that was rejected for being shorter than the minimum length was dropped without being zeroized, leaving the secret in memory even though the crate uses zeroize for all other password/key handling. Rejected attempts are now zeroized before re-prompting. - verified_password(): the confirmation copy of the password was never zeroized (neither on match nor on mismatch), and a non-matching first attempt was dropped without zeroization too. Both are now zeroized on every path, including when the confirmation prompt itself fails. - rpassword::prompt_password(...).unwrap() panicked with a backtrace when stdin is not a TTY (e.g. cotp invoked from a script or with a closed stdin). The prompt logic now lives in Result-returning helpers (try_password / try_verified_password) that propagate the io::Error; the public String-returning signatures are kept as thin wrappers that print a clear message and exit, because their callers (src/main.rs, src/arguments/passwd.rs, src/reading.rs, src/importers/aegis_encrypted.rs) are outside the scope of this change. - The direct rpassword unwraps in the add (OTP URI prompt) and edit (--change-secret prompt) subcommands now propagate the error through their color_eyre::Result return values instead of panicking.
add_with_label_should_work ran the add subcommand directly against test_samples/cli_integration_test/empty_database and persisted the new element into it, dirtying the git working tree on every `cargo test` run (and making subsequent runs start from a different database state). The test now copies the fixture into an assert_fs::TempDir first and runs the binary against the temporary copy, so the committed fixture is never modified.
…ugs, list panic, globset removal)
cotp only ever prints errors with plain Display formatting ({e} in
main.rs), so color-eyre's extra machinery — panic/error report hooks,
backtrace capture and symbolization (gimli, addr2line, object) and the
tracing-error span-trace stack — was compiled in but never visible to
users. Depending directly on eyre keeps the exact same Result/Report
API (imports simply change from color_eyre::eyre::* to eyre::*) while
dropping the whole reporting stack, measured at -199 KB (-8.4%) on the
release binary.
Changes:
- Cargo.toml: color-eyre 0.6.5 -> eyre 0.6.12 (default features kept)
- src/**: color_eyre::eyre:: -> eyre::, color_eyre::Result -> eyre::Result
- src/main.rs: drop the color_eyre::install() hook, no longer needed
Error output remains readable: messages are unchanged, they just lose
the (never-requested) colorized backtrace support.
cotp already depends on data-encoding for base32/base64 in several modules; the hex and base64 crates only served a handful of remaining call sites. Routing those through data-encoding removes two direct dependencies and their duplicate encode/decode code paths. Replacements: - src/otp/otp_element.rs: hex::decode -> HEXLOWER_PERMISSIVE.decode (keeps accepting mixed-case hex secrets, as the hex crate did) - src/otp/algorithms/motp_maker.rs: hex::encode -> HEXLOWER.encode - src/importers/aegis_encrypted.rs: hex::FromHex -> a decode_hex helper over HEXLOWER_PERMISSIVE (Aegis writes lowercase, stays permissive) - src/clipboard.rs: base64 STANDARD engine -> BASE64.encode (same padded standard alphabet for the OSC52 escape sequence) - src/importers/google_authenticator.rs: base64 STANDARD engine -> BASE64.decode with a BASE64_NOPAD fallback. The old engine required canonical padding; the fallback additionally accepts migration URIs whose trailing '=' was stripped by a QR scanner (new test covers it). Semantic notes: - The invalid-hex error text changes from the hex crate's "Odd number of digits" to data-encoding's "invalid length at N" (test updated). - base64 remains in Cargo.lock as a transitive dependency of rust-argon2; hex is gone entirely.
Three independent dependency-weight cuts, none of which change any
user-visible behavior:
- ratatui: default features minus "all-widgets" and "macros". The TUI
only uses Block, Borders, Cell, Clear, Gauge, Paragraph, Row, Table
and Wrap - all available without extra features. In ratatui 0.30 the
default feature set itself enables all-widgets (= widget-calendar),
which drags the `time` crate into every build; cotp also never uses
the ratatui-macros crate. Kept: crossterm, layout-cache,
underline-color.
- qrcode: default-features = false. cotp only renders unicode QR
strings for `cotp list --qrcode`; the default "image" feature
compiles the whole `image` crate for PNG rendering that is never
called.
- idna_adapter = "~1.0" pinned as a direct dependency so `url` resolves
its IDNA backend to the small unicode-rs adapter instead of the
ICU4X normalizer/properties tables. cotp only parses otpauth:// URIs
and never needs internationalized domain names. Measured -131 KB on
the release binary.
cargo tree -e normal | grep -iE 'icu|image|time':
before: icu_collections, icu_locale_core, icu_normalizer(+data),
icu_properties(+data), icu_provider, image v0.25.9,
time v0.3.47, time-core v0.1.8
after: (no matches; remaining time/wait-timeout hits are
dev-dependencies of assert_cmd only)
Four related cleanups on the binary entry path: - Print all startup errors to stderr. Init/database errors went to stdout while subcommand errors went to stderr; stdout must stay clean for piping (the TUI already runs on stderr for the same reason). - Use conventional exit codes. exit(-1)/exit(-2) wrap to 255/254 on POSIX; main now returns std::process::ExitCode with 1 (init or save failure) and 2 (subcommand failure). The derived key is still zeroized on every path before returning. - Drop dead Event variants. Event::Mouse(()), Resize((), ()), FocusGained(), FocusLost() and Paste(()) carried erased unit payloads through the channel only for main.rs to ignore them; the event thread now drops those crossterm events at the source and the enum shrinks to Tick and Key. The thread still exits when a send fails (receiver dropped). - Propagate password-prompt failures. utils::password / verified_password printed and exited the process on prompt errors (e.g. stdin not a TTY); their callers (main.rs, arguments/passwd.rs, arguments/import.rs, reading.rs) now use the Result-returning try_password / try_verified_password and bubble the error up, and the exit-on-failure wrappers are removed. utils::init_app also returns a proper eyre error (including the directory and the underlying io::Error) instead of Err(()) that main.rs replaced with a generic message.
Both the import and export subcommands modeled their mutually exclusive
format flags as a struct of booleans and dispatched through an if/else
ladder. The import ladder ended in a reachable
Err("Invalid arguments provided") fallthrough, and the export ladder
ended in unreachable!() guarded only by an odd Default impl that set
cotp: true.
Convert the parsed flags into proper enums instead:
- import: BackupType::format() maps the flags to a new ImportFormat
enum via a flag table; run_command dispatches with a single
exhaustive match, including the special-cased password-prompting
import_aegis_encrypted() path. The Authy / Microsoft Authenticator /
FreeOTP flags share one arm since all three import the intermediate
ConvertedJsonList produced by the Python converters.
- export: ExportFormat::kind() maps the flags to a new ExportKind enum;
the strange Default for ExportFormat impl is deleted and the
"no flag means cotp format" default is expressed explicitly at the
call site with map_or.
The CLI surface (flag names, short options, ArgGroup semantics, help
output) is unchanged; --help output was verified byte-identical before
and after.
Also add an integration test asserting that 'cotp delete' without any
of --index/--issuer/--label is rejected by clap (via
required_unless_present_any) instead of falling through to an
empty-string issuer/label match that would target the first element.
…efaulting
OTPType and OTPAlgorithm implemented From<&str> mapping any
unrecognized string to Totp / Sha1. A backup entry with an unknown or
misspelled type (e.g. "OCRA") or algorithm silently became a
wrong-code TOTP/SHA1 entry on import instead of surfacing an error.
Replace both impls with TryFrom<&str> that still parses known names
case-insensitively but returns a descriptive error ("Unknown OTP
type/algorithm: ... (expected one of ...)") for anything else.
Propagate the fallible conversion through every caller:
- importers/aegis.rs, converted.rs, freeotp_plus.rs: the per-element
From impls become TryFrom, and the list-level TryFrom impls collect
Results so a single bad entry fails the import with the specific
reason (errors mapped to the modules' existing String error type).
- importers/authy_remote_debug.rs: From<AuthyExportedJsonElement> and
From<AuthyExportedList> become TryFrom for the same reason.
- otp/from_otp_uri.rs: unknown type/algorithm in an otpauth:// URI now
fails URI parsing via ? instead of being coerced to TOTP/SHA1.
- freeotp_plus.rs additionally derives the HOTP counter from the
parsed OTPType instead of re-comparing the uppercased raw string.
Add unit tests covering case-insensitive parsing of all known values,
the error for unknown values, and an importer-level test showing an
import file with an unknown type now produces a clear error.
Importer and exporter boundaries used ad-hoc String errors that were
then re-wrapped (and sometimes Debug-formatted, printing quoted
strings) before reaching the user:
- The TryInto<Vec<OTPElement>> impls in aegis.rs, converted.rs,
freeotp_plus.rs and authy_remote_debug.rs used Error = String; they
now use eyre::Report, so the OTPType/OTPAlgorithm parse errors from
the previous commit propagate without .to_string() round-trips.
- AegisEncryptedDatabase::decrypt and its helpers (calc_master_key,
get_params, map_results) returned Result<_, String> built from
format! with {e:?}; they now return eyre::Result with Display-
formatted ({e}) causes.
- do_export in exporters/mod.rs returned Result<PathBuf, String>; it
now returns eyre::Result<PathBuf>, and the serde failure message no
longer Debug-formats the error.
- import_from_path now bounds the conversion error with
Into<eyre::Report> instead of Debug, so import errors are no longer
stringified via {:?} (which wrapped String errors in quotes in
user-facing output). The serde deserialization error is also
Display-formatted now.
- arguments/import.rs drops the error-laundering
.map_err(|e| eyre!("{e}")) conversions, both on the dispatch result
and in import_aegis_encrypted.
- arguments/export.rs wraps the export error with WrapErr context
instead of flattening it into a new eyre! message, and main.rs
prints errors with the alternate {e:#} format so the whole context
chain ("outer context: root cause") stays visible to the user.
OtpError (src/otp/otp_error.rs) is intentionally left as-is: it is a
proper domain error type.
User-visible behavior is unchanged except for cleaner error messages
(no more Debug-quoted strings).
Make the needs_modification field private so the dirty flag can only be toggled through mark_modified()/the new clear_modified(), and route every direct field access from outside otp_element.rs through accessors: - interface/handlers/popup.rs: the "don't save before quit" path now calls clear_modified() instead of writing the field directly. - arguments/list.rs, arguments/extract.rs, exporters/freeotp_plus.rs, exporters/otp_uri.rs: use elements_ref() instead of touching the (now private) elements field. - exporters/andotp.rs: use the new into_elements() accessor for the by-value export and elements_ref() for the borrowed one (which now converts to &[OTPElement], adjusting arguments/export.rs). - main.rs: build the first-run database with OTPDatabase::default() instead of a struct literal over private fields. save() now clears the dirty flag only AFTER a successful encrypt+write, so a failed save leaves the database marked dirty, and documents the passwd-path invariant: passwd persists the database itself via save_with_pw (new salt + key), and the cleared flag is what makes the final is_modified() check in main() skip a second save that would re-encrypt with the old key and undo the password change. mut_element() keeps handing out &mut OTPElement without marking the database dirty, now with a doc comment making the contract explicit: callers mark modified themselves, which lets no-op edits (cotp edit with unchanged values) skip rewriting the database file.
OTPDatabase mixed domain data with encryption and filesystem writes: save()/save_with_pw() reached into crypto:: and wrote to the global DATABASE_PATH from inside the domain type. Introduce src/storage/mod.rs as the persistence layer and fold the small reading.rs module into it, so load and save live side by side: - get_elements_from_input/get_elements_from_stdin/read_from_file moved from reading.rs (deleted), keeping the ReadResult flow identical; read_decrypted_text and read_from_file now take the database path as a parameter instead of reading the global. - save(db, key, salt, path) and save_with_pw(db, password, path) moved from OTPDatabase; both take the path explicitly, and callers (main.rs, arguments/passwd.rs) pass DATABASE_PATH. migrate() still runs on every save, the modified flag is still cleared only after a successful write, and the passwd-path invariant doc moved along. - create_database_file() (0600 on unix) and the encrypt+write logic moved to storage; plaintext-JSON zeroization after encryption is preserved. The only non-mechanical change: the encryption result is now propagated with `?` instead of the previous unwrap(), so an encryption failure reports an error instead of panicking. OTPDatabase keeps only pure domain operations (elements accessors, add/delete/mut, dirty flag, sort) and loses its crypto/path/fs imports.
CLAUDE.md documents the split as "app.rs holds mutable App state; ui.rs renders", but the render functions (render_main_page, render_qrcode_page, render_table_box, render_alert and the top-level render dispatcher) lived on App while ui.rs only handled the terminal lifecycle. Move them to ui.rs as free functions taking &mut App, purely mechanically (self -> app). The QR-code render cache keeps its exact behavior: rendering still populates app.qrcode_cache keyed by the selected index, and tick()/reset() in app.rs keep invalidating it. App keeps only state and state-derived logic: title and qrcode_cache become pub(crate) so ui.rs can read/fill them, and progress() becomes pub(crate) but stays in app.rs since it is a state computation, not rendering.
The CI clippy gate (cargo clippy -- -D warnings) only lints the lib/bin target, so clippy::err_expect violations in #[cfg(test)] code went unnoticed. Replace the three result.err().expect(...) patterns in src/crypto/cryptography.rs tests with result.expect_err(...), making cargo clippy --all-targets -- -D warnings pass cleanly too. No other --all-targets lints surfaced.
Update the guidance to match what this branch changed: - Runtime data flow: ReadResult and load/save now live in the new storage/ module; document the dirty-flag contract (private flag, mark_modified/clear_modified, mut_element not marking, flag cleared only after a successful write and the passwd invariant) and the exit codes (1 init/save errors, 2 subcommand errors). - Key modules: add the storage/ persistence-layer bullet; OTPDatabase described as pure domain data; ui.rs now really owns all rendering (free functions over &mut App) plus the terminal lifecycle; import/ export format dispatch through the exhaustive ImportFormat/ ExportKind enums; extract's hand-rolled wildcard matcher; Yandex always HMAC-SHA256; OTPType/OTPAlgorithm TryFrom<&str> rejecting unknown strings. - Common commands: note cargo clippy --all-targets is kept clean too. - New short "Dependency notes" section: plain eyre (no color-eyre), data-encoding for all codecs (no hex/base64 crates), trimmed ratatui/qrcode features and the idna_adapter pin. - Database format: read_from_file reference now points at storage::.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| /// derive the exact same key and keep existing databases decryptable. | ||
| #[test] | ||
| fn test_derived_key_unchanged_by_thread_mode() { | ||
| let key = argon_derive_key(b"pa$$w0rd", b"0123456789abcdef").unwrap(); |
| fn test_decrypt_invalid_base64_nonce_returns_error() { | ||
| let corrupted = | ||
| r#"{"version":1,"nonce":"!!!not-base64!!!","salt":"c2FsdA==","cipher":"Y2lwaGVy"}"#; | ||
| let result = decrypt_string(corrupted, "pa$$w0rd"); |
| fn test_decrypt_invalid_base64_cipher_returns_error() { | ||
| let corrupted = | ||
| r#"{"version":1,"nonce":"bm9uY2U=","salt":"c2FsdA==","cipher":"!!!not-base64!!!"}"#; | ||
| let result = decrypt_string(corrupted, "pa$$w0rd"); |
| fn test_decrypt_invalid_base64_salt_returns_error() { | ||
| let corrupted = | ||
| r#"{"version":1,"nonce":"bm9uY2U=","salt":"!!!not-base64!!!","cipher":"Y2lwaGVy"}"#; | ||
| let result = decrypt_string(corrupted, "pa$$w0rd"); |
| ) | ||
| .expect("Invalid test database JSON"); | ||
|
|
||
| let result = database.decrypt("password"); |
| }"#, | ||
| ); | ||
|
|
||
| let result = calc_master_key(&slot, "password"); |
| }"#, | ||
| ); | ||
|
|
||
| let result = calc_master_key(&slot, "password"); |
| }"#, | ||
| ); | ||
|
|
||
| let result = calc_master_key(&slot, "password"); |
| // whose parent is the empty path and never "exists", which previously caused an | ||
| // existing database to be re-initialized and overwritten. | ||
| if db_path.exists() { | ||
| return Ok(false); |
| )); | ||
| } | ||
| Ok(!db_path.exists()) | ||
| Ok(true) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes every finding from a full-codebase review (security, correctness, performance, binary size, architecture). Each fix is an individual commit with a detailed body; merges group related workstreams. Every reported bug was reproduced against the built binary before fixing, and regression tests were added where practical.
Critical & high-severity bug fixes
cotp -d db.cotp <cmd>with a bare relative path treated an existing database as first run and silently overwrote it with a new empty one (init_appcheckedparent().exists(), which isfalsefor""). Now checks the file itself; regression test added.addvalidation was entirely dead: the conditional clap rules referenced a wrong arg id ("type") and wrong-case values ("HOTP"vshotp). Consequences fixed: HOTP could be added without--counter(then panicked the TUI on+/-), Steam produced 6-character codes instead of 5 (wrong for login), Yandex/MOTP could be added without--pin(permanentlyERROR).extract --indexwas 0-based whilelist/edit/delete/TUI are 1-based — it returned the next account's code. Now 1-based;--index 0rejected. Same fordelete --index 0, which previously fell through to an empty-filter match and targeted the first element.list --jsonfail for the whole database. Both fixed.cotp listpanicked (usize underflow) when all issuers/labels were shorter than the column headers.Security
cotp exportin every format) were world-readable; now created 0600 on unix with a plaintext warning, and I/O errors return instead of panicking. The database file itself is also created 0600.db.cotp(invalid base64 nonce/cipher/salt), malformed encrypted Aegis backups (missing slot params, non-hex fields, non-power-of-two scryptn— which previously derived a silently wrong key).OTPElement'sDebugoutput now redactssecret/pin;period == 0is rejected everywhere (previously a division-by-zero panic in three code generators);edit --change-secretnow goes through the same validation asadd.Other correctness fixes
C%23Corppreviously broke parsing; values were decoded twice).export --otp-uri→import: secrets are no longer force-uppercased andpinis emitted/parsed. The Aegis importer now mapspintoo.cno longer opens the delete popup; no more panics on tiny terminals, unset HOTP counters, oversized QR payloads, or thetodo!()popup path; the event thread exits cleanly on shutdown; OSC52 clipboard writes go to stderr so stdout stays pipeable.reading.rsno longer silently deletes an empty database file; declining a delete confirmation is no longer treated as an error; init errors go to stderr with sane exit codes; theaddintegration test no longer mutates the committed fixture.Performance
ThreadMode::Parallel). Same parameters, identical derived key — verified by test.Binary size (measured, release build)
globset(pulled the full regex engine for*/?matching inextract) for a small hand-rolled matcher: −512 KB.color-eyrewith plaineyre: −199 KB.idna_adapter = "~1.0"sourlstops linking ICU4X tables: −131 KB.hex+base64onto the already-useddata-encoding; trimmedratatuito default features (drops thetimecrate) andqrcodeto no default features (drops theimagecrate — compile-time win).Net: roughly −35% binary size (~2.26 MiB → ~1.5 MiB).
Architecture
unreachable!()) to exhaustive internal enums; CLI flags unchanged.OTPType/OTPAlgorithmnow implementTryFrom<&str>and reject unknown strings instead of silently defaulting to TOTP/SHA1 (imports error instead of mis-converting).OTPDatabaseintostorage/— the domain type no longer touches crypto or the global path, andsave()clears the dirty flag only after a successful write.needs_modificationis private withmark_modified()/clear_modified()accessors; the TUI no longer pokes the field directly. Importer/exporter error types unified oneyre.app.rsintoui.rs; the Aegis-encrypted password prompt moved out of theTryFromconversion into the argument layer; CLAUDE.md updated to match reality.Deliberately not done
rust-argon2→ RustCryptoargon2swap: the RustCrypto crate computes lanes sequentially, which would forfeit the parallel-unlock win above. Keptrust-argon2.opt-level = "z": skipped pending an Argon2 unlock-time benchmark (−64 KB measured, but "z" disables loop vectorization).PromptReaderabstraction for prompt testability: larger redesigns, left for follow-ups.Verification
cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings, andcargo test --lockedall pass at the merge head.