Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bring back unsafe terminal profile paths with a confirmation step #167721

Closed
Tyriar opened this issue Nov 30, 2022 · 4 comments · Fixed by #170193
Closed

Bring back unsafe terminal profile paths with a confirmation step #167721

Tyriar opened this issue Nov 30, 2022 · 4 comments · Fixed by #170193
Assignees
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders on-testplan terminal-profiles
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Nov 30, 2022

The idea is to bring back cygwin and add other shells like cmder/msys2 and any other profiles that may be in "unsafe" paths (ie. not those owned by the user or admin) and present them in the list:

image

Note that they're in the detected section, not the profiles section, so it won't show up unless it's created via the select default profile button. It's not clear to me whether we need a warning icon or warning text here or not, but clicking it would get a notification warning before proceeding which is the main protection.

Code for the screenshot:

		detectedProfiles.set('Cygwin $(warning)', {
			path: [
				'C:\\cygwin64\\bin\\bash.exe',
				'C:\\cygwin\\bin\\bash.exe'
			],
			args: ['--login'],
			isAutoDetected: true
		});
@Tyriar Tyriar added feature-request Request for new features or functionality terminal-profiles labels Nov 30, 2022
@Tyriar Tyriar added this to the Backlog milestone Nov 30, 2022
@meganrogge
Copy link
Contributor

meganrogge commented Nov 30, 2022

IMO, we don't need a visual indicator in that list as the notification is sufficient.

Presumably we'd only ask once per user? Would we remove the entry in the list when the user has answered no to creating with that profile?

@Tyriar
Copy link
Member Author

Tyriar commented Nov 30, 2022

@meganrogge if they click either that entry or the gear icon, a notification would appear and if accepted it would be added to settings to become a proper profile. So no need to cache the answer or anything as this only needs to happen once typically.

@Tyriar
Copy link
Member Author

Tyriar commented Nov 30, 2022

We could also add the msys2 shells, I saw this in feedback we got from from the website:

Not sure if we need the env:

"MSYS2 Bash": {
  "path": "C:\\msys64\\usr\\bin\\bash.exe",
  "args": ["--login", "-i"],
  "env": {
      "MSYSTEM": "MINGW64",
      "CHERE_INVOKING": "1",
      "MSYS2_PATH_TYPE": "inherit"
  }
}

From https://gist.github.com/dhkatz/106891324c9624074a84d11e2691144b

@DRSDavidSoft
Copy link

DRSDavidSoft commented Nov 30, 2022

Awesome, BTW this is not strictly related, but there is an issue with shell profiles that contain a space in the environment variable. I'd super appreciate it if you could take a look and confirm the issue: #164696


Update: nevermind, I see it's already confirmed and has been assigned. Hopefully, this comment won't be marked as offtopic 😅

@Tyriar Tyriar self-assigned this Dec 6, 2022
@Tyriar Tyriar modified the milestones: Backlog, January 2023 Dec 9, 2022
@VSCodeTriageBot VSCodeTriageBot added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Dec 29, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Feb 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders on-testplan terminal-profiles
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants