Uh oh!
There was an error while loading. Please reload this page.
Inherit environment variables deemed safe by default - #36
Conversation
| * Environment variables to inherit by default, if an environment is not explicitly given. | ||
| */ | ||
| export const DEFAULT_INHERITED_ENV_VARS = | ||
| process.platform === "win32" |
There was a problem hiding this comment.
Some of these are unnecessary, I'd nuke:
ALLUSERSPROFILE, NUMBER_OF_PROCESSORS, OS, PATHEXT, TMP, WINDIR
There was a problem hiding this comment.
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 asTEMP, maybe applications incorrectly use one or the other
There was a problem hiding this comment.
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
| this._serverParams.env === undefined | ||
| ? {} | ||
| : { ...this._serverParams.env }, | ||
| env: this._serverParams.env ?? getDefaultEnvironment(), |
There was a problem hiding this comment.
Do we want to merge this? something like:
env: { ...this._serverParams.env, ...getDefaultEnvironment()}There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I figure it's easy enough for callers to build their env on getDefaultEnvironment() if they want to.
… path classifyEntryRequest grows a needsForward flag (default true) that gates the body-preserving request.clone() it tees off for the legacy leg. isLegacyRequest passes false: it already classifies a clone of the caller's request and never reads forwardRequest, so the second clone buffered one extra body copy per routed POST for nothing. The entry handler call site is unchanged (default true) and the body-readability contract test stays. Follow-up from the #2316 review (r3435103544); closes#36.
Will need to make the same changes in the Python SDK, but let's review this for correctness first.
To do
After merging