Skip to content

Commit

Permalink
fix npm view exploits
Browse files Browse the repository at this point in the history
  • Loading branch information
@aeschli @chrmarti
aeschli authored and chrmarti committed Sep 8, 2023
1 parent 8b617bd commit e7b3397
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 5 deletions.
9 changes: 5 additions & 4 deletions extensions/npm/src/features/packageJSONContribution.ts
View file
Expand Up @@ -252,11 +252,12 @@ export class PackageJSONContribution implements IJSONContribution {
}

private isValidNPMName(name: string): boolean {
// following rules from https://github.com/npm/validate-npm-package-name
if (!name || name.length > 214 || name.match(/^[_.]/)) {
// following rules from https://github.com/npm/validate-npm-package-name,
// leading slash added as additional security measure
if (!name || name.length > 214 || name.match(/^[-_.\s]/)) {
return false;
}
const match = name.match(/^(?:@([^/]+?)[/])?([^/]+?)$/);
const match = name.match(/^(?:@([^/~\s)('!*]+?)[/])?([^/~)('!*\s]+?)$/);
if (match) {
const scope = match[1];
if (scope && encodeURIComponent(scope) !== scope) {
Expand Down Expand Up @@ -284,7 +285,7 @@ export class PackageJSONContribution implements IJSONContribution {

private npmView(npmCommandPath: string, pack: string, resource: Uri | undefined): Promise<ViewPackageInfo | undefined> {
return new Promise((resolve, _reject) => {
const args = ['view', '--json', pack, 'description', 'dist-tags.latest', 'homepage', 'version', 'time'];
const args = ['view', '--json', '--', pack, 'description', 'dist-tags.latest', 'homepage', 'version', 'time'];
const cwd = resource && resource.scheme === 'file' ? dirname(resource.fsPath) : undefined;
cp.execFile(npmCommandPath, args, { cwd }, (error, stdout) => {
if (!error) {
Expand Down
2 changes: 1 addition & 1 deletion extensions/npm/src/npmMain.ts
View file
Expand Up @@ -97,7 +97,7 @@ export async function activate(context: vscode.ExtensionContext): Promise<void>
}

async function getNPMCommandPath(): Promise<string | undefined> {
if (canRunNpmInCurrentWorkspace()) {
if (vscode.workspace.isTrusted && canRunNpmInCurrentWorkspace()) {
try {
return await which(process.platform === 'win32' ? 'npm.cmd' : 'npm');
} catch (e) {
Expand Down

2 comments on commit e7b3397

@TakesTheBiscuit
Copy link

Choose a reason for hiding this comment

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

I am not sure this concept of 'isTrusted' really solves the problem; it just moves the accountability off of the product and into the hands of the user - who frankly has no idea about whether or not a trusted or untrusted workspace can or would be impacted by this sort of vulnerability? Wouldn't be it better to somehow track the usage of VSC by the user and prompt them over a period ('nagging') and educating them into understanding the risks behind 'trusting' a workspace? I'm just saying, it's a single click of a button and you've just undone all of these workarounds.

@BrianJDrake
Copy link

@BrianJDrake BrianJDrake commented on e7b3397 Oct 1, 2023 •
edited

Choose a reason for hiding this comment

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

I am not sure this concept of 'isTrusted' really solves the problem; it just moves the accountability off of the product and into the hands of the user - who frankly has no idea about whether or not a trusted or untrusted workspace can or would be impacted by this sort of vulnerability? Wouldn't be it better to somehow track the usage of VSC by the user and prompt them over a period ('nagging') and educating them into understanding the risks behind 'trusting' a workspace? I'm just saying, it's a single click of a button and you've just undone all of these workarounds.

It’s worse than that: such education is impossible to begin with.

According to the VS Code documentation, the purpose of restricted mode is to prevent code execution. But it should be obvious that viewing information about dependencies doesn’t execute any code, so there is no reason to disable this feature in restricted mode. You can’t educate users about something that doesn’t even make sense to the people who should understand it the most.

Of course, the real problem is that VS Code is messing about with command lines and incomprehensible regular expressions instead of using a proper API. Since VS Code and npm are under common ownership (both owned by Microsoft), surely they can sort this out?

Please sign in to comment.