Security Scan — 2026-05-09#23
Conversation
Fixed: - Improved timing side-channel protection in CLI by replacing early-exit constant-time comparison. - Ensured Unicode-aware password strength estimation in GUI using character property methods. - Fixed potential UTF-8 slicing panics in GUI by using safe string suffix removal. - Hardened error handling in CLI by replacing generic unwrap calls with descriptive panic messages. - Migrated GUI and Main to egui 0.33 API. Deferred to SECURITY_DEFER.md: - Insufficient Argon2id memory cost (64MB vs 256MB) in no-touch src/crypto.rs. - Unhandled unwrap calls in no-touch src/crypto.rs. - Unmaintained 'paste' dependency. Co-authored-by: darkmaster0345 <152901560+darkmaster0345@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideSecurity-scan-driven hardening of the CLI password comparison, GUI password-strength evaluation and filename handling, plus minor egui API updates and safer progress/JSON error reporting, with deferred crypto-parameter work documented in SECURITY_DEFER.md. Class diagram for updated GUI and CLI helper functionsclassDiagram
class NeuronEncryptApp {
+draw_screen_header(ui: &mut egui_Ui)
+draw_password_field(ui: &mut egui_Ui, primary: bool)
+draw_warning_frame(ui: &mut egui_Ui, fill: Color32, border: Color32, text: Color32)
+update_ui(ctx: &egui_Context)
}
class eval_strength {
<<function>>
+eval_strength(password: &str) Strength_f32_str
}
class preview_output_name {
<<function>>
+preview_output_name(mode: Mode, path: &Path) String
}
class Mode {
<<enum>>
+Encrypt
+Decrypt
}
class Palette {
<<struct>>
+BG: Color32
+SURFACE: Color32
+SURFACE_1: Color32
+BORDER: Color32
+BORDER_MED: Color32
+WARNING: Color32
+warning_muted() Color32
}
class IndProgress {
<<struct>>
-inner: indicatif_ProgressBar
+new_with_bytes(total: Option_u64) IndProgress
}
class constant_time_eq {
<<function>>
+constant_time_eq(a: &[u8], b: &[u8]) bool
}
class emit_json {
<<function>>
+emit_json(result: &JsonResult) void
}
class read_password {
<<function>>
+read_password(password_file: &Option_PathBuf) Zeroizing_String
}
class read_password_confirmed {
<<function>>
+read_password_confirmed(password_file: &Option_PathBuf) Result_Zeroizing_String_String
}
class main_gui {
<<function>>
+main() eframe_Result
}
class crypto {
<<module>>
+EXTENSION: &str
}
NeuronEncryptApp ..> eval_strength : uses
NeuronEncryptApp ..> preview_output_name : uses
NeuronEncryptApp ..> Palette : styles
preview_output_name ..> Mode : parameter
preview_output_name ..> crypto : uses_EXTENSION
IndProgress ..> constant_time_eq : no
IndProgress ..> emit_json : no
read_password_confirmed ..> read_password : calls
main_gui ..> NeuronEncryptApp : constructs
main_gui ..> egui_FontDefinitions : configures_fonts
class egui_Frame {
<<struct>>
+new() egui_Frame
+fill(color: Color32) egui_Frame
+stroke(stroke: Stroke) egui_Frame
+corner_radius(radius: f32) egui_Frame
+inner_margin(margin: f32) egui_Frame
}
class egui_Painter {
<<struct>>
+rect_filled(rect: Rect, rounding: f32, fill: Color32) void
+rect_stroke(rect: Rect, rounding: f32, stroke: Stroke, kind: egui_StrokeKind) void
}
NeuronEncryptApp ..> egui_Frame : layouts
NeuronEncryptApp ..> egui_Painter : draws
class egui_StrokeKind {
<<enum>>
+Middle
}
class Stroke {
<<struct>>
+new(width: f32, color: Color32) Stroke
}
egui_Painter ..> Stroke : parameter
egui_Painter ..> egui_StrokeKind : parameter
eval_strength : uses_is_uppercase
eval_strength : uses_is_numeric
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis PR documents three deferred security findings, updates audit results, hardens error handling in CLI code via ChangesSecurity Findings & Audit Results
Runtime Error Handling Hardening
UI Improvements and Egui Framework API Modernization
Font Registration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
preview_output_namelogic usingstrip_suffix(crypto::EXTENSION)is now case-sensitive, which changes behavior from the previous case-insensitiveto_lowercase().ends_with(...)check and may break extension stripping for files with differently cased extensions. - The
constant_time_eqimplementation has become quite complex (bit tricks onusizefor the length mismatch flag); consider simplifying by folding the length difference directly into the accumulator or using a well-reviewed helper (e.g., fromsubtle) to reduce the risk of subtle bugs while keeping constant-time behavior. - The empty
audit_output.jsonfile was added to the repo; if this is a generated artifact rather than configuration, consider removing it or adding it to.gitignoreinstead of committing it.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `preview_output_name` logic using `strip_suffix(crypto::EXTENSION)` is now case-sensitive, which changes behavior from the previous case-insensitive `to_lowercase().ends_with(...)` check and may break extension stripping for files with differently cased extensions.
- The `constant_time_eq` implementation has become quite complex (bit tricks on `usize` for the length mismatch flag); consider simplifying by folding the length difference directly into the accumulator or using a well-reviewed helper (e.g., from `subtle`) to reduce the risk of subtle bugs while keeping constant-time behavior.
- The empty `audit_output.json` file was added to the repo; if this is a generated artifact rather than configuration, consider removing it or adding it to `.gitignore` instead of committing it.
## Individual Comments
### Comment 1
<location path="neuron-encrypt/src/gui.rs" line_range="291-293" />
<code_context>
match mode {
Mode::Encrypt => format!("{}{}", name, crypto::EXTENSION),
Mode::Decrypt => {
- let lower = name.to_lowercase();
- if lower.ends_with(crypto::EXTENSION) {
- // Use char-boundary-safe slicing
- let strip_len = name.len() - crypto::EXTENSION.len();
- name[..strip_len].to_owned()
- } else {
- name
- }
+ name.strip_suffix(crypto::EXTENSION)
+ .map(|s| s.to_owned())
+ .unwrap_or(name)
}
</code_context>
<issue_to_address>
**issue:** Decrypt output name no longer strips extension case-insensitively.
The previous logic handled the extension case-insensitively (e.g. `.vx2`, `.VX2`, `.Vx2`) by checking `to_lowercase().ends_with(crypto::EXTENSION)` and slicing the original string. `strip_suffix` is case-sensitive, so those variants will no longer be treated as encrypted and the extension will appear in the preview. If that behavior is still required, you could still compute the strip index from a lowercased copy while slicing the original string to stay boundary-safe.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| Mode::Decrypt => { | ||
| let lower = name.to_lowercase(); | ||
| if lower.ends_with(crypto::EXTENSION) { | ||
| // Use char-boundary-safe slicing | ||
| let strip_len = name.len() - crypto::EXTENSION.len(); | ||
| name[..strip_len].to_owned() | ||
| } else { | ||
| name | ||
| } | ||
| name.strip_suffix(crypto::EXTENSION) | ||
| .map(|s| s.to_owned()) |
There was a problem hiding this comment.
issue: Decrypt output name no longer strips extension case-insensitively.
The previous logic handled the extension case-insensitively (e.g. .vx2, .VX2, .Vx2) by checking to_lowercase().ends_with(crypto::EXTENSION) and slicing the original string. strip_suffix is case-sensitive, so those variants will no longer be treated as encrypted and the extension will appear in the preview. If that behavior is still required, you could still compute the strip index from a lowercased copy while slicing the original string to stay boundary-safe.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1daf61c4fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else { | ||
| name | ||
| } | ||
| name.strip_suffix(crypto::EXTENSION) |
There was a problem hiding this comment.
preview_output_name now uses a case-sensitive strip_suffix(crypto::EXTENSION), so decrypting files like report.VX2 leaves the name unchanged. In the GUI decrypt flow this makes destination path equal to source path, and the crypto path validation rejects it as SourceAndDestinationSame, so decryption fails for uppercase/mixed-case extensions. This is a regression from the previous case-insensitive behavior and should keep extension stripping case-insensitive.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
neuron-encrypt/src/bin/cli.rs (2)
346-351: 💤 Low value
expect()here is cosmetic — failure mode is still a panic.
serde_json::to_stringon a#[derive(Serialize)]struct ofString/Option<String>/u64/u128cannot fail in practice (noSerializeimpl inJsonResultreturnsErr). Switching fromunwrap()toexpect("...")only changes the panic message, so the PR description's "improved error handling for JSON emission" overstates the change.If you want this hardening to be meaningful, consider degrading to plain text on the (unreachable) error path so a
--jsonconsumer at least gets non-panicking output:♻️ Optional graceful fallback
fn emit_json(result: &JsonResult) { - println!( - "{}", - serde_json::to_string(result).expect("Failed to serialize JSON result") - ); + match serde_json::to_string(result) { + Ok(s) => println!("{s}"), + Err(e) => eprintln!("Error: failed to serialize JSON result: {e}"), + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@neuron-encrypt/src/bin/cli.rs` around lines 346 - 351, The current emit_json function uses expect which still panics on serialization failure; instead, call serde_json::to_string(result) and handle the Result explicitly in emit_json: on Ok print the JSON string, and on Err print a sensible plain-text fallback representation of the same data (e.g., the human-readable fields of JsonResult) to stdout so a --json consumer still gets non-panicking output, and write the serialization error details to stderr (or processLogger) so the failure is recorded; reference emit_json and the JsonResult type when making this change.
197-216: 💤 Low valueConsider swallowing write errors in the formatter closures instead of panicking.
For
ProgressStyle::with_template(...)(lines 201, 208), the templates are compile-time-constant string literals, soexpect()is appropriate — a malformed template would be caught in CI.For the
bytes_per_secformatter closures (lines 203–205, 212–215), thewith_keyclosure signature does not return a Result, so any formatting errors must be handled internally. Whilewrite!into the indicatif-provided&mut dyn fmt::Writeis unlikely to fail in practice, swallowing the error defensively prevents a potential panic from unwinding through indicatif's internal tick thread and aborting a long-running encrypt operation mid-stream.♻️ Optional defensive write
- .with_key("bytes_per_sec", |state: &ProgressState, w: &mut dyn std::fmt::Write| { - write!(w, "{}/s", HumanBytes(state.pos())).expect("Failed to format bytes per second"); - }) + .with_key("bytes_per_sec", |state: &ProgressState, w: &mut dyn std::fmt::Write| { + let _ = write!(w, "{}/s", HumanBytes(state.pos())); + })(and likewise for the spinner branch at line 212.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@neuron-encrypt/src/bin/cli.rs` around lines 197 - 216, The bytes_per_sec formatter closures passed to ProgressStyle::with_template use write!(...) followed by .expect(...), which can panic from inside the indicatif tick thread; change those closures in both branches (the with_key closures referencing ProgressState and HumanBytes) to swallow or handle fmt::Error internally (e.g., replace the .expect panic with matching or ignoring the Result from write! so the closure never panics), ensuring the closure still writes the same formatted "{}/s" output but does not call panic-inducing methods.SECURITY_DEFER.md (1)
1-9: ⚡ Quick winTrack the Critical Argon2id deferral with a hard owner/deadline.
This item is correctly flagged Critical with a concrete remediation (m_cost=262144), but the entry only records detection/attempt dates and an attempt count. For a Critical item parked under a no-touch zone, consider adding an explicit owner and target-by date (or linked tracking issue) to this entry so the deferral does not silently age out across weekly scans. The PR description already notes "Human review required" — wiring that to a tracked issue here would close the loop.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SECURITY_DEFER.md` around lines 1 - 9, The SECURITY_DEFER.md entry for the Critical Argon2id deferral lacks an explicit owner and a remediation deadline/tracking link; update the entry for "[ARGON2ID_PARAMS] - Insufficient Argon2id memory cost" to include a hard owner (e.g., team or person), a target-by date or a link to a tracking ticket, and mention the concrete remediation (update derive_key and derive_key_v3 to use m_cost=262144) so the deferral is tracked and won't age out silently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@neuron-encrypt/src/bin/cli.rs`:
- Around line 144-166: The constant_time_eq function leaks the longer input
length and may be optimized into branches; to fix, ensure comparisons operate
over a fixed, public length or prevent optimizer short-circuiting: either (A) in
read_password_confirmed pad/normalize both passphrase slices to a predetermined
fixed length before calling subtle::ConstantTimeEq::ct_eq (or subtle::ct_eq) so
lengths are equal, or (B) keep the hand-rolled constant_time_eq but wrap the
comparison accumulator and final_result uses with core::hint::black_box (or use
volatile reads/writes) to stop LLVM from introducing branches; update calls to
constant_time_eq/read_password_confirmed accordingly so inputs are padded or
black_box-protected.
---
Nitpick comments:
In `@neuron-encrypt/src/bin/cli.rs`:
- Around line 346-351: The current emit_json function uses expect which still
panics on serialization failure; instead, call serde_json::to_string(result) and
handle the Result explicitly in emit_json: on Ok print the JSON string, and on
Err print a sensible plain-text fallback representation of the same data (e.g.,
the human-readable fields of JsonResult) to stdout so a --json consumer still
gets non-panicking output, and write the serialization error details to stderr
(or processLogger) so the failure is recorded; reference emit_json and the
JsonResult type when making this change.
- Around line 197-216: The bytes_per_sec formatter closures passed to
ProgressStyle::with_template use write!(...) followed by .expect(...), which can
panic from inside the indicatif tick thread; change those closures in both
branches (the with_key closures referencing ProgressState and HumanBytes) to
swallow or handle fmt::Error internally (e.g., replace the .expect panic with
matching or ignoring the Result from write! so the closure never panics),
ensuring the closure still writes the same formatted "{}/s" output but does not
call panic-inducing methods.
In `@SECURITY_DEFER.md`:
- Around line 1-9: The SECURITY_DEFER.md entry for the Critical Argon2id
deferral lacks an explicit owner and a remediation deadline/tracking link;
update the entry for "[ARGON2ID_PARAMS] - Insufficient Argon2id memory cost" to
include a hard owner (e.g., team or person), a target-by date or a link to a
tracking ticket, and mention the concrete remediation (update derive_key and
derive_key_v3 to use m_cost=262144) so the deferral is tracked and won't age out
silently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: edced831-2f80-4b6e-837e-223ae3280b97
📒 Files selected for processing (5)
SECURITY_DEFER.mdaudit_output.jsonneuron-encrypt/src/bin/cli.rsneuron-encrypt/src/gui.rsneuron-encrypt/src/main.rs
| fn constant_time_eq(a: &[u8], b: &[u8]) -> bool { | ||
| if a.len() != b.len() { | ||
| return false; | ||
| } | ||
| let mut diff = 0u8; | ||
| for (&x, &y) in a.iter().zip(b.iter()) { | ||
| diff |= x ^ y; | ||
| let len_a = a.len(); | ||
| let len_b = b.len(); | ||
| let max_len = len_a.max(len_b); | ||
|
|
||
| let mut byte_comparison_result = 0; | ||
|
|
||
| for i in 0..max_len { | ||
| let byte_a = a.get(i).unwrap_or(&0); | ||
| let byte_b = b.get(i).unwrap_or(&0); | ||
| byte_comparison_result |= byte_a ^ byte_b; | ||
| } | ||
| diff == 0 | ||
|
|
||
| let len_diff = len_a ^ len_b; | ||
| // len_mismatch_flag will be 0 if lengths are equal, 1 if lengths are different | ||
| let len_mismatch_flag = (((len_diff | len_diff.wrapping_neg()) >> (usize::BITS - 1)) & 1) as u8; | ||
|
|
||
| // Combine byte comparison result with length mismatch flag | ||
| // If either bytes don't match OR lengths don't match, final_result will be non-zero | ||
| let final_result = byte_comparison_result | len_mismatch_flag; | ||
|
|
||
| final_result == 0 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'Cargo.toml' --exec rg -nH '^\s*(subtle|aes-gcm|aes-gcm-siv|argon2|chacha20poly1305|curve25519-dalek)\b' {}
fd -t f 'Cargo.lock' --exec rg -n '^name = "subtle"' {}Repository: darkmaster0345/Neuron-Encrypt
Length of output: 273
🌐 Web query:
Does the subtle crate's ConstantTimeEq::ct_eq handle slices of differing lengths in constant time, and what is its current stable version?
💡 Result:
No. The subtle crate’s ConstantTimeEq::ct_eq for slices explicitly short-circuits (and therefore does not run in time independent of slice lengths) when slice lengths differ: it checks the lengths and returns Choice::from(0) immediately if they are not equal.[1][2] Current stable version (crates.io): subtle 2.6.1.[3]
Citations:
- 1: https://github.com/dalek-cryptography/subtle/blob/master/src/lib.rs
- 2: https://docs.rs/subtle/latest/subtle/trait.ConstantTimeEq.html
- 3: https://crates.io/crates/subtle/0.8.0
🏁 Script executed:
# Verify subtle version and behavior more precisely
rg -n 'name = "subtle"' neuron-encrypt/Cargo.lock | head -20Repository: darkmaster0345/Neuron-Encrypt
Length of output: 94
Hand-rolled constant-time compare still leaks the longer length, and bit tricks are fragile under LLVM.
The original early-exit timing leak is gone — good. But two critical concerns remain:
-
Residual timing leak on input length. The loop runs for
max_len = max(len_a, len_b)iterations, so runtime is proportional to the longer input's length. An observer learnsmax(|a|, |b|), which inread_password_confirmed(line 182) is the length of the longer of the two typed passphrases. A genuinely constant-time compare requires fixed iteration count or accepts that input length is public. -
Compiler may defeat the masking.
final_result == 0and the|=loop are not guaranteed to compile to branchless code. LLVM is free to introduce branches or short-circuit if it can prove the result. Constant-time properties at source level are not preserved by the optimizer without explicit barriers likecore::hint::black_box.
subtle::ConstantTimeEq fix is inadequate: The subtle crate's ct_eq for slices also short-circuits when input lengths differ—it checks lengths and returns early if unequal. This preserves the exact timing vulnerability identified in concern #1. Using subtle requires padding both inputs to equal length before comparison.
Correct approaches:
- Pad both inputs to a fixed length, then use
subtle::ct_eq(padded_a, padded_b), or - Keep a hand-rolled constant-time loop but add
core::hint::black_box()or volatile reads around the comparison result to prevent LLVM from introducing branches.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@neuron-encrypt/src/bin/cli.rs` around lines 144 - 166, The constant_time_eq
function leaks the longer input length and may be optimized into branches; to
fix, ensure comparisons operate over a fixed, public length or prevent optimizer
short-circuiting: either (A) in read_password_confirmed pad/normalize both
passphrase slices to a predetermined fixed length before calling
subtle::ConstantTimeEq::ct_eq (or subtle::ct_eq) so lengths are equal, or (B)
keep the hand-rolled constant_time_eq but wrap the comparison accumulator and
final_result uses with core::hint::black_box (or use volatile reads/writes) to
stop LLVM from introducing branches; update calls to
constant_time_eq/read_password_confirmed accordingly so inputs are padded or
black_box-protected.
1 participant
Security Scan Results — 2026-05-09
FIXED ISSUES (4)
[High] neuron-encrypt/src/bin/cli.rs:145 — replaced early-exit constant-time equality check to prevent timing side-channels.
[Medium] neuron-encrypt/src/gui.rs:242 — updated password strength logic to be Unicode-aware.
[Medium] neuron-encrypt/src/gui.rs:293 — used strip_suffix for safe string handling.
[Medium] neuron-encrypt/src/bin/cli.rs:200 — improved error handling in progress bars and JSON emission.
DEFERRED ISSUES (3)
[Critical] neuron-encrypt/src/crypto.rs:114 — insufficient Argon2id memory cost — no-touch zone.
[Medium] neuron-encrypt/src/crypto.rs:116 — unhandled unwrap() calls in crypto paths — no-touch zone.
[Medium] Cargo.lock — unmaintained dependency: paste — no-touch zone.
See SECURITY_DEFER.md for full details.
DEFERRED FILE CHANGES
New entries added: 3
Fixed entries removed: 0
Existing entries updated (attempt count): 0
TESTING
cargo check — passed
cargo clippy — passed
cargo test — passed
NO-TOUCH ZONES RESPECTED
src/crypto.rs — not modified
Cargo.lock — not modified
NEXT STEPS FOR HUMAN
Human review required for Argon2id parameter hardening in src/crypto.rs. Current m_cost (64MB) is below the project's 256MB hardened requirement.
PR created automatically by Jules for task 5855676036325782532 started by @darkmaster0345
Summary by Sourcery
Address security scan findings by hardening password handling, improving CLI robustness, and documenting deferred security issues.
Bug Fixes:
Enhancements:
Documentation:
Summary by CodeRabbit
Documentation
Bug Fixes
Style
Chores