Skip to content

Treat backslash as a path separator in XModem filename validation#11085

Merged
caveman99 merged 2 commits into
developfrom
xmodem-path-separator
Jul 20, 2026
Merged

Treat backslash as a path separator in XModem filename validation#11085
caveman99 merged 2 commits into
developfrom
xmodem-path-separator

Conversation

@caveman99

@caveman99 caveman99 commented Jul 20, 2026

Copy link
Copy Markdown
Member

isValidFilename in src/xmodem.cpp split path components on / only. Backslash separated components were therefore evaluated as a single component, and drive qualified names were not considered at all.

This is portable behaviour on the embedded targets, where the filesystem only recognises /. It matters for the native daemon: FSCommon.h maps FSCom to PortduinoFS under ARCH_PORTDUINO, so filenames reach the host filesystem, and the Windows build added in #11031 runs on a host where both separators are significant.

Changes:

  • Split components with strpbrk(seg, "/\\") so / and \ are both treated as separators.
  • Reject drive qualified names such as C:\dir\file, which the component loop alone would not catch.
  • Extend test/test_xmodem with backslash, mixed separator, and drive qualified cases.

Existing accepted forms are unchanged: absolute paths, subdirectories, and names that merely contain dots (my..file, ...) still pass.

native-windows test_xmodem passes, 5 of 5 cases. Other suites were not run locally.

Summary by CodeRabbit

  • Bug Fixes
    • Strengthened filename validation to reject Windows-style drive-qualified and drive-relative paths (e.g., C:\..., C:/..., c:relative.txt).
    • Improved protection against directory traversal attempts by treating both forward slashes and backslashes as path separators.
    • Updated and expanded the automated test suite with additional negative cases to ensure unsafe filename patterns are rejected while valid colon-containing time-like filenames remain allowed.

isValidFilename split components on forward slash only, so backslash
separated components were not examined individually. Use strpbrk to split
on both, and reject drive qualified names.

The native Windows daemon build maps FSCom to PortduinoFS on the host
filesystem, where both separators are significant.

Adds test coverage for backslash and drive qualified inputs.
@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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: 4e9e63a1-3e25-48e4-a533-5edabc2e347e

📥 Commits

Reviewing files that changed from the base of the PR and between 142780c and 9f1bbd6.

📒 Files selected for processing (2)
  • src/xmodem.cpp
  • test/test_xmodem/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/xmodem.cpp

📝 Walkthrough

Walkthrough

XModem filename validation rejects drive-qualified paths and detects .. traversal using both slash styles. Unity tests cover unsafe Windows-style paths and valid colon-containing filenames.

Changes

XModem filename validation

Layer / File(s) Summary
Filename validation and coverage
src/xmodem.cpp, test/test_xmodem/test_main.cpp
The validator rejects drive-qualified names, recognizes / and \ separators for traversal checks, and adds registered tests for unsafe and valid filename cases.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the primary change: treating backslashes as path separators in XModem filename validation.
Description check ✅ Passed The description is mostly complete with motivation, implementation details, and test results, though it does not follow the repo's attestation checklist template.
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-path-separator

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

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@src/xmodem.cpp`:
- Around line 65-66: Restrict the drive-prefix rejection in the filename
validator to cases where name[0] is an ASCII letter and name[1] is ':',
preserving other colon-containing filenames. In test/test_xmodem/test_main.cpp
lines 33-38, add a positive regression assertion confirming a non-drive colon
filename is accepted.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 29c19157-a223-4601-ac24-38ef1f3d7623

📥 Commits

Reviewing files that changed from the base of the PR and between 19ddadf and 142780c.

📒 Files selected for processing (2)
  • src/xmodem.cpp
  • test/test_xmodem/test_main.cpp

Comment thread src/xmodem.cpp Outdated
Only [A-Za-z] can begin a drive qualifier, so rejecting on the colon alone
also refused legal POSIX filenames such as 1:30pm.txt.
@caveman99
caveman99 merged commit ba8b1b1 into develop Jul 20, 2026
4 of 6 checks passed
@caveman99
caveman99 deleted the xmodem-path-separator branch July 20, 2026 08:46
@github-actions

Copy link
Copy Markdown
Contributor

⚡ 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant