fix: correct paths being passed to fix streaming bug #17
Conversation
WalkthroughThe updates introduce a clearer distinction between absolute and display paths for file operations and UI, refactor file transfer utilities to use async traits and atomic progress tracking, and enhance the file transfer protocol to explicitly communicate file paths. Extensive tracing and error handling are added, and comprehensive async tests for file transfer are introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant Sender as File Sender
participant Receiver as File Receiver
Sender->>Receiver: Open file transfer stream
Sender->>Receiver: Send path length (u64 LE)
Sender->>Receiver: Send file path (UTF-8 bytes)
Sender->>Receiver: Send file size (usize LE)
Sender->>Receiver: Stream file data in chunks
Receiver->>Sender: Acknowledge (implicit by protocol completion)
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)src/app.rs(11 hunks)src/main.rs(8 hunks)src/service/node.rs(3 hunks)src/service/utils.rs(1 hunks)src/tests.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Format
🔇 Additional comments (14)
.gitignore (1)
5-6: LGTM!Standard gitignore patterns for test directories and macOS system files.
src/service/node.rs (1)
488-513: Good error handling implementationThe outbound file transfer properly handles errors at each step with appropriate logging.
src/main.rs (3)
282-285: Potential issue with path canonicalization fallbackIf
canonicalizefails (e.g., file doesn't exist or permissions issue), the fallback returns the original path which might not be absolute. This could lead to inconsistent behavior.Consider handling the error case more explicitly:
- let abs_path = std::fs::canonicalize(path).unwrap_or_else(|_| path.clone()); + let abs_path = match std::fs::canonicalize(path) { + Ok(canonical) => canonical, + Err(e) => { + tracing::warn!("Failed to canonicalize path {:?}: {}", path, e); + // Ensure we at least have an absolute path + if path.is_absolute() { + path.clone() + } else { + std::env::current_dir().unwrap_or_default().join(path) + } + } + };
373-385: Consistent path handling in download modeGood implementation of setting both absolute and display paths for UI consistency.
Also applies to: 415-422
239-240: Improved debug loggingGood change to log the full list of items instead of just the count, providing better debugging information.
src/app.rs (2)
37-38: Good separation of concerns for path handlingThe distinction between
path(for file operations) anddisplay_path(for UI) is a clean design that prevents path-related bugs.
428-437: Good defensive programmingExcellent validation to ensure the path is not empty before using it.
src/tests.rs (3)
14-62: Well-implemented async trait adapterThe
StreamWrappercorrectly bridges betweentokio::ioandfutures::ioasync traits, enabling proper testing of the refactored file transfer utilities.
295-370: Comprehensive test coverage for new file transfer protocolExcellent test coverage including:
- Path handling verification
- Full protocol testing with mock streams
- Both relative and absolute path scenarios
284-284: Test updated correctly for new DirectoryItem structureThe test properly initializes the new
display_pathfield.src/service/utils.rs (4)
1-10: LGTM! Well-organized imports for async file operations.The dual import of async traits from both
futuresandtokiois correctly handled with aliasing to avoid conflicts.
11-38: Well-designed error handling with proper trait implementations.The
FileTransferErrorenum provides comprehensive error handling for file transfer operations with appropriate trait implementations for error propagation.
73-136: Robust file streaming implementation with proper protocol handling.The
stream_filemethod correctly implements the file transfer protocol by sending path information and file size before content, with efficient buffered reading and atomic progress tracking.
138-231: Well-implemented file receiving with proper protocol adherence.The
FileReceivercorrectly implements the receiving side of the protocol, with proper bounds checking to prevent over-reading and automatic parent directory creation. The atomic progress tracking matches the sender side.
| let mut path_len_bytes = [0u8; 8]; | ||
| if let Err(e) = stream.read_exact(&mut path_len_bytes).await { | ||
| tracing::error!("Failed to read path length from peer {}: {}", peer, e); | ||
| drop(permit); | ||
| return; | ||
| } | ||
| let path_len = usize::try_from(u64::from_le_bytes(path_len_bytes)) | ||
| .map_err(|e| Box::new(e) as Box<dyn Error + Send>); | ||
|
|
||
| // Read the requested file path | ||
| let mut path_bytes = vec![0u8; path_len.unwrap()]; | ||
| if let Err(e) = stream.read_exact(&mut path_bytes).await { | ||
| tracing::error!("Failed to read path from peer {}: {}", peer, e); | ||
| drop(permit); | ||
| return; | ||
| } | ||
| let file_path = match String::from_utf8(path_bytes) { | ||
| Ok(path) => path, | ||
| Err(e) => { | ||
| tracing::error!("Invalid UTF-8 in path from peer {}: {}", peer, e); | ||
| drop(permit); | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| match receiver.receive_file(&mut stream).await { | ||
| Ok(file_name) => { | ||
| tracing::info!("Successfully received file '{}' from peer {}", file_name, peer); | ||
| tracing::info!("Received file request for '{}' from peer {}", file_path, peer); |
There was a problem hiding this comment.
Critical security issues: unbounded memory allocation and path traversal vulnerability
The code has two critical security vulnerabilities:
- DoS via unbounded memory allocation: Line 288 allocates memory based on untrusted input without bounds checking.
- Path traversal vulnerability: Line 306 uses the received path without validation.
Apply this diff to fix both issues:
- let path_len = usize::try_from(u64::from_le_bytes(path_len_bytes))
- .map_err(|e| Box::new(e) as Box<dyn Error + Send>);
-
- // Read the requested file path
- let mut path_bytes = vec![0u8; path_len.unwrap()];
+ let path_len = match usize::try_from(u64::from_le_bytes(path_len_bytes)) {
+ Ok(len) if len <= 4096 => len, // Reasonable path length limit
+ Ok(len) => {
+ tracing::error!("Path length too large from peer {}: {}", peer, len);
+ drop(permit);
+ return;
+ }
+ Err(e) => {
+ tracing::error!("Invalid path length from peer {}: {}", peer, e);
+ drop(permit);
+ return;
+ }
+ };
+
+ // Read the requested file path
+ let mut path_bytes = vec![0u8; path_len];Additionally, validate the path after line 301:
};
tracing::info!("Received file request for '{}' from peer {}", file_path, peer);
+
+ // Validate path - prevent directory traversal
+ let path = PathBuf::from(&file_path);
+ if path.components().any(|c| matches!(c, std::path::Component::ParentDir)) {
+ tracing::error!("Invalid path with directory traversal from peer {}: {}", peer, file_path);
+ drop(permit);
+ return;
+ }
// Send the file
- let transfer = FileTransfer::new(&PathBuf::from(&file_path));
+ let transfer = FileTransfer::new(&path);📝 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.
| let mut path_len_bytes = [0u8; 8]; | |
| if let Err(e) = stream.read_exact(&mut path_len_bytes).await { | |
| tracing::error!("Failed to read path length from peer {}: {}", peer, e); | |
| drop(permit); | |
| return; | |
| } | |
| let path_len = usize::try_from(u64::from_le_bytes(path_len_bytes)) | |
| .map_err(|e| Box::new(e) as Box<dyn Error + Send>); | |
| // Read the requested file path | |
| let mut path_bytes = vec![0u8; path_len.unwrap()]; | |
| if let Err(e) = stream.read_exact(&mut path_bytes).await { | |
| tracing::error!("Failed to read path from peer {}: {}", peer, e); | |
| drop(permit); | |
| return; | |
| } | |
| let file_path = match String::from_utf8(path_bytes) { | |
| Ok(path) => path, | |
| Err(e) => { | |
| tracing::error!("Invalid UTF-8 in path from peer {}: {}", peer, e); | |
| drop(permit); | |
| return; | |
| } | |
| }; | |
| match receiver.receive_file(&mut stream).await { | |
| Ok(file_name) => { | |
| tracing::info!("Successfully received file '{}' from peer {}", file_name, peer); | |
| tracing::info!("Received file request for '{}' from peer {}", file_path, peer); | |
| let mut path_len_bytes = [0u8; 8]; | |
| if let Err(e) = stream.read_exact(&mut path_len_bytes).await { | |
| tracing::error!("Failed to read path length from peer {}: {}", peer, e); | |
| drop(permit); | |
| return; | |
| } | |
| - let path_len = usize::try_from(u64::from_le_bytes(path_len_bytes)) | |
| - .map_err(|e| Box::new(e) as Box<dyn Error + Send>); | |
| - | |
| - // Read the requested file path | |
| - let mut path_bytes = vec![0u8; path_len.unwrap()]; | |
| + let path_len = match usize::try_from(u64::from_le_bytes(path_len_bytes)) { | |
| + Ok(len) if len <= 4096 => len, // Reasonable path length limit | |
| + Ok(len) => { | |
| + tracing::error!("Path length too large from peer {}: {}", peer, len); | |
| + drop(permit); | |
| + return; | |
| + } | |
| + Err(e) => { | |
| + tracing::error!("Invalid path length from peer {}: {}", peer, e); | |
| + drop(permit); | |
| + return; | |
| + } | |
| + }; | |
| + | |
| + // Read the requested file path | |
| + let mut path_bytes = vec![0u8; path_len]; | |
| if let Err(e) = stream.read_exact(&mut path_bytes).await { | |
| tracing::error!("Failed to read path from peer {}: {}", peer, e); | |
| drop(permit); | |
| return; | |
| } | |
| let file_path = match String::from_utf8(path_bytes) { | |
| Ok(path) => path, | |
| Err(e) => { | |
| tracing::error!("Invalid UTF-8 in path from peer {}: {}", peer, e); | |
| drop(permit); | |
| return; | |
| } | |
| }; | |
| tracing::info!("Received file request for '{}' from peer {}", file_path, peer); | |
| + // Validate path — prevent directory traversal | |
| + let path = PathBuf::from(&file_path); | |
| + if path.components().any(|c| matches!(c, std::path::Component::ParentDir)) { | |
| + tracing::error!("Invalid path with directory traversal from peer {}: {}", peer, file_path); | |
| + drop(permit); | |
| + return; | |
| + } | |
| // Send the file | |
| - let transfer = FileTransfer::new(&PathBuf::from(&file_path)); | |
| + let transfer = FileTransfer::new(&path); |
🤖 Prompt for AI Agents
In src/service/node.rs around lines 278 to 303, the code reads a path length
from the stream and allocates a vector of that size without any bounds checking,
which can lead to unbounded memory allocation and potential DoS. To fix this,
add a maximum allowed path length check after reading path_len and before
allocating the vector, returning early with an error if the length exceeds this
limit. Additionally, after converting the path bytes to a string, validate the
file path to prevent path traversal attacks by ensuring it does not contain
components like ".." or absolute paths; reject or sanitize invalid paths before
further use.
There was a problem hiding this comment.
Open a follow-up GitHub issue for this discussion
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
TITLE="Security: Validate path length and sanitize file paths in file transfer handler"
PR_URL="https://github.com/maschad/junkanoo/pull/17"
COMMENT_URL="https://github.com/maschad/junkanoo/pull/17#discussion_r2140895560"
BODY=$(cat <<EOF
Follow-up issue for security concerns identified in ${PR_URL}.
Context:
- Discussion comment: ${COMMENT_URL}
- Raised by: @coderabbitai
- Requested by: @maschad
Summary of issues:
1. Potential Denial-of-Service via unbounded memory allocation when allocating buffer for file path length received from peer in \`src/service/node.rs\`.
2. Path traversal vulnerability due to lack of validation/sanitization of the received file path prior to use.
This issue tracks implementing safeguards (reasonable maximum path length, rejection of paths containing parent directory components) and adding tests to cover these cases.
EOF
)
gh issue create --title "$TITLE" --body "$BODY" --assignee "maschad"
Length of output: 1840
@maschad The follow-up security issue has been opened successfully:
#20
It captures the DoS risk from unbounded allocation and the path-traversal vulnerability, and links back to the PR discussion for context. Let me know if you’d like any tweaks to the issue description or additional details!
🧬 Code Graph Analysis Results
Relevant snippets from src/service/utils.rs (lines 48-66 and 73-135)
pub fn new(path: &PathBuf) -> Self {
// Convert to relative path immediately
let current_dir = std::env::current_dir().unwrap_or_default();
let relative_path = path
.strip_prefix(¤t_dir)
.unwrap_or(path)
.to_path_buf();
tracing::debug!(
"Relative path being used for file transfer: {:?}",
relative_path
);
Self {
path: relative_path,
chunk_size: 1024 * 1024, // 1MB chunks
progress: Arc::new(AtomicUsize::new(0)),
}
}
pub async fn stream_file<S>(&self, stream: &mut S) -> Result<(), Box<dyn Error + Send>>
where
S: AsyncWrite + Unpin,
{
let current_dir =
std::env::current_dir().map_err(|e| Box::new(e) as Box<dyn Error + Send>)?;
let full_path = current_dir.join(&self.path);
tracing::debug!("Full path being used for file transfer: {:?}", full_path);
let file = File::open(&full_path)
.await
.map_err(|e| Box::new(e) as Box<dyn Error + Send>)?;
let metadata = file
.metadata()
.await
.map_err(|e| Box::new(e) as Box<dyn Error + Send>)?;
let file_size =
usize::try_from(metadata.len()).map_err(|e| Box::new(e) as Box<dyn Error + Send>)?;
// Send the relative path and file size
let path_str = self.path.to_string_lossy().to_string();
let path_bytes = path_str.as_bytes();
stream
.write_all(&(path_bytes.len() as u64).to_le_bytes())
.await
.map_err(|e| Box::new(e) as Box<dyn Error + Send>)?;
stream
.write_all(path_bytes)
.await
.map_err(|e| Box::new(e) as Box<dyn Error + Send>)?;
stream
.write_all(&file_size.to_le_bytes())
.await
.map_err(|e| Box::new(e) as Box<dyn Error + Send>)?;
let mut reader = tokio::io::BufReader::with_capacity(self.chunk_size, file);
let mut buffer = vec![0u8; self.chunk_size];
let mut total_read = 0;
loop {
let bytes_read = reader
.read(&mut buffer)
.await
.map_err(|e| Box::new(e) as Box<dyn Error + Send>)?;
if bytes_read == 0 {
break;
}
stream
.write_all(&buffer[..bytes_read])
.await
.map_err(|e| Box::new(e) as Box<dyn Error + Send>)?;
total_read += bytes_read;
self.progress.store(total_read, Ordering::SeqCst);
}
stream
.flush()
.await
.map_err(|e| Box::new(e) as Box<dyn Error + Send>)?;
Ok(())
}Summary:
These functions define a FileTransfer struct constructor and a method to stream a file over an asynchronous writable stream. The new function converts a given path to a relative path based on the current working directory. The stream_file method opens the file, sends the relative path length, path bytes, and file size over the stream, then reads the file in chunks and writes these chunks to the stream asynchronously. It handles errors by boxing them as Box<dyn Error + Send>. Progress is tracked via an atomic counter.
Possibly relevant snippet from src/app.rs (lines 65-91)
pub fn new() -> Self {
let mut app = Self {
directory_items: Vec::new(),
all_shared_items: Vec::new(),
directory_cache: HashMap::new(),
selected_index: None,
current_path: std::env::current_dir().unwrap_or_else(|_| PathBuf::from("/")),
connection_state: ConnectionState::Disconnected,
peer_id: PeerId::random(),
connected_peer_id: None,
state: AppState::Share,
is_host: true,
is_loading: false,
listening_addrs: Vec::new(),
items_to_share: HashSet::new(),
items_being_shared: HashSet::new(),
items_to_download: HashSet::new(),
items_being_downloaded: HashSet::new(),
warning: None,
refresh_sender: None,
client: None,
clipboard_success: false,
};
app.populate_directory_items();
app
}Summary:
This snippet shows the initialization of the main application state struct, including fields for directory items, connection state, peer IDs, sharing and downloading state, and a client handle. It sets up the initial current directory and peer ID, which are relevant to the network service's peer and directory management.
These snippets provide context on file transfer handling, peer and directory state initialization, and asynchronous streaming operations relevant to the src/service/node.rs file's network and file transfer logic.
| let file_names: Vec<String> = self | ||
| .items_to_download | ||
| .iter() | ||
| .map(|path| path.to_string_lossy().to_string()) | ||
| .filter_map(|path| { | ||
| // Find the original item in all_shared_items to get the absolute path | ||
| self.all_shared_items | ||
| .iter() | ||
| .find(|item| { | ||
| item.name == path.file_name().unwrap_or_default().to_string_lossy() | ||
| }) | ||
| .map(|item| { | ||
| let path_str = item.path.to_string_lossy().to_string(); | ||
| tracing::info!("Using absolute path for download: {}", path_str); | ||
| path_str | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Potential issue with file name matching
The current implementation matches files by name only, which could select the wrong file if multiple files have the same name in different directories.
Consider using a more robust matching strategy:
- .find(|item| {
- item.name == path.file_name().unwrap_or_default().to_string_lossy()
- })
+ .find(|item| {
+ // Match by the full path or a unique identifier
+ &item.path == path ||
+ (item.name == path.file_name().unwrap_or_default().to_string_lossy()
+ && item.path.parent() == path.parent())
+ })📝 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.
| let file_names: Vec<String> = self | |
| .items_to_download | |
| .iter() | |
| .map(|path| path.to_string_lossy().to_string()) | |
| .filter_map(|path| { | |
| // Find the original item in all_shared_items to get the absolute path | |
| self.all_shared_items | |
| .iter() | |
| .find(|item| { | |
| item.name == path.file_name().unwrap_or_default().to_string_lossy() | |
| }) | |
| .map(|item| { | |
| let path_str = item.path.to_string_lossy().to_string(); | |
| tracing::info!("Using absolute path for download: {}", path_str); | |
| path_str | |
| }) | |
| }) | |
| let file_names: Vec<String> = self | |
| .items_to_download | |
| .iter() | |
| .filter_map(|path| { | |
| // Find the original item in all_shared_items to get the absolute path | |
| self.all_shared_items | |
| .iter() | |
| .find(|item| { | |
| // Match by the full path or a unique identifier | |
| &item.path == path | |
| || (item.name == path.file_name().unwrap_or_default().to_string_lossy() | |
| && item.path.parent() == path.parent()) | |
| }) | |
| .map(|item| { | |
| let path_str = item.path.to_string_lossy().to_string(); | |
| tracing::info!("Using absolute path for download: {}", path_str); | |
| path_str | |
| }) | |
| }) |
🤖 Prompt for AI Agents
In src/app.rs around lines 554 to 569, the code matches files by name only when
filtering items_to_download against all_shared_items, which can cause incorrect
matches if multiple files share the same name in different directories. To fix
this, update the matching logic to compare the full relative or absolute paths
instead of just the file names, ensuring that the correct file is selected by
uniquely identifying it with its complete path.
There was a problem hiding this comment.
Open a follow-up GitHub issue for this discussion
There was a problem hiding this comment.
🧩 Analysis chain
@maschad Sure thing — I’ll open a follow-up issue that captures the file-matching concern raised in the review discussion.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue documenting the path-matching bug discussed in PR #17.
gh issue create \
--title "Follow-up: Improve file matching when resolving download paths" \
--body "$(cat <<'EOF'
### Context
During code review of pull request #17, a potential bug was identified in `src/app.rs` (see comment https://github.com/maschad/junkanoo/pull/17#discussion_r2140895565).
The current implementation resolves files to download by matching **only the file name**, which can select the wrong file when multiple shared files have identical names in different directories.
### Expected
Files chosen for download should be matched unambiguously, e.g. by comparing full relative/absolute paths or another unique identifier.
### Impact
If duplicate file names exist in separate directories, users may receive the wrong file.
### Acceptance Criteria
- Matching logic uniquely identifies the intended file (full path or equivalent).
- Unit/integration tests cover scenarios with duplicate file names.
Referenced PR: https://github.com/maschad/junkanoo/pull/17
EOF
)" \
--assignee "maschad"
Length of output: 1036
@maschad ✅ Follow-up issue created: #18 — it captures the file-matching bug noted in the review and includes context, expected behaviour, impact, and acceptance criteria. Let me know if you’d like any tweaks or extra details added.
🧬 Code Graph Analysis Results
Relevant snippets from src/service/node.rs (lines 55-126)
pub fn new() -> Result<(Client, impl Stream<Item = Event>, EventLoop, PeerId), Box<dyn Error>> {
// Create a public/private key pair, either random or based on a seed.
// let id_keys = Keypair::generate_ed25519();
// let peer_id = id_keys.public().to_peer_id();
let peer_id = PeerId::random();
let mut swarm = SwarmBuilder::with_new_identity()
.with_tokio()
.with_tcp(
tcp::Config::default(),
noise::Config::new,
yamux::Config::default,
)?
.with_quic()
.with_behaviour(|key| Behaviour {
kademlia: kad::Behaviour::new(
peer_id,
kad::store::MemoryStore::new(key.public().to_peer_id()),
),
request_response: request_response::cbor::Behaviour::new(
[(JUNKANOO_REQUEST_RESPONSE_PROTOCOL, ProtocolSupport::Full)],
request_response::Config::default(),
),
file_stream: stream::Behaviour::new(),
})?
.with_swarm_config(|c| {
c.with_idle_connection_timeout(Duration::from_secs(CONNECTION_TIMEOUT))
})
.build();
// Set up file transfer protocol listener
let incoming_streams = swarm
.behaviour_mut()
.file_stream
.new_control()
.accept(JUNKANOO_FILE_PROTOCOL)
.unwrap();
tracing::info!(
"Listening for file transfer streams on protocol: {}",
JUNKANOO_FILE_PROTOCOL
);
// Set Kademlia into server mode before adding bootnodes
swarm
.behaviour_mut()
.kademlia
.set_mode(Some(kad::Mode::Server));
// Then add the bootnodes
for peer in &BOOTNODES {
if let Ok(peer_id) = peer.parse() {
swarm
.behaviour_mut()
.kademlia
.add_address(&peer_id, "/dnsaddr/bootstrap.libp2p.io".parse()?);
}
}
let (command_sender, command_receiver) = mpsc::channel(0);
let (event_sender, event_receiver) = mpsc::channel(0);
let local_peer_id = *swarm.local_peer_id();
Ok((
Client {
sender: command_sender,
},
event_receiver,
EventLoop::new(swarm, command_receiver, event_sender, incoming_streams),
local_peer_id,
))
}Relevant snippet from src/main.rs (lines 352-464)
async fn handle_download_mode(
client: &mut Client,
target_peer_addr: Multiaddr,
app: Arc<Mutex<App>>,
) -> Result<(), &'static str> {
let target_peer_id = target_peer_addr
.iter()
.find_map(|p| match p {
Protocol::P2p(peer_id) => Some(peer_id),
_ => None,
})
.ok_or("Peer address must contain a peer ID component (/p2p/...)")?;
client.dial(target_peer_id, target_peer_addr).await.unwrap();
tokio::time::sleep(tokio::time::Duration::from_secs(1)).await;
// Initial directory request
match client.request_directory(target_peer_id).await {
Ok(display_response) => {
let mut items = display_response.items;
// For download mode, we need to ensure both paths are properly set
for item in &mut items {
tracing::info!("path before setting paths is: {:?}", item.path);
// Keep the absolute path for file operations
let abs_path = item.path.clone();
// Use just the name for display
let display_path = PathBuf::from(&item.name);
item.path = abs_path;
item.display_path = display_path;
tracing::info!(
"Set paths - absolute: {:?}, display: {:?}",
item.path,
item.display_path
);
}
items.sort_by(|a, b| match (a.is_dir, b.is_dir) {
(true, false) => std::cmp::Ordering::Less,
(false, true) => std::cmp::Ordering::Greater,
_ => match a.depth.cmp(&b.depth) {
std::cmp::Ordering::Equal => a.name.to_lowercase().cmp(&b.name.to_lowercase()),
other => other,
},
});
{
let mut app = app.lock();
app.all_shared_items.clone_from(&items);
app.directory_items = items;
app.current_path = PathBuf::new();
app.populate_directory_items();
tracing::info!("Initial directory items: {:?}", app.directory_items);
}
// Start a background task to handle directory updates
let mut client_clone = client.clone();
let app_clone = app.clone();
tokio::spawn(async move {
loop {
tokio::time::sleep(tokio::time::Duration::from_millis(100)).await;
match client_clone.request_directory(target_peer_id).await {
Ok(display_response) => {
let mut items = display_response.items;
// For download mode, we need to ensure both paths are properly set
for item in &mut items {
// Keep the absolute path for file operations
let abs_path = item.path.clone();
// Use just the name for display
let display_path = PathBuf::from(&item.name);
item.path = abs_path;
item.display_path = display_path;
}
items.sort_by(|a, b| match (a.is_dir, b.is_dir) {
(true, false) => std::cmp::Ordering::Less,
(false, true) => std::cmp::Ordering::Greater,
_ => match a.depth.cmp(&b.depth) {
std::cmp::Ordering::Equal => {
a.name.to_lowercase().cmp(&b.name.to_lowercase())
}
other => other,
},
});
let mut app = app_clone.lock();
if app.all_shared_items != items {
app.all_shared_items.clone_from(&items);
app.directory_items = items;
app.populate_directory_items();
tracing::info!(
"Updated directory items: {:?}",
app.directory_items
);
// Notify UI to refresh
if let Some(refresh_sender) = &app.refresh_sender {
let _ = refresh_sender.try_send(());
}
}
}
Err(e) => {
tracing::error!("Failed to request directory: {}", e);
break;
}
}
}
});
Ok(())
}
Err(e) => {
tracing::error!("Failed to request directory: {}", e);
Err("Failed to request directory")
}
}
}Summary of relevant code in src/app.rs (the provided file)
- The
Appstruct manages application state including directory items, shared/downloaded items, connection state, and UI state. - It supports two modes:
AppState::ShareandAppState::Download. - The method
populate_directory_itemspopulates the directory listing based on the current path and mode. - The method
handle_download_modefilters and sorts directory items when in download mode, showing items fromall_shared_items. - The
select_itemandunselect_itemmethods manage selection of files/directories for sharing or downloading, storing relative paths for sharing and absolute paths for downloading. - The
start_downloadasync method initiates file download requests via theClientusing absolute paths resolved fromall_shared_items. - The
Appmaintains aclient: Option<Client>to communicate with peers. - The
refresh_senderis used to notify the UI to refresh when selections or directory items change.
These snippets and summaries provide context on how the app manages file sharing and downloading, peer connections, directory listings, and UI updates, which are relevant for understanding the file and any related discussions or issues.
| pub fn new(path: &PathBuf) -> Self { | ||
| // Convert to relative path immediately | ||
| let current_dir = std::env::current_dir().unwrap_or_default(); | ||
| let relative_path = path | ||
| .strip_prefix(¤t_dir) | ||
| .unwrap_or(path) | ||
| .to_path_buf(); | ||
|
|
||
| tracing::debug!( | ||
| "Relative path being used for file transfer: {:?}", | ||
| relative_path | ||
| ); | ||
|
|
||
| Self { | ||
| path: metadata.path.into(), | ||
| path: relative_path, | ||
| chunk_size: 1024 * 1024, // 1MB chunks | ||
| progress: 0, | ||
| progress: Arc::new(AtomicUsize::new(0)), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider explicit error handling for current directory retrieval.
The use of unwrap_or_default() on line 50 could mask errors when getting the current directory. If the current directory cannot be determined, using an empty path as default might lead to unexpected behavior.
Consider propagating the error or using a more explicit fallback:
-pub fn new(path: &PathBuf) -> Self {
+pub fn new(path: &PathBuf) -> Result<Self, std::io::Error> {
// Convert to relative path immediately
- let current_dir = std::env::current_dir().unwrap_or_default();
+ let current_dir = std::env::current_dir()?;
let relative_path = path
.strip_prefix(¤t_dir)
.unwrap_or(path)
.to_path_buf();
tracing::debug!(
"Relative path being used for file transfer: {:?}",
relative_path
);
- Self {
+ Ok(Self {
path: relative_path,
chunk_size: 1024 * 1024, // 1MB chunks
progress: Arc::new(AtomicUsize::new(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.
| pub fn new(path: &PathBuf) -> Self { | |
| // Convert to relative path immediately | |
| let current_dir = std::env::current_dir().unwrap_or_default(); | |
| let relative_path = path | |
| .strip_prefix(¤t_dir) | |
| .unwrap_or(path) | |
| .to_path_buf(); | |
| tracing::debug!( | |
| "Relative path being used for file transfer: {:?}", | |
| relative_path | |
| ); | |
| Self { | |
| path: metadata.path.into(), | |
| path: relative_path, | |
| chunk_size: 1024 * 1024, // 1MB chunks | |
| progress: 0, | |
| progress: Arc::new(AtomicUsize::new(0)), | |
| } | |
| } | |
| pub fn new(path: &PathBuf) -> Result<Self, std::io::Error> { | |
| // Convert to relative path immediately | |
| let current_dir = std::env::current_dir()?; | |
| let relative_path = path | |
| .strip_prefix(¤t_dir) | |
| .unwrap_or(path) | |
| .to_path_buf(); | |
| tracing::debug!( | |
| "Relative path being used for file transfer: {:?}", | |
| relative_path | |
| ); | |
| Ok(Self { | |
| path: relative_path, | |
| chunk_size: 1024 * 1024, // 1MB chunks | |
| progress: Arc::new(AtomicUsize::new(0)), | |
| }) | |
| } |
🤖 Prompt for AI Agents
In src/service/utils.rs around lines 48 to 66, the current implementation uses
unwrap_or_default() when retrieving the current directory, which can mask errors
and lead to unexpected behavior if the directory cannot be determined. Modify
the new function to explicitly handle the error from std::env::current_dir() by
either propagating the error using a Result return type or by implementing a
clear fallback strategy with proper error logging or handling, instead of
defaulting to an empty path.
There was a problem hiding this comment.
Open a follow-up GitHub issue for this discussion
There was a problem hiding this comment.
🧩 Analysis chain
@maschad Sure thing — I’ll open a follow-up issue that captures this refactor task and links back to the PR discussion.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track explicit error handling in FileTransfer::new
gh issue create \
--title "Refactor: handle std::env::current_dir() errors in FileTransfer::new" \
--body $'In PR #17 (fix: correct paths being passed to fix streaming bug), CodeRabbit flagged that `FileTransfer::new` currently calls `std::env::current_dir().unwrap_or_default()`, which can silently fall back to an empty path and mask OS errors.\n\nThis issue tracks replacing that `unwrap_or_default()` usage with proper error propagation (e.g. returning `Result<Self, std::io::Error>` or another explicit strategy).\n\nReference discussion:\nhttps://github.com/maschad/junkanoo/pull/17#discussion_r2140895570\n\nRequested by @maschad.' \
--assignee maschad
Length of output: 693
@maschad Done! 📌 I’ve opened the follow-up issue and assigned it to you:
Let me know if there’s anything else you’d like tweaked or tracked.
🧬 Code Graph Analysis Results
Relevant snippets from src/service/utils.rs (lines 1-174)
The entire file content is relevant as it defines the core file transfer utilities used in the service layer:
FileTransferErrorenum: Defines error types for file transfer operations, including IO and UTF-8 errors, with implementations forDisplay,Error, and conversions.FileTransferstruct and impl:- Fields:
path(relative path),chunk_size(default 1MB),progress(atomic counter). new(path: &PathBuf) -> Self: Creates a newFileTransferinstance, converting the given path to a relative path based on the current directory.stream_file<S>(&self, stream: &mut S) -> Result<(), Box<dyn Error + Send>>: Asynchronously streams the file contents over the provided async write stream. It sends the relative path length, path bytes, and file size before streaming the file in chunks. Progress is tracked via an atomic counter.
- Fields:
FileReceiverstruct and impl:- Fields:
chunk_size(default 1MB),progress(atomic counter). new() -> Self: Creates a newFileReceiver.receive_file<S>(&self, stream: &mut S) -> Result<String, Box<dyn Error + Send>>: Asynchronously receives a file from the provided async read/write stream. It reads the relative path length, path bytes, and file size, creates necessary directories, writes the file in chunks, and tracks progress. Returns the relative path as aString.
- Fields:
These utilities handle the low-level details of file transfer over asynchronous streams, including error handling, chunked reading/writing, and path management.
Possibly relevant snippets from src/app.rs (lines 65-91)
pub fn new() -> Self {
let mut app = Self {
directory_items: Vec::new(),
all_shared_items: Vec::new(),
directory_cache: HashMap::new(),
selected_index: None,
current_path: std::env::current_dir().unwrap_or_else(|_| PathBuf::from("/")),
connection_state: ConnectionState::Disconnected,
peer_id: PeerId::random(),
connected_peer_id: None,
state: AppState::Share,
is_host: true,
is_loading: false,
listening_addrs: Vec::new(),
items_to_share: HashSet::new(),
items_being_shared: HashSet::new(),
items_to_download: HashSet::new(),
items_being_downloaded: HashSet::new(),
warning: None,
refresh_sender: None,
client: None,
clipboard_success: false,
};
app.populate_directory_items();
app
}This snippet shows the initialization of the main application state, including current directory, peer IDs, connection state, and collections related to sharing and downloading items. It may be relevant for understanding how file transfers are integrated into the app's state.
Possibly relevant snippets from src/service/node.rs (lines 55-126)
pub fn new() -> Result<(Client, impl Stream<Item = Event>, EventLoop, PeerId), Box<dyn Error>> {
// Create a public/private key pair, either random or based on a seed.
// let id_keys = Keypair::generate_ed25519();
// let peer_id = id_keys.public().to_peer_id();
let peer_id = PeerId::random();
let mut swarm = SwarmBuilder::with_new_identity()
.with_tokio()
.with_tcp(
tcp::Config::default(),
noise::Config::new,
yamux::Config::default(),
)?
.with_quic()
.with_behaviour(|key| Behaviour {
kademlia: kad::Behaviour::new(
peer_id,
kad::store::MemoryStore::new(key.public().to_peer_id()),
),
request_response: request_response::cbor::Behaviour::new(
[(JUNKANOO_REQUEST_RESPONSE_PROTOCOL, ProtocolSupport::Full)],
request_response::Config::default(),
),
file_stream: stream::Behaviour::new(),
})?
.with_swarm_config(|c| {
c.with_idle_connection_timeout(Duration::from_secs(CONNECTION_TIMEOUT))
})
.build();
// Set up file transfer protocol listener
let incoming_streams = swarm
.behaviour_mut()
.file_stream
.new_control()
.accept(JUNKANOO_FILE_PROTOCOL)
.unwrap();
tracing::info!(
"Listening for file transfer streams on protocol: {}",
JUNKANOO_FILE_PROTOCOL
);
// Set Kademlia into server mode before adding bootnodes
swarm
.behaviour_mut()
.kademlia
.set_mode(Some(kad::Mode::Server));
// Then add the bootnodes
for peer in &BOOTNODES {
if let Ok(peer_id) = peer.parse() {
swarm
.behaviour_mut()
.kademlia
.add_address(&peer_id, "/dnsaddr/bootstrap.libp2p.io".parse()?);
}
}
let (command_sender, command_receiver) = mpsc::channel(0);
let (event_sender, event_receiver) = mpsc::channel(0);
let local_peer_id = *swarm.local_peer_id();
Ok((
Client {
sender: command_sender,
},
event_receiver,
EventLoop::new(swarm, command_receiver, event_sender, incoming_streams),
local_peer_id,
))
}This snippet shows the creation of the network node, including setting up the libp2p swarm with file transfer protocol support. It is relevant for understanding how the file transfer utilities might be used in the network context.
Possibly relevant snippets from src/service/node.rs (lines 243-259)
fn new(
swarm: Swarm<Behaviour>,
command_receiver: mpsc::Receiver<Command>,
event_sender: mpsc::Sender<Event>,
incoming_streams: stream::IncomingStreams,
) -> Self {
Self {
swarm,
command_receiver,
event_sender,
pending_dial: HashMap::default(),
pending_request_file: HashMap::default(),
pending_request_display: HashMap::default(),
pending_directory_items: HashMap::default(),
incoming_streams,
}
}This snippet shows the initialization of the event loop handling the swarm and incoming streams, which likely includes handling file transfer streams. It is relevant for understanding the integration of file transfer in the node's event loop.
No other snippets appear directly relevant to the file transfer utilities or their usage context.
|
Mustapha Muhammad from Nigeria Sokoto Tambuwal Dogon daji Akalawa Area 850102 |
Summary by CodeRabbit
.gitignoreto exclude macOS system files and test directories.