fix(mount): align late-bound path validation - #34
Conversation
|
Warning Review limit reachedNext included review available in 3 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
![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: f9ca2268e5
ℹ️ 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".
| 'relayfile_mount_remote_suffix="${relayfile_mount_remote_path#/}";', | ||
| 'relayfile_mount_remote_suffix="${relayfile_mount_remote_suffix%/}";', | ||
| 'if [ -z "$relayfile_mount_remote_suffix" ]; then echo "relayfile remote root must not be empty" >&2; exit 2; fi;', |
There was a problem hiding this comment.
When a late-bound argument is --remote-path '//' (or contains more slashes), ${var#/} and ${var%/} each remove only one slash, so the suffix remains nonempty and this check permits the root-equivalent path. The template then starts a daemon against the local mount root instead of visibly rejecting the root-empty argument; strip all leading/trailing slashes or explicitly reject an all-slash value.
Useful? React with 👍 / 👎.
kjgbot
left a comment
kjgbot
left a comment
There was a problem hiding this comment.
Review from sandbox-lead-0823. Ratifying — no concerns. Khaliq owns the merge gate.
Diff is minimal and well-scoped: two additional preflight guards in dynamicMountPreflight — one case */) check enforcing an absolute-path prefix, one if -z check on the normalized suffix — with matching test cases ('github/repos/acme/cloud' → status 2, '/' → status 2). The traversal-segment guard that already existed is preserved, so the three checks now cover the full set: absolute-prefix, no-traversal, non-empty-normalized-suffix. That matches the "static builders already reject through scopedRemoteRoots; make the late-bound template behave identically" framing in the body.
Fail-closed error messages are precise (relayfile remote root must be absolute / must not be empty), match the test assertions, and are distinguishable from the pre-existing traversal error. Exit status 2 is the same status used by the neighbouring guards, so callers that already special-case a preflight failure don't need to grow a new branch.
No release scope tail on this one (as the body notes) — 0.1.7 already carries the sentinel correction from #33, and the normal generated-path path goes through buildRelayfileMountPathArgsShell, so this is parity hardening rather than a prod unblock. All four automated checks (Build & Test, cubic, CodeRabbit, Devin) are green.
Ready from my side.
2 participants
Summary
Keeps the sandbox and Cloud vendored late-bound mount templates on the same validation contract after #31 and #33.
Dynamic template arguments now fail closed when a remote root is non-absolute or normalizes to the empty root. Static builders already reject those forms through scopedRemoteRoots; this makes the public late-bound template path behave identically and matches the Cloud #3143 port.
Validation
Release scope
The critical sentinel correction is already in published 0.1.7 from #33. Normal generated path arguments already pass through buildRelayfileMountPathArgsShell, so this follow-up is validation parity and hardening rather than a blocker for the current production mount recovery.
No version bump, merge, publish, deploy, or image rebuild was performed.
Summary by cubic
Aligns late-bound mount path validation with static builders and Cloud. Previously, dynamic mounts accepted non-absolute or root-empty remote roots; now they fail closed with exit status 2 and clear errors.
Written for commit f9ca226. Summary will update on new commits.