XModem: reject path-traversal filenames in the transfer handler

[caveman99]

The XModem SOH/STX control frame carries a client-supplied destination filename that was used
directly for filesystem access with no validation:

strncpy(filename, (const char *)xmodemPacket.buffer.bytes, sizeof(filename) - 1);
...
if (FSCom.exists(filename)) FSCom.remove(filename);
file = FSCom.open(filename, FILE_O_WRITE);   // and FILE_O_READ on the transmit path

A .. component lets the filename escape the filesystem root - arbitrary file write, read
(exfiltration), and delete (the CAN control also FSCom.remove(filename)).

Impact by target. On embedded LittleFS this is largely inert: the partition root has no
parent to traverse into. On the Portduino / meshtasticd daemon, FSCom is PortduinoFS,
which maps onto the host filesystem under a mountpoint, so .. traverses out of the mountpoint
into real host paths. That build is where this matters.

Reachability. handlePacket is called only from PhoneAPI::handleToRadio (the
xmodemPacket ToRadio case), i.e. from a local client connection - BLE, USB/serial, or the TCP
API. It is not reachable over the RF mesh. On the daemon the TCP API is network-reachable, so on a
networked meshtasticd an unauthenticated API client can trigger it (lockdown builds already drop
xmodem from unauthorized connections).

Fix. Validate the filename before any FS access: reject empty and any .. path component, and
NAK the transfer. Absolute paths and subdirectories are still accepted - the file manager transfers
them straight from the manifest (getFiles("/", ...)), and PortduinoFS confines them to its
mountpoint - so only traversal out of the root is blocked, not legitimate transfers.

Test (test_xmodem): .., ../secret, /prefs/../../etc/passwd, a/../b are rejected; empty/null
rejected; /prefs/config.proto, dir/sub/file.txt, and a name that merely contains .. (my..file)
are accepted.

native-suite-count is bumped to 34: +1 for the new suite and +1 correcting a pre-existing
off-by-one (the file read 32 against 33 existing suite directories).

Summary by CodeRabbit

• Bug Fixes

  • Blocked unsafe XMODEM filenames containing path traversal components.
  • Invalid, empty, or missing filenames are now rejected during transfers.

• Tests

  • Added coverage for traversal attempts, invalid inputs, and valid relative and absolute filenames.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7cadcb67-8440-4c5b-bd71-56c15ec4a0ee

📥 Commits

Reviewing files that changed from the base of the PR and between f18a8b0 and a491912.

📒 Files selected for processing (4)

• src/xmodem.cpp
• src/xmodem.h
• test/native-suite-count
• test/test_xmodem/test_main.cpp

---

📝 Walkthrough

Walkthrough

Adds XMODEM filename validation to reject null, empty, and .. path components, applies it during packet handling, and adds FSCom-gated Unity coverage for accepted and rejected paths.

Changes

XMODEM filename validation

Layer / File(s)	Summary
Filename validation and receive-path enforcement src/xmodem.h, src/xmodem.cpp	Adds isValidFilename(), validates filenames extracted from initial packets, and sends NAK while aborting invalid transfers.
Filename validation tests test/test_xmodem/test_main.cpp, test/native-suite-count	Adds Unity tests for traversal, empty, null, absolute, relative, and valid embedded-dot filenames, and updates the expected native suite count.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: thebentern

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description explains the fix and tests, but it omits the template's attestations/testing checklist and device coverage fields.	Add the Attestations section with checked testing items and any device/regression notes requested by the template.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: rejecting path-traversal filenames in the XModem transfer handler.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch xmodem-filename-validation

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

⚡ Try this PR in the Web Flasher

Note

Building this pull request… the flash button, badges and supported-board
list will appear here automatically once CI finishes.