Merge pull request #9188 from desktop/schannel-revoke-check-failure

Support disabling certificate revocation checks on Windows

app/src/lib/feature-flag.ts

@@ -116,6 +116,16 @@ export function enableCreateForkFlow(): boolean {
   return true
 }
 
+/**
+ * Whether or not we should attempt to detect the specific curl
+ * error from the WinSSL (schannel) https backend when it fails
+ * to check the revocation details of a certificate due to lacking
+ * CRL distribution points and/or an offiline revocation server.
+ */
+export function enableSchannelCheckRevokeOptOut(): boolean {
+  return enableDevelopmentFeatures()
+}
+
 /**
  * Whether or not to enable support for automatically resolving the
  * system-configured proxy url and passing that to Git.

app/src/lib/git/config.ts

@@ -17,7 +17,24 @@ export function getGlobalConfigValue(
     HOME: string
   }
 ): Promise<string | null> {
-  return getConfigValueInPath(name, null, env)
+  return getConfigValueInPath(name, null, undefined, env)
+}
+
+/**
+ * Look up a global config value by name.
+ *
+ * Treats the returned value as a boolean as per Git's
+ * own definition of a boolean configuration value (i.e.
+ * 0 -> false, "off" -> false, "yes" -> true etc)
+ */
+export async function getGlobalBooleanConfigValue(
+  name: string,
+  env?: {
+    HOME: string
+  }
+): Promise<boolean | null> {
+  const value = await getConfigValueInPath(name, null, 'bool', env)
+  return value === null ? null : value !== 'false'
 }
 
 /** Set the local config value by name. */
@@ -38,9 +55,21 @@ export async function setGlobalConfigValue(
   )
 }
 
+/**
+ * Look up a global config value by name
+ *
+ * @param path The path to execute the `git` command in. If null
+ *             we'll use the global configuration (i.e. --global)
+ *             and execute the Gitt call from the same location that
+ *             GitHub Desktop is installed in.
+ * @param type Canonicalize configuration values according to the
+ *             expected type (i.e. 0 -> false, "on" -> true etc).
+ *             See `--type` documentation in `git config`
+ */
 async function getConfigValueInPath(
   name: string,
   path: string | null,
+  type?: 'bool' | 'int' | 'bool-or-int' | 'path' | 'expiry-date' | 'color',
   env?: {
     HOME: string
   }
@@ -50,6 +79,10 @@ async function getConfigValueInPath(
     flags.push('--global')
   }
 
+  if (type !== undefined) {
+    flags.push('--type', type)
+  }
+
   flags.push(name)
 
   const result = await git(flags, path || __dirname, 'getConfigValueInPath', {

app/src/models/popup.ts

@@ -56,6 +56,7 @@ export enum PopupType {
   PushRejectedDueToMissingWorkflowScope,
   SAMLReauthRequired,
   CreateFork,
+  SChannelNoRevocationCheck,
 }
 
 export type Popup =
@@ -221,3 +222,7 @@ export type Popup =
       repository: RepositoryWithGitHubRepository
       account: Account
     }
+  | {
+      type: PopupType.SChannelNoRevocationCheck
+      url: string
+    }

app/src/ui/app.tsx

@@ -111,6 +111,7 @@ import { WorkflowPushRejectedDialog } from './workflow-push-rejected/workflow-pu
 import { getUncommittedChangesStrategy } from '../models/uncommitted-changes-strategy'
 import { SAMLReauthRequiredDialog } from './saml-reauth-required/saml-reauth-required'
 import { CreateForkDialog } from './forks/create-fork-dialog'
+import { SChannelNoRevocationCheckDialog } from './schannel-no-revocation-check/schannel-no-revocation-check'
 
 const MinuteInMilliseconds = 1000 * 60
 const HourInMilliseconds = MinuteInMilliseconds * 60
@@ -1903,6 +1904,13 @@ export class App extends React.Component<IAppProps, IAppState> {
             account={popup.account}
           />
         )
+      case PopupType.SChannelNoRevocationCheck:
+        return (
+          <SChannelNoRevocationCheckDialog
+            onDismissed={this.onPopupDismissed}
+            url={popup.url}
+          />
+        )
       default:
         return assertNever(popup, `Unknown popup type: ${popup}`)
     }

app/src/ui/dialog/dialog.tsx

@@ -143,9 +143,56 @@ export class Dialog extends React.Component<IDialogProps, IDialogState> {
   private disableClickDismissalTimeoutId: number | null = null
   private disableClickDismissal = false
 
+  /**
+   * Resize observer used for tracking width changes and
+   * refreshing the internal codemirror instance when
+   * they occur
+   */
+  private readonly resizeObserver: ResizeObserver
+  private resizeDebounceId: number | null = null
+
   public constructor(props: IDialogProps) {
     super(props)
     this.state = { isAppearing: true }
+
+    // Observe size changes and let codemirror know
+    // when it needs to refresh.
+    this.resizeObserver = new ResizeObserver(this.scheduleResizeEvent)
+  }
+
+  private scheduleResizeEvent = () => {
+    if (this.resizeDebounceId !== null) {
+      cancelAnimationFrame(this.resizeDebounceId)
+      this.resizeDebounceId = null
+    }
+    this.resizeDebounceId = requestAnimationFrame(this.onResized)
+  }
+
+  /**
+   * Attempt to ensure that the entire dialog is always visible. Chromium
+   * takes care of positioning the dialog when we initially show it but
+   * subsequent resizes of either the dialog (such as when switching tabs
+   * in the preferences dialog) or the Window doesn't affect positioning.
+   */
+  private onResized = () => {
+    if (!this.dialogElement) {
+      return
+    }
+
+    const { offsetTop, offsetHeight } = this.dialogElement
+
+    // Not much we can do if the dialog is bigger than the window
+    if (offsetHeight > window.innerHeight - titleBarHeight) {
+      return
+    }
+
+    const padding = 10
+    const overflow = offsetTop + offsetHeight + padding - window.innerHeight
+
+    if (overflow > 0) {
+      const top = Math.max(titleBarHeight, offsetTop - overflow)
+      this.dialogElement.style.top = `${top}px`
+    }
   }
 
   private clearDismissGraceTimeout() {
@@ -215,6 +262,9 @@ export class Dialog extends React.Component<IDialogProps, IDialogState> {
     this.focusFirstSuitableChild()
 
     window.addEventListener('focus', this.onWindowFocus)
+
+    this.resizeObserver.observe(this.dialogElement)
+    window.addEventListener('resize', this.scheduleResizeEvent)
   }
 
   /**
@@ -375,6 +425,9 @@ export class Dialog extends React.Component<IDialogProps, IDialogState> {
 
     window.removeEventListener('focus', this.onWindowFocus)
     document.removeEventListener('mouseup', this.onDocumentMouseUp)
+
+    this.resizeObserver.disconnect()
+    window.removeEventListener('resize', this.scheduleResizeEvent)
   }
 
   public componentDidUpdate() {

app/src/ui/dispatcher/error-handlers.ts

@@ -18,7 +18,10 @@ import {
 } from '../../models/repository'
 import { getDotComAPIEndpoint } from '../../lib/api'
 import { hasWritePermission } from '../../models/github-repository'
-import { enableCreateForkFlow } from '../../lib/feature-flag'
+import {
+  enableCreateForkFlow,
+  enableSchannelCheckRevokeOptOut,
+} from '../../lib/feature-flag'
 import { RetryActionType } from '../../models/retry-actions'
 
 /** An error which also has a code property. */
@@ -627,6 +630,64 @@ export async function insufficientGitHubRepoPermissions(
   return null
 }
 
+// Example error message (line breaks added):
+//    fatal: unable to access 'https://github.com/desktop/desktop.git/': schannel:
+//    next InitializeSecurityContext failed: Unknown error (0x80092012) - The
+//    revocation function was unable to check revocation for the certificate.
+//
+// We can't trust anything after the `-` since that string might be localized
+//
+// 0x80092012 is CRYPT_E_NO_REVOCATION_CHECK
+// 0x80092013 is CRYPT_E_REVOCATION_OFFLINE
+//
+// See
+// https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-certverifyrevocation
+// https://github.com/curl/curl/blob/fa009cc798f/lib/vtls/schannel.c#L1069-L1070
+// https://github.com/curl/curl/blob/fa009cc798f/lib/strerror.c#L966
+// https://github.com/curl/curl/blob/fa009cc798f/lib/strerror.c#L983
+const fatalSchannelRevocationErrorRe = /^fatal: unable to access '(.*?)': schannel: next InitializeSecurityContext failed: .*? \((0x80092012|0x80092013)\)/m
+
+/**
+ * Attempts to detect whether an error is the result of the
+ * Windows SSL backend's (schannel) failure to contact a
+ * certificate revocation server. This can only occur on Windows
+ * when the `http.sslBackend` is set to `schannel`.
+ */
+export async function schannelUnableToCheckRevocationForCertificate(
+  error: Error,
+  dispatcher: Dispatcher
+) {
+  if (!__WIN32__) {
+    return error
+  }
+
+  if (!enableSchannelCheckRevokeOptOut()) {
+    return error
+  }
+
+  const errorWithMetadata = asErrorWithMetadata(error)
+  const underlyingError =
+    errorWithMetadata === null ? error : errorWithMetadata.underlyingError
+
+  const gitError = asGitError(underlyingError)
+  if (gitError === null) {
+    return error
+  }
+
+  const match = fatalSchannelRevocationErrorRe.exec(gitError.message)
+
+  if (!match) {
+    return error
+  }
+
+  dispatcher.showPopup({
+    type: PopupType.SChannelNoRevocationCheck,
+    url: match[1],
+  })
+
+  return null
+}
+
 /**
  * Extract lines from Git's stderr output starting with the
  * prefix `remote: `. Useful to extract server-specific

app/src/ui/index.tsx

@@ -24,6 +24,7 @@ import {
   refusedWorkflowUpdate,
   samlReauthRequired,
   insufficientGitHubRepoPermissions,
+  schannelUnableToCheckRevocationForCertificate,
 } from './dispatcher'
 import {
   AppStore,
@@ -281,6 +282,7 @@ dispatcher.registerErrorHandler(openShellErrorHandler)
 dispatcher.registerErrorHandler(mergeConflictHandler)
 dispatcher.registerErrorHandler(lfsAttributeMismatchHandler)
 dispatcher.registerErrorHandler(insufficientGitHubRepoPermissions)
+dispatcher.registerErrorHandler(schannelUnableToCheckRevocationForCertificate)
 dispatcher.registerErrorHandler(gitAuthenticationErrorHandler)
 dispatcher.registerErrorHandler(pushNeedsPullHandler)
 dispatcher.registerErrorHandler(samlReauthRequired)

app/src/ui/preferences/advanced.tsx

@@ -4,6 +4,7 @@ import { Checkbox, CheckboxValue } from '../lib/checkbox'
 import { LinkButton } from '../lib/link-button'
 import { SamplesURL } from '../../lib/stats'
 import { UncommittedChangesStrategyKind } from '../../models/uncommitted-changes-strategy'
+import { enableSchannelCheckRevokeOptOut } from '../../lib/feature-flag'
 
 interface IAdvancedPreferencesProps {
   readonly optOutOfUsageTracking: boolean
@@ -11,6 +12,7 @@ interface IAdvancedPreferencesProps {
   readonly confirmDiscardChanges: boolean
   readonly confirmForcePush: boolean
   readonly uncommittedChangesStrategyKind: UncommittedChangesStrategyKind
+  readonly schannelCheckRevoke: boolean | null
   readonly onOptOutofReportingchanged: (checked: boolean) => void
   readonly onConfirmDiscardChangesChanged: (checked: boolean) => void
   readonly onConfirmRepositoryRemovalChanged: (checked: boolean) => void
@@ -18,6 +20,7 @@ interface IAdvancedPreferencesProps {
   readonly onUncommittedChangesStrategyKindChanged: (
     value: UncommittedChangesStrategyKind
   ) => void
+  readonly onSchannelCheckRevokeChanged: (checked: boolean) => void
 }
 
 interface IAdvancedPreferencesState {
@@ -89,6 +92,13 @@ export class Advanced extends React.Component<
     this.props.onUncommittedChangesStrategyKindChanged(value)
   }
 
+  private onSchannelCheckRevokeChanged = (
+    event: React.FormEvent<HTMLInputElement>
+  ) => {
+    const value = event.currentTarget.checked
+    this.props.onSchannelCheckRevokeChanged(value === false)
+  }
+
   private reportDesktopUsageLabel() {
     return (
       <span>
@@ -191,7 +201,39 @@ export class Advanced extends React.Component<
             onChange={this.onReportingOptOutChanged}
           />
         </div>
+        {this.renderGitAdvancedSection()}
       </DialogContent>
     )
   }
+
+  private renderGitAdvancedSection() {
+    if (!__WIN32__) {
+      return
+    }
+
+    if (!enableSchannelCheckRevokeOptOut()) {
+      return
+    }
+
+    // If the user hasn't set `http.schannelCheckRevoke` before we don't
+    // have to show them the preference.
+    if (this.props.schannelCheckRevoke === null) {
+      return
+    }
+
+    return (
+      <div className="git-advanced-section">
+        <h2>Git</h2>
+        <Checkbox
+          label="Disable certificate revocation checks"
+          value={
+            this.props.schannelCheckRevoke
+              ? CheckboxValue.Off
+              : CheckboxValue.On
+          }
+          onChange={this.onSchannelCheckRevokeChanged}
+        />
+      </div>
+    )
+  }
 }

app/src/ui/preferences/preferences.tsx

@@ -12,6 +12,7 @@ import { Dialog, DialogFooter, DialogError } from '../dialog'
 import {
   getGlobalConfigValue,
   setGlobalConfigValue,
+  getGlobalBooleanConfigValue,
 } from '../../lib/git/config'
 import { lookupPreferredEmail } from '../../lib/email'
 import { Shell, getAvailableShells } from '../../lib/shells'
@@ -74,6 +75,8 @@ interface IPreferencesState {
    * choice to delete the lock file.
    */
   readonly existingLockFilePath?: string
+  readonly initialSchannelCheckRevoke: boolean | null
+  readonly schannelCheckRevoke: boolean | null
 }
 
 /** The app-level preferences component. */
@@ -101,6 +104,8 @@ export class Preferences extends React.Component<
       selectedExternalEditor: this.props.selectedExternalEditor,
       availableShells: [],
       selectedShell: this.props.selectedShell,
+      initialSchannelCheckRevoke: null,
+      schannelCheckRevoke: null,
     }
   }
 
@@ -108,6 +113,14 @@ export class Preferences extends React.Component<
     const initialCommitterName = await getGlobalConfigValue('user.name')
     const initialCommitterEmail = await getGlobalConfigValue('user.email')
 
+    // There's no point in us reading http.schannelCheckRevoke on macOS, it's
+    // just a wasted Git process since the option only affects Windows. Besides,
+    // the checkbox will not be visible unless running on Windows so we'll just
+    // default to the default value for lack of anything better.
+    const initialSchannelCheckRevoke = __WIN32__
+      ? await getGlobalBooleanConfigValue('http.schannelCheckRevoke')
+      : null
+
     let committerName = initialCommitterName
     let committerEmail = initialCommitterEmail
 
@@ -151,6 +164,8 @@ export class Preferences extends React.Component<
       uncommittedChangesStrategyKind: this.props.uncommittedChangesStrategyKind,
       availableShells,
       availableEditors,
+      initialSchannelCheckRevoke,
+      schannelCheckRevoke: initialSchannelCheckRevoke,
     })
   }
 
@@ -306,6 +321,8 @@ export class Preferences extends React.Component<
             onUncommittedChangesStrategyKindChanged={
               this.onUncommittedChangesStrategyKindChanged
             }
+            schannelCheckRevoke={this.state.schannelCheckRevoke}
+            onSchannelCheckRevokeChanged={this.onSchannelCheckRevokeChanged}
           />
         )
         break
@@ -380,6 +397,10 @@ export class Preferences extends React.Component<
     )
   }
 
+  private onSchannelCheckRevokeChanged = (value: boolean) => {
+    this.setState({ schannelCheckRevoke: value })
+  }
+
   private renderFooter() {
     const hasDisabledError = this.state.disallowedCharactersMessage != null
 
@@ -414,6 +435,18 @@ export class Preferences extends React.Component<
       if (this.state.committerEmail !== this.state.initialCommitterEmail) {
         await setGlobalConfigValue('user.email', this.state.committerEmail)
       }
+
+      if (
+        this.state.schannelCheckRevoke !==
+          this.state.initialSchannelCheckRevoke &&
+        this.state.schannelCheckRevoke !== null &&
+        __WIN32__
+      ) {
+        await setGlobalConfigValue(
+          'http.schannelCheckRevoke',
+          this.state.schannelCheckRevoke.toString()
+        )
+      }
     } catch (e) {
       if (isConfigFileLockError(e)) {
         const lockFilePath = parseConfigLockFilePathFromError(e.result)

app/src/ui/schannel-no-revocation-check/schannel-no-revocation-check.tsx

@@ -0,0 +1,72 @@
+import * as React from 'react'
+import { Dialog, DialogContent, DialogFooter } from '../dialog'
+import { OkCancelButtonGroup } from '../dialog/ok-cancel-button-group'
+import { setGlobalConfigValue } from '../../lib/git'
+
+interface ISChannelNoRevocationCheckDialogProps {
+  /**
+   * The url that Git failed to access due to schannel being unable
+   * to perform a certificate revocation check, parsed from the
+   * error message.
+   */
+  readonly url: string
+
+  readonly onDismissed: () => void
+}
+interface ISChannelNoRevocationCheckDialogState {
+  readonly loading: boolean
+}
+
+/**
+ * The dialog shown when a Git network operation fails due to the
+ * schannel https backend being unable to perform a certificate
+ * revocation check.
+ *
+ * This can only occur on Windows.
+ */
+export class SChannelNoRevocationCheckDialog extends React.Component<
+  ISChannelNoRevocationCheckDialogProps,
+  ISChannelNoRevocationCheckDialogState
+> {
+  public constructor(props: ISChannelNoRevocationCheckDialogProps) {
+    super(props)
+    this.state = { loading: false }
+  }
+
+  private onDisableRevocationChecks = async () => {
+    this.setState({ loading: true })
+    await setGlobalConfigValue('http.schannelCheckRevoke', 'false')
+    this.props.onDismissed()
+  }
+
+  public render() {
+    return (
+      <Dialog
+        title="Certificate revocation check failed"
+        loading={this.state.loading}
+        onDismissed={this.props.onDismissed}
+        onSubmit={this.onDisableRevocationChecks}
+        type="error"
+      >
+        <DialogContent>
+          <p>
+            Error when attempting to access '{this.props.url}', unable to
+            perform certificate revocation check. See the Event Viewer for more
+            details.
+          </p>
+          <p>
+            This error is common when accessing the Internet through a corporate
+            proxy server or a debugging proxy that performs SSL inspection.
+          </p>
+          <p>
+            Would you like to disable certificate revocation checks? You can
+            change this at any time in options.
+          </p>
+        </DialogContent>
+        <DialogFooter>
+          <OkCancelButtonGroup okButtonText="Disable revocation checks" />
+        </DialogFooter>
+      </Dialog>
+    )
+  }
+}

app/test/unit/git/config-test.ts

@@ -7,6 +7,7 @@ import {
   getGlobalConfigPath,
   getGlobalConfigValue,
   setGlobalConfigValue,
+  getGlobalBooleanConfigValue,
 } from '../../../src/lib/git'
 
 import { mkdirSync } from '../../helpers/temp'
@@ -65,5 +66,57 @@ describe('git/config', () => {
         expect(value).toBe('the correct value')
       })
     })
+
+    describe('getGlobalBooleanConfigValue', () => {
+      const key = 'foo.bar'
+
+      it('treats "false" as false', async () => {
+        await setGlobalConfigValue(key, 'false', env)
+        const value = await getGlobalBooleanConfigValue(key, env)
+        expect(value).toBeFalse()
+      })
+
+      it('treats "off" as false', async () => {
+        await setGlobalConfigValue(key, 'off', env)
+        const value = await getGlobalBooleanConfigValue(key, env)
+        expect(value).toBeFalse()
+      })
+
+      it('treats "no" as false', async () => {
+        await setGlobalConfigValue(key, 'no', env)
+        const value = await getGlobalBooleanConfigValue(key, env)
+        expect(value).toBeFalse()
+      })
+
+      it('treats "0" as false', async () => {
+        await setGlobalConfigValue(key, '0', env)
+        const value = await getGlobalBooleanConfigValue(key, env)
+        expect(value).toBeFalse()
+      })
+
+      it('treats "true" as true', async () => {
+        await setGlobalConfigValue(key, 'true', env)
+        const value = await getGlobalBooleanConfigValue(key, env)
+        expect(value).toBeTrue()
+      })
+
+      it('treats "yes" as true', async () => {
+        await setGlobalConfigValue(key, 'yes', env)
+        const value = await getGlobalBooleanConfigValue(key, env)
+        expect(value).toBeTrue()
+      })
+
+      it('treats "on" as true', async () => {
+        await setGlobalConfigValue(key, 'on', env)
+        const value = await getGlobalBooleanConfigValue(key, env)
+        expect(value).toBeTrue()
+      })
+
+      it('treats "1" as true', async () => {
+        await setGlobalConfigValue(key, '1', env)
+        const value = await getGlobalBooleanConfigValue(key, env)
+        expect(value).toBeTrue()
+      })
+    })
   })
 })