feat: add NAT-traversing P2P file sharing example - #1220
Conversation
Signed-off-by: Gautam Manchandani <gautammanch@Gautams-MacBook-Air.local>
acul71
left a comment
acul71
left a comment
There was a problem hiding this comment.
Review summary
Thanks for this example — it’s a clear, self-contained demo for #877 (UPnP + friends list + chunked file streaming), and the newsfragment looks good.
Merge readiness: Needs fixes before approval.
Blockers
- Path traversal on receive — remote
filenameis written asreceived_{filename}without sanitization. - Framing correctness —
stream.read(n)may return fewer thannbytes (mplex); header parse needsread_exact. - Resource / integrity bounds — unbounded
fname_len/file_size; incomplete transfers still print success. - Zero-byte send crash —
sent_bytes / filesizeraisesZeroDivisionError. - CLI UX bug — message allows raw multiaddr, but empty friends list
continues before prompting.
Also please address
- Document UPnP-only NAT limitations (no STUN/relay fallback yet) vs issue wording.
- Prefer
Fixes #877in the PR body; expand README with security notes + failure modes. - Consider using / pointing at the existing persistent peerstore APIs for the “peerstore” hack-value story.
Local checks on this branch: make lint / make typecheck / make linux-docs passed. make test had 3 unrelated WebRTC caplog failures (not from this PR).
Inline comments cover the main code issues.
| print(f"📄 Receiving '{filename}' ({file_size} bytes)") | ||
|
|
||
| # Step C: Write to Disk (Prevent overwriting) | ||
| save_name = f"received_{filename}" |
There was a problem hiding this comment.
Critical — path traversal: filename comes from the remote peer and is used directly in received_{filename}.
Please sanitize before writing (at least os.path.basename, reject empty/../separators) and ideally confine writes to a dedicated download directory with a resolve/containment check. Example code is often copy-pasted; automatic inbound writes need to be safe by default.
|
|
||
| try: | ||
| # Step A: Read Filename Length & Filename | ||
| len_bytes = await stream.read(4) |
There was a problem hiding this comment.
Critical — partial reads: stream.read(n) is not guaranteed to return exactly n bytes (mplex docs say it may return fewer). Reading the 4-byte length, filename, and 8-byte size this way can desync the protocol.
Please add a read_exact(stream, n) helper that loops until n bytes are collected (or EOF/error), and use it for all header fields.
| sys.stdout.write(f"\r⏳ Downloading: {percent}%") | ||
| sys.stdout.flush() | ||
|
|
||
| print(f"\n✅ File saved as '{save_name}'") |
There was a problem hiding this comment.
Critical — incomplete transfer reported as success: if the stream ends early (if not chunk: break), this still prints that the file was saved successfully.
Also please bound fname_len / file_size to avoid memory/disk DoS from a malicious peer. Only treat the transfer as successful when received_bytes == file_size (otherwise error and remove/leave a clear partial).
| await stream.write(chunk) | ||
| sent_bytes += len(chunk) | ||
|
|
||
| percent = int((sent_bytes / filesize) * 100) |
There was a problem hiding this comment.
Major — zero-byte send crash: when filesize == 0, sent_bytes / filesize raises ZeroDivisionError. Special-case empty files for the progress display (or skip percent).
| friends = load_friends() | ||
| if not friends: | ||
| print("⚠️ No friends saved. Add one first or enter raw Multiaddr.") | ||
| continue |
There was a problem hiding this comment.
Major — UX bug: the message says the user can enter a raw multiaddr, but with an empty friends list this continues and never prompts.
Allow raw multiaddr entry even when no friends are saved; only require the friends list when resolving by name.
| ): | ||
| print(f" 🏠 Local: {addr_str}") | ||
| else: | ||
| print(f" 🌍 Public: {addr_str} (NAT Traversal Success!)") |
There was a problem hiding this comment.
Major — overstated NAT success: any address that isn’t a simple RFC1918/loopback string match is labeled “NAT Traversal Success!”. That can mis-label CGNAT, some IPv6, or non-UPnP interfaces.
Please soften this messaging and document in the README that this demo is UPnP-only (no STUN/relay fallback yet), which only partially covers #877’s “STUN/relay if needed” ask.
4 participants
What was wrong?
The repository lacked a practical, runnable example demonstrating serverless, peer-to-peer file sharing across NATs with local peer persistence, as requested in the product-innovation bounty.
Issue #877
How was it fixed?
Added a lightweight, serverless file-sharing application under examples/file-share/ that fulfills the exact requirements of the issue.
Summary of approach.
NAT Traversal: Leveraged the library's built-in capabilities by initializing the host with
enable_upnp=True. The CLI visually splits Local IPs and Public IPs on startup to prove hole-punching success.Peer Persistence: Implemented a lightweight peerstore alternative using a local
friends.jsonfile. This acts as a "Contacts List," allowing the user to persist and instantly reconnect to known peers across restarts without needing to rely on external DHT or bootstrap nodes.Stream Protocol: Implemented a custom, chunk-based binary stream protocol
(/file-share/1.0.0)over trio to transfer file metadata (name, size) and safely stream file data directly to disk without bloating RAM.To-Do
Cute Animal Picture
ʕ•ᴥ•ʔ