Network: Fallback to NODE_TLS_REJECT_UNAUTHORIZED for cert auth (#6037)

Before we ignored the `NODE_TLS_REJECT_UNAUTHORIZED` environment variable. This variable is used by JetBrains to pass along UI config. Now we respect the environment variable if set. ## Test plan Manually verified with a self-signed cert that enabling the setting bypasses the issue. https://github.com/user-attachments/assets/408ad9a8-a88d-4039-9f1d-c3bd82906091
sourcegraph · Nov 1, 2024 · 3cade20 · 3cade20
1 parent d241897
commit 3cade20
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 3 deletions.
diff --git a/lib/shared/src/configuration.ts b/lib/shared/src/configuration.ts
@@ -43,7 +43,7 @@ export interface NetConfiguration {
     proxy?: {
         endpoint?: string | undefined | null
         cacert?: string | undefined | null
-        skipCertValidation?: boolean | null
+        skipCertValidation?: boolean | undefined | null
     }
     vscode?: string | undefined | null
 }

diff --git a/lib/shared/src/configuration/environment.ts b/lib/shared/src/configuration/environment.ts
@@ -26,6 +26,12 @@ export const cenv = defineEnvBuilder({
      */
     CODY_NODE_DEFAULT_PROXY: proxyStringWithNodeFallback,
 
+    /**
+     * A setting that is similar (and falls back to) Node's NODE_TLS_REJECT_UNAUTHORIZED setting
+     */
+    CODY_NODE_TLS_REJECT_UNAUTHORIZED: (envValue, _) =>
+        bool(envValue) ?? bool(getEnv('NODE_TLS_REJECT_UNAUTHORIZED')),
+
     /**
      * Enables unstable internal testing configuration to be read from settings.json
      */

diff --git a/vscode/src/net/DelegatingAgent.ts b/vscode/src/net/DelegatingAgent.ts
@@ -245,13 +245,24 @@ function normalizeSettings(raw: NetConfiguration): [Error, null] | [null, Normal
         const proxyPath = proxyEndpointString?.startsWith('unix://')
             ? normalizePath(proxyEndpointString.slice(7))
             : null
+
+        // TODO: For all other nullish types like `skipCertValidation` we try to
+        // have the overrides be priority based and only apply if they're set.
+        // Meaning that `skipCertValidation` should have been ?? not || as it
+        // would override the config value with the environment value even if
+        // the config was set. However VSCode's config doesn't give
+        // undefined/null for boolean types and an extra call to config.inspect
+        // is required to see if a value has been set by the user. We don't have
+        // a way of doing that in our ConfigGetter wrapper.
+        const skipCertValidation =
+            raw.proxy?.skipCertValidation || cenv.CODY_NODE_TLS_REJECT_UNAUTHORIZED === false || false
         return [
             null,
             {
                 bypassVSCode:
                     raw.mode?.toLowerCase() === 'bypass' ||
                     (raw.mode?.toLowerCase() !== 'vscode' && (!!proxyServer || !!proxyPath)),
-                skipCertValidation: raw.proxy?.skipCertValidation || false,
+                skipCertValidation,
                 ...caCertConfig,
                 proxyPath,
                 proxyServer,
@@ -309,11 +320,12 @@ async function resolveSettings([error, settings]:
             }
         }
     }
+    const ca = await buildCaCerts(caCert ? [caCert] : null)
     return {
         error: err,
         proxyServer: settings.proxyServer || null,
         proxyPath,
-        ca: await buildCaCerts(caCert ? [caCert] : null),
+        ca,
         skipCertValidation: settings.skipCertValidation,
         bypassVSCode: settings.bypassVSCode,
         vscode: settings.vscode,