feat(cli): add `coder open vscode` #11191

mafredri · 2023-12-13T15:37:27Z

This PR adds support for coder open vscode that can be used on a local machine or remote.

On a local machine, the URI is automatically opened via platform specific open methods (open, xdg-open, etc.) and on a remote machine the URI is printed to the console (or if automatic open fails).

As a (maybe overkill?) security precaution, we don't print the API key by default, it has to be requested with --generate-token. The automatic open code-path always includes it, though. Just like the Web UI.

The most unfortunate part of this PR is the path handling (due to Windows). It could be simplified at the loss of convenience, or it could be refactor to be more robust, the latter I don't feel is in scope for this PR, though.

In a future incantation, we may even go as far as SSH into the workspace to expand the path which would resolve a lot of issues -- maybe even allow tab-autocompletion once that's are re-introduced.

Alternatively, in another future improvement, it would be nice if the agent reported the $HOME, this way we'd have a fallback for when ExpandedDirectory is empty.

Fixes #7667

matifali · 2023-12-13T19:30:29Z

Do we need to specify workspace name everytime?
What if we run it from an ssh session that is already connected to the workspace?
In that case we should default the current workspace a user is connected to.
Also thought about making it coder vscode only to make it more concise?
And probably in a follow up we can also have coder gateway

matifali · 2023-12-13T19:38:07Z

Perhaps the above should be behind the verbose flag.

Yes I agree it should be replaced with "Opening vscode desktop in workspace w"

mafredri · 2023-12-14T09:51:49Z

Do we need to specify workspace name everytime? What if we run it from an ssh session that is already connected to the workspace? In that case we should default the current workspace a user is connected to.

This version was intended only for opening from a users local machine. Opening from within the workspace on the local machine is much more complicated, although I agree would be convenient.

Opening from the workplace is a much harder problem since we can’t tell the users machine to execute a command from within the SSH session. The best we can do for now within the workspace is to print the open uri and let the user open it themselves. Or, we can print a URL to the dashboard (solves the issue of providing the token without printing it in the terminal).

If getting the same automatic open experience remotely as locally is a hard requirement. Then I’d like to discuss with @deansheather about security implications and whether or not we should do it.

Also thought about making it coder vscode only to make it more concise? And probably in a follow up we can also have coder gateway

I did consider it, but I’m worried we’ll overload the top level scope with too many commands if we do that. Especially if more open commands are introduced.

matifali · 2023-12-14T10:08:04Z

I did consider it, but I’m worried we’ll overload the top level scope with too many commands if we do that. Especially if more open commands are introduced.

I agree with naming.

For allowing opening from the web terminal, it's just convenient and saves user from having to install coder locally first before using this command.
I think if we can just print the URI its enough and user can click on it to launch the IDE.

deansheather · 2023-12-14T12:24:03Z

VSCode itself doesn't support "local open" in an SSH session outside of the integrated terminal so I don't think we should either. You can't SSH into a server and then run code . to open a native VSCode window locally AFAIK.

There are security issues with adding a reverse shell or support for the workspace to open URLs on the local machine without input as well. A malicious workspace could run custom commands or could abuse custom scheme handlers to coerce other apps on the local system to do malicious actions. Or it could just DoS your local machine by spamming URLs.

matifali

Great Work. I am happy withe functionality and will defer code review to engineers.

johnstcn

LGTM 👍 I think we could probably extract some util / helper functions from the command though, especially around the windows path handling logic.

cli/open.go

deansheather · 2023-12-18T14:15:32Z

cli/open.go

+		// Path starts with a drive letter.
+		return len(path) == 2 || (len(path) >= 4 && path[2] == '\\' && path[3] == '\\')
+	default:
+		return false


I tried /path/to/something, \path\to\something and C:/path/to/something which also both work in VSCode. Defaults to the drive Windows is installed on if there isn't a drive letter.

IDK how paths work in Windows containers either, unsure if they have drive letters or not.

Maybe I'll need to play around with this some more. I was actually planning on testing with agents on Windows today, but it turns out our win template is broken atm. I know at least the "expanded directory" resolves to C:\... via test runners. I suspected / might work in VS Code but out of precaution I wanted to transform them.

Perhaps we can just make everything slash and only have the logic for detecting abs path on Windows.

I added some handling for the cases you mentioned, and relaxed the abs requirement to include /, \. I know there are more exceptions like \\.UNC\ paths and the like, but I don't know if it's worth adding preemptive handling for that.

Actually, I can't seem to make CI happy because Go filepath.IsAbs on windows seems to think differently? https://github.com/coder/coder/actions/runs/7262677715/job/19786347550?pr=11191#step:5:496

(I kept the fallback to filepath vs our impl. on Windows so that we could catch these types of inconsistencies, but I can just remove that if we don't care.)

deansheather

I think it's probably fine if we don't cover every single edge case on Windows, especially since not many of our customers are using Windows workspaces. If we do find issues they can be fixed later

Fixes #7667

github-actions bot assigned mafredri Dec 13, 2023

mafredri force-pushed the mafredri/feat-cli-open branch 9 times, most recently from b4abb31 to 36c7e20 Compare December 13, 2023 15:50

mafredri force-pushed the mafredri/feat-cli-open branch from 0384ac8 to 42c6b03 Compare December 14, 2023 13:46

This comment was marked as outdated.

Sign in to view

mafredri force-pushed the mafredri/feat-cli-open branch 4 times, most recently from 49025a5 to 5d370fc Compare December 15, 2023 17:34

mafredri marked this pull request as ready for review December 15, 2023 17:49

mafredri requested review from johnstcn, deansheather and matifali December 15, 2023 17:56

matifali approved these changes Dec 16, 2023

View reviewed changes

johnstcn approved these changes Dec 18, 2023

View reviewed changes

cli/open.go Outdated Show resolved Hide resolved

cli/open.go Outdated Show resolved Hide resolved

cli/open.go Outdated Show resolved Hide resolved

cli/open.go Outdated Show resolved Hide resolved

deansheather reviewed Dec 18, 2023

View reviewed changes

mafredri force-pushed the mafredri/feat-cli-open branch 2 times, most recently from 7b54988 to cdea111 Compare December 19, 2023 12:08

mafredri force-pushed the mafredri/feat-cli-open branch 3 times, most recently from 3c3564a to b1f10a7 Compare December 19, 2023 12:47

mafredri requested a review from deansheather December 19, 2023 13:16

mafredri force-pushed the mafredri/feat-cli-open branch from df26e52 to 2e9f563 Compare December 19, 2023 13:42

deansheather reviewed Dec 24, 2023

View reviewed changes

github-actions bot added the stale This issue is like stale bread. label Jan 2, 2024

matifali removed the stale This issue is like stale bread. label Jan 2, 2024

mafredri added 18 commits January 2, 2024 18:06

feat(cli): add coder open vscode

c7dba53

Fixes #7667

in-workspace detction, explicit api token generation (for printed URIs)

31e21fc

handle more abs paths

2fd2464

add todo

b0c3dce

fix

bc71c63

win

d438c49

table test

3a69da2

rename

8e8e08d

better path handling

9ee8d8f

win

257c45f

refactor

3fec654

gen

0d647bd

allow more abs paths on windows, transform unix

1202b65

move if to switch

7c3f48b

convert / to \ for abs path check using filepath

747ecad

unixToWindowsPath

caf84c5

remove filepath fallbacks on windows, go abs check is stricter

081a006

yolo, you aint got nothing on me, misspell

d09546c

mafredri force-pushed the mafredri/feat-cli-open branch from 7f04d9d to d09546c Compare January 2, 2024 18:16

mafredri merged commit df3c310 into main Jan 2, 2024

mafredri deleted the mafredri/feat-cli-open branch January 2, 2024 18:46

github-actions bot locked and limited conversation to collaborators Jan 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): add `coder open vscode` #11191

feat(cli): add `coder open vscode` #11191

mafredri commented Dec 13, 2023 •

edited

Loading

matifali commented Dec 13, 2023 •

edited

Loading

matifali commented Dec 13, 2023

mafredri commented Dec 14, 2023

matifali commented Dec 14, 2023

deansheather commented Dec 14, 2023

This comment was marked as outdated.

matifali left a comment

johnstcn left a comment

deansheather Dec 18, 2023

mafredri Dec 18, 2023

mafredri Dec 19, 2023

mafredri Dec 19, 2023 •

edited

Loading

deansheather left a comment

feat(cli): add coder open vscode #11191

feat(cli): add coder open vscode #11191

Conversation

mafredri commented Dec 13, 2023 • edited Loading

matifali commented Dec 13, 2023 • edited Loading

matifali commented Dec 13, 2023

mafredri commented Dec 14, 2023

matifali commented Dec 14, 2023

deansheather commented Dec 14, 2023

This comment was marked as outdated.

matifali left a comment

Choose a reason for hiding this comment

johnstcn left a comment

Choose a reason for hiding this comment

deansheather Dec 18, 2023

Choose a reason for hiding this comment

mafredri Dec 18, 2023

Choose a reason for hiding this comment

mafredri Dec 19, 2023

Choose a reason for hiding this comment

mafredri Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

deansheather left a comment

Choose a reason for hiding this comment

feat(cli): add `coder open vscode` #11191

feat(cli): add `coder open vscode` #11191

mafredri commented Dec 13, 2023 •

edited

Loading

matifali commented Dec 13, 2023 •

edited

Loading

mafredri Dec 19, 2023 •

edited

Loading