Terminate ssh/scp options with "--" separator#13697
Conversation
Append the POSIX end-of-options "--"
github-actions
Bot
added
external
github-actions
Bot
added
unmet-requirements
and removed
needs-triage
There was a problem hiding this comment.
Pull request overview
This PR hardens the Codespaces SSH/SCP command construction by inserting the POSIX end-of-options marker (--) so that destinations and file operands are always treated as positional arguments by ssh/scp, preventing option-injection via crafted values.
Changes:
- Add
--before the SSH destination innewSSHCommand. - Add
--before SCP file operands innewSCPCommand. - Add unit tests asserting the separator is present and positioned correctly.
Show a summary per file
| File | Description |
|---|---|
| internal/codespaces/ssh.go | Inserts -- into constructed ssh and scp argv to terminate option parsing before positional operands. |
| internal/codespaces/ssh_test.go | Adds tests ensuring -- is included for both SSH and SCP command builders. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 3
| cmdArgs = append(cmdArgs, connArgs...) | ||
| cmdArgs = append(cmdArgs, "--") // end of scp options | ||
|
|
There was a problem hiding this comment.
I think Copilot is right. I think we should respect a user-provided --. WDYT @anumol-baby?
There was a problem hiding this comment.
Good call — addressed in latest commit. parseArgs now treats a bare -- as the end-of-options marker: it's dropped, and everything after it becomes the command, so a user-supplied -- is respected.
There was a problem hiding this comment.
nit: I think these new tests could just be one table test
nit: I know this file doesn't already use it, so this isn't a blocking thing, but it would be ideal if we could modernize the tests by using testify.Assert / testify.Require like the rest of the codebase. Happy to tackle that as a follow-up ourselves if no appetite for it 😁
There was a problem hiding this comment.
To keep this PR focused on the security fix, I'd prefer to leave the table-test consolidation (and the testify migration you mentioned) for a follow-up rather than expand the diff here. Happy to open that follow-up, or I can fold it in now if you'd rather — your call.
Thank you so much for the review.
Summary
Closes - https://github.com/github/codespaces/issues/23695
Append the POSIX end-of-options
--marker before the destination innewSSHCommandand before the file arguments innewSCPCommand(internal/codespaces/ssh.go).--is the standard convention to signal "stop parsing options". Adding it here guarantees that the destination/file arguments are always treated as positional, regardless of their contents, and matches common-practice hardening across other CLIs that shell out tossh/scp.Changes
internal/codespaces/ssh.go— insert"--"immediately before the destination innewSSHCommand, and before the file list innewSCPCommand.internal/codespaces/ssh_test.go— add tests covering both separators (TestNewSSHCommandUsesEndOfOptionsSeparator,TestNewSCPCommandUsesEndOfOptionsSeparator).Validation
go test ./internal/codespaces/passesssh -- user@hostandscp -- src dstbehave identically to the pre-change invocations for normal inputs.