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.

Sign up for GitHub

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

Inherit environment variables deemed safe by default #36

Merged
merged 2 commits into from
Nov 5, 2024
Merged

Inherit environment variables deemed safe by default #36

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3447575
Inherit environment variables deemed safe by default
jspahrsummers Nov 5, 2024
08b8940
Trim Windows list down based on review
jspahrsummers Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 48 additions & 7 deletions src/client/stdio.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { ChildProcess, spawn } from "node:child_process";
import process from "node:process";
import { ReadBuffer, serializeMessage } from "../shared/stdio.js";
import { JSONRPCMessage } from "../types.js";
import { Transport } from "../shared/transport.js";
Expand All @@ -17,11 +18,55 @@ export type StdioServerParameters = {
/**
* The environment to use when spawning the process.
*
* The environment is NOT inherited from the parent process by default.
* If not specified, the result of getDefaultEnvironment() will be used.
*/
env?: object;
env?: Record<string, string>;
};

/**
* Environment variables to inherit by default, if an environment is not explicitly given.
*/
export const DEFAULT_INHERITED_ENV_VARS =
process.platform === "win32"
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of these are unnecessary, I'd nuke:

ALLUSERSPROFILE, NUMBER_OF_PROCESSORS, OS, PATHEXT, TMP, WINDIR

Copy link
Member Author

Choose a reason for hiding this comment

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

Will definitely defer to you, but here was my reasoning:

  • NUMBER_OF_PROCESSORS: Maybe needed to set reasonable concurrency defaults (e.g., for thread pools)?
  • OS: Maybe needed for compatibility checks?
  • TMP: Seemed like the same thing as TEMP, maybe applications incorrectly use one or the other

Copy link
Contributor

Choose a reason for hiding this comment

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

OS is basically a legacy constant, it's always Windows_NT, and TMP is also like, a legacy env var that shouldn't exist. NUMBER_OF_PROCESSORS you can probably keep but usually people use a platform API like os.cpus().length on node

? [
"APPDATA",
"HOMEDRIVE",
"HOMEPATH",
"LOCALAPPDATA",
"PATH",
"PROCESSOR_ARCHITECTURE",
"SYSTEMDRIVE",
"SYSTEMROOT",
"TEMP",
"USERNAME",
"USERPROFILE",
]
: /* list inspired by the default env inheritance of sudo */
["HOME", "LOGNAME", "PATH", "SHELL", "TERM", "USER"];

/**
* Returns a default environment object including only environment variables deemed safe to inherit.
*/
export function getDefaultEnvironment(): Record<string, string> {
const env: Record<string, string> = {};

for (const key of DEFAULT_INHERITED_ENV_VARS) {
const value = process.env[key];
if (value === undefined) {
continue;
}

if (value.startsWith("()")) {
// Skip functions, which are a security risk.
continue;
}

env[key] = value;
}

return env;
}

/**
* Client transport for stdio: this will connect to a server by spawning a process and communicating with it over stdin/stdout.
*
Expand Down Expand Up @@ -56,11 +101,7 @@ export class StdioClientTransport implements Transport {
this._serverParams.command,
this._serverParams.args ?? [],
{
// The parent process may have sensitive secrets in its env, so don't inherit it automatically.
env:
this._serverParams.env === undefined
? {}
: { ...this._serverParams.env },
env: this._serverParams.env ?? getDefaultEnvironment(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to merge this? something like:

env: { ...this._serverParams.env, ...getDefaultEnvironment() }

Copy link
Member Author

Choose a reason for hiding this comment

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

No, because then there's no way to clobber the default environment. Even if we flipped the order, it'd be very annoying to unset the default keys.

Copy link
Member Author

Choose a reason for hiding this comment

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

I figure it's easy enough for callers to build their env on getDefaultEnvironment() if they want to.

stdio: ["pipe", "pipe", "inherit"],
signal: this._abortController.signal,
},
Expand Down